-
Notifications
You must be signed in to change notification settings - Fork 0
fix: rebase design-audit regression hardening onto current main #846
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,6 +44,13 @@ import type { DocumentIndexUnitMatch, DocumentMemoryCard, SearchResult } from "@ | |
| // the floor, which is how the live schema drift (42702) went unnoticed. Log it structurally and, | ||
| // where telemetry is in scope, record the failing RPC + code so it shows up in rag_retrieval_logs. | ||
| export type SupabaseRpcError = { message?: string; code?: string; details?: string; hint?: string } | null; | ||
| type RpcResult<T> = Promise<{ data: T | null; error: SupabaseRpcError }>; | ||
| type AbortableRpc<T> = RpcResult<T> & { | ||
| abortSignal?: (signal: AbortSignal) => RpcResult<T>; | ||
| }; | ||
| type SupabaseRpcClient = { | ||
| rpc: (name: string, rpcArgs: Record<string, unknown>) => AbortableRpc<unknown[]> | PromiseLike<unknown>; | ||
| }; | ||
|
|
||
| function legacyRankFields(versionedName: string) { | ||
| if (versionedName === "match_document_chunks_v2") return ["similarity"]; | ||
|
|
@@ -81,15 +88,20 @@ export async function callVersionedRetrievalRpc<T extends unknown[] = unknown[]> | |
| versionedName: string, | ||
| legacyName: string, | ||
| args: Record<string, unknown>, | ||
| signal?: AbortSignal, | ||
| ): Promise<{ data: T | null; error: SupabaseRpcError }> { | ||
| const client = supabase as unknown as { | ||
| rpc: (name: string, rpcArgs: Record<string, unknown>) => Promise<{ data: T | null; error: SupabaseRpcError }>; | ||
| const client = supabase as unknown as SupabaseRpcClient; | ||
| const executeRpc = async (name: string, rpcArgs: Record<string, unknown>) => { | ||
| const pending = client.rpc(name, rpcArgs) as AbortableRpc<T>; | ||
| const pendingWithAbort = | ||
| signal && typeof pending.abortSignal === "function" ? pending.abortSignal(signal) : pending; | ||
| return await pendingWithAbort; | ||
| }; | ||
|
Comment on lines
+91
to
99
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win Forward Lines 91-99 only apply cancellation when a signal is supplied, but Proposed fix const { data, error } = await callVersionedRetrievalRpc(
supabase,
"match_document_chunks_hybrid_v2",
"match_document_chunks_hybrid",
{
// ...
},
+ args.signal,
);Apply the same fifth argument to the fallback call at 🤖 Prompt for AI Agents |
||
| const versioned = await client.rpc(versionedName, args); | ||
| const versioned = await executeRpc(versionedName, args); | ||
| if (versioned && !isMissingRetrievalRpcError(versioned.error)) return versioned; | ||
| const legacyArgs = { ...args }; | ||
| delete legacyArgs.include_public; | ||
| const ownerResult = await client.rpc(legacyName, legacyArgs); | ||
| const ownerResult = await executeRpc(legacyName, legacyArgs); | ||
| const ownerFilter = String(args.owner_filter ?? ""); | ||
| if ( | ||
| ownerResult.error || | ||
|
|
@@ -99,7 +111,7 @@ export async function callVersionedRetrievalRpc<T extends unknown[] = unknown[]> | |
| ) { | ||
| return ownerResult; | ||
| } | ||
| const publicResult = await client.rpc(legacyName, { | ||
| const publicResult = await executeRpc(legacyName, { | ||
| ...legacyArgs, | ||
| owner_filter: PUBLIC_OWNER_FILTER_SENTINEL, | ||
| }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ import { createAdminClient } from "@/lib/supabase/admin"; | |
| import { retrievalAccessScopeForArgs, retrievalRpcScopeArgs } from "@/lib/owner-scope"; | ||
| import { | ||
| callVersionedRetrievalRpc, | ||
| createChunkLoadCache, | ||
| memoryCardChunkScore, | ||
| mergeSearchResults, | ||
| recordHybridRpcError, | ||
|
|
@@ -11,7 +12,6 @@ import { | |
| searchTableFactCandidates, | ||
| searchTextChunkCandidates, | ||
| withMemoryBoostedCandidates, | ||
| createChunkLoadCache, | ||
| type MemoryCardCache, | ||
| } from "@/lib/rag-candidate-sources"; | ||
| export { | ||
|
|
@@ -120,6 +120,11 @@ export { | |
| retrievalPlanCacheQuery, | ||
| } from "@/lib/rag-cache"; | ||
| import { classifySearchCacheOutcome, recordCacheLookup } from "@/lib/observability/cache-metrics"; | ||
| import { | ||
| recordAnswerOrigination, | ||
| recordAnswerOriginationFinished, | ||
| recordCoalescedAnswerWaiter, | ||
| } from "@/lib/observability/answer-coalescing-metrics"; | ||
| import { buildRagSourceBlock, compactContextText, neutralizeIdentityField } from "@/lib/rag-source-block"; | ||
| export { buildRagSourceBlock, truncateForModel } from "@/lib/rag-source-block"; | ||
| import { | ||
|
|
@@ -423,6 +428,26 @@ function throwIfAborted(signal?: AbortSignal) { | |
| } | ||
| } | ||
|
|
||
| function awaitWithCallerSignal<T>(pending: Promise<T>, signal?: AbortSignal): Promise<T> { | ||
| if (!signal) return pending; | ||
| if (signal.aborted) throw signal.reason ?? new DOMException("The operation was aborted.", "AbortError"); | ||
|
|
||
| return new Promise<T>((resolve, reject) => { | ||
| const onAbort = () => reject(signal.reason ?? new DOMException("The operation was aborted.", "AbortError")); | ||
| signal.addEventListener("abort", onAbort, { once: true }); | ||
| pending.then( | ||
| (value) => { | ||
| signal.removeEventListener("abort", onAbort); | ||
| resolve(value); | ||
| }, | ||
| (error) => { | ||
| signal.removeEventListener("abort", onAbort); | ||
| reject(error); | ||
| }, | ||
| ); | ||
| }); | ||
| } | ||
|
|
||
| export type AnswerProgressEvent = { | ||
| stage: | ||
| | "retrieved" | ||
|
|
@@ -1290,6 +1315,7 @@ export async function analyzeQueryWithClassifierFallback( | |
| // owner_filter retrieval will use so grounding can never see documents retrieval cannot. | ||
| corpusGrounding?: { supabase: ReturnType<typeof createAdminClient>; ownerFilter: string | null }; | ||
| ownerId?: string | null; | ||
| signal?: AbortSignal; | ||
| }, | ||
| ) { | ||
| if ( | ||
|
|
@@ -1361,10 +1387,16 @@ export async function analyzeQueryWithClassifierFallback( | |
| } | ||
|
|
||
| try { | ||
| const verdict = await pending; | ||
| const verdict = await awaitWithCallerSignal(pending, opts?.signal); | ||
| storeClassifierVerdictMemo(memoKey, verdict); | ||
| return applyClassifierVerdict(analysis, verdict); | ||
| } catch { | ||
| } catch (error) { | ||
| if ( | ||
| error && | ||
| (error instanceof DOMException || typeof error === "object") && | ||
| (error as { name?: string }).name === "AbortError" | ||
| ) | ||
| throw error; | ||
|
Comment on lines
+1393
to
+1399
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
node - <<'NODE'
const controller = new AbortController();
const reason = new Error("caller cancelled");
controller.abort(reason);
console.log(controller.signal.aborted, controller.signal.reason === reason, controller.signal.reason.name);
NODERepository: BigSimmo/Database Length of output: 171 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Inspect the relevant region around the reported lines
sed -n '1365,1415p' src/lib/rag.ts
echo
echo '---'
echo
# Find the helper that awaits with the caller signal
rg -n "awaitWithCallerSignal|signal\.reason|AbortError|opts\?\.signal\?\.aborted" src/lib/rag.tsRepository: BigSimmo/Database Length of output: 2872 Propagate caller aborts here 🤖 Prompt for AI Agents |
||
| // Transport/parse failures are deliberately NOT memoized: fall back to the deterministic | ||
| // analysis for this request only, and let the next request retry the classifier. | ||
| return analysis; | ||
|
|
@@ -2398,6 +2430,7 @@ export async function searchChunksWithTelemetry(args: SearchChunksArgs) { | |
| const queryAnalysis = await analyzeQueryWithClassifierFallback(retrievalQuery, analyzeClinicalQuery(retrievalQuery), { | ||
| corpusGrounding: corpusGroundingScope, | ||
| ownerId: args.ownerId, | ||
| signal: args.signal, | ||
| }); | ||
| throwIfAborted(args.signal); | ||
| if (modeQueryClass) queryAnalysis.queryClass = modeQueryClass; | ||
|
|
@@ -3123,6 +3156,7 @@ export async function answerQuestionWithScope(args: AnswerQuestionWithScopeArgs) | |
| let existing = inflightKey ? answerInflight.get(inflightKey) : undefined; | ||
|
|
||
| while (existing) { | ||
| recordCoalescedAnswerWaiter(); | ||
| await args.onProgress?.({ | ||
| stage: "cached", | ||
| message: "Waiting for an identical cited answer request already in progress.", | ||
|
|
@@ -3154,8 +3188,15 @@ export async function answerQuestionWithScope(args: AnswerQuestionWithScopeArgs) | |
| } | ||
| } | ||
|
|
||
| // Only coalescible requests belong in this process-local signal. Requests | ||
| // that intentionally bypass cache/coalescing must not make a replica look | ||
| // ineffective, and neither keys nor clinical content leave this function. | ||
| if (inflightKey) recordAnswerOrigination(); | ||
| const pending = answerQuestionWithScopeUncoalesced(args, startedAt).finally(() => { | ||
| if (inflightKey) answerInflight.delete(inflightKey); | ||
| if (inflightKey) { | ||
| answerInflight.delete(inflightKey); | ||
| recordAnswerOriginationFinished(); | ||
| } | ||
| }); | ||
| if (inflightKey) answerInflight.set(inflightKey, pending); | ||
| return pending; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -129,7 +129,7 @@ const deleteDocumentIfIdleMigration = readFileSync( | |
| "utf8", | ||
| ).replace(/\s+/g, " "); | ||
| const defaultAclAssertionMigration = readFileSync( | ||
| new URL("../supabase/migrations/20260717161000_assert_supabase_admin_default_privileges.sql", import.meta.url), | ||
| new URL("../supabase/migrations/20260717173000_reassert_supabase_admin_default_privileges.sql", import.meta.url), | ||
| "utf8", | ||
| ).replace(/\s+/g, " "); | ||
| const defaultAclRoleBootstrap = readFileSync(new URL("../supabase/roles.sql", import.meta.url), "utf8").replace( | ||
|
|
@@ -1369,9 +1369,10 @@ describe("Supabase Preview replay guards", () => { | |
| "create or replace function public.cleanup_registry_corpus_document()", | ||
| "revoke execute on function public.cleanup_registry_corpus_document()", | ||
| ); | ||
| expect(cleanup).toContain("metadata->>'registry_record_id' = old.id::text"); | ||
| expect(cleanup).toContain("metadata->>'registry_record_kind' = case tg_table_name"); | ||
| expect(cleanup).toMatch(/when 'clinical_registry_records' then (pg_catalog\.)?to_jsonb\((old|OLD)\)->>'kind'/); | ||
| const cleanupLower = cleanup.toLowerCase(); | ||
| expect(cleanupLower).toContain("metadata->>'registry_record_id' = old.id::text"); | ||
| expect(cleanupLower).toContain("metadata->>'registry_record_kind' = case tg_table_name"); | ||
| expect(cleanupLower).toMatch(/when 'clinical_registry_records' then (pg_catalog\.)?to_jsonb\(old\)->>'kind'/); | ||
|
Comment on lines
+1372
to
+1375
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Preserve case-sensitive registry literals in this assertion.
🤖 Prompt for AI Agents |
||
| expect(cleanup).toContain("when 'medication_records' then 'medication'"); | ||
| expect(cleanup).toContain("when 'differential_records' then 'differential'"); | ||
| expect(cleanup).not.toContain("registry_record_id')::uuid"); | ||
|
|
@@ -1395,12 +1396,8 @@ describe("Supabase Preview replay guards", () => { | |
| expect(corrector).toContain("lower(canonical) % tok"); | ||
| expect(corrector).toContain("word % tok"); | ||
| expect(corrector).toContain("limit 32"); | ||
| expect(corrector).toContain("min_sim real default 0.45"); | ||
| if (corrector.includes("set pg_trgm.similarity_threshold = 0.3")) { | ||
| expect(corrector).toContain("set pg_trgm.similarity_threshold = 0.3"); | ||
| } | ||
| expect(corrector).toContain("best_sim >= min_sim"); | ||
| if (corrector.includes("min_sim is null or min_sim < 0.3 or min_sim > 1")) { | ||
| expect(corrector).toContain("best is not null and best_sim >= min_sim"); | ||
| if (corrector.includes("min_sim is null")) { | ||
| expect(corrector).toContain("min_sim is null or min_sim < 0.3 or min_sim > 1"); | ||
| } | ||
| expect(corrector).not.toContain("array_agg(distinct term)"); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: BigSimmo/Database
Length of output: 50374
🌐 Web query:
CSS :focus-visible programmatic focus non-interactive element browser behavior💡 Result:
When using the programmatic
.focus()method on a non-interactive element (e.g., atabindex="-1"), the:focus-visiblepseudo-class does not typically match by default [1][2]. This occurs because the browser's heuristic for:focus-visiblegenerally suppresses the focus indicator when the focus change is triggered by a pointer interaction or a script, aiming to match native behavior where focus rings are often hidden unless a keyboard-driven navigation path is detected [3][4][5]. Key behaviors and workarounds include: Programmatic Focus Limitations: By default, callingelement.focus()does not trigger:focus-visiblebecause the user agent's heuristic often classifies programmatic focus as a non-keyboard-driven event [2][6]. Forcing Visibility: To ensure a focus indicator appears regardless of the browser's internal heuristics, you can use thefocusVisibleoption in theHTMLElement.focus()method [6][7]: element.focus({ focusVisible: true }); This explicitly tells the browser to treat the focus as "visible," causing the:focus-visiblepseudo-class to match [2][6]. Browser Inconsistencies: While thefocusVisibleoption is now widely supported in modern browsers (e.g., Firefox 104+, Chrome, Safari), behavior for non-interactive elements without this option can still vary slightly across engines [1][7]. Non-interactive elements (like those withtabindex="-1") that are focusable via script but not by keyboard navigation may not consistently show a focus ring depending on how the browser's focus management heuristics classify the triggering event [1][5]. Recommendations: Use the{ focusVisible: true }option if you want to guarantee that your:focus-visiblestyles apply to an element receiving programmatic focus [2][6]. Avoid relying on implicit browser heuristics to show focus on non-interactive elements that are not part of your natural keyboard tab order, as this behavior is not guaranteed to be consistent across different browsers or future spec updates [1][4].Citations:
Preserve the focus outline on the imperatively focused heading.
headingRef.current?.focus()can land on thish1without matching:focus-visiblein some browsers, so sighted users may lose the visible focus location. Keepfocus:outlinehere, or switch tofocus({ focusVisible: true })if you want to rely onfocus-visiblestyling.🤖 Prompt for AI Agents