Give log-viewer rows a stable unique key, so messages stop stacking on top of each other#587
Conversation
Rows in the virtualized session log were keyed `entry.uuid || entry.timestamp`. `baseEntry` leaves `uuid` as "" for every CLI that writes no per-record uuid (Codex, Copilot, Cursor, Pi), so the key degraded to the timestamp — which those CLIs reuse freely: one 771-record Codex session carries 93 timestamps shared by 2-5 records each. Duplicate React keys break reconciliation, and in a virtualized list the fallout is worse than a warning: orphaned DOM nodes are stranded at their old transform and never removed, so an unrelated message paints over the one you are reading and scrolling away and back does not clear it. Reproduced against a real Codex transcript — after one segment collapse, data-index 2 had two nodes and data-index 3 had three, one of them a message from 58 minutes later drawn across the two beneath it. Entries now take their identity from `buildEntryKeys` (lib/entry-keys.ts), which prefers the real uuid and disambiguates collisions by occurrence order. The same map is threaded into `getSegmentId` — queue-operation dividers are uuid-less even on Claude, so two sharing a millisecond collapsed into one segment — and into the virtualizer as `getItemKey`, which was absent entirely: TanStack defaults to keying its size and element caches by index, so collapsing a segment shifted every index and replayed one entry's cached height onto a different entry, misplacing rows independently of the key bug. The nested subagent list carried the same duplicate key. `buildEntryKeys` lives in its own dependency-free module rather than in log-entries.ts on purpose: that module reaches fs/promises through its project-resolution imports, so a *value* import of it from a client component pulls node:fs into the browser bundle and 500s the session page. Client code may only import types from there. Also fixes the Policies filter placeholder, which rendered the literal text `…` — a JSX attribute string is not a JS string literal, so the escape was painted verbatim beside a sibling input that had it right. Latent since before the open-source release but unreachable until #226 routed Codex transcripts into the Claude viewer. Known gap left open: EntryRow still anchors on entry.uuid, so #entry-… deep links stay inert for uuid-less transcripts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe session log viewer now generates collision-resistant entry keys shared by segment logic, virtualization, and nested subagent rows. The Activity policy filter placeholder uses a literal ellipsis, and both fixes are recorded in the changelog. ChangesLog viewer keying
Policy filter placeholder
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@app/policies/hooks-client.tsx`:
- Line 578: Add or update a unit test for the ActivityTab component to render it
and assert that the policy input exposes the exact placeholder value “filter by
policy…”.
In `@CHANGELOG.md`:
- Line 7: Correct the changelog entry’s before/after description: state that the
previous escaped representation rendered verbatim, and that the implementation
now uses the literal ellipsis character. Keep the policy filter and session
filter context and issue reference unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8fb86273-0ab1-4829-82da-eb9e11f6b1d1
📒 Files selected for processing (6)
CHANGELOG.md__tests__/lib/build-entry-keys.test.tsapp/components/log-viewer/entry-row.tsxapp/components/raw-log-viewer.tsxapp/policies/hooks-client.tsxlib/entry-keys.ts
|
Automated code review started - full review. Results will be posted here. |
|
⏳ [progress] Phase 0-1 complete: read existing comments (2 coderabbit threads, both resolved), full diff analysis done. CI checks: build ✅, quality ✅, test ✅, test-e2e ✅, docs ✅, OSV-Scanner ❌ (pre-existing). Proceeding to Phase 2 deep analysis and posting review. |
|
✅ Build & test complete. Results:
Local verification with |
🔍 Automated Code Review📋 Executive SummaryThis PR fixes a critical rendering bug where log viewer rows with duplicate keys overlapped after segment collapse. The fix introduces a stable, collision-resistant key system ( 📊 Change Architecturegraph TD
A[LogEntry[] entries] --> B[buildEntryKeys]
B -->|"Map<LogEntry, string>"| C[React key: virtualRow.key]
B --> D[getSegmentId: stable segment ids]
B --> E[getItemKey: virtualizer cache]
B --> F[SubagentToolCard keys]
C --> G[No duplicate React keys]
D --> H[No collapsed segment merges]
E --> I[Stable height measurements]
F --> J[Subagent rows stable]
style B fill:#90EE90
style H fill:#90EE90
style I fill:#90EE90
Legend: 🟢 New | 🔵 Modified | 🟡 Breaking Change Risk 🔴 Breaking Changes✅ No breaking changes detected. Pure bugfix — no API, DB schema, config, or dependency changes.
|
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Automated review: Approved. ✅
- All 2335 unit tests pass (including 7 new build-entry-keys tests)
- All 307 e2e tests pass
- CI: build, quality, tests, docs all green
- No breaking changes
- Two minor suggestions (non-blocking): getItemKey fallback warning, component-level rendering test
- Both CodeRabbit threads already resolved
- Code is clean, well-structured, and addresses the root cause comprehensively
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Automated review: Approved. ✅
The advisory published after main's last green scan, so every branch went red at once with no dependency change of its own — re-running main's last Supply Chain scan on its unchanged commit reproduced the failure. Both affected copies were transitive: 5.0.6 via minimatch@10, and three copies of 1.1.15 via the minimatch@3 that eslint-plugin-import, -jsx-a11y and -react still pull. `bun update brace-expansion` is the wrong tool: it adds the package as a direct dependency and lifts only the top-level copy, leaving the nested 5.0.6 and all three 1.1.15 copies vulnerable. Bun also ignores npm's range-keyed (`brace-expansion@^1.1.7`) and yarn's nested-path (`parent/child/pkg`) override forms — resolving nothing, silently — so the two majors cannot be pinned separately. Only the plain-key `overrides` form takes effect, as the existing postcss/vite/undici pins show. A single pin therefore also hands v5 to minimatch@3, which declares ^1.1.7. That is safe in practice: the package's entire surface is one `expand()` function whose signature is unchanged across these majors, and eslint exercises that path directly — lint runs clean with the same five pre-existing warnings. Verified with CI's own scanner image (ghcr.io/google/osv-scanner-action:v2.3.8) against the updated lockfile: "No issues found", exit 0. osv-scanner.toml keeps its zero ignored vulnerabilities. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Automated code review started - full review. Results will be posted here. |
|
⏳ [progress] Phase 0-1 complete. Analyzing 3 commits since last review:
New since previous review: brace-expansion override, CHANGELOG updates with PR numbers. Two coderabbit threads already resolved, no human review comments. Proceeding to deep analysis and build verification. |
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Automated review test
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Automated Code Review
Executive Summary
This PR fixes a critical rendering bug where log viewer rows with duplicate keys overlapped after segment collapse. The fix introduces buildEntryKeys - a collision-resistant key system shared across React keys, segment IDs, and the virtualizer measurement cache. Three new commits since the last review: (1) the brace-expansion CVE pin (84a6d87) which cleared the OSV-Scanner CI gate, (2) changelog PR number completion (478795f), and (3) the original log-viewer fix (e96a456). All 11 CI checks are now green including OSV-Scanner. Code quality remains excellent.
Breaking Changes
No breaking changes detected. Pure bugfix + CVE mitigation.
Issue Found
[Minor] app/components/raw-log-viewer.tsx:353 - getItemKey fallback silently reverts to index-based keying if entryKeys and visibleEntries fall out of sync. Consider adding console.warn for defense-in-depth.
Deep Analysis
buildEntryKeys correctness: Sound implementation. Key identity uses entry.uuid or source:timestamp, collisions disambiguated by occurrence-order #N suffixes.
Virtualizer integration: Correct fix pattern using virtualRow.key + getItemKey. All three subsystems (React keys, segment IDs, virtualizer cache) share one identity map.
Module separation: lib/entry-keys.ts uses import type only to avoid node:fs bundle issue. Well documented.
Brace-expansion pin: Well-researched. bun update is wrong tool, bun ignores nested override syntax, only plain-key overrides works. Single pin to 5.0.7 is safe - one expand() function unchanged across majors, eslint exercises it directly. Verified with CI scanner locally.
lockfile: Correctly removes 4 nested vulnerable copies + deduplicates tailwindcss entries.
Evidence - CI Checks (all 11 pass)
build/docs/quality/test variants/test-e2e all pass. OSV-Scanner now passes (was failing before brace-expansion pin). buildEntryKeys has 7 tests covering all edge cases.
Human Review Feedback
No human review comments. Both CodeRabbit bot threads are resolved. Previous hermes-exosphere review approved with two minor suggestions that still apply.
Verdict
VERDICT: APPROVED
Automated code review - 2026-07-21
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Automated review: Approved. All CI green, no breaking changes, one minor suggestion (getItemKey warning). Previous CodeRabbit threads resolved. Previous review suggestions still applicable.
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Automated review: Approved. ✅
3 participants
The bug
Opening a Codex session in the dashboard and collapsing a session divider leaves messages painted on top of each other, permanently — scrolling away and back does not clear it.
Root cause
Rows in the virtualized log were keyed:
baseEntryleavesuuidas""for every CLI that writes no per-record uuid — Codex, Copilot, Cursor, Pi — so the key silently degraded to the timestamp. Those CLIs reuse timestamps freely:Duplicate React keys break reconciliation. In a virtualized list the fallout is worse than a console warning: orphaned DOM nodes keep their old
transformand are never removed.Measured against a real Codex transcript, after one segment collapse:
Fix
Three parts, all needed:
lib/entry-keys.ts(new) —buildEntryKeysgives every entry a unique, render-stable identity: real uuid when present, collisions disambiguated by occurrence order.getItemKeyon the virtualizer — it was absent. TanStack defaults to keying its size/element caches by index, so collapsing a segment shifted every index and replayed one entry's cached height onto a different entry. This misplaced rows independently of the key bug.getSegmentIduses the same map — queue-operation dividers are uuid-less even on Claude, so two sharing a millisecond collapsed into one segment.The nested subagent list had the same duplicate key and is fixed alongside.
Why
buildEntryKeysis its own modulelog-entries.tsreachesfs/promisesthrough its project-resolution imports. A value import of it from a client component pullsnode:fsinto the browser bundle and 500s the session page — this was hit and backed out during development. Client code may only import types from there. The module carries a comment saying so.Also fixed
app/policies/hooks-client.tsxrendered the literal text…in the policy filter placeholder — a JSX attribute string is not a JS string literal, so the escape was painted verbatim, beside a sibling input that had it right. Found by looking at the rendered page.Verification
Behavioural check against the built artifact, driving real Chrome over CDP:
Claude sessions re-checked for regression (clean), and all six dashboard routes screenshot-inspected with zero console errors.
Scope notes
scrollMargin), which is why the remaining symptom presented intermittently rather than on every load.file-history-snapshotentries.EntryRowstill anchors onentry.uuid, so#entry-…deep links and the copy-link button remain inert for uuid-less transcripts. Fixing that means replacing uuid-as-identity throughout the viewer including subagent deep-linking — a larger, riskier diff, deliberately not bundled here.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests