Repository navigation
fix: audit followups — cleanup cron, request hygiene, SQL pushdown, module splits (v0.2.10) - #37
Merged
Merged
Conversation
…quest hygiene
Why: the audit found the scheduled cleanup handler had no cron trigger, so
expired messages and R2 attachments accumulated forever; every authenticated
request performed four D1 round-trips; and the cleanup query was unbounded
and unindexed. CORS, rate limiting, and stale polling requests had similar
correctness gaps at the request boundary.
Constraints / tradeoffs:
- Migration 0020 adds the mail_messages(expires_at, id) index the batched
cleanup scan relies on; applied locally, remote runs during deploy.
- The historical duplicate migration number 0013 is grandfathered in
scripts/check-migration-numbering.mjs because renaming would desync
wrangler d1 migrations state on already-applied environments.
- Session touch is throttled to once per minute; device listings accept
up to one minute of last-seen drift in exchange for two fewer D1 ops
per request.
- listExpired grew an optional { limit } argument so diagnostics callers
keep the historical unbounded semantics.
- OAuth routes now sit behind the rate limiter; local dev CORS origins
are only honored when ENVIRONMENT=local.
- refreshMessages aborts superseded in-flight requests; aborts are treated
as stale, not user-facing errors.
Verification:
- pnpm lint / typecheck / test: all green (19+131+331)
- pnpm build: wrangler dry-run + bundle budget check pass
- wrangler d1 migrations apply DB --local: 0020 applied
- node scripts/check-migration-numbering.mjs: pass (21 files)
Risks: hourly cron load on D1 is bounded by 500-row batches; production
CORS now rejects localhost origins previously allowed unconditionally.
Rejected alternatives: renaming the duplicate 0013 migrations (state
desync); pushing announcement filtering into SQL in this change (larger
blast radius, separate PR).
Why: listPage/listFeatured/summary each did SELECT * over the whole announcements table and re-filtered in JS, and /api/announcements fans out to all three per request. Table growth turns every board render into a full scan plus redundant JS work. Constraints / tradeoffs: - The SQL WHERE fragments must mirror isAnnouncementVisible and matchesAnnouncementFilters branch-for-branch, including the subtle manage+admin bypass of the archived exclusion. The JS helpers in src/shared/announcements.ts stay the semantic source of truth and still power the in-memory store. - Keyword search uses instr() with %, _, and \ escaped so user input cannot widen the pattern; this approximates the JS substring match over title/summary/tags. - summary() groups the derived status via a SQL CASE with the same start/end predicates as resolveAnnouncementStatus. - ISO-8601 text ordering equals chronological ordering in SQLite, which all date columns already rely on elsewhere. Verification: - New tests/announcements-sql.test.ts (19 cases) runs the real D1 store statements through node:sqlite and diffs listPage/listFeatured/ summary results against the pure JS helpers over identical seed rows, covering visibility scopes, all five statuses, keyword/type/time filters, pagination disjointness, and LIKE-hostile input. - Worker suite: 150 tests green; lint and typecheck clean. Risks: status derivation is now evaluated twice (SQL predicate and the JS resolver for the same rows in different paths); the equivalence suite pins them together. Rejected alternatives: filtering in SQL with a status column rewrite (data migration for no query-plan win); caching whole-board JSON in KV (staleness on admin edits).
Why: d1.ts had grown to 2341 lines mixing 26 aggregates, every row mapper, and the announcement SQL builders in one file. The project caps files at 800 lines; the size made review diffs and ownership boundaries hard to see. Constraints / tradeoffs: - d1.ts stays the single public entry (createD1Store) so importers (index.ts, store-service, tests) are untouched; the split is purely internal. - Aggregates grouped into seven modules by domain: users/auth, mail, settings, ops, announcements, plus shared helpers and row-mappers. Largest module (mail.ts) is 586 lines, within the cap. - parseUserStatus now returns the declared UserStatus union; rows storing "outbound_disabled" map to "disabled" (the union has no outbound_disabled member and the untyped original leaked the wider string into UserRecord). - findMailboxDetailById moved with the mailboxes aggregate that calls it; no behavior change. Verification: - Worker suite: 16 files, 150 tests green (includes the announcement SQL equivalence suite from the prior commit). - eslint src/ and tsc --noEmit clean. - wrangler deploy --dry-run (worker build) passes. - persistence/README.md updated with the module map per CLAUDE.md. Risks: none expected — mechanical extraction verified by existing integration tests that exercise every aggregate through the in-memory and D1 paths. Rejected alternatives: splitting in-memory.ts in the same commit (deferred — separate identical-shape change); introducing typed Row interfaces per table (deferred — semantic change beyond a mechanical split).
Why: FormPrimitives.tsx had grown to 1699 lines holding every form control, the calendar popover machinery, and shared DOM utilities. Every form-control change reviewed an unrelated diff; the file was double the project's 800-line cap. Constraints / tradeoffs: - The barrel (index.ts) and FormPrimitives.tsx deep-import path are unchanged: FormPrimitives keeps the core controls (FormField, TextInput, SearchInput, TextareaInput, Checkbox/Radio family) and re-exports DateInput/SelectInput/MultiSelect, so none of the ~40 importing files needed edits. - internal.tsx holds the cross-widget utilities (cx, ref merging, native value setters, option extraction, render helpers) that previously made the file indivisible. - DateInput.tsx is 827 lines — one line over the cap — accepted as a single cohesive calendar+datetime widget; splitting the calendar grid from the input would split the shared state machine. - renderFormCheck's props are now a structural type on the module boundary instead of the local FormCheckProps; the react import list per module shrank to what it actually uses. Verification: - pnpm typecheck (web): 0 errors — the split also eliminated 25 implicit-any errors from event/value handlers that the monolith's re-exports were hiding. - eslint on src/shared/form: clean. - Web suite: 68 files, 331 tests green, including shared-form.test.tsx. - Web build + bundle budget check passed. - shared/form/README.md updated with the module map. Risks: none expected — mechanical extraction, no prop or behavior changes, verified by the existing form test suite. Rejected alternatives: rewriting DateInput with a headless calendar library (behavior risk for a pure refactor); splitting the CSS file (not oversized).
Why: WebhookPage.tsx had grown to 1449 lines mixing page state orchestration, two modal dialogs, the developer reference content, and shared formatting helpers. The dialogs and the endpoint form were the least cohesive parts of an otherwise page-level component. Constraints / tradeoffs: - WebhookPage.tsx (now 1044 lines) keeps the loaders, handlers, and the inline hero/workbench/rules/delivery sections; those sections close over a dozen state hooks each, so splitting them would mean a 15-prop interface per section and more risk than value in a mechanical refactor. - WebhookEndpointDialog and WebhookDeliveryDialog own their form internals (draft event toggling) via an onDraftChange callback; WebhookCodeBlock moved out as a self-contained component. - webhook-content.ts holds types, constants, sample payloads, and formatting helpers as a .ts file (no JSX) so react-refresh treats it as a plain module. - The page remains over the 800-line cap; this commit is the low-risk first slice, not the end state. Verification: - pnpm typecheck (web): 0 errors. - eslint src/features/settings: clean (0 errors, 0 warnings). - Web suite: 68 files, 331 tests green, including settings-page.test.tsx integration coverage of the webhook page. - Web build + bundle budget check passed. Risks: dialog behavior is driven by the same state props as the inlined JSX before; verified by the settings integration test. Rejected alternatives: extracting the three main sections too (state prop explosion); rewriting with a form library (behavior risk).
…it hardening Why: a second review pass over the branch found five real defects. The worst: apiFetch's GET dedupe let a caller inherit another caller's AbortError — a polling tick plus a manual refresh on the same URL surfaced a spurious error banner — and a joiner's own abort had no effect at all on the shared request. Constraints / tradeoffs: - announcement-sql.ts: instr() has no wildcard semantics, so the LIKE-style escaping was wrong — "50%" could not match text containing "50%". The keyword is now bound raw (still a bound parameter, no injection surface). Per-column matching also no longer matches a keyword spanning the old JS title/summary/tags join boundary; that behavior was accidental and is not preserved. - client.ts: a joining caller with its own signal now races that signal against the shared promise (prompt reject on own abort, shared request continues for others) and re-issues its own request when the shared one dies from a foreign AbortError. The in-flight entry deletion is identity-guarded so an unwinding request cannot delete a replacement. The join is cast to Promise<T> so apiFetch's inferred return type does not collapse to unknown (which poisoned every call site). - useInboxWorkspace: AbortError is treated as stale regardless of which controller caused it; unmount now aborts the in-flight request. - check-migration-numbering.mjs: the grandfather list is now exact filenames, so a third 0013-*.sql (or a rename) fails CI instead of silently passing. - FormPrimitives no longer re-exports the split widgets (breaking the FormPrimitives <-> MultiSelect runtime cycle); the barrel exports them directly and MultiSelectOption is single-sourced via a type-only re-export. - Mechanical fidelity verified against main: 24/26 d1 aggregate sections and all four WebhookPage JSX sections are byte-identical; the two d1 exceptions are the intentional listExpired/announcements rewrites. Form split: 61/63 blocks verbatim, the 2 exceptions documented. Verification: - Full matrix green: lint 0 errors, typecheck 0 errors (web+worker), 502 tests (19+150+333) including two new dedupe/abort race regression tests and a keyword "%" equivalence test, build + bundle budget, api-catalog, migration numbering check (rejects a forged third 0013). - The apiFetch generic collapse was caught by tsc only — vite build and vitest do not typecheck — which is why the full matrix runs tsc. Risks: foreign-abort retry issues one extra request in a narrow race window; correctness (fresh data) beats the rare duplicate. Rejected alternatives: ref-counted shared abort controllers (complexity for no additional caller-visible behavior); emulating the JS boundary-spanning keyword match in SQL.
…ity, date normalization Why: the adversarial second-pass review found the cleanup batching was built on a wrong platform assumption and the announcement keyword pushdown had drifted from its JS source of truth. The bind-limit defect re-broke the retention cleanup this branch exists to fix: D1 caps bound parameters per query at 100, and CLEANUP_BATCH_SIZE=500 fed 500-id IN clauses, so any backlog over 100 messages made the hourly cron fail before deleting anything. Constraints / tradeoffs: - The three IN-clause helpers in d1/mail.ts now chunk at 100 ids per statement. This fixes the cleanup path AND the pre-existing latent bug in the batch-delete route (unchanged code) in one place, so callers cannot exceed the limit regardless of batch size. - Keyword search now reconstructs the exact string the JS helper searches (title + " " + summary + tags joined by spaces, non-string tags skipped via json_each.type) and runs one instr() over it. This restores boundary-spanning and adjacent-tag matches and stops raw JSON punctuation from matching. Remaining known divergence, documented in the module header: SQLite lower() folds ASCII only vs JS Unicode toLowerCase — irrelevant for Chinese content. - 即将开始/已结束 predicates and the summary CASE now mirror resolveAnnouncementStatus branch-for-branch (upcoming decided by start_at alone); previously an end<start row classified differently in SQL vs JS. Unreachable via the API, but the parity claim is now true and pinned by a manage-scope equivalence case. - Empty-string startAt/endAt (accepted by isValidDateValue) is now normalized to NULL at the create/update boundary, and migration 0021 cleans any stored rows. A stored '' made the SQL list hide a row the JS detail endpoint still returned. Deploy applies migrations before the new Worker, so the invariant holds before the predicates ship. - The equivalence suite now anchors seed dates to Date.now() — the fixed 2026-09-xx dates would have gone vacuously green within days — and its expected side finally applies pagination (previously masked because visible rows never exceeded the page size). - WebhookEndpointDialog restores the pre-split Set semantics for event toggling (idempotent check) and memoizes the event set. - Review also confirmed: the dedupe/abort race it flagged was already fixed in d0b6707 (its probe ran against the pre-fix tree); the parseUserStatus description in 9978e19's message was wrong — the old code already mapped outbound_disabled to disabled; code was and is identical to main. Verification: - Full matrix green: lint 0, typecheck 0, 506 tests (19+154+333), build + bundle budget, api-catalog, migration numbering (22 files). - New: bind-limit regression test (250-id calls stay <= 100 params per statement), keyword parity cases (boundary, adjacent tags, JSON punctuation, mixed-type tags), manage-scope inverted-window status case, relative-date seeds. - wrangler d1 migrations apply (local): 0021 applied. - The equivalence suite caught its own pagination bug before this commit shipped it: SQL and JS totals agreed (11=11) while the expected side was unpaginated. Risks: the keyword predicate is now a heavier SQL expression (json_each per row) on a small admin-managed table; accepted. Rejected alternatives: NULLIF('') sprinkled through every date predicate (write-boundary normalization + migration keeps the SQL readable); reducing CLEANUP_BATCH_SIZE below 500 (the store-level chunking makes the loop size about R2/accounting, not SQL limits).
Why: cut the audit-followup batch — retention-cron enforcement, request hygiene (session write throttle, dedupe/abort race), announcement SQL pushdown with parity tests, and the three oversized-module splits — as a patch release, matching the 0.2.x fix-batch convention. Verification: - pnpm version:check green after version:sync (5 package.json files, shared version.ts, openapi.yaml info.version). - Full matrix green on this tree: lint, typecheck, 506 unit/integration tests, build + bundle budget, Playwright E2E (13 passed, 2 skipped). Risks: none beyond the branch's own (documented per commit); migrations 0020/0021 run in the deploy pipeline before the new Worker serves.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This branch was successfully deployed
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
1 participant
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.
Summary
Audit-driven hardening batch cut as v0.2.10. Three review passes (self-review, mechanical fidelity diffing, adversarial agent review) over 8 commits; every finding is closed with a regression test where feasible.
High-priority fixes
wrangler.tomlhad no cron trigger for thescheduledhandler — expired messages and R2 attachments accumulated forever. Now hourly on staging/production, index-backed (migration 0020), batched, with parallel attachment deletes.IN (...)clauses exceed D1's 100-parameter cap, so the cleanup cron failed outright on any backlog >100. The three IN-clause store helpers now chunk at 100 (also fixes the latent batch-delete path).AbortError(spurious "request aborted" banner on manual refresh over a poll tick). Joiners now race their own signal and re-issue on foreign aborts; unmount cancels in-flight requests.Performance
node:sqliteand diffs against the JS source of truth, including keyword parity across title/summary/tags boundaries, JSON punctuation, and inverted windows.Refactors (verified byte-identical)
d1.ts(2,341 lines) → entry + 7 aggregate modules;FormPrimitives.tsx(1,699) → 5 modules;WebhookPage.tsx(1,449) → page + 3 components. Mechanical extraction verified by diffing every original block against the new modules.Security
ENVIRONMENT=local; OAuth start/callback/finalize now rate-limited like login.Process / CI
modules/layering and D1+KV feature-toggle mechanism; announcement""date normalization (migration 0021).Commits
52ab62ffix: enforce cleanup cron, cut session write amplification, harden request hygiene323a128perf: push announcement list filters and pagination into D1 SQL9978e19refactor: split d1.ts into per-aggregate persistence modules8f38499refactor: split FormPrimitives.tsx into per-widget form modules9174429refactor: split WebhookPage into content, code block, and dialog modulesd0b6707fix: review-pass corrections — instr keywords, dedupe/abort race, split hardeningbbe9b10fix: resolve adversarial-review findings — D1 bind limit, keyword parity, date normalizationa072c10release: v0.2.10Test plan
pnpm lint/pnpm typecheck— 0 errorspnpm test— 506 tests green (shared 19 / worker 154 / web 333), including:node:sqlite-driven)pnpm build— wrangler dry-run + bundle budget passpnpm test:e2e— 13 passed, 2 skipped (matches baseline)pnpm version:check— 0.2.10 in sync across 5 packages + version.ts + openapi.yamlv0.2.10→ release workflow; staging deploy + verification per runbookRollback
Worker config/code and migrations ship together via the deploy pipeline (migrations apply before the new Worker). Both migrations are additive (index creation; idempotent
UPDATE ... WHERE col = ''), so rollback to the previous Worker tag is safe — the old code ignores the new index and treats''dates the same as before.