feat: exhibit component library + stage#116
Open
Coldaine wants to merge 5 commits into
Open
Conversation
… primitives Introduce the exhibit vocabulary from docs/plans/plan-exhibit-floor.md: the ExhibitArtifact envelope, the discriminated payload union for the seven authored exhibit types plus live_graph, and type guards. Port the prototype primitives (Frame, accentByKind, Chip, ExhibitNode, curvedPath, arcPath) to TypeScript against plain CSS classes, respecting prefers-reduced-motion via framer-motion's useReducedMotion. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U9EpjZNoWaXLzfG93tdvTU
Port the five prototype views (relationship DAG with draw-on edges, folding activity narrative, spatial walkthrough, heat-classed concern snapshot, momentum gauge with arcPath needle + confidence bars) and design two new exhibits in the same language: a seismograph (horizontal time axis, kind-colored magnitude spikes, left-to-right trace draw-on, named-tremor labels) and a thermal map (squarified treemap, cool-blue to hot-red heat lerp, white-hot hottest cell with a why callout). Each takes its typed payload plus the artifact envelope. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U9EpjZNoWaXLzfG93tdvTU
Tell the real Codex homelab coordinator session (see the ground-truth doc) as a standing exhibition so the stage renders with zero inference: an activity narrative threading the plan pivot, PG18 recovery, token exposure, and GHCR stall; a relationship DAG of the nine noteworthy entities with the exposed FORGEJO_INTERNAL_TOKEN marked urgent; a concern snapshot heat-ranking the work; a seismograph with the GHCR-403 stall and turn abort visible; a momentum exhibit that reads NOT done; a walkthrough of the plan-authoring turn; the live_graph; and a retired early-guess exhibit for the archive shelf. Narratives are interpretive, never event recaps. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U9EpjZNoWaXLzfG93tdvTU
Add the stage: gallery rail (type icon, status, relevance bar, narrative preview), main Frame with an AnimatePresence swap, glass agent-commentary card, relevance-weighted idle cycling every 8s that pauses on hover with a visible pill and manual prev/next, and an archive shelf for retired artifacts. live_graph renders the existing CanvasRenderer inside the Frame. Register the stage in the surface adapter and make it the primary main-area surface in App; the legacy panels move into a collapsible area and the canvas is preserved as the live_graph exhibit. Extend styles.css with the exhibit classes and add a jsdom matchMedia stub for reduced-motion probing in tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U9EpjZNoWaXLzfG93tdvTU
Add tests/renderer/exhibits: a render test per component asserting key content, envelope type-guard + fixture-consistency tests, and stage tests covering fixture-gallery render, relevance-weighted rotation order, 8s auto-cycle, pause-on-hover, and retired artifacts landing in the archive shelf (out of rotation) with their retirement reason on the title attribute. 20 new tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U9EpjZNoWaXLzfG93tdvTU
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Comment on lines
+36
to
+45
| it('narrows each authored type to exactly one guard', () => { | ||
| const cases: Array<[ExhibitArtifact, (a: ExhibitArtifact) => boolean]> = [ | ||
| [envelope('relationship_dag', { nodes: [], edges: [], focusNodeId: '' }), isRelationshipDag], | ||
| [envelope('activity_narrative', { beats: [], threads: [] }), isActivityNarrative], | ||
| [envelope('walkthrough', { headline: '', body: '', tags: [], satellites: [], files: [] }), isWalkthrough], | ||
| [envelope('concern_snapshot', { concerns: [], flows: [] }), isConcernSnapshot], | ||
| [envelope('momentum', { value: 0, label: '', stats: [], next: [], curation: [] }), isMomentum], | ||
| [envelope('seismograph', { trace: [], annotations: [], windowMinutes: 60 }), isSeismograph], | ||
| [envelope('thermal_map', { cells: [], hottest: { path: '', why: '' } }), isThermalMap], | ||
| [envelope('live_graph', {}), isLiveGraph], |
There was a problem hiding this comment.
Suggestion: The test title says it validates only authored exhibit types, but the test data also includes live_graph, which is explicitly non-authored elsewhere in this suite. Rename the test or split the live_graph check into a separate case so the test contract is accurate and not misleading for future maintenance. [docstring mismatch]
Severity Level: Minor 🧹
⚠️ Test description slightly misstates coverage of live_graph guard.
⚠️ Future maintainers may misinterpret authored versus live types.
⚠️ Suggestion targets wording only; code behavior correct.Steps of Reproduction ✅
1. Open `tests/renderer/exhibits/types.test.ts` and locate the suite `describe('exhibit
type guards', () => {` at line 35; inside it, the test `it('narrows each authored type to
exactly one guard', () => {` starts at line 36.
2. Within that test, inspect the `cases` array defined at lines 37–45, which includes
envelopes for `'relationship_dag'`, `'activity_narrative'`, `'walkthrough'`,
`'concern_snapshot'`, `'momentum'`, `'seismograph'`, `'thermal_map'`, and also
`'live_graph'`.
3. Cross-check the authored vocabulary by opening `src/renderer/exhibits/types.ts` and
examining the `AUTHORED_EXHIBIT_TYPES` constant at lines 253–261, which lists seven
authored exhibit types and intentionally excludes `'live_graph'`, as documented in the
header comment at lines 4–6.
4. Observe that while the test name claims it narrows "each authored type", the
implementation actually exercises all exhibit type guards including `isLiveGraph`; this is
a minor wording mismatch in the test description with no behavioral or runtime impact,
making the suggestion a small clarity improvement rather than a functional bug.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/renderer/exhibits/types.test.ts
**Line:** 36:45
**Comment:**
*Docstring Mismatch: The test title says it validates only authored exhibit types, but the test data also includes `live_graph`, which is explicitly non-authored elsewhere in this suite. Rename the test or split the `live_graph` check into a separate case so the test contract is accurate and not misleading for future maintenance.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+316
to
+325
| export const fixtureGallery: ExhibitArtifact[] = [ | ||
| activityNarrative, | ||
| relationshipDag, | ||
| walkthrough, | ||
| concernSnapshot, | ||
| seismograph, | ||
| momentum, | ||
| liveGraph, | ||
| retiredEarlyGuess, | ||
| ]; |
There was a problem hiding this comment.
Suggestion: The default fixture gallery is missing a thermal_map artifact, so the stage cannot render the full seven authored exhibit types and one component remains unreachable in the shipped “standing demo.” Add a thermal_map entry (or include an existing one) in the exported fixtureGallery rotation to match the declared exhibit vocabulary. [incomplete implementation]
Severity Level: Major ⚠️
❌ ThermalMap exhibit unreachable in default museum floor demo.
⚠️ Fixture gallery fails to cover full authored vocabulary.
⚠️ Stage UI never exercises ThermalMap integration or motion.Steps of Reproduction ✅
1. Open `src/renderer/exhibits/types.ts` and note the documented vocabulary at lines 4-9
and the `ExhibitPayloadMap` keys at lines 175-185: it declares seven authored exhibit
types (`relationship_dag`, `activity_narrative`, `walkthrough`, `concern_snapshot`,
`momentum`, `seismograph`, `thermal_map`) plus `live_graph`.
2. In `src/renderer/exhibits/ExhibitStage.tsx`, see that the stage defaults to the fixture
gallery via `export default function ExhibitStage({ artifacts = fixtureGallery, liveGraph
}: ExhibitStageProps)` at lines 95-102, and that `ActiveExhibit` renders `ThermalMap` when
`isThermalMap(artifact)` is true at lines 72-80.
3. Inspect the authored fixture gallery in `src/renderer/exhibits/fixture-gallery.ts`: the
exported `fixtureGallery` array at lines 316-325 contains `activityNarrative`,
`relationshipDag`, `walkthrough`, `concernSnapshot`, `seismograph`, `momentum`,
`liveGraph`, and `retiredEarlyGuess`, but no artifact with `exhibitType: 'thermal_map'`,
so the default rotation has zero thermal-map entries.
4. Run the UI or the `ExhibitStage` tests (e.g.,
`tests/renderer/exhibits/ExhibitStage.test.tsx`) so that `ExhibitStage` is rendered with
its default props; observe that the rail titles and main frame cycle only through the
non-retired exhibits from `fixtureGallery`, and the Thermal Map component
(`src/renderer/exhibits/ThermalMap.tsx`) is never reached because there is no
`thermal_map` artifact in the default gallery.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/renderer/exhibits/fixture-gallery.ts
**Line:** 316:325
**Comment:**
*Incomplete Implementation: The default fixture gallery is missing a `thermal_map` artifact, so the stage cannot render the full seven authored exhibit types and one component remains unreachable in the shipped “standing demo.” Add a `thermal_map` entry (or include an existing one) in the exported `fixtureGallery` rotation to match the declared exhibit vocabulary.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| export default function ActivityNarrative({ payload }: ActivityNarrativeProps) { | ||
| const reduce = useReducedMotion(); | ||
| const { beats, threads } = payload; | ||
| const [active, setActive] = useState(beats.length - 1); |
There was a problem hiding this comment.
Suggestion: Initializing the slider index with beats.length - 1 produces -1 when the payload has zero beats, which makes the range input value invalid and leaves the component in an out-of-range state. Initialize to 0 and clamp to valid bounds. [logic error]
Severity Level: Major ⚠️
- ⚠️ Activity Narrative breaks when beats array is empty.
- ⚠️ Scrub label disappears; slider index becomes inconsistent.Steps of Reproduction ✅
1. A host embeds the renderer via `App` (src/renderer/App.tsx), which uses the default
surface adapter from `getRendererSurfaceAdapter()`
(src/renderer/renderer-surface-adapter.tsx:15-24) and mounts `ExhibitStage`
(src/renderer/exhibits/ExhibitStage.tsx:101) as the main exhibit floor.
2. In a live or customized setup, the host passes an `artifacts` prop to `ExhibitStage`
(ExhibitStage.tsx:95-101) that includes an `activity_narrative` `ExhibitArtifact` whose
payload has `beats: []` but a valid `threads` array, matching the
`ActivityNarrativePayload` shape.
3. When that artifact is selected as `current` inside `ExhibitStage`
(ExhibitStage.tsx:115-117), `ActiveExhibit` (ExhibitStage.tsx:72-75) renders
`ActivityNarrative` with `payload={artifact.payload}`, and `ActivityNarrative` initializes
`active` with `useState(beats.length - 1)` at
src/renderer/exhibits/ActivityNarrative.tsx:20; with zero beats this evaluates to `-1`.
4. The scrub control renders with `min={0}`, `max={Math.max(0, beats.length - 1)}` and
`value={active}` at ActivityNarrative.tsx:104-108, producing `min=0`, `max=0`, `value=-1`,
while the label uses `beats[active]?.at` at line 102, which is `undefined`. The browser
clamps the thumb to the 0 position, but the component’s internal index is out of range and
the current beat label is blank, confirming the negative-index bug when `beats` is empty.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/renderer/exhibits/ActivityNarrative.tsx
**Line:** 20:20
**Comment:**
*Logic Error: Initializing the slider index with `beats.length - 1` produces `-1` when the payload has zero beats, which makes the range input value invalid and leaves the component in an out-of-range state. Initialize to `0` and clamp to valid bounds.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+28
to
+29
| const [selected, setSelected] = useState(concerns[0]?.id ?? ''); | ||
| const current = byId[selected] ?? concerns[0]; |
There was a problem hiding this comment.
Suggestion: The selected concern id is initialized from the first payload item only once and never realigned when concerns changes, so after live artifact refreshes it can point to a removed id and desynchronize highlight vs focus-card content. Reset or clamp selection when the concern list updates. [stale reference]
Severity Level: Major ⚠️
- ⚠️ Concern Snapshot focus card can desync from visual highlight.
- ⚠️ Live refreshes may leave selection pointing to removed concern.Steps of Reproduction ✅
1. The `concern_snapshot` artifact from the fixture gallery
(src/renderer/exhibits/fixture-gallery.ts:171-201) is rendered through `ExhibitStage` as
`current`, and `ActiveExhibit` (src/renderer/exhibits/ExhibitStage.tsx:76) mounts
`ConcernSnapshot` with `payload={artifact.payload}`.
2. On initial render `ConcernSnapshot` builds `byId` from `concerns` at
src/renderer/exhibits/ConcernSnapshot.tsx:27 and initializes `selected` to
`concerns[0]?.id ?? ''` at line 28, while `current` is computed as `byId[selected] ??
concerns[0]` at line 29 so that the focus card and selected box match.
3. A user hovers or focuses a different concern box in the map (the `motion.button` loop
at ConcernSnapshot.tsx:60-78), which calls `setSelected(concern.id)` at lines 66-67. The
selected box is highlighted via `selected === concern.id && 'concern-box--selected'` at
lines 68-72, and the focus card mirrors `current` for that id.
4. When a future host refreshes the artifact by changing `payload.concerns` in place (same
artifact id, new concerns array that removes or renames the previously selected id),
`byId` recomputes from the new concerns, but `selected` still holds the stale id.
`current` falls back to `concerns[0]` because `byId[selected]` is undefined, while the
highlight condition `selected === concern.id` no longer matches any entry. The result is a
focus card showing the first concern with no box visually marked as selected, confirming
the desynchronized selection state after live payload updates.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/renderer/exhibits/ConcernSnapshot.tsx
**Line:** 28:29
**Comment:**
*Stale Reference: The selected concern id is initialized from the first payload item only once and never realigned when `concerns` changes, so after live artifact refreshes it can point to a removed id and desynchronize highlight vs focus-card content. Reset or clamp selection when the concern list updates.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+20
to
+22
| const [selected, setSelected] = useState(focusNodeId); | ||
| const byId = useMemo(() => Object.fromEntries(nodes.map((n) => [n.id, n])), [nodes]); | ||
| const selectedNode = byId[selected] ?? nodes.find((n) => n.id === focusNodeId) ?? nodes[0]; |
There was a problem hiding this comment.
Suggestion: selected is initialized from focusNodeId only once and never re-synced, so when the same artifact id is refreshed with a new payload the focused node can stay stale and the UI highlights the wrong node. Sync state when focusNodeId changes (or derive selection directly from props until user interaction). [logic error]
Severity Level: Major ⚠️
⚠️ Relationship DAG focus card shows stale node after refresh.
⚠️ Exhibit floor miscommunicates curator-intended urgent focus entity.Steps of Reproduction ✅
1. App renders `ExhibitStageSurface` from `renderer-surface-adapter.tsx:15-21`, which uses
`ExhibitStage` (ExhibitStage.tsx:101) with its default `artifacts` prop pointing at
`fixtureGallery` (fixture-gallery.ts:316-327).
2. When the active artifact has `exhibitType: 'relationship_dag'`, `ActiveExhibit` in
ExhibitStage.tsx:72-73 renders `RelationshipDag` with `artifact.payload` and the envelope
artifact, wiring in `focusNodeId` from `RelationshipDagPayload` (types.ts:40-44 and
fixture-gallery.ts:92-130).
3. Inside `RelationshipDag`, local state `selected` is initialized once from `focusNodeId`
via `useState(focusNodeId)` at RelationshipDag.tsx:20, and hover handlers update
`selected` to the hovered node id; `selectedNode` resolution at RelationshipDag.tsx:22
always prefers `byId[selected]` over the current `focusNodeId` prop.
4. If the curator or host later refreshes the same artifact id (e.g., `session-entities`
from fixture-gallery.ts:92-105) with a new `focusNodeId` while ExhibitStage keeps using
`artifact.id` as the React key at ExhibitStage.tsx:221, `RelationshipDag` stays mounted
with its old `selected` value and the focus card continues highlighting the stale node
instead of the newly designated focused node.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/renderer/exhibits/RelationshipDag.tsx
**Line:** 20:22
**Comment:**
*Logic Error: `selected` is initialized from `focusNodeId` only once and never re-synced, so when the same artifact id is refreshed with a new payload the focused node can stay stale and the UI highlights the wrong node. Sync state when `focusNodeId` changes (or derive selection directly from props until user interaction).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+87
to
+90
| {points.map((p, idx) => ( | ||
| <line | ||
| key={`spike-${idx}`} | ||
| x1={p.x} |
There was a problem hiding this comment.
Suggestion: Using list index as the key for trace points causes unstable reconciliation when the trace window slides (oldest removed/newest added), which can reuse DOM nodes for different tremors and produce incorrect animations/labels. Use a stable per-point identity (for example a composite of timestamp and kind) instead of idx. [possible bug]
Severity Level: Major ⚠️
⚠️ Seismograph tremor animations inconsistent across sliding trace updates.
⚠️ Time-series visualization less accurate under dynamic artifact refresh.Steps of Reproduction ✅
1. ExhibitStage selects the seismograph artifact `session-seismograph` from
`fixtureGallery` (fixture-gallery.ts:203-234) and renders `Seismograph` when
`isSeismograph(artifact)` is true in ExhibitStage.tsx:78.
2. Seismograph builds `points` from the `trace` array (Seismograph.tsx:39-46) and then
renders spikes, peak dots, and labels by mapping `points` three times, using index-based
keys: `key={'spike-${idx}'}` for spikes (Seismograph.tsx:87-90), `key={'dot-${idx}'}` for
dots (Seismograph.tsx:114-116), and `key={'label-${idx}'}` for labels
(Seismograph.tsx:133-135).
3. Live snapshot updates are driven by App.tsx, which calls `dispatch({ type:
'LIVE_UPDATE', snapshot: liveSnapshot })` inside `refreshLiveSnapshot` whenever new events
arrive (App.tsx:13-21, 26-41), and the exhibit vocabulary in types.ts:175-185 is
explicitly designed so a curator can re-author SeismographPayload for the same artifact id
as live data evolves.
4. When the curator or host implements a sliding time window by dropping the oldest tremor
from `trace` and appending a new one, React reconciliation reuses spikes, dots, and label
DOM nodes at each index due to these index-based keys; new tremors inherit previous nodes’
animation state, so draw-on and entry animations from the `initial` props
(Seismograph.tsx:108-110, 121-123) no longer track the actual tremors being visualized.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/renderer/exhibits/Seismograph.tsx
**Line:** 87:90
**Comment:**
*Possible Bug: Using list index as the key for trace points causes unstable reconciliation when the trace window slides (oldest removed/newest added), which can reuse DOM nodes for different tremors and produce incorrect animations/labels. Use a stable per-point identity (for example a composite of timestamp and kind) instead of `idx`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+122
to
+134
| const hottestIdx = useMemo(() => { | ||
| const byPath = layout.findIndex((l) => l.cell.path === hottest.path); | ||
| if (byPath >= 0) return byPath; | ||
| let max = -1; | ||
| let maxIdx = 0; | ||
| layout.forEach((l, i) => { | ||
| if (l.cell.heat > max) { | ||
| max = l.cell.heat; | ||
| maxIdx = i; | ||
| } | ||
| }); | ||
| return maxIdx; | ||
| }, [layout, hottest.path]); |
There was a problem hiding this comment.
Suggestion: When hottest.path is missing from cells, the code highlights a fallback max-heat rectangle but still renders the callout text from payload.hottest, so the highlighted cell and explanation can disagree. Derive callout content from the same resolved hottest index used for highlighting. [logic error]
Severity Level: Major ⚠️
⚠️ Highlighted hottest tile can disagree with explanatory callout.
⚠️ ThermalMap misleads about true hottest subsystem or file.Steps of Reproduction ✅
1. ThermalMap is wired into the exhibit vocabulary via `ThermalMapPayload`
(types.ts:156-168) and is rendered by ExhibitStage when `isThermalMap(artifact)` is true,
per ExhibitStage.tsx:79 and the type guards in types.ts:245-246.
2. Inside ThermalMap, `layout` is computed from `cells` using the `squarify` helper at
ThermalMap.tsx:113-120, then `hottestIdx` is derived in a memoized block
(ThermalMap.tsx:122-134) by first finding the index where `layout[i].cell.path ===
hottest.path` and, if none is found, falling back to the index of the maximum `cell.heat`
across `layout`.
3. During rendering, each cell uses `const isHottest = i === hottestIdx` to decide whether
to include the `thermal-cell--hottest` CSS class (ThermalMap.tsx:143-150), but the callout
panel below always displays `hottest.path` and `hottest.why` directly from the payload
object (ThermalMap.tsx:176-177).
4. If upstream curation supplies a ThermalMapPayload where `hottest.path` no longer
matches any `cell.path` in `cells`—for example because that file was filtered out,
renamed, or omitted—the fallback path in `hottestIdx` highlights the maximum-heat cell
while the callout continues to describe the stale `hottest.path`, causing the visually
hottest tile and the explanatory “hottest” text to disagree.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/renderer/exhibits/ThermalMap.tsx
**Line:** 122:134
**Comment:**
*Logic Error: When `hottest.path` is missing from `cells`, the code highlights a fallback max-heat rectangle but still renders the callout text from `payload.hottest`, so the highlighted cell and explanation can disagree. Derive callout content from the same resolved hottest index used for highlighting.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+44
to
+45
| key={file.label} | ||
| d={curvedPath(CENTER, { x: file.x, y: file.y }, 5)} |
There was a problem hiding this comment.
Suggestion: file.label is used as the React key even though labels are not guaranteed unique, so duplicate labels will collapse/reuse elements and lose connector or tile rendering. Use a guaranteed-unique identifier for each file entry (or include coordinates/index in a stable composite key). [missing react key]
Severity Level: Major ⚠️
⚠️ Duplicate file labels lose connectors or tiles during render.
⚠️ Walkthrough exhibit misrepresents touched-file set for session.Steps of Reproduction ✅
1. ExhibitStage renders the Walkthrough exhibit when `isWalkthrough(artifact)` returns
true (ExhibitStage.tsx:75); the fixture gallery defines a Walkthrough artifact
`plan-walkthrough` with a `files` array in fixture-gallery.ts:132-168.
2. Walkthrough destructures `files` from the payload (Walkthrough.tsx:20) and uses
`files.map` twice: once for dashed connector paths in the SVG and once for positioned file
tiles, using `key={file.label}` for both connectors and tiles (Walkthrough.tsx:44, 90).
3. The `WalkthroughFile` type in types.ts:78-82 only specifies `label`, `x`, and `y`, with
no uniqueness guarantees on `label`, so a curator implementation can legitimately emit
multiple entries sharing the same `label` when representing repeated touches of the same
file.
4. When `files` contains duplicate labels, React’s reconciliation logic merges or reuses
elements with the same key; later entries will overwrite or reuse DOM nodes created for
earlier ones, causing some dashed connectors or file tiles to be dropped or misaligned and
making the visual “touched files” representation in the Walkthrough exhibit unreliable.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/renderer/exhibits/Walkthrough.tsx
**Line:** 44:45
**Comment:**
*Missing React Key: `file.label` is used as the React key even though labels are not guaranteed unique, so duplicate labels will collapse/reuse elements and lose connector or tile rendering. Use a guaranteed-unique identifier for each file entry (or include coordinates/index in a stable composite key).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Pull request overview
Adds a new “exhibit floor” renderer surface that can present a curated, rotating set of exhibit visualizations (plus an embedded live graph) using a shared visual language and fixture data so the UI renders without any inference configured.
Changes:
- Introduces a typed exhibit artifact vocabulary + fixture gallery, and a new
ExhibitStagesurface that renders/rotates exhibits and archives retired artifacts. - Adds seven exhibit components (ported + newly designed) built on shared
Frame/primitive helpers and new exhibit CSS styling. - Integrates the stage as the primary main-area surface in
App.tsx, addsframer-motion, and adds targeted renderer tests + jsdommatchMediasetup.
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/setup.ts | Adds a jsdom matchMedia stub to support reduced-motion probing in tests. |
| tests/renderer/exhibits/types.test.ts | Tests exhibit type guards, authored type list, and fixture gallery consistency. |
| tests/renderer/exhibits/ExhibitStage.test.tsx | Tests stage ordering, manual next/prev, idle cycling, hover pause, archive behavior, and live graph slot. |
| tests/renderer/exhibits/components.test.tsx | Smoke-tests that each exhibit component renders key content. |
| src/renderer/styles.css | Adds the exhibit-floor CSS: rail, stage frame, primitives, exhibit-specific styles, and reduced-motion overrides. |
| src/renderer/renderer-surface-adapter.tsx | Registers ExhibitStage as a first-class renderer surface slot. |
| src/renderer/exhibits/types.ts | Defines ExhibitArtifact envelope + discriminated payload union and type guards. |
| src/renderer/exhibits/primitives.tsx | Adds shared exhibit primitives (Frame, chips, nodes, SVG helpers, animation variants). |
| src/renderer/exhibits/RelationshipDag.tsx | Implements the curated relationship DAG exhibit view. |
| src/renderer/exhibits/ActivityNarrative.tsx | Implements the folding timeline narrative exhibit view. |
| src/renderer/exhibits/Walkthrough.tsx | Implements the walkthrough deep-dive exhibit view. |
| src/renderer/exhibits/ConcernSnapshot.tsx | Implements the concern/heat snapshot exhibit view. |
| src/renderer/exhibits/Momentum.tsx | Implements the momentum gauge + next-steps exhibit view. |
| src/renderer/exhibits/Seismograph.tsx | Implements the seismic-trace exhibit view. |
| src/renderer/exhibits/ThermalMap.tsx | Implements the squarified treemap thermal/churn exhibit view. |
| src/renderer/exhibits/fixture-gallery.ts | Provides the hand-authored fixture gallery used to render the stage without inference. |
| src/renderer/exhibits/ExhibitStage.tsx | Implements the stage (rail + main frame + rotation + archive + live graph slot). |
| src/renderer/App.tsx | Makes the stage the primary surface and moves legacy panels into a collapsible fallback area. |
| package.json | Adds framer-motion dependency. |
| package-lock.json | Locks framer-motion and its transitive deps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+148
to
+150
| onMouseEnter={() => setPaused(true)} | ||
| onMouseLeave={() => setPaused(false)} | ||
| > |
Comment on lines
+205
to
+207
| <span className={cx('exhibit-pill', paused ? 'exhibit-pill--paused' : 'exhibit-pill--active')}> | ||
| {paused ? 'paused' : 'cycling'} | ||
| </span> |
There was a problem hiding this comment.
Verdict: APPROVE
I found no high-confidence blocking correctness, reliability, security, compatibility, or deploy-safety issues in this head.
Non-blocking follow-ups
- P2 —
src/renderer/exhibits/ActivityNarrative.tsx:21-24,90-93: the payload'sNarrativeThread.labelis never used; the UI renders the rawbeat.threadID. Mapping the beat to its declared thread label would preserve the curator-facing story names when PR-C starts sending model-authored IDs. - P2 —
src/renderer/exhibits/ExhibitStage.tsx:124-130,144-150andsrc/renderer/exhibits/ActivityNarrative.tsx:28-34: automatic movement pauses for pointer hover, but not for keyboard focus/activation, and the narrative has its own timer. Consider pausing on focus-within/interaction so keyboard and assistive-technology users can inspect a stable exhibit. - P2 —
src/renderer/styles.css:1142-1149,1350-1360,1392-1406: the 560px minimum frame plus fixed-width absolute cards can clip content on narrow windows because the frame usesoverflow: hidden. Add a narrow-viewport layout/size treatment before treating the stage as mobile-responsive.
Integration note
This PR intentionally defaults ExhibitStage to fixtureGallery (src/renderer/exhibits/ExhibitStage.tsx:101), and App does not yet pass live/replay artifacts (src/renderer/App.tsx:504-507). That matches the accepted PR-B/PR-C split, but PR-C must replace the fixture source before the stage is used as live-session truth.
Validation
- Reviewed the complete 20-file patch, related renderer contracts/call sites, accepted exhibit-floor plan, architecture/north-star/roadmap guidance, tests, dependency lockfile, PR discussion, review threads, and current checks.
- Existing GitHub checks for this head:
testpassed; GitGuardian passed. Copilot review was still in progress and Kilo review was queued when inspected. - No local CI-style commands were rerun during this review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Implements PR-B of the accepted exhibit-floor design (
docs/plans/plan-exhibit-floor.md): the pre-built exhibit component library and the museum-floor stage, so the shadow LLM can later curate a live gallery. Renders the full exhibit floor today with zero inference configured, off a hand-authored fixture gallery of the real Codex homelab session.What shipped
src/renderer/exhibits/types.ts— theExhibitArtifactenvelope, a discriminated payload union for the 7 authored exhibit types +live_graph, and type guards.src/renderer/exhibits/primitives.tsx— ported prototype primitives (Framewith radial glows + faint 60px grid + vignette,accentByKind,Chip,ExhibitNode,curvedPath,arcPath), typed and on plain CSS classes (no Tailwind), respectingprefers-reduced-motionvia framer-motion'suseReducedMotion.RelationshipDag,ActivityNarrative,Walkthrough,ConcernSnapshot,Momentum(ported from the five prototype views, same composition and motion: draw-on edges, folding timeline, satellites, heat boxes, gauge + needle + confidence bars) plus two newly designed in the same language:Seismograph(horizontal time axis, kind-colored magnitude spikes, left-to-right trace draw-on) andThermalMap(squarified treemap, cool-blue to hot-red heat lerp, white-hot hottest cell + why callout).ExhibitStage.tsx— gallery rail (icon, status, relevance bar, narrative preview), mainFramewithAnimatePresenceswap, glass agent-commentary card, relevance-weighted idle cycling every 8s that pauses on hover (visible pill + manual prev/next), and an archive shelf for retired artifacts.live_graphrenders the existingCanvasRendererinside the Frame.fixture-gallery.ts— the real homelab session as a standing exhibition (plan pivot, PG18 eight-database recovery, urgent exposedFORGEJO_INTERNAL_TOKEN, GHCR-403 stall + turn abort in the seismograph, a momentum exhibit that reads NOT done, a walkthrough of the plan-authoring turn, a retired early-guess exhibit). Narratives are interpretive, not event recaps.renderer-surface-adapter.tsxand made the primary main-area surface inApp.tsx; the legacy panels move into a collapsible area and the canvas is preserved as thelive_graphexhibit.styles.cssextended with the exhibit classes (Frame, glows, grid, rail, commentary, chips, heat classes, accent gradients) following existing var conventions.Tests
tests/renderer/exhibits/adds 20 tests (component render per exhibit, envelope type-guards + fixture consistency, stage: fixture render, relevance-weighted rotation, 8s auto-cycle, pause-on-hover, archive shelf). Full suite: 494 passed (tsc --noEmit && vitest run), renderer build clean.Deviations from the plan
ExhibitStage. To preserve the existingapp.integration/app-graph-props/app-live-refreshtests unchanged, App guards the stage render (ExhibitStageSurface ? ... : null) and renders the legacyGraphpanel (which those tests rely on for canvas prop-forwarding) only when the stage surface is absent. In production the stage is always present, so the canvas lives solely in thelive_graphexhibit — no double canvas.momentumcuration cards are shown only on taller frames (min-height: 760px) to avoid crowding the gauge; the gauge, stats, and what's-next always render.matchMediastub totests/setup.ts(framer-motion'suseReducedMotionprobes it).🤖 Generated with Claude Code
https://claude.ai/code/session_01U9EpjZNoWaXLzfG93tdvTU
CodeAnt-AI Description
Add a curated exhibit floor with auto-rotating visual stories
What Changed
Impact
✅ Clearer session storytelling✅ Faster review of key events✅ Fewer missing-stage failures in existing tests💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.