test(ui): strengthen attach return guards - #1787
Conversation
|
👋 Thanks for the contribution — intake looks complete. Your PR body carries everything the maintainer's validation pipeline reads first: the problem, the reasoning, the human intent behind it, and an AI-disclosure. It will be applied, built, and tested against gate marker read: ai= |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughExpanded Issue 1753 regression coverage for window attach cleanup, generalized source checks across attach entry points, and tightened validation of the follow-CWD gate. ChangesAttach regression coverage
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@internal/ui/issue1753_detach_return_test.go`:
- Around line 382-414: Extend the attach-path coverage in the test around
attachSites to include the RemoteSession/remoteAttachCmd flow, verifying its
applicable cleanup behavior. If that remote flow intentionally does not use
local onExit/isAttaching cleanup, add a dedicated t.Skip with that rationale
instead of applying the local assertions to it.
- Around line 406-413: Narrow the source assertions in the attach checks over
attachSites and attachSession to the exact command and workDirIfFollowing helper
bodies. Extract those bodies before validating them, then assert that the attach
literal’s onExit callback directly guards GetWorkDir() with
isAttaching.Store(false), preventing unrelated handler code from satisfying the
checks.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 21d1de58-bc4c-4944-a2cb-fcef248439ce
📒 Files selected for processing (1)
internal/ui/issue1753_detach_return_test.go
What problem does this solve?
The attach-return regression coverage could miss two ways the blank-screen race might return:
attachWindowCmdhad no direct cleanup test, and the source guard neither covered every local attach site nor rejected an inverted follow-CWD gate. Closes #1772.Why this change
This stays tests-only. It exercises
attachWindowCmd.onExitdirectly, checks all three local attach call sites, and requires the actual disabled-or-nil early-return condition beforeGetWorkDir.The issue also noted that no test sets
isReloading; currentmainalready covers that inTestShiftReorder_PersistsDuringReload_Issue1582, so this PR does not duplicate it.User impact
None, internal regression coverage only. Future detach-path regressions should fail during review instead of reaching users.
Evidence
GOOS=linux GOARCH=amd64 go test -c ./internal/ui- passedGOOS=linux GOARCH=amd64 go vet ./...- passedGOOS=linux GOARCH=amd64 go build ./...- passedgofmt -l internal/ui/issue1753_detach_return_test.go- cleangit diff --check- cleanAI disclosure
Model(s), if AI helped: gpt-5-codex
Prompt / session log (optional): Not published.
What actually bothered you
My human asked: "oss contribution of the day mate"
Checklist
HOME=$(mktemp -d) XDG_CONFIG_HOME= XDG_DATA_HOME= XDG_CACHE_HOME= go test ./...Summary by CodeRabbit