[Quality] Repo-wide audit fixes: error containment, drifted forks, silent failures (stacked on #294→#295) - #356
Draft
amal66 wants to merge 10 commits into
Draft
Conversation
|
|
…xtraction WHY THIS MATTERS Two workloads in Mike are expensive and can outlive the HTTP request that started them: DOCX -> PDF conversion (LibreOffice) and tabular-review cell extraction (one LLM call per row). Today both run inline on the request thread, so a closed laptop lid, a dropped connection, or a server restart mid-run silently loses the work — the review grid is left with spinners that never resolve, and a large upload blocks its request on LibreOffice. WHAT IS A DURABLE JOB QUEUE A job queue moves work out of the request/response cycle: the request records WHAT should happen (a small JSON payload in Redis) and returns; a worker process picks the job up, runs it, and retries it with exponential backoff if it fails. "Durable" means the job survives the death of the thing that created it — the queue (BullMQ on Redis) holds the job until a worker finishes it, no matter what happens to the original HTTP request or even the server process (BullMQ re-queues jobs whose worker crashed via its stalled-job detection). The classic hazard of queues is the DOUBLE SUBMIT: a client that reconnects and re-POSTs would enqueue the same work twice. This design pushes correctness into the queue's identity model — every job's id is derived deterministically from the work itself (`convert:<versionId>`, `extract:<reviewId>:<rowId>`), so BullMQ collapses a duplicate submit into the already-in-flight job. Durable STATE lives only in Postgres (documents.status, tabular_cells); jobs re-read that state when they run and skip columns already done, which is what makes retries idempotent. HOW IT WORKS - Both queues are OFF by default and opt-in per deployment: ASYNC_DOCUMENT_CONVERSION / ASYNC_TABULAR_EXTRACTION (default "false"). With the flags off the server never dials Redis — the queue connection is created lazily and only reached via enqueue/startWorkers, so a fresh clone still runs fully synchronously with zero new infrastructure. - lib/queue/: a shared lazy Redis connection (maxRetriesPerRequest: null, which BullMQ's blocking commands require), the two queues with deterministic jobIds + retry/backoff, and runProgress — a Redis pub/sub bridge that carries per-cell progress frames from workers to any HTTP request that is watching. - workers/: conversionWorker (DOCX->PDF off the request thread; conversion failure finalizes the document without a PDF rendition, matching the sync path) and extractionWorker (throws on incomplete extraction so BullMQ retries; after the last retry a permanent-failure handler flips surviving cells to "error" so the grid never shows an eternal spinner). A declarative registry + startWorkers()/stopWorkers() lifecycle, started from index.ts only when a flag is on, with graceful SIGTERM/SIGINT drain. - lib/tabular/: the extraction core factored out of routes/tabular.ts so the synchronous route and the async worker share ONE loop (extractRowColumns). The unit of work is the review ROW — one document, or a folder of source documents extracted together — matching the row model main adopted for folder-grouped reviews. tabular.rows.ts carries the row loaders (loadReviewRows / loadRowDocumentText) that both the routes and the worker need. - POST /:reviewId/generate keeps its exact synchronous behavior by default; with the flag on it enqueues one job per row, subscribes to the review's progress channel BEFORE enqueuing (so a fast worker cannot publish into the void), and forwards the same cell_update SSE frames the sync path emits. A 3-second DB-poll backstop reconciles any missed pub/sub frame, so a dropped message can never hang the stream. A new GET /:reviewId/generate/stream lets a disconnected client reattach to a running generation without re-triggering work. Ported from amal66/mike (upstream-pr/durable-queues, #40) and re-derived against current main: the extraction core is row-based (not document-based) to match the folder-grouped row model (open-legal-products#274) and db pagination (open-legal-products#263) that landed after the original branch, and the moved helper bodies match main's current copies byte-for-byte (multi-document citation prompts, Ollama key exemption). Tests: 510 passing (was 499), including queue jobId determinism, worker idempotency/retry/permanent-failure policy, row extraction core, and the pending-cell targeting used by the reconnectable stream. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…enerate-cell, stale-work reaper, and a frontend that can consume it
WHY THIS MATTERS
The first commit made two workloads durable, but a feature flag is only
real if flipping it on produces a working product. Three gaps stood in
the way. First, ASYNC_DOCUMENT_CONVERSION covered one of the five places
that spawn LibreOffice — project uploads, added versions, replaced
versions and document-to-version copies still blocked their requests for
seconds to minutes. Second, regenerate-cell ran a full LLM extraction
inline even with the extraction queue enabled, and a crash mid-call
stranded the cell in "generating" forever. Third, the frontend had no
way to see async results: nothing polled a "processing" document, and
the reconnectable generate stream had zero callers — enabling the flags
produced spinners that never resolved.
WHAT IS AN ORPHANED TRANSIENT STATE
Transient statuses ("processing", "generating") encode a promise: some
running code will eventually write a terminal state. A crash in the
window between the transient write and the terminal write breaks the
promise, and because nothing else owns the row, the lie persists
forever. The fix has two halves: narrow the windows (hand the work to a
queue that retries and survives restarts) and add an owner of last
resort (a reaper that flips provably-orphaned rows to "error").
HOW IT WORKS
- Conversion queue covers all five LibreOffice call sites. Version flows
pass a per-version pdfKey (renditions of different versions must not
collide on the document-level key) and finalizeDocumentStatus: false —
their document is already "ready", so a rendition failure must not
flip a healthy document to "error"; only the initial-upload flow parks
the document "processing" and lets the worker finalize it. Terminal
conversion jobs are now removed immediately (same rationale as
extraction): replace-file reuses the versionId, and a lingering
completed job record would silently dedupe the re-conversion.
- Regenerate-cell becomes a single-cell job: payload gains columnIndex,
jobId gains a column suffix (extract:<review>:<row>:<col>) so it never
dedupes against a full-row job, and the worker narrows to that one
column. The route keeps its synchronous JSON contract by waiting on
the cell's terminal state (pub/sub + DB-poll backstop); if the wait
budget elapses it answers 202 {status:"generating"} — the job keeps
running and the client catches up through the resume stream. The
disconnect-divergence bug (client marks error, backend later writes
done) is gone: the DB is the only authority.
- Stale-work reaper (lib/maintenance/staleWork.ts, swept at boot + every
10 min): documents "processing" past a 30-minute age gate with no live
conversion job flip to "error"; "generating" cells with no live job
flip to "error" (async mode only — cells have no timestamp column, so
in sync mode a live inline run is indistinguishable from an orphan).
Job existence is the liveness signal, which immediate job removal
makes trustworthy.
- Frontend catch-up: GET /single-documents/:documentId exists so the
client can poll one document instead of refetching the collection;
DocTable polls pending/processing rows every 3s and merges status
changes through the existing update path. The tabular view now aborts
its generate stream on unmount, reconnects once through
GET /generate/stream on a dropped stream, resumes an in-flight run
found at mount (cells still "generating"), and treats regenerate's
202 as "keep the skeleton, tail the stream" instead of an error.
- The GET stream view no longer dials Redis in synchronous deployments
(the subscribe is flag-gated; the DB-poll backstop does the resolving
there), so the no-Redis-by-default invariant holds for every new path.
Tests: backend 523 passing (+13: payload passthrough, per-version pdf
keys, finalize semantics, single-cell narrowing in worker + failure
handler, reaper liveness/age-gate/no-op cases); frontend 174 passing,
tsc and production build clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ice layers
WHY THIS MATTERS
The backend's route files were monoliths — routes/tabular.ts (1,649
lines), routes/documents.ts (1,504), routes/projects.ts (1,139),
routes/user.ts (1,132) — each interleaving HTTP parsing, auth checks,
storage IO, and business logic inline in a dozen unrelated handlers.
In a monolith, every change lands in a giant file where the blast
radius is unclear, business logic can only be exercised through a live
HTTP stack, and a new contributor cannot tell which lines are "the
endpoint" and which are "the feature". For an open-source repo this is
the difference between a drive-by contributor shipping a fix and giving
up: small files with one concern each are reviewable; 1,600-line route
files are not.
WHAT IS A SERVICE LAYER
A service layer separates WHAT the application does from HOW it is
reached. The route (HTTP layer) owns request parsing, validation, and
mapping results onto status codes; the service owns business logic and
data access, takes its database handle as an explicit parameter, returns
typed results (discriminated unions like { ok: false, kind: "not_found" }
instead of writing to `res`), and never touches the HTTP request or
response. That inversion is what makes logic unit-testable (call the
function with a fake db — no server needed), reusable (the async
extraction worker calls the exact same functions the SSE route calls),
and safe to change (the compiler knows every result shape a route must
handle).
HOW IT WORKS
- src/routes/*.ts (11 files, 7,853 lines) is replaced by
src/modules/<domain>/ — chat, project-chat, projects, documents,
tabular, user, workflows, library, downloads, case-law, models —
each a thin <name>.routes.ts plus <name>.service.ts. Large domains
split the service into topic files behind a named-re-export facade
(documents: access/upload/versions/download/edits; projects:
crud/folders/documents/chats; user: profile/mfa/apiKeys/mcp/account/
export; tabular: reviews/rows/extract/extractRow/generate/
generateStream/chats) so intra-module helpers cannot leak.
- lib/tabular/* (from the durable-queues change this builds on) moves
into modules/tabular/ — the domain's extraction core, row loaders and
route layer now live together; src/lib/ keeps only cross-domain
infrastructure (storage, llm, chat, queue, access...).
- Streaming endpoints keep their SSE loops in the routes file; only
their non-streaming prepare/persist logic moved into services —
streaming lifetime and client-abort handling are HTTP concerns.
- Pure motion, verified three ways: the endpoint inventory
(method+path multiset, 67 endpoints) is byte-identical before and
after; tsc is clean; the full suite — 510 tests, including the 11
route-level integration suites that exercise the real express app —
passes unchanged. Handler bodies moved verbatim; the only rewrites
are the mechanical seam (res.status(...) inside moved code became
typed returns mapped back to the identical status/JSON in the route).
- DRY within domains only: helpers duplicated across handlers in the
same domain (shared_with normalization in projects, the doc-access
guard sequence in documents, findSystemWorkflow) now have one copy in
their service; similar-but-not-identical code was left alone rather
than force-merged.
- Zero new dependencies. No logging framework, no validation framework,
no observability hooks — organization only, so the diff is reviewable
as motion and each future concern can be its own decision.
Re-derived against this branch's code from the fork's service-layer
refactor (#42, running in the amal66 fork), whose module
boundaries and routes/service contract this follows; the fork's
pino/OTel/zod adoption was deliberately NOT ported to keep this
dependency-free pure motion.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…trail
Express 4 does not forward async handler rejections, and app.ts had no
terminal error middleware — so any throw in the 96 unwrapped async handlers
meant an unhandled rejection and a socket held open until client timeout.
Only the workflows router defended itself. Its asyncRoute wrapper is now a
shared middleware (middleware/asyncRoute.ts) applied to the un-modularized
routes, and app.ts gained one terminal handler. The handler honors
body-parser's 4xx statuses (err.status) because express.json() errors used
to be mapped to 400/413 by Express's default handler; a blanket 500 would
have regressed malformed-JSON requests.
Word chat was the only chat surface that never called recordChatTurn — every
Word turn, including tracked-change edits, was invisible to the audit page.
Cloud-mode turns now audit with surface "word" (recordChatTurn gained an
optional surface override; the history page's filter learned the label).
Local-storage mode deliberately writes NO audit row: it is the user's
explicit never-persist-server-side choice, and audit rows carry
content-derived titles.
Also fixed in the leftover route files, where errors were being silently
converted to success or not-found:
- quickActions: sort_order validation was unreachable for non-integers
("3", 1.5, NaN were coerced to 0 with a 201); DELETE returned 204 with no
row matched; DB failures on workflow lookup read as 404 (now 500, via the
now-exported resolveWorkflowAccess instead of a drifted private copy).
- workflowAddons: the import response hand-rebuilt the workflow shape and
had drifted from GET /workflows/:id on four fields — now uses the exported
withDatabaseWorkflow.
- audit: the route's private project-access helper duplicated
lib/access.listAccessibleProjectIds and swallowed both query errors,
silently narrowing the audit view; CSV export could never show display
names because it forced resolution off and then read the resolved field.
- sourceDocuments: raw upstream error bodies were returned as 502s
unredacted; now safeErrorMessage, with credential failures mapped to 400.
- wordChat: raw error objects logged (now safeErrorLog), and an errored
first turn left the chat untitled/stale because updateChatActivity only
ran on success/abort paths.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ed into bugs
The per-domain modules were built by copying rather than sharing, and the
copies were drifting independently. Two of those drifts were live bugs:
- Document rename returned HTTP 200 with a synthesized new name even when
the rename failed: the real rename lives on document_versions.filename,
and BOTH the projects and library copies never destructured that update's
error. An RLS denial or null current_version_id showed the UI a rename
that never persisted. Both now check the error, 404 on a versionless doc,
and read the row back instead of synthesizing it.
- The projects upload path was a ~130-line fork of createDocumentFromUpload
and had already dropped three response fields the shared pipeline sets.
Deleted; the shared pipeline gained a surface param so project uploads
still audit as surface "project".
Consolidations (each was implemented 2-8 times with local variations):
suffix validation -> lib/documentTypes.parseAllowedSuffix (8 sites),
countPdfPages -> lib/pdfjs (2 verbatim copies), version-file cleanup ->
lib/storage.deleteVersionFilesForDocuments (3 near-verbatim copies),
project-chat's inline SSE setup -> the existing lib/chat openAssistantSse.
Correctness in the same files:
- tabular: review DELETE, chat delete, and chat rename returned success
without matching a row (a collaborator 'deleting' a shared review got 204
for a no-op) — now .select("id") + 404; the chat SSE handler fetched user
model settings (a profile read + key decrypt) twice per new chat; one
call site built a second Supabase client by omitting db.
- folder moves: the ancestor walk that blocks moving a folder into its own
descendant had no cycle guard in EITHER module, so two interleaved moves
could persist a cycle and any later walk spun forever holding a request
slot. Also reordered tabular's regenerate to clear-pending -> enqueue ->
conditionally mark generating, closing the window where the stale-work
sweep saw a 'generating' cell with no job and flipped it to error.
- user module's errorMessage() stringified whole PostgREST error objects
into client-facing detail text with no redaction; it now redacts and
drops the JSON.stringify fallback.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…d provider drift Five independent lib-level bugs, each user-visible: - CourtListener bulk->API citation fallback merged results POSITIONALLY (fallbackRows.shift() per slot), assuming one API row per submitted citation. The API returns one row per citation it detects in the text, so a 0- or 2-row response shifted every later verdict onto the wrong citation. The merge is now keyed by normalized citation string. - crypto.scryptSync ran on every encrypt/decrypt (~40ms each, measured); getUserModelSettings decrypts up to 5 keys per chat request, i.e. ~200ms of blocked event loop per request. Derived keys are memoized per (secret, salt) — the secret is constant for the process. - Ollama's abort threw a plain Error the stream classifier doesn't recognize, so a user pressing stop had a red error event persisted into their chat instead of a clean partial save. It now throws the same AbortError shape as the other three adapters. - OpenAI/Ollama never cancelled their response readers on throw paths, leaking the undici socket until GC (gemini already did this right). - generateDocx's persistence tail was a drifted copy of persistGeneratedFile: generated .docx files were the only type that never got a PDF rendition. The shared helper now accepts docx and the copy is gone (it also fixes a byteOffset-ignoring buffer slice). Drift/hygiene in the same layer: - CLAUDE_API_KEY: accepted as an env fallback by the key-status endpoint but not by the Claude adapter — the UI showed a green key, then requests failed "not configured". The adapter now mirrors the fallback. - Overview RPC builders: projects normalized the email before the case-sensitive shared_with containment check; tabular and workflows passed it raw (masked only by middleware lowercasing). All normalized. - Raw LLM stream logging (privileged client document text, for a legal product) is hard-disabled under NODE_ENV=production and its env vars are documented with an explicit warning. - One lib/log.ts replaces six byte-identical devLog copies and adds an always-on redacting logError; lib/supabase exports the Db type 22 files redeclare. - Stale-work sweep bounded (limit 500) with per-row batched job lookups. - .env.example: 31 vars were read in code but undocumented, including four secrets and all 12 RATE_LIMIT_* knobs — all documented now. - Dead code: docxTrackedChanges' unreferenced _internal test exports (and with them the fast-diff dependency), logRawOpinionPayload, an unused module-load SYSTEM_PROMPT build, mcp sha256Base64Url. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… matters
Backend tests (12.9k LOC) were excluded from tsc and vitest strips types
without checking them, so they were never type-checked at all — 41 latent
type errors had accumulated (wrong-arity stubs whose tuple lookups could
never be checked, implicit-any rpc rows, node16 import specifiers). New
tsconfig.test.json + a typecheck:test script wired into CI; all 41 fixed.
The integration suites each hand-rolled the same 72-line Supabase
query-builder mock — three copies byte-identical down to the indentation —
plus six identical requireAuth mocks. Extracted to __tests__/helpers/
(-195 lines net across the three suites refactored; the divergent forks in
other suites are left for a follow-up). Test expectation updates for this
branch's behavior changes (tabular delete 404s, audit access via
lib/access) ride along here.
Coverage measured only src/lib/** — routes, modules, workers and middleware
could drop to 0% with CI green. Scope widened to src/** and the ratchet
floors RAISED from the stale 23/17/23/23 to 35/29/37/36 (~2 points under
current reality; the tree had gained tests since the floors were set).
docs/testing-coverage.md updated to match.
e2e: shared helpers.ts (selectClaudeModel and the PDF fixture path were
copied per spec), a default expect.timeout of 10s, and the five swallowed
networkidle waits replaced with element-visibility waits — networkidle
against an app with SSE/polling never settles, and .catch(() => {}) made
the following assertions race instead of fail.
CI: the generated-workflows drift check diffed a landing/ output that does
not exist in this repo (the generator's write was existsSync-guarded, so
the path silently matched nothing) — removed from both the workflow and
the generator. package.json also drops the fast-diff dependency orphaned
by the previous commit (lockfiles regenerated).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…fecycles Five upload endpoints threw new Error(await response.text()) instead of MikeApiError, so isMfaRequiredError was unconditionally false for them — a 403 mfa_verification_required on upload showed the user raw JSON instead of opening the MFA popup. All seven copy-pasted multipart blocks now go through one apiUploadRequest helper with the real error contract, and a regression test pins the MFA path. downloadDocumentsZip had the same defect. The SSE wire format was parsed by three hand-rolled inline loops that had already drifted: only one flushed the decoder and trailing buffer on stream close, so the other two silently dropped the final frame whenever the stream ended without a trailing newline; one swallowed every malformed frame in a bare catch. Replaced with one readSseFrames async generator (CRLF handling, decoder flush, [DONE] semantics, reader.cancel on early exit) with 10 transport tests. The 12 existing useAssistantChat SSE tests pass unchanged. Stream lifecycles had no ownership discipline: useAssistantChat never aborted on unmount and a second send overwrote the shared AbortController and events ref while the first loop kept appending — two responses could interleave into one message. TRChatPanel's 16ms drip interval and reader survived unmount (the panel is conditionally mounted), and loading another chat let old frames rewrite the new chat's messages. Both now use a generation counter that stales out superseded turns, abort the previous stream on entry, and clean up on unmount. Auth/profile context values were rebuilt every render, re-rendering every consumer on every provider render. Both are memoized now, so a consumer re-renders when the value it reads actually changes. Dead code: useGenerateChatTitle (titles arrive via the chat_title SSE frame), five unreferenced mikeApi endpoints, eight needlessly-exported types. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Effect and updater misuse that produced real misbehavior: - AskInputPopup ran submit() from a useEffect with NO dependency array, guarded only by a flag — a double network turn under StrictMode. The submit now fires from the interaction handlers, with a pendingSkipId threaded through so skipping the last item serializes as skipped rather than as an empty answer. - The project chat page called three sibling setters from INSIDE its setTabs updater (updaters must be pure; StrictMode double-invokes them), fetched the project twice on every mount because its mutation-signature guard was always truthy, and carried a reloadingDocIds map that was only ever deleted from — the whole doc-reload wiring was dead and is gone. - PdfView cleared shared DOM/refs then appended across per-page awaits with six ungated call sites — two overlapping renders (rapid zoom) interleaved pages and corrupted the highlight index. Renders now carry a generation stamp and cancel the previous pdfjs render task. - NewTRModal snapshotted fixedProjectDocs into state on open; the parent passes [] until the project loads, so opening early left the picker permanently empty. The list is now derived from the prop. - selectAllMatching in all three pagination hooks had try/finally with no catch and every call site void'ed it — a failed ids fetch was an unhandled rejection and a checkbox that silently did nothing. - Sidebar persistence read one state but depended on another, storing a value one toggle stale. TableRow rendered a div with onClick and no keyboard affordance — every row-based screen (19 usages) was keyboard-unreachable. Fixed once in the primitive: role=button, tabIndex, Enter/Space, with a target check so nested controls don't double-fire. Dead code verified and removed: ProjectPickerModal, AddColumnModal's never-passed editing props and their unreachable branches, unused icon and citation-helper exports (three list entries turned out to be live internal helpers — un-exported instead of deleted). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…I sharing Three user-facing bugs in the tracked-edit flow: - mutationApplied was set AFTER the context.sync() that executes the queued insertText, so a post-mutation sync failure reported "Word couldn't apply this change" for an edit that IS in the document — and the user re-applied it. The flag now flips before the sync, and that failure mode reports applied-but-unmanaged instead of error. - Clicking View on an edit the user had already accepted in Word's Review tab DELETED the bookmark anchor and demoted the card to historical — a read-only action destroying data. Reveal is now non-destructive; the resolve/release path keeps sole ownership of deletion. A new e2e test pins this (verified red with the old behavior). - A failed OfficeRuntime.storage read left chat storage at the "cloud" initial value — silently inverting an explicit "local, never persist my chats server-side" privacy choice. It now fails closed to local: a wrong guess of local is recoverable, a wrong guess of cloud is not. Shared-UI drift, authored in rather than accumulated (the tokens never matched the frontend, the catalog shipped incomplete): - The webpack define was a fourth copy of the default model id, drifting from modelCatalog.ts's DEFAULT_MODEL_ID the moment either moved; it is a deployment override only now, empty by default. - Four design tokens differed from the frontend under identical names; the composer lost its max-height cap and its accessible button names. - mike-icon was a byte-identical 320-line copy — now an @mike/* alias like the rest of the shared layer; the dead, drifted button.tsx fork is gone. (input/tab-pill/toggle-switch stay vendored: their diffs are not purely mechanical, and aliasing them would drag the web app's @/ convention into the add-in build.) - CI path filter claimed the add-in was self-contained, but the bundle includes frontend/src/shared/ui — shared-UI changes now trigger the add-in checks. The three e2e-live scripts got npm entries and typecheck coverage so they stop rotting invisibly. Full add-in e2e suite: 149/149 on chromium and webkit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
amal66
force-pushed
the
olp-pr/code-quality
branch
from
August 21, 2026 18:27
1283c6b to
1b03c6f
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| pdfKey, | ||
| pdfBuf.buffer.slice( | ||
| pdfBuf.byteOffset, | ||
| pdfBuf.byteOffset + pdfBuf.byteLength, |
| } catch (err) { | ||
| console.error( | ||
| `[versions/copy] Office→PDF conversion failed for ${filename}:`, | ||
| err, |
| console.error("[versions/upload] storage write failed", e); | ||
| return { ok: false, detail: "Failed to upload new version." }; | ||
| } | ||
|
|
| db: Db, | ||
| ): Promise<{ ok: true; version: unknown } | { ok: false; detail: string }> { | ||
| const { userId, documentId, versionId, file, suffix, target } = params; | ||
|
|
Comment on lines
+279
to
+289
| return html | ||
| .replace( | ||
| /<h([1-6])[^>]*>(.*?)<\/h\1>/gi, | ||
| (_, l, t) => "#".repeat(Number(l)) + " " + t + "\n\n", | ||
| ) | ||
| .replace(/<strong[^>]*>(.*?)<\/strong>/gi, "**$1**") | ||
| .replace(/<li[^>]*>(.*?)<\/li>/gi, "- $1\n") | ||
| .replace(/<p[^>]*>(.*?)<\/p>/gi, "$1\n\n") | ||
| .replace(/<[^>]+>/g, "") | ||
| .replace(/ /g, " ") | ||
| .replace(/&/g, "&") |
Comment on lines
+279
to
+287
| return html | ||
| .replace( | ||
| /<h([1-6])[^>]*>(.*?)<\/h\1>/gi, | ||
| (_, l, t) => "#".repeat(Number(l)) + " " + t + "\n\n", | ||
| ) | ||
| .replace(/<strong[^>]*>(.*?)<\/strong>/gi, "**$1**") | ||
| .replace(/<li[^>]*>(.*?)<\/li>/gi, "- $1\n") | ||
| .replace(/<p[^>]*>(.*?)<\/p>/gi, "$1\n\n") | ||
| .replace(/<[^>]+>/g, "") |
Comment on lines
+42
to
+46
| }) { | ||
| return ( | ||
| process.env.API_PUBLIC_URL || | ||
| process.env.BACKEND_URL || | ||
| `${req.protocol}://${req.get("host")}` |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Code quality follow-up: repo-wide audit fixes (stacked on #294 → #295)
A seven-area audit of the codebase (backend modules, backend lib/workers, remaining route files, frontend data layer, frontend components, Word add-in + shared UI, cross-cutting infra) surfaced ~100 findings. This PR lands the S/M-effort, high-confidence fixes — real bugs, silent-failure paths, drifted copy-paste forks, and missing safety infrastructure. Net −904 lines (+2,350/−3,254 across 123 files).
Stacked on #294 (durable queues) → #295 (service layers). Merge those first; this PR's diff includes their commits until they land.
What's fixed
Backend — error containment and route correctness
asyncRoute+routerErrorHandlernow wraps them, with a terminal handler inapp.tsthat honors body-parser 4xx statuses (so malformed JSON stays 400).recordChatTurn. Cloud-mode turns now audit with surface"word"(history page filter included). Local-mode turns deliberately write no audit row: local storage is the user's explicit "never persist server-side" choice, and audit rows carry content-derived titles.sort_ordervalidation was unreachable for non-integers (silently coerced to 0); DB errors reported as 404 "not found" in quick-actions/add-ons; audit view silently narrowed to own-events on a failed project lookup; CSV export could never show display names.sourceDocumentsreturned raw upstream error bodies as 502 (now redacted viasafeErrorMessage, credential failures map to 400).Backend — deduplication that had already caused bugs
surfaceparam.countPdfPages(×2), storage cleanup (×3),devLog(×6), SSE setup in project-chat — all consolidated intolib/.Backend lib — correctness
scryptSyncran on every encrypt/decrypt (~40ms of blocked event loop each; ~200ms per chat request). Derived keys are now memoized per (secret, salt)..docxfiles were the only type that never got a PDF rendition (drifted fork ofpersistGeneratedFile).pending, enqueues, then conditionally promotes — and the sweep is bounded (limit 500) with batched job lookups.CLAUDE_API_KEYfallback drift (key showed green in the UI, then "not configured" at call time); overview builders' email normalization drift (tabular + workflows now match projects); raw LLM payload logging (privileged client content) is hard-disabled in production and documented; 31 undocumented env vars added to.env.example(incl. 4 secrets).Frontend
Errors instead ofMikeApiError— a 403mfa_verification_requiredon upload never opened the MFA popup (regression test added). Seven multipart blocks collapsed onto oneapiUploadRequest.readSseFramesgenerator (10 transport tests).useAssistantChatandTRChatPanelhad no unmount abort and no generation guard — two concurrent turns could interleave into one message; the drip interval survived unmount. Both guarded now.error+retrywithprofile: null.setTabsupdater, double project fetch per chat-page mount, stale sidebar persistence, StrictMode double-submit in AskInputPopup, NewTRModal's empty-picker race, unhandledselectAllMatchingrejections (×3 hooks), keyboard-inaccessible table rows (fixed once inTablePrimitive, covers 19 usages), context values rebuilt every render (×4 providers), dead-code sweep (~600 lines incl. two whole files).claude-haiku-4-5, lite models) were silently rewritten to the default byuseSelectedModel— allowlist now covers them.Word add-in
mutationAppliedset after the sync that executes the mutation).mike-iconfork replaced with an@mike/*alias, deadbutton.tsxfork deleted, e2e-live scripts wired into npm scripts + typecheck.Tests & CI
tsconfig.test.json+ CI step; 41 latent type errors in test files fixed.__tests__/helpers/(−195 lines).src/lib/**to all ofsrc/**; ratchet floors raised 23/17/23/23 → 35/29/37/36 (the old floors were stale).expect.timeoutdefault, five swallowednetworkidlewaits replaced with element waits.landing/drift-check path removed; add-in workflow path filter now includes the frontend shared-UI files it actually bundles.Verification
tsc+tsc -p tsconfig.test.jsonclean; 658 tests pass.tscclean, 351+ tests pass (new: SSE transport ×10, MFA-upload regression), production build green.Replication steps
Base case (bugs on the base branch, pick any):
olp-pr/service-layers, call the chatverify_citationstool with a citation string CourtListener's/citation-lookup/splits into two rows — every subsequent citation's verdict shifts by one.This PR: repeat each — (1) citations stay aligned (keyed merge), (2) the MFA popup opens (regression test
mikeApi.test.ts"mfa_verification_required upload"), (3) View is read-only (e2e test "View never deletes the bookmark…").Tradeoffs / design decisions
user.shared.errorMessagekeeps PostgREST detail text (now redacted) rather than flattening to a generic message — its detail strings are load-bearing in tests and UI.updated_aton cells); an age-gated sweep remains a follow-up.packages/contractsshared types (SSE event contract is authored 3× with real drift), tabular service-layer adoption, wordChat modularization, ChatView unification,usePaginatedListextraction, zod env config at boot, backend/add-in ESLint.🤖 Generated with Claude Code