Repository navigation
Conversation
![opencodejr[bot]](https://avatars.githubusercontent.com/in/5497873?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix looks right for the detached case. On the base, the new component test fails (none instead of Ctrl+Alt+B); with this change it passes. The nearby browser component tests (44) and the gui-plugins browser tests (46) pass, and tsc -b passes in packages/gui-plugins. I didn't run the desktop app. Reading the code, the launcher's open sends a command through the pane's connection, which calls register() first, so clicking it reconnects a suspended or reopening pane. The launcher's gate now also matches the browser.open shortcut, which only checked available.
Public API: unchanged.
One small point (inline): the launcher now also shows after a pane is replaced (browser.pane.replaced), where clicking it can't reconnect. The description says that path is unchanged.
Also, #60023 lists other ways the launcher stays hidden that this doesn't touch: the unsupported flag that sticks after a transient rpc.unavailable, and the replaced state. Closes #60023 may close it before those are handled.
| // Only once the desktop's pane answers. | ||
| if (!view || !value?.available(view) || !value.attached(view)) return undefined | ||
| // Opening the launcher can wake or reconnect a pane that is available but not attached. | ||
| if (!view || !value?.available(view)) return undefined |
There was a problem hiding this comment.
Dropping attached also shows the launcher after browser.pane.replaced, not only for suspended or reopening panes. In that state the connection is blocked, so register() returns early. command() then rejects with browser.pane.unavailable without the unavailable code. model.command first clears the attachment's replaced error, then sets the error to "Request failed", so a click does nothing useful and the explanation is lost. The ⌃⌘B command already behaves this way, so this isn't a new path, but the description says the replaced path is unchanged. Could you either keep the launcher hidden when the attachment's error is browser.pane.replaced, or confirm the new behaviour there is intended?
There was a problem hiding this comment.
The launcher now stays hidden after browser.pane.replaced, with a new component test covering that state. The issue link is now Related #60023 because the unsupported path remains out of scope.
There was a problem hiding this comment.
The new commit addresses my earlier point: the launcher is now hidden once the pane has been replaced. browser-pane-restore.spec.ts (16) and the gui-plugins browser tests (46) pass, and tsc -b passes in packages/gui-plugins. I didn't run the desktop app.
Public API: unchanged.
One new point, inline: the check compares the error message the user sees with the translated "replaced" text, and that message can be replaced by another one.
| // Only once the desktop's pane answers. | ||
| if (!view || !value?.available(view) || !value.attached(view)) return undefined | ||
| // Opening the launcher can wake or reconnect a pane that is available but not attached. | ||
| if (!view || !value?.available(view) || value.error(view) === ctx.t("replaced")) return undefined |
There was a problem hiding this comment.
value.error(view) returns the text shown to the user (state.errors[key] ?? attachment.error), so this check compares against a translated message. That text can change:
- On a replaced pane, pressing ⌃⌘B (
browser.openis still enabled there) runsmodel.command. - That clears the attachment's
replacederror and, when the connection rejects, setserrors[key]to "Request failed". - After that,
error(view) !== ctx.t("replaced"), so the launcher reappears and clicking it does nothing.
Could the model expose a dedicated check instead, e.g. replaced(session) based on the raw browser.pane.replaced error or the connection being blocked? Then this launcher and the browser.open command's enabled could both use it.
There was a problem hiding this comment.
Implemented a dedicated replaced(session) check derived from the raw browser.pane.replaced event. Both the launcher and browser.open now use it, so a later request error cannot make the launcher reappear. The component test now presses the shortcut after replacement and verifies the launcher stays hidden. Verified: 16 component tests, 46 browser tests, and tsc -b.
There was a problem hiding this comment.
Both earlier points are addressed. The model now has its own replaced flag, set from the raw browser.pane.replaced error, and the launcher and the browser.open command both use it. model.command only clears the displayed error, so a later "Request failed" no longer brings the launcher back. The browser-pane component specs (45) and the gui-plugins browser tests (46) pass, and tsc -b passes. I didn't run the desktop app.
Public API: unchanged.
No further concerns.
Issue for this PR
Related #60023
Type of change
What does this PR do?
Keeps the new-tab Browser launcher available while its pane is detached. Opening the launcher can wake or reconnect the pane, so requiring an existing attachment prevented recovery. Replaced attachments remain hidden because they cannot reconnect; the unsupported path is unchanged.
How did you verify your code works?
noneinstead ofCtrl+Alt+B) and passes after it. A second test keeps the launcher hidden after pane replacement.browser-pane-restore.spec.ts: 16 passed.browser-pane.spec.ts: 29 passed.packages/gui-pluginstests: 241 passed.packages/gui-pluginstypecheck: passed.packages/app/src/custom-elements.d.ts, a symlink whose target text is parsed as TypeScript; the affected package typecheck passes.Screenshots / recordings
Before:
After:
Checklist
Added by the opencode bot (AI-generated).
Related: #60023