Skip to content

fix(palette): restore composer remount for never-activated pet widgets - #4605

Open
Yeachan-Heo wants to merge 1 commit into
devfrom
fix/issue-4604-command-palette-pet-restore
Open

fix(palette): restore composer remount for never-activated pet widgets#4605
Yeachan-Heo wants to merge 1 commit into
devfrom
fix/issue-4604-command-palette-pet-restore

Conversation

@Yeachan-Heo

Copy link
Copy Markdown
Owner

Summary

Fixes #4604. Restores the palette close/cancel contract broken by #4591: a CommandPaletteComponent (with its DynamicBorder + CommandPaletteList children) leaked in editorContainer whenever a palette overlay closed, because the composer was never re-mounted.

Root cause

  • InteractiveMode.restoreComposer() routes through petWidget.remountComposer() whenever a pet widget exists — which is always after init(), even with pet.mode: "off".
  • fix(pet): stabilize iTerm2 Gajae Pet placement, drag handling, and animation rendering #4591 gated remountComposer() behind #canMutateSharedUi():
    remountComposer(): void {
        if (this.#canMutateSharedUi()) this.#mountEditor(this.#mode !== "off");
    }
    #canMutateSharedUi() admits only (a) the current overlay-emitter owner, or (b) a previously-activated widget whose ownership epoch still matches.
  • A widget that never activated (pet off at startup: #ownedOverlayEpoch === 0, never registered in petOverlayEmitterOwners) satisfies neither clause → remountComposer() becomes a silent no-op → the palette modal stays mounted.
  • Pre-fix(pet): stabilize iTerm2 Gajae Pet placement, drag handling, and animation rendering #4591, remountComposer() mounted unconditionally; for a never-activated widget #mountEditor(false) is byte-identical to the no-pet fallback (editorContainer.clear(); addChild(editor)).

The guard's real purpose — preventing a predecessor retired by a successor from clobbering the successor's composer — only ever applies to widgets that once claimed the overlay slot. Composer-mount authority and overlay ownership are separate concerns; the fix separates them:

remountComposer(): void {
    if (this.#disposed) return;
    const owner = petOverlayEmitterOwners.get(this.#ui);
    const neverClaimedOverlay = this.#ownedOverlayEpoch === 0;
    if (owner === this || (owner === undefined && (neverClaimedOverlay || this.#canMutateSharedUi()))) {
        this.#mountEditor(this.#mode !== "off");
    }
}

This keeps #4591's successor-takeover isolation intact (remountComposer remains a no-op for a widget retired by a successor, covered by the existing "ignores stale off and remount calls after successor takeover" test) and additionally makes the active-pet case remount the framed editor directly rather than relying on the epoch clause alone.

Validation

Gate Result
bun test packages/coding-agent/test/command-palette-interactive-host.test.ts 8 pass / 0 fail (was 3 fail at dev head e1849e6)
bun test packages/coding-agent/test/gajae-pet-widget.test.ts 62 pass / 0 fail (incl. 2 new regression tests)
bun test packages/tui/test/gajae-pet.test.ts 19 pass / 0 fail
bun run --cwd packages/tui check clean (biome + tsc)
bun run --cwd packages/coding-agent check clean
Adjacent suites (selector-controller-command-palette, qa-pet-restore-redteam, interactive-mode-editor-component, hook-editor, model-selector ×2) 104 pass / 0 fail

New regressions pin both ownership states:

  • never-activated widget + foreign overlay in editorContainerremountComposer() restores [editor]
  • active widget + foreign overlay → remountComposer() restores [framedEditor]

Non-goals honored

Closes #4604

#4591 gated GajaePetWidget.remountComposer() behind #canMutateSharedUi(),
which only admits the current overlay owner or a widget whose epoch still
matches. A widget that never activated (pet.mode "off" at startup) claims
neither, so InteractiveMode.restoreComposer() -- which always routes
through petWidget.remountComposer() once init() creates the widget --
silently no-opped. Palette cancel/close paths then left their
CommandPaletteComponent mounted in editorContainer, leaking the modal
(issue #4604, three command-palette-interactive-host failures).

Composer-mount authority and overlay ownership are separate concerns:
a never-activated widget still owns its host's composer mount and must
remount the plain editor exactly like the no-pet fallback, while only
disposal or a live successor widget revokes the mount. The active-owner
case now also remounts the framed editor instead of relying on the
epoch clause alone.

Lore-id: 4604-palette-pet-remount
Constraint: must not weaken #4591 successor-takeover isolation
Tested: bun test command-palette-interactive-host.test.ts (8/8)
Tested: bun test gajae-pet-widget.test.ts (62 pass incl. 2 new regressions)
Tested: bun test packages/tui/test/gajae-pet.test.ts (19 pass)
Not-tested: live iTerm2 pet drag interaction (requires real terminal)
Confidence: high
Scope-risk: narrow
Reversibility: trivial
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@Yeachan-Heo

Copy link
Copy Markdown
Owner Author

Root cause

InteractiveMode.restoreComposer() routes through petWidget.remountComposer() whenever a pet widget exists — which is always after init(), even with pet.mode: "off". #4591 gated remountComposer() behind #canMutateSharedUi(), which admits only (a) the current overlay-emitter owner or (b) a previously-activated widget whose ownership epoch still matches. A widget that never activated (pet off at startup: #ownedOverlayEpoch === 0, never registered in petOverlayEmitterOwners) satisfies neither clause, so remountComposer() became a silent no-op and the palette's CommandPaletteComponent stayed mounted in editorContainer — the exact leak in the three failing tests (palette cancel path, active-command guard path, rejected-extension recovery path).

Pre-#4591, remountComposer() mounted unconditionally; for a never-activated widget #mountEditor(false) is byte-identical to the no-pet fallback (editorContainer.clear(); addChild(editor)). The fix separates composer-mount authority from overlay ownership: a never-activated widget still owns its host's composer mount and must remount the plain editor; only disposal or a live successor widget revokes the mount. #4591's successor-takeover isolation is untouched (still covered by the existing "ignores stale off and remount calls after successor takeover" test), and #canMutateSharedUi() gating in dispose()/#applyMode() is unchanged.

Validation (exact PR head c194e79)

Gate Result
bun test packages/coding-agent/test/command-palette-interactive-host.test.ts 8 pass / 0 fail (was 3 fail at dev head e1849e6)
bun test packages/coding-agent/test/gajae-pet-widget.test.ts 62 pass / 0 fail (incl. 2 new regression tests)
bun test packages/tui/test/gajae-pet.test.ts 19 pass / 0 fail
bun run --cwd packages/tui check clean (biome + tsc)
bun run --cwd packages/coding-agent check clean
Adjacent suites (selector-controller-command-palette, qa-pet-restore-redteam, interactive-mode-editor-component, hook-editor, model-selector ×2) 104 pass / 0 fail

CI on this head (run 31932247745 + 31932247752)

Green: affected-path plan, native-build, test:packages/coding-agent/test/gajae-pet-widget.test.ts, ts-build (coding-agent), gjc-state-gates ×4, Local public surfaces, Virtual integration validation.

Red: PR contract bootstrap — the PR body lacks a gajae.pr-review-verdict.v1 merge-approved verdict line from an independent reviewer with an exact-head approval (governance gate; needs a maintainer review, not a code change). Bounded blocker report filed on #4604.


[repo owner's gaebal-gajae (clawdbot) 🦞]

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.

1 participant