Skip to content

fix(tui): suppress Working loader for blocking custom UI overlays - #1677

Open
flora131 wants to merge 11 commits into
mainfrom
fix/workflow-spinner-overlays
Open

fix(tui): suppress Working loader for blocking custom UI overlays#1677
flora131 wants to merge 11 commits into
mainfrom
fix/workflow-spinner-overlays

Conversation

@flora131

@flora131 flora131 commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes #1670: the host's global Working... spinner kept rendering underneath workflow slash-command overlays that were waiting on user input (e.g. /workflow <name> input forms, the session connect/resume pickers, the kill-confirm dialog), making the TUI look busy when it was actually idle and blocked on a keypress.

Rather than having each overlay manually hide/restore the spinner, the fix centralizes suppression in the interactive host: any non-overlay ctx.ui.custom() mount (the default, or explicit { overlay: false }) is now treated as blocking user input and automatically suppresses Working... for as long as it's mounted, restoring it when the component settles or is dismissed. Floating { overlay: true } custom UIs are left alone since they can be passive views over still-active work (e.g. the kill-confirm modal). The workflows package's overlays are updated to rely on this host behavior instead of duplicating hide/restore logic, and inline-form-overlay.ts is migrated from the legacy setEditorComponent/getEditorComponent editor-swap to mounting through ctx.ui.custom() directly so it participates in the same lifecycle.

Changes

  • interactive-extension-runtime.ts / interactive-extension-context.ts / interactive-agent-events.ts / interactive-mode-surface.ts: Added isWorkingLoaderAllowed() (true only when workingVisible and blockingInlineCustomUiDepth === 0) and refreshWorkingLoaderVisibility(), and wired beginHostInlineCustomUi/setWorkingVisible/the agent-event loader restart path through them so the spinner is suppressed for the full lifetime of any blocking inline custom UI, not just while workingVisible is explicitly toggled off.
  • inline-form-overlay.ts: openInlineInputsForm now mounts the inline form via ctx.ui.custom(factory, { overlay: false }) instead of swapping in a custom editor with setEditorComponent/getEditorComponent, removing the bespoke "was our factory still installed" staleness checks needed by the old swap-and-restore approach (including the /new//resume//fork//reload stale-context case). Working-loader suppression is now handled entirely by the host for the duration of the mount.
  • inputs-overlay.ts: Removed openInputsPicker's own hideWorking/restoreWorking closures and the setWorkingVisible surface requirement — the host now suppresses the spinner for the picker's mounted lifetime.
  • session-overlays.ts: Removed manual Working-visibility handling from openSessionPicker/openKillConfirm. Hardened mount-failure handling so both a synchronous throw and an async rejection from custom(...) resolve to a safe default ({ kind: "close" } / false) instead of leaving the promise unsettled, and added ConfirmUiSurface to the surface types.
  • workflow-resume-selector.ts: Applied the same defensive synchronous/async mount-failure handling to openWorkflowResumeSelector, settling to { kind: "close" } on either failure path.
  • workflow-command-registration.ts: Aligned canOpenPicker's capability gate to only require ctx.ui.custom (dropped the now-dead typeof ctx.ui?.setEditorComponent === "function" fallback check), and always route through openInlineInputsForm rather than gating that call on setEditorComponent support, so custom-only UI hosts correctly reach the inline input form instead of skipping straight to the picker fallback.
  • inline-form-editor.ts: Updated doc comments to describe the component as mounted via non-overlay ctx.ui.custom() rather than swapped in via setEditorComponent.
  • Docs: packages/coding-agent/docs/extensions.md and docs/tui.md document that non-overlay ctx.ui.custom() suppresses the global Working... loader while mounted, and that { overlay: true } opts out.
  • Changelog entries added under [Unreleased] / Fixed in both packages/coding-agent/CHANGELOG.md and packages/workflows/CHANGELOG.md.

Tests

  • packages/coding-agent/test/interactive-mode-status-custom-ui.suite.ts: new coverage asserting the Working loader is stopped for the duration of a non-overlay showExtensionCustom mount and restarted after it settles, and that overlay ({ overlay: true }) mounts do not suppress it.
  • test/unit/overlay-mount-placement.test.ts: new assertions that openSessionPicker/openKillConfirm/openWorkflowResumeSelector never call setWorkingVisible themselves (connect, close, dispose, and mount-failure paths), confirming visibility is left entirely to the host.
  • test/unit/inline-form-04.test.ts, test/unit/inline-form-helpers.ts, test/unit/inputs-picker-01.ts, test/unit/inputs-picker-helpers.ts: updated to mount openInlineInputsForm/openInputsPicker through the ctx.ui.custom() surface instead of the retired setEditorComponent/getEditorComponent mocks.
  • test/integration/mock-extension-api-workflow-actions.test.ts / test/integration/overlay-entrypoints-helpers.ts: new coverage confirming custom-only UI hosts (no setEditorComponent) reach the inline workflow input form instead of skipping to the fallback picker.

Validation

  • AGENT=1 bun test test/unit/inline-form-04.test.ts test/unit/inputs-picker-01.ts test/unit/overlay-mount-placement.test.ts packages/coding-agent/test/interactive-mode-status-custom-ui.suite.ts
  • AGENT=1 bun run typecheck
  • AGENT=1 bun run check:file-length
  • AGENT=1 bun run test:unit

flora131 added 2 commits July 8, 2026 18:09
Hide the global Working row while workflow slash-command overlays are awaiting user input, then restore visibility when they settle or hand off to real work.

Covers inline workflow inputs, session picker/connect, resume selector, and related overlay confirm paths.

Refs: #1670

Assistant-model: GPT-5.5
@claude claude Bot changed the title fix(workflows): hide spinner during workflow overlays fix(workflows): hide Working spinner during workflow overlays Jul 8, 2026
@claude

claude Bot commented Jul 9, 2026

Copy link
Copy Markdown

Review: fix(workflows): hide Working spinner during workflow overlays (#1677)

Solid, defensive fix for #1670. The hide/restore lifecycle is applied consistently across every settle path (submit, cancel, close, dispose, mount-failure, stale-context late settle), the restoreWorking guard makes double-restore a no-op, and the ordering correctly checks ui.custom/setEditor availability before hiding so no path can orphan the spinner in a hidden state. Test coverage is thorough — I traced every resolve() in each of the four functions and each hide is paired with a restore. Nice work.

A few non-blocking observations:

1. Duplicated hideWorking/restoreWorking trio (maintainability)

The identical let workingHidden = false; const hideWorking … const restoreWorking … block now appears five times: inline-form-overlay.ts, session-overlays.ts (x2), workflow-resume-selector.ts, plus the pre-existing inputs-overlay.ts. A tiny shared helper would DRY this and centralize the semantics:

```ts
export function createWorkingVisibilityGuard(ui: { setWorkingVisible?: (v: boolean) => void }) {
let hidden = false;
return {
hide() { try { ui.setWorkingVisible?.(false); hidden = true; } catch {} },
restore() { if (!hidden) return; hidden = false; try { ui.setWorkingVisible?.(true); } catch {} },
};
}
```

Worth considering as a follow-up given the pattern is now load-bearing in 5 spots.

2. Inconsistent try/catch around setWorkingVisible

inline-form-overlay.ts wraps both setWorkingVisible calls in try/catch (for stale-ctx safety), but session-overlays.ts and workflow-resume-selector.ts call ui.setWorkingVisible?.(…) bare. If a host's setWorkingVisible threw in those paths it would propagate out of the settle path. In practice the session/resume overlays hold ui directly (not a stale-able ctx.ui getter) so this is likely fine — but the divergence is subtle. Folding everything into the shared guard above would make the guarantee uniform.

3. restoreWorking always sets true rather than restoring prior state

restoreWorking() unconditionally calls setWorkingVisible(true) — it assumes Working should be visible after the overlay. Since the host only renders the loader while streaming (interactive-extension-runtime.ts gates on session.isStreaming), this is benign today and matches the existing inputs-overlay.ts prior art. It's an implicit assumption though (there's no getWorkingVisible to snapshot) — a one-line comment noting "restore = re-enable; host suppresses the loader when not streaming" would help the next reader.

4. Nested overlays cause a transient true→false flip (cosmetic)

/workflow kill runs openSessionPicker then openKillConfirm sequentially (workflow-run-control-command.ts:146,162). The picker's restoreWorking() fires setWorkingVisible(true) (+requestRender) immediately before the confirm's hideWorking() fires setWorkingVisible(false). Harmless (no spinner unless streaming), just an extra render pair. A guard shared across the two calls would avoid it; not worth restructuring for on its own.

Nits

  • finish/settle's try { done(undefined) } finally { restoreWorking(); resolve() }: if done throws, restore+resolve still run (an improvement over the prior done(); resolve()), but the error still propagates out of the input handler. Pre-existing behavior; just flagging.
  • Changelog entry, file-length, and type plumbing (setWorkingVisible? optional on PiUISurface/UiSurface/ConfirmUiSurface) all check out.

None of these block merge — they're refactor/clarity suggestions. The correctness of the actual fix is sound.

Note: I was unable to execute bun test/bun run typecheck in this review environment (command approval gated), so I relied on static tracing plus the validation documented in the PR description.

@claude

claude Bot commented Jul 9, 2026

Copy link
Copy Markdown

Review: fix(workflows): hide Working spinner during workflow overlays

Nice, focused fix for #1670. The extraction of createWorkingVisibilityGuard cleanly de-duplicates the hide/restore lifecycle that was previously inlined in inputs-overlay.ts, and it is now applied consistently across the inline form, session picker, kill confirm, and resume selector. Test coverage is genuinely thorough — every settle path (submit/cancel/close/dispose/stale-context/mount-failure/fallback confirm) asserts the exact [false, true] call sequence.

What is solid

  • Idempotent restore. The hidden flag guards against double-restore and against restoring when hide() never ran (the "ui.custom absent -> no setWorkingVisible calls" test locks this in).
  • Error containment. Both hide/restore swallow host throws, and mount is wrapped in Promise.resolve(custom(...)).catch() plus a synchronous try/catch, so both sync-throw and async-rejection mount failures still restore the spinner and resolve the promise. Good belt-and-suspenders.
  • No flicker in the /workflow kill picker->confirm chain. The picker restore fires before openKillConfirm hides again, but since the host setWorkingVisible(true) only re-creates the loader when session.isStreaming (interactive-extension-runtime.ts:112), the intermediate true is a no-op. The guard doc comment correctly calls this out.
  • .call(ui, ...) preserves this binding, matching the existing getEditor.call(ui) convention. File-length gate and changelog placement ([Unreleased] / Fixed) both fine.

Minor nits (non-blocking)

  1. hide() flips hidden = true even when setWorkingVisible is undefined (working-visibility-guard.ts) — the optional-chaining call is a no-op but the flag still flips, so the guard reports "hidden" without having touched anything. Harmless because restore() is then also a no-op; purely cosmetic.
  2. openInputsPicker refactor has no new dedicated working-visibility test. Behavior-preserving (swaps the inlined closure for the shared helper, still exercised in overlay-mount-placement.test.ts), so fine — just flagging that its spinner lifecycle now rides entirely on the shared-guard tests.

One thing to confirm

  • I could not run bun test / bun run typecheck in this environment. The PR body reports these green locally; please ensure CI typecheck passes given inputs-overlay.ts dropped its local workingHidden/hideWorking/restoreWorking bindings — a stale reference would surface there.

Overall this is a clean, well-tested, correctly-scoped fix. LGTM pending green CI.

@claude

claude Bot commented Jul 9, 2026

Copy link
Copy Markdown

Code Review — fix(workflows): hide Working spinner during workflow overlays

Reviewed against CLAUDE.md. Overall this is a high-quality, defensive fix with excellent test coverage. The refactor into a shared createWorkingVisibilityGuard is the right call, and the idempotent hide()/restore() (guarded by the hidden flag) neatly prevents double-toggles when both finish() and dispose() fire. Nice work.

Things done well

  • Correct host semantics. Restoring to setWorkingVisible(true) unconditionally is safe because the interactive host only re-creates the loader if (this.session.isStreaming && !this.loadingAnimation) (interactive-extension-runtime.ts:112). The guard's doc comment ("restore means allow Working to render again … the host still suppresses the loader when the session is not streaming") accurately captures this. It also matches the existing convention in ask-user-question.ts:98/113, so the change is consistent with the codebase.
  • Exhaustive teardown coverage. Every settle path — submit, cancel, close, dispose, editor-swap failure, sendMessage failure, stale-ctx late settle, and both sync-throw and async-reject mount failures — restores visibility. done() is wrapped in try/finally so a throwing host still restores.
  • Backward compatible. setWorkingVisible? is optional on all surface types and the guard no-ops when absent, so existing callers and older hosts are unaffected.
  • Strong tests — the [false, true] call-sequence assertions across all lifecycle paths, plus the isolated guard test (no-op + exception-swallowing on both paths), are exactly what's needed.

Minor suggestions (non-blocking)

  1. Inconsistent hide-before-guard ordering across overlays. In inputs-overlay.ts:96 hideWorking() is called before the custom presence check (line 98), so the no-custom fallback produces a redundant false → true toggle. But openSessionPicker / openWorkflowResumeSelector check custom first and never touch visibility when it's absent — there's even a test asserting exactly that (overlay-mount-placement.test.ts: "does not touch working visibility when ui.custom is absent"). The toggle is harmless (host no-ops when not streaming), but aligning inputs-overlay to check custom first would make the four overlays behave identically and match the tested contract.

  2. @ts-nocheck in inline-form-working-visibility.test.ts. CLAUDE.md pushes against any/unknown and unchecked TS. The sibling additions in inline-form-04.test.ts are fully typed with as never casts; consider dropping the blanket @ts-nocheck for consistency with the rest of the suite.

  3. Pre-existing, not introduced here: the sendMessage-failure path in inline-form-overlay.ts:322-329 resolves the promise but never sets resolved = true (unlike settle()). Harmless today because both restoreWorking() and resolve() are idempotent, but it's a latent inconsistency worth a follow-up if a later editor onExit could still fire after this branch.

Verification

I could not execute bun test/typecheck in this review sandbox (command execution was restricted), so I relied on static analysis. The PR description documents that bun run typecheck, check:file-length, and the affected unit suites pass — worth a maintainer confirming CI is green. All touched files are well under the 500-line gate (working-visibility-guard.ts is 44 lines, session-overlays.ts 302).

Nothing here is blocking — the suggestions are polish. LGTM. ✅

@flora131

flora131 commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author

Implementation direction I want to switch toward:

Instead of having each workflow overlay manually hide/restore the global Working... indicator, centralize this in the host/runtime layer.

The core issue is a UI-state mismatch: after the eager spinner change, Working... can appear immediately after command submit, but workflow commands may then open a blocking form/picker and wait for human input. While a blocking custom UI overlay is active, the app is not really “working”; it is waiting on the user.

Preferred approach:

  • Treat blocking custom UI overlays as a host-level “waiting for user input” state.
  • Suppress the global Working... indicator automatically while such an overlay is mounted.
  • Restore normal spinner behavior when the overlay settles/disposes.
  • Remove per-overlay setWorkingVisible(false/true) calls and the working-visibility-guard.ts helper.
  • Keep focused regression tests for the original cases (/workflow <name> input form and /workflow connect picker) plus submit/cancel/dispose restoration.

This keeps ownership in the right layer: overlays should not need to know about global spinner state. The current helper-based solution is safe and pragmatic, but it spreads global UI-state responsibility into individual overlay implementations. Central suppression should be cleaner and less error-prone for future overlays.

@claude

claude Bot commented Jul 10, 2026

Copy link
Copy Markdown

Review: fix(workflows): hide Working spinner during workflow overlays

Solid, well-scoped fix for #1670. The central idea — extracting the hide/restore lifecycle into createWorkingVisibilityGuard and reusing it across every overlay — is the right call, and the coverage of exit paths (submit, cancel, dispose, close, mount-failure, stale-context late settle) is thorough. A few observations, mostly minor.

Strengths

  • Idempotent guard. restore() no-ops unless hide() actually succeeded (hidden is only set to true inside the try after setWorkingVisible returns). This avoids re-showing the spinner if hiding never took effect, and makes the many restoreWorking() call sites safe against double-invocation.
  • Ordering is careful. In inline-form-overlay.ts, openWorkflowResumeSelector, and openSessionPicker, the early "unsupported"/"close" returns (empty fields, missing custom, missing setEditor) all happen before hideWorking(), so the spinner is never suppressed on a no-mount path. The "does not touch working visibility when ui.custom is absent" test locks this in.
  • Mount failure is genuinely hardened. Wrapping custom(...) in Promise.resolve(...).catch() and a synchronous try/catch covers both a sync throw and an async rejection, resolving to a safe default instead of leaving the promise unsettled and the spinner stuck hidden.

Suggestions

  1. Duplicated mount-safety boilerplate (DRY). The "settle to a safe default on either a synchronous throw or an async rejection from custom(...)" block is now repeated three times nearly verbatim (openSessionPicker, openKillConfirm, openWorkflowResumeSelector):

    try {
      void Promise.resolve(custom(factory, opts)).catch(() => { /* settle safe */ });
    } catch { /* settle safe */ }

    Given this PR's own thesis — factor the shared lifecycle into one guard — it would be consistent to extract a small mountOverlaySafely(custom, factory, opts, onFailure) helper alongside working-visibility-guard.ts. That removes ~24 lines and gives the fallback one tested home.

  2. Unguarded synchronous window in openInlineInputsForm. After hideWorking(), the seed-state code (makeFormId(), store seeding, finalizeForm wiring) runs outside any try/catch before new Promise(...). The getEditor lookup is correctly guarded, but if any of that seed code threw synchronously, the function would propagate the error with the spinner left hidden and no restoreWorking(). Unlikely (and pre-existing), but since the whole point is "never leave Working suppressed," consider moving hideWorking() to just before the mount, or wrapping the body so a synchronous throw restores. Low priority.

  3. Minor test gap. The fallback-confirm rejection branch in openKillConfirm (the () => { restoreWorking(); resolve(false); } arm) restores working, but only the resolve arm is asserted. A one-line case with a rejecting confirm would close the last uncovered restore path.

Correctness / bugs

No blocking issues found. The try { done(...) } finally { restoreWorking(); resolve(...) } shape in the finish/settle helpers correctly guarantees restore+resolve even if done() throws, while still letting the host see the error. Changelog entry is present and accurately scoped under [Unreleased] / Fixed, per CLAUDE.md conventions.

Nice work overall — the fix is defensive in the right places and the test matrix is thorough. Suggestions 1–3 are all optional polish.

— automated review

@claude

claude Bot commented Jul 10, 2026

Copy link
Copy Markdown

Review: fix(workflows): hide Working spinner during workflow overlays

Thanks for this — it's a well-scoped, thoroughly-tested fix for #1670. The extraction of createWorkingVisibilityGuard is a clean way to de-duplicate the hide/restore lifecycle, and the exit-path coverage is genuinely exhaustive (submit, cancel, dispose, close, editor-swap failure, sendMessage failure, stale-ctx late settle, and both synchronous-throw and async-rejection mount failures). Nice work.

Strengths

  • Correct restore semantics. restore() calling setWorkingVisible(true) is a re-enable, not a force-show, and the guard doc comment makes that explicit ("The host still suppresses the loader when the session is not streaming"). That's the right model — the run/connect path correctly lets the spinner reappear once real streaming work begins.
  • Defensive hide() bookkeeping. hidden is only set to true after a successful setWorkingVisible(false), so a host that throws on hide won't cause restore() to spuriously re-enable something that was never disabled. Symmetric and idempotent — verified by the repeated restore()/double-hide() guard test.
  • Both mount-failure surfaces covered. Wrapping custom(...) in Promise.resolve(...).catch(...) and a synchronous try/catch handles both a throwing custom and a rejecting promise, each settling to a safe default rather than leaking a suppressed spinner or an unsettled promise.
  • All touched files stay under the 500-line gate (session-overlays.ts 301, inline-form-overlay.ts 341); changelog entry is correctly placed under [Unreleased] / Fixed with the issue link.

Minor points (non-blocking)

  1. Sequential picker → kill-confirm toggle. In workflow-run-control-command.ts:160-176, a connect-intent picker that resolves with kind: "kill" runs restoreWorking() (→ setWorkingVisible(true)) and is immediately followed by openKillConfirm, which calls hideWorking() (→ false). There's a transient truefalse toggle across the handoff. In practice there's no render between the two (synchronous continuation after the await), so it's likely invisible, but worth confirming the host coalesces visibility writes so no spinner flash sneaks in between overlays.

  2. Full-screen graph attach pane not covered. overlay-adapter.ts:396 mounts via void custom(factory, options) without the guard. This looks intentional — it's an overlay: true full-screen pane over actively-running work, not an idle input-wait — but a one-line note confirming it's deliberately out of scope for Workflow input/connect UI incorrectly shows Working spinner #1670 would help future readers.

  3. Mount-fallback boilerplate is duplicated. The try { void Promise.resolve(custom(...)).catch(settleToDefault) } catch { settleToDefault() } shape now appears in session-overlays.ts (twice) and workflow-resume-selector.ts. If it grows a fourth copy, consider folding it into a small shared mountWithFallback(...) helper alongside the guard. Fine as-is for now.

Tests

Coverage is strong across all lifecycle paths and both failure modes. I couldn't execute bun test in this review sandbox (command approval), but the PR reports green on the four affected suites plus typecheck/check:file-length/test:unit, and the assertions read correctly (workingCalls consistently [false, true] on settle, [] when no overlay mounts). Small suggestion: an explicit assertion that the guard is not touched on the no-mount short-circuit in openInlineInputsForm (empty-fields → { kind: "run" }) would mirror the nice openSessionPicker does not touch working visibility when ui.custom is absent test.

Overall this is solid and ready to merge once the picker→confirm toggle (point 1) is confirmed harmless. 👍

@claude claude Bot changed the title fix(workflows): hide Working spinner during workflow overlays fix(tui): suppress Working loader for blocking custom UI overlays Jul 10, 2026
@claude

claude Bot commented Jul 10, 2026

Copy link
Copy Markdown

Review: fix(workflows): hide Working spinner during workflow overlays

Nice, well-scoped fix. Moving spinner suppression into the host (auto-suppress while a non-overlay ctx.ui.custom() is mounted) is the right layering — it removes the fragile per-overlay setWorkingVisible bookkeeping and gives every blocking custom UI consistent behavior. The defensive touches are genuinely good: resetExtensionUI now zeroing blockingInlineCustomUiDepth (prevents a leaked overlay from permanently suppressing the loader), the idempotent finalizeIfNeeded, and try/catch guards around every custom() mount. Lifecycle test coverage is thorough.

A few things worth addressing before merge.

🔴 Stale capability gate — inline form now depends on custom, but is still gated on setEditorComponent

packages/workflows/src/extension/workflow-command-registration.ts:191

let pickerResult = typeof ctx.ui?.setEditorComponent === "function"
  ? await openInlineInputsForm(pi, ctx, { workflowName, fields, prefilled: inputs, theme: pickerTheme })
  : { kind: "unsupported" as const };
if (pickerResult.kind === "unsupported" && typeof ctx.ui?.custom === "function") {
  pickerResult = await openInputsPicker(ctx.ui, { ... });
}

This PR rewrites openInlineInputsForm to use ctx.ui.custom (setEditorComponent/getEditorComponent are gone from it — it returns unsupported unless custom is a function), but the gate on line 191 still keys off setEditorComponent. The handler receives the raw host ctx here (handler: (args, ctx) => workflowSlashHandler(...)), not the wiring.ts-normalized surface, so the two capabilities can genuinely diverge:

  • Host with custom but not setEditorComponent: line 191 is false → the inline form is silently skipped and the user gets the fallback openInputsPicker instead of the intended inline form — even though canOpenPicker (line 176) explicitly admits custom-only hosts.
  • Host with setEditorComponent but not custom: line 191 is true → openInlineInputsForm immediately returns unsupported (no custom), then line 194's fallback also requires customno picker shown at all, so the workflow proceeds with missing required inputs. Previously this host would have shown the inline form successfully.

Atomic's host exposes both surfaces so this is latent today, but the gate no longer expresses the real requirement. Suggest gating the inline form on ctx.ui?.custom to match the refactor (and revisiting the setEditorComponent branch in canOpenPicker on line 176).

🟡 Cross-host portability of the suppression

Removing the manual ui.setWorkingVisible?.(false) toggles means spinner suppression now relies entirely on the host's new auto-suppress-on-non-overlay-custom behavior. That's great on Atomic, but packages/workflows is also loadable on upstream pi (per CLAUDE.md tip #1, dev symlink into ~/.pi/agent/extensions), where the host has no such behavior — so the Working… spinner will re-appear under these overlays there. Intentional per the changelog, but worth a comment in the overlay headers so the coupling to the host feature isn't rediscovered the hard way.

🟡 Behavior changes in inline-form-overlay.ts — confirm intended

  • If the host resolves custom() with undefined (e.g. teardown/dispose without onExit firing), the result is now {kind:"unsupported"} rather than the previous {kind:"cancel"}. Both abort the command, so low impact, but it's a semantic shift from the old stale-context handling.
  • When sendMessage succeeds but the custom() mount then fails, the card is now emitted and frozen as cancelled (previously no card was emitted). The updated test asserts this — just flagging it's a deliberate UX change.

Nits

  • Extra blank lines introduced: session-overlays.ts (double blank after the fallback-confirm block, ~line 209) and workflow-resume-selector.ts (double blank ~line 105). Trim for consistency.
  • Test gap: there's no coverage at the workflow-command-registration layer for a custom-only host actually reaching openInlineInputsForm — a test there would have caught the gate mismatch above.

Overall the host-side approach is a solid improvement; the gate mismatch is the main item I'd want fixed.

@claude

claude Bot commented Jul 10, 2026

Copy link
Copy Markdown

Code Review — fix(tui): suppress Working loader for blocking custom UI overlays

Reviewed the full diff plus the host-side plumbing in interactive-extension-custom-ui.ts / interactive-extension-context.ts. Overall this is a clean, well-motivated refactor: centralizing loader suppression in the host and deleting the per-overlay hideWorking/restoreWorking (and the whole setEditorComponent swap-and-restore machinery) is the right call, and it removes a real class of duplicated/racey code. Nice work, and the test coverage is genuinely thorough.

👍 Strengths

  • Right abstraction. Gating on blockingInlineCustomUiDepth === 0 via isWorkingLoaderAllowed() means suppression tracks the lifetime of the mount rather than an explicit toggle — this is what actually fixes Workflow input/connect UI incorrectly shows Working spinner #1670, and it composes across nested/overlapping mounts thanks to the depth counter + Math.max(0, …) underflow guard.
  • Defensive mount handling is consistent. Wrapping every custom(...) in both a synchronous try/catch and an async .catch() so both failure paths settle the promise ({kind:"close"} / false / unsupported) is a solid robustness improvement — the old void custom(...) could leave promises unsettled on a synchronous throw.
  • try { done() } finally { resolve() } ordering in the settle helpers is a good touch — unmount happens before the awaiter proceeds, and a throwing done can't strand the promise.
  • Docs (extensions.md, tui.md) and both changelogs are updated, and the overlay-vs-non-overlay opt-out semantics are clearly documented.

🔎 Questions / possible edge cases

  1. Teardown while an inline form is still mounted (previously the "stale ctx" path). The old openInlineInputsForm had explicit handling for /new /resume /fork /reload marking the command ctx stale (This extension ctx is stale) before the form settled. That logic is now gone — the comment says it's obviated because the host owns editor restore, which is true for the restore concern. But note resetExtensionUI() now force-resets blockingInlineCustomUiDepth = 0 and calls refreshWorkingLoaderVisibility(). If a blocking {overlay:false} custom UI is still mounted/awaited when resetExtensionUI runs, does the host reject the pending custom() promise? If not, openInlineInputsForm's await custom.call(...) could hang (the awaiting workflowSlashHandler never resolves), and the depth reset could briefly un-suppress the loader under a still-visible surface. Worth confirming the session-lifecycle path rejects/settles in-flight custom UIs. (Low-med confidence — may be a non-issue if teardown always aborts pending mounts.)

  2. custom() rejection → unsupported → fallback double-surface. On a mount rejection/abort, openInlineInputsForm freezes the form card as cancelled and returns {kind:"unsupported"}, which makes workflowSlashHandler fall back to openInputsPicker (a second custom() mount). In an abort-mid-fill scenario the user could end up with a frozen cancelled card in scrollback and a fresh fallback picker. Intentional per the doc comment, but confirm the abort case (vs genuine "host can't mount") doesn't produce a confusing double-surface.

  3. Re-enable only restarts on session.isStreaming. refreshWorkingLoaderVisibility() recreates the loader on release only when this.session.isStreaming. If a blocking UI opens during preflight (before agent_start, while showWorkingLoaderNow() had created the pre-stream spinner) and closes before streaming begins, the pre-stream spinner won't come back until agent_start. This matches the old setWorkingVisible(true) behavior so it's not a regression — just flagging it isn't a full restoration of pre-stream state.

Nits

  • interactive-mode-status-custom-ui.suite.ts imports from vitest while the repo standard is bun:test — consistent with the pre-existing file in that package, so fine, just noting the divergence.

None of the above are blocking; (1) is the one I'd most want a second look at before merge. Everything else reads as a solid, well-tested cleanup.

Reviewed with Claude Code.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Workflow input/connect UI incorrectly shows Working spinner

1 participant