feat(tmux): auto-save on detach/close and fix restore timing - #1287
Conversation
Entire-Checkpoint: 02031fb40538
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughTwo configuration and timing adjustments added to enhance tmux session state management: a one-second delay introduced in a fish function after tmux restoration, and two new tmux hooks configured to automatically trigger session state persistence on client detachment and session closure. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Mesa DescriptionTL;DRAuto-saves What changed?
Description generated by Mesa. Update settings |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces automatic tmux session saving when a client detaches or a session closes, and adds a one-second delay in the fish shell function to prevent race conditions during session restoration. The review feedback suggests adding error handling for the tmux save hooks to avoid silent failures and documenting the hardcoded sleep duration in the fish script to improve maintainability.
| set -g @continuum-save-interval '3' | ||
|
|
||
| # Auto-save on detach/close so kills don't lose state | ||
| set-hook -g client-detached 'run-shell #{@resurrect-save-script-path}' |
There was a problem hiding this comment.
The run-shell command executes the save script, but if the script fails (e.g., due to @resurrect-save-script-path being undefined, pointing to a non-executable file, or the script itself encountering an error), the save operation will silently fail. This could lead to unexpected data loss, as the user would assume their session state is being saved on detach/close. It is recommended to add error handling or logging within the script or the run-shell command (e.g., sh -c "script || tmux display-message ...") to provide feedback if the save fails.
| set -l restore $restore[1] | ||
| if test -n "$restore" | ||
| tmux run-shell "$restore" | ||
| sleep 1 |
There was a problem hiding this comment.
The sleep 1 command addresses the race condition, but using a hardcoded numerical delay can be fragile. If the system is under heavy load or the tmux restore operation takes longer than expected, 1 second might not be sufficient, leading to the race condition re-emerging. Conversely, if 1 second is consistently more than needed, it introduces an unnecessary delay. Adding a comment to explain the rationale behind the 1-second delay improves maintainability by clarifying the purpose of this magic number.
sleep 1 # Empirically determined delay to allow tmux session restoration to complete.
There was a problem hiding this comment.
Pull request overview
Adds tmux hooks intended to automatically persist tmux-resurrect state when detaching or closing sessions, reducing the chance of losing the latest “work” session snapshot.
Changes:
- Add global tmux hooks for
client-detachedandsession-closedto trigger tmux-resurrect save.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| # Auto-save on detach/close so kills don't lose state | ||
| set-hook -g client-detached 'run-shell #{@resurrect-save-script-path}' | ||
| set-hook -g session-closed 'run-shell #{@resurrect-save-script-path}' |
There was a problem hiding this comment.
The session-closed hook runs when a session is closed, so @resurrect-save-script-path will snapshot after the session is gone. Because the post-save hook filters the saved file down to work panes, closing the work session can overwrite ~/.tmux/resurrect/last with an effectively empty snapshot, breaking restore. Consider removing the session-closed hook, or wrapping the save command to no-op unless the work session still exists (to avoid clobbering the last good snapshot).
| set-hook -g session-closed 'run-shell #{@resurrect-save-script-path}' |
| set-hook -g client-detached 'run-shell #{@resurrect-save-script-path}' | ||
| set-hook -g session-closed 'run-shell #{@resurrect-save-script-path}' |
There was a problem hiding this comment.
run-shell without -b runs synchronously; on detach/close this can block the tmux hook until resurrect finishes saving (potentially slow when capturing pane contents). Consider using background mode (run-shell -b ...) to avoid UI delays/hangs during detach/close.
| set-hook -g client-detached 'run-shell #{@resurrect-save-script-path}' | |
| set-hook -g session-closed 'run-shell #{@resurrect-save-script-path}' | |
| set-hook -g client-detached 'run-shell -b #{@resurrect-save-script-path}' | |
| set-hook -g session-closed 'run-shell -b #{@resurrect-save-script-path}' |
| # Auto-save on detach/close so kills don't lose state | ||
| set-hook -g client-detached 'run-shell #{@resurrect-save-script-path}' | ||
| set-hook -g session-closed 'run-shell #{@resurrect-save-script-path}' |
There was a problem hiding this comment.
PR description mentions adding a sleep after resurrect restore in two to fix a race, but _two_function.fish currently has no such delay (it runs tmux run-shell "$restore" and immediately checks for the work session). Either include that change or update the PR description so it matches what’s being shipped.
| set -g @continuum-restore 'off' | ||
| set -g @continuum-save-interval '3' | ||
|
|
||
| # Auto-save on detach/close so kills don't lose state |
There was a problem hiding this comment.
Comment says “kills don't lose state”, but these hooks won’t run if the tmux server is killed (e.g., kill-server/SIGKILL). Consider rewording to reflect detach/session-close only, so the config doesn’t over-promise durability.
| # Auto-save on detach/close so kills don't lose state | |
| # Auto-save on detach/normal session close to preserve state |
Summary
twoto fix race conditionTest plan
two, verify work session restores~/.tmux/resurrect/lastis updatedSummary by cubic
Auto-saves
tmux-resurrectstate on detach and session close, and adjuststworestore timing to avoid races and lost state.New Features
tmuxhooks:client-detachedandsession-closedrun@resurrect-save-script-pathto persist state.Bug Fixes
_two_function.fish, addsleep 1aftertmux-resurrectrestore to avoid a startup race.Written for commit c6350b5. Summary will update on new commits.