feat(expose): instrument first paint timing - #370
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
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:
📝 WalkthroughWalkthrough
ChangesExposé timing instrumentation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ExposeRuntime
participant Output
participant RuntimeManager
participant TimingCallback
ExposeRuntime->>TimingCallback: emit lifecycle and render events
ExposeRuntime->>Output: write warm preview
ExposeRuntime->>TimingCallback: emit first-live-capture-start
ExposeRuntime->>RuntimeManager: captureTarget
RuntimeManager-->>ExposeRuntime: return capture result
ExposeRuntime->>TimingCallback: emit first-live-capture-end
ExposeRuntime->>TimingCallback: emit reload and focus outcomes
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.
Actionable comments posted: 1
🤖 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 375-386: Update the markTiming function so exceptions from
options.onTiming?.(event) are caught and treated as non-fatal, while preserving
the existing timing event creation and debug logging. Ensure callers such as
exit continue to perform timer cleanup and terminal restoration even when the
instrumentation callback throws.
🪄 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: 25137c44-3140-46e6-a07e-965c9587b281
📒 Files selected for processing (2)
src/tmux/expose.test.tssrc/tmux/expose.ts
|
Sub-agent finding fixed in 90565b0: empty item lists now skip live capture timing entirely, so a later populated scope can still record the real first capture. |
|
Sub-agent finding fixed in 90565b0: Expose timing logs now use the existing |
|
Sub-agent finding fixed in 90565b0: item load timing now has terminal stale and error events, keeping every load-start pairable even when a reload is superseded or fails. |
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)
377-389: 🩺 Stability & Availability | 🟠 MajorKeep timing sinks non-fatal.
options.onTiming?.(event)is still outside atry/catch. If it throws duringterminal-ready, Exposé can leave the terminal in raw/alternate-screen mode; duringexit, timer cleanup and terminal restoration are skipped. This is the same unresolved issue from the previous review.🤖 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 377 - 389, The markTiming function must keep timing sinks non-fatal: wrap options.onTiming?.(event) in a try/catch so exceptions from the callback are swallowed or safely logged, allowing subsequent terminal-ready and exit cleanup to continue. Keep timing event construction and debug logging behavior unchanged.
🤖 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 377-389: The markTiming function must keep timing sinks non-fatal:
wrap options.onTiming?.(event) in a try/catch so exceptions from the callback
are swallowed or safely logged, allowing subsequent terminal-ready and exit
cleanup to continue. Keep timing event construction and debug logging behavior
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1550c470-ec23-4226-862d-3bb48f68460e
📒 Files selected for processing (2)
src/tmux/expose.test.tssrc/tmux/expose.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Sub-agent finding fixed in 053c31e: refresh reloads now skip inline live capture until after the first non-empty item render, preserving preview-first first paint for empty-to-populated scopes. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
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)
808-824: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTerminate pending focus timing on early exit.
If Exposé finishes while
focusExposeItemis pending, both settlement handlers return atfinishedbefore emittingfocus-end. This leaves a reachablefocus-startwithout a terminal event; emit a singlefocus-endor explicit cancellation event when the operation is abandoned.🤖 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 808 - 824, Update the pending focus flow around focusExposeItem so an Exposé early exit while the promise is pending emits exactly one terminal focus event, such as focus-end or an explicit cancellation event. Ensure the settlement handlers and the early-finish path coordinate through existing state to prevent duplicate emission while preserving the current success and retry 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.
Outside diff comments:
In `@src/tmux/expose.ts`:
- Around line 808-824: Update the pending focus flow around focusExposeItem so
an Exposé early exit while the promise is pending emits exactly one terminal
focus event, such as focus-end or an explicit cancellation event. Ensure the
settlement handlers and the early-finish path coordinate through existing state
to prevent duplicate emission while preserving the current success and retry
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d1ed4194-76ac-4863-8d3d-af370cd52969
📒 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
|
CodeRabbit outside-diff finding fixed in 4ebfcf9: pending focus timing now closes through the shared finish path, with a guard to emit exactly one focus-end across success, retry, and early-exit paths. |
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)
694-703: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRender newly populated items before live capture.
When a reload changes an empty scope to non-empty, this guard skips capture inside
reload(), but the caller immediately invokesrefreshCaptures()before the first item render. Live tmux output can therefore replace seeded previews beforefirst-items-render, violating the preview-first contract.Render once after the committed reload when
firstItemsRenderMarkedis still false, then perform the live capture. Add a regression test for empty → populated refresh ordering.🤖 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 694 - 703, Update the committed reload flow in reload() so that when items become populated and firstItemsRenderMarked is still false, it performs the initial item render before calling refreshCaptures(). Preserve the existing capture behavior for already-rendered items, and add a regression test covering empty-to-populated reload ordering to ensure first-items-render precedes live capture.
🤖 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 694-703: Update the committed reload flow in reload() so that when
items become populated and firstItemsRenderMarked is still false, it performs
the initial item render before calling refreshCaptures(). Preserve the existing
capture behavior for already-rendered items, and add a regression test covering
empty-to-populated reload ordering to ensure first-items-render precedes live
capture.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b47bb500-c2e5-4cc3-ba59-83b77a7ec976
📒 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
|
Sub-agent finding fixed in 9f39409: fatal signal cleanup now shares the focus timing closeout and signal-detach path, so pending focus spans emit a matching focus-end before exit. |
|
Sub-agent finding fixed in 9f39409: reload() now observes the finished state before starting and immediately after awaited item loads, preventing item-load terminal events or item mutations after Exposé has emitted exit. |
|
Sub-agent finding fixed in cfde1e6: scheduleRefresh now exits early when Exposé is finished and rechecks after awaited reloads before handling stale results, so late refresh reloads cannot re-arm timers or write/capture after exit. |
|
CodeRabbit preview-first summary is already addressed in current head: 053c31e added the empty-to-populated regression, reload() skips inline capture until firstItemsRenderMarked, loadInitialItems/zoomOut render before calling refreshCaptures(), and the periodic refresh path short-circuits capture on reloadedItems before rendering. |
1 participant
Summary
Verification
Summary by CodeRabbit
New Features
Tests