fix(site/src/pages/AgentsPage/components): derive diff cache keys from patch content - #27982
fix(site/src/pages/AgentsPage/components): derive diff cache keys from patch content#27982DanielleMaywood wants to merge 2 commits into
Conversation
Follow-up to 07f79af (markdown rendering improvements), which sanitized notification labels before template rendering but regenerated only the Docker-free golden files. The AI budget templates conditionally render 'This limit is a per-user override.' on eq .Labels.limit_source "user_override"; SanitizeMarkdown escapes the underscore to user\_override, so the condition no longer matches and the committed goldens are stale. CI's gen job restores coderd/notifications/testdata instead of regenerating (it needs a DB), which is why main's gen and fmt jobs are red. Regenerate the four AI budget goldens with the golden test and apply gci's import ordering to smtp_internal_test.go.
…m patch content @pierre/diffs 1.3.x defaults each parsed diff's cacheKey to the file name, and the singleton worker pool caches highlighted ASTs by that key alone. Two different diff bodies for the same path (e.g. consecutive edit_files turns on one file) therefore share a cache entry, and rendering the newer diff against the older highlighted AST throws DiffHunksRenderer.processDiffResult: deletionLine and additionLine are null. Pass a content-derived (FNV-1a) cacheKeyPrefix from parseSingleFileDiff and useParsedDiff so keys are specific to the exact patch body while staying stable across re-renders.
|
/coder-agents-review |
|
Chat: Review in progress (0/1 reviewers complete) | View chat deep-review v0.9.0 | Round 1 | Status: Netero (first pass) About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3aafaa09c1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let hash = 0x811c9dc5; | ||
| for (let i = 0; i < text.length; i++) { | ||
| hash ^= text.charCodeAt(i); | ||
| hash = Math.imul(hash, 0x01000193); |
There was a problem hiding this comment.
Use a collision-resistant patch cache key
When two different patches for the same path produce the same 32-bit FNV-1a value, they still receive identical cache keys and can reproduce the stale-AST renderer failure this change is meant to prevent. This is concrete rather than hypothetical: otherwise identical file.ts patches replacing old with 3frRhAsv2W and kDMJTx1ES3 both produce content-119c7773. If the first AST remains in the singleton worker pool, rendering the second patch reuses it, so use a substantially wider synchronous digest or a collision-free content identity.
Useful? React with 👍 / 👎.
Problem
Since the
@pierre/diffsbump to 1.3.3 (#27932), the chat UI intermittently shows a red error box instead of a tool diff:The 1.3.x web components default each parsed diff's
cacheKeyto the file name, and the singleton worker pool caches highlighted ASTs by that key alone. Two different diff bodies for the same path (e.g. consecutiveedit_filesturns on one file, or a re-render after a later edit) therefore share one cache entry, and rendering the newer diff against the older highlighted AST throws inside the renderer. Upstream tracks this as pierrecomputer/pierre#1052; #964 documents the same throw path. The existingRemoteDiffPanelalready works around it by passing a uniquecacheKeyPrefix, but the chat tool renderers andLocalDiffPanelparse without one.Fix
Derive the
cacheKeyPrefixfrom the patch text (FNV-1a checksum) so keys are specific to the exact diff body while staying stable across re-renders and remounts:parseSingleFileDiff(shared by thewrite_filediff builder, the syntheticedit_filesbuilder, and the server-supplied diff parser) now passes a content-derived prefix.useParsedDifffalls back to the same prefix when a caller supplies none, which also fixesLocalDiffPanel. Explicit prefixes (RemoteDiffPanel's timestamp-scoped key) still win.A 32-bit checksum is deliberate: the key only needs to separate different patch bodies for the same path while an older AST is cached, and
crypto.subtleis async so it cannot run in the render path.The first commit is an unrelated prerequisite: main is currently not
make gen/make fmtclean (itsgenandfmtCI jobs are red), and the mandatory pre-commit hook fails on any unstaged change. That drift is a mechanical follow-up to 07f79af, which sanitized notification labels before template rendering but regenerated only the Docker-free goldens; the AI budget templates'eq .Labels.limit_source "user_override"condition no longer matches becauseSanitizeMarkdownescapes the underscore. Kept as a separate commit so it can be dropped or split out.Root-cause trace (agent tool call to renderer crash)
edit_files(coderd/x/chatd/chattool/editfiles.go), which calls the workspace agent'sPOST /api/v0/edit-fileswithIncludeDiff: true.agent/agentfiles/files.go), returned asFileEditResponse.Files[].Diff.tool-resultmessage part; the frontend pairs it with the tool call (messageParsing.ts).Tool.tsxdispatches toEditFilesRenderer, which parses the server diff (or synthesizes one from the args viaDiff.createPatch) throughparseSingleFileDiffinChatElements/tools/utils.ts. Before this fix,parsePatchFileswas called without acacheKeyPrefix, so the parsedFileDiffMetadatahad nocacheKey.EditFilesTool.tsxrenders<FileDiff fileDiff=...>. The@pierre/diffscomponent assignscacheKey = fileDiff.namewhen absent (components/FileDiff.js:472).DiffHunksRenderer.renderDiffconsults the singleton worker pool's highlight cache keyed only by thatcacheKey(worker/WorkerPoolManager.js:76-77, entries stored at :653/:701).processDiffResultwalks the new hunks and indexes the old, shortercode.additionLines/deletionLinesarrays (renderers/DiffHunksRenderer.js:646-651), bothundefined, and throws. The component catches it and renders the error box viaapplyErrorToDOM.Reproduced locally against the shipped 1.3.3 library by seeding the pool cache with a first diff's result and rendering a second diff of the same path; with content-derived keys the lookup misses and rendering succeeds.
Generated with Coder Agents.