fix: prevent data loss with onbeforeunload warning#188
Conversation
| ); | ||
| expect(within(dialog).getByRole('button', { name: 'Keep Reviewing' })).toBeInTheDocument(); | ||
| expect(within(dialog).getByRole('button', { name: 'Approve Plan' })).toBeInTheDocument(); | ||
| expect(within(dialog).queryByRole('button', { name: 'Close Anyway' })).not.toBeInTheDocument(); |
There was a problem hiding this comment.
Review agent: use test ids throughout vs. getByRole. Review the agent instructions for details.
| }); | ||
| }); | ||
|
|
||
| it('opens the approval close dialog after staying on an attempted close with no comments', async () => { |
There was a problem hiding this comment.
This helps prevent the case described here: https://contextbridge.slack.com/archives/C0ATDN9SSR3/p1779224919519899
Where a user closed the tab without clicking approve. I learned that browsers do require some interaction with the page before onbeforeunload will work (to avoid spammy stuff). But I assume most people will have at least scrolled the document.
| }); | ||
| }); | ||
|
|
||
| describe('beforeunload guard', () => { |
There was a problem hiding this comment.
I tested these cases manually and they all work.
blimmer
changed the title
jcarver989
left a comment
jcarver989
left a comment
There was a problem hiding this comment.
Overall looks good, would like to make these tests less brittle before merging though.
| expect(dialog).toHaveTextContent('Approve plan before closing?'); | ||
| expect(dialog).toHaveTextContent( | ||
| 'No comments have been added. Select Approve Plan to tell the agent to proceed with the plan as written.', |
There was a problem hiding this comment.
These sorts of tests are super brittle because they test copy that can change at any time. I'd either just assert that the test id you want is there or alternatively pull the copy into a constant / exported var and import it in both the component and tests.
There was a problem hiding this comment.
Historically I would have agreed strongly with you. But idk that I feel that way with AI-driven development. If the message changes, just verify will fail and the agent will just update the test.
My main goal here is that we're testing the two different cases, which it feels like we're doing here decently well right now. But if you prefer the constant, that's an easy fix 👍
There was a problem hiding this comment.
You still end up burning a lot of wall clock time and tokens while the agent fumbles around fixing the tests. And that scales as an O(N) problem the more brittle tests the agent adds.
I think it's good to do it now so it re-enforces the pattern with agents to not do that, which avoids them needing to update 100 broken tests b/c we changed copy when our test suite gets huge later down the road.
There was a problem hiding this comment.
I'll extract to constants
## Summary Aligns three existing UI surfaces with the test-copy-constants rule introduced in #188 (`.claude/rules/test-copy-constants.md`): `UpdateNoticeCard` text, the close-review dialog's "Keep Reviewing" cancel label, and the empty-state notice now flow through exported `xxxCopy` objects that the corresponding tests import. Also removes two stale `not.toHaveTextContent('Close Anyway')` assertions whose target string never existed in source.
🤖 I have created a release *beep* *boop* --- ## [0.8.0](v0.7.2...v0.8.0) (2026-05-20) ### Features * add planbridge-last skill to open the last agent message ([#189](#189)) ([dbfc615](dbfc615)) ### Bug Fixes * prevent data loss with onbeforeunload warning ([#188](#188)) ([ea1f5d5](ea1f5d5)) * render images referenced from the local filesystem ([#165](#165)) ([bdf98ff](bdf98ff)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: contextbridge-pr-automation[bot] <259134118+contextbridge-pr-automation[bot]@users.noreply.github.com>
2 participants
Summary
Adds a custom close-review dialog when a user attempts to leave an unsubmitted annotation review. The dialog now recommends approving an empty review or submitting existing feedback, while keeping the browser beforeunload guard in place.
No comments
With comments
Review focus
Please look closely at the
FrontendBrowser.addBeforeUnloadGuardcallback shape and whether triggering React dialog state from the beforeunload path is the right API boundary.Commits
75f4d32— prompt before closing annotation review