Conversation
- Wrap icon and 'agent-render' title in clickable link - Add handleGoHome to clear fragment hash and show empty state - Add e2e test for header navigation Co-authored-by: Aanish Bhirud <baanish@users.noreply.github.com>
Co-authored-by: Aanish Bhirud <baanish@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds clickable header home navigation (clearing URL hash and resetting hash state), an e2e test for that flow, many new payload/link encoding utilities (sync + async, ARX support), wire-format pack/unpack, envelope normalization, fragment encoding/decoding, language loading, git-patch parsing, and widespread JSDoc additions. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Viewer as ViewerShell
participant Browser as Browser (location)
participant AppState as App State
User->>Viewer: Click header logo/title
Viewer->>Browser: history.replaceState(preserve path+search, remove hash)
Viewer->>AppState: reset hash-based navigation state
AppState-->>Viewer: emit empty/home state
Viewer-->>User: render empty/home UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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: |
4940e77
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c648f8e0.agent-render.pages.dev |
| Branch Preview URL: | https://cursor-header-homepage-navig.agent-render.pages.dev |
Code Review SummaryStatus: No New Issues Found | Recommendation: Merge Overview
DetailsThis PR adds JSDoc documentation to public exported functions/components, following the newly documented rule in AGENTS.md. Changes include:
The existing 4 inline comments (marked as Minor/Potential issues) are pre-existing and relate to the JSDoc additions in this PR. Files Reviewed (15 files)
|
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @baanish. The following files were modified: * `src/components/viewer-shell.tsx` These files were ignored: * `tests/e2e/viewer.spec.ts`
…-modules Add JSDoc for payload fragment, wire-format, envelope, and link-creator APIs
…d-components docs: add JSDoc to exported viewer and app shell components
…ions-in-the-codebase Add JSDoc for diff parser, language loader, payload guard, and cn helper
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/lib/payload/link-creator.ts (2)
164-171:⚠️ Potential issue | 🟠 MajorDon't let the async generator default to
arxuntil first render can decode it.With no options passed on Line 171, this helper can emit
arxfragments by default.src/components/viewer-shell.tsx:262-271still seeds state with synchronousdecodeFragment(hash), so arx links render aninvalid-formatstate on the first pass and only recover in the effect-driven async decode. Either update the viewer bootstrap in the same PR or pin this call toplain|lz|deflatefor now.🩹 Temporary safeguard
- const hash = `#${await encodeEnvelopeAsync(normalized.envelope)}`; + const hash = `#${await encodeEnvelopeAsync(normalized.envelope, { + codecPriority: ["deflate", "lz", "plain"], + })}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/payload/link-creator.ts` around lines 164 - 171, createGeneratedArtifactLinkAsync can currently emit arx fragments because encodeEnvelopeAsync is called without options; to avoid first-render decode failures in decodeFragment (viewer-shell.tsx), update createGeneratedArtifactLinkAsync to pass an explicit encoder option (e.g., force plain, lz or deflate) into encodeEnvelopeAsync so it never defaults to arx; locate createGeneratedArtifactLinkAsync and change the encodeEnvelopeAsync(normalized.envelope) call to include the chosen format option (keeping normalizeEnvelope, createDraftEnvelope and the existing error handling intact).
171-177:⚠️ Potential issue | 🟠 MajorMeasure async links with transport length, not raw string length.
encodeEnvelopeAsync()already picks arx candidates bytransportLengthinsrc/lib/payload/fragment.ts:160-173, but this path re-validates withhash.length - 1. For non-ASCII arx payloads that undercounts the real fragment budget, so an oversize link can pass here and the returnedfragmentLengthcan be wrong too. Please carry the selected candidate’s transport length through instead of recomputing from the raw hash.As per coding guidelines, "Fail clearly on malformed or oversized payloads before renderer mount when possible."
Also applies to: 188-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/payload/link-creator.ts` around lines 171 - 177, The length check is using the raw hash string (hash.length - 1) which miscounts non-ASCII transport bytes; update the logic in link-creator.ts to use the transportLength chosen by encodeEnvelopeAsync rather than recomputing from the raw hash—have encodeEnvelopeAsync (or the helper it calls) return the selected candidate's transportLength alongside the encoded envelope and use that transportLength in place of fragmentLength for the MAX_FRAGMENT_LENGTH comparisons (apply the same change to the second check around lines 188-193), referencing the functions/values encodeEnvelopeAsync, normalized.envelope, MAX_FRAGMENT_LENGTH, and the local variables that compute hash/fragmentLength.src/lib/payload/fragment.ts (2)
375-380:⚠️ Potential issue | 🟠 MajorReject bad ARX headers before attempting decompression.
This branch currently accepts
v1.arx.<payload>by treating the missing dict segment as a legacy payload, and it still triesversionedPayloadeven when the parsed dict version does not matchgetActiveDictVersion(). That turns a fragment-header problem into a generic decode failure and weakens the wire-format contract.As per coding guidelines, "Fragment format must follow `v1..` for `plain|lz|deflate`, and `v1.arx..` for `arx`."🧩 Suggested fix
function resolveArxDictVersion(version: number | null): boolean { - if (version === null) { - return true; - } - - return version === getActiveDictVersion(); + return version !== null && version === getActiveDictVersion(); }if (codec === "arx") { const thirdDot = remainder.indexOf("."); const parsedDictVersion = thirdDot > 0 && /^\d+$/.test(remainder.slice(0, thirdDot)) ? Number.parseInt(remainder.slice(0, thirdDot), 10) : null; - const versionedPayload = parsedDictVersion === null ? remainder : remainder.slice(thirdDot + 1); - const fallbackPayload = remainder; - const useVersionedPayload = resolveArxDictVersion(parsedDictVersion); - const decodeAttempts = useVersionedPayload && parsedDictVersion !== null - ? [decodeArxEncodedPayload(fallbackPayload), decodeArxEncodedPayload(versionedPayload)] - : useVersionedPayload - ? [decodeArxEncodedPayload(versionedPayload)] - : [decodeArxEncodedPayload(fallbackPayload), decodeArxEncodedPayload(versionedPayload)]; + if (parsedDictVersion === null) { + return { + ok: false, + code: "invalid-format", + message: "The fragment format is invalid. Expected v1.arx.<dictVersion>.<payload>.", + }; + } + if (!resolveArxDictVersion(parsedDictVersion)) { + return { + ok: false, + code: "invalid-format", + message: `Unsupported arx dictionary version "${parsedDictVersion}".`, + }; + } + + const versionedPayload = remainder.slice(thirdDot + 1); + const decodeAttempts = [decodeArxEncodedPayload(versionedPayload)];Also applies to: 406-419
194-205:⚠️ Potential issue | 🟠 Major
targetMaxFragmentLengthnever changes the outcome.Because
sortedis ordered by ascendingtransportLength, the first candidate that satisfies<= budgetis alwayssorted[0]. Ifsorted[0]is too large, none of the later candidates can fit, and this still returns the oversize fragment instead of failing. As written, callers cannot use this option to enforce the fragment cap.As per coding guidelines, "Fail clearly on malformed or oversized payloads before renderer mount when possible" and "Fragment size budget must not exceed 8000 characters."🧩 Suggested fix
function selectCandidate(candidates: CandidateFragment[], budget?: number): CandidateFragment { if (candidates.length === 0) { throw new Error("No payload codec candidates are available."); } const sorted = [...candidates].sort((a, b) => a.transportLength - b.transportLength); - if (typeof budget !== "number") { - return sorted[0]; - } - - const inBudget = sorted.find((candidate) => candidate.transportLength <= budget); - return inBudget ?? sorted[0]; + const selected = sorted[0]; + + if (typeof budget === "number" && selected.transportLength > budget) { + throw new Error( + `This payload needs ${selected.transportLength.toLocaleString()} fragment characters, which is over the ${budget.toLocaleString()} character limit.`, + ); + } + + return selected; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/payload/fragment.ts` around lines 194 - 205, selectCandidate currently sorts candidates ascending and then picks the first match with transportLength <= budget, which incorrectly returns an oversized fragment; change its logic to enforce the budget (capping budget to 8000), filter candidates to those with transportLength <= budget, and if none exist throw a clear Error; otherwise pick an appropriate fit (e.g., the largest transportLength that is still <= budget) instead of using sorted[0]; update references in this function (selectCandidate, sorted, inBudget) accordingly so callers can enforce fragment caps before renderer mount.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/renderers/markdown-renderer.tsx`:
- Around line 58-62: The current ready-block reset schedules readyBlockIds = []
for the next render, allowing the effect that checks readyBlockIds.length to
read stale readiness and call onReady prematurely; change the readiness tracking
to be keyed by artifact.id (e.g., make readyBlockIds a Map or object keyed by
artifact.id, or store currentArtifactId alongside readyBlockIds) and update the
effect to derive readiness from the entry for the current artifact.id (or
compare against the current block callbacks/count for that artifact) so the
reset is immediate and scoped to the artifact, and ensure the effect’s
dependency array includes artifact.id and any current block identifiers so
onReady is only called when the blocks for the active artifact are actually
ready.
In `@src/lib/code/language.ts`:
- Around line 3-14: The file maps .sh/.bash to the language key "shell" (see the
extension-to-language mapping in language.ts) but loadLanguageSupport() does not
handle "shell", causing shell files to render as plain text; either add a case
in loadLanguageSupport() to recognize "shell" and load the appropriate
shell/bourne/bash support (or alias it to the existing "bash" loader), or change
the mapping for .sh/.bash to "text" (or to an already-supported key like "bash")
so CodeRenderer uses a supported loader; update loadLanguageSupport() and the
extension mapping consistently (referencing loadLanguageSupport() and the
.sh/.bash entries) so shell artifacts no longer fall back to plain text.
In `@src/lib/payload/envelope.ts`:
- Around line 20-31: normalizeEnvelope() currently treats whitespace-only diff
artifacts like " " as valid because it checks patch length without trimming;
update the normalization logic in normalizeEnvelope (and any helper it calls) to
trim patch before validating length and treat an empty trimmed patch as
missing—i.e., require either a non-empty trimmed `patch` or both `oldContent`
and `newContent`; ensure the check uses trimmedPatch = patch?.trim() and
rejects/normalizes artifacts where trimmedPatch is empty so malformed
whitespace-only patches fail early.
In `@src/lib/payload/schema.ts`:
- Around line 95-107: The runtime guard is too permissive: isPayloadEnvelope
currently only verifies required fields and lets malformed optional fields
(e.g., activeArtifactId as number, language as number, view as arbitrary string)
pass; update isPayloadEnvelope in src/lib/payload/schema.ts to validate typed
optional fields as well — ensure activeArtifactId (if present) is a string
matching the artifact id/hash pattern, language (if present) is a string from
the expected set or null, and view (for diff artifacts) is restricted to the
allowed union values "unified" or "split"; apply these checks where artifacts
are validated (look for the artifact-level validator functions and the diff
artifact branch in isPayloadEnvelope) so malformed optional fields are rejected
at decode time.
---
Outside diff comments:
In `@src/lib/payload/fragment.ts`:
- Around line 194-205: selectCandidate currently sorts candidates ascending and
then picks the first match with transportLength <= budget, which incorrectly
returns an oversized fragment; change its logic to enforce the budget (capping
budget to 8000), filter candidates to those with transportLength <= budget, and
if none exist throw a clear Error; otherwise pick an appropriate fit (e.g., the
largest transportLength that is still <= budget) instead of using sorted[0];
update references in this function (selectCandidate, sorted, inBudget)
accordingly so callers can enforce fragment caps before renderer mount.
In `@src/lib/payload/link-creator.ts`:
- Around line 164-171: createGeneratedArtifactLinkAsync can currently emit arx
fragments because encodeEnvelopeAsync is called without options; to avoid
first-render decode failures in decodeFragment (viewer-shell.tsx), update
createGeneratedArtifactLinkAsync to pass an explicit encoder option (e.g., force
plain, lz or deflate) into encodeEnvelopeAsync so it never defaults to arx;
locate createGeneratedArtifactLinkAsync and change the
encodeEnvelopeAsync(normalized.envelope) call to include the chosen format
option (keeping normalizeEnvelope, createDraftEnvelope and the existing error
handling intact).
- Around line 171-177: The length check is using the raw hash string
(hash.length - 1) which miscounts non-ASCII transport bytes; update the logic in
link-creator.ts to use the transportLength chosen by encodeEnvelopeAsync rather
than recomputing from the raw hash—have encodeEnvelopeAsync (or the helper it
calls) return the selected candidate's transportLength alongside the encoded
envelope and use that transportLength in place of fragmentLength for the
MAX_FRAGMENT_LENGTH comparisons (apply the same change to the second check
around lines 188-193), referencing the functions/values encodeEnvelopeAsync,
normalized.envelope, MAX_FRAGMENT_LENGTH, and the local variables that compute
hash/fragmentLength.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c37f5866-49ee-4a12-8e6c-01d53b950033
📒 Files selected for processing (20)
src/app/layout.tsxsrc/app/page.tsxsrc/components/home/link-creator.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/theme-provider.tsxsrc/components/theme-toggle.tsxsrc/components/viewer/artifact-selector.tsxsrc/components/viewer/fragment-details-disclosure.tsxsrc/lib/code/language.tssrc/lib/diff/git-patch.tssrc/lib/payload/envelope.tssrc/lib/payload/fragment.tssrc/lib/payload/link-creator.tssrc/lib/payload/schema.tssrc/lib/payload/wire-format.tssrc/lib/utils.ts
✅ Files skipped from review due to trivial changes (6)
- src/app/page.tsx
- src/components/viewer/artifact-selector.tsx
- src/components/renderers/csv-renderer.tsx
- src/components/theme-provider.tsx
- src/components/renderers/json-renderer.tsx
- src/app/layout.tsx
| /** | ||
| * Displays markdown artifacts in the primary viewer stage using sanitized GFM output. | ||
| * Consumes `artifact` content and optional `onReady`, which fires after embedded fenced code blocks report ready. | ||
| * Reuses the CodeMirror renderer for code fences and keeps raw HTML disabled for safer rendering. | ||
| */ |
There was a problem hiding this comment.
onReady can reuse stale block readiness after an artifact change.
The reset on Lines 68-70 only schedules readyBlockIds = [] for the next render, but the effect on Lines 72-76 in the same commit still reads the previous readyBlockIds.length. If the last artifact had at least as many fenced blocks as the new one, parents can get onReady before the new embedded CodeMirror blocks mount. Keying the readiness state by artifact.id or deriving readiness directly from current block callbacks would avoid that false-ready transition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/renderers/markdown-renderer.tsx` around lines 58 - 62, The
current ready-block reset schedules readyBlockIds = [] for the next render,
allowing the effect that checks readyBlockIds.length to read stale readiness and
call onReady prematurely; change the readiness tracking to be keyed by
artifact.id (e.g., make readyBlockIds a Map or object keyed by artifact.id, or
store currentArtifactId alongside readyBlockIds) and update the effect to derive
readiness from the entry for the current artifact.id (or compare against the
current block callbacks/count for that artifact) so the reset is immediate and
scoped to the artifact, and ensure the effect’s dependency array includes
artifact.id and any current block identifiers so onReady is only called when the
blocks for the active artifact are actually ready.
| /** | ||
| * Determines the code language token used by the viewer. | ||
| * | ||
| * Explicit language wins first (trimmed and lowercased); when absent, language is inferred from | ||
| * the filename extension, and defaults to `text` when no mapping matches. | ||
| * | ||
| * @param filename - Optional source filename used for extension-based inference. | ||
| * @param explicit - Optional user-provided language override. | ||
| * @returns A normalized language key for syntax selection. | ||
| * | ||
| * Failure/fallback: unknown or missing inputs fall back to `text`. | ||
| */ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that shell files are detected, but no matching loader branch exists.
rg -n -C2 'endsWith\("\.sh"\)|endsWith\("\.bash"\)|case "shell"|loadLanguageSupport|detectCodeLanguage' \
src/lib/code/language.ts src/components/renderers/code-renderer.tsxRepository: baanish/agent-render
Length of output: 2856
🏁 Script executed:
sed -n '49,$p' src/lib/code/language.ts | head -60Repository: baanish/agent-render
Length of output: 1496
Detection and loading disagree for shell files.
Line 33 maps .sh/.bash to shell, but loadLanguageSupport() never handles shell, so the CodeRenderer path still falls back to plain text for those artifacts. Either add a loader case for shell or map those extensions to text until shell support exists.
Also applies to: 38-48
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/code/language.ts` around lines 3 - 14, The file maps .sh/.bash to the
language key "shell" (see the extension-to-language mapping in language.ts) but
loadLanguageSupport() does not handle "shell", causing shell files to render as
plain text; either add a case in loadLanguageSupport() to recognize "shell" and
load the appropriate shell/bourne/bash support (or alias it to the existing
"bash" loader), or change the mapping for .sh/.bash to "text" (or to an
already-supported key like "bash") so CodeRenderer uses a supported loader;
update loadLanguageSupport() and the extension mapping consistently (referencing
loadLanguageSupport() and the .sh/.bash entries) so shell artifacts no longer
fall back to plain text.
| /** | ||
| * Validates and normalizes a payload envelope before encode/render use. | ||
| * | ||
| * Validation guarantees: | ||
| * - artifact ids are unique within the bundle (duplicate ids fail validation) | ||
| * - diff artifacts include either a non-empty `patch` or both `oldContent` and `newContent` | ||
| * - at least one artifact exists | ||
| * | ||
| * Normalization behavior: | ||
| * - `activeArtifactId` is preserved only when it matches an artifact in the bundle | ||
| * - otherwise `activeArtifactId` is normalized to the first artifact id | ||
| */ |
There was a problem hiding this comment.
Reject whitespace-only diff patches during normalization.
Line 9 accepts " " as a valid patch, so malformed diff payloads can pass normalizeEnvelope() and only fail later in DiffRenderer. Trimming before the length check keeps this helper as the pre-render validation boundary.
🛡️ Suggested fix
- const hasPatch = typeof artifact.patch === "string" && artifact.patch.length > 0;
+ const hasPatch = typeof artifact.patch === "string" && artifact.patch.trim().length > 0;As per coding guidelines "Fail clearly on malformed or oversized payloads before renderer mount when possible".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/payload/envelope.ts` around lines 20 - 31, normalizeEnvelope()
currently treats whitespace-only diff artifacts like " " as valid because it
checks patch length without trimming; update the normalization logic in
normalizeEnvelope (and any helper it calls) to trim patch before validating
length and treat an empty trimmed patch as missing—i.e., require either a
non-empty trimmed `patch` or both `oldContent` and `newContent`; ensure the
check uses trimmedPatch = patch?.trim() and rejects/normalizes artifacts where
trimmedPatch is empty so malformed whitespace-only patches fail early.
| /** | ||
| * Runtime shape guard for payload envelopes decoded from untyped input. | ||
| * | ||
| * Validates top-level structure (`v`, `codec`, non-empty `artifacts`) plus per-artifact minimum | ||
| * requirements: base fields, `content` for non-diff artifacts, and either `patch` or both | ||
| * `oldContent`/`newContent` for diff artifacts. | ||
| * | ||
| * @param value - Unknown value to validate as a payload envelope. | ||
| * @returns `true` when the value matches the runtime envelope contract, otherwise `false`. | ||
| * | ||
| * Failure/fallback: this guard checks structure only and does not perform full semantic | ||
| * normalization beyond required runtime fields. | ||
| */ |
There was a problem hiding this comment.
This documented “runtime contract” still lets malformed optional fields through.
isPayloadEnvelope() only rejects the required shape today, so values like activeArtifactId: 1, language: 123, or view: "side-by-side" still pass once the required fields are present. Since this is the boundary for decoded fragment input, please validate the remaining typed optional fields here as well so malformed hashes fail before render instead of being treated as valid envelopes. Based on learnings, src/lib/payload/**/*.{ts,tsx} should fail clearly on malformed payloads before renderer mount when possible, and in src/lib/payload/schema.ts diff artifacts may specify view as unified or split.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/payload/schema.ts` around lines 95 - 107, The runtime guard is too
permissive: isPayloadEnvelope currently only verifies required fields and lets
malformed optional fields (e.g., activeArtifactId as number, language as number,
view as arbitrary string) pass; update isPayloadEnvelope in
src/lib/payload/schema.ts to validate typed optional fields as well — ensure
activeArtifactId (if present) is a string matching the artifact id/hash pattern,
language (if present) is a string from the expected set or null, and view (for
diff artifacts) is restricted to the allowed union values "unified" or "split";
apply these checks where artifacts are validated (look for the artifact-level
validator functions and the diff artifact branch in isPayloadEnvelope) so
malformed optional fields are rejected at decode time.
…tion-rule-in-ci Resolve conflicts by keeping more detailed JSDoc comments from base branch over the minimal placeholder JSDoc added by the CI docs check PR. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add CI check for JSDoc on public exports
Enable navigation to the homepage by clicking the header icon or name.
This allows users to easily return to the initial empty state by clearing the URL hash and resetting the viewer's state.
Summary by CodeRabbit
New Features
Tests
Documentation
Chores