Deep cleanup: rate-limit message regression, correctness fixes, and dependency audit - #32
Merged
Merged
Conversation
Pre-existing, unrelated to AUDIT.md's scope: a transitive build-tool dependency (pulled in via Tailwind/PostCSS tooling), flagged by GitHub's Dependabot alert on the repo. npm audit fix bumped it and its own transitive deps (update-browserslist-db, node-releases, electron-to-chromium, caniuse-lite, baseline-browser-mapping) - lockfile only, no direct dependency version changed. npm audit now reports zero vulnerabilities. Lint, build, and full test suite still pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… real rate-limit message
Found while checking whether AGENTS.md's parsing/rate-limit descriptions
still matched the code after this session's earlier fixes: there are two
independent rate limiters in front of app/api/chat/route.ts, not one.
proxy.ts (Next.js middleware, runs first) computes and returns a real
"Rate limit reached. Try again in N minutes." from its own window state.
lib/rateLimit.ts's in-route check (a second, separate limiter) only ever
returns the generic "Too many requests. Please try again later."
The earlier followup-2 fix only ever looked at the second one and
concluded the server's 429 body was never worth showing, so it hardcoded
AI_LIMIT_NOTICE unconditionally - discarding proxy.ts's actual reset-time
message for what is the more common rate-limit path in practice (it runs
first). aiLimitError() now takes the parsed body's message when there is
one, falling back to AI_LIMIT_NOTICE only when the body doesn't parse or
carries no message - not for res.statusText, which is just a generic HTTP
reason phrase and would be a worse fallback than the constant.
Verified live: confirmed proxy.ts's actual 429 body via 21 real requests
against the dev server ("Rate limit reached. Try again in 60 minutes."),
then confirmed via mocked fetch that the UI now shows a server message
verbatim when present and falls back to AI_LIMIT_NOTICE on a malformed body.
Also updates AGENTS.md's Output Parsing section, which had gone stale
against this session's F4/F7/F8 changes: the "## fallback only when no ###
at all" description, the outputHealth issue list missing incomplete-clips,
and the F6 "strictly between" duration wording.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A dedicated correctness-review pass (separate from and after this session's
AUDIT.md work) found 4 real, empirically-reproduced bugs in code this
session had already touched or was adjacent to:
1. writeHistory([]) - deleting the last remaining history item - fell into
the "every write attempt failed, leave storage alone" fallback (a
zero-length list makes the shrink loop run zero times) and read back the
item that was just supposed to be deleted, silently undoing the
deletion. Now special-cased: an empty target list clears storage.
2. parseBentoSections' heading-level split had a tie-breaking gap: it only
switched from "###" to "##" when the "##" pass recovered *more* canonical
sections. A model using "###" for two real sections and "##" for the
other two (sibling sections at different levels, not one nested inside
the other) tied 2-vs-2, and the tie-break kept only one pair - the
other pair's content was swallowed into the section before it. Replaced
the whole strict/relaxed dual-pass with a single split on "#{2,3}\s+",
which recovers all four regardless of which level the model used per
section - simpler than the two-pass comparison it replaces, not just a
patch on top of it.
3. ClipsBentoCard rendered the Clips section's preamble text twice when the
model didn't emit recognizable "Option N" headers: once as its own styled
paragraph (unconditional whenever non-empty) and again inside the full
raw-body markdown fallback that renders whenever there's nothing
structured to show in the grid. Preamble now only renders in the
structured (grid) branch, where the fallback isn't also showing it.
4. handleFiles' "don't let a slow file read override an active generation"
guard (added while fixing followup 1) discarded the file's content
entirely rather than just skipping the status transition - a file
dropped just before switching to Direct Paste and clicking Generate was
silently lost, with no error and no way to tell anything went wrong.
Now captures (or clears, on a validation failure) the file data
unconditionally and only guards the setStatus() calls specifically.
Also, from the same review's cleanup pass (bar: a nameable win, not a style
preference - skipped a few candidates that didn't clear it, including
extracting shared chapter-naming prompt text, which would mean editing
production prompt wording without a live model check):
- HISTORY_LIMIT was defined once in each of lib/history.ts (a bare literal
`10`) and lib/historyStorage.ts (the actual constant) - moved to
history.ts (avoids a circular import the other way) and re-exported.
- Extracted useCopyToClipboard(), shared by BentoCard and TitlesBentoCard,
which had byte-for-byte identical copy-to-clipboard state and handlers.
- Extracted resetResultState() for the "clear per-generation UI state"
cluster (errorMessage/limitNotice/outputIssues/historySaveFailed/isDemo)
that runWithText, analyzeAnotherSermon, viewDemo, and loadFromHistory each
set individually - the exact pattern that made adding outputIssues and
historySaveFailed, earlier this session, three separate edits each.
- app/api/chat/route.ts's eight hand-copied `new Response(JSON.stringify(...))`
error blocks became one `jsonError(status, message)` helper.
Verified: full test suite (100 tests, up from 97) plus live browser checks
for each of the four bugs (a real localStorage delete-to-empty, a mocked
Clips section with no Option headers, a controllable-delay FileReader
racing a mocked generation) and a full live smoke test of every reset path
(view demo, reset, load from history) after the resetResultState extraction.
Lint and build clean.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…on removed
countCanonicalSections() no longer exists after the strict/relaxed dual-pass
was replaced with a single unified heading split; updated the Output Parsing
section to describe the actual current mechanism (a single /^#{2,3}\s+/m
split) and why, rather than the two-pass comparison it replaced.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
jakeh280
added a commit
that referenced
this pull request
Sep 8, 2026
… docs and merged branches (#33) Rate limiting: proxy.ts and lib/rateLimit.ts were two independent, unsynchronized rate limiters guarding the same endpoint - proxy.ts (a fixed window, run as real Next 16 middleware since the framework upgrade silently activated it) and lib/rateLimit.ts (a sliding window, called directly from the API route). They weren't equivalent: only the sliding window closes the gap a fixed window has, where a burst of MAX_REQUESTS right before the hourly reset plus another right after can let ~2x the limit through in a short span straddling the boundary. Consolidating to one limiter means giving the survivor the *stricter* algorithm, not just picking one arbitrarily - otherwise removing the "redundant" check would have quietly doubled the worst-case burst instead of just deleting dead weight. Extracted the sliding-window decision logic into a new pure module, lib/proxyRateLimit.ts (checkAndRecord/getClientIp), kept separate from proxy.ts's NextRequest/NextResponse glue so it's unit-testable without Next's runtime - `next/server` isn't resolvable under plain Node, which is why this logic used to run untested. Deleted lib/rateLimit.ts and its now-fully-redundant call site in app/api/chat/route.ts. Verified via `next build` (both before and after) that proxy.ts really does compile to real middleware (`ƒ Proxy (Middleware)`, confirmed nodejs runtime in functions-config-manifest.json), and added tests/proxyRateLimit.test.ts (8 tests, including one that specifically pins the sliding-window boundary: aging out one hit opens exactly one slot, not a fresh batch). Docs: AGENTS.md's Rate Limiting section and file tree were stale on multiple fronts - "not a Next.js middleware" (true when written, false since the Next 16 upgrade), a missing lib/proxyRateLimit.ts entry, and several pre-existing gaps (boundedBody.ts, demoContent.ts, and three test files never added to the tree). All corrected. Also dropped dangling "AUDIT.md F#" citations from test/lib comments now that the doc is gone, rephrasing each to describe the bug directly. Housekeeping: deleted AUDIT.md - everything in it is fixed, merged, and now documented in AGENTS.md and the PR #31/#32 descriptions, so the standalone doc no longer earns its keep. Deleted three remote branches (agent/sermon-cross-promotion, claude/admiring-bassi, claude/lucid-cray) that were fully merged into main (zero unique commits) and left over from other sessions. Verification: 108/108 tests pass (up from 100), lint clean, `next build` succeeds with the middleware chunk intact, two independent review passes (one of which caught the fixed-vs-sliding-window regression this commit fixes). Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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
Second pass after #31, per an explicit "fix everything you can find" mandate. Four independent fixes, each verified individually:
main).lib/rateLimit.tsalone. It turns outproxy.tsis a separate rate limiter (its own in-memory store, runs earlier in the request path) that already produces a more useful message ("Rate limit reached. Try again in N minutes."). That real message was being discarded and replaced with the generic one. Now the actual parsed body message is surfaced, with the generic message only as a fallback when none is present. This bug was introduced by me earlier today, from reasoning about only one of the two rate limiters without knowing the second existed./code-reviewpass (2 parallel subagents, correctness + cleanup angles) run againstapp/andlib/beyond AUDIT.md's original scope:outputParsing.ts: a strict/relaxed heading-detection tie-break dropped sections when sibling (non-nested) sections used different heading levels. Replaced the whole dual-pass approach with one unified/^#{2,3}\s+/mregex — a real simplification, not a patch.historyStorage.ts:writeHistory([])(deleting the last history item) fell through to a "leave storage untouched" fallback and read back the deleted item, silently undoing the deletion. Added an explicit empty-list case.page.tsx:ClipsBentoCard's preamble paragraph rendered twice when the model didn't emit "Option N" headers.page.tsx:handleFiles's loading-status guard discarded successfully-read file content entirely instead of just skipping the status transition.outputParsing.tsfunctions removed in fix chore: dependency updates, clip duration prompt fix, cleanup #3 —countCanonicalSections()/hasHeadedSection()no longer exist; docs now describe the unified regex approach.Verification
npm test), up from 74 at the start of this work.npm run lint).1:00:00/1:10:00chapter timestamps past the hour mark, confirming the fix without a mock.Deliberately left alone
proxy.tsandlib/rateLimit.ts) still keep separate state. Consolidating them is a real architectural fix but is a rearchitecture, not a bug fix — out of scope here.systemPrompt.ts— editing production prompt wording needs a live-quality A/B comparison, not a mechanical edit.🤖 Generated with Claude Code