Skip to content

Explain controls that cannot work during the first review load, and keep composer focus when it fails #173

Open

Description

@mchwang

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."). Consider aria-disabled instead of disabled if 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-disabled while the review read runs, and pressing it says why. The message should be written under the current status owner (setPlanStatus(planStatusGeneration, …)), as importPlan does 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 readonly or aria-disabled with a guard instead of disabled, 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/review response. Each test holds that response with page.route('**/api/review', …) and releases it from the test, and does not use openReview or reloadReview for the step under test (see test/browser/first-load.spec.ts for 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).

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions