fix(expose): keep navigation responsive - #367
Conversation
|
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:
📝 WalkthroughWalkthroughExposé now delays refresh and resize checks during input bursts, incrementally redraws moved selections, protects reloads from stale commits, preserves user selection across reloads, and expands navigation, focus, resize, and redraw test coverage. ChangesExposé interaction updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant runTmuxExpose
participant ExposeItemsAPI
participant ExposeFocusAPI
participant TmuxOutput
User->>runTmuxExpose: send navigation or numeric key
runTmuxExpose->>TmuxOutput: redraw affected selection tiles
runTmuxExpose->>ExposeItemsAPI: reload expose items
ExposeItemsAPI-->>runTmuxExpose: return scoped tiles
runTmuxExpose->>runTmuxExpose: reject stale reload result
runTmuxExpose->>ExposeFocusAPI: focus selected tile
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
🧹 Nitpick comments (1)
src/tmux/expose.ts (1)
469-517: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the shared tile-draw logic used by
render()andrenderTileIndexes().Both functions independently compute per-tile geometry (
r,c,top,left) and calldrawTile(...)with the same argument shape (lines 490-508 here mirror lines 564-581 inrender()). Any future change to tile geometry ordrawTile's call signature now needs to be kept in sync in two places.♻️ Suggested extraction
+ const renderTileAt = (tileIndex: number, layout: ReturnType<typeof computeLayout>, geo: ReturnType<typeof panelGeometry>): string => { + const r = Math.floor(tileIndex / layout.tileCols); + const c = tileIndex % layout.tileCols; + const top = geo.top + layout.gridTopRow + r * layout.tileHeight; + const left = geo.left + 1 + c * (layout.tileWidth + GAP); + const item = items[tileIndex]!; + const preview = tilePreview(captures.get(item.target.windowId) ?? "", layout.bodyLines); + return drawTile( + item, + preview, + tileIndex + 1, + tileIndex === index, + top, + left, + layout.tileWidth, + layout, + tileSublabel(item), + options, + false, + ); + };Then both
renderTileIndexesandrendercan callout += renderTileAt(tileIndex, layout, geo);instead of duplicating the block.🤖 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 `@src/tmux/expose.ts` around lines 469 - 517, Extract the duplicated per-tile geometry and drawTile invocation from renderTileIndexes and render into a shared renderTileAt helper that accepts tileIndex, layout, and geo and returns the rendered tile string. Replace both existing tile-drawing blocks with this helper while preserving their current filtering, selection, preview, and output behavior.
🤖 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.
Nitpick comments:
In `@src/tmux/expose.ts`:
- Around line 469-517: Extract the duplicated per-tile geometry and drawTile
invocation from renderTileIndexes and render into a shared renderTileAt helper
that accepts tileIndex, layout, and geo and returns the rendered tile string.
Replace both existing tile-drawing blocks with this helper while preserving
their current filtering, selection, preview, and output behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 15b91227-46df-4d38-9770-9dc7394206bb
📒 Files selected for processing (2)
src/tmux/expose.test.tssrc/tmux/expose.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tmux/expose.ts (1)
787-795: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winTimestamp only actionable input for refresh throttling.
lastInputAtis updated beforefocusin,focusout, andmouseevents are filtered out. Repeated ignored terminal events can therefore keepscheduleRefresh()in its quiet-period branch without any navigation occurring. Move the assignment after filtering and gate it onevents.length > 0.🤖 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 `@src/tmux/expose.ts` around lines 787 - 795, Update onData so lastInputAt is assigned only after parseKeys events are filtered and expanded, and only when events.length > 0. Remove the current unconditional assignment before filtering while preserving the existing opening guard and event-processing flow.
🤖 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 `@src/tmux/expose.ts`:
- Around line 591-596: Update the reload flow around loadExposeScopeItems to
prevent stale overlapping requests from replacing newer state: track a reload
generation/request token (or serialize reloads), and before assigning items,
index, view, or selection, verify the request is still current. Preserve the
selection snapshot only for the request that successfully commits its
corresponding item list.
---
Outside diff comments:
In `@src/tmux/expose.ts`:
- Around line 787-795: Update onData so lastInputAt is assigned only after
parseKeys events are filtered and expanded, and only when events.length > 0.
Remove the current unconditional assignment before filtering while preserving
the existing opening guard and event-processing flow.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 85bdb177-6544-41d0-a825-11cf907324cd
📒 Files selected for processing (2)
src/tmux/expose.test.tssrc/tmux/expose.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tmux/expose.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tmux/expose.ts (1)
792-834: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRestart resize throttling for every input burst.
Line 792 does not reset
lastResizeCheckAt. After a prior burst, a new keypress can immediately satisfy Line 820 and call synchronousqueryClientSize()while input is active—the behavior this throttling intends to avoid.Proposed fix
- lastInputAt = Date.now(); + lastInputAt = Date.now(); + lastResizeCheckAt = lastInputAt;🤖 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 `@src/tmux/expose.ts` around lines 792 - 834, Reset lastResizeCheckAt when processing input in the key-handling loop so each new input burst starts the resize-check throttle window anew. Update the event-processing logic around parseKeys and handleKey, preserving the existing refresh scheduling and resize behavior for inactive input.
🤖 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.
Outside diff comments:
In `@src/tmux/expose.ts`:
- Around line 792-834: Reset lastResizeCheckAt when processing input in the
key-handling loop so each new input burst starts the resize-check throttle
window anew. Update the event-processing logic around parseKeys and handleKey,
preserving the existing refresh scheduling and resize behavior for inactive
input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 162b6b7b-55ec-4253-9e02-00b4b8fb9af6
📒 Files selected for processing (2)
src/tmux/expose.test.tssrc/tmux/expose.ts
1 participant
Summary
Verification
Summary by CodeRabbit