fix(two): restore work session from resurrect when server is dead - #1286
Conversation
Entire-Checkpoint: 358285925965
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Disabled knowledge base sources:
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThe Changes
Sequence DiagramsequenceDiagram
participant Fish as Fish Function
participant Tmux as Tmux
participant Resurrect as Resurrect Script
participant Tmuxinator as Tmuxinator
Fish->>Tmux: has-session -t work
alt Session Exists
Tmux-->>Fish: success (exit 0)
Fish->>Tmux: attach-session -t work
else Session Not Found
Tmux-->>Fish: failure (exit 1)
Fish->>Fish: check ~/.tmux/resurrect/last
alt Resurrect Path Exists
Fish->>Tmux: new-session -d -s _restore
Fish->>Tmux: run-shell (restore.sh)
Tmux->>Resurrect: execute restore script
Resurrect-->>Tmux: restoration complete
Fish->>Tmux: has-session -t work
alt Work Session Created
Fish->>Tmux: attach-session -t work
else Work Session Not Created
Fish->>Tmux: kill-session -t _restore
end
else No Resurrect Path
Fish->>Tmuxinator: start work
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ 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;DRFix
What changed?Description generated by Mesa. Update settings |
![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 refactors the _two_function fish function to attempt session restoration via tmux-resurrect before falling back to tmuxinator. The implementation involves creating a temporary tmux session to execute the restoration script. Feedback suggests improving the test suite's reliability by using an exit handler for file restoration to avoid potential data loss. Additionally, it is recommended to consolidate the session cleanup logic in the main function to reduce redundancy.
| set -l _bak "" | ||
| if test -f ~/.tmux/resurrect/last | ||
| set _bak (mktemp) | ||
| mv ~/.tmux/resurrect/last $_bak | ||
| end | ||
|
|
||
| _two_function | ||
|
|
||
| if test -n "$_bak" | ||
| mv $_bak ~/.tmux/resurrect/last | ||
| end |
There was a problem hiding this comment.
The manual backup and restore of ~/.tmux/resurrect/last is not robust. If the test script fails or is interrupted after moving the file (line 18) but before restoring it (line 24), the user's file will be lost. This could be made safer by using an exit handler to guarantee the file is restored.
Consider using functions --on-process-exit to define a cleanup function that will run regardless of how the script terminates.
Example:
function _cleanup_resurrect_file --on-process-exit
if set -q --global __resurrect_last_bak
# Logic to move file back
if test -f "$__resurrect_last_bak"
mv "$__resurrect_last_bak" ~/.tmux/resurrect/last
end
set -e __resurrect_last_bak
end
end
if test -f ~/.tmux/resurrect/last
set -g __resurrect_last_bak (mktemp)
mv ~/.tmux/resurrect/last $__resurrect_last_bak
end
# ... test logic ...
# No manual restore needed here| # Clean up temp session if restore created work | ||
| if tmux has-session -t work 2>/dev/null | ||
| tmux kill-session -t _restore 2>/dev/null | ||
| tmux attach-session -t work | ||
| return | ||
| end | ||
| tmux kill-session -t _restore 2>/dev/null |
There was a problem hiding this comment.
The cleanup logic for the temporary _restore session can be simplified to avoid repeating the tmux kill-session command in the source code. This makes the code more DRY (Don't Repeat Yourself) and easier to maintain.
# Clean up temp session and attach if restore created work
tmux has-session -t work 2>/dev/null
set -l work_restored $status
tmux kill-session -t _restore 2>/dev/null
if test $work_restored -eq 0
tmux attach-session -t work
return
end
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="spec/fish/_two_function_test.fish">
<violation number="1" location="spec/fish/_two_function_test.fish:18">
P2: This test mutates the real ~/.tmux/resurrect/last file and only restores it if the script reaches the cleanup block. If the test aborts early, the user’s resurrect data is lost. Use a guaranteed cleanup (fish_exit handler) or avoid moving the real file.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| set -l _bak "" | ||
| if test -f ~/.tmux/resurrect/last | ||
| set _bak (mktemp) | ||
| mv ~/.tmux/resurrect/last $_bak |
There was a problem hiding this comment.
P2: This test mutates the real ~/.tmux/resurrect/last file and only restores it if the script reaches the cleanup block. If the test aborts early, the user’s resurrect data is lost. Use a guaranteed cleanup (fish_exit handler) or avoid moving the real file.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/fish/_two_function_test.fish, line 18:
<comment>This test mutates the real ~/.tmux/resurrect/last file and only restores it if the script reaches the cleanup block. If the test aborts early, the user’s resurrect data is lost. Use a guaranteed cleanup (fish_exit handler) or avoid moving the real file.</comment>
<file context>
@@ -1,23 +1,35 @@
+set -l _bak ""
+if test -f ~/.tmux/resurrect/last
+ set _bak (mktemp)
+ mv ~/.tmux/resurrect/last $_bak
+end
+
</file context>
There was a problem hiding this comment.
Pull request overview
Updates the Fish two helper to better restore the work tmux session from tmux-resurrect when the tmux server isn’t running, with fallback to tmuxinator.
Changes:
- Reworks
_two_functionto (1) attach/switch ifworkexists, otherwise (2) bootstrap tmux and attempt resurrect restore, then (3) fall back totmuxinator start work. - Updates the Fish spec for
_two_functionto account for the new tmux subcommands used by the restore path.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| spec/fish/_two_function_test.fish | Adjusts stubs and setup for the “no resurrect file” scenario and existing-session attach behavior. |
| home-manager/programs/fish/functions/_two_function.fish | Adds resurrect-based restore flow when work is missing and server may be down, with tmuxinator fallback. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Ensure no resurrect file interferes | ||
| set -l _bak "" | ||
| if test -f ~/.tmux/resurrect/last | ||
| set _bak (mktemp) | ||
| mv ~/.tmux/resurrect/last $_bak | ||
| end | ||
|
|
||
| _two_function | ||
|
|
||
| if test -n "$_bak" | ||
| mv $_bak ~/.tmux/resurrect/last | ||
| end |
There was a problem hiding this comment.
This test manipulates the real ~/.tmux/resurrect/last on the machine running the suite (moves it out of the way and back). That makes the test non-hermetic and can clobber a developer’s resurrect state if the test aborts before the restore step. Prefer isolating by setting HOME to a temp dir (as other fish specs do) and creating/removing a resurrect file under that temp HOME instead of touching the real ~ path.
| # Start a detached session so the server is running | ||
| tmux new-session -d -s _restore 2>/dev/null | ||
| set -l restore (tmux list-keys 2>/dev/null | string match -rg '(/\S+/resurrect/scripts/restore\.sh)') | ||
| set -l restore $restore[1] |
There was a problem hiding this comment.
Using a fixed temp session name (_restore) is risky: if a user already has a session with that name, this function will unconditionally kill it. Consider generating a unique session name (e.g., include PID/random) and only killing the session if it was actually created by this function.
| # Clean up temp session if restore created work | ||
| if tmux has-session -t work 2>/dev/null | ||
| tmux kill-session -t _restore 2>/dev/null | ||
| tmux attach-session -t work |
There was a problem hiding this comment.
In the resurrect-restore path, this always runs tmux attach-session -t work when work appears, even if $TMUX is set (already inside tmux). Attaching from inside tmux will fail; this should mirror the earlier logic and switch-client -t work when inside tmux.
| tmux attach-session -t work | |
| if test -n "$TMUX" | |
| tmux switch-client -t work | |
| else | |
| tmux attach-session -t work | |
| end |
| # No work session — try resurrect restore | ||
| if test -f ~/.tmux/resurrect/last | ||
| # Start a detached session so the server is running | ||
| tmux new-session -d -s _restore 2>/dev/null | ||
| set -l restore (tmux list-keys 2>/dev/null | string match -rg '(/\S+/resurrect/scripts/restore\.sh)') | ||
| set -l restore $restore[1] | ||
| if test -n "$restore" | ||
| tmux run-shell "$restore" | ||
| end | ||
| # Clean up temp session if restore created work | ||
| if tmux has-session -t work 2>/dev/null | ||
| tmux kill-session -t _restore 2>/dev/null | ||
| tmux attach-session -t work | ||
| return | ||
| end | ||
| tmux kill-session -t _restore 2>/dev/null | ||
| end |
There was a problem hiding this comment.
The new resurrect-restore behavior (creating a detached bootstrap session, running restore, then switching/attaching) isn’t covered by the fish specs yet. Adding a test that simulates ~/.tmux/resurrect/last existing and asserts that tmux run-shell is invoked and the function attaches/switches to work would prevent regressions in this main new code path.
Summary
twofunction to properly restore work session via resurrect when tmux server is not runningtmuxinator start workif no resurrect data existsTest plan
two, verify work session is restored from resurrecttwowhen work session already exists, verify it attachesmake shell-testpasses (253/253)Summary by cubic
Fixes
twoso it restores theworksession when thetmuxserver is down. It bootstrapstmuxand restores viatmux-resurrect.workexists, attach or switch immediately.~/.tmux/resurrect/lastexists: start a detached_restoresession to starttmux, run theresurrectrestore script, then clean up and attach towork.workis created, fall back totmuxinator start work.Written for commit 0950d91. Summary will update on new commits.