Repository navigation
Explain controls that cannot work during the first review load, and keep composer focus when it fails #173
Copy link
Copy link
Open
Labels
bugSomething isn't workingSomething isn't working
Description
Follow-up from #172, found during its review passes 3, 7 and 8. All three problems existed before #172.
Background
web/public/app.js ends with a top-level await refresh(). That first /api/review request can take up to 30 seconds on a large history. Since #170, the page stays usable while the request runs, so a reviewer can now reach these controls during that time.
AGENTS.md "Async review UI" has two relevant rules:
- "While a request is in flight, do not disable the control that has keyboard focus … Mark it
aria-disabled, ignore repeat activation with an in-flight guard, and test that focus stays on the control after the response." - DESIGN.md: "A disabled button still explains itself."
Problems
1. "Ask agent" is disabled during the first load and gives no reason
renderAttachment() (web/public/app.js:747) sets $("save-note").disabled = busy || ….
- What happens: while the first load runs, a reviewer types a question and tries to send it. The button is greyed out and nothing says why.
- Why it matters: the text is kept, so nothing is lost, but the control does not explain itself.
- Expected: a visible reason, for example in
#saved("The review is still loading. Your text is kept; send it after it loads."). Consideraria-disabledinstead ofdisabledif the button can hold focus.
2. "Refresh plan" silently does nothing during the first load
The #plans-refresh handler (web/public/app.js:1686) returns early at line 1688 (if (busy) return;). The button is not marked aria-disabled, and no message appears.
- What happens: the reviewer opens Plans during the first load. The summary says "Loading plan…", and the reviewer presses "Refresh plan". Nothing happens.
- Expected: the button is
aria-disabledwhile the review read runs, and pressing it says why. The message should be written under the current status owner (setPlanStatus(planStatusGeneration, …)), asimportPlandoes for its "still loading" refusal, so the load's result still replaces it.
3. A failed first load drops keyboard focus from the composer
showFailure() (web/public/app.js:113) sets $("message").disabled = true.
- What happens: the reviewer is typing in the composer when the first load fails. The textarea becomes disabled and focus moves to the page. The text stays visible and is restored on the next successful load.
- Expected: focus stays on the composer. Use
readonlyoraria-disabledwith a guard instead ofdisabled, and keep refusing submission while no review is loaded.
Acceptance
- Each control above gives a visible reason while it cannot work, with the status icon, word and colour that DESIGN.md requires.
- No control that can hold focus is natively disabled while a request is in flight.
- Playwright regressions in
test/browser/act before the first/api/reviewresponse. Each test holds that response withpage.route('**/api/review', …)and releases it from the test, and does not useopenRevieworreloadReviewfor the step under test (seetest/browser/first-load.spec.tsfor the pattern). Each test asserts:- the visible message;
- that focus stays on the control;
- that no request was sent and nothing was saved by mistake;
- and fails on the current
main.
- Any message written during a running read is replaced by that read's own result. Test each status line the message appears on (the Plans status line or
#saved).
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
bugSomething isn't workingSomething isn't working
Activity