Skip to content

Conversation

@ammar-agent
Copy link
Collaborator

@ammar-agent ammar-agent commented Oct 19, 2025

Remembers users' manual expand/collapse choices in Review tab, centralizes keybinds, and fixes Space key to operate on selected hunk.

Problems

  1. When users manually expand or collapse hunks, their choices are lost on remount (e.g., switching workspaces and back)
  2. Keybind strings hardcoded in components instead of centralized
  3. Space key operated on focused (blue border) hunk instead of selected (yellow border) hunk during j/k navigation

Solutions

Persistent Expand/Collapse State

  • Store per-workspace hunk expand state in localStorage via usePersistedState
  • Key: reviewExpandState:{workspaceId} → maps hunkId to isExpanded boolean
  • Manual choices override automatic collapse logic (read status, size threshold)
  • Storage copies on workspace fork, clears on workspace deletion
  • Refactored with PERSISTENT_WORKSPACE_KEY_FUNCTIONS array for DRY

Centralized Keybinds

  • Added TOGGLE_HUNK_COLLAPSE to KEYBINDS registry in src/utils/ui/keybinds.ts
  • All keybind displays use formatKeybind() for dynamic OS-appropriate formatting
  • Added special handling for space key to display as "Space" (not invisible whitespace)

Fixed Space Key Behavior

  • Removed confusing blue focus ring from HunkContainer
  • Moved Space key handler from HunkViewer to ReviewPanel level
  • Space now operates on selected (yellow border) hunk, not focused element
  • All keyboard shortcuts (j/k/m/Space) consistently operate on same selection state

Behavior

Priority order for expand state:

  1. Manual state (user clicked expand/collapse or pressed Space) → persisted
  2. Read status (marked as read) → collapsed
  3. Size threshold (>200 lines) → collapsed
  4. Default → expanded

Keybind Display:

  • Expand indicator: "Click to expand (N lines) or press Space"
  • Collapse indicator: "Click here or press Space to collapse"
  • Mark as read: "Mark as read (m)"
  • All keybinds show OS-appropriate format (e.g., ⌘ on Mac, Ctrl+ on Windows/Linux)

Keyboard Navigation:

  • j/k - Navigate between hunks (updates yellow border selection)
  • m - Toggle read state on selected hunk
  • Space - Toggle expand/collapse on selected hunk
  • All operations work on the yellow-bordered (selected) hunk

Generated with cmux

- Added workspaceId prop to HunkViewer
- Use usePersistedState to store per-workspace hunk expand state
- Manual expand/collapse choices persisted in localStorage
- Priority: manual state > read status > size (>200 lines)
- State survives remounts/reloads but clears when workspace changes

Result: Users' manual expand/collapse preferences persist across sessions,
improving workflow when reviewing large diffs incrementally.
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

- Added getReviewExpandStateKey() helper to constants/storage.ts
- Follows established colon pattern: reviewExpandState:{workspaceId}
- DRY: Refactored to shared PERSISTENT_WORKSPACE_KEY_FUNCTIONS list
- Added deleteWorkspaceStorage() to clean up on workspace removal
- Frontend calls deleteWorkspaceStorage() in useWorkspaceManagement
- Fork copies expand state via existing copyWorkspaceStorage() mechanism

Result: Hunk expand state properly integrated into workspace lifecycle.
Copied on fork, deleted on removal, follows SoC (frontend handles localStorage).
@ammar-agent
Copy link
Collaborator Author

✅ Addressed review feedback:

  1. Centralized storage pattern: Added getReviewExpandStateKey() helper to constants/storage.ts following the established colon pattern (reviewExpandState:{workspaceId})

  2. DRY refactor: Extracted shared PERSISTENT_WORKSPACE_KEY_FUNCTIONS list used by both copyWorkspaceStorage() and deleteWorkspaceStorage()

  3. Frontend cleanup: Added deleteWorkspaceStorage() call in useWorkspaceManagement hook after successful workspace removal

  4. SoC compliance: localStorage operations stay in frontend, not in main process

Behavior verified:

  • ✅ Fork copies expand state (via existing copyWorkspaceStorage in workspaceFork.ts)
  • ✅ Delete removes expand state (via deleteWorkspaceStorage in useWorkspaceManagement)
  • ✅ All 661 tests pass

- Space now toggles expand/collapse for all hunks (not just selection)
- Enter still selects the hunk
- Show collapse button for all manually-expanded hunks (not just >200 lines)
- Updated text: "Click here or press [Space] to collapse"
- Pure renames: Space falls back to selection (no content to collapse)

Result: Consistent UX - users can always collapse what they manually expand,
and spacebar provides quick keyboard toggle for any hunk.
@ammar-agent
Copy link
Collaborator Author

UX Improvements Added

Spacebar Toggle

Added keyboard shortcut for quick expand/collapse:

  • Space - Toggle expand/collapse (works on any hunk, any time)
  • Enter - Select hunk (unchanged)
  • Pure renames - Space selects (no content to collapse)

Collapse Button Logic

Changed from size-based to intention-based:

Before: Only showed for hunks >200 lines

  • Small hunks couldn't be collapsed after manual expand
  • Inconsistent with "Click to expand" showing for all

After: Shows for all manually-expanded hunks

  • If user clicked "Click to expand" → they get "Click here or press [Space] to collapse"
  • Consistent: any expand action can be undone
  • Works with persistence: users can "undo" their manual choices

Behavior Matrix

Hunk Type Initial State Collapse Button? Space Action
Small (<200) auto-expanded Expanded No Collapse
Small manually expanded Expanded Yes Collapse
Large (>200) auto-collapsed Collapsed N/A Expand
Large manually expanded Expanded Yes Collapse
Pure rename N/A No Select hunk

Result: Intuitive, keyboard-friendly, and respects user intent.

- Added TOGGLE_HUNK_COLLAPSE keybind to centralized keybinds registry
- Updated HunkViewer to use formatKeybind() for dynamic keybind display
- Updated Mark as read tooltip to use formatKeybind() for consistency
- Removed blue focus ring from HunkContainer (confusing dual-state)
- Moved Space key handler from HunkViewer to ReviewPanel level
- Space now operates on selected (yellow border) hunk, not focused hunk
- Added onRegisterToggleExpand prop pattern for parent control
- All keyboard navigation (j/k/m/Space) now consistently operates on selection

Fixes UX confusion where Space would collapse the blue-bordered (focused)
hunk instead of the yellow-bordered (selected) hunk during j/k navigation.
Display space key as 'Space' instead of invisible whitespace character.
Without this, keybind hints like 'press Space' would show blank text.
@ammar-agent ammar-agent changed the title 🤖 Preserve hunk expand/collapse state across remounts 🤖 Preserve hunk expand/collapse state and centralize keybinds Oct 19, 2025
Each HunkViewer instance now subscribes to storage changes via the
listener option in usePersistedState. When one hunk's expand state
changes, all other instances immediately see the update and won't
overwrite it with stale data. This prevents the race condition where
toggling multiple hunks would lose all but the most recent choice.

Fixes P1 Codex review comment: PRRT_kwDOPxxmWM5ehWa0
@ammario ammario merged commit efd2f14 into main Oct 19, 2025
8 checks passed
@ammario ammario deleted the preserve-hunk-expand-state branch October 19, 2025 17:47
ammar-agent added a commit that referenced this pull request Oct 19, 2025
Use existing usePersistedState pattern to remember directory expand/collapse
preferences across workspace switches. State is stored in localStorage with
workspace-scoped key 'fileTreeExpandState:{workspaceId}'.

Implementation follows the same pattern as hunk expand state (PR #332):
- Store expand state as Record<string, boolean> keyed by node path
- Default to expanded for first 2 levels if no manual state exists
- Use { listener: true } for cross-component synchronization
- State is copied on workspace fork and deleted on workspace removal

Benefits:
- Users don't need to re-expand folders after switching workspaces
- Reduces cognitive load when navigating between workspaces
- Consistent UX with hunk expand/collapse behavior
ammar-agent added a commit that referenced this pull request Oct 19, 2025
Use existing usePersistedState pattern to remember directory expand/collapse
preferences across workspace switches. State is stored in localStorage with
workspace-scoped key 'fileTreeExpandState:{workspaceId}'.

Implementation follows the same pattern as hunk expand state (PR #332):
- Store expand state as Record<string, boolean> keyed by node path
- Default to expanded for first 2 levels if no manual state exists
- Use { listener: true } for cross-component synchronization
- State is copied on workspace fork and deleted on workspace removal

Benefits:
- Users don't need to re-expand folders after switching workspaces
- Reduces cognitive load when navigating between workspaces
- Consistent UX with hunk expand/collapse behavior
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants