fix: animate the workspace row-click expand/collapse - #413
Merged
Conversation
A workspace row carries two hit targets for one toggle. The disclosure triangle goes through AppKit's native path and animates; the row-click path called expandItem/collapseItem directly, which do not animate, so the same action on the same row rendered two different ways. Route that toggle through outline.animator(). Checked against AppKit that the proxy animates only the row insert/remove while flipping expansion and firing didExpand/didCollapse synchronously, so the suppressExpansionPersist protocol and the persist write-back are unaffected; testWorkspaceCollapsePersistsAcrossRelaunch and testActiveWorkspaceCollapsePersistsDespiteReveal both drive the collapse from a row click and pin that. Not gated on Reduce Motion, unlike the app's own pulses: both hit targets reach the same AppKit animation, so a gate here would re-split them. Scope is the row-click path. The other call sites stay unanimated, being either bulk (expand/collapse all, rebuild re-apply) or programmatic (selection reveal, drag spring-load). The delay half of the report is a separate behavior decision and is untouched. Related to #407
…clicks Surfaced while fixing the animation half of #407. The single pendingRowToggle slot and the guard ordering in handleSingleClick mean a second workspace-row click cancels the first with no feedback, and a session-row click returns before the cancel so a pending workspace toggle still fires under it. Deferred because settling them is the same behavior decision as the report's delay half, not a rendering fix.
The doc comment on toggleExpansion restated three facts the same change wrote into .claude/rules/sidebar.md, which already owns the click-routing contract. Keep only what is code-local - that the animator proxy fires didExpand/didCollapse synchronously, so the persist write-back and the suppressExpansionPersist bracketing around the other call sites still hold - and cross-reference the rest. Also correct the backlog item: workspace rename sits in the File menu's Workspace section, not a Workspace menu (there is none; the only CommandMenu is Navigate), and shouldSelectItem is at WorkspaceSidebar+RowRendering.swift:15-18.
… delay Keeps the next reader from re-proposing any of them: dropping double-click rename, the optimistic toggle with an inverted undo, and press feedback during the deferral were each weighed and judged either a worse trade or disproportionate to the defect.
There was a problem hiding this comment.
Pull request overview
This PR aligns workspace expand/collapse behavior in the AppKit sidebar so that clicking anywhere on a workspace row animates the same way as clicking the native disclosure triangle, addressing the UX inconsistency described in #407.
Changes:
- Route workspace row-click expand/collapse through
NSOutlineView.animator()to match native disclosure animation. - Document the decision and its scope in the sidebar rules.
- Add a backlog note describing known issues with the current single pending-toggle deferral behavior (out of scope for this fix).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
agterm/Views/WorkspaceSidebar+ContextMenu.swift |
Uses the outline view animator proxy for row-click expand/collapse so the transition animates like the disclosure triangle. |
.claude/rules/sidebar.md |
Records the animation decision (including why it’s not Reduce Motion–gated) and clarifies which other expansion paths remain unanimated. |
docs/backlog/workspace-row-toggle-deferral-drops-clicks.md |
Captures deferred-toggle edge cases and rejected approaches for future follow-up work. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+50
to
+53
| /// Animated to match the disclosure triangle; `.claude/rules/sidebar.md` owns why, why it is not gated on | ||
| /// Reduce Motion, and which sites stay unanimated. The proxy flips expansion and fires | ||
| /// didExpand/didCollapse synchronously, so the persist write-back is unaffected and the | ||
| /// `suppressExpansionPersist` bracketing the other sites wrap their calls in still holds. |
"the suppressExpansionPersist bracketing the other sites wrap their calls in" stranded a preposition off a gerund and read as noise. Same three facts, one fewer line.
Deploying agterm with
|
| Latest commit: |
58e0aed
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4cb473ee.agterm.pages.dev |
| Branch Preview URL: | https://fix-sidebar-row-expand-anima.agterm.pages.dev |
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.
a workspace row has two hit targets for one toggle and they rendered differently. The disclosure triangle goes through AppKit's native path and animates; clicking anywhere else on the row called
expandItem/collapseItemdirectly, which do not animate, so the sessions just appeared and disappeared.routing the row-click toggle through
outline.animator()makes both paths render the same.the animator proxy is not documented for expand/collapse, so I checked it before committing:
outlineViewItemDidExpand/DidCollapsestill fire synchronously andisItemExpanded/numberOfRowsupdate immediately, which is what keeps thesuppressExpansionPersistbracketing at the other call sites intact.testWorkspaceCollapsePersistsAcrossRelaunchandtestActiveWorkspaceCollapsePersistsDespiteRevealboth collapse from a row click and assert the state reaches disk, so that leg is pinned.no Reduce Motion gate here, unlike the app's own pulses. Both hit targets reach the same AppKit animation, so gating this one would split them again.
scope is the row-click path only. The other expand/collapse sites stay unanimated, being either bulk (Expand/Collapse Workspaces, rebuild re-apply) or programmatic (selection reveal, drag spring-load).
what this does not fix: the row click still waits out
NSEvent.doubleClickIntervalbefore toggling, so it reads as a pause and then motion. Getting rid of that means dropping double-click rename on workspace rows, or toggling optimistically and inverting on a double-click, or adding press feedback during the wait. None of the three is worth its cost for a defect this size, so the delay stays.it is written up in
docs/backlog/workspace-row-toggle-deferral-drops-clicks.mdwith the rejected approaches, along with two real defects in the same deferral: a second workspace-row click inside the interval cancels the first one's pending toggle, and a session-row click returns before the cancel so a pending workspace toggle still fires under it.Fix #407