Fix ARIA ID uniqueness in HistoryView#17
Merged
Conversation
Include array index in id and aria-controls attributes to prevent duplicate HTML IDs when multiple sessions share the same dayNumber. Closes #16 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds Changes
Sequence Diagram(s)sequenceDiagram
participant UI as HistoryView (Client)
participant API as /api/thoughts (Server)
participant DB as Thoughts DB
UI->>API: GET /api/thoughts
API->>DB: query thoughts (select sessionId)
DB-->>API: thoughts[] (with sessionId)
API-->>UI: thoughts[] (with sessionId)
UI->>UI: group thoughts by sessionId (useMemo)
UI->>UI: build entries with entryId = day + index
UI->>UI: determine canExpand via getThoughtsForSession(sessionId)
UI->>UI: toggle expandedEntryId when entry activated (click/Enter/Space)
Note over UI: Expanded region renders entryThoughts (t.id keys)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/HistoryView.tsx`:
- Around line 211-213: The expansion state is currently keyed only by entry.day
(expandedDay) so multiple entries from the same day toggle together; change to
an entry-scoped key (e.g. create a new state variable expandedEntry and setter
setExpandedEntry) and use a unique key such as `${entry.day}-${idx}` or the
entry's stable id instead of entry.day; update the aria-expanded checks and
onClick handlers (replace expandedDay with expandedEntry and setExpandedDay(...)
with setExpandedEntry(...) using the same unique key) so each entry
expands/collapses independently and preserves unique region labeling.
Switch expandedDay (keyed by dayNumber) to expandedEntryId (keyed by day-idx composite) so duplicate-day entries expand independently. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/HistoryView.tsx`:
- Around line 216-218: The aria-expanded state is being set to true even when no
region is rendered (entryThoughts.length === 0), causing an accessibility
mismatch; update the logic around aria-expanded and the click handler (where
setExpandedEntryId is used) to consider a hasThoughts boolean (e.g., const
hasThoughts = entryThoughts.length > 0) and only set aria-expanded to true when
isExpanded && hasThoughts, and prevent toggling expansion when hasThoughts is
false (or alternatively disable the toggle UI) in the onClick; apply the same
change where aria-expanded and setExpandedEntryId are used (including the other
occurrences mentioned).
- Around line 88-89: getThoughtsForDay currently filters only by day
(thoughts.filter(t => t.dayNumber === day)), which merges thoughts across
sessions and causes duplicate/misaligned panels; change the logic to scope
thoughts to the specific entry's session by filtering on both dayNumber and
sessionId (e.g., add an argument or create getThoughtsForEntry that uses
entry.sessionId and entry.day), and update all call sites that use
getThoughtsForDay (places referencing entry.day/entry.thoughtCount and any other
usages) so the returned list matches entry.thoughtCount and is limited to the
same session.
- Return sessionId from /api/thoughts endpoint - Add sessionId to Thought type in api.ts - Filter thoughts per session (not per day) to prevent cross-session merging - Add canExpand guard: only set aria-expanded/aria-controls when thoughts exist - Prevent click/keydown expansion on entries with no captured thoughts Addresses CR review round 2 findings on PR #17. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/HistoryView.tsx`:
- Around line 90-92: The current getThoughtsForSession function causes a full
scan of thoughts for every row render; refactor inside the HistoryView component
to pre-group thoughts by sessionId once (e.g., build a Map or object keyed by
sessionId) using useMemo with thoughts as the dependency, then replace
getThoughtsForSession to simply lookup the grouped map by sessionId (returning
[] if missing); ensure symbols referenced are getThoughtsForSession, thoughts,
sessionId and that the grouping memoization lives in the same component so
renders reuse the precomputed groups.
- Around line 217-220: The row currently sets role="button" and tabIndex={0}
regardless of canExpand, causing a non-actionable focusable element; update the
JSX in HistoryView (the element using canExpand, entryId, isExpanded) so role
and tabIndex are only added when canExpand is true (e.g., role={canExpand ?
"button" : undefined} and tabIndex={canExpand ? 0 : undefined}), keeping
aria-expanded and aria-controls behavior as-is.
- Pre-group thoughts by sessionId with useMemo to avoid per-row filter scans
- Only set role="button" and tabIndex={0} when canExpand is true
- Non-expandable rows no longer receive focus or button semantics
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/HistoryView.tsx`:
- Around line 319-320: The list rendering inside entryThoughts.map uses the
array index as the React key (key={i}) which can cause incorrect DOM reuse;
change the key to the stable Thought id (use t.id from the mapped item) in the
map callback where entryThoughts.map((t, i) => ...) and ensure the mapped
element uses that id as its key instead of i to provide stable identity for each
Thought.
Replace array index keys with t.id for stable DOM identity in the expanded thoughts panel. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
This was referenced Mar 27, 2026
14 tasks
This was referenced Apr 30, 2026
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
1 participant
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.
Summary
idandaria-controlsattributes in HistoryView expanded thought panelsdayNumber(e.g., abandoned + retried)Closes #16
Test plan
aria-controlsusesday-${entry.day}-${idx}-thoughtspatterniduses matchingday-${entry.day}-${idx}-thoughtspatternnpm run buildpasses without errors🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Improvements
Documentation