feat(sidebar-tag-collapse-and-delete): collapsible tag area + guarded global tag delete - #387
Conversation
… global tag delete Implements OpenSpec change sidebar-tag-collapse-and-delete: - Single master collapse over the sidebar tag+phase area (default collapsed, persisted; count + active-filter indicator + fold-free clear on the header). - Overflow cap (10) + `+N more`/`show less` inline expander on the user-tag group. - Destructive per-tag remove (guarded confirm dialog) that strips a tag from every carrying session via the new `remove_tag_globally` browser->server verb (server fan-out; one session_updated per changed session). Archives + syncs specs. QA/manual tasks (9.1, X4) deferred to post-merge.
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughThe sidebar tag area now collapses as one persisted section, caps visible user tags with an overflow control, and supports confirmed global removal of user tags. A new browser protocol message, server fan-out handler, client action, UI components, specifications, mockup, and tests implement the behavior. ChangesSidebar Tags
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SessionList
participant BrowserGateway
participant SessionMetaHandler
participant SessionManager
User->>SessionList: confirm tag removal
SessionList->>BrowserGateway: send remove_tag_globally
BrowserGateway->>SessionMetaHandler: route message
SessionMetaHandler->>SessionManager: update matching sessions
SessionManager-->>User: broadcast session_updated events
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
packages/client/src/components/session/SessionList.tsx (1)
1361-1382: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded count/badge strings bypass
t().The count (
{allTags.length} tag{…}s · {phasesInUse.length} phase{…}s) and the{activeFilterCount} activebadge are literal English, while the surrounding component routes all user-facing copy throught(). Consider localizing these for consistency.🤖 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 `@packages/client/src/components/session/SessionList.tsx` around lines 1361 - 1382, Localize the tag/phase count text and active-filter badge in the tag filter bar by routing their user-facing strings through the component’s existing t() helper. Preserve the current pluralization and interpolated counts, and update the “active” badge text without changing its display conditions or styling.
🤖 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
`@openspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/.openspec.yaml`:
- Around line 1-2: Move the entire change directory from
openspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/ to
openspec/changes/2026-07-22-sidebar-tag-collapse-and-delete/, preserving
.openspec.yaml, design.md, proposal.md, specs/session-tags/spec.md, tasks.md,
test-plan.md, and the mockups subtree. Update mockups/ux-review.md to reference
the relocated mockup, with no content changes required elsewhere.
In
`@openspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/mockups/index.html`:
- Around line 61-66: Update the remove control styling around .chip .rm and the
toggle styling around the referenced toggle rule to meet the documented
accessibility floor: ensure interactive targets are at least 24px and preserve a
clearly visible focus indicator instead of removing the native outline with all:
unset. Keep the existing visual states and behavior unchanged.
- Around line 102-118: Update the tag-filter mockup around the area header and
chip rendering logic to model active selections: maintain selection state,
toggle each chip’s aria-pressed value, and reflect selected filters in the
header counts. Add a folded-state clear-filters control that appears when
filters are active and clears all selected tags and phases, including the
corresponding count and accessibility updates.
In
`@openspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/test-plan.md`:
- Around line 17-18: Align the test-plan entries E1, E2, F2, F4, and X2 with
their implemented component-test level instead of L3, and update the L3 coverage
summary to match. Preserve the declared Playwright coverage definition; only add
Playwright tests if retaining those rows as L3.
In `@packages/client/src/components/session/SessionList.tsx`:
- Around line 1433-1439: Update the TagDeleteConfirmDialog onConfirm handler to
remove pendingDeleteTag from selectedTags before or alongside calling
onRemoveTagGlobally, while preserving other selected tags and the existing
global removal behavior. Use the selectedTags state update mechanism and ensure
the confirmed tag is no longer passed to passesTagAxes after deletion.
In `@packages/client/src/components/tags/AGENTS.md`:
- Line 10: Reorder the file table entry for TagDeleteConfirmDialog.tsx so it
appears before TagEditor.tsx, maintaining strict path-alphabetical ordering
while leaving the entry content unchanged.
In `@packages/server/src/browser-handlers/session-meta-handler.ts`:
- Around line 91-95: Update the global tag-removal loop in the session metadata
handler to perform each session’s tag removal as an atomic, serialized
read-modify-write operation. Re-read tags within the per-session mutation or use
the session manager’s compare-and-swap/retry mechanism, then remove only the
target tag while preserving concurrent updates and making repeated global
deletions safe.
- Around line 89-90: Validate msg.tag with a runtime typeof check ensuring it is
a string before calling normalizeTags in the session metadata handler; for null
or other invalid payload values, return the handler’s established structured
error or no-op path. Preserve normalization for valid string tags and prevent
malformed input from reaching normalizeTags.
---
Nitpick comments:
In `@packages/client/src/components/session/SessionList.tsx`:
- Around line 1361-1382: Localize the tag/phase count text and active-filter
badge in the tag filter bar by routing their user-facing strings through the
component’s existing t() helper. Preserve the current pluralization and
interpolated counts, and update the “active” badge text without changing its
display conditions or styling.
🪄 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: e568658d-de69-4a78-8243-ab4817bc64f1
📒 Files selected for processing (31)
openspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/.openspec.yamlopenspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/design.mdopenspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/mockups/index.htmlopenspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/mockups/ux-review.mdopenspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/proposal.mdopenspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/specs/session-tags/spec.mdopenspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/tasks.mdopenspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/test-plan.mdopenspec/specs/session-tags/spec.mdpackages/client/src/App.tsxpackages/client/src/components/__tests__/SessionList.seek-to-card.test.tsxpackages/client/src/components/__tests__/SessionList.tags-filter.test.tsxpackages/client/src/components/session/SessionList.tsxpackages/client/src/components/session/SessionList.tsx.AGENTS.mdpackages/client/src/components/tags/AGENTS.mdpackages/client/src/components/tags/TagChip.tsxpackages/client/src/components/tags/TagDeleteConfirmDialog.tsxpackages/client/src/components/tags/TagFilterGroup.tsxpackages/client/src/components/tags/__tests__/tags-components.test.tsxpackages/client/src/hooks/AGENTS.mdpackages/client/src/hooks/useSessionActions.tspackages/client/src/lib/session/session-filter-storage.tspackages/client/src/lib/session/session-filter-storage.ts.AGENTS.mdpackages/server/src/browser-handlers/AGENTS.mdpackages/server/src/browser-handlers/__tests__/session-meta-handler.test.tspackages/server/src/browser-handlers/session-meta-handler.tspackages/server/src/pairing/browser-gateway.tspackages/server/src/pairing/browser-gateway.ts.AGENTS.mdpackages/shared/src/AGENTS.mdpackages/shared/src/browser-protocol.tstests/e2e/session-tags.spec.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 8
🧹 Nitpick comments (1)
packages/client/src/components/session/SessionList.tsx (1)
1361-1382: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded count/badge strings bypass
t().The count (
{allTags.length} tag{…}s · {phasesInUse.length} phase{…}s) and the{activeFilterCount} activebadge are literal English, while the surrounding component routes all user-facing copy throught(). Consider localizing these for consistency.🤖 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 `@packages/client/src/components/session/SessionList.tsx` around lines 1361 - 1382, Localize the tag/phase count text and active-filter badge in the tag filter bar by routing their user-facing strings through the component’s existing t() helper. Preserve the current pluralization and interpolated counts, and update the “active” badge text without changing its display conditions or styling.
🤖 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
`@openspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/.openspec.yaml`:
- Around line 1-2: Move the entire change directory from
openspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/ to
openspec/changes/2026-07-22-sidebar-tag-collapse-and-delete/, preserving
.openspec.yaml, design.md, proposal.md, specs/session-tags/spec.md, tasks.md,
test-plan.md, and the mockups subtree. Update mockups/ux-review.md to reference
the relocated mockup, with no content changes required elsewhere.
In
`@openspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/mockups/index.html`:
- Around line 61-66: Update the remove control styling around .chip .rm and the
toggle styling around the referenced toggle rule to meet the documented
accessibility floor: ensure interactive targets are at least 24px and preserve a
clearly visible focus indicator instead of removing the native outline with all:
unset. Keep the existing visual states and behavior unchanged.
- Around line 102-118: Update the tag-filter mockup around the area header and
chip rendering logic to model active selections: maintain selection state,
toggle each chip’s aria-pressed value, and reflect selected filters in the
header counts. Add a folded-state clear-filters control that appears when
filters are active and clears all selected tags and phases, including the
corresponding count and accessibility updates.
In
`@openspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/test-plan.md`:
- Around line 17-18: Align the test-plan entries E1, E2, F2, F4, and X2 with
their implemented component-test level instead of L3, and update the L3 coverage
summary to match. Preserve the declared Playwright coverage definition; only add
Playwright tests if retaining those rows as L3.
In `@packages/client/src/components/session/SessionList.tsx`:
- Around line 1433-1439: Update the TagDeleteConfirmDialog onConfirm handler to
remove pendingDeleteTag from selectedTags before or alongside calling
onRemoveTagGlobally, while preserving other selected tags and the existing
global removal behavior. Use the selectedTags state update mechanism and ensure
the confirmed tag is no longer passed to passesTagAxes after deletion.
In `@packages/client/src/components/tags/AGENTS.md`:
- Line 10: Reorder the file table entry for TagDeleteConfirmDialog.tsx so it
appears before TagEditor.tsx, maintaining strict path-alphabetical ordering
while leaving the entry content unchanged.
In `@packages/server/src/browser-handlers/session-meta-handler.ts`:
- Around line 91-95: Update the global tag-removal loop in the session metadata
handler to perform each session’s tag removal as an atomic, serialized
read-modify-write operation. Re-read tags within the per-session mutation or use
the session manager’s compare-and-swap/retry mechanism, then remove only the
target tag while preserving concurrent updates and making repeated global
deletions safe.
- Around line 89-90: Validate msg.tag with a runtime typeof check ensuring it is
a string before calling normalizeTags in the session metadata handler; for null
or other invalid payload values, return the handler’s established structured
error or no-op path. Preserve normalization for valid string tags and prevent
malformed input from reaching normalizeTags.
---
Nitpick comments:
In `@packages/client/src/components/session/SessionList.tsx`:
- Around line 1361-1382: Localize the tag/phase count text and active-filter
badge in the tag filter bar by routing their user-facing strings through the
component’s existing t() helper. Preserve the current pluralization and
interpolated counts, and update the “active” badge text without changing its
display conditions or styling.
🪄 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: e568658d-de69-4a78-8243-ab4817bc64f1
📒 Files selected for processing (31)
openspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/.openspec.yamlopenspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/design.mdopenspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/mockups/index.htmlopenspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/mockups/ux-review.mdopenspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/proposal.mdopenspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/specs/session-tags/spec.mdopenspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/tasks.mdopenspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/test-plan.mdopenspec/specs/session-tags/spec.mdpackages/client/src/App.tsxpackages/client/src/components/__tests__/SessionList.seek-to-card.test.tsxpackages/client/src/components/__tests__/SessionList.tags-filter.test.tsxpackages/client/src/components/session/SessionList.tsxpackages/client/src/components/session/SessionList.tsx.AGENTS.mdpackages/client/src/components/tags/AGENTS.mdpackages/client/src/components/tags/TagChip.tsxpackages/client/src/components/tags/TagDeleteConfirmDialog.tsxpackages/client/src/components/tags/TagFilterGroup.tsxpackages/client/src/components/tags/__tests__/tags-components.test.tsxpackages/client/src/hooks/AGENTS.mdpackages/client/src/hooks/useSessionActions.tspackages/client/src/lib/session/session-filter-storage.tspackages/client/src/lib/session/session-filter-storage.ts.AGENTS.mdpackages/server/src/browser-handlers/AGENTS.mdpackages/server/src/browser-handlers/__tests__/session-meta-handler.test.tspackages/server/src/browser-handlers/session-meta-handler.tspackages/server/src/pairing/browser-gateway.tspackages/server/src/pairing/browser-gateway.ts.AGENTS.mdpackages/shared/src/AGENTS.mdpackages/shared/src/browser-protocol.tstests/e2e/session-tags.spec.ts
🛑 Comments failed to post (8)
openspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/.openspec.yaml (1)
1-2: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Move this OpenSpec change out of
archive/.These artifacts are being introduced under a forbidden location, preventing the change from following the repository’s required OpenSpec layout.
openspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/.openspec.yaml#L1-L2: move metadata with the change directory.openspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/design.md#L1-L60: move to the non-archive change directory.openspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/proposal.md#L1-L35: move to the non-archive change directory.openspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/specs/session-tags/spec.md#L1-L117: preserve the delta spec under the relocated change directory.openspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/tasks.md#L1-L66: move with the change artifacts.openspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/test-plan.md#L1-L55: move with the change artifacts.openspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/mockups/index.html#L1-L252: move with the mockup subtree.openspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/mockups/ux-review.md#L1-L46: move with the mockup subtree and update its mockup reference.As per coding guidelines,
openspec/changes/**/*artifacts must be stored underopenspec/changes/<name>/, never underactive/orarchive/.📍 Affects 8 files
openspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/.openspec.yaml#L1-L2(this comment)openspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/design.md#L1-L60openspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/proposal.md#L1-L35openspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/specs/session-tags/spec.md#L1-L117openspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/tasks.md#L1-L66openspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/test-plan.md#L1-L55openspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/mockups/index.html#L1-L252openspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/mockups/ux-review.md#L1-L46🤖 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 `@openspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/.openspec.yaml` around lines 1 - 2, Move the entire change directory from openspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/ to openspec/changes/2026-07-22-sidebar-tag-collapse-and-delete/, preserving .openspec.yaml, design.md, proposal.md, specs/session-tags/spec.md, tasks.md, test-plan.md, and the mockups subtree. Update mockups/ux-review.md to reference the relocated mockup, with no content changes required elsewhere.Source: Coding guidelines
openspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/mockups/index.html (2)
61-66: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Meet the documented accessibility floor in the mockup.
Line 62 limits the remove target to 16px, and Line 194 removes the toggle’s native focus outline with
all: unset. This contradicts the stated ≥24px target and visible-focus checks.Proposed adjustment
- .chip .rm{margin-left:1px;border:none;background:none;cursor:pointer;color:currentColor; - opacity:.45;font-size:11px;line-height:1;padding:0 3px;border-radius:3px;min-width:16px} + .chip .toggle:focus-visible, + .chip .rm:focus-visible{outline:2px solid currentColor;outline-offset:2px} + .chip .rm{margin-left:1px;border:none;background:none;cursor:pointer;color:currentColor; + opacity:.45;font-size:11px;line-height:1;padding:0;border-radius:3px; + min-width:24px;min-height:24px;display:inline-flex;align-items:center;justify-content:center}- chip.innerHTML=`<button style="all:unset;cursor:pointer" aria-pressed="false" aria-label="Filter by tag ${name}">#${name}</button>`; + chip.innerHTML=`<button class="toggle" aria-pressed="false" aria-label="Filter by tag ${name}">#${name}</button>`;Also applies to: 192-198
🤖 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 `@openspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/mockups/index.html` around lines 61 - 66, Update the remove control styling around .chip .rm and the toggle styling around the referenced toggle rule to meet the documented accessibility floor: ensure interactive targets are at least 24px and preserve a clearly visible focus indicator instead of removing the native outline with all: unset. Keep the existing visual states and behavior unchanged.
102-118: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Model the required folded active-filter behavior.
The mockup has no selection state: chip toggles remain
aria-pressed="false", the header only renders counts, and no folded clear-filters control exists. It therefore cannot validate the active-selection requirement.Also applies to: 192-205, 219-222
🤖 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 `@openspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/mockups/index.html` around lines 102 - 118, Update the tag-filter mockup around the area header and chip rendering logic to model active selections: maintain selection state, toggle each chip’s aria-pressed value, and reflect selected filters in the header counts. Add a folded-state clear-filters control that appears when filters are active and clears all selected tags and phases, including the corresponding count and accessibility updates.openspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/test-plan.md (1)
17-18: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the declared test level with the implemented tests.
This plan defines L3 as Playwright, but
tasks.mdrecords E1, E2, F2, F4, and X2 as component tests. Correct these rows and the L3 coverage summary, or add the corresponding Playwright coverage.Also applies to: 31-33, 40-40
🤖 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 `@openspec/changes/archive/2026-07-22-sidebar-tag-collapse-and-delete/test-plan.md` around lines 17 - 18, Align the test-plan entries E1, E2, F2, F4, and X2 with their implemented component-test level instead of L3, and update the L3 coverage summary to match. Preserve the declared Playwright coverage definition; only add Playwright tests if retaining those rows as L3.packages/client/src/components/session/SessionList.tsx (1)
1433-1439: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clear the deleted tag from
selectedTagson confirm.When the confirmed tag is currently an active filter (
selectedTagsholds it),onRemoveTagGloballystrips it from every session,allTagsrecomputes without it, butselectedTagsstill contains it.passesTagAxesthen hides every session, and if that tag was the only entry the wholetag-filter-bar(allTags.length > 0 || phasesInUse.length > 0) unmounts — taking the clear affordance with it and stranding the user until reload.🐛 Proposed fix: drop the tag from the selection when deleting
{pendingDeleteTag != null && onRemoveTagGlobally && ( <TagDeleteConfirmDialog tag={pendingDeleteTag} count={deleteTagCount} - onConfirm={() => onRemoveTagGlobally(pendingDeleteTag)} + onConfirm={() => { + onRemoveTagGlobally(pendingDeleteTag); + setSelectedTags((prev) => { + if (!prev.has(pendingDeleteTag)) return prev; + const next = new Set(prev); + next.delete(pendingDeleteTag); + return next; + }); + }} onClose={() => setPendingDeleteTag(null)} /> )}📝 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.{pendingDeleteTag != null && onRemoveTagGlobally && ( <TagDeleteConfirmDialog tag={pendingDeleteTag} count={deleteTagCount} onConfirm={() => { onRemoveTagGlobally(pendingDeleteTag); setSelectedTags((prev) => { if (!prev.has(pendingDeleteTag)) return prev; const next = new Set(prev); next.delete(pendingDeleteTag); return next; }); }} onClose={() => setPendingDeleteTag(null)} /> )}🤖 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 `@packages/client/src/components/session/SessionList.tsx` around lines 1433 - 1439, Update the TagDeleteConfirmDialog onConfirm handler to remove pendingDeleteTag from selectedTags before or alongside calling onRemoveTagGlobally, while preserving other selected tags and the existing global removal behavior. Use the selectedTags state update mechanism and ensure the confirmed tag is no longer passed to passesTagAxes after deletion.packages/client/src/components/tags/AGENTS.md (1)
10-10: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep the AGENTS.md file table path-alphabetical.
Move
TagDeleteConfirmDialog.tsxbeforeTagEditor.tsx; the new row is currently inserted afterTagFilterGroup.tsx.As per coding guidelines: maintain per-file purpose records in the nearest
AGENTS.mdand keep rows path-alphabetical.🤖 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 `@packages/client/src/components/tags/AGENTS.md` at line 10, Reorder the file table entry for TagDeleteConfirmDialog.tsx so it appears before TagEditor.tsx, maintaining strict path-alphabetical ordering while leaving the entry content unchanged.Source: Coding guidelines
packages/server/src/browser-handlers/session-meta-handler.ts (2)
89-90: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Validate the wire payload before normalizing it.
BrowserToServerMessageis only a TypeScript cast afterJSON.parse; a malformed client can sendtag: nullor another non-string value.normalizeTags([msg.tag])then calls.trim()and throws, so the request is dropped by the gateway error boundary instead of being handled cleanly. Add a runtimetypeof msg.tag === "string"check and return a structured error or no-op.As per coding guidelines: validate all agent inputs before processing and handle errors gracefully with meaningful error messages.
🤖 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 `@packages/server/src/browser-handlers/session-meta-handler.ts` around lines 89 - 90, Validate msg.tag with a runtime typeof check ensuring it is a string before calling normalizeTags in the session metadata handler; for null or other invalid payload values, return the handler’s established structured error or no-op path. Preserve normalization for valid string tags and prevent malformed input from reaching normalizeTags.Source: Coding guidelines
91-95: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make global tag removal an atomic read-modify-write.
The handler snapshots
session.tagsfromlistAll()and later writes a replacement array. A concurrentset_session_tagsor second global deletion can update the same session between those operations, allowing this stale write to overwrite newer tags. Use a per-session serialized mutation or compare-and-swap/retry mechanism.🤖 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 `@packages/server/src/browser-handlers/session-meta-handler.ts` around lines 91 - 95, Update the global tag-removal loop in the session metadata handler to perform each session’s tag removal as an atomic, serialized read-modify-write operation. Re-read tags within the per-session mutation or use the session manager’s compare-and-swap/retry mechanism, then remove only the target tag while preserving concurrent updates and making repeated global deletions safe.
- Localize tag-area count + active badge via t() (#1) - Deselect a globally-deleted tag from the active filter set on confirm so the list can't stay filtered to 0 with no chip to clear (#5) - Path-alphabetical order for the TagDeleteConfirmDialog AGENTS.md row (#6) - Guard non-string `tag` on remove_tag_globally before normalize (untrusted WS payload; normalizeTags calls .trim()) + L1 test (#8) Rejected (with reason): #2 archive dir is the project ship convention; #3 archived static mockups (real components already meet 24px+focus/active- selection); #4 test-plan is the frozen plan-stage artifact, level deviation documented in tasks.md; #7 last-write-wins fan-out is the design's documented accepted trade-off (D5/Risks, scenario X4 manual-only).
|
Thanks — addressed the review. Applied #1 (localize count/badge via Intentionally not applied:
|
…lapse-and-delete # Conflicts: # packages/client/src/components/session/SessionList.tsx # packages/server/src/pairing/browser-gateway.ts.AGENTS.md
1 participant
Implements OpenSpec change
sidebar-tag-collapse-and-delete.What
N tags · M phases, an active-selection indicator when a filter is active, and a fold-free clear affordance (D8). Phases stay a distinct read-only sub-group (D9).+N more/show lessinline expander on the user-tag group.remove_tag_globally { tag }browser→server verb. Server normalizes, fans out overlistAll(), strips from every carrier via the existing normalize→update→broadcast path (onesession_updatedper changed session). Phase chips stay read-only.Tests
handleRemoveTagGlobally— E3–E6, X3 (fan-out, no-op, blank, normalize, reconnect replay).QA/manual tasks (9.1 derived-union reappear, X4 concurrent-write race) deferred to post-merge verification.