Rebuild UI as bench instrument; migrate diff renderer to @pierre/diffs - #111
Rebuild UI as bench instrument; migrate diff renderer to @pierre/diffs#111baanish wants to merge 2 commits into
Conversation
Fuse C3 hardware-bench skeuomorphism, C4 Japanese density, D7 QRH placard grammar, and D1 CR-39 carbonless slip into one design system. Bone chrome in light theme, charcoal placard chrome in dark, dark content chassis (code/diff/csv/json) in both. One token spine in globals.css; bench.css is grammar-only. Replace @git-diff-view/react with @pierre/diffs (lazy-loaded, Shiki, no vendor CSS). Per-file PatchDiff inside sections; MultiFileDiff for old/new pairs. Webpack alias for the ./react subpath; @pierre/theme peer dep added. Diff budget raised to 900 KiB. Merge artifact metadata and fragment disclosure into one spec-sheet placard: two-column dot-leader ledger, status lamp in its row, hash truncated to one line, no self-evident chrome text.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
📝 WalkthroughWalkthroughThe PR redesigns the artifact viewer with a bench-style interface, replaces the diff renderer with ChangesArtifact viewer redesign
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ViewerShell
participant LinkCreator
participant ArtifactRenderer
participant InspectorPanel
ViewerShell->>LinkCreator: render artifact-link creation controls
LinkCreator-->>ViewerShell: return generated fragment link state
ViewerShell->>InspectorPanel: show fragment diagnostics when required
ViewerShell->>ArtifactRenderer: render decoded artifact content
ArtifactRenderer-->>ViewerShell: display markdown, code, JSON, CSV, or diff output
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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: |
c21fecb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://de974585.agent-render.pages.dev |
| Branch Preview URL: | https://kimi-ui-redesign.agent-render.pages.dev |
|
| Filename | Overview |
|---|---|
| src/components/renderers/diff-renderer.tsx | Migrates patch and file-pair rendering to lazily loaded @pierre/diffs components while retaining binary and raw-text fallbacks. |
| src/lib/diff/pierre-react.ts | Provides an isolated re-export boundary for the deferred Pierre React components. |
| src/app/globals.css | Establishes the redesigned theme token spine and shared bench-instrument styling. |
| src/app/bench.css | Adds the reusable visual grammar for lamps, readouts, boards, ledger rows, and carbon-transfer details. |
| src/app/layout.tsx | Replaces remotely sourced font integration with locally bundled Manrope, SUSE, and Spline Sans Mono assets. |
| src/components/viewer/fragment-details-disclosure.tsx | Reworks fragment and artifact metadata into the compact placard presentation. |
| public/_headers | Removes the obsolete precompressed diff stylesheet header rule after the renderer migration. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
F[URL fragment] --> D[Client-side decode]
D --> S[Viewer shell]
S --> K{Artifact kind}
K -->|Diff patch| P[Parse patch sections]
P --> PD[Lazy-loaded PatchDiff]
K -->|Old and new content| MD[Lazy-loaded MultiFileDiff]
K -->|Other kinds| R[Existing artifact renderers]
Reviews (2): Last reviewed commit: "Add overnight redesign review notes and ..." | Re-trigger Greptile
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| ]; | ||
|
|
||
| for (const sample of SAMPLES) { | ||
| const link = createGeneratedArtifactLink(sample.draft, PREVIEW_BASE); |
There was a problem hiding this comment.
WARNING: The sync createGeneratedArtifactLink ignores draft.codec, so the intended arx3/arx4 samples never actually exercise those codecs.
createGeneratedArtifactLink (src/lib/payload/link-creator.ts:167) calls the sync encodeEnvelope, which only considers the sync candidates (deflate/lz/plain) and throws on the arx family (src/lib/payload/fragment.ts:149,231). The draft.codec value here (arx3 for the markdown sample at line 170, arx4 for the json sample at line 218, plus the lz/deflate choices) is never read, so every sample is silently encoded with the smallest sync codec instead. The Codec column written to SAMPLE_LINKS.md will therefore report deflate/lz/plain and the arx3/arx4 renderers are never exercised — contradicting the script's stated purpose (line 2-3) and the wire-size table in the markdown sample body.
Use the async variant (which honors draft.codec at link-creator.ts:194 and supports the arx family) and await it:
| const link = createGeneratedArtifactLink(sample.draft, PREVIEW_BASE); | |
| const link = await createGeneratedArtifactLinkAsync(sample.draft, PREVIEW_BASE); |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous Review Summary (commit 59c2d3c)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 59c2d3c)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (30 files)
Reviewed by deepseek-v4-flash · Input: 40.7K · Output: 7.4K · Cached: 538.6K |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (17)
.impeccable.md (1)
74-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the remaining design context.
This file still specifies the previous editorial UI system, including Fraunces and IBM Plex fonts, ghost borders, and purple-black dark mode.
.impeccable/design-contract.mddefines the authoritative bench system with a different token, font, and component grammar.Update or remove the obsolete sections with this renderer migration. Otherwise, future UI work can follow conflicting design instructions.
Based on learnings: Keep documentation, examples, and the OpenClaw skill contract aligned whenever fragment format, supported kinds, payload limits, zero-retention wording, renderer capabilities, commands, or deployment assumptions change.
🤖 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 @.impeccable.md at line 74, Update the remaining design context in .impeccable.md to match the authoritative system defined by .impeccable/design-contract.md, removing obsolete Fraunces/IBM Plex typography, ghost-border styling, and purple-black dark-mode guidance. Ensure the renderer migration’s documentation, examples, and OpenClaw skill contract consistently reflect the current tokens, fonts, component grammar, renderer capabilities, commands, and deployment assumptions.Source: Learnings
package.json (2)
42-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid
npx tsxfor an.mjsscript.
npx tsxdownloadstsxat run time when it is not installed. That makes the script non-deterministic and it fails without network access. The target isscripts/generate-sample-artifacts.mjs, so plainnoderuns it directly.If the script needs TypeScript, add
tsxtodevDependenciesand dropnpx.♻️ Proposed script fix
- "generate:samples": "npx tsx scripts/generate-sample-artifacts.mjs", + "generate:samples": "node scripts/generate-sample-artifacts.mjs",🤖 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 `@package.json` at line 42, Update the generate:samples script in package.json to invoke scripts/generate-sample-artifacts.mjs with node instead of npx tsx, since the target is a JavaScript module; only add tsx as a devDependency if the script actually requires TypeScript execution.
61-62: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valuePin
@pierre/theme@2.0.0if keeping this peer dependency.
@pierre/themeis listed but does not appear imported by the diff renderer; it is only provided by@pierre/diffs@1.3.5as a dependency. If the app does not consume this peer dependency, remove it to avoid unnecessary resolution pressure or an installed dead dependency.🤖 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 `@package.json` around lines 61 - 62, Update the package dependencies around `@pierre/theme`: remove it if the application does not directly consume the theme package, or pin the dependency exactly to version 2.0.0 if it must remain as a peer dependency. Keep `@pierre/diffs` unchanged.tests/components/diff-renderer.test.tsx (2)
8-12: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale
FileDiffreferences intests/components/diff-renderer.test.tsxanddocs/architecture.md.src/lib/diff/pierre-react.tsre-exports onlyPatchDiffandMultiFileDiff, andsrc/components/renderers/diff-renderer.tsxrenders only those two. Both sites still name aFileDiffcomponent that the product does not use.
tests/components/diff-renderer.test.tsx#L8-L12: remove theFileDiffkey from thevi.mock("@/lib/diff/pierre-react")factory.docs/architecture.md#L63-L67: change the component list toPatchDiffandMultiFileDiff, and apply the same correction to the duplicate sentence under "## Diff choice".🤖 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/diff-renderer.test.tsx` around lines 8 - 12, Remove the stale FileDiff mock from tests/components/diff-renderer.test.tsx lines 8-12, leaving PatchDiff and MultiFileDiff in the vi.mock factory. In docs/architecture.md lines 63-67, update the component list and duplicate “## Diff choice” sentence to mention only PatchDiff and MultiFileDiff.
90-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore coverage for the parse-error fallback branch.
This suite covers the
invalid-shapefallback only.diff-renderer.tsxstill handles a separateparse-errorbranch at Lines 317-323, which emits a different message and aParser detail:line. The removal of the malformed-patch test leaves that branch untested.Add a case that makes
parseGitPatchBundlethrow, then assert thecould not be rendered as a valid unified diffmessage and the parser detail text.I can generate this test case. Do you want me to add it?
🤖 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/diff-renderer.test.tsx` around lines 90 - 96, Add a DiffRenderer test that forces parseGitPatchBundle to throw, covering the separate parse-error fallback rather than the existing invalid-shape case. Assert the renderer enters fallback state and displays the “could not be rendered as a valid unified diff” message along with the “Parser detail:” text and thrown error detail.src/components/renderers/diff-renderer.tsx (3)
435-437: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRespect
prefers-reduced-motionfor the smooth scroll.The file navigation always uses
behavior: "smooth". Users who setprefers-reduced-motion: reducereceive an animated scroll they asked to suppress.♿ Proposed fix
onClick={() => { - document.getElementById(`patch-file-${file.id}`)?.scrollIntoView({ behavior: "smooth", block: "start" }); + const prefersReducedMotion = window.matchMedia("(prefers-reduced-motion: reduce)").matches; + document.getElementById(`patch-file-${file.id}`)?.scrollIntoView({ + behavior: prefersReducedMotion ? "auto" : "smooth", + block: "start", + }); }}🤖 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 435 - 437, Update the onClick handler in the diff renderer to detect the user’s prefers-reduced-motion setting and use non-animated scrolling when it is set to reduce, while retaining smooth scrolling otherwise. Keep the existing patch-file-${file.id} target and scrollIntoView behavior unchanged apart from the conditional scroll behavior.
20-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider carrying
patchFilesin thepatchstate.
{ kind: "patch" }holds no data. The render path must therefore re-checkparsedPatchBundle.kind === "parsed"at Line 426, and the: nullbranch at Line 468 becomes unreachable dead code that the compiler cannot prove. PuttingpatchFileson the state removes the second check.♻️ Proposed refactor
type DiffRenderState = - | { kind: "patch" } + | { kind: "patch"; patchFiles: ReturnType<typeof parseGitPatchBundle> } | { kind: "filepair"; oldContent: string; newContent: string }Then at the producer:
if (parsedPatchBundle.kind === "parsed") { - return { kind: "patch" }; + return { kind: "patch", patchFiles: parsedPatchBundle.patchFiles }; }🤖 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 - 34, Update DiffRenderState so the "patch" variant carries patchFiles, and populate that field when creating the patch state from a parsed bundle. In the render path, use state.patchFiles directly instead of rechecking parsedPatchBundle.kind === "parsed"; remove the unreachable null branch while preserving existing handling for non-patch states.
367-375: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMemoize
fileDiffOptionsby mode.
fileDiffOptionschanges only whenmodechanges, but it becomes a new object on every render and is passed toPatchDiff/MultiFileDiff. Memoize[mode]to avoid unnecessary prop churn on unrelated re-renders. The option names and values match@pierre/diffsusage.♻️ Proposed refactor
Move the value into a memo declared with the other hooks, above the early returns:
+ const fileDiffOptions = useMemo( + () => + ({ + diffStyle: mode, + themeType: "dark", + overflow: "wrap", + }) as const, + [mode], + );Then delete the inline definition under the existing
fileDiffOptionsassignment.🤖 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 367 - 375, Memoize the fileDiffOptions object in the diff renderer using a hook keyed by mode, declaring it with the other hooks before any early returns. Remove the inline object construction while preserving its existing diffStyle, themeType, and overflow values.src/components/home/sample-links.tsx (1)
37-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExpose the active sample to assistive technology.
isActiveonly adds a class. A screen reader user cannot tell which sample is currently open. Addaria-current.♻️ Proposed change
className={cn("sample-link", "bench-cell", isActive && "is-active")} + aria-current={isActive ? "true" : undefined}🤖 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/sample-links.tsx` at line 37, Update the sample link element using the isActive state to include an aria-current attribute, marking the active sample while leaving inactive samples unset. Preserve the existing className behavior.src/components/viewer/artifact-stage.tsx (2)
394-394: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
aria-labelon a role-lessdivhas no effect. Both sites putaria-labelon a genericdiv. Adivhas no implicit role, so most assistive technology ignores the label and the grouping is never announced. The shared fix is to give the element a role, or to drop the label.
src/components/viewer/artifact-stage.tsx#L394-L394: the stat strip already labels each value inline (kind,size,codec,fragment). Removearia-label="Artifact metadata", or addrole="group"to make the label effective.src/components/home/bench-hero.tsx#L19-L19: removearia-label="Trust details"; the three pills read as text on their own.🤖 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/artifact-stage.tsx` at line 394, Remove the ineffective aria-label attributes from the generic divs: delete “Artifact metadata” in src/components/viewer/artifact-stage.tsx lines 394-394 and “Trust details” in src/components/home/bench-hero.tsx lines 19-19, since both groups already expose their content as text and require no replacement role.
394-413: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe console strip and the spec sheet show the same four values.
The console strip renders kind, size, codec, and fragment/max.
FragmentDetailsDisclosurethen renders Kind and Size (fromartifactDetailRows), Codec, and Budget. The disclosure is always open, so a reader sees each value twice on one page.The PR objective describes consolidating artifact metadata into a single two-column spec-sheet placard. Choose one surface for these four values. Keep the strip for the at-a-glance readout and drop the duplicated ledger rows, or keep the ledger and reduce the strip to the filename plus actions.
Also applies to: 563-571
🤖 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/artifact-stage.tsx` around lines 394 - 413, Consolidate the duplicated artifact metadata shown by the console strip and FragmentDetailsDisclosure into one surface. Prefer keeping the console strip’s kind, size, codec, and fragment/max readout, and remove the corresponding Kind, Size, Codec, and Budget rows from FragmentDetailsDisclosure and its artifactDetailRows data; preserve the filename and action controls.src/components/home/link-creator.tsx (1)
469-469: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHide the decorative stamp from assistive technology.
LINK TRANSFERis visual grammar, not information. Screen readers read it as content between the action row and the bundle row. Addaria-hidden="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/home/link-creator.tsx` at line 469, Update the decorative stamp div with className "bench-stamp" to include aria-hidden="true", keeping the existing visual content and styling unchanged.src/components/viewer/inspector-panel.tsx (1)
78-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGive the progress bar an accessible name.
role="progressbar"carries no name here. A screen reader announces the percentage without stating what it measures. Addaria-label.♻️ Proposed change
<div className="budget-track" role="progressbar" + aria-label="Fragment budget used" aria-valuemin={0} aria-valuemax={100} aria-valuenow={Math.round(budgetRatio * 100)} >🤖 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/inspector-panel.tsx` around lines 78 - 86, Add an aria-label to the progressbar div in the budget-track markup so screen readers identify what the budgetRatio percentage measures, while preserving the existing aria-valuemin, aria-valuemax, and aria-valuenow attributes.src/components/viewer/fragment-details-disclosure.tsx (1)
46-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe lamp colour depends on free-form label strings.
statusLabelis astring. The mapping matches the exact values"Error"and"Ready", whichgetStatusToneproduces insrc/components/viewer-shell.tsx(lines 158-180). If a label changes there, this component silently falls back to amber and no compiler error appears.Narrow the prop to a union, or pass a separate tone field.
♻️ Proposed change
- statusLabel: string; + statusLabel: "Ready" | "Empty" | "Error";🤖 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/fragment-details-disclosure.tsx` around lines 46 - 47, Update the statusLabel prop used by statusLampClass to a constrained union of the supported status values, such as "Error" and "Ready" (or pass the corresponding tone field), and align getStatusTone’s output with that type so label changes are caught by the compiler instead of silently defaulting to amber.src/components/viewer-shell.tsx (1)
413-413: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCheck the heading hierarchy of the empty state.
Line 413 renders a screen-reader
h2, andBenchHerorenders a secondh2at the same level. The page has noh1in this state. Promote one of them toh1so the document outline starts at the top level.Also applies to: 427-427
🤖 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` at line 413, Update the empty-state heading hierarchy in the component containing the “Zero-retention artifact viewer” heading and the nearby BenchHero heading so one serves as an h1 instead of both being h2 elements. Ensure the empty-state document outline begins with a single top-level heading while preserving the other heading’s relative hierarchy.src/components/home/bench-hero.tsx (1)
5-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe optional
animationStyleprops now have no caller. The bench redesign removed the staggered entrance wrapper from the shell.src/components/viewer-shell.tsxrenders<BenchHero />(line 427) and<SampleLinks activeHash={hash} />(line 434), so neither component receivesanimationStyle. The prop, theCSSPropertiesimport, and thestylebinding are dead surface in both files.
src/components/home/bench-hero.tsx#L5-L7: removeanimationStylefromBenchHeroProps, drop theCSSPropertiesimport, and removestyle={animationStyle}from thesection.src/components/home/sample-links.tsx#L12-L12: removeanimationStylefromSampleLinksPropsand removestyle={animationStyle}from thesection.Keep the props only if a caller will pass them in a follow-up.
🤖 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/bench-hero.tsx` around lines 5 - 7, Remove the unused animationStyle prop and related styling from both components: in src/components/home/bench-hero.tsx lines 5-7, update BenchHeroProps, remove the CSSProperties import, and remove the section style binding; in src/components/home/sample-links.tsx line 12, update SampleLinksProps and remove the section style binding. No changes are needed in viewer-shell.tsx because its callers already pass no animationStyle.src/app/bench.css (1)
284-313: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the dark-mode hero colors into the token spine.
The file header states that all tokens live in
globals.css. These rules hardcode#ede7da,#b0a695, and#e8e0d4..bench-chip.is-onalso hardcodes#fff8ef. Define--bench-ink,--bench-ink-softoverrides under.darkinglobals.cssinstead, so the two sheets stay consistent.🤖 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/app/bench.css` around lines 284 - 313, Move the dark-mode color values from the `.dark .bench-hero-headline`, `.dark .bench-hero .bench-hero-truth`, `.dark .bench-hero .bench-hero-warning`, and bold-text rules into `.dark` token overrides in `globals.css` for `--bench-ink` and `--bench-ink-soft`; remove the corresponding hardcoded color declarations from `bench.css` and update `.bench-chip.is-on` to use the appropriate token instead of `#fff8ef`.
🤖 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 @.impeccable/drafts/c1-crt-arcade.html:
- Line 132: Clarify the privacy boundary across all three sites: in
.impeccable/drafts/c1-crt-arcade.html lines 132-132, replace “The host never
receives it” with the scoped statement that static hosts do not receive artifact
content in the page request, plus the caveat that fragment links can leak
through browser history, copied URLs, screenshots, or analytics; in
.impeccable/drafts/c1-crt-arcade.html lines 172-175, remove or qualify “HOST
SEES NOTHING” with the same boundary; in .impeccable/drafts/d2-terminal-log.html
lines 117-120, add the link-leak caveat after the local-decoding statement.
In @.impeccable/drafts/c4-high-density.html:
- Around line 229-232: Update the SELF-HOST panel in the mini section to remove
the unsupported “Optional UUID server with SQLite TTL” persistence claim.
Replace it with the supported static-export behavior defined by the
architecture, and only label it as future work if a separate implementation
exists.
In `@docs/architecture.md`:
- Around line 63-67: Remove the duplicated paragraph and bullet from the earlier
architecture section, retaining them only under “## Diff choice.” Update the
`agent-render` component claim to mention only `PatchDiff` and `MultiFileDiff`,
matching the exports in `pierre-react.ts` and usage in `diff-renderer.tsx`; do
not claim that `FileDiff` is used.
In `@scripts/generate-sample-artifacts.mjs`:
- Around line 1-13: Update the usage documentation for
generate-sample-artifacts.mjs to invoke it through tsx, using “npx tsx
scripts/generate-sample-artifacts.mjs” or only documenting the existing npm
wrapper; do not advertise direct plain-Node execution because the script imports
the TypeScript module createGeneratedArtifactLink.
In `@src/components/home/link-creator.tsx`:
- Around line 203-207: Remove aria-live from the bench-readout element, or
otherwise ensure the live region contains only the settled generatedLink result
rather than the draft.content.length fallback. Preserve the displayed count and
codec behavior while preventing announcements on every textarea keystroke.
In `@src/components/theme-toggle.tsx`:
- Around line 35-36: Update the theme toggle’s accessibility semantics to use
one pattern: keep aria-pressed with a static aria-label describing the setting,
such as “Dark mode,” and preserve the existing mounted/loading handling; do not
retain the changing “Switch to … theme” action label alongside aria-pressed.
In `@src/components/viewer-shell.tsx`:
- Around line 378-389: Update the home anchor in the viewer shell to use the
base path produced by the existing withBasePath helper instead of href="#",
while preserving the current onClick handler and handleGoHome behavior.
In `@src/components/viewer/fragment-details-disclosure.tsx`:
- Around line 13-21: Update the LedgerRow type annotation to use a locally
imported ReactNode type instead of React.ReactNode. Add the appropriate
type-only ReactNode import and change the children property annotation, leaving
the component’s runtime imports and behavior unchanged.
In `@src/components/viewer/inspector-panel.tsx`:
- Around line 70-75: Update the artifact count rendering in the inspector panel
to display "—" when artifactCount is null, matching the unknown codec behavior,
while continuing to format non-null counts with numberFormatter.
In `@tests/e2e/viewer.spec.ts`:
- Around line 236-245: Update the viewport bounds assertion loop in the viewer
test to validate vertical visibility as well: obtain the viewport height and
assert each valueBox.top is at least -1 and valueBox.bottom is no greater than
viewport height plus 1. If the intended contract is only horizontal clipping,
instead narrow the test comment and assertions to explicitly describe horizontal
bounds.
---
Nitpick comments:
In @.impeccable.md:
- Line 74: Update the remaining design context in .impeccable.md to match the
authoritative system defined by .impeccable/design-contract.md, removing
obsolete Fraunces/IBM Plex typography, ghost-border styling, and purple-black
dark-mode guidance. Ensure the renderer migration’s documentation, examples, and
OpenClaw skill contract consistently reflect the current tokens, fonts,
component grammar, renderer capabilities, commands, and deployment assumptions.
In `@package.json`:
- Line 42: Update the generate:samples script in package.json to invoke
scripts/generate-sample-artifacts.mjs with node instead of npx tsx, since the
target is a JavaScript module; only add tsx as a devDependency if the script
actually requires TypeScript execution.
- Around line 61-62: Update the package dependencies around `@pierre/theme`:
remove it if the application does not directly consume the theme package, or pin
the dependency exactly to version 2.0.0 if it must remain as a peer dependency.
Keep `@pierre/diffs` unchanged.
In `@src/app/bench.css`:
- Around line 284-313: Move the dark-mode color values from the `.dark
.bench-hero-headline`, `.dark .bench-hero .bench-hero-truth`, `.dark .bench-hero
.bench-hero-warning`, and bold-text rules into `.dark` token overrides in
`globals.css` for `--bench-ink` and `--bench-ink-soft`; remove the corresponding
hardcoded color declarations from `bench.css` and update `.bench-chip.is-on` to
use the appropriate token instead of `#fff8ef`.
In `@src/components/home/bench-hero.tsx`:
- Around line 5-7: Remove the unused animationStyle prop and related styling
from both components: in src/components/home/bench-hero.tsx lines 5-7, update
BenchHeroProps, remove the CSSProperties import, and remove the section style
binding; in src/components/home/sample-links.tsx line 12, update
SampleLinksProps and remove the section style binding. No changes are needed in
viewer-shell.tsx because its callers already pass no animationStyle.
In `@src/components/home/link-creator.tsx`:
- Line 469: Update the decorative stamp div with className "bench-stamp" to
include aria-hidden="true", keeping the existing visual content and styling
unchanged.
In `@src/components/home/sample-links.tsx`:
- Line 37: Update the sample link element using the isActive state to include an
aria-current attribute, marking the active sample while leaving inactive samples
unset. Preserve the existing className behavior.
In `@src/components/renderers/diff-renderer.tsx`:
- Around line 435-437: Update the onClick handler in the diff renderer to detect
the user’s prefers-reduced-motion setting and use non-animated scrolling when it
is set to reduce, while retaining smooth scrolling otherwise. Keep the existing
patch-file-${file.id} target and scrollIntoView behavior unchanged apart from
the conditional scroll behavior.
- Around line 20-34: Update DiffRenderState so the "patch" variant carries
patchFiles, and populate that field when creating the patch state from a parsed
bundle. In the render path, use state.patchFiles directly instead of rechecking
parsedPatchBundle.kind === "parsed"; remove the unreachable null branch while
preserving existing handling for non-patch states.
- Around line 367-375: Memoize the fileDiffOptions object in the diff renderer
using a hook keyed by mode, declaring it with the other hooks before any early
returns. Remove the inline object construction while preserving its existing
diffStyle, themeType, and overflow values.
In `@src/components/viewer-shell.tsx`:
- Line 413: Update the empty-state heading hierarchy in the component containing
the “Zero-retention artifact viewer” heading and the nearby BenchHero heading so
one serves as an h1 instead of both being h2 elements. Ensure the empty-state
document outline begins with a single top-level heading while preserving the
other heading’s relative hierarchy.
In `@src/components/viewer/artifact-stage.tsx`:
- Line 394: Remove the ineffective aria-label attributes from the generic divs:
delete “Artifact metadata” in src/components/viewer/artifact-stage.tsx lines
394-394 and “Trust details” in src/components/home/bench-hero.tsx lines 19-19,
since both groups already expose their content as text and require no
replacement role.
- Around line 394-413: Consolidate the duplicated artifact metadata shown by the
console strip and FragmentDetailsDisclosure into one surface. Prefer keeping the
console strip’s kind, size, codec, and fragment/max readout, and remove the
corresponding Kind, Size, Codec, and Budget rows from FragmentDetailsDisclosure
and its artifactDetailRows data; preserve the filename and action controls.
In `@src/components/viewer/fragment-details-disclosure.tsx`:
- Around line 46-47: Update the statusLabel prop used by statusLampClass to a
constrained union of the supported status values, such as "Error" and "Ready"
(or pass the corresponding tone field), and align getStatusTone’s output with
that type so label changes are caught by the compiler instead of silently
defaulting to amber.
In `@src/components/viewer/inspector-panel.tsx`:
- Around line 78-86: Add an aria-label to the progressbar div in the
budget-track markup so screen readers identify what the budgetRatio percentage
measures, while preserving the existing aria-valuemin, aria-valuemax, and
aria-valuenow attributes.
In `@tests/components/diff-renderer.test.tsx`:
- Around line 8-12: Remove the stale FileDiff mock from
tests/components/diff-renderer.test.tsx lines 8-12, leaving PatchDiff and
MultiFileDiff in the vi.mock factory. In docs/architecture.md lines 63-67,
update the component list and duplicate “## Diff choice” sentence to mention
only PatchDiff and MultiFileDiff.
- Around line 90-96: Add a DiffRenderer test that forces parseGitPatchBundle to
throw, covering the separate parse-error fallback rather than the existing
invalid-shape case. Assert the renderer enters fallback state and displays the
“could not be rendered as a valid unified diff” message along with the “Parser
detail:” text and thrown error detail.
🪄 Autofix
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 Plus
Run ID: d6e43e60-92b4-4807-95fd-fbf604009ed6
⛔ Files ignored due to path filters (23)
.impeccable/screenshots/c1-crt-arcade.pngis excluded by!**/*.png.impeccable/screenshots/c2-night-sixpack.pngis excluded by!**/*.png.impeccable/screenshots/c3-hardware-bench.pngis excluded by!**/*.png.impeccable/screenshots/c4-high-density.pngis excluded by!**/*.png.impeccable/screenshots/c6-rain-garden.pngis excluded by!**/*.png.impeccable/screenshots/d1-ncr-form.pngis excluded by!**/*.png.impeccable/screenshots/d2-terminal-log.pngis excluded by!**/*.png.impeccable/screenshots/d3-broadcast-chyron.pngis excluded by!**/*.png.impeccable/screenshots/d4-nautical-chart.pngis excluded by!**/*.png.impeccable/screenshots/d5-blueprint.pngis excluded by!**/*.png.impeccable/screenshots/d6-cd-catalog.pngis excluded by!**/*.png.impeccable/screenshots/d7-cockpit-placard.pngis excluded by!**/*.pngpackage-lock.jsonis excluded by!**/package-lock.jsonsrc/app/icon.svgis excluded by!**/*.svgsrc/fonts/manrope-v20-latin-500.woff2is excluded by!**/*.woff2src/fonts/manrope-v20-latin-600.woff2is excluded by!**/*.woff2src/fonts/manrope-v20-latin-700.woff2is excluded by!**/*.woff2src/fonts/manrope-v20-latin-800.woff2is excluded by!**/*.woff2src/fonts/spline-sans-mono-v13-latin-500.woff2is excluded by!**/*.woff2src/fonts/spline-sans-mono-v13-latin-600.woff2is excluded by!**/*.woff2src/fonts/suse-v4-latin-500.woff2is excluded by!**/*.woff2src/fonts/suse-v4-latin-600.woff2is excluded by!**/*.woff2src/fonts/suse-v4-latin-700.woff2is excluded by!**/*.woff2
📒 Files selected for processing (57)
.gitignore.impeccable.md.impeccable/config.json.impeccable/design-contract.md.impeccable/drafts/_index.html.impeccable/drafts/c1-crt-arcade.html.impeccable/drafts/c2-night-sixpack.html.impeccable/drafts/c3-hardware-bench.html.impeccable/drafts/c4-high-density.html.impeccable/drafts/c6-rain-garden.html.impeccable/drafts/d1-ncr-form.html.impeccable/drafts/d2-terminal-log.html.impeccable/drafts/d3-broadcast-chyron.html.impeccable/drafts/d4-nautical-chart.html.impeccable/drafts/d5-blueprint.html.impeccable/drafts/d6-cd-catalog.html.impeccable/drafts/d7-cockpit-placard.htmlAGENTS.mdPRODUCT.mddocs/architecture.mddocs/dependency-notes.mdpackage.jsonpublic/_headerspublic/vendor/diff-view-pure.csspublic/vendor/diff-view-pure.css.brscripts/bench-codecs.mjsscripts/check-build-budgets.mjsscripts/compress-dictionary.mjsscripts/generate-sample-artifacts.mjssrc/app/bench.csssrc/app/globals.csssrc/app/layout.tsxsrc/components/home/bench-hero.tsxsrc/components/home/link-creator.tsxsrc/components/home/sample-links.tsxsrc/components/renderers/code-renderer.tsxsrc/components/renderers/diff-renderer.tsxsrc/components/renderers/diff-view-stylesheet.tssrc/components/renderers/json-renderer.tsxsrc/components/renderers/markdown-renderer.tsxsrc/components/theme-toggle.tsxsrc/components/viewer-shell.tsxsrc/components/viewer/artifact-selector.tsxsrc/components/viewer/artifact-stage.tsxsrc/components/viewer/fragment-details-disclosure.tsxsrc/components/viewer/inspector-panel.tsxsrc/lib/diff/pierre-react.tstests/build-budgets.test.tstests/components/diff-renderer.test.tsxtests/components/fragment-details-disclosure.test.tsxtests/components/viewer-shell.test.tsxtests/diff-style-asset.test.tstests/e2e/viewer.spec.tstests/headers.test.tstests/selfhosted/api-catalog.test.tstests/selfhosted/static-headers.test.tstests/serve-export-headers.test.ts
💤 Files with no reviewable changes (10)
- src/components/renderers/code-renderer.tsx
- public/_headers
- tests/diff-style-asset.test.ts
- tests/headers.test.ts
- src/components/renderers/json-renderer.tsx
- src/components/renderers/diff-view-stylesheet.ts
- tests/serve-export-headers.test.ts
- tests/selfhosted/static-headers.test.ts
- public/vendor/diff-view-pure.css
- tests/selfhosted/api-catalog.test.ts
| </div> | ||
|
|
||
| <marquee class="marquee" style="display:block">ZERO-RETENTION<br>ARTIFACT <span class="red">VIEWER</span></marquee> | ||
| <p class="sub-marquee">Payload lives in the URL fragment. <span class="glow">The host never receives it.</span> Five renderers loaded: markdown · code · diff · csv · json.</p> |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
State the privacy boundary and the URL-leak risks.
Zero retention does not make a link secret-safe. State that static hosts do not receive artifact content on the page request. Also state that fragment URLs can leak through browser history, copied URLs, screenshots, and analytics.
.impeccable/drafts/c1-crt-arcade.html#L132-L132: replace “The host never receives it” with the scoped static-host statement and the link-leak caveat..impeccable/drafts/c1-crt-arcade.html#L172-L175: remove “HOST SEES NOTHING” or qualify it with the same boundary..impeccable/drafts/d2-terminal-log.html#L117-L120: add the link-leak caveat after the local-decoding statement.
Based on learnings: the product is zero-retention by host design but is not absolutely secret-safe because links can leak through browser history, copied URLs, screenshots, or analytics.
📍 Affects 2 files
.impeccable/drafts/c1-crt-arcade.html#L132-L132(this comment).impeccable/drafts/c1-crt-arcade.html#L172-L175.impeccable/drafts/d2-terminal-log.html#L117-L120
🤖 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 @.impeccable/drafts/c1-crt-arcade.html at line 132, Clarify the privacy
boundary across all three sites: in .impeccable/drafts/c1-crt-arcade.html lines
132-132, replace “The host never receives it” with the scoped statement that
static hosts do not receive artifact content in the page request, plus the
caveat that fragment links can leak through browser history, copied URLs,
screenshots, or analytics; in .impeccable/drafts/c1-crt-arcade.html lines
172-175, remove or qualify “HOST SEES NOTHING” with the same boundary; in
.impeccable/drafts/d2-terminal-log.html lines 117-120, add the link-leak caveat
after the local-decoding statement.
Source: Learnings
| <div class="mini"> | ||
| <div class="mh">SELF-HOST<em>NEW</em></div> | ||
| <p>Optional UUID server with SQLite TTL when the team genuinely needs persistence.</p> | ||
| </div> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the unsupported persistence mode claim.
The supplied architecture defines a static shell with fragment-resident artifact data. “Optional UUID server with SQLite TTL” describes a backend persistence feature that is not part of that contract.
Replace this panel with the supported static-export behavior. Mark it as future work only if a separate implementation exists.
Based on learnings: do not describe roadmap ideas as shipped behavior.
🤖 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 @.impeccable/drafts/c4-high-density.html around lines 229 - 232, Update the
SELF-HOST panel in the mini section to remove the unsupported “Optional UUID
server with SQLite TTL” persistence claim. Replace it with the supported
static-export behavior defined by the architecture, and only label it as future
work if a separate implementation exists.
Source: Learnings
| The largest remaining deferred cost is still the diff renderer stack. After the full rebuild, that stack is `@pierre/diffs` (Shiki-based, dynamically imported, no vendor CSS mirror into `public/`). It lazy-loads only when a diff artifact mounts, keeping the empty shell and non-diff artifacts off that cost. The stack provides the review-style UX for multi-file git patches, split/unified modes, and syntax-aware rendering with less product code than a bespoke replacement. | ||
|
|
||
| `agent-render` uses `@pierre/diffs` `PatchDiff`/`MultiFileDiff`/`FileDiff` components instead of `@codemirror/merge`. | ||
|
|
||
| - `@pierre/diffs` matches the product goal better because it is already shaped like a GitHub-style review surface |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the duplicated paragraph and the FileDiff claim.
Lines 65-67 repeat lines 78-80 word for word, including the bullet. Keep the statement only under "## Diff choice".
Also, the product code does not use FileDiff. src/lib/diff/pierre-react.ts re-exports only PatchDiff and MultiFileDiff, and diff-renderer.tsx renders only those two.
📝 Proposed documentation fix
The largest remaining deferred cost is still the diff renderer stack. After the full rebuild, that stack is `@pierre/diffs` (Shiki-based, dynamically imported, no vendor CSS mirror into `public/`). It lazy-loads only when a diff artifact mounts, keeping the empty shell and non-diff artifacts off that cost. The stack provides the review-style UX for multi-file git patches, split/unified modes, and syntax-aware rendering with less product code than a bespoke replacement.
-`agent-render` uses `@pierre/diffs` `PatchDiff`/`MultiFileDiff`/`FileDiff` components instead of `@codemirror/merge`.
-
-- `@pierre/diffs` matches the product goal better because it is already shaped like a GitHub-style review surface
-
The JSON and markdown paths are now substantially lighter because:And in the "## Diff choice" section:
-`agent-render` uses `@pierre/diffs` `PatchDiff`/`MultiFileDiff`/`FileDiff` components instead of `@codemirror/merge`.
+`agent-render` uses the `@pierre/diffs` `PatchDiff` and `MultiFileDiff` components instead of `@codemirror/merge`.Based on learnings: "Base changes on the current shipped product, trust code over stale guidance, and do not describe roadmap ideas as shipped behavior."
🤖 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 `@docs/architecture.md` around lines 63 - 67, Remove the duplicated paragraph
and bullet from the earlier architecture section, retaining them only under “##
Diff choice.” Update the `agent-render` component claim to mention only
`PatchDiff` and `MultiFileDiff`, matching the exports in `pierre-react.ts` and
usage in `diff-renderer.tsx`; do not claim that `FileDiff` is used.
Source: Learnings
| #!/usr/bin/env node | ||
| // Generates fresh sample fragment links across all supported artifact kinds, | ||
| // exercises the app's own encoder (`createGeneratedArtifactLink`), and writes | ||
| // a clickable markdown index to `.impeccable/overnight-shots/SAMPLE_LINKS.md`. | ||
| // | ||
| // Usage: node scripts/generate-sample-artifacts.mjs | ||
| // Requires tsx (devDependency): `npx tsx` resolves this automatically when | ||
| // invoked via `npm run generate:samples`. | ||
|
|
||
| import { mkdir, writeFile } from "node:fs/promises"; | ||
| import path from "node:path"; | ||
| import { fileURLToPath } from "node:url"; | ||
| import { createGeneratedArtifactLink } from "../src/lib/payload/link-creator.ts"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,15p' scripts/generate-sample-artifacts.mjs
rg -n -C 2 '"generate:samples"|tsx' package.jsonRepository: baanish/agent-render
Length of output: 1654
🌐 Web query:
Node.js ESM TypeScript imports .ts files require loader tsx
💡 Result:
To use TypeScript files (.ts) within a Node.js ESM environment, the tsx package provides a seamless solution by leveraging esbuild for fast, on-demand compilation [1][2][3]. You can integrate tsx in two primary ways: 1. Using the tsx CLI as a drop-in replacement for node: You can run your entry point directly using the tsx command, which handles the necessary loader registration automatically [1][2][3]. tsx your-file.ts 2. Using Node.js with the --import flag (Recommended): For Node.js v20.6.0 and later, you can use the module customization API to register tsx as a loader [1][4]. This is often preferred for CI/CD or production-like environments as it uses the official Node.js binary [4][3]. node --import tsx your-file.ts Key considerations: - Module Resolution: When using ESM, Node.js strictly requires explicit file extensions in import statements (e.g., import { x } from './file.js') [5][6][4]. While tsx is sometimes more lenient and may resolve./file.ts automatically, it is best practice to use the.js extension in your source code to ensure portability and alignment with standard ESM behavior [4]. - TypeScript Features: tsx supports tsconfig.json features like path aliases [1][2][3]. However, if you require full TypeScript type-checking or support for advanced features (like legacy decorators or experimental syntax), it is recommended to use a formal build step (e.g., tsc) before execution [7][3]. - Loader Specificity: If you need to target specific module systems, you can use more granular imports, such as node --import tsx/esm for ESM-only or node --require tsx/cjs for CommonJS-only contexts [1][8][3]. For projects that require official, minimal runtime support without third-party dependencies, Node.js now includes experimental built-in type stripping [7]; however, this feature ignores tsconfig.json settings and does not perform type checking [7]. For full compatibility with existing TypeScript configurations, third-party loaders like tsx remain the standard approach [5][7].
Citations:
- 1: https://github.com/blefnk/tsx
- 2: https://github.com/privatenumber/tsx/blob/master/docs/index.md
- 3: https://www.pkgpulse.com/guides/tsx-vs-ts-node-vs-esno-running-2026
- 4: https://fixdevs.com/blog/tsx-not-working/
- 5: https://nodejs.org/docs/latest/api/typescript.html
- 6: https://github.com/nodejs/node/blob/main/doc/api/typescript.md
- 7: https://nodejs.org/api/typescript.html
- 8: https://registry.npmjs.org/@matthd/tsx
🌐 Web query:
TypeScript tsx npm GitHub README usage npx tsx node
💡 Result:
tsx (TypeScript Execute) is a tool designed to run TypeScript and ESM files directly in Node.js without requiring manual compilation [1][2]. It uses esbuild to provide seamless TypeScript execution with modern, sensible defaults [2][3]. Usage Instructions To use tsx in your project, follow these steps: 1. Installation Install tsx as a development dependency in your project: npm install --save-dev tsx [4][5] 2. Running with the tsx CLI You can use the tsx binary as a direct alias to the node command, allowing you to run TypeScript files immediately [2][3]: npx tsx your-file.ts [4][5] 3. Running with the node CLI Alternatively, you can integrate tsx directly into the standard node command using the --import flag [4][5]: node --import tsx your-file.ts [5][6] Key Features - Seamless TypeScript execution: Runs TypeScript code directly, handling both CommonJS and ESM modes [2][3]. - No configuration required: Works out-of-the-box with sensible defaults, though it supports tsconfig.json [2][3]. - Watch mode: Includes a built-in watch mode to re-run files automatically upon saving [3]. - ESM support: Resolves common issues encountered with ESM in Node.js [2][3]. Important Note tsx does not perform type checking. It is recommended to use the official TypeScript compiler (tsc) for type checking your code before running it with tsx [4][5].
Citations:
- 1: https://www.npmjs.com/package/tsx
- 2: https://github.com/esbuild-kit/tsx/blob/master/docs/index.md
- 3: https://github.com/privatenumber/tsx/blob/master/docs/index.md
- 4: https://nodejs.org/en/learn/typescript/run
- 5: https://nodejs.org/docs/latest/api/typescript.html
- 6: https://nodejs.org/api/typescript.md
Use tsx for the direct script execution contract.
This script imports ../src/lib/payload/link-creator.ts; plain node scripts/generate-sample-artifacts.mjs cannot resolve that import, so the documented usage must require npx tsx scripts/generate-sample-artifacts.mjs or only advertise the wrapper script.
Proposed fix
-#!/usr/bin/env node
// Generates fresh sample fragment links across all supported artifact kinds,
@@
-// Usage: node scripts/generate-sample-artifacts.mjs
+// Usage: npm run generate:samples📝 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.
| #!/usr/bin/env node | |
| // Generates fresh sample fragment links across all supported artifact kinds, | |
| // exercises the app's own encoder (`createGeneratedArtifactLink`), and writes | |
| // a clickable markdown index to `.impeccable/overnight-shots/SAMPLE_LINKS.md`. | |
| // | |
| // Usage: node scripts/generate-sample-artifacts.mjs | |
| // Requires tsx (devDependency): `npx tsx` resolves this automatically when | |
| // invoked via `npm run generate:samples`. | |
| import { mkdir, writeFile } from "node:fs/promises"; | |
| import path from "node:path"; | |
| import { fileURLToPath } from "node:url"; | |
| import { createGeneratedArtifactLink } from "../src/lib/payload/link-creator.ts"; | |
| // Generates fresh sample fragment links across all supported artifact kinds, | |
| // exercises the app's own encoder (`createGeneratedArtifactLink`), and writes | |
| // a clickable markdown index to `.impeccable/overnight-shots/SAMPLE_LINKS.md`. | |
| // | |
| // Usage: npm run generate:samples | |
| // Requires tsx (devDependency): `npx tsx` resolves this automatically when | |
| // invoked via `npm run generate:samples`. | |
| import { mkdir, writeFile } from "node:fs/promises"; | |
| import path from "node:path"; | |
| import { fileURLToPath } from "node:url"; | |
| import { createGeneratedArtifactLink } from "../src/lib/payload/link-creator.ts"; |
🤖 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/generate-sample-artifacts.mjs` around lines 1 - 13, Update the usage
documentation for generate-sample-artifacts.mjs to invoke it through tsx, using
“npx tsx scripts/generate-sample-artifacts.mjs” or only documenting the existing
npm wrapper; do not advertise direct plain-Node execution because the script
imports the TypeScript module createGeneratedArtifactLink.
| <div className="bench-readout" aria-live="polite"> | ||
| {numberFormatter.format( | ||
| generatedLink?.fragmentLength ?? draft.content.length, | ||
| )} chars / {generatedLink?.codec ?? draft.codec ?? "auto"} | ||
| </div> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not put aria-live on a readout that changes on every keystroke.
The readout value falls back to draft.content.length. That value changes on each character the user types in the content textarea. With aria-live="polite", a screen reader announces the new count after every keystroke, which floods the announcement queue while the user is typing.
Announce only the settled generation result, or drop the live region.
🛠️ Proposed fix
- <div className="bench-readout" aria-live="polite">
+ <div className="bench-readout">
{numberFormatter.format(
generatedLink?.fragmentLength ?? draft.content.length,
)} chars / {generatedLink?.codec ?? draft.codec ?? "auto"}
</div>📝 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.
| <div className="bench-readout" aria-live="polite"> | |
| {numberFormatter.format( | |
| generatedLink?.fragmentLength ?? draft.content.length, | |
| )} chars / {generatedLink?.codec ?? draft.codec ?? "auto"} | |
| </div> | |
| <div className="bench-readout"> | |
| {numberFormatter.format( | |
| generatedLink?.fragmentLength ?? draft.content.length, | |
| )} chars / {generatedLink?.codec ?? draft.codec ?? "auto"} | |
| </div> |
🤖 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 203 - 207, Remove
aria-live from the bench-readout element, or otherwise ensure the live region
contains only the settled generatedLink result rather than the
draft.content.length fallback. Preserve the displayed count and codec behavior
while preventing announcements on every textarea keystroke.
| aria-pressed={mounted ? isDark : false} | ||
| aria-label={mounted ? `Switch to ${isDark ? "light" : "dark"} theme` : "Theme toggle loading"} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Pick one toggle pattern: aria-pressed or a changing label.
aria-pressed describes the state of the control. The label describes the action. Together a screen reader announces "Switch to light theme, pressed", which is contradictory.
Use a static name plus aria-pressed (for example, name the control "Dark mode" and let aria-pressed report whether dark mode is on), or keep the action label and remove aria-pressed.
♻️ Proposed change (state pattern)
aria-pressed={mounted ? isDark : false}
- aria-label={mounted ? `Switch to ${isDark ? "light" : "dark"} theme` : "Theme toggle loading"}
+ aria-label={mounted ? "Dark mode" : "Theme toggle loading"}📝 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.
| aria-pressed={mounted ? isDark : false} | |
| aria-label={mounted ? `Switch to ${isDark ? "light" : "dark"} theme` : "Theme toggle loading"} | |
| aria-pressed={mounted ? isDark : false} | |
| aria-label={mounted ? "Dark mode" : "Theme toggle loading"} |
🤖 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/theme-toggle.tsx` around lines 35 - 36, Update the theme
toggle’s accessibility semantics to use one pattern: keep aria-pressed with a
static aria-label describing the setting, such as “Dark mode,” and preserve the
existing mounted/loading handling; do not retain the changing “Switch to …
theme” action label alongside aria-pressed.
| <a | ||
| href="#" | ||
| onClick={(e) => { | ||
| e.preventDefault(); | ||
| handleGoHome(); | ||
| }} | ||
| className="flex items-center gap-2.5 sm:gap-3 focus:outline-none focus-visible:ring-2 focus-visible:ring-[color:var(--accent)] focus-visible:ring-offset-2 rounded-[var(--radius-lg)] -m-1 p-1" | ||
| className="-m-1 flex items-center gap-2.5 rounded-[var(--radius-sm)] p-1 focus:outline-none focus-visible:ring-2 focus-visible:ring-[color:var(--accent)] focus-visible:ring-offset-2" | ||
| aria-label="Go to homepage" | ||
| > | ||
| <div className="grid h-8 w-8 place-items-center rounded-[var(--radius-lg)] border border-[color:var(--border)] bg-[color:var(--surface-strong)] sm:h-9 sm:w-9"> | ||
| <span | ||
| aria-hidden="true" | ||
| className="h-4.5 w-4.5 sm:h-5 sm:w-5" | ||
| style={iconImageStyle} | ||
| /> | ||
| </div> | ||
| <h1 className="font-display text-lg font-semibold tracking-[-0.03em] sm:text-xl"> | ||
| Agent Render | ||
| </h1> | ||
| <span aria-hidden="true" className="bench-lamp" /> | ||
| <span className="nav-wordmark">agent-render</span> | ||
| </a> |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Do not use href="#" for the home link; it can discard the artifact fragment.
handleGoHome clears the fragment correctly after hydration. Before the click handler is attached, or if the handler does not run, the browser follows href="#" and replaces the current fragment. The fragment is the only copy of the artifact content, so the artifact is lost from the address bar.
Point the anchor at the base path instead. withBasePath is already imported in this file.
🛠️ Proposed fix
<a
- href="#"
+ href={withBasePath("/")}
onClick={(e) => {
e.preventDefault();
handleGoHome();
}}📝 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.
| <a | |
| href="#" | |
| onClick={(e) => { | |
| e.preventDefault(); | |
| handleGoHome(); | |
| }} | |
| className="flex items-center gap-2.5 sm:gap-3 focus:outline-none focus-visible:ring-2 focus-visible:ring-[color:var(--accent)] focus-visible:ring-offset-2 rounded-[var(--radius-lg)] -m-1 p-1" | |
| className="-m-1 flex items-center gap-2.5 rounded-[var(--radius-sm)] p-1 focus:outline-none focus-visible:ring-2 focus-visible:ring-[color:var(--accent)] focus-visible:ring-offset-2" | |
| aria-label="Go to homepage" | |
| > | |
| <div className="grid h-8 w-8 place-items-center rounded-[var(--radius-lg)] border border-[color:var(--border)] bg-[color:var(--surface-strong)] sm:h-9 sm:w-9"> | |
| <span | |
| aria-hidden="true" | |
| className="h-4.5 w-4.5 sm:h-5 sm:w-5" | |
| style={iconImageStyle} | |
| /> | |
| </div> | |
| <h1 className="font-display text-lg font-semibold tracking-[-0.03em] sm:text-xl"> | |
| Agent Render | |
| </h1> | |
| <span aria-hidden="true" className="bench-lamp" /> | |
| <span className="nav-wordmark">agent-render</span> | |
| </a> | |
| <a | |
| href={withBasePath("/")} | |
| onClick={(e) => { | |
| e.preventDefault(); | |
| handleGoHome(); | |
| }} | |
| className="-m-1 flex items-center gap-2.5 rounded-[var(--radius-sm)] p-1 focus:outline-none focus-visible:ring-2 focus-visible:ring-[color:var(--accent)] focus-visible:ring-offset-2" | |
| aria-label="Go to homepage" | |
| > | |
| <span aria-hidden="true" className="bench-lamp" /> | |
| <span className="nav-wordmark">agent-render</span> | |
| </a> |
🤖 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 378 - 389, Update the home
anchor in the viewer shell to use the base path produced by the existing
withBasePath helper instead of href="#", while preserving the current onClick
handler and handleGoHome behavior.
| function LedgerRow({ | ||
| label, | ||
| children, | ||
| className, | ||
| }: { | ||
| label: string; | ||
| children: React.ReactNode; | ||
| className?: string; | ||
| }) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check the React import and the tsconfig UMD setting for the disclosure component.
set -euo pipefail
f=$(fd -t f 'fragment-details-disclosure.tsx' src)
echo "== head of $f =="
sed -n '1,25p' "$f"
echo
echo "== React imports in the file =="
rg -n '^\s*import .*"react"' "$f" || echo "no react import found"
echo
echo "== allowUmdGlobalAccess / jsx settings =="
fd -t f 'tsconfig*.json' . --max-depth 2 --exec rg -n 'allowUmdGlobalAccess|"jsx"' {}Repository: baanish/agent-render
Length of output: 951
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate tsconfig files =="
fd -t f 'tsconfig.*json' . -H
echo
echo "== tsconfig snippets =="
while IFS= read -r f; do
echo "--- $f"
rg -n 'allowUmdGlobalAccess|"jsx"|"types"|compilerOptions|include' "$f" || true
done < <(fd -t f 'tsconfig.*json' . -H)
echo
echo "== full disclosure file imports and relevant React references =="
f=$(fd -t f 'fragment-details-disclosure.tsx' src | head -n 1)
rg -n 'import|React\.React|ReactNode|react' "$f"Repository: baanish/agent-render
Length of output: 307
Import ReactNode locally instead of using React.ReactNode.
src/components/viewer/fragment-details-disclosure.tsx is a module that uses React.ReactNode without importing React. Add a type import for ReactNode and replace this annotation with children: ReactNode.
🤖 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/fragment-details-disclosure.tsx` around lines 13 - 21,
Update the LedgerRow type annotation to use a locally imported ReactNode type
instead of React.ReactNode. Add the appropriate type-only ReactNode import and
change the children property annotation, leaving the component’s runtime imports
and behavior unchanged.
| <span className="stat-item"> | ||
| artifacts{" "} | ||
| <span className="stat-value"> | ||
| {artifactCount !== null ? numberFormatter.format(artifactCount) : "0"} | ||
| </span> | ||
| </span> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Show — when the artifact count is unknown.
artifactCount is null when decoding fails. src/components/viewer-shell.tsx line 419 passes null on the error path. The panel then prints 0, which states that the bundle contains zero artifacts. The count is unknown, not zero. Line 68 already uses — for an unknown codec.
🛠️ Proposed fix
<span className="stat-value">
- {artifactCount !== null ? numberFormatter.format(artifactCount) : "0"}
+ {artifactCount !== null ? numberFormatter.format(artifactCount) : "—"}
</span>📝 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.
| <span className="stat-item"> | |
| artifacts{" "} | |
| <span className="stat-value"> | |
| {artifactCount !== null ? numberFormatter.format(artifactCount) : "0"} | |
| </span> | |
| </span> | |
| <span className="stat-item"> | |
| artifacts{" "} | |
| <span className="stat-value"> | |
| {artifactCount !== null ? numberFormatter.format(artifactCount) : "—"} | |
| </span> | |
| </span> |
🤖 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/inspector-panel.tsx` around lines 70 - 75, Update the
artifact count rendering in the inspector panel to display "—" when
artifactCount is null, matching the unknown codec behavior, while continuing to
format non-null counts with numberFormatter.
| // every row label and value stays on-screen | ||
| const viewportWidth = page.viewportSize()?.width ?? 0; | ||
| const valueBoxes = await specSheet | ||
| .locator(".artifact-meta-value, .artifact-hash-preview-code") | ||
| .evaluateAll((elements) => elements.map((element) => element.getBoundingClientRect())); | ||
| expect(valueBoxes.length).toBeGreaterThan(0); | ||
| for (const valueBox of valueBoxes) { | ||
| expect(valueBox.width).toBeGreaterThan(0); | ||
| expect(valueBox.right).toBeLessThanOrEqual(viewportWidth + 1); | ||
| expect(valueBox.left).toBeGreaterThanOrEqual(-1); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Check vertical viewport bounds, or narrow the test claim.
Lines 236-245 only check left and right bounds. A value box below the phone viewport passes this test. If full viewport visibility is required, also check top and bottom against the viewport height. Otherwise, state that this test checks horizontal clipping only.
🤖 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/e2e/viewer.spec.ts` around lines 236 - 245, Update the viewport bounds
assertion loop in the viewer test to validate vertical visibility as well:
obtain the viewport height and assert each valueBox.top is at least -1 and
valueBox.bottom is no greater than viewport height plus 1. If the intended
contract is only horizontal clipping, instead narrow the test comment and
assertions to explicitly describe horizontal bounds.
![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: 59c2d3c9d8
ℹ️ 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".
| // Guards the rich diff library (@pierre/diffs), the heaviest deferred | ||
| // chunk. Shiki's engine makes it ~860 KiB gzipped; always lazy-loaded. | ||
| // A meaningful jump means a version bump pulled in more — confirm first. | ||
| importKeyParts: ["components/renderers/diff-renderer", "@pierre/diffs"], |
There was a problem hiding this comment.
After a build, check:build-budgets cannot find this entry: @pierre/diffs is now a static import through pierre-react.ts, while the relevant loadable-manifest key comes from artifact-stage.tsx dynamically importing diff-renderer.tsx. No key therefore contains both configured substrings, so getLoadableFiles throws No react-loadable entry matched, breaking both npm run check and npm run test:ci; match the artifact-stage/diff-renderer loadable entry instead.
Useful? React with 👍 / 👎.
| <span className="bench-cell stat-item"> | ||
| kind <span className="stat-value">{activeArtifact.kind}</span> | ||
| </span> |
There was a problem hiding this comment.
Whenever an artifact is open, this adds self-evident chrome such as kind markdown or kind csv, even though the owner-established UI contract explicitly prohibits artifact-kind narration and requires showing the real filename or nothing. The filename is already rendered immediately above, so omit this stat rather than duplicating the renderer identity.
AGENTS.md reference: AGENTS.md:L258-L260
Useful? React with 👍 / 👎.
| for (const sample of SAMPLES) { | ||
| const link = createGeneratedArtifactLink(sample.draft, PREVIEW_BASE); |
There was a problem hiding this comment.
For samples requesting arx3, arx4, or any other explicit codec, this synchronous helper ignores draft.codec and calls the sync encoder without options, which can only select plain/lz/deflate. Consequently npm run generate:samples does not generate the codecs declared by its sample drafts; use the async link creator so those requested codec fixtures are actually exercised.
Useful? React with 👍 / 👎.
| "check": "npm run lint && npm run test && npm run bench:codecs && npm run typecheck && npm run build && npm run check:build-budgets", | ||
| "check:build-budgets": "node scripts/check-build-budgets.mjs", | ||
| "generate:samples": "npx tsx scripts/generate-sample-artifacts.mjs", | ||
| "shots:overnight": "RUN_OVERNIGHT=1 playwright test tests/e2e/overnight.spec.ts", |
There was a problem hiding this comment.
Running npm run shots:overnight always fails because tests/e2e/overnight.spec.ts does not exist; a repository-wide search finds only viewer.spec.ts, visual.spec.ts, and arx4-determinism.spec.ts under tests/e2e. Add the intended spec or change this command to target the actual screenshot suite.
Useful? React with 👍 / 👎.
| dependencies: { | ||
| "@codemirror/view": "^6.38.2", | ||
| "@git-diff-view/react": "^0.1.1", | ||
| "@pierre/diffs": "^1.3.3", |
There was a problem hiding this comment.
Changing the benchmark corpus from @git-diff-view/react to this string without updating scripts/bench-baseline.json makes npm run bench:codecs fail deterministically: the json-package rows regress by 1.45–2.49% and the command exits 1, whereas the parent version of this script passes against the same dictionaries and baseline. Regenerate the committed baseline for the intended corpus—or leave the benchmark fixture stable—so the documented npm run check validation can complete.
AGENTS.md reference: AGENTS.md:L185-L193
Useful? React with 👍 / 👎.
| <MultiFileDiff | ||
| oldFile={{ name: fileName, contents: renderedDiff.oldContent }} | ||
| newFile={{ name: fileName, contents: renderedDiff.newContent }} | ||
| options={fileDiffOptions} |
There was a problem hiding this comment.
For diff artifacts with an extensionless or missing filename, or where language intentionally overrides the extension, the migration now drops the public DiffArtifact.language hint entirely: neither the patch component nor these old/new file descriptors receives it. Those valid payloads therefore lose the syntax-aware highlighting that the previous renderer obtained through detectCodeLanguage; thread the explicit hint into Pierre's file/options metadata for both rendering paths.
Useful? React with 👍 / 👎.
| const display = Fraunces({ | ||
| subsets: ["latin"], | ||
| const display = localFont({ | ||
| src: [ |
There was a problem hiding this comment.
The newly vendored Manrope, SUSE, and Spline Sans Mono WOFF2 files are SIL OFL fonts, but a repository-wide search finds no OFL or upstream copyright notice—only the project's MIT LICENSE. Redistributing these binaries without their accompanying OFL and copyright terms does not satisfy the fonts' redistribution conditions; add the upstream license/notices and document their provenance before release.
AGENTS.md reference: AGENTS.md:L206-L214
Useful? React with 👍 / 👎.
- Document the overnight UI redesign and review risks - Add gated Playwright screenshots across artifact kinds, themes, and viewports
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/e2e/overnight.spec.ts (1)
73-73: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse one deterministic screenshot-readiness barrier.
tests/e2e/helpers.tsLines [31]-[61] already wait for renderer readiness, fonts, and two animation frames.captureSpotCheckthen adds a fixed 350 ms delay at Line [73], while the JSON path screenshots immediately after readiness at Lines [123]-[124]. If the delay is needed for CSS transitions, JSON captures can record an intermediate state. If it is not needed, generic captures pay an unnecessary delay. Centralize the transition-completion check and use it for both paths.Also applies to: 123-124
🤖 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/e2e/overnight.spec.ts` at line 73, Remove the fixed 350 ms wait from captureSpotCheck and centralize a deterministic transition-completion readiness barrier in the shared screenshot flow. Apply the same barrier after renderer/font/animation readiness for both captureSpotCheck and the JSON-path screenshots, ensuring neither path captures an intermediate CSS-transition state or incurs an unnecessary unconditional delay.
🤖 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 `@MORNING.md`:
- Around line 60-62: Update the “Lint / typecheck / unit / bench” section in
MORNING.md beside npm run check to document that the pre-existing codec
benchmark failure on main is expected, while lint, typecheck, unit tests, and
other checks must still pass.
- Line 3: Update the scope summary in MORNING.md to accurately mention the
migration from `@git-diff-view/react` to `@pierre/diffs`, related dependency
changes, and AGENTS.md/docs updates, or explicitly limit the note to the
bench-chrome cohort so it does not claim there was no renderer rewrite or
documentation work.
- Around line 49-50: Update the light-mode --text-soft token in globals.css to a
darker value that achieves at least 4.5:1 contrast against --page-bg for normal
text, then revise the corresponding MORNING.md note to document the corrected
contrast and remove the deferred follow-up language before calling the redesign
ship-ready.
In `@tests/e2e/overnight.spec.ts`:
- Around line 111-128: Add a mobile 390x844 case to the “json bundle (arx
showcase) desktop” test, matching the existing mobile artifact checks and
capturing the JSON artifact screenshot for each theme; if mobile coverage is
intentionally excluded, explicitly document that decision instead.
---
Nitpick comments:
In `@tests/e2e/overnight.spec.ts`:
- Line 73: Remove the fixed 350 ms wait from captureSpotCheck and centralize a
deterministic transition-completion readiness barrier in the shared screenshot
flow. Apply the same barrier after renderer/font/animation readiness for both
captureSpotCheck and the JSON-path screenshots, ensuring neither path captures
an intermediate CSS-transition state or incurs an unnecessary unconditional
delay.
🪄 Autofix
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 Plus
Run ID: 6fd0e023-9cc6-4ac2-a53e-bd7f311d9420
📒 Files selected for processing (2)
MORNING.mdtests/e2e/overnight.spec.ts
| @@ -0,0 +1,85 @@ | |||
| # Overnight redesign — morning review | |||
|
|
|||
| The fused world ("Creator Hardware Bench" skeuomorphism + Japanese density + QRH/CR‑39 engraved-panel grammar) is now inked across the app. Payload protocol, renderers, colors-in-code (rainbow brackets), and `.dark` theme still work — this is a *chrome* redesign with functional content tweaks, not a renderer rewrite. | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the scope summary for the renderer migration.
The supplied PR scope includes migration from @git-diff-view/react to @pierre/diffs, plus dependency and documentation updates. This conflicts with the statements that the change is “not a renderer rewrite,” that there are “no dependency changes,” and that no AGENTS.md or docs/ updates are required. Scope this note to the bench-chrome cohort or list the migration and documentation changes.
Also applies to: 82-85
🤖 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 `@MORNING.md` at line 3, Update the scope summary in MORNING.md to accurately
mention the migration from `@git-diff-view/react` to `@pierre/diffs`, related
dependency changes, and AGENTS.md/docs updates, or explicitly limit the note to
the bench-chrome cohort so it does not claim there was no renderer rewrite or
documentation work.
| 1. **Contrast, light mode.** `--text-soft #7d7466` on `--page-bg #ebe7dd` is right at WCAG AA for 14px regular. Mono-pill labels pass (they now use `--text-primary`), but the small uppercase kicker `--text-soft` only just clears 4.5:1. | ||
| 2. **`#ebe7dd` vs `#14110d`.** Verify light/dark transition doesn't look like a blinding flash on system-dark defaults. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Treat the light-mode contrast failure as a release issue.
#7d7466 on #ebe7dd is approximately 3.7:1, below the 4.5:1 WCAG AA threshold for normal text. Line [80] defers the darker token to an optional follow-up while Line [78] calls the redesign ship-ready. Update src/app/globals.css and this note before release, or restrict --text-soft to large text.
Suggested wording
-1. **Contrast, light mode.** `--text-soft `#7d7466`` on `--page-bg `#ebe7dd`` is right at WCAG AA for 14px regular.
+1. **Contrast, light mode.** `--text-soft `#7d7466`` on `--page-bg `#ebe7dd`` is approximately 3.7:1 and fails WCAG AA for normal text. Apply a darker token before release.Also applies to: 78-80
🤖 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 `@MORNING.md` around lines 49 - 50, Update the light-mode --text-soft token in
globals.css to a darker value that achieves at least 4.5:1 contrast against
--page-bg for normal text, then revise the corresponding MORNING.md note to
document the corrected contrast and remove the deferred follow-up language
before calling the redesign ship-ready.
| # 2. Lint / typecheck / unit / bench | ||
| npm run check | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the expected benchmark exception beside npm run check.
This step includes bench, while the PR scope reports a pre-existing codec benchmark failure on main. State the expected failure and the checks that must still pass. Otherwise, a failed check can look like a redesign regression.
🤖 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 `@MORNING.md` around lines 60 - 62, Update the “Lint / typecheck / unit /
bench” section in MORNING.md beside npm run check to document that the
pre-existing codec benchmark failure on main is expected, while lint, typecheck,
unit tests, and other checks must still pass.
| test("json bundle (arx showcase) desktop", async ({ page }) => { | ||
| await page.setViewportSize({ width: 1440, height: 900 }); | ||
| for (const theme of ["light", "dark"] as const) { | ||
| await setTheme(page, theme); | ||
| await goToHash(page); | ||
| await stabilizePage(page); | ||
| await waitForViewerState(page, "empty"); | ||
| await page.getByRole("link", { name: /arx showcase/i }).click(); | ||
| await waitForViewerState(page, "artifact"); | ||
| await page | ||
| .getByRole("button", { name: /Open artifact Artifact manifest/i }) | ||
| .click(); | ||
| await waitForRendererReady(page, "json"); | ||
| await page.screenshot({ | ||
| fullPage: true, | ||
| path: path.join(SHOT_DIR, `json-${theme}-desktop.png`), | ||
| }); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add mobile coverage for the JSON artifact.
The other artifact checks include a mobile case at Lines [106]-[108], but the JSON test runs only at the desktop viewport. Add a 390x844 JSON capture, matching the other mobile checks, or state that JSON is intentionally excluded from mobile coverage.
Suggested addition
+test("json bundle (arx showcase) mobile", async ({ page }) => {
+ await page.setViewportSize({ width: 390, height: 844 });
+ await setTheme(page, "light");
+ await goToHash(page);
+ await stabilizePage(page);
+ await waitForViewerState(page, "empty");
+ await page.getByRole("link", { name: /arx showcase/i }).click();
+ await waitForViewerState(page, "artifact");
+ await page.getByRole("button", { name: /Open artifact Artifact manifest/i }).click();
+ await waitForRendererReady(page, "json");
+ await page.screenshot({
+ fullPage: true,
+ path: path.join(SHOT_DIR, "json-light-mobile.png"),
+ });
+});📝 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.
| test("json bundle (arx showcase) desktop", async ({ page }) => { | |
| await page.setViewportSize({ width: 1440, height: 900 }); | |
| for (const theme of ["light", "dark"] as const) { | |
| await setTheme(page, theme); | |
| await goToHash(page); | |
| await stabilizePage(page); | |
| await waitForViewerState(page, "empty"); | |
| await page.getByRole("link", { name: /arx showcase/i }).click(); | |
| await waitForViewerState(page, "artifact"); | |
| await page | |
| .getByRole("button", { name: /Open artifact Artifact manifest/i }) | |
| .click(); | |
| await waitForRendererReady(page, "json"); | |
| await page.screenshot({ | |
| fullPage: true, | |
| path: path.join(SHOT_DIR, `json-${theme}-desktop.png`), | |
| }); | |
| } | |
| test("json bundle (arx showcase) desktop", async ({ page }) => { | |
| await page.setViewportSize({ width: 1440, height: 900 }); | |
| for (const theme of ["light", "dark"] as const) { | |
| await setTheme(page, theme); | |
| await goToHash(page); | |
| await stabilizePage(page); | |
| await waitForViewerState(page, "empty"); | |
| await page.getByRole("link", { name: /arx showcase/i }).click(); | |
| await waitForViewerState(page, "artifact"); | |
| await page | |
| .getByRole("button", { name: /Open artifact Artifact manifest/i }) | |
| .click(); | |
| await waitForRendererReady(page, "json"); | |
| await page.screenshot({ | |
| fullPage: true, | |
| path: path.join(SHOT_DIR, `json-${theme}-desktop.png`), | |
| }); | |
| } | |
| }); | |
| test("json bundle (arx showcase) mobile", async ({ page }) => { | |
| await page.setViewportSize({ width: 390, height: 844 }); | |
| await setTheme(page, "light"); | |
| await goToHash(page); | |
| await stabilizePage(page); | |
| await waitForViewerState(page, "empty"); | |
| await page.getByRole("link", { name: /arx showcase/i }).click(); | |
| await waitForViewerState(page, "artifact"); | |
| await page.getByRole("button", { name: /Open artifact Artifact manifest/i }).click(); | |
| await waitForRendererReady(page, "json"); | |
| await page.screenshot({ | |
| fullPage: true, | |
| path: path.join(SHOT_DIR, "json-light-mobile.png"), | |
| }); | |
| }); |
🤖 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/e2e/overnight.spec.ts` around lines 111 - 128, Add a mobile 390x844
case to the “json bundle (arx showcase) desktop” test, matching the existing
mobile artifact checks and capturing the JSON artifact screenshot for each
theme; if mobile coverage is intentionally excluded, explicitly document that
decision instead.
1 participant
Summary
Full chrome redesign fusing four directions into one design system ("bench instrument"):
Also migrates the diff renderer from
@git-diff-view/reactto@pierre/diffs.Design
globals.css :root/.darkowns all tokens (--bench-*aliases, keycap/recess depth recipe);bench.cssis grammar-only, defines no colors.next/fontinlayout.tsx).Key changes
New files
src/app/bench.css: grammar layer (lamp, readout, carbon strip, stamp, perf, board, cell, ledger row)src/components/home/bench-hero.tsx,src/components/viewer/inspector-panel.tsxsrc/lib/diff/pierre-react.ts: pierre react re-export so the deferred diff chunk stays isolatedsrc/fonts/*.woff2(9 files),.impeccable/design-contract + drafts,scripts/generate-sample-artifacts.mjsDiff renderer (
diff-renderer.tsx)@git-diff-view/react→@pierre/diffs/react:PatchDiffper parsed file section,MultiFileDifffor old/new pairs.patch-file-section,.patch-bundle-link, binary empty-state preserveddiff-view-stylesheet.ts,public/vendor/diff-view-pure.css(.br),tests/diff-style-asset.test.tsdeletednext.config.ts: webpack alias for the./reactsubpath (package exports only theimportcondition);@pierre/themepeer dep installedSpec sheet (
fragment-details-disclosure.tsx+artifact-stage.tsx)InspectorPanel(self-evident chrome text)Docs updated
AGENTS.md,docs/architecture.md,docs/dependency-notes.md,README.md,.impeccable/design-contract.mdaligned to the shipped world.Verification
npm run lintpass,npm run typecheckpassnpm run test: 49 files, 313/313 passCI=1 npx playwright test tests/e2e/viewer.spec.ts --project=chromium: 28/28 pass.impeccable/screenshots/)Known pre-existing failure
npm run bench:codecsgate fails:json-package:arx regressed 1.45% vs baselineetc. Confirmed pre-existing onmain(baseline/dictionary drift), NOT caused by this change. Left as-is.Summary by CodeRabbit
New Features
Enhancements
Documentation