Keep input made before the first review load, and explain a refused plan import

[mchwang]

Follows #170, which added test/browser/review-page.ts and stopped the server from blocking the page while the first review load runs. This PR was stacked on #170. After #170 was squash-merged as 99e58a9, this PR was retargeted to main, and origin/main was merged in with -s ours. That merge is exact because main's tree equals c4667ad, this branch's former base. The diff against main is byte-identical to the former diff against c4667ad (both hash 4ce3d34). This PR fixes what the page does with input made before that first load answers.

Problem

The page ends with a top-level await refresh(). Its /api/review request can take up to 30 seconds. Before it answered:

1. Text typed into the composer ("Question about this item") was erased when the answer arrived. rememberDraft() keyed drafts by the selected item, and no item was selected yet. Switching between Ask and Request change erased it too.
2. "Import next revision" on Plans did nothing and showed nothing: importPlan() returned when !data?.plan.

Both break the rule in AGENTS.md "Async review UI": a background response must not erase input made after the request started.

Change (web/public/app.js)

Composer

• Text typed with no selected item is kept under the key :<mode>, for both Ask and Request change.
• The first time render() selects an item, it hands that text to that item. No "Retained draft" row is created.
• If the first load fails, the text is kept, and the next successful load hands it over the same way.

Plans and import

• While the review is being read with no plan loaded, the Plans summary shows "Loading plan…" and the agent panel says "The plan is still loading. Ask the agent after it appears." This covers the first load and Refresh plan.
• During the first load, and during a Review Refresh after a failed load, the Plans status line says "Loading the current plan…". During Refresh plan, it keeps saying "Refreshing the current plan…".
• #plan-import is aria-disabled whenever no plan is loaded. Pressing it explains the refusal, with or without a file chosen:
  • While loading: "! The plan is still loading. Nothing was imported. Import after it appears." This is written under the load's status ownership, so the load's result replaces it: ✓ on success. If the page's review read fails, its ✕ appears when the reviewer is on Plans, and the status is cleared when they are on Review. If Refresh plan fails, it always writes "✕ Could not refresh the current plan…".
  • After a failed load: "✕ No plan is loaded, so nothing was imported. Use Refresh plan, then import."
• The import form is now novalidate (web/public/index.html). Before, the file input's required let the browser's own check answer first, asking for a file instead of explaining that no plan was loaded. importPlan already reports a missing file itself.
  • User-visible change: with a plan loaded and no file chosen, Import now shows "✕ Choose a JSON or YAML plan file." on the status line instead of the browser's "select a file" bubble. Like the wrong-format refusal, this always claims the status line, even while a refresh is running. Feedback on the file just chosen is newer than that refresh, so the refresh's result must not replace it. The existing test "does not let an older failed Review refresh replace newer Plans validation feedback" already requires this.
• The "another action is running" refusal is now checked before the "no plan" refusal. It writes under the current status ownership instead of claiming it, so a running Apply replaces it with its result. After a successful act or a merge, neither of which writes the Plans status line, the refusal stays until the next status write, as before. A failed act claims the line: its ✕ appears if the reviewer is on Plans, and the line is cleared if they are on Review. Without this, an import pressed during a Retry Apply made after a failed refresh claimed the status line and hid the Apply's result.

Tests

test/browser/first-load.spec.ts (new, 5 tests)

Each test holds /api/review with page.route and releases it from the test. None of them uses openReview or reloadReview for the step under test.

1. Question and change-request text typed before load, including text typed after the last mode switch: kept, focus stays on the composer, the text is given to P1, no Retained draft row. Checks: no /api/action request and no notes saved.
2. Question and change-request text typed before a first load that fails, including text typed after the last mode switch: the text stays visible in the disabled composer. When Refresh succeeds, it is given to P1, with no Retained draft row and no notes saved.
3. Import pressed during the first load, first with no file chosen and then with one: each keyboard activation of the aria-disabled button shows the warning and keeps focus. After the load, the ✓ result replaces the warning, no import request was sent, and the plan is still r1. The file chosen earlier then imports normally (r2).
4. Import pressed after the first load failed, with no file and then with one: the ✕ refusal is shown with the bad status colour, no import request is sent, and the plan is still r1.
5. Import pressed during Refresh plan after a failed first load: the "still loading" warning is shown, then the refresh's ✓ result replaces it.

test/browser/plans.spec.ts (+1 test)

• With a plan loaded, Import pressed with no file chosen shows "✕ Choose a JSON or YAML plan file." with the bad colour and keeps focus. No import request is sent and the plan is still r1. Without novalidate it fails: the status line still shows "✓ Revision r1 is current.".

test/browser/plans-authoring.spec.ts (+1 test)

6. Import pressed during a Retry Apply made after a failed review refresh: the warning is shown, then the Apply's "✓ Revision r2 is current." replaces it, and no import request is sent.

Failing before, passing after: all 6 tests were run against the base app.js (c4667ad). Each failed at this first assertion:

1. Composer value was "" (the Ask/Request change switch erased the text).
2. After the failed load, the composer held only " Still typing." instead of the full text. The cause is the same as in test 1: switching Ask/Request change before the load erased the earlier text, and only the text typed after the switch survived.
3. #plans-summary showed "Plan unavailable" instead of "Loading plan…".
4. #plan-import was not aria-disabled while no plan was loaded.
5. #plans-summary showed "Plan unavailable" during Refresh plan.
6. The status line still showed "Applying suggestion…": the import press was a silent no-op.

All 6 pass on this branch. The no-file presses in tests 3 and 4 were also run without novalidate: both failed, because the browser's file check stopped the form before importPlan ran.

Validation

These are separate gates.

• Validated head fb14a34 (base main 99e58a9; same tree as ffd353f):
  • Full browser suite: 144/144 passed (12.8 min).
  • CI test: passed.
• Main after the Run review history work in an abortable worker thread #170 merge (99e58a9): CI test and real-docker both passed.
• Intermediate checkpoint ffd353f (former base c4667ad):
  • Full browser suite 144/144 (13.8 min).
  • tsc --noEmit clean.
  • All 7 new tests failed against base app.js and index.html, at the symptoms listed above.
• Earlier intermediate checkpoints:
  • 13757a9: 142/142.
  • 6167c6f: stopped before finishing; head replaced.
  • 71a7a4a: 143/143.
  • c576ecb: 144/144.
  • 8072620: stopped before finishing; head replaced.
• Vitest: not run. No server or TypeScript source changed.
• Real Docker: not run on this branch; only browser code and tests changed. It passed on main 99e58a9.

Review record

Passes 1 to 10 and Copilot round 1 reviewed the full diff against the former base c4667ad; the sync onto main leaves that diff byte-identical. Passes 1 to 3 ran on the uncommitted working tree; passes 4 onward name their exact commit pair.

1. Pass 1. Finding: Refresh plan reads without beginBusy, so an import during it claimed the ✕ status and hid the refresh's ✓. Fixed: reviewLoading() now includes planRefreshPending, and Plans re-renders when a refresh starts and ends; added the test now numbered 5. Also added a pageerror check to the test now numbered 4.

2. Pass 2. Finding: Retry Apply can be a write while data is null, and the import's claimed ✕ hid the Apply's result. Fixed: the busy-write refusal is checked first and writes under the current owner; added the test now numbered 6. The reviewLoading comment was corrected.

3. Pass 3. No defects. Low-severity points:

   • The renderPlanImport comment overstated behaviour during a running import. Fixed: comment reworded, no behaviour change.
   • "Ask agent" is natively disabled during the first load and gives no reason. Declined: the text is preserved, this is not a regression, and the button behaved this way during every refresh before. It is listed as a follow-up below.
   • After act or merge, the "! Wait for the current review action…" refusal stays on the Plans status line. Declined: this is unchanged from before; neither operation writes that line.

4. Pass 4 (independent subagent, exact pair c4667ad..13757a9). No code defects. Findings:

   • Test gap: no test covered "text typed before a failed first load is kept and handed over by the next successful load". Fixed: test 2 added in 6167c6f. It fails on base.
   • PR body: the failing-before symptoms for tests 3, 4 and 6 did not match the base app.js. The Refresh plan status wording was wrong, and the review record was stale. Fixed: this body was corrected, and each symptom above comes from a run against c4667ad.
   • Low, declined: if the first load returns no items, the adopted draft goes to "Unplanned", where the composer is disabled but the text stays visible. Before this PR the text was erased, and the plan schema requires at least one item.
   • Low, declined: #plans-identity is not cleared when no plan is loaded. This is unchanged from base.

5. Pass 5 (independent subagent, exact pair c4667ad..6167c6f). No defects in status ownership, drafts, or aria-disabled. Findings:

   • Low: with no file chosen, the browser's required check stopped the form before importPlan, so the no-plan explanation never appeared. The renderPlanImport comment and this body claimed it always did. Fixed in 71a7a4a: the form is novalidate; tests 3 and 4 now press Import with no file first, and both fail without novalidate.
   • Low, test gap: test 4 did not check the bad class of its ✕ message. Fixed: it does now.
   • PR body: test numbers in the review record were stale, and symptom 2 was misattributed. Fixed above.

6. Pass 6 (independent subagent, exact pair c4667ad..71a7a4a). No correctness defects. It confirmed that novalidate affects only the import form, and judged that blanking #plans-status between presses in tests 3 and 4 is acceptable: the page writes that element but never reads it. Findings:

   • Low: the importPlan comment said refusals keep the running operation's ownership, but the file refusal claims it. Fixed in c576ecb: the comment now names which refusals keep ownership.
   • Low, test gap: nothing tested Import with a plan loaded and no file. Fixed: new plans.spec.ts test, which fails without novalidate.
   • PR body: three claims were imprecise: the busy refusal's ownership during act or merge, the ✕ result when the user is on Review, and the user-visible novalidate change with a plan loaded. Fixed above.

7. Pass 7 (independent subagent, exact pair c4667ad..c576ecb). No correctness defects. All six failing-before symptoms were confirmed against base. Findings:

   • Low: the importPlan comment said "these two refusals keep ownership", but the not-loading "no plan" branch claims the line. Fixed in 8072620: comment only.
   • Pre-existing, now easier to reach (declined here): during the first load, "Refresh plan" returns silently while busy, without aria-disabled or a reason. Since Run review history work in an abortable worker thread #170 the page can be used during that load, so this is now reachable for up to 30 s. Added to Follow-up.
   • PR body: the review-record header was stale, and the claim about act and merge left out that a failed act writes ✕. The validation list showed the exact-head suite as running. Fixed above; the result will be recorded when the run finishes.

8. Pass 8 (independent subagent, exact pair c4667ad..8072620). No correctness defects in the PR's fixes, and no inaccurate PR-body claims. Findings:

   • Low, declined: since novalidate, a no-file press during a running refresh claims the status line and hides that refresh's result. The reviewer suggested writing it under the refresh's ownership. Declined: the project already requires the opposite for file feedback, in "does not let an older failed Review refresh replace newer Plans validation feedback", and the no-file press now follows that rule.
   • Low: the importPlan comment said the file refusal claims the line only "with nothing running". Fixed in ffd353f: the comment now says the file refusal always claims it, and why.
   • Low, test gap: tests 1 and 2 switched modes before the response, which saved the text early. Neither covered the save made when the response applies. Fixed: both now type after the last switch. Test 1 fails if the rememberDraft() call just before render() in refresh is removed.
   • Pre-existing (added to Follow-up): when the first load fails, showFailure sets disabled on #message, so a focused composer drops focus to the page.

9. Pass 9 (independent subagent, exact pair c4667ad..ffd353f). No code, comment or test defects. Every new test fails if its feature breaks. Finding:

   • PR body only: failing-before symptom 2 was stale after pass 8 changed test 2. Fixed above; the head is unchanged.
   • Observation, no change: the rememberDraft() in the refresh catch predates this PR and is redundant, because the next refresh saves the composer at its start. This PR does not claim that test 2 covers it.

10. Pass 10 (independent subagent, exact pair c4667ad..ffd353f). No code, comment or test defects. Focused runs on the head: first-load.spec.ts 5/5; the new plans.spec.ts and plans-authoring.spec.ts tests 2/2. All failing-before symptoms confirmed against base. Finding:

    • PR body only: "A failed act replaces it with its own ✕" holds only on Plans; on Review the line is cleared. Fixed above; the head is unchanged.

11. Pass 11 (independent subagent, exact pair 99e58a9..fb14a34, after the sync onto main). No code, comment or test defects. It verified the sync claims: equal trees, an identical diff hash 4ce3d34, and the merge commit's parents. All 7 new tests passed on the head (17 s). It checked the failing-before symptoms against 99e58a9 by reading the code. Findings:

    • PR body only: the "While loading" bullet's ✕-or-cleared result applies to the page's review read; a failed Refresh plan always writes its own ✕. It also did not mention "Loading the current plan…" during a Review Refresh after a failed load. Fixed above; the head is unchanged.

Copilot round 1 (requested on ffd353f after pass 10; the user approved the request). Review submitted on commit ffd353f: "Approval recommended", 0 open findings, 0 inline comments, 0 unresolved threads.

Copilot round 2 (requested on fb14a34 after pass 11, on the new base main 99e58a9; the user approved the request). Review submitted on commit fb14a34 at 04:22 UTC: "Approval recommended", 0 open findings, 0 inline comments, 0 unresolved threads.

Follow-up

• While the first review load is pending, explain the two controls that cannot work yet:
  • the disabled "Ask agent" button, for example with a status line in #saved
  • "Refresh plan", which returns silently while busy; it needs aria-disabled and a reason
• When the first load fails, keep focus on the composer: use aria-disabled or readonly instead of disabled on #message.

Review-lesson audit

Every finding from every round is classified below. Quoted rules are in AGENTS.md, mirrored in CLAUDE.md where noted, or in DESIGN.md.

• Pass 1 (Refresh plan not treated as a load): covered by an existing rule. AGENTS.md "Async review UI": "A background response must not erase text, selections, attachments, navigation changes, or other input made after the request started". Also "Required race regressions": test every applicable interleaving. A missed interleaving, not a new kind of rule.
• Pass 2 (Retry Apply with no plan, hidden by a claimed ✕): covered by an existing rule, the same two rules as pass 1.
• Pass 3 (the renderPlanImport comment overstated behaviour): covered by an existing rule. "Treat every behavioural claim the change makes, in code comments, the PR body or docs … as something to verify" (AGENTS.md and CLAUDE.md).
  • Declined, one-off: "Ask agent" disabled without a reason, and the act/merge refusal persisting. Neither is a regression; the first is listed under Follow-up.
• Pass 4:
  • Untested failed-load handover: covered by an existing rule. "…and give it a test that fails if the claim is false" (AGENTS.md and CLAUDE.md).
  • PR-body symptom errors: covered by an existing rule, the behavioural-claim rule as in pass 3.
  • Declined, one-off: the no-items edge case (the schema requires at least one item) and #plans-identity, which is unchanged from base.
• Pass 5:
  • The browser's required check pre-empted the refusal: covered by an existing rule, the behavioural-claim rule as in pass 3. The comment and body claimed the refusal is always explained.
  • The ✕ colour was not asserted: covered by an existing rule. DESIGN.md "Status is always an icon plus a word plus a color", with the test rule as in pass 4.
  • Stale body numbers: covered by an existing rule, the behavioural-claim rule as in pass 3.
• Pass 6 (comment wording, the no-file test gap, three imprecise body claims): covered by existing rules, the behavioural-claim and test rules as in passes 3 and 4.
• Pass 7 (comment wording, stale body text): covered by an existing rule, the behavioural-claim rule as in pass 3.
  • Pre-existing, one-off: Refresh plan returns silently during the first load. It falls under AGENTS.md "While a request is in flight, do not disable the control that has keyboard focus … Mark it aria-disabled". It is listed under Follow-up, not fixed here.
• Pass 8:
  • An earlier mode switch saved the text that the test meant to check: covered by an existing rule. "A test's setup must leave the state the production path would: if production never runs a step … the test must not run it before the behaviour under test either" (AGENTS.md and CLAUDE.md).
  • Comment wording: covered by an existing rule, the behavioural-claim rule as in pass 3.
  • Declined: the file refusal during a running refresh. The existing test "does not let an older failed Review refresh replace newer Plans validation feedback" already sets the opposite rule.
  • Pre-existing, one-off: showFailure disables a focused composer. It falls under the AGENTS.md keyboard-focus rule above and is listed under Follow-up.
• Pass 9 (stale symptom 2 in the body): covered by an existing rule, the behavioural-claim rule as in pass 3.
  • Observation, one-off: the redundant rememberDraft() in the refresh catch predates this PR. No change.
• Pass 10 (one imprecise sentence about a failed act): covered by an existing rule, the behavioural-claim rule as in pass 3.
• Pass 11 (one imprecise sentence about Refresh plan's failure, and one omission): covered by an existing rule, the behavioural-claim rule as in pass 3.
• Copilot rounds 1 and 2: no findings, so nothing to classify.

Result: every finding is covered by an existing rule or recorded as one-off. No new AGENTS.md rule is needed, so this branch makes no AGENTS.md or CLAUDE.md change.

🤖 Generated with Claude Code

[Copilot]

🟢 Approval recommended

The asynchronous state transitions are guarded appropriately and comprehensively covered by regression tests.

0 open findings

What changed in this PR

Preserves composer input across the initial asynchronous review load and provides clear plan-import feedback when no plan is available.

Changes:

• Retains pre-load drafts and assigns them to the first selected plan item.
• Adds loading, disabled-state, and refusal feedback for plan imports.
• Adds browser regressions for draft retention and import-status races.

File	Description
web/​public/​index.html	Enables application-managed import validation.
web/​public/​app.js	Preserves drafts and improves plan loading/import state handling.
test/​browser/​first-load.spec.ts	Covers first-load draft and import behavior.
test/​browser/​plans.spec.ts	Tests missing-file import feedback.
test/​browser/​plans-authoring.spec.ts	Tests import refusal during retry apply.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟢 Approval recommended

The asynchronous state handling is consistent and backed by focused race regression coverage.

0 open findings

🧠 Review effort: Balanced