Fix plan-approval mode not sticking across app restarts#3737
Conversation
|
😎 Merged directly without going through the merge queue, as the queue was empty and the PR was up to date with the target branch - details. |
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
haacked
added
the
Stamphog
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/ui/src/features/permissions/PlanApprovalSelector.tsx:145-147
**Interaction Guard Stays Set Across Requests**
If this component remains mounted while its `options` change for a later approval request, a mode selected during the first request leaves `userChangedModeRef` permanently true. The effect then ignores the new `initialMode`, so the later request can retain a stale mode or show the fallback selector when that mode is no longer available.
Reviews (1): Last reviewed commit: "Fix plan-approval mode not sticking acro..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Contained UI fix for a settings-hydration race in the plan-approval mode selector, with unit tests covering both the hydration re-sync and the don't-clobber-user-choice case; not in risky territory (no auth/billing/API/CI/dependency/migration surface touched).
- greptile-apps[bot] reviewed the current head.
- Greptile flagged (P2, non-blocking) that if this component stays mounted across multiple sequential approval requests, the userChangedModeRef guard could keep blocking re-sync for a later request — a pre-existing limitation this PR doesn't worsen for the common case, but worth a follow-up if the component is ever reused without remounting between requests.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 19L, 1F substantive, 73L/2F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1b-small (73L, 2F, single-area, unknown) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ 744d66a · reviewed head 505456a |
505456a to
56fc0e9
Compare
New commits pushed (delta classified non_linear_history) — stamphog approval dismissed; re-review running automatically.
There was a problem hiding this comment.
Contained UI state-management fix with two new tests covering the hydration race; the sole reviewer comment (about a stale mode leaking into a later request) is already addressed by the added toolCallId-reset logic and corresponding test, and is marked outdated. Not risky territory.
- 👍 on the PR from greptile-apps[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 26L, 1F substantive, 97L/2F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1b-small (97L, 2F, single-area, unknown) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ c9605b5 · reviewed head 56fc0e9 |
|
✅ Visual changes approved by @haacked — baseline updated in 12 new. |
22042b1 to
6934c99
Compare
Settings persist asynchronously (an IPC round trip on desktop), so
PlanApprovalSelector can mount before lastPlanApprovalMode has loaded
from disk -- e.g. resuming a task with an already-pending plan
approval. The pre-selected mode was seeded once via useState and never
revisited, so it stayed on the pre-hydration fallback ("auto") even
after the real remembered mode loaded moments later.
Only the user's own pick now lives in state; the pre-selected mode
derives from lastApprovalMode on every render instead, so it tracks
the settings store live and self-corrects once hydration lands.
If this component survives to a later approval request without
remounting, reset that pick when the request's toolCallId changes so
it doesn't leak into (and potentially not exist among the options of)
the next request.
The dropdown applies a mode pick when its close animation completes, not synchronously with the click. Two tests moved on immediately after clicking a menu item, racing that pending callback against a subsequent store mutation or rerender -- flaky roughly half the time under the full suite.
6934c99 to
061daab
Compare
New commits pushed (delta classified non_linear_history) — stamphog approval dismissed; re-review running automatically.
There was a problem hiding this comment.
Contained UI bug fix (React state hydration race) with tests; the one substantive review concern (stale mode surviving into a later request) is directly addressed by the toolCallId-based reset and covered by a new test, and is marked resolved. No risky-territory surface touched.
- 👍 on the PR from greptile-apps[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 26L, 1F substantive, 113L/2F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (113L, 2F, single-area, unknown) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ 65af803 · reviewed head 061daab |
1 participant
Problem
Settings persist asynchronously (an IPC round trip on desktop), so PlanApprovalSelector can mount before lastPlanApprovalMode has loaded from disk, such as when resuming a task with an already-pending plan approval. Since useState's initializer only runs once, the selector locked onto the pre-hydration fallback of "auto" and never picked up the real remembered mode once hydration finished, so the remembered plan-approval mode didn't stick across app restarts.
Changes
Re-sync the selected mode once the settings store finishes hydrating, unless the user has already picked a mode themselves. A
userChangedModeReftracks explicit user selection so the re-sync doesn't clobber a choice made before hydration lands.How did you test this?
Added two tests covering the hydration race: one confirms the selector re-syncs to the remembered mode once hydration lands after mount, and one confirms an explicit user selection made before hydration lands isn't overwritten by it.
Automatic notifications