Optimize fragment rendering and homepage link generation - #67
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ARX3 transport and dictionary loading, updates fragment sizing and selection, rewires viewers and renderers for lazy client loading and raw-source paths, normalizes base paths, and expands static-serving, scripts, docs, and tests around the new codec and asset flow. ChangesARX3 transport and fragment flow
Sequence Diagram(s)sequenceDiagram
participant Browser
participant ViewerShell
participant FragmentModule
participant ArtifactStage
participant StaticServer
Browser->>ViewerShell: hash change / initial payload
ViewerShell->>FragmentModule: decodeFragmentAsync(hash, options)
FragmentModule-->>ViewerShell: { envelope, activeArtifact } or error
ViewerShell->>ArtifactStage: render(activeArtifact) (dynamic import)
ArtifactStage->>ArtifactStage: load renderer (code/json/markdown/diff/csv)
ArtifactStage-->>ViewerShell: onRendererReady(readyKey)
Browser->>StaticServer: GET /arx-dictionary.json.br or /vendor/diff-view-pure.css.br
StaticServer-->>Browser: precompressed asset (Content-Encoding: br)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying agent-render with
|
| Latest commit: |
648ed11
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1599fd60.agent-render.pages.dev |
| Branch Preview URL: | https://codex-optimize-codebase.agent-render.pages.dev |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b61cac405
ℹ️ 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".
| fragmentModulePromise ??= import("@/lib/payload/fragment"); | ||
| return fragmentModulePromise; |
There was a problem hiding this comment.
If the dynamic import of @/lib/payload/fragment fails once (for example due to a transient chunk/network error), fragmentModulePromise stays set to that rejected promise forever, so every later decode/encode attempt in the same session immediately fails even after connectivity recovers. This turns a temporary asset-load issue into a persistent viewer/link-creator outage until full page reload; clear the cache when the import rejects so retries can succeed.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/viewer-shell.tsx (1)
201-204:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear injected mode on internal navigation.
After a UUID-injected load,
setFragmentHash()andhandleGoHome()updatehashwithout ahashchangeevent, soinjectedPayloadRef.currentstaystrue. Every later decode in that session then keeps using{ skipFragmentBudget: true }, which silently disables the normal fragment budget.🔧 Suggested fix
const setFragmentHash = useCallback((nextHash: string) => { + injectedPayloadRef.current = false; if (window.location.hash === nextHash) { return; } window.history.replaceState(null, "", nextHash); setHash(nextHash); }, []); const handleGoHome = useCallback(() => { + injectedPayloadRef.current = false; const url = window.location.pathname + (window.location.search || ""); window.history.replaceState(null, "", url); setHash(""); }, []);Also applies to: 233-235, 297-310
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/viewer-shell.tsx` around lines 201 - 204, The injectedPayloadRef flag must be cleared whenever the app updates the location hash internally so subsequent decodes don't keep using skipFragmentBudget; update the routines that mutate hash — specifically syncHash, setFragmentHash, handleGoHome and the similar hash-update handlers around the other occurrences (the blocks at the locations analogous to 233-235 and 297-310) to set injectedPayloadRef.current = false immediately before calling setHash(window.location.hash) (or before updating the hash state) so injected mode is cleared on internal navigation.
🧹 Nitpick comments (9)
tests/envelope.test.ts (1)
13-25: ⚡ Quick winAdd a regression test for empty bundles.
This PR now rejects
artifacts.length === 0, but that new path is still untested here.Suggested test addition
describe("normalizeEnvelope", () => { it("normalizes single-artifact bundles to the only artifact id", () => { @@ expect(result.envelope.activeArtifactId).toBe("one"); }); + + it("rejects bundles with no artifacts", () => { + const result = normalizeEnvelope({ + ...baseEnvelope, + artifacts: [], + }); + + expect(result.ok).toBe(false); + if (result.ok) { + return; + } + expect(result.message).toMatch(/at least one artifact/i); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/envelope.test.ts` around lines 13 - 25, Add a regression test that exercises the new code path rejecting envelopes with zero artifacts: create a variant of baseEnvelope where artifacts is an empty array and call normalizeEnvelope; assert the result is a failure (result.ok is false) and optionally assert the error or rejection reason exists. Place this alongside the existing tests (e.g., near the "normalizes single-artifact bundles..." test) and reference normalizeEnvelope and baseEnvelope so the CI covers the new artifacts.length === 0 branch.scripts/bench-codecs.mjs (2)
176-179: 💤 Low valueNote:
medianmutates the input.
values.sort(...)sorts in place. Callers (measure, line 189) passsamplesand then return{ result, ms: median(samples) }without usingsamplesagain, so it's safe today—but future readers may be surprised. A defensive[...values].sort(...)is a one-line guard.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/bench-codecs.mjs` around lines 176 - 179, The median function currently mutates its input by calling values.sort(...); update median to sort a shallow copy instead (e.g., use [...values] or Array.from(values]) before sort) so callers like measure (which passes samples) are not affected by in-place mutation; keep the rest of median logic the same and return 0 for empty arrays as before.
211-219: 💤 Low valueMinor:
repeatedFixturemay slice mid-character for multi-byte input.
fixture.slice(0, targetLength)operates on JavaScript string units (UTF-16 code units), not bytes. With ASCII fixtures this is fine, but if anyone passes a block with multi-byte/emoji content the slice could split a surrogate pair. Not an issue with current fixtures—just flagging for future use.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/bench-codecs.mjs` around lines 211 - 219, The current repeatedFixture builds a string then uses fixture.slice(0, targetLength), which can split UTF-16 surrogate pairs for multi-byte characters; update repeatedFixture to trim by Unicode code points instead of UTF-16 code units — e.g., replace the final slice with a safe truncation using Array.from(fixture) (or [...fixture]) and join the first targetLength code points (Array.from(fixture).slice(0, targetLength).join('')) so block, segmentSuffix and index logic remain unchanged but surrogate pairs/emoji won't be sliced in half.src/components/home/link-creator.tsx (1)
36-79: 💤 Low valueConsider co-locating constants in a module helper if reuse grows.
fieldHints,fieldPlaceholders,codecOptions,defaultLinkCreatorDraft, andgetBodyFieldLabelare now owned by this component. If the same draft/labels logic is ever needed by another surface (e.g., a docs preview or the viewer shell), pulling them into a smalllink-creator-presets.tswould avoid drift. Not required for this PR.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/home/link-creator.tsx` around lines 36 - 79, The constants and small helpers used only in this component—fieldHints, fieldPlaceholders, codecOptions, defaultLinkCreatorDraft, and getBodyFieldLabel—should be moved into a shared module so they can be reused and kept in sync; create a new module (e.g., link-creator-presets) exporting those symbols, update src/components/home/link-creator.tsx to import fieldHints, fieldPlaceholders, codecOptions, defaultLinkCreatorDraft, and getBodyFieldLabel from that module, and ensure any types (ArtifactKind, LinkCreatorDraft) remain imported or re-exported as needed so behavior of the existing functions (getBaseUrl, numberFormatter, etc.) is unchanged.scripts/serve-export.mjs (2)
60-92: 💤 Low valueConsider extending Brotli handling to other pre-compressed assets.
The current logic only flags
.json.brand.css.bras Brotli-encoded. If the build ever emits other.brvariants (e.g.,.js.br,.svg.br,.html.br), they'll be served withContent-Type: application/octet-streamand noContent-Encoding: br, breaking the client. A general.brhandler that derives the underlying MIME from the inner extension would be safer.♻️ Proposed generalization
function contentTypeFor(filePath) { if (filePath.endsWith(`${path.sep}.well-known${path.sep}api-catalog`)) { return apiCatalogContentType; } - if (filePath.endsWith(".json.br")) { - return "application/json; charset=utf-8"; - } - - if (filePath.endsWith(".css.br")) { - return "text/css; charset=utf-8"; - } + if (filePath.endsWith(".br")) { + const innerExt = path.extname(filePath.slice(0, -3)); + return contentTypes.get(innerExt) || "application/octet-stream"; + } return contentTypes.get(path.extname(filePath)) || "application/octet-stream"; }- if (filePath.endsWith(".json.br") || filePath.endsWith(".css.br")) { + if (filePath.endsWith(".br")) { headers["Content-Encoding"] = "br"; headers.Vary = "Accept-Encoding"; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/serve-export.mjs` around lines 60 - 92, The code only treats .json.br and .css.br as Brotli files; update contentTypeFor(filePath) and headersFor(filePath) to handle any ".br" suffix: detect if filePath.endsWith(".br"), derive the real ext by removing the ".br" suffix and lookup the MIME via contentTypes.get(path.extname(realPath)) (falling back to "application/octet-stream"), return that as the Content-Type in contentTypeFor (and in headersFor set "Content-Encoding": "br" and "Vary": "Accept-Encoding" whenever a ".br" file is detected), preserving existing logic for isNextStaticAsset(filePath) and the api catalog Link header in headersFor.
159-178: 💤 Low valueConsider adding
Content-Lengthfor HEAD/GET responses.
stat()is already called and yieldsdetails.size, but it's discarded. SettingContent-Lengthlets clients (and CI HEAD checks for.brassets) verify byte counts and avoids chunked-transfer mode, which can confuse some tooling and proxies. Worth doing here since this is the preview/self-hosted entrypoint validated in the PR's tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/serve-export.mjs` around lines 159 - 178, The responses currently send headers from headersFor(finalPath) but omit Content-Length even though a prior stat() call yields details.size; update the handler to call fs.statSync (or reuse the existing stat result) for finalPath and add a "Content-Length" header with details.size to the headers passed to response.writeHead (for both GET and HEAD paths), ensuring the HEAD branch still ends after sending headers and the GET branch streams via createReadStream(finalPath); reference the symbols finalPath, headersFor, response.writeHead, method, and createReadStream when making the change.src/components/renderers/diff-renderer.tsx (2)
198-201: 💤 Low valueEdge case: module-level stylesheet state isn't reference-counted across instances.
diffViewStylesheetPromiseand the<link id="agent-render-diff-view-styles">element are module globals. If twoDiffRendererinstances coexist (e.g., a future split layout, or a stale rich instance + a newly mounted fallback during transitions), the fallback effect at line 624 will callremoveDiffViewStylesheet()and strip the stylesheet that another rich instance is still using.Today the viewer shell renders one artifact at a time, so this is not user-visible. If multi-diff layouts ever land, consider reference-counting active rich instances before removing the link.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/renderers/diff-renderer.tsx` around lines 198 - 201, The stylesheet globals (diffViewStylesheetPromise and DIFF_VIEW_STYLESHEET_ID) are not reference-counted, so removeDiffViewStylesheet() can remove the shared <link> while another DiffRenderer still needs it; add a module-level counter (e.g., diffViewInstanceCount) and update the lifecycle helpers so mounting a rich renderer increments the count and unmounting decrements it, and change removeDiffViewStylesheet() to only remove the DOM node and clear diffViewStylesheetPromise when the count reaches 0; update the places that previously called removeDiffViewStylesheet() (the effect in DiffRenderer that tears down the fallback) to use the new increment/decrement semantics so the stylesheet is retained while any instance remains.
621-642: ⚡ Quick winStylesheet load failure is silently swallowed.
loadDiffViewStylesheet().catch(() => undefined).then(() => setStylesReady(true))marks styles "ready" even when both the.brand plain.cssURLs fail. The rich diff then mounts unstyled with no console signal. Consider surfacing the error (e.g., falling back to the raw patch state, or at leastconsole.warn) so production breakage is observable.♻️ Proposed refinement
setStylesReady(false); loadDiffViewStylesheet() - .catch(() => undefined) - .then(() => { + .then(() => { if (!cancelled) { setStylesReady(true); } - }); + }) + .catch((error) => { + if (!cancelled) { + console.warn("Failed to load rich diff stylesheet", error); + setStylesReady(true); + } + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/renderers/diff-renderer.tsx` around lines 621 - 642, The current useEffect in diff-renderer.tsx swallows stylesheet load errors; update the loadDiffViewStylesheet promise handling so failures are surfaced and you don't silently mark styles as ready: call loadDiffViewStylesheet().then(() => { if (!cancelled) setStylesReady(true) }).catch((err) => { console.warn('Failed to load diff stylesheet', err); if (!cancelled) { removeDiffViewStylesheet(); /* fall back to raw/unstyled diff */ setStylesReady(true); } }); keep the cancelled guard and the existing checks around renderedDiff, and reference renderedDiff, loadDiffViewStylesheet, removeDiffViewStylesheet, setStylesReady and diffFilesHaveRenderableFile to locate the code.src/lib/diff/git-patch.ts (1)
48-74: 💤 Low valueMinor: leading content before first
diff --gitis silently dropped.In
parsePatchSections, if the patch contains preamble text before the firstdiff --githeader (e.g., aFrom ...mbox-style header fromgit format-patch), that prefix is skipped because the loop only starts accumulating after the first match. Today’s callers (UI/test fixtures) always feed plaindiff --gitbundles so this is not user-facing, but it’s worth noting as a known limitation if patches fromgit format-patchare ever piped in.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/diff/git-patch.ts` around lines 48 - 74, parsePatchSections currently drops any leading preamble before the first DIFF_SECTION_HEADER_RE match (e.g., mbox "From ..." headers); update the loop in parsePatchSections so when the first match has match.index > 0 you call parsePatchSection(normalized.slice(0, match.index).trim(), sectionIndex) (using parsePatchSection and the current sectionIndex), increment sectionIndex, and then set previousStart = match.index; keep the existing logic for subsequent matches and the final push so all text before the first "diff --git" is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Around line 95-99: Add a short explanatory comment for the "overrides" entry
that documents why "next" forces "postcss": "8.5.14" — e.g., note that Tailwind
CSS v4 requires postcss ^8.5.6 while Next.js ships postcss 8.4.31, and pinning
to 8.5.14 bridges that gap; place this comment adjacent to the overrides block
(the "overrides" -> "next" -> "postcss" entry) in package.json or add the same
note to docs/dependency-notes.md so future maintainers understand the
constraint.
In `@public/vendor/diff-view-pure.css`:
- Line 138: The vendored CSS triggers stylelint errors (e.g.,
declaration-empty-line-before and keyword-case) for lines like the rule
containing "border-spacing: var(--tw-border-spacing-x)
var(--tw-border-spacing-y);"—either normalize the file to comply with our lint
rules (fix empty-line-before formatting around declarations and normalize
keyword casing to lowercase across the affected selectors/rules) or exclude the
vendored files from linting by adding "public/vendor/**" to the stylelint ignore
pattern in the Stylelint config; update only the code around the offending
declarations or the stylelint config accordingly and ensure the change
references the same symbols (the border-spacing declaration and the listed
offending rules) so CI stops failing.
In `@src/components/renderers/markdown-renderer.tsx`:
- Around line 47-73: The getCodeLanguage function strips non-alphanumeric
characters so languages like "c++" and "c#" are reduced to "c"; update
getCodeLanguage (and its use of LANGUAGE_CLASS_PREFIX) to allow additional valid
characters such as '+' and '#' (and optionally '.' or other common alias chars)
when scanning the language token, so the loop does not break on those characters
and the returned slice preserves them (then still lower-case it and fallback to
"text" when empty).
In `@src/components/theme/use-theme-controller.ts`:
- Around line 73-125: Add a JSDoc block immediately above the exported
useThemeController function that documents what the hook does, its behavior, and
its return shape; include a short description, the fact that it reads/writes
theme via localStorage and syncs with system and cross-tab events, and an
`@returns` entry describing the returned object properties (preference,
resolvedTheme, setTheme) and their types/meanings so consumers and editors get
proper intellisense for useThemeController.
In `@src/components/viewer-shell.tsx`:
- Around line 258-263: The selection should be local-first: restore and use a
local state activeArtifactId in viewer-shell.tsx to control which artifact opens
immediately (set activeArtifactId when the user selects an artifact), derive
activeArtifact from that id (fallback to getActiveArtifact(envelope) only if
activeArtifactId is null), and avoid waiting for
encodeEnvelopeAsync()/parsed/hash round-trip to update the visible selection;
keep rendererReadyKey = activeArtifact ? hash : "" or use hash only for renderer
syncing but do not gate the UI selection on encodeEnvelopeAsync completion;
update any logic in functions/components referencing activeArtifact and
rendererReadyKey so they read from activeArtifactId-first and only reconcile
with parsed/hash results when encoding completes.
In `@src/lib/payload/arx-codec.ts`:
- Around line 499-518: In envelopeToArx2Tuple, the activeIndex check uses
`activeIndex > 0` which treats index 0 as absent; change the condition to
include zero (e.g., `activeIndex >= 0 ? activeIndex : undefined`) so the
activeArtifactId that maps to the first artifact is preserved in the tuple;
update the return that calls trimOptionalTuple ([2, artifacts, envelope.title,
activeIndex ...]) accordingly and keep the artifact loop and activeIndex
assignment (artifact.id === activeArtifactId) as-is.
In `@tests/arx-codec.test.ts`:
- Around line 543-559: The test currently only calls loadArxDictionary and never
verifies the arx2 overlay is fetched separately; update the test to also call
loadArx2OverlayDictionary (after stubbing fetch) so both URLs are requested, and
assert requests includes both "/arx-dictionary.json" and the overlay path (e.g.,
the arx2 overlay URL) in the correct order or as separate entries; keep the
vi.unstubAllGlobals() and loadArxDictionarySync(arxDictionaryJson) in finally as
before. Ensure you reference loadArxDictionary, loadArx2OverlayDictionary, and
loadArxDictionarySync when adding the second call and the new assertion.
In `@tests/components/link-creator.test.tsx`:
- Around line 72-79: The test is using the wrong element type for getByLabelText
— change the generic from HTMLInputElement to HTMLTextAreaElement for the
"Generated agent-render link" queries in link-creator.test.tsx (the lines
calling screen.getByLabelText<...>("Generated agent-render link").value and the
subsequent assertions) so the type matches the component's <textarea
aria-label="Generated agent-render link">; update all occurrences of that
generic to HTMLTextAreaElement.
---
Outside diff comments:
In `@src/components/viewer-shell.tsx`:
- Around line 201-204: The injectedPayloadRef flag must be cleared whenever the
app updates the location hash internally so subsequent decodes don't keep using
skipFragmentBudget; update the routines that mutate hash — specifically
syncHash, setFragmentHash, handleGoHome and the similar hash-update handlers
around the other occurrences (the blocks at the locations analogous to 233-235
and 297-310) to set injectedPayloadRef.current = false immediately before
calling setHash(window.location.hash) (or before updating the hash state) so
injected mode is cleared on internal navigation.
---
Nitpick comments:
In `@scripts/bench-codecs.mjs`:
- Around line 176-179: The median function currently mutates its input by
calling values.sort(...); update median to sort a shallow copy instead (e.g.,
use [...values] or Array.from(values]) before sort) so callers like measure
(which passes samples) are not affected by in-place mutation; keep the rest of
median logic the same and return 0 for empty arrays as before.
- Around line 211-219: The current repeatedFixture builds a string then uses
fixture.slice(0, targetLength), which can split UTF-16 surrogate pairs for
multi-byte characters; update repeatedFixture to trim by Unicode code points
instead of UTF-16 code units — e.g., replace the final slice with a safe
truncation using Array.from(fixture) (or [...fixture]) and join the first
targetLength code points (Array.from(fixture).slice(0, targetLength).join(''))
so block, segmentSuffix and index logic remain unchanged but surrogate
pairs/emoji won't be sliced in half.
In `@scripts/serve-export.mjs`:
- Around line 60-92: The code only treats .json.br and .css.br as Brotli files;
update contentTypeFor(filePath) and headersFor(filePath) to handle any ".br"
suffix: detect if filePath.endsWith(".br"), derive the real ext by removing the
".br" suffix and lookup the MIME via contentTypes.get(path.extname(realPath))
(falling back to "application/octet-stream"), return that as the Content-Type in
contentTypeFor (and in headersFor set "Content-Encoding": "br" and "Vary":
"Accept-Encoding" whenever a ".br" file is detected), preserving existing logic
for isNextStaticAsset(filePath) and the api catalog Link header in headersFor.
- Around line 159-178: The responses currently send headers from
headersFor(finalPath) but omit Content-Length even though a prior stat() call
yields details.size; update the handler to call fs.statSync (or reuse the
existing stat result) for finalPath and add a "Content-Length" header with
details.size to the headers passed to response.writeHead (for both GET and HEAD
paths), ensuring the HEAD branch still ends after sending headers and the GET
branch streams via createReadStream(finalPath); reference the symbols finalPath,
headersFor, response.writeHead, method, and createReadStream when making the
change.
In `@src/components/home/link-creator.tsx`:
- Around line 36-79: The constants and small helpers used only in this
component—fieldHints, fieldPlaceholders, codecOptions, defaultLinkCreatorDraft,
and getBodyFieldLabel—should be moved into a shared module so they can be reused
and kept in sync; create a new module (e.g., link-creator-presets) exporting
those symbols, update src/components/home/link-creator.tsx to import fieldHints,
fieldPlaceholders, codecOptions, defaultLinkCreatorDraft, and getBodyFieldLabel
from that module, and ensure any types (ArtifactKind, LinkCreatorDraft) remain
imported or re-exported as needed so behavior of the existing functions
(getBaseUrl, numberFormatter, etc.) is unchanged.
In `@src/components/renderers/diff-renderer.tsx`:
- Around line 198-201: The stylesheet globals (diffViewStylesheetPromise and
DIFF_VIEW_STYLESHEET_ID) are not reference-counted, so
removeDiffViewStylesheet() can remove the shared <link> while another
DiffRenderer still needs it; add a module-level counter (e.g.,
diffViewInstanceCount) and update the lifecycle helpers so mounting a rich
renderer increments the count and unmounting decrements it, and change
removeDiffViewStylesheet() to only remove the DOM node and clear
diffViewStylesheetPromise when the count reaches 0; update the places that
previously called removeDiffViewStylesheet() (the effect in DiffRenderer that
tears down the fallback) to use the new increment/decrement semantics so the
stylesheet is retained while any instance remains.
- Around line 621-642: The current useEffect in diff-renderer.tsx swallows
stylesheet load errors; update the loadDiffViewStylesheet promise handling so
failures are surfaced and you don't silently mark styles as ready: call
loadDiffViewStylesheet().then(() => { if (!cancelled) setStylesReady(true)
}).catch((err) => { console.warn('Failed to load diff stylesheet', err); if
(!cancelled) { removeDiffViewStylesheet(); /* fall back to raw/unstyled diff */
setStylesReady(true); } }); keep the cancelled guard and the existing checks
around renderedDiff, and reference renderedDiff, loadDiffViewStylesheet,
removeDiffViewStylesheet, setStylesReady and diffFilesHaveRenderableFile to
locate the code.
In `@src/lib/diff/git-patch.ts`:
- Around line 48-74: parsePatchSections currently drops any leading preamble
before the first DIFF_SECTION_HEADER_RE match (e.g., mbox "From ..." headers);
update the loop in parsePatchSections so when the first match has match.index >
0 you call parsePatchSection(normalized.slice(0, match.index).trim(),
sectionIndex) (using parsePatchSection and the current sectionIndex), increment
sectionIndex, and then set previousStart = match.index; keep the existing logic
for subsequent matches and the final push so all text before the first "diff
--git" is preserved.
In `@tests/envelope.test.ts`:
- Around line 13-25: Add a regression test that exercises the new code path
rejecting envelopes with zero artifacts: create a variant of baseEnvelope where
artifacts is an empty array and call normalizeEnvelope; assert the result is a
failure (result.ok is false) and optionally assert the error or rejection reason
exists. Place this alongside the existing tests (e.g., near the "normalizes
single-artifact bundles..." test) and reference normalizeEnvelope and
baseEnvelope so the CI covers the new artifacts.length === 0 branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e70ead9-0bf9-4889-9914-f182ec60c6b6
⛔ Files ignored due to path filters (8)
package-lock.jsonis excluded by!**/package-lock.jsontests/e2e/visual.spec.ts-snapshots/code-light-chromium.pngis excluded by!**/*.pngtests/e2e/visual.spec.ts-snapshots/csv-compact-light-chromium.pngis excluded by!**/*.pngtests/e2e/visual.spec.ts-snapshots/diff-light-chromium.pngis excluded by!**/*.pngtests/e2e/visual.spec.ts-snapshots/empty-state-light-chromium.pngis excluded by!**/*.pngtests/e2e/visual.spec.ts-snapshots/json-light-chromium.pngis excluded by!**/*.pngtests/e2e/visual.spec.ts-snapshots/markdown-dark-chromium.pngis excluded by!**/*.pngtests/e2e/visual.spec.ts-snapshots/markdown-light-chromium.pngis excluded by!**/*.png
📒 Files selected for processing (81)
README.mddocs/architecture.mddocs/dependency-notes.mddocs/deployment.mddocs/payload-format.mddocs/testing.mdeslint.config.mjsnext-env.d.tsnext.config.tspackage.jsonplaywright.config.tspublic/_headerspublic/arx2-dictionary.jsonpublic/arx2-dictionary.json.brpublic/vendor/diff-view-pure.csspublic/vendor/diff-view-pure.css.brscripts/bench-baseline.jsonscripts/bench-codecs.mjsscripts/check-build-budgets.mjsscripts/clean-build-output.mjsscripts/compress-dictionary.mjsscripts/ensure-next-types.mjsscripts/serve-export.mjsselfhosted/server.tsskills/agent-render-linking/SKILL.mdsrc/app/globals.csssrc/app/layout.tsxsrc/app/security/page.tsxsrc/app/url-explainer/page.tsxsrc/components/home/link-creator.tsxsrc/components/home/sample-link-data.tssrc/components/home/sample-links.tsxsrc/components/renderers/code-renderer.tsxsrc/components/renderers/csv-renderer.tsxsrc/components/renderers/diff-renderer.tsxsrc/components/renderers/json-renderer.tsxsrc/components/renderers/markdown-renderer.tsxsrc/components/renderers/mermaid-block.tsxsrc/components/theme-provider.tsxsrc/components/theme-toggle.tsxsrc/components/theme/use-theme-controller.tssrc/components/viewer-shell.tsxsrc/components/viewer/artifact-selector.tsxsrc/components/viewer/artifact-stage.tsxsrc/components/viewer/fragment-details-disclosure.tsxsrc/lib/code/language.tssrc/lib/diff/git-patch.tssrc/lib/payload/arx-codec.tssrc/lib/payload/envelope.tssrc/lib/payload/fragment-arx.tssrc/lib/payload/fragment.tssrc/lib/payload/link-creator.tssrc/lib/payload/schema.tssrc/lib/payload/wire-format.tssrc/lib/site/base-path.tssrc/lib/site/canonical-base.tssrc/lib/utils.tstests/arx-codec.test.tstests/base-path.test.tstests/code-language.test.tstests/components/code-renderer.test.tsxtests/components/csv-renderer.test.tsxtests/components/diff-renderer.test.tsxtests/components/json-renderer.test.tsxtests/components/link-creator.test.tsxtests/components/markdown-renderer.test.tsxtests/components/mermaid-block.test.tsxtests/components/theme-toggle.test.tsxtests/components/viewer-shell-artifact-select.test.tsxtests/components/viewer-shell.test.tsxtests/diff-style-asset.test.tstests/e2e/viewer.spec.tstests/envelope.test.tstests/fragment.test.tstests/git-patch.test.tstests/headers.test.tstests/sample-link-data.test.tstests/selfhosted/api-catalog.test.tstests/setup.tsxtests/utils.test.tstsconfig.json
💤 Files with no reviewable changes (3)
- src/components/viewer/fragment-details-disclosure.tsx
- tests/setup.tsx
- src/components/theme-provider.tsx
| .diff-tailwindcss-wrapper .border-spacing-0 { | ||
| --tw-border-spacing-x: 0px; | ||
| --tw-border-spacing-y: 0px; | ||
| border-spacing: var(--tw-border-spacing-x) var(--tw-border-spacing-y); |
There was a problem hiding this comment.
Stylelint violations in vendored CSS should be resolved or excluded from lint scope.
This file currently contains stylelint-reported violations (declaration-empty-line-before and keyword casing at Line 200). Please either normalize the file to match the active rules or exclude public/vendor/** from stylelint targeting to keep checks stable.
Also applies to: 145-145, 149-149, 153-153, 200-200, 252-252, 656-656
🧰 Tools
🪛 Stylelint (17.11.0)
[error] 138-138: Expected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@public/vendor/diff-view-pure.css` at line 138, The vendored CSS triggers
stylelint errors (e.g., declaration-empty-line-before and keyword-case) for
lines like the rule containing "border-spacing: var(--tw-border-spacing-x)
var(--tw-border-spacing-y);"—either normalize the file to comply with our lint
rules (fix empty-line-before formatting around declarations and normalize
keyword casing to lowercase across the affected selectors/rules) or exclude the
vendored files from linting by adding "public/vendor/**" to the stylelint ignore
pattern in the Stylelint config; update only the code around the offending
declarations or the stylelint config accordingly and ensure the change
references the same symbols (the border-spacing declaration and the listed
offending rules) so CI stops failing.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/serve-export.mjs (1)
156-162:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid the unchecked
stat()afterexistsSync().This adds a TOCTOU window: if the file disappears or is replaced between those calls, the request handler throws instead of returning a normal 404.
Suggested fix
- if (!existsSync(finalPath)) { - response.writeHead(404); - response.end("Not found"); - return; - } - - const finalDetails = await stat(finalPath); + let finalDetails; + try { + finalDetails = await stat(finalPath); + } catch { + response.writeHead(404); + response.end("Not found"); + return; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/serve-export.mjs` around lines 156 - 162, Replace the unsafe existsSync() + stat() pattern with a single awaited stat() inside a try/catch: call stat(finalPath) in a try block, on success verify finalDetails.isFile() (or other required checks) and proceed, and on any error (ENOENT or other) catch it and respond with response.writeHead(404); response.end("Not found"); return; this eliminates the TOCTOU window and ensures missing or inaccessible files are handled safely.
🧹 Nitpick comments (3)
tests/components/viewer-shell-artifact-select.test.tsx (1)
106-129: ⚡ Quick winConsider more robust hash parsing in the mock.
The
activeArtifactIdderivation at line 112 uses simple string includes (hash.includes("three")), which is fragile. If hash formats evolve or if these substrings appear elsewhere, the mock could return unexpected artifacts. Consider using a more explicit pattern like splitting on delimiters or regex matching.♻️ Example: More robust parsing
- const activeArtifactId = hash.includes("three") ? "three" : hash.includes("two") ? "two" : "one"; + // Extract artifact ID from hash pattern: agent-render=v1.plain.<artifactId> + const match = hash.match(/agent-render=v1\.plain\.(\w+)/); + const activeArtifactId = match?.[1] ?? "one";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/components/viewer-shell-artifact-select.test.tsx` around lines 106 - 129, The mock's activeArtifactId derivation in decodeFragmentAsync is fragile because it uses hash.includes(...); change it to explicitly parse the artifact id (e.g., use a regex like /(?:^|[:\-])(?:one|two|three)(?:$|[:\-])/, or split the hash on a known delimiter and pick the token) so decodeFragmentAsync deterministically maps hashes to "one", "two", or "three" and update any tests expecting createEnvelope(activeArtifactId) accordingly; reference the decodeFragmentAsync mock, fragmentMock.decodes, and createEnvelope to locate where to apply this parsing fix.tests/components/link-creator.test.tsx (2)
17-23: ⚡ Quick winConsider adding error handling tests.
The mock includes a
rejectcallback but no tests verify error handling behavior. Consider adding a test that rejects a generation request to ensure the component handles errors gracefully.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/components/link-creator.test.tsx` around lines 17 - 23, The mock for createGeneratedArtifactLinkAsync exposes a reject callback via generationMock.pending but no test exercises it; add a test that triggers rejection (call the pending item's reject with an Error or rejection value) after initiating generation and then assert the LinkCreator component's error handling — e.g., that an error message is rendered or an error state callback is invoked. Target the mocked function createGeneratedArtifactLinkAsync and the generationMock.pending queue to perform the rejection, await any async UI updates (e.g., using findByText or waitFor) and verify the expected error UI/behavior, then clean up/reset mocks.
1-119: ⚡ Quick winConsider error handling and visual state verification.
While E2E Playwright tests cover LinkCreator workflows (tests/e2e/viewer.spec.ts), the unit tests verify visual assertions (textarea content, warning message visibility) without corresponding Playwright coverage for those specific state changes. Additionally, error handling is not tested—the mock's
rejectfunction is never invoked, leaving error scenarios uncovered.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/components/link-creator.test.tsx` around lines 1 - 119, Tests for LinkCreator lack coverage for rejection/error flows and miss asserting visual states after errors; update tests in link-creator.test.tsx to simulate and assert those cases by using generationMock.pending[].reject(...) to trigger createGeneratedArtifactLinkAsync failure and then verify UI reacts (e.g., an error message, disabled/cleared Generated agent-render link textarea, and any visual error state) and also add assertions after UI interactions (like clicking format/quality buttons) that visual state remains consistent; reference generationMock.pending, createGeneratedLink, and the LinkCreator component to locate where to add a new it() block(s) that rejects pending promises and asserts the expected DOM changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/serve-export.mjs`:
- Around line 97-103: The code slices the percent-encoded request URL (urlPath)
into cleanPath and assigns it to relativePath, so filesystem lookups will see
raw percent sequences; fix by decoding the path before resolving on disk:
compute cleanPath = urlPath.slice(0, pathEnd) and then set relativePath =
decodeURIComponent(cleanPath) (wrap decodeURIComponent in try/catch if you want
to handle malformed encodings). Apply the same decoding change to the other
occurrence referenced (lines around 121-122) so all file-resolution uses of
relativePath operate on a decoded path.
- Around line 91-94: The current isInsideOutputDirectory(filePath) check can be
bypassed via symlinks; update the check to resolve and compare real filesystem
targets instead of raw path strings (or alternatively reject any symlink segment
while walking the path). Specifically, in isInsideOutputDirectory and the
similar logic around the other export checks (the block at lines 122-128), call
fs.promises.realpath() (or synchronous realpath if appropriate) on both
outputDirectory and the resolved filePath and then ensure the file's real path
startsWith the outputDirectory real path; or walk the filePath with fs.lstat()
and fail if any path segment is a symbolic link before performing
stat/createReadStream. Ensure you reference and update isInsideOutputDirectory
and the corresponding export validation block to prevent symlink escape.
In `@src/components/renderers/diff-renderer.tsx`:
- Around line 20-23: Remove the Brotli sidecar entry from the stylesheet list:
update the diffViewStylesheetHrefs array (used where withBasePath is called) to
omit "/vendor/diff-view-pure.css.br" and only include the actual stylesheet path
("/vendor/diff-view-pure.css") so the renderer uses standard content-negotiated
CSS rather than attempting to load raw .br bytes as a stylesheet.
In `@src/components/renderers/markdown-renderer.tsx`:
- Around line 190-208: The markBlockReady callback currently reassigns
readyKeyRef.current and clears readyBlockIdsRef when it detects a mismatch,
which adopts a stale readyKey; instead, detect when readyKeyRef.current !==
readyKey and simply ignore/drop the callback by returning early. Update
markBlockReady (the function using readyKeyRef, readyKey, readyBlockIdsRef,
reportedReadyKeyRef, setReadyBlockState, embeddedBlockCount, and reportReady) so
it does not mutate refs/state on mismatch — just return — and keep the rest of
the logic (adding blockId, updating setReadyBlockState, and calling reportReady)
unchanged.
- Around line 35-50: The readiness counter undercounts fenced code blocks
because countEmbeddedCodeBlocks() uses EMBEDDED_CODE_BLOCK_PATTERN that
disallows up-to-3-space indented fences; replace the raw-regex counting approach
with deriving the embeddedBlockCount from actual rendered nodes instead: remove
or stop using EMBEDDED_CODE_BLOCK_PATTERN and instead increment the ready target
when ReactMarkdown/remarkGfm produces a code/mermaid/embedded block (e.g.,
inside the render path where EmbeddedCodeRenderer and MermaidBlock are
instantiated), then keep markBlockReady() logic unchanged so nextCount compares
against the real rendered block count rather than a fragile regex result.
In `@src/lib/diff/git-patch.ts`:
- Around line 64-69: The code currently converts leading preamble text into a
ParsedPatchFile by calling parsePatchSection(preamble, sectionIndex) and pushing
it into files; instead, do not promote preamble into the files array. Change the
branch that handles a non-empty preamble (the block using match.index,
normalized, preamble, parsePatchSection, and sectionIndex) to either discard the
preamble or collect it in a separate variable/array (e.g., preambleText or
preambleSections) for non-file metadata, and remove the files.push(...) call and
the sectionIndex increment so files remains strictly file-only.
In `@tests/components/markdown-renderer.test.tsx`:
- Around line 62-125: Add a real-browser Playwright test that exercises the
fenced-code presentation path instead of relying solely on mocks: create a new
Playwright test that mounts the MarkdownRenderer (or navigates to a page that
renders it) with a fenced code sample (e.g., "```ts\nconst a = 1;\n```") and
assert the visible language chip text ("ts") and the rendered code content
("const a = 1") in the browser DOM; reference the component name
MarkdownRenderer and the existing unit test cases in
tests/components/markdown-renderer.test.tsx as the examples to mirror, and place
the new test alongside other Playwright/e2e tests so CI will run it.
---
Outside diff comments:
In `@scripts/serve-export.mjs`:
- Around line 156-162: Replace the unsafe existsSync() + stat() pattern with a
single awaited stat() inside a try/catch: call stat(finalPath) in a try block,
on success verify finalDetails.isFile() (or other required checks) and proceed,
and on any error (ENOENT or other) catch it and respond with
response.writeHead(404); response.end("Not found"); return; this eliminates the
TOCTOU window and ensures missing or inaccessible files are handled safely.
---
Nitpick comments:
In `@tests/components/link-creator.test.tsx`:
- Around line 17-23: The mock for createGeneratedArtifactLinkAsync exposes a
reject callback via generationMock.pending but no test exercises it; add a test
that triggers rejection (call the pending item's reject with an Error or
rejection value) after initiating generation and then assert the LinkCreator
component's error handling — e.g., that an error message is rendered or an error
state callback is invoked. Target the mocked function
createGeneratedArtifactLinkAsync and the generationMock.pending queue to perform
the rejection, await any async UI updates (e.g., using findByText or waitFor)
and verify the expected error UI/behavior, then clean up/reset mocks.
- Around line 1-119: Tests for LinkCreator lack coverage for rejection/error
flows and miss asserting visual states after errors; update tests in
link-creator.test.tsx to simulate and assert those cases by using
generationMock.pending[].reject(...) to trigger createGeneratedArtifactLinkAsync
failure and then verify UI reacts (e.g., an error message, disabled/cleared
Generated agent-render link textarea, and any visual error state) and also add
assertions after UI interactions (like clicking format/quality buttons) that
visual state remains consistent; reference generationMock.pending,
createGeneratedLink, and the LinkCreator component to locate where to add a new
it() block(s) that rejects pending promises and asserts the expected DOM
changes.
In `@tests/components/viewer-shell-artifact-select.test.tsx`:
- Around line 106-129: The mock's activeArtifactId derivation in
decodeFragmentAsync is fragile because it uses hash.includes(...); change it to
explicitly parse the artifact id (e.g., use a regex like
/(?:^|[:\-])(?:one|two|three)(?:$|[:\-])/, or split the hash on a known
delimiter and pick the token) so decodeFragmentAsync deterministically maps
hashes to "one", "two", or "three" and update any tests expecting
createEnvelope(activeArtifactId) accordingly; reference the decodeFragmentAsync
mock, fragmentMock.decodes, and createEnvelope to locate where to apply this
parsing fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4722b931-85f1-43b5-a188-c9b3689c7907
📒 Files selected for processing (14)
docs/dependency-notes.mdscripts/bench-codecs.mjsscripts/serve-export.mjssrc/components/renderers/diff-renderer.tsxsrc/components/renderers/markdown-renderer.tsxsrc/components/theme/use-theme-controller.tssrc/components/viewer-shell.tsxsrc/lib/diff/git-patch.tstests/arx-codec.test.tstests/components/link-creator.test.tsxtests/components/markdown-renderer.test.tsxtests/components/viewer-shell-artifact-select.test.tsxtests/envelope.test.tstests/git-patch.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/envelope.test.ts
- src/components/theme/use-theme-controller.ts
- tests/arx-codec.test.ts
- src/components/viewer-shell.tsx
- scripts/bench-codecs.mjs
| const diffViewStylesheetHrefs = [ | ||
| withBasePath("/vendor/diff-view-pure.css.br"), | ||
| withBasePath("/vendor/diff-view-pure.css"), | ||
| ]; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Do browsers automatically decompress a .css.brfile when it is linked via, or is Brotli only applied when the response is served with Content-Encoding: brfor the.css URL?
💡 Result:
Browsers do not decide “this is Brotli because the URL ends with.br (e.g. /styles.css.br)”. Brotli is only applied when the HTTP response indicates that the body is Brotli-compressed via the response header Content-Encoding: br (selected via Accept-Encoding negotiation). This is independent of how you name the file; the extension by itself doesn’t trigger decompression. When you link the browser will fetch that URL as a stylesheet. If the server returns the resource with Content-Encoding: br, the browser will decompress it to obtain the underlying CSS. If Content-Encoding: br is missing (or wrong), the browser will treat the bytes as-is and CSS parsing will fail (or you’ll get an invalid resource / decoding error). Content-Encoding is explicitly defined as telling the recipient how to decode the response body, and the client negotiates supported encodings with Accept-Encoding; the server then signals the chosen encoding with Content-Encoding.[1][2] Practical implication for precompressed static assets: servers commonly serve the precompressed.br variant only to clients that accept br, and they do so by rewriting to the.br file and setting Content-Encoding: br on the response.[3] So, to answer your question directly: - Yes: a browser can decompress a linked /styles.css.br file, but only because the response has Content-Encoding: br. - No: Brotli is not automatically inferred solely from the filename (.css.br) or the tag.
Citations:
- 1: http://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Content-Encoding
- 2: https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Accept-Encoding
- 3: https://css-tricks.com/brotli-static-compression/
🏁 Script executed:
rg -n "diffViewStylesheetHrefs" src/components/renderers/diff-renderer.tsx -A 5 -B 2Repository: baanish/agent-render
Length of output: 655
🏁 Script executed:
rg -n "loadStylesheetHref" src/components/renderers/diff-renderer.tsx -A 10 -B 2Repository: baanish/agent-render
Length of output: 910
🏁 Script executed:
sed -n '126,150p' src/components/renderers/diff-renderer.tsxRepository: baanish/agent-render
Length of output: 831
Remove the Brotli sidecar from the stylesheet array.
Linking /vendor/diff-view-pure.css.br via <link rel="stylesheet"> requests the file directly without Content-Encoding: br headers. The server returns the file with HTTP 200, the browser's load event fires, and this prevents fallback to the actual stylesheet—leaving the rich diff unstyled while attempting to parse raw Brotli bytes as CSS.
Brotli decompression only occurs when the server explicitly sends Content-Encoding: br for the .css file; the .br filename extension alone has no effect. Rely on server-side content negotiation instead.
Suggested change
const diffViewStylesheetHrefs = [
- withBasePath("/vendor/diff-view-pure.css.br"),
withBasePath("/vendor/diff-view-pure.css"),
];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const diffViewStylesheetHrefs = [ | |
| withBasePath("/vendor/diff-view-pure.css.br"), | |
| withBasePath("/vendor/diff-view-pure.css"), | |
| ]; | |
| const diffViewStylesheetHrefs = [ | |
| withBasePath("/vendor/diff-view-pure.css"), | |
| ]; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/renderers/diff-renderer.tsx` around lines 20 - 23, Remove the
Brotli sidecar entry from the stylesheet list: update the
diffViewStylesheetHrefs array (used where withBasePath is called) to omit
"/vendor/diff-view-pure.css.br" and only include the actual stylesheet path
("/vendor/diff-view-pure.css") so the renderer uses standard content-negotiated
CSS rather than attempting to load raw .br bytes as a stylesheet.
| const EMBEDDED_CODE_BLOCK_PATTERN = /(?:^|\n)```[^\n]*\n[\s\S]*?\n```(?=\n|$)/g; | ||
| const LANGUAGE_CLASS_PREFIX = "language-"; | ||
|
|
||
| type ReadyBlockState = { | ||
| key: string; | ||
| count: number; | ||
| }; | ||
|
|
||
| function countEmbeddedCodeBlocks(content: string): number { | ||
| EMBEDDED_CODE_BLOCK_PATTERN.lastIndex = 0; | ||
| let count = 0; | ||
| while (EMBEDDED_CODE_BLOCK_PATTERN.exec(content)) { | ||
| count += 1; | ||
| } | ||
| return count; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file and check its structure
find . -type f -name "markdown-renderer.tsx" 2>/dev/nullRepository: baanish/agent-render
Length of output: 113
🏁 Script executed:
# Read the markdown-renderer.tsx file
cat -n ./src/components/renderers/markdown-renderer.tsxRepository: baanish/agent-render
Length of output: 10893
The readiness counter undercounts fenced blocks when the regex pattern is stricter than the parsed markdown format.
countEmbeddedCodeBlocks() uses a regex that requires zero indentation before the opening fence ((?:^|\n)```), but the GFM spec allows up to 3 spaces of indentation. ReactMarkdown with remarkGfm parses these indented fences, so blocks that match the spec but not the regex will still render and call markBlockReady()afterdata-renderer-readyhas already flipped to "true" (once the undercountednextCount >= embeddedBlockCount at line 205). This causes the ready state to signal completion while async renderers (EmbeddedCodeRenderer, MermaidBlock`) are still loading. Derive the ready count from the actual rendered blocks instead of re-parsing raw markdown with a narrower regex.
🧰 Tools
🪛 OpenGrep (1.20.0)
[ERROR] 46-46: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/renderers/markdown-renderer.tsx` around lines 35 - 50, The
readiness counter undercounts fenced code blocks because
countEmbeddedCodeBlocks() uses EMBEDDED_CODE_BLOCK_PATTERN that disallows
up-to-3-space indented fences; replace the raw-regex counting approach with
deriving the embeddedBlockCount from actual rendered nodes instead: remove or
stop using EMBEDDED_CODE_BLOCK_PATTERN and instead increment the ready target
when ReactMarkdown/remarkGfm produces a code/mermaid/embedded block (e.g.,
inside the render path where EmbeddedCodeRenderer and MermaidBlock are
instantiated), then keep markBlockReady() logic unchanged so nextCount compares
against the real rendered block count rather than a fragile regex result.
| describe("MarkdownRenderer", () => { | ||
| it("renders language-less fenced code blocks with the text fallback language", async () => { | ||
| const { container } = render(<MarkdownRenderer artifact={createArtifact({ content: "```\nplain text\n```" })} />); | ||
|
|
||
| await waitFor(() => { | ||
| expect( | ||
| Array.from(container.querySelectorAll(".markdown-code-chip")).map((node) => node.textContent), | ||
| ).toEqual(["text"]); | ||
| expect(container).toHaveTextContent("plain text"); | ||
| }); | ||
| }); | ||
|
|
||
| it("renders every fenced code block with its language chip", async () => { | ||
| const { container } = render( | ||
| <MarkdownRenderer | ||
| artifact={createArtifact({ | ||
| content: "```ts\nconst a = 1;\n```\n\n```json\n{\"ok\":true}\n```", | ||
| })} | ||
| />, | ||
| ); | ||
|
|
||
| await waitFor(() => { | ||
| expect( | ||
| Array.from(container.querySelectorAll(".markdown-code-chip")).map((node) => node.textContent), | ||
| ).toEqual(["ts", "json"]); | ||
| expect(container).toHaveTextContent("const a = 1"); | ||
| expect(container).toHaveTextContent('{"ok":true}'); | ||
| }); | ||
| }); | ||
|
|
||
| it("preserves non-alphanumeric fenced code language ids", async () => { | ||
| const { container } = render( | ||
| <MarkdownRenderer | ||
| artifact={createArtifact({ | ||
| content: "```c++\nint main() {}\n```\n\n```c#\nConsole.WriteLine();\n```", | ||
| })} | ||
| />, | ||
| ); | ||
|
|
||
| await waitFor(() => { | ||
| expect( | ||
| Array.from(container.querySelectorAll(".markdown-code-chip")).map((node) => node.textContent), | ||
| ).toEqual(["c++", "c#"]); | ||
| }); | ||
| }); | ||
|
|
||
| it("does not wait forever on tilde blocks that are not mounted as embedded renderers", async () => { | ||
| const onReady = vi.fn(); | ||
|
|
||
| render( | ||
| <MarkdownRenderer | ||
| artifact={createArtifact({ | ||
| content: "~~~json\n{\"ok\":true}\n~~~", | ||
| })} | ||
| onReady={onReady} | ||
| />, | ||
| ); | ||
|
|
||
| await waitFor(() => { | ||
| expect(screen.getByTestId("renderer-markdown")).toHaveAttribute("data-renderer-ready", "true"); | ||
| expect(onReady).toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add one real-browser Playwright case for the fenced-code presentation path.
These assertions run entirely through mocks, so they won't catch regressions in the actual dynamic renderer/chip rendering path. Please mirror at least one fenced-code example in Playwright as well.
As per coding guidelines, "Verify visual changes with Playwright when they affect layout or renderer presentation in test files".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/components/markdown-renderer.test.tsx` around lines 62 - 125, Add a
real-browser Playwright test that exercises the fenced-code presentation path
instead of relying solely on mocks: create a new Playwright test that mounts the
MarkdownRenderer (or navigates to a page that renders it) with a fenced code
sample (e.g., "```ts\nconst a = 1;\n```") and assert the visible language chip
text ("ts") and the rendered code content ("const a = 1") in the browser DOM;
reference the component name MarkdownRenderer and the existing unit test cases
in tests/components/markdown-renderer.test.tsx as the examples to mirror, and
place the new test alongside other Playwright/e2e tests so CI will run it.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scripts/serve-export.mjs (1)
146-153:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDecode the request path before route lookup.
Line 152 still keeps
%xxescapes intact, so requests like/images/My%20File.pngor non-ASCII filenames won't match the precomputed route map and will 404.Suggested fix
- const cleanPath = urlPath.slice(0, pathEnd); + let cleanPath; + try { + cleanPath = decodeURIComponent(urlPath.slice(0, pathEnd)); + } catch { + return null; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/serve-export.mjs` around lines 146 - 153, The toRoutePath function currently slices the raw URL and leaves percent-escapes intact, causing mismatches for encoded filenames; decode the cleaned path (cleanPath) using decodeURIComponent before building routePath so `/images/My%20File.png` matches the precomputed routes, and wrap the decode in a try/catch to fall back to the original cleanPath if decodeURIComponent throws (handle malformed percent-encodings), then ensure the decoded value still gets normalized to start with a leading slash when assigning routePath.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/serve-export.mjs`:
- Around line 193-199: The code sends a 200 response before ensuring the file
stream is readable; change the flow so you do not call response.writeHead(200,
...) before the ReadStream opens and add an error handler on the stream: for
method === "HEAD" you can still call response.writeHead(200, headersFor(...))
and response.end() immediately, but for GET create the
ReadStream(staticFile.filePath), attach stream.once('open', () => {
response.writeHead(200, headersFor(staticFile.filePath, staticFile.size));
stream.pipe(response); }), and attach stream.on('error', err => {
response.destroy(); response.writeHead(err.code === 'ENOENT' ? 404 : 500);
response.end(); }) so missing/unreadable files don’t cause a broken 200 response
or crash.
---
Duplicate comments:
In `@scripts/serve-export.mjs`:
- Around line 146-153: The toRoutePath function currently slices the raw URL and
leaves percent-escapes intact, causing mismatches for encoded filenames; decode
the cleaned path (cleanPath) using decodeURIComponent before building routePath
so `/images/My%20File.png` matches the precomputed routes, and wrap the decode
in a try/catch to fall back to the original cleanPath if decodeURIComponent
throws (handle malformed percent-encodings), then ensure the decoded value still
gets normalized to start with a leading slash when assigning routePath.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| response.writeHead(200, headersFor(staticFile.filePath, staticFile.size)); | ||
| if (method === "HEAD") { | ||
| response.end(); | ||
| return; | ||
| } | ||
|
|
||
| createReadStream(finalPath).pipe(response); | ||
| createReadStream(staticFile.filePath).pipe(response); |
There was a problem hiding this comment.
Don't send 200 before the file stream opens.
Line 199 pipes a ReadStream without an error handler after the 200 headers are already sent. If the file disappears or becomes unreadable after startup, the preview server can crash or return a broken 200 response.
Suggested fix
- response.writeHead(200, headersFor(staticFile.filePath, staticFile.size));
- if (method === "HEAD") {
- response.end();
- return;
- }
-
- createReadStream(staticFile.filePath).pipe(response);
+ if (method === "HEAD") {
+ response.writeHead(200, headersFor(staticFile.filePath, staticFile.size));
+ response.end();
+ return;
+ }
+
+ const stream = createReadStream(staticFile.filePath);
+ stream.once("error", () => {
+ if (response.headersSent) {
+ response.destroy();
+ return;
+ }
+ response.writeHead(500);
+ response.end("Internal server error");
+ });
+ stream.once("open", () => {
+ response.writeHead(200, headersFor(staticFile.filePath, staticFile.size));
+ stream.pipe(response);
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| response.writeHead(200, headersFor(staticFile.filePath, staticFile.size)); | |
| if (method === "HEAD") { | |
| response.end(); | |
| return; | |
| } | |
| createReadStream(finalPath).pipe(response); | |
| createReadStream(staticFile.filePath).pipe(response); | |
| if (method === "HEAD") { | |
| response.writeHead(200, headersFor(staticFile.filePath, staticFile.size)); | |
| response.end(); | |
| return; | |
| } | |
| const stream = createReadStream(staticFile.filePath); | |
| stream.once("error", () => { | |
| if (response.headersSent) { | |
| response.destroy(); | |
| return; | |
| } | |
| response.writeHead(500); | |
| response.end("Internal server error"); | |
| }); | |
| stream.once("open", () => { | |
| response.writeHead(200, headersFor(staticFile.filePath, staticFile.size)); | |
| stream.pipe(response); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/serve-export.mjs` around lines 193 - 199, The code sends a 200
response before ensuring the file stream is readable; change the flow so you do
not call response.writeHead(200, ...) before the ReadStream opens and add an
error handler on the stream: for method === "HEAD" you can still call
response.writeHead(200, headersFor(...)) and response.end() immediately, but for
GET create the ReadStream(staticFile.filePath), attach stream.once('open', () =>
{ response.writeHead(200, headersFor(staticFile.filePath, staticFile.size));
stream.pipe(response); }), and attach stream.on('error', err => {
response.destroy(); response.writeHead(err.code === 'ENOENT' ? 404 : 500);
response.end(); }) so missing/unreadable files don’t cause a broken 200 response
or crash.
|
| Filename | Overview |
|---|---|
| src/lib/payload/fragment-arx.ts | New module splitting ARX candidate-building and decoding out of fragment.ts; introduces dictionary load caching with module-level promises and the intentional double-try decode pattern for backward compatibility. |
| src/lib/payload/arx-codec.ts | Adds arx3 compress/decompress, arxCompressPayloads (compress-once, encode-four-ways), brotli-first dictionary fallback fetch, and trie-reversed parameter replacing the extra map allocation. |
| src/lib/payload/fragment.ts | ARX imports removed; build-candidate functions now dynamic-import fragment-arx.ts; arx3 plumbed through codec selection; minor hot-path optimisations (loop-based base64, codepoint integer checks). |
| scripts/serve-export.mjs | Static file map pre-built at startup via collectStaticFiles; method guard moved before I/O; Content-Length and brotli headers added; basePath normalization refactored. Startup will hard-crash if out/ is absent rather than returning 404. |
| scripts/check-build-budgets.mjs | New CI budget gate measuring gzip size of route and react-loadable chunks; self-validates by throwing on unknown manifest keys. |
| src/components/viewer/artifact-stage.tsx | New component extracting renderer loading, toolbar, copy/download, and raw-source view from viewer-shell.tsx for improved code-splitting. |
| src/lib/site/base-path.ts | New utility normalising NEXT_PUBLIC_BASE_PATH and providing withBasePath; replaces duplicated inline env-var reads across arx-codec and viewer-shell. |
| selfhosted/server.ts | Adds .json.br and .css.br special-case Content-Encoding/Content-Type, Cache-Control for _next/static assets, and isNextStaticAsset helper; path traversal check improved with pre-computed outputDirectoryWithSeparator. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
VS[viewer-shell.tsx] -->|dynamic import| AS[artifact-stage.tsx]
VS -->|cached promise| FM[fragment.ts]
FM -->|sync codecs| SC[plain / deflate / lz]
FM -->|dynamic import| FAX[fragment-arx.ts]
FAX -->|ensureArxLoaded| AC[arx-codec.ts]
FAX -->|ensureArx2Loaded| AC
AC -->|fetch with .br fallback| DICT[arx-dictionary.json]
AC -->|fetch with .br fallback| DICT2[arx2-dictionary.json]
AC -->|arxCompressPayloads| WP[ArxWirePayloads: base76 / base1k / baseBMP / base64url]
FAX -->|buildArxCandidates| WP
FAX -->|buildArx2Candidates| WP
FAX -->|buildArx3Candidates
prefers baseBMP visible len| WP
BP[base-path.ts
normalizeBasePath / withBasePath] -->|resolves URL| DICT
BP -->|resolves URL| DICT2
SX[serve-export.mjs] -->|collectStaticFiles at startup| SF[staticFiles Map]
SF -->|pre-computed aliases| ROUTES[/ /index.html /page /page/]
Reviews (2): Last reviewed commit: "Add ARX3 homepage sample and auto-encode..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (filePath.endsWith(".json.br")) { | ||
| return "application/json; charset=utf-8"; | ||
| } | ||
|
|
||
| if (filePath.endsWith(".css.br")) { | ||
| return "text/css; charset=utf-8"; | ||
| } | ||
|
|
||
| return contentTypes.get(path.extname(filePath)) || "application/octet-stream"; | ||
| } | ||
|
|
||
| function isNextStaticAsset(filePath: string): boolean { | ||
| return filePath.includes(`${path.sep}_next${path.sep}static${path.sep}`); | ||
| } | ||
|
|
||
| function headersFor(filePath: string): Record<string, string> { | ||
| const headers: Record<string, string> = { "Content-Type": contentTypeFor(filePath) }; | ||
|
|
||
| if (filePath.endsWith(`${path.sep}.well-known${path.sep}api-catalog`)) { | ||
| headers.Link = API_CATALOG_LINK_HEADER; | ||
| } | ||
|
|
||
| if (filePath.endsWith(".json.br") || filePath.endsWith(".css.br")) { | ||
| headers["Content-Encoding"] = "br"; | ||
| headers.Vary = "Accept-Encoding"; | ||
| } | ||
|
|
||
| if (isNextStaticAsset(filePath)) { | ||
| headers["Cache-Control"] = "public, max-age=31536000, immutable"; | ||
| } | ||
|
|
||
| return headers; | ||
| } |
There was a problem hiding this comment.
serve-export.mjs is generic
serve-export.mjs handles every .br file by stripping the extension to determine content type and unconditionally sets Content-Encoding: br. The selfhosted server only applies the correct content type and Content-Encoding: br for .json.br and .css.br. Any future pre-compressed file (e.g., a .wasm.br or .js.br) served through the selfhosted path would be returned as application/octet-stream without Content-Encoding: br, causing the browser to receive raw brotli bytes. Aligning the two servers to use the same generic extension-stripping approach would avoid this drift.
|
|
||
| if (parsedDictVersion !== null) { | ||
| try { | ||
| return await decodeArxAttempt(codec, decodeArxEncodedPayload(remainder)); | ||
| } catch (error) { | ||
| lastError = normalizeArxDecodeError(error); | ||
| } | ||
| } | ||
|
|
||
| try { | ||
| return await decodeArxAttempt(codec, decodeArxEncodedPayload(versionedPayload)); | ||
| } catch (error) { | ||
| lastError = normalizeArxDecodeError(error); | ||
| } | ||
|
|
||
| throw lastError ?? new Error("Failed to decode arx fragment"); | ||
| } |
There was a problem hiding this comment.
When parsedDictVersion !== null the function first attempts to decompress remainder (the full "<version>.<payload>" string). For any correctly-formed, version-prefixed ARX fragment this attempt will always fail at the decompressor level, the error is silently swallowed, and the second attempt with just versionedPayload succeeds. This double-try is preserved from the old fragment.ts code for backward compatibility with pre-versioning links where the payload might start with digits and a dot. The behavior is intentional, but the first failed decompress is wasted CPU on every modern fragment decode — a comment explaining why the full remainder is tried first would help readers not "fix" this.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/payload/schema.ts (1)
146-157:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
diff.viewat runtime inisPayloadEnvelope.
isPayloadEnvelopecurrently allows anyviewvalue on diff artifacts. Invalid values should fail schema validation before mount.Proposed fix
if (artifact.kind === "diff") { const diffArtifact = artifact as { patch?: unknown; oldContent?: unknown; newContent?: unknown; + view?: unknown; }; if (!hasString(diffArtifact.patch) && (!hasString(diffArtifact.oldContent) || !hasString(diffArtifact.newContent))) { return false; } + if ( + diffArtifact.view !== undefined && + diffArtifact.view !== "unified" && + diffArtifact.view !== "split" + ) { + return false; + } continue; }As per coding guidelines,
src/lib/payload/{schema,envelope}.ts: "Fail clearly on malformed or oversized payloads before renderer mount when possible in envelope.ts and schema.ts" andsrc/lib/payload/schema.ts: "Supportviewproperty with valuesunifiedorsplitfor diff artifacts in schema.ts and diff-renderer.tsx".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/payload/schema.ts` around lines 146 - 157, The diff-artifact branch in isPayloadEnvelope currently doesn't validate the diff.view value; update the check in the artifact.kind === "diff" block (where diffArtifact is defined and hasString is used) to also validate diffArtifact.view and return false for any value other than "unified" or "split" (i.e., if view is present and not one of those two strings, return false). Ensure the combined condition still enforces the existing patch/oldContent+newContent requirement and uses the same hasString helper for view validation.
🧹 Nitpick comments (1)
scripts/bench-codecs.mjs (1)
514-533: ⚡ Quick winAdd decode round-trip assertions in the benchmark loop.
The loop measures decode time but never verifies decoded content matches the expected payload shape for each codec. A parseable-but-wrong decode can pass the benchmark gate unnoticed.
Proposed patch
for (const entry of corpus) { for (const codec of ["arx", "arx2", "arx3"]) { const [encoder, decoder] = codecImplementations[codec]; const rawJson = JSON.stringify({ ...entry.envelope, codec }); + const expectedDecoded = + codec === "arx" + ? { ...entry.envelope, codec } + : tupleEnvelope({ ...entry.envelope, codec }); const encodedInput = encoder(entry.envelope); const encode = measure(() => brotli(encodedInput)); const decode = measure(() => decoder(encode.result)); + if (JSON.stringify(decode.result) !== JSON.stringify(expectedDecoded)) { + failures.push(`${entry.name}:${codec} failed decode round-trip validation.`); + } rows.push({ id: `${entry.name}:${codec}`, codec,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/bench-codecs.mjs` around lines 514 - 533, Add a decode round-trip assertion after measuring decode in the corpus loop: after const decode = measure(() => decoder(encode.result)); verify the decoded value matches the original envelope (entry.envelope) using a deep-equality check (e.g. assert.deepStrictEqual or a JSON.stringify comparison) so any parseable-but-wrong output fails the benchmark; do the check outside the timed measure and before pushing the row, referencing encoder/decoder, measure, encode/decode results, and rows.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/payload/fragment-arx.ts`:
- Around line 39-40: The cached promise arxDictionaryLoadPromise can remain
rejected and block future loads; modify the usage where you set
arxDictionaryLoadPromise = loadArxDictionary()... so that you attach a .catch
handler which clears arxDictionaryLoadPromise (set it back to undefined/null)
before rethrowing the error, then await the promise; do the same fix for the
other occurrence around lines 50-53 so any transient failure resets the cache
and allows retries (refer to arxDictionaryLoadPromise and loadArxDictionary to
locate both spots).
---
Outside diff comments:
In `@src/lib/payload/schema.ts`:
- Around line 146-157: The diff-artifact branch in isPayloadEnvelope currently
doesn't validate the diff.view value; update the check in the artifact.kind ===
"diff" block (where diffArtifact is defined and hasString is used) to also
validate diffArtifact.view and return false for any value other than "unified"
or "split" (i.e., if view is present and not one of those two strings, return
false). Ensure the combined condition still enforces the existing
patch/oldContent+newContent requirement and uses the same hasString helper for
view validation.
---
Nitpick comments:
In `@scripts/bench-codecs.mjs`:
- Around line 514-533: Add a decode round-trip assertion after measuring decode
in the corpus loop: after const decode = measure(() => decoder(encode.result));
verify the decoded value matches the original envelope (entry.envelope) using a
deep-equality check (e.g. assert.deepStrictEqual or a JSON.stringify comparison)
so any parseable-but-wrong output fails the benchmark; do the check outside the
timed measure and before pushing the row, referencing encoder/decoder, measure,
encode/decode results, and rows.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 55f640dc-aa1c-4cf4-b628-a0f87ef607c0
⛔ Files ignored due to path filters (2)
tests/e2e/visual.spec.ts-snapshots/bundle-switcher-light-chromium.pngis excluded by!**/*.pngtests/e2e/visual.spec.ts-snapshots/json-light-chromium.pngis excluded by!**/*.png
📒 Files selected for processing (24)
README.mddocs/architecture.mddocs/dependency-notes.mddocs/payload-format.mddocs/testing.mddocs/url-fragments.mdscripts/bench-baseline.jsonscripts/bench-codecs.mjsskills/agent-render-linking/SKILL.mdskills/selfhosted-agent-render/SKILL.mdsrc/components/home/sample-link-data.tssrc/components/viewer-shell.tsxsrc/lib/payload/arx-codec.tssrc/lib/payload/examples.tssrc/lib/payload/fragment-arx.tssrc/lib/payload/fragment.tssrc/lib/payload/link-creator.tssrc/lib/payload/schema.tstests/arx-codec.test.tstests/e2e/viewer.spec.tstests/fixtures/baanish-code-bench-report.mdtests/fragment.test.tstests/link-creator.test.tstests/sample-link-data.test.ts
✅ Files skipped from review due to trivial changes (3)
- README.md
- src/lib/payload/examples.ts
- skills/agent-render-linking/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (6)
- src/components/home/sample-link-data.ts
- tests/e2e/viewer.spec.ts
- src/lib/payload/link-creator.ts
- src/components/viewer-shell.tsx
- src/lib/payload/fragment.ts
- src/lib/payload/arx-codec.ts
baanish
left a comment
baanish
left a comment
There was a problem hiding this comment.
Automated multi-agent review of the optimization pass. No correctness blockers — codecs round-trip, loop rewrites are behavior-preserving, raw rendering is XSS-safe, and the static-server traversal posture is improved. The inline comments below are the SHOULD-FIX items: two authoritative numbers that need fortifying into defended contracts, two duplicated helpers, one silent UX change to confirm, and a server-header test gap. I'm pushing fixes for all of these to this branch.
| value, | ||
| codec: "arx3", | ||
| packed: false, | ||
| transportLength: preferVisibleChars ? value.length : computeTransportLength(value), |
There was a problem hiding this comment.
SHOULD-FIX — arx3 baseBMP wins by yardstick, not by size. The baseBMP candidate is measured with raw value.length, while every other candidate in the shared pool — including arx2's byte-identical baseBMP payload — is measured with percent-escape-inflated computeTransportLength (~9x for BMP chars). Since selectCandidate picks the global minimum, arx3 essentially always wins via the inconsistent metric, not a real difference. This is the intended outcome, but per our 'fortify heuristics into defended contracts' bar it should be (1) an explicitly-owned policy in the doc-comment (the visible-length-wins decision and its consequence), and (2) pinned by a characterization test asserting arx3-baseBMP is selected over arx2 in auto mode. Today no test exercises that head-to-head.
| const reactLoadableManifestPath = join(nextDir, "react-loadable-manifest.json"); | ||
| const gzipSizeCache = new Map(); | ||
|
|
||
| const budgets = [ |
There was a problem hiding this comment.
SHOULD-FIX — budgets are undefended magic numbers. The four maxBytes ceilings carry no comment on why each was chosen, what regression each guards, or what a maintainer should do when one trips (tighten vs. accept-and-bump), and nothing pins the enforcement semantics. Per the 'defended contracts' bar: add a per-budget rationale comment naming the policy, and a characterization test pinning the boundary behavior (at-budget passes, over-budget fails) so a refactor that silently doubles a chunk is caught. No test currently references this script.
| const inspectorAnimationStyle: CSSProperties = { animationDelay: "220ms" }; | ||
| const initializeAnimationStyle: CSSProperties = { animationDelay: "260ms" }; | ||
|
|
||
| function getVisibleHashLength(hash: string): number { |
There was a problem hiding this comment.
SHOULD-FIX — duplicates the budget-length contract. getVisibleHashLength is byte-for-byte identical to the exported getVisibleFragmentLength in src/lib/payload/fragment.ts (same decodeURIComponent(...).length + try/catch). This is exactly the length metric the lib enforces at fragment time — it should have one owner. Import and reuse getVisibleFragmentLength; the switch from raw hash.length - 1 to decoded visible length is a genuine improvement, so keep the behavior, just dedupe the function.
There was a problem hiding this comment.
Retracting this after verifying against the build budget. getVisibleHashLength is a deliberate local copy, not an oversight: importing getVisibleFragmentLength statically pulls the fragment/codec module (lz-string, fflate) into the homepage initial chunk, which pushed check:build-budgets over (homepage 115.7 KiB > 115.0 KiB; the shell otherwise loads that module dynamically). Reverted the dedupe and added a comment documenting the bundle-splitting reason so it isn't re-flagged. Fixed in 32e3efc.
|
|
||
| const codecOptions = ["auto", ...codecs] as const; | ||
|
|
||
| const defaultLinkCreatorDraft: LinkCreatorDraft = { |
There was a problem hiding this comment.
SHOULD-FIX — duplicated source of truth. defaultLinkCreatorDraft and getBodyFieldLabel are redefined here but still exported (currently identical) from @/lib/payload/link-creator. They will drift. These constants don't pull in the encoder, so a static import from the lib is fine — only createGeneratedArtifactLinkAsync needs the lazy import() for chunk-splitting. Import them from the lib and delete the local copies (or remove the now-unused lib exports).
There was a problem hiding this comment.
Retracting — this was a bad observation on my part. At HEAD defaultLinkCreatorDraft/getBodyFieldLabel existed only in the component, not in @/lib/payload/link-creator, so there was no duplication to remove. Moving them into the lib and importing them statically also pulled link-creator.ts → fragment.ts (codec deps) into the homepage chunk and breached the build budget. Reverted in 32e3efc; the helpers stay local to the component.
| }, [onReady]); | ||
|
|
||
| return ( | ||
| <pre className="artifact-raw-source" data-testid={testId}> |
There was a problem hiding this comment.
SHOULD-FIX — confirm the raw-view de-highlighting is intentional. Raw mode for markdown/CSV previously rendered through CodeRenderer (CodeMirror-highlighted source); RawArtifactSource now emits a plain <pre>. Testids/e2e still pass, but it's a visible UX change (no highlighting) that's silent in the diff. If deliberate (lighter chunk, no CodeMirror on the raw path) — which is consistent with the optimization goal — call it out in a comment and pin it with a unit test asserting raw markdown/CSV does not mount CodeMirror, so it can't silently regress.
| } | ||
|
|
||
| if (filePath.endsWith(".br")) { | ||
| headers["Content-Encoding"] = "br"; |
There was a problem hiding this comment.
SHOULD-FIX — no server-level test pins the .br header contract. tests/headers.test.ts only asserts the static Cloudflare public/_headers file. Neither this script nor selfhosted/server.ts has a test asserting a .br request returns Content-Encoding: br + the decompressed Content-Type + Vary: Accept-Encoding (and that _next/static gets immutable). These are security-sensitive headers — worth pinning.
| /^diff --git /m.test(patch) || | ||
| (/^--- /m.test(patch) && /^\+\+\+ /m.test(patch)) || | ||
| /^@@ /m.test(patch) || | ||
| /^Binary files .* differ$/m.test(patch) || |
There was a problem hiding this comment.
NIT — CRLF binary-marker gate. These two $-anchored checks won't match on CRLF input (the \r sits before the line end), so a CRLF binary-only patch falls to the raw view instead of the rich view. Low impact, but cheap to fix: differ\r?$ / patch\r?$, or normalize once before testing.
…plitting Responds to the automated review of the optimization branch: - arx3: reframe the visible-length selection metric as an explicitly owned policy and pin it with a characterization test (arx3 baseBMP is chosen over arx2 by visible length, not a real byte-size difference). - build budgets: add per-budget rationale, extract a pure evaluateBudget helper (+ .d.mts types), and pin the policy table and boundary semantics. - viewer raw view: document the deliberate un-highlighted plain-<pre> render and pin that the raw path never mounts CodeMirror. - static servers: add tests asserting the precompressed (.br) header contract (Content-Encoding: br + decompressed Content-Type + Vary) and immutable _next/static caching, for serve-export and the self-hosted server. - diff renderer: make the binary-marker gate CR-tolerant, name the hash-based reset key as a deliberate bound, and add a CRLF binary-patch regression test. - examples: pin the showcase benchmark figures to scripts/bench-baseline.json. - viewer shell: document why getVisibleHashLength is intentionally a local copy — deduping it pulls the codec module (lz-string/fflate) into the homepage chunk and breaches check:build-budgets. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Too many files changed for review. ( |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| port = await getFreePort(); | ||
| child = spawn( | ||
| process.execPath, | ||
| ["--import", "tsx", path.join(repoRoot, "selfhosted", "server.ts")], |
There was a problem hiding this comment.
WARNING: The self-hosted server is spawned with cwd: repoRoot, so this fixture never points the process at the temporary export directory it just creates. That means the test is validating the real repository output rather than the isolated fixture, which makes the header contract assertion brittle and can hide regressions in the generated export tree.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (4 files)
Previous Review Summary (commit 32e3efc)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 32e3efc)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (4 files)
Reviewed by gpt-5.4-mini-2026-03-17 · 88,588 tokens |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/selfhosted/static-headers.test.ts (1)
19-95: ⚡ Quick winConsider extracting shared test helpers to reduce duplication.
The helper functions
getFreePort,createExportFixture,waitForReady, andstopServer(lines 19-95) are duplicated nearly verbatim intests/serve-export-headers.test.ts. Extracting these to a shared test utility module (e.g.,tests/helpers/server-testing.ts) would improve maintainability and reduce code duplication.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/selfhosted/static-headers.test.ts` around lines 19 - 95, Extract the helper functions getFreePort, createExportFixture, waitForReady, and stopServer from this test file and create a shared test utility module at tests/helpers/server-testing.ts. Move these four functions to the new module, then import and use them in both tests/selfhosted/static-headers.test.ts and tests/serve-export-headers.test.ts to eliminate the duplication. This will centralize the shared test infrastructure and make future maintenance easier.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/selfhosted/static-headers.test.ts`:
- Around line 19-95: Extract the helper functions getFreePort,
createExportFixture, waitForReady, and stopServer from this test file and create
a shared test utility module at tests/helpers/server-testing.ts. Move these four
functions to the new module, then import and use them in both
tests/selfhosted/static-headers.test.ts and tests/serve-export-headers.test.ts
to eliminate the duplication. This will centralize the shared test
infrastructure and make future maintenance easier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d83e75f-2fbc-416d-990f-091b67e32524
📒 Files selected for processing (14)
scripts/check-build-budgets.d.mtsscripts/check-build-budgets.mjssrc/components/renderers/diff-renderer.tsxsrc/components/viewer-shell.tsxsrc/components/viewer/artifact-stage.tsxsrc/lib/payload/examples.tssrc/lib/payload/fragment-arx.tstests/build-budgets.test.tstests/components/artifact-stage-raw.test.tsxtests/components/diff-renderer.test.tsxtests/examples-arx3-benchmark.test.tstests/fragment-arx-selection.test.tstests/selfhosted/static-headers.test.tstests/serve-export-headers.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/components/diff-renderer.test.tsx
- src/components/viewer/artifact-stage.tsx
- src/lib/payload/examples.ts
- src/components/renderers/diff-renderer.tsx
- src/components/viewer-shell.tsx
- src/lib/payload/fragment-arx.ts
…e, arx resilience - serve-export.mjs: decode percent-escaped request paths (files with spaces/ non-ASCII names now resolve) and attach a read-stream error handler so a file removed after startup tears down the response instead of crashing the server. - markdown-renderer: drop stale block-ready callbacks on key mismatch instead of adopting them into the new artifact's key (the readyKey effect already resets the tracking refs), avoiding a premature/incorrect onReady for new content. - fragment-arx: reset the cached dictionary-load promise on failure so a transient load error can retry; document the intentional pre-versioning double-decode. - selfhosted/server.ts: strip a trailing .br generically for content-type and Content-Encoding so future *.js.br / *.wasm.br don't regress to octet-stream (matches serve-export.mjs). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
isPayloadEnvelope validated diff content but not the optional `view` field, so a payload with `view` set to anything other than "unified"/"split" passed validation and reached the diff renderer with an unsupported value. Reject it at the schema boundary, matching the declared `view?: "unified" | "split"` type, with a regression test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
2 participants
Summary
Testing
npm run checknpm run test:e2egit diff --checkSummary by CodeRabbit
arx3fragment codec support with improved visible-length selection and updated ARX/ARX2 payload behavior..brasset delivery with correct headers; builds also addassets:compress.Review response (commit 32e3efc)
Addressed the automated review of this branch.
Fortified the two heuristic numbers into defended contracts
evaluateBudgethelper (+.d.mtstypes), and pinned the policy table and boundary semantics.Coverage + docs
<pre>, no CodeMirror on the raw path) and pinned it with a test..brheader contract (Content-Encoding: br+ decompressedContent-Type+Vary) and immutable_next/staticcaching, for both serve-export and the self-hosted server.scripts/bench-baseline.json.Two review suggestions reverted after verification — the build-budget gate caught them
getVisibleHashLengthin the viewer shell is intentionally a local copy ofgetVisibleFragmentLength: deduping it pulls the codec module (lz-string/fflate) into the homepage initial chunk and breachescheck:build-budgets. Documented in place instead.defaultLinkCreatorDraft/getBodyFieldLabelwere never duplicated — they live only in the home component; moving them into the lib pulled codec deps into the homepage chunk. Reverted.npm run check(lint + unit + bench + typecheck + build + budgets) is green; homepage route JS is 107.4 / 115.0 KiB.Scope note
This branch is a cohesive optimization pass. The removed deps (
clsx,tailwind-merge,next-themes,@tanstack/react-table,@codemirror/commands/search,@git-diff-view/file) are load-bearing consequences of the code changes and can't be separated without dead deps or a broken build. The one cleanly separable change is the Next 15.1 → 15.5 bump (+ matchingeslint-config-nextand thepostcssoverride) — happy to peel that into its own prerequisite PR if you'd prefer it reviewed in isolation.