[puri-adityakumar]

Description

Closes #1407 and #1814.

The tool approval box showed Yes/No right away, with "Yes" highlighted, while nanocoder was still checking the call. Pressing Enter during the validator step (#1814, no loading message at all) or during "Loading preview..." (#1407) approved the tool before you could see what it would do.

What changed in source/components/tool-confirmation.tsx:

1. A new isChecking flag stays on until the schema check, the validator and the formatter preview have all finished. The Yes/No list only appears after that.
2. During the validator step the box shows "Checking tool call...", and during the formatter step it shows "Loading preview...". Both show "Press Escape to cancel".
3. Escape cancels the whole time. Keys pressed before the list appears are dropped, so an early Enter is not replayed once it shows up.
4. If a new request reuses the component, or the theme changes, while a check is still running, the old run's results are ignored so they can't open the gate for the new call.

One choice to flag: in the issue thread, "Esc and 'No' keep working" was the expected behavior. Here Escape is the way to cancel during the check, and "No" comes back as soon as the list appears. I didn't show a "No"-only list during the check because swapping in "Yes" as the default when the check finishes means an Enter pressed right at that moment could approve by mistake. Happy to change it if you'd prefer the "No" option to stay visible.

Type of Change

• Bug fix
• New feature
• Breaking change
• Documentation update

Changeset

• Added a changeset (pnpm changeset) describing this change for the changelog

Testing

Automated Tests

• New features include passing tests in .spec.ts/tsx files
• All existing tests pass (pnpm test:all completes successfully)
• Tests cover both success and error scenarios

Added three tests to the existing source/components/tool-confirmation.spec.tsx: Enter is ignored while the validator runs, Enter is ignored while the preview loads, and Escape cancels during the checks. Three existing tests that pressed keys right after mount now wait for the prompt first. All 13 pass. The two Enter tests fail against the old component, so they catch both bugs. The Escape test passes there too, since Escape already worked.

Format, lint and type checks pass. pnpm test:all has 95 failures locally, and the same 95 fail on a clean checkout of main. They come from my machine: a global git commit-msg hook rejects the test repos' init commits, and the CLI integration tests need a built dist/. None of them touch this component.

Manual Testing

• Tested with Ollama
• Tested with OpenRouter
• Tested with OpenAI-compatible API
• Tested MCP integration (if applicable)

Checklist

• If this was for an open issue, I was assigned to it
• Code follows project style guidelines
• Self-review completed
• Documentation updated (if needed)
• No breaking changes (or clearly documented)
• Appropriate logging added using structured logging (see CONTRIBUTING.md)

No badges/*.svg changes.

[github-actions]

nc-review: comments — 1 important, 2 nits

@puri-adityakumar — a few things worth a look, none blocking.

The change gates the Yes/No tool-approval selector behind a new isChecking flag that clears once the schema check, validator and formatter preview have all settled, so Enter can no longer approve a call that has not been checked or shown. The implementation is sound: the .finally() clears the flag on every exit path including the early returns and the !toolManager bail, the cancelled guard correctly prevents a stale async run from opening the gate for a newer request, Escape still works through the component-level useInput, and the three added tests are genuine regression tests (they assert the prompt is absent during the check, which the old component violated). Issue #1407 is fully resolved; issue #1814's body was not supplied in the context so I could only verify it indirectly via the author's description. One design gap worth fixing before merge: the effect now resets isChecking and isLoadingPreview on a re-run but not the other per-run error flags.

🟠 important · design · source/components/tool-confirmation.tsx:79

The effect now resets isChecking and isLoadingPreview when it re-runs, but leaves hasFormatterError, hasValidationError and formatterPreview from the previous run:

let cancelled = false;
setIsChecking(true);
setIsLoadingPreview(false);

That partial reset implies a full one. The auto-cancel effect below keys on exactly one of the flags that is not reset:

React.useEffect(() => {
  if (hasFormatterError && !hasValidationError) {
    answerOnce(() => onConfirm(false));
  }
}, [hasFormatterError, hasValidationError, onConfirm, answerOnce]);

So if the instance is reused (the case the sibling answeredRef reset exists to cover, and which the guard resets when a new request reuses the instance exercises) after a formatter crash, the stale hasFormatterError=true satisfies that condition and answerOnce cancels the new tool call on mount — before the user has seen it, which is the same class of blind-decision bug this PR exists to close.

In practice chat-input.tsx keys the component on pendingToolConfirmation.toolCall.id so a queue advance remounts rather than reuses, which is why I rate this important rather than blocking: I could not construct the failing path through the real app's current wiring, only through the instance-reuse fallback the code deliberately keeps alive. Reset hasFormatterError, hasValidationError and formatterPreview alongside the two flags already being reset, so the whole per-run state resets together — or drop the resets you do not need rather than doing half of them.

⚪ nit · completeness · source/components/tool-confirmation.tsx

On the schema-validation and validator-failure fast paths, formatterPreview and hasValidationError are set and loadPreview returns before isChecking is cleared, so for one render the error preview and Checking tool call... are on screen together before the approval prompt appears. It resolves on the immediately following .finally(), so it is cosmetic only — but a one-frame flicker of "Checking tool call..." underneath an already-final validation error is the kind of thing a careful reviewer notices. Clearing isChecking in the same batch as the error state (or deriving the pending label from whether a preview exists) would avoid it.

⚪ nit · tests · source/components/tool-confirmation.spec.tsx

The new tests are genuine regression tests and correctly assert against the old behaviour, which is good. One note on the guard resets when a new request reuses the instance: it now calls await waitForPrompt(app.lastFrame) before the first Enter, which is required by the new gate. That test is also the one place the instance-reuse path is actually exercised — and it only answers the first request with Enter and the second with Escape, so it would not have caught the stale-hasFormatterError auto-cancel described in the other finding. If you close that gap, a test that drives a formatter-crash followed by a reused instance for a healthy call would pin the behaviour.

---

_{🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional}

_{Automated code review — correctness, security, design, tests, plus duplicates and scope. A human still decides; this is not a substitute for review and is not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with /re-review.}

[puri-adityakumar]

Hey, I've pushed the fixes from the review in d42648f and f00b2ff. Could you take a look when you get a chance? The one CI failure is in models-dev-client.spec.ts: models.dev dropped claude-3-opus, so it's not related to this change.