Route the checklist's first item through the popover in popover style#7797
Route the checklist's first item through the popover in popover style#7797azooz2003-bit wants to merge 7 commits into
Conversation
usesPopoverPresentation && totalCount > 0 forced inline entry for a workspace's very first checklist item regardless of the popover setting, since an empty checklist has no summary line to anchor a popover to. Move the popover anchor from the summary line to the section container (stable across the 0->1 item transition), route the ghost "Add item" button through the same popover-toggle path as the summary line, and fix the "Add Checklist Item..." notification handler in ContentView.swift (it special-cased non-empty checklists, so the context-menu/palette path still fell back to inline for a fresh workspace even after the view-level fix). Inline style is unaffected.
|
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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR updates workspace checklist popover behavior for first-item creation. The main changes are:
Confidence Score: 4/5This is close, but the popover dismissal state should be fixed before merging.
Sources/ContentView.swift and the checklist popover close callbacks. Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /// below, which is present whether or not a summary line exists yet. | ||
| private var presentsPopover: Bool { | ||
| usesPopoverPresentation && totalCount > 0 | ||
| usesPopoverPresentation |
There was a problem hiding this comment.
When an empty checklist is opened through the new popover path and then dismissed without adding an item, the inline cancel path no longer runs and the popover close path does not consume addFieldActivationToken. That leaves checklistAddFieldActivationTokens[workspaceId] > 0, so an otherwise-empty checklist section can stay rendered indefinitely just because of stale add state.
Rule Used: Flag SwiftUI changes that can cause stale state, b... (source)
| if presentsPopover { | ||
| onPopoverPresentedChange(!isPopoverPresented) | ||
| } else { |
There was a problem hiding this comment.
For an empty checklist opened by the add-item notification, the ghost “Add item” row remains visible while the popover is already open. Clicking it sends false here and closes the popover instead of keeping the add field available, so the add affordance behaves like a dismiss action.
| if presentsPopover { | |
| onPopoverPresentedChange(!isPopoverPresented) | |
| } else { | |
| if presentsPopover { | |
| onPopoverPresentedChange(true) | |
| } else { |
Rule Used: Flag SwiftUI changes that can cause stale state, b... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
taskStatus is nil whenever a workspace's status is hidden (statusHidden), which restore-workspace-todos-default-none made the default for all new workspaces. The checklist section's render gate required taskStatus != nil, so "Add Checklist Item..." silently did nothing for any new workspace. Drop that condition; the checklist section only needs its own content/token gate.
| Button { | ||
| isAddingItem = true | ||
| if presentsPopover { | ||
| onPopoverPresentedChange(!isPopoverPresented) | ||
| } else { | ||
| isAddingItem = true | ||
| } |
There was a problem hiding this comment.
expandedList keeps this ghost add row visible while the popover is already open. Clicking “Add item” then sends false through this toggle and closes the popover instead of keeping the add field available. The row should open the popover when closed and avoid dismissing it when it is already presented.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
The only way to delete a checklist item was a hidden right-click
"Remove" context menu entry. Add a trailing xmark.circle.fill button
that appears on row hover (reserved layout space, opacity/hit-test
toggle only, so row height never jumps) in both row implementations:
the inline sidebar row and the NSPopover-hosted row. Both call the
existing actions.removeItem(_:) closure, the same one the context
menu already used; the context menu entry is unchanged.
New localized tooltip key sidebar.checklist.removeItemTooltip
("Remove item" / "項目を削除").
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… add keyboard delete - Highlighting a checklist item (click, Up/Down) and typing a new item in the add field are now mutually exclusive: a non-empty draft clears any highlight, and highlight-setting paths no-op while a draft exists. Makes Return's outcome (toggle item vs. commit new item) unambiguous. - Popover-style checklists no longer show a persistent ghost "Add item" row inline when a workspace has zero items. "Add Checklist Item…" now opens the popover directly with nothing shown inline until an item is actually added, at which point the existing summary line is the small status preview. - Item rows now center-align the checkbox and text (was firstTextBaseline, which misaligned the checkbox glyph against the text's baseline). - Backspace with an empty draft and a highlighted item now removes it, a keyboard-driven delete alongside the existing hover "x" and context menu "Remove".
| // checklist) — independent of whether a status glyph is shown, | ||
| // since workspaces now default to hidden status and must still | ||
| // be able to add checklist items from that state. | ||
| if !workspaceSnapshot.checklistItems.isEmpty || checklistAddFieldActivationToken > 0 { |
There was a problem hiding this comment.
checklistAddFieldActivationTokens. The popover close path clears checklistPopoverWorkspaceId, but it does not consume the token. Since this gate keeps rendering the checklist section whenever checklistAddFieldActivationToken > 0, an otherwise-empty workspace can keep an empty checklist section alive after the user clicks outside the transient popover or cancels without committing. Clear the token when the popover add flow closes without a commit, or stop using the pending activation token as section visibility state after dismissal.
An empty VStack collapses to a degenerate frame, which broke the NSPopover anchor view's bounds/window attachment when a workspace had zero checklist items — "Add Checklist Item…" silently failed to open the popover. Keep a minimal invisible placeholder so the anchor always has real geometry. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| if WorkspaceTodoFeature.checklistStyle == .popover { | ||
| statusPopoverWorkspaceId = nil | ||
| checklistPopoverWorkspaceId = workspaceId |
There was a problem hiding this comment.
checklistAddFieldActivationTokens set. If the user dismisses the transient popover without adding an item, the close path clears checklistPopoverWorkspaceId but does not consume the activation token. Since the sidebar section still renders when checklistAddFieldActivationToken > 0, an empty workspace can keep stale add state after the popover closes. Clear the token when the popover add flow is dismissed without a commit, or make section visibility stop depending on a token after that flow has closed.
…line alignment The prior zero-item placeholder fix didn't retry reliably: it depended on an unrelated later re-render landing after the anchor NSView actually attached to its window, so "Add Checklist Item..." on a fresh workspace usually opened nothing until some other UI event fired. Replace it with an AnchorView subclass that retries present() from viewDidMoveToWindow(), so the popover opens deterministically as soon as AppKit finishes attaching it. The popover host's NSViewRepresentable anchor was also stacked as a .background() spanning the whole checklist section, which suppressed .onHover for the item rows underneath it (the hover-reveal delete "x" rarely appeared). Shrink the anchor to a fixed 1x1pt .overlay corner so it never intercepts hover/hit-testing, and drop the now-unnecessary Color.clear placeholder that caused the "bottom padding" artifact. Both the inline expansion and the popover now show all items in a ScrollView capped at 6 rows instead of hard-clamping with a "... N more" row, and item rows use .firstTextBaseline alignment with an alignmentGuide offset (derived from font ascender/descender) so the checkbox and delete button center on the first line of wrapped multi-line item text rather than the baseline or the whole block. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit aee3354. Configure here.
| .opacity(isHovered ? 1 : 0) | ||
| .allowsHitTesting(isHovered) | ||
| .accessibilityHidden(!isHovered) | ||
| .accessibilityIdentifier("SidebarChecklistPopoverRemoveItemButton") |
There was a problem hiding this comment.
Stale highlight after item removal
Low Severity
Keyboard highlight state is not cleared when an item is removed via the new hover delete button or the context-menu Remove action. Only handleAddFieldDelete resets highlightedItemId. After deleting the highlighted row, arrow-key navigation uses fallback indices and can jump to the wrong item; toggle shortcuts and Return-on-empty may no-op until the user clicks a row again.
Reviewed by Cursor Bugbot for commit aee3354. Configure here.
WorkspaceTodoPaneItemRow was missing the same multi-line-wrap fix already applied to the sidebar's inline list and popover: long item text overflowed unwrapped past the pane's edge instead of wrapping. Adds .fixedSize(horizontal: false, vertical: true) + .multilineTextAlignment(.leading) to the item Text, and centers the checkbox on the text's first line via .alignmentGuide(.firstTextBaseline).
1 participant
Stacks on #7790
This PR is stacked on #7790 (
restore-workspace-todos-default-none) and should be reviewed/merged after it. Do not merge this PR on its own.Summary
SidebarWorkspaceChecklistView.swift'spresentsPopoverwasusesPopoverPresentation && totalCount > 0, so a workspace with zero checklist items always fell into inline text-field entry regardless ofsidebar.beta.workspaceTodos.checklistStyle(default.popover). A workspace's very first checklist item never got the popover treatment.presentsPopovernow equalsusesPopoverPresentationdirectly (nototalCountgate).ChecklistSummaryPopoverModifier(the NSPopover anchor) fromsummaryLineto the section's outer container, so the same backingNSView/Coordinatoranchors the popover across the 0->1 item transition instead of re-anchoring to a freshly created view (which would close and immediately reopen the popover).presentsPopoveris true.ContentView.swift's.onReceivehandler for.workspaceChecklistAddItemRequested: it special-cased!workspace.todoState.checklist.isEmptybefore routing to the popover, so the "Add Checklist Item..." context-menu/palette action still fell back to inline for a workspace's first item even after the view-level fix. Removed that gate; popover style now always routes the add request into the popover..inlinestyle is untouched:presentsPopoverisfalsethere regardless of item count, same as before.No changes to
WorkspaceTodoChecklistStyle, its default, the settings UI, orWorkspaceTodoState.statusHidden/default-status logic (all handled by #7790).Test plan
No new pure-logic seam was created worth a unit test:
presentsPopoverreduces to exactlyusesPopoverPresentation, an identity, and the rest of the fix is SwiftUI view-wiring (anchor attachment point, button action branch, notification-handler condition) that isn't expressible as a standalone testable function. Verified manually instead, tagchkpop:.popoverstyle (default): triggering "Add Checklist Item..." (context-menu/palette path, exercised via thedebug.workspace_todo.checklist_add_fielddebug RPC, which shares the sameWorkspaceTodoActions.requestChecklistAddFieldpath as the context menu and the ghost row's own tap) opens the anchoredNSPopoverimmediately, with the header showing the workspace title and0/0, and the add field focused — no intermediate inlineTextFieldstate at any point. Screenshot evidence captured.1. [ ] Buy milk,0/1 completed), popover stays anchored (no visible close/reopen), summary line updates to0/1 · Buy milk.2. [ ] Walk the dog,0/2 completed) — pre-existing popover-for-non-empty-checklist behavior unaffected.sidebarWorkspaceTodosChecklistStyleoverridden toinline: triggering the same add-item path shows the inline ghost-rowTextField(not a popover) for the very first item — confirms no regression to the non-default path../scripts/reload.sh --tag chkpopsucceeded (local reload;reload-cloud.shknown-broken in this hq checkout).cmux DEV chkpopprocesses remain.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches NSPopover lifecycle, focus, and sidebar row layout/hover in a user-facing area; changes are localized to workspace todo UI with no data or auth impact.
Overview
Popover style now covers empty checklists end-to-end.
presentsPopoverno longer requires items; add-item notifications and context-menu paths always open the anchoredNSPopoverin.popovermode. The popover anchor moves from the summary line to the section container (1×1pt overlay) so it stays stable across 0→1 items, andSidebarWorkspaceTodoPopoverHostretriespresent()when the anchor isn’t in a window yet instead of clearingisPresented.Sidebar checklist visibility and layout. The checklist block can show when there’s content or a pending add even if workspace status is hidden. Inline and popover lists drop the “… N more” clamp in favor of a 6-row scroll viewport; item text wraps with checkbox/delete aligned to the first line. Rows get hover-reveal delete (plus tooltip
sidebar.checklist.removeItemTooltip); the popover add field gains Backspace-to-delete highlighted items and stricter highlight vs. draft keyboard rules.Todo pane gets the same multiline wrapping / first-line checkbox alignment for long items.
Reviewed by Cursor Bugbot for commit 7996c98. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Empty checklists in
.popoverstyle now open the checklist popover for the first item reliably, with a stable anchor, smooth focus, and clear keyboard behavior..inlineis unchanged; the checklist section renders when status is hidden, and long item text now wraps with aligned controls in the Todo Pane.Bug Fixes
.popover(nototalCountgate)..overlay; add anAnchorViewthat retriespresent()on window attach for zero-item workspaces; fixes open reliability and hover interactions..workspaceChecklistAddItemRequestedto the popover in.popover..popover; open the popover instead.New Features
sidebar.checklist.removeItemTooltip(en, ja).Written for commit 7996c98. Summary will update on new commits.