feat(desktop): add a Developer setting to stop starting a local gateway - #2917
Conversation
Design Review (Fable 5, fork) — ✅ PASSAdvisory design-level review of Design-Verdict: PASS Sound, well-scoped intent switch at the single decision point; the design's own limits are honestly stated and deferred correctly — except one tracking slip. Watch
[DESIGN-REVIEWED] 09ca90b |
GPT 5.6 Review (fork) — ✅ no blocking findingsReviewed Review detailsNo findings. |
UX Review (Fable 5, fork) — 🟡 CONCERNSAdvisory UX-level review of UX-Verdict: CONCERNS The recovery dialog's "Start Local Gateway" silently re-enables the setting permanently, and the palette deep-links browser users to a toggle that isn't rendered. Watch
Suggestions
[UX-REVIEWED] 09ca90b |
15b99c2 to
b68cf54
Compare
The desktop app decides whether to start its own gateway by probing the port, not by asking what the user wants: a configured remote host only changes how an ALREADY-occupied port is classified. So a client-only install gets a local gateway whenever its connection is not up yet at launch — a Python backend plus a kiro-cli process per session on a machine that is only drawing the UI — and the tab keeps showing the remote's name, because that name comes from config rather than from whatever is answering. Add `runLocalGateway` (default on) to the app's electron-store config, exposed as a Settings > Developer toggle over a new preload bridge. When it is off and nothing is answering, startGateway reports a fail-fast reason instead of spawning, so the existing error dialog names the port and the setting and its Retry re-enters startGateway once the connection is up. Both branches of startGateway funnel through one guard, so a takeover that frees the port by quitting the other channel's app does not start a gateway here either.
b68cf54 to
09ca90b
Compare
|
Both findings on
Thanks — the first one would have been a nasty field report. |
|
Re: UX Review 🟡 CONCERNS — two points, two different dispositions. 1. Circular recovery path: FIXED in Both halves are fixed:
2. Palette advertises the toggle where it does not exist: REBUTTED (pre-existing, not introduced here). |
|
Re: Design Review 🟡 CONCERNS — ACCEPTED, and the PR body was overstating. Corrected in You are right on both counts, and the second one is a fair hit on my description rather than on the code. The framing was wrong. I led with the silent-fallback/misnamed-tab harm as the sharper problem, then shipped a change that leaves it intact in the default state. With On the general fix: accepted-and-deferred, not dismissed. Making the UI reflect the actual responder rather than the configured name is the right answer, and it is a different change from this one. It is recorded on #2909 as the remaining half rather than left implicit, so it does not get lost when this merges. On your suggestion specifically — flagging the tab when the configured remote name does not match what answered — that is the shape I would build, and it also happens to be the cheapest version, since it needs one probe at connect time rather than continuous monitoring. Noting it there. |
Opus 4.8 Review (fork) — ✅ no blocking findingsReviewed Review detailsThe single candidate concerns the
It dies under falsification. No findings. [OPUS-REVIEWED] 09ca90b |
Open PR relationship auditThis is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion. Relationship findings
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
Closes #2909
Problem
The desktop app starts a gateway of its own whenever nothing is answering on its port, even when it is configured to use a remote one and the user has no interest in running a backend locally. There is no setting for it:
startGateway()probes the port and nothing else, and a configured remote host only changes how an already-occupied port is classified (resolveGatewayConflicttreats the holder as non-local and reuses it). Whether you end up local or remote therefore depends on whether your connection happened to be up at launch, not on anything you chose.Two things follow from that, and the second one is the sharper problem:
kiro-cliprocess per session, sub-agents and terminal shells included.customName, thenremoteHosts[port].defaultName, then[:port]— so a named tab normally tells you which machine you are on. But that name comes from config, not from whatever is answering right now. When the remote is unreachable at launch and a local backend is started on the same port, the tab still shows the remote's name, and the app is quietly working against the local data home instead.applyTitle()has no way to know.Why it matters
Running the gateway remotely is a supported, documented setup, and on a laptop the local backend is a cost paid for nothing. The silent fallback is worse than the waste: a user can be working against a different data home than the one their tab names, with no signal anywhere in the UI.
Scope, stated plainly: with the default on, that misnamed-tab fallback still happens for anyone who never finds this switch. This PR makes the harm opt-out rather than unavoidable — it does not solve it. The general fix is to make the tab reflect what actually answered on the port rather than what config says should have, which means probing the live responder and is a separate change; #2909 carries it.
Fix
Symptom — a client-only install gets an unwanted local gateway, and the tab keeps naming the remote.
Root cause — the decision to spawn is derived from a port probe, and no user intent is ever consulted.
Change — make the intent explicit and read it at the one place the decision is made.
runLocalGateway(default on, so nothing changes for existing installs) joins the app'selectron-storeconfig, withlocal-gateway.jsholding the read/write rules: only an explicitfalsedisables it, so a hand-edited config carrying"false"or0cannot silently stop the gateway starting.localGatewayAPIpreload bridge plus twoipcMain.handlecalls expose it to the renderer, followingzoomAPI: the setting lives in the app's own config, which page JS cannot reach, and a missing bridge means the control is not rendered. That is what keeps it invisible in a plain browser and the PWA, where there is no local gateway to manage.startGateway()now funnel through one guard, so the choice cannot be honoured on one path and ignored on the other — including a takeover, where quitting the other channel's app frees the port on this machine but is not a request to run a gateway here.describeGatewayFailuregives it wording that names both the port and the setting, andshowLoadingThenConnectgives it its own dialog title instead of "gateway failed to start" — nothing failed, so sending the user hunting a crash log would be wrong. The dialog's existing Retry re-entersstartGateway(), which is what makes "bring the connection up, then retry" work without a relaunch.The setting takes effect on the next launch, stated under the switch, and is read once at launch into a session snapshot. That matters because
startGateway()is also the recovery path for a gateway that died mid-session: re-reading the store there would let a flip made minutes ago refuse to replace a gateway the session is still using, stranding the user with no backend. The one thing that lifts the snapshot is the user explicitly asking for a gateway from the error dialog.Recovery without a dashboard. The dialog cannot tell the user to open Settings, because Settings is served by a gateway — the thing that is not running. So the client-only variant carries a
Start Local Gatewaybutton that persists the setting, lifts the session snapshot and retries. Retry stays the primary action, since restoring the remote is the likelier fix.Failure classification.
classifyStartFailure()decides which story the dialog tells, and puts the client-only case ahead of the port-conflict probe. The launch log persists across launches, so an "address already in use" line left by an earlier run would otherwise offer to force-stop the holder of a port that is empty.Placement. The toggle starts in Settings › Developer because that panel already exists and is where advanced switches live, so this needs no new Settings section and no decision about naming one. It is not the setting's permanent home — running everything remotely is a deployment choice, not a developer activity — but moving a toggle once there is a second setting to sit beside it is cheap, and it is called out in the component's own docstring so the placement is not mistaken for a claim.
Tests
electron/test/local-gateway.test.js(new) — the default-on rule, that only an explicitfalsedisables it, that a non-boolean stored value ("false",0,null,"") leaves the gateway enabled, and that writes are coerced to a real boolean. PlusclassifyStartFailure: the client-only case outranks a stale port-in-use log line, a genuine conflict still wins when nothing is disabled, a bound port on another window's port is not our conflict, and spawn-failure vs timeout stay distinct.electron/test/gateway-wait.test.js— the disabled failure message names the port and both ways out, does not send the user to Settings (unreachable in that state), avoids launch-failure wording, and wins over a staleerrorfield on the same record.src/test/useLocalGateway.test.ts(new) — reports unsupported and stays inert with no bridge, reads the stored value on mount, writes through, keeps the value the bridge returns rather than the one requested, and survives a rejecting bridge.src/test/DeveloperPanel.test.tsx— the Gateway section is absent without the bridge and renders and writes through with it.Each of these was mutation-checked: the behaviour it covers was broken in turn and the test confirmed to fail. That includes the new precedence rule — flipping the two branches in
classifyStartFailurefails exactly one test.Not covered: the glue inside
startGateway()and the dialog markup itself, sincemain.jsis not unit-testable. Both decisions they delegate to —isLocalGatewayEnabledandclassifyStartFailure— are covered.Manual verification
Local gates, all green — per CONTRIBUTING.md's documented commands:
cd website && npm run check(typecheck +eslint --max-warnings 1116+ vitest + electron): 994 files / 16,328 passed, 2 expected-fail, 3 skipped; 869/869 electron.npm run i18n:checkwithI18N_BASE_REFset — every lane ok, including the DNT catalog check across all 11 translated catalogs.npx jscpd .— 0 clones../scripts/scrub-lint.sh --no-history— passed.scripts/check_brand_name.py— passed.No backend files are touched, so the Python gates do not apply.
Two pre-existing guards caught real defects during development, both fixed here:
shell-contract.test.jsflagged thatlocal-gateway.jswas missing from the electron-builder allowlist (it would have worked from source and been absent from the packaged DMG), and the settings-registry anti-stale guard required regeneratingsettingsRegistry.gen.ts— which also makes the toggle reachable from the command palette asdeveloper.run-a-local-gateway.The rendered surface is confirmed on macOS (see Screenshots): the branch's Electron shell against a pod serving this branch's frontend shows the Gateway section with the toggle in its default-on state.
Still unverified: the full off-path — turning the switch off, relaunching with the remote unreachable, confirming the new dialog, then restoring the connection and hitting Retry. That needs a packaged build against a real remote host, and the six lines of glue in
startGateway()are the part no unit test covers.Screenshots
Settings › Developer, captured from the branch's own Electron shell (
npm startinwebsite/electron) connected to an isolated pod serving this branch's frontend — so both halves of the change are the ones under review, not a released shell against a new bundle.The section's presence is itself the evidence that the preload bridge resolved:
useLocalGatewayreportssupported: falseand renders nothing whenwindow.localGatewayAPIis absent, which is why this surface does not exist in a browser or the PWA and cannot be captured from a loopback URL.DeveloperPanel.test.tsxasserts both halves of that.