Skip to content

feat: name runtime machines - #22094

Merged
brennanb2025 merged 27 commits into
mainfrom
brennanb2025/mobile-host-machine-name
Sep 24, 2026
Merged

brennanb2025 merged 27 commits into
mainfrom
brennanb2025/mobile-host-machine-name

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 22 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​694 $\color{#cf222e}{\Huge{\mathbf{−}}}$​6 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​688
Prod 32 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​448 $\color{#cf222e}{\Huge{\mathbf{−}}}$​58 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​390

ELI5

Each computer running Orca can now say what it is called, and paired phones and the CLI can ask it. Today a phone lists every paired desktop as "Host 1", "Host 2", and so on, so people name them from memory, and a wrong guess sends work to the wrong computer. This is the desktop half: the machine publishes its own name. The phone half that shows it is #22104.

What Changed

Before: nothing a desktop sends ever names the computer. The pairing screen shows a QR code and no machine identity, and the status reply carries the operating system but no name.

After:

  • The runtime keeps an optional machine name setting. Left blank, it uses the computer name it detects: macOS's friendly Computer Name when available, otherwise the hostname. Detection runs once per process and is shared.
  • The name goes out as a new optional field on the existing status reply. A status read waits for the detection to land, up to a 750 ms budget that stays under the CLI's one-second status probe; if the lookup is still running on a busy machine, that read answers with the hostname and the next read carries the friendly name. The lookup itself gets five seconds, and a lookup that fails or times out is retried after thirty seconds instead of being remembered as the answer for the rest of the session.
  • One Machine name field, a single component backed by the same saved setting, appears wherever other devices join this computer: Settings → Mobile, the Orca Mobile page's "Pair this computer" step, and Settings → Remote Servers → Share this host. It sits next to the pairing code it describes, so the name is set where it will be seen. Its caption always names what other devices and hosts receive: the saved override when one is set, otherwise the detected name read from the runtime. Settings search finds it under "machine", "hostname", "computer", "rename". It is deliberately not in General or in the flows where this app joins another machine (Add SSH host, Add remote server): those forms describe the remote host, and a field about this computer's own name confused readers.
  • orca host name prints the name and platform of the runtime it is pointed at, inside that runtime's own reply envelope, so orca host name --environment <name> (or an ambient ORCA_ENVIRONMENT) reports the remote's runtime id, not "local". orca host name --name <name> sets it and prints what the runtime now publishes; --name "" returns to the detected name. Against an older runtime that does not publish a name, the write is refused with a plain message instead of a schema error. orca host list shows this machine's own name on its local row.
  • Two places that already told other machines what this computer is called now use the same name: the structured-chat handoff banner ("Agent is open in terminal on …") and the client name this desktop sends when it connects to an SSH remote workspace.
  • What this does and does not do for SSH hosts. No SSH screen changes: the Add SSH host dialog, the SSH Hosts settings page, and the host picker are identical to main. SSH hosts are not named by this feature; they run Orca's uploaded relay, not the Orca app, so there is no name for them to set and nothing flows from them to this desktop (their detected name would be a separate follow-up, and your Label would still come first). The only SSH-side effect is one-way and currently invisible: when this desktop asks an SSH remote workspace who else is connected, it introduces itself by this machine name instead of its raw hostname. No desktop UI reads that list today, so nothing visible changes.
  • Trimming and the 255-character bound live in one shared normalizer, applied at the single settings write boundary every writer crosses and when a settings file is loaded.
  • The pairing code and invite are unchanged. No phone code is in this PR.

Why

This is one name for the whole machine, not a mobile setting: every client that reads status gets it, and this desktop introduces itself by it when it connects to remote workspace hosts. The name belongs to the machine that owns it, not to whoever typed a label for it: the runtime that answers reports its own name, and nothing is inferred from the phone or the network address. The phone keeps its own personal label on top (in #22104), so a machine-reported name never overwrites what a user chose to call it.

Alternatives considered:

  • Phone-only labels (the status quo) are exactly what causes the wrong-machine mistakes.
  • A pushed rename event would let an already-connected phone learn of a rename without re-asking. Orca has no settings push to paired clients today, and the phone reads status on every connect, so a reconnect-time read covers the real case without new stream machinery.
  • A richer identity object (name, source, platform, id) would restate facts the status reply already carries (runtimeId, hostPlatform), and "custom vs detected" is answered on the phone by whether its own label is set. A single string is the derivable, wire-stable choice.
  • Awaiting detection everywhere (at construction or before the RPC server listens) would gate unrelated user actions on bookkeeping. Only the status read waits, and only until the one-time lookup settles.

Two things deliberately stay as they were:

  • Listings stay local. orca host list and orca environment list answer from this machine's own pairing store. Asking each paired server for its name would make them slow and fail whenever one is offline, so a server's name is read one at a time with orca host name --environment <name>.
  • No proof of the physical machine. A copied Orca profile carries the same identity to another computer, so this names machines; it does not prove which one answered.

Letting the host's user override the detected name is an addition on top of the common "host reports its detected name" shape; its nearest existing pattern is a host-detected value with a host-stored override so every device sees the same thing.

Linked Issue

Mobile P0: worktrees created on the wrong machine because paired desktops could not be told apart.

Visual Proof

Captured from the running app (hidden renderer over the debugging protocol) in fresh dev profiles. Mobile pane and CLI round-trip at 8be3f060b4:

Detected name (field blank) Override saved CLI rename reflected live
Settings → Mobile with the detected name as placeholder and caption Override typed; caption names the override CLI rename shown in the open Settings page without a reload

At the final head f11b974ff8, cold start on a loaded machine (load average 33 to 41): the detected name from the first read, the same value on all three surfaces, and no field on General or the join-side forms.

Settings → Mobile (detected) Remote Servers → Share this host Orca Mobile page pairing step
Mobile detected Share this host Pairing step
SSH pane → Add Target: no field Add SSH host dialog: no field Add remote server dialog: no field
SSH pane add target Add SSH host Add remote server

Those three re-shot at the merged head f7879e4abf; General is unchanged from f11b974ff8 (no field).

Earlier capture at 00421902ae (same three surfaces, override saved):

Mobile pane Remote Servers → Share this host Orca Mobile page pairing step
Mobile pane Share this host Pairing step

Before this change the Mobile pane had no name field and the status reply carried no name, so there is no "before" state to screenshot beyond the pane without the field.

Testing

Validated in the running app on macOS (8be3f060b4 for the Mobile pane and CLI round-trip, 00421902ae for every surface, 4f5edbcc73 for cold start under load, f11b974ff8 for the narrowed surfaces):

  1. Settings → Mobile shows the detected name (Brennan’s MacBook Pro) as the placeholder and in the caption; window.api.runtime.getStatus().machineName returns the same string.
  2. Typing QA Override Desk and leaving the field updates the caption, the store, and getStatus() to the override; clearing it returns all three to the detected name.
  3. Settings search for "hostname" surfaces the field.
  4. From a second shell, orca host name prints Brennan’s MacBook Pro (darwin); --name "CLI Named Desk" prints CLI Named Desk (darwin); --json carries the answering runtime's id; host list shows this machine (CLI Named Desk); --name "" prints the detected name again. The still-open Settings page reflected the CLI rename without a reload.

Automated:

  • Automated tests added/updated
  • pnpm tc; pnpm run check:code-quality:changed; oxlint and oxfmt on every changed file; all localization and RPC-catalog verifiers.
  • Focused suites: 27118 tests / 0 failures at 8be3f060b4 (src/cli, src/main/runtime, src/main/ipc, src/main/persistence, src/shared, src/relay, renderer settings and web); at 4f5edbcc73, renderer settings/mobile/sidebar/i18n 4147 tests / 0 failures and main-process runtime + IPC 6866 tests / 0 failures.
  • Every fix carries a test that fails without it (verified by removing the fix line and watching it go red): the CLI reply shape, the routed envelope, the store-boundary normalizer, the shared one-time detection, the handoff label, the caption source, the status wait, the older-host refusal, and the remote-workspace client name.

AI Disclosure

Review

Agent skill upstream boundary

  • Not applicable

Notes

  • Wire: a new optional field on the status reply and on client settings; older clients ignore it and older desktops send nothing. No capability flag is needed for reading. The settings-update contract is strict, so a newer client writing a name to an older host would be rejected; the CLI checks the host advertises the field before writing, and the desktop field only ever writes to its own runtime.
  • A name longer than 255 characters is truncated by the desktop field and on file load, but rejected by the RPC contract (so orca host name --name <very long> errors rather than silently shortening).
  • settings.update was already on the mobile method allowlist, so a paired phone can rename the desktop through the existing setting path; nothing new is exposed.
  • The phone PR feat(mobile): name the machine after pairing #22104 still carries a trim-and-slice hunk in src/main/ipc/settings.ts; that edge normalizer was removed here in favor of the store boundary and should be dropped there before it lands.

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • Before/after screenshots attached
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path impact considered
  • Localization catalog synced; English values match the in-code fallbacks

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Two defects sit in the new orca host name handler: the --name path prints undefined because the settings.update reply is { settings }, and the read path stamps a routed answer with _meta.runtimeId: "local".

Reviewed changes

  • New orca host name CLI command — reads the answering runtime's name/platform from status.get, or writes one through settings.update; host name is now routable while host list stays pinned local (index.ts:37).
  • Runtime machine name — RuntimeMachineName resolves a configured override, else a detected name (scutil --get ComputerName on macOS, os.hostname() otherwise), published as an optional machineName on RuntimeStatus.
  • Settings plumbing — machineName added to GlobalSettings (default ''), trimmed on load and IPC, projected to paired clients, and accepted by the strict SettingsUpdate.
  • host list / environment list output — each paired environment is now queried for its name/platform, and listings show (machineName) and platform.
  • Desktop pairing screen — new machine-name field on MobilePairingSetupSection fed by a useDetectedMachineName hook, with the pairing output sections extracted into MobilePanePairingOutput.
  • Mobile — machineName threaded through host-status-gates.ts, use-mobile-home-host-connections.ts, and the MobileHomeHostList prop type.

ℹ️ SettingsUpdate is strict, so machineName is rejected wholesale by an older host

SettingsUpdate ends in .strict() (client-settings-params.ts:123), and old hosts do not know the new key. host name is now routable, so orca host name --name X --environment old-server sends machineName to a host whose decoder rejects the entire payload — the user gets an opaque validation error rather than "that runtime can't be renamed". A new optional reply field is safe, but a new request field is only safe if the peer tolerates unknown keys, which strict decoding does not.

Technical details
# New request field against a strict decoder

## Affected sites
- `src/shared/rpc-contract/client-settings-params.ts:82-123` — `SettingsUpdate` is `.strict()`, so `machineName` fails validation on a host predating it.
- `src/cli/handlers/environment.ts:30-36` — the only new sender; can be routed via `--environment` / `ORCA_ENVIRONMENT` to an older paired server.
- `mobile/src/tasks/task-runtime-reply-schema.ts:110-119` — notes `settings.update` sent from mobile is fire-and-forget, so a rejection there is currently invisible.

## Required outcome
- Decide whether renaming a host that predates the field should fail as unsupported (preferred) or be silently ignored, and make the failure legible. If the field must stay, verify the host-side decode path does not reject the whole update.

## Open questions for the human
- Is `settings.update` from mobile expected to carry `machineName` in a later PR? If so, an old host rejecting the payload would also drop whatever else that update carried.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Comment thread src/cli/handlers/environment.ts Resolved
Comment thread src/cli/handlers/environment.ts Resolved
Comment thread src/cli/handlers/environment.ts Resolved
@brennanb2025
brennanb2025 force-pushed the brennanb2025/mobile-host-identity branch 2 times, most recently from d7ade23 to ac6450c Compare September 23, 2026 00:24
Listing paired servers read each one's machine name by dialing it, so both listings made a network
round trip per server and waited out a timeout on any that were offline. They answer from this
machine's own pairing store; `orca host name --environment <name>` reads one server's name.
The caption read the runtime's published name once, when the pane opened, so saving an override
left it naming the old computer while phones already showed the new one. It now re-reads whenever
the saved override changes; the settings write lands in the main process before the store publishes
it, so that read already sees the new name. The name is interpolated rather than baked into the
fallback, and the caption, label and placeholder are in the English catalog.
@brennanb2025
brennanb2025 force-pushed the brennanb2025/mobile-host-machine-name branch from b30b287 to 1191973 Compare September 23, 2026 02:13
@brennanb2025
brennanb2025 changed the base branch from brennanb2025/mobile-host-identity to main September 23, 2026 02:14
@coderabbitai

coderabbitai Bot commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 318564ac-c6e8-43c0-ac96-a3c55cbd4059

📥 Commits

Reviewing files that changed from the base of the PR and between c4a228e and 6943454.

📒 Files selected for processing (24)
  • src/cli/handlers/environment.ts
  • src/cli/index-local-command-routing-flags.test.ts
  • src/main/ipc/remote-workspace-client-identity.ts
  • src/main/ipc/remote-workspace-connected-clients.test.ts
  • src/main/ipc/remote-workspace-connected-clients.ts
  • src/main/ipc/remote-workspace-patch-queue.test.ts
  • src/main/ipc/remote-workspace-stale-resync.test.ts
  • src/main/ipc/remote-workspace.test.ts
  • src/main/ipc/remote-workspace.ts
  • src/main/ipc/runtime.test.ts
  • src/main/ipc/runtime.ts
  • src/main/ipc/ssh-host-partition-session-export.test.ts
  • src/main/runtime/orca-runtime-get-status.ts
  • src/main/runtime/remote-runtime-request-connection.integration.test.ts
  • src/main/runtime/rpc/core-typed-method-contract.test.ts
  • src/main/runtime/rpc/methods/status.test.ts
  • src/main/runtime/rpc/methods/status.ts
  • src/main/runtime/rpc/methods/updater.test.ts
  • src/main/runtime/runtime-extraction-regressions.test.ts
  • src/main/runtime/runtime-machine-name.test.ts
  • src/main/runtime/runtime-machine-name.ts
  • src/main/runtime/runtime-rpc-mobile-method-allowlist-fixtures.ts
  • src/main/runtime/runtime-rpc-mobile-terminal-streaming.test.ts
  • src/main/window/attach-main-window-services.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/main/runtime/orca-runtime-get-status.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The change adds configurable machine names to shared settings and runtime status. The runtime detects a default name and reports the configured or detected value. The CLI can display or set a runtime name, and the mobile settings pane can edit it. Runtime-backed web settings read and update the name. Remote-workspace presence and the terminal handoff host label use the runtime’s current name.

Priority: ➖ Normal

Merge Risk: 🟡 Moderate · up to 69434

Settings can show the wrong machine name or report a rename as saved when the paired runtime rejected it. Resolve those mismatches before merging; the transient mobile caption failure is narrower.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 30.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 40 functions across 59 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the main change: adding names for runtime machines.
Description check ✅ Passed The description is detailed and covers the user impact, implementation, rationale, visual proof, testing, compatibility, and checklist items. It is on topic and mostly complete, although the Linked Is…
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 726a02d4-3969-4e64-b36b-e41dd6b16389

📥 Commits

Reviewing files that changed from the base of the PR and between ebed096 and 1191973.

📒 Files selected for processing (29)
  • src/cli/format.ts
  • src/cli/handler-group-manifest.ts
  • src/cli/handlers/environment.ts
  • src/cli/index-local-command-routing-flags.test.ts
  • src/cli/index.ts
  • src/cli/specs/environment.ts
  • src/cli/workspace-format.ts
  • src/main/ipc/settings.ts
  • src/main/persistence/loading-store/normalize-loaded-global-settings.test.ts
  • src/main/persistence/loading-store/normalize-loaded-global-settings.ts
  • src/main/runtime/orca-runtime-get-status.ts
  • src/main/runtime/orca-runtime-state-fields.ts
  • src/main/runtime/runtime-client-settings-minimax-projection.test.ts
  • src/main/runtime/runtime-client-settings.ts
  • src/main/runtime/runtime-extraction-regressions.test.ts
  • src/main/runtime/runtime-machine-name.test.ts
  • src/main/runtime/runtime-machine-name.ts
  • src/main/runtime/runtime-store-contract.ts
  • src/renderer/src/components/settings/MobilePairingSetupSection.test.tsx
  • src/renderer/src/components/settings/MobilePairingSetupSection.tsx
  • src/renderer/src/components/settings/MobilePane.tsx
  • src/renderer/src/components/settings/MobilePanePairingOutput.tsx
  • src/renderer/src/components/settings/use-published-machine-name.test.tsx
  • src/renderer/src/components/settings/use-published-machine-name.ts
  • src/renderer/src/i18n/locales/en.json
  • src/shared/default-global-settings.ts
  • src/shared/global-settings-types.ts
  • src/shared/rpc-contract/client-settings-params.ts
  • src/shared/runtime-session-contracts.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review.

Comment on lines +112 to +113
machineName:
typeof parsed.settings?.machineName === 'string' ? parsed.settings.machineName.trim() : '',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Enforce the length limit when loading persisted settings.

A hand-edited or imported profile bypasses SettingsUpdate and sanitizeRendererSettingsUpdate. This path trims machineName but does not apply the 255-character limit. The runtime can then publish an oversized name to connected clients. Slice the normalized value to the same limit and add a persisted-profile test case.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No new blocking issues in this delta — one minor suggestion inline. The two orca host name defects flagged in the prior review are untouched by these commits and remain open.

Reviewed changes

  • Listings stay local — host list and environment list no longer dial each paired server for a name/platform; host list reads the local runtime's own name once, so an offline server can no longer stall the listing.
  • Machine-name caption re-reads — useDetectedMachineName becomes usePublishedMachineName, re-reading the published name whenever the saved override changes so the caption never names the computer it replaced.
  • Pairing output extracted — the QR, firewall, paired-device, and auto-restore sections move into MobilePanePairingOutput (MobilePane hit max-lines), with selectedAddress retyped to string | undefined to match its caller.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Comment thread src/cli/workspace-format.ts Resolved
The trim and length rules for `machineName` were spelled out separately at the
renderer IPC (trim + 255), the settings load path (trim only, no cap), the RPC
schema (zod trim + 255) and the runtime reader (trim). A hand-edited or legacy
profile could therefore load a longer name than any writer accepts.

`src/shared/machine-name.ts` now owns `MACHINE_NAME_MAX_LENGTH` and
`normalizeMachineName`, and every writer and the load path use it. The RPC
schema keeps rejecting over-long names but derives its cap from the constant,
and the runtime settings controller normalizes an RPC write before storing it.
Every runtime constructed in a process (the app, plus each one a test builds)
ran its own `scutil` lookup. The friendly name is a property of the host, so the
lookup is now a single shared promise; construction still never blocks on it,
and a rejected lookup can no longer surface as an unhandled rejection.

The structured-chat handoff banner ("Agent is open in terminal on X") named this
host with the bare `os.hostname()` while paired devices saw the published name.
The transport now reads the same `RuntimeMachineName`, through a getter so a
rename in Settings is reflected without rebuilding the transport.
`orca host name --name X` printed `undefined`: `settings.update` replies with
`{ settings }`, but the handler read a bare `machineName` off the reply, and the
test fixture mirrored the wrong shape so it passed. After a write the command
now re-reads `status.get` and prints what the runtime publishes, so a blank
`--name` prints the detected name it returned to rather than an empty string.

The read path wrapped a possibly routed answer in a local envelope, stamping
`_meta.runtimeId: "local"` on a reply from another server. It now returns the
`status.get` envelope itself, and an unreachable runtime is reported as the
usual error instead of an invented "unknown" name.

`environment list` had gained machine-name and platform columns that no caller
populated, so every row printed "platform unknown"; the columns are removed.
The caption under the field re-read runtime status every time the saved value
changed, relying on a comment about write ordering to show the new name. A saved
override already is what paired devices see, so the hook now derives the caption
from it and asks the runtime only for the detected name; a stale status read can
no longer show the previous name.

`MobileMachineNameField` owns the store read, the published-name hook and the
debounced input, so `MobilePairingSetupSection` returns to its prop shape and
the pass-through `MobilePanePairingOutput` wrapper is gone. Paired-device
revocation moves into `useMobilePairedDeviceRevocation`, which keeps
`MobilePane` within its line budget with an extraction that carries behavior.

The web client mounts this pane too, but its settings store kept the name
locally where nothing published it. `machineName` now rides the existing
runtime-backed settings sync so the field renames the paired runtime.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: cc96bae0-92a4-4f7d-a9d3-b748ccb65322

📥 Commits

Reviewing files that changed from the base of the PR and between 1191973 and 159aa4d.

📒 Files selected for processing (25)
  • src/cli/format.ts
  • src/cli/handlers/environment.ts
  • src/cli/index-local-command-routing-flags.test.ts
  • src/main/ipc/settings.test.ts
  • src/main/ipc/settings.ts
  • src/main/persistence/loading-store/normalize-loaded-global-settings.test.ts
  • src/main/persistence/loading-store/normalize-loaded-global-settings.ts
  • src/main/runtime/orca-runtime-get-worktree-ps.ts
  • src/main/runtime/runtime-client-settings-minimax-projection.test.ts
  • src/main/runtime/runtime-client-settings.ts
  • src/main/runtime/runtime-extraction-regressions.test.ts
  • src/main/runtime/runtime-machine-name.test.ts
  • src/main/runtime/runtime-machine-name.ts
  • src/renderer/src/components/settings/MobileMachineNameField.test.tsx
  • src/renderer/src/components/settings/MobileMachineNameField.tsx
  • src/renderer/src/components/settings/MobilePane.tsx
  • src/renderer/src/components/settings/use-mobile-paired-device-revocation.ts
  • src/renderer/src/components/settings/use-published-machine-name.test.tsx
  • src/renderer/src/components/settings/use-published-machine-name.ts
  • src/renderer/src/i18n/locales/en.json
  • src/renderer/src/web/preload-api/web-preferences-store.ts
  • src/renderer/src/web/web-preload-api-machine-name.test.ts
  • src/shared/machine-name.test.ts
  • src/shared/machine-name.ts
  • src/shared/rpc-contract/client-settings-params.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/main/persistence/loading-store/normalize-loaded-global-settings.test.ts
  • src/renderer/src/i18n/locales/en.json

Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review.

return () => {
cancelled = true
}
}, [override])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Retry a transient status-read failure.

If getStatus() rejects while the runtime starts, the effect does not read status again while override stays blank. The field keeps its generic placeholder even after the runtime becomes ready. Retry while the field is mounted, and cancel the retry on cleanup. React reruns this effect only when its dependency changes. (react.dev)

Comment on lines +227 to +229
if (typeof updates.machineName === 'string') {
runtimeUpdates.machineName = normalizeMachineName(updates.machineName)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Do not report a machine-name update before the runtime accepts it.

If settings.update fails, settings.set has already persisted the new name locally. syncRuntimeBackedSettings then returns localNext for this update, so the UI reports a successful rename while the paired runtime still publishes its old name. Require an acknowledged machineName update before retaining the local value; propagate failure or restore the previous value. Cover the failure path in a test. (raw.githubusercontent.com)

Every writer (desktop IPC, web RPC, CLI) reaches the store through
updateSettings, which already normalizes the other free-text settings
there. Trim and bound the machine name in that one place instead of at
two upstream edges, so a future main-process writer is covered too.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

This run re-reviewed the four commits pushed since the prior pullfrog review at 1191973. Both open orca host name defects and the environment list formatting note are now fixed, and the follow-on refactors are clean.

  • orca host name prints the runtime's own answer — the write path no longer reads .machineName off the settings.update { settings } envelope; both paths now print the status.get result inside that runtime's envelope, so a routed answer carries its real _meta.runtimeId and a blank --name prints the detected name.
  • One machine-name normalizer — normalizeMachineName / MACHINE_NAME_MAX_LENGTH move into src/shared/machine-name.ts and are applied on persisted load, renderer IPC sanitize, paired-client projection, the RPC schema, and the web preferences store.
  • Detection runs once per process — detectRuntimeMachineNameOnce shares the macOS scutil lookup across runtimes, and the structured-handoff transport labels hosts through a hostLabel getter that reads the published name live, so a Settings rename reaches later handoffs.
  • Machine-name field extracted — the field becomes its own MobileMachineNameField component with its own test, and paired-device revocation moves to useMobilePairedDeviceRevocation.
  • formatEnvironmentList column reverted — environment list no longer prints a platform/machineName column it has no source for.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

This run re-reviewed the single commit pushed after the prior pullfrog review at 159aa4d.

  • Machine-name normalization moved to the store boundary — normalizeMachineName now runs in updateSettings (src/main/persistence/applying-settings/settings-update.ts), the one boundary desktop IPC, the web RPC and the CLI all cross, so the persisted name is the trimmed, bounded form no matter which client wrote it. The now-redundant copies in sanitizeRendererSettingsUpdate and RuntimeClientSettingsController.update are removed.
  • Projection reads the stored value — RuntimeClientSettingsController.get() returns settings.machineName ?? '', relying on the boundary to have normalized it.
  • Boundary test added — settings-update-machine-name.test.ts covers trimming, the length cap, blank/non-string writes, and that listeners receive the normalized value; the old IPC-level test is removed.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

…field searchable

The shared machine-name lookup test spawned the real `scutil` twice and compared the answers, so a
slow runner could time one spawn out to the hostname and fail. It now mocks the subprocess, proves
the hostname answers until the one shared lookup lands, and that a second runtime does not spawn
again.

`host name` is no longer pinned local, but only the explicit `--environment` route was covered; an
ambient `ORCA_ENVIRONMENT` now has its own test so the pin cannot silently grow back.

The Machine name field is added to the Mobile pane's search catalog at the tail, keeping every
existing row's tie-break index.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — one minor test-coverage suggestion inline.

Reviewed changes

This run re-reviewed the single commit pushed after the prior pullfrog review at 87e4967.

  • Machine name is now searchable — getMobilePaneSearchEntries gains a "Machine name" entry with machine/hostname/computer/rename keywords, the matching keys are added to en.json, and the catalog test's expected titles are extended.
  • host name routing pinned by test — a new CLI case proves host name honours an ambient ORCA_ENVIRONMENT (the client is built with undefined, undefined, not the local null, null), unlike the pinned host list.
  • Shared detection test reworked — runtime-machine-name.test.ts mocks runProcess and asserts two runtimes plus a repeated start() share one lookup, replacing the flaky live-scutil assertion.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

A status read answered in the first few milliseconds after launch published the bare
hostname because the friendly-name lookup had not landed yet, and a caption fetched in
that window never corrected itself. RuntimeMachineName now exposes the settled lookup
as a promise, and both status publishers (the status.get RPC and the desktop
runtime:getStatus IPC) await it before reading. Construction, listen, and every other
method stay unblocked; the worst case is one wait of at most a second on the first read.
…name

An older Orca runtime rejects the unknown settings field with a bare invalid_params, so
'orca host name --name' routed at one failed with no explanation. The runtime that does
not publish machineName on status cannot store one either, so the CLI reads status first
and refuses with incompatible_runtime and a message that says to update that host,
before writing anything.
When this desktop connected to a remote workspace host it announced itself under a
hostname captured once at module load, so a renamed machine kept its old name on every
other device's connected-clients list. The client name is now read at send time from
the runtime's machine name (the configured override, else the detected one), passed in
where the remote workspace handlers are registered, so a rename reaches the next
presence frame without a relaunch.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No new issues in this delta — the three commits are clean. One earlier test-coverage thread remains open (runtime-machine-name.test.ts's shared-lookup assertion is macOS-only), so this is not an approval.

Reviewed changes

This run re-reviewed the three commits pushed after the prior pullfrog review at c4a228e.

  • Remote workspace presence introduces this desktop by its published name — remote-workspace-client-identity.ts drops the module-load CLIENT_NAME constant for readClientName, which reads runtime.readMachineName() per presence frame, so a Settings rename reaches the next workspace.presence frame without a relaunch. registerRemoteWorkspaceHandlers and listRemoteWorkspaceConnectedClients now take a RemoteWorkspaceClientNameSource, threaded from attach-main-window-services.ts.
  • Status waits for the machine-name lookup — status.get and runtime:getStatus now await runtime.machineNameReady(), and RuntimeMachineName gains ready(), so a status answered while the macOS friendly-name lookup is in flight never publishes the bare hostname.
  • CLI refuses to rename an unsupported runtime — orca host name --name reads status.get first and raises incompatible_runtime when the answering runtime publishes no machineName, instead of sending a settings.update that an older strict schema rejects wholesale.
  • Tests — new remote-workspace-connected-clients.test.ts and status.test.ts, a runtime:getStatus gate test, and machineNameReady added to the affected runtime fakes.

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Status publishers now wait for the one-time name lookup, and `orca status`
probes them with a one-second budget. scutil answers in milliseconds, so a
half-second cap keeps a stalled lookup from making a healthy runtime read as
"starting" while still preferring the friendly name.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — one stale comment inline.

Reviewed changes

This run re-reviewed the single commit pushed after the prior pullfrog review at 6943454.

  • macOS friendly-name lookup capped at 500 ms — MACOS_COMPUTER_NAME_TIMEOUT_MS drops from 1 s to 500 ms, with a comment recording why: status.get now waits on this lookup, and orca status probes that reply with a 1 s timeout (src/cli/runtime/status.ts:40), so a full-second scutil stall could make a healthy app read as starting.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

The Mobile settings section is desktop-only, so the paired web client can
never render the field. Forwarding the name through the web settings sync was
dead code, and against an older host the strict update contract would have
rejected it while the local mirror kept the value. Remove it until a web
surface exists.
…erver rows

Add the Machine name field and its Settings search entry to the five non-English
catalogs, explain in the host list spec why paired-server rows report an unknown
platform, and drop a stale timeout figure from a test comment.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No new issues in this delta — the two commits are clean. One earlier test-coverage thread (runtime-machine-name.test.ts's shared-lookup assertion is macOS-only, so CI never exercises it) remains open, so this is not an approval.

Reviewed changes

This run re-reviewed the two commits pushed after the prior pullfrog review at 8be3f060.

  • Translated the machine-name strings — the machineName search subtree and MobileMachineNameField strings now exist in es, fr, ja, ko, and zh alongside en; pnpm verify:localization-catalog passes and every locale file parses.
  • Dropped the web machine-name path — web-preferences-store.ts no longer projects machineName from the host or forwards it on settings.update. Nothing under src/renderer/src/web references the key and the web build does not import components/settings, so the desktop field remains the only writer and this is a no-op.
  • Documented paired-server rows — the host list help now states that paired-server rows come from the pairing store and report platform unknown, pointing at orca host name --environment <name> for a server's own name.
  • Fixed the stale timeout comment — runtime-machine-name.test.ts now says "lookup timeout" instead of "the 1 s timeout", closing the prior review's inline nit.

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Review status: ready for merge at 1d3f75b201

Six review passes, an architecture challenge, two readiness checklists, and in-app validation on the final code. Every pass either found something and fixed it with a test that fails without the fix, or came back clean.

What the review loops fixed (12 commits on top of the original four)

  • orca host name --name X printed undefined: the settings write replies with a settings object, not a name. The command now re-reads status after the write and prints what the runtime actually publishes.
  • orca host name --environment X labelled a remote answer as this machine. It now returns the answering runtime's own reply envelope; an unreachable runtime is a normal error, not an invented name.
  • orca environment list gained columns nothing ever filled in ("platform unknown" on every row). Removed.
  • Trim and the 255-character bound were copied at four call sites. There is now one shared normalizer applied at the single settings write boundary and on file load.
  • The Settings caption could name the old value after a save. It is now derived from the saved override, and asks the runtime only for the detected name.
  • Each runtime instance spawned its own name lookup. One lookup per process, shared.
  • Status answered in the first milliseconds after launch published the bare hostname. Status publishers now wait for the one-time lookup (capped at half a second, under the CLI's one-second status probe).
  • Two other places already told other machines what this computer is called (the structured-chat handoff banner and the client name sent to remote workspace hosts) and used the raw hostname. Both now use the published name, so a rename reaches them.
  • Renaming an older runtime that cannot store a name failed with a raw schema error. The CLI now checks the host advertises the field and refuses with a plain message.
  • A pass-through settings wrapper component was replaced by a component that owns the field; the field was added to Settings search; the five non-English catalogs got the new strings; one test that compared two live lookups was made deterministic.

Architecture check. The design was challenged after three loops and kept: the host owns and publishes its name on the status reply; the phone keeps its own label (#22104). The three changes that came out of that challenge (status waits for detection, CLI gate for older hosts, one name source for every emitter) are in.

Validation on the real app (hidden renderer over the debugging protocol, fresh profile, head 8be3f060b4; the two later commits touch only translations, a CLI help note, a test comment, and remove unreachable web code):

  • Cold-start status returns the friendly name, never the .local hostname.
  • Detected name shows as placeholder and caption; typing an override updates caption, store, and status; clearing returns to the detected name.
  • orca host name, --name, --json, host list, and --name "" all print the expected values; the open Settings page reflected the CLI rename without a reload; orca status stayed ready / reachable.
    Screenshots are in the PR body.

Gates at the final head: typecheck clean; changed-code quality gate 0 findings across 51 files; oxlint and oxfmt clean; all localization and RPC-catalog verifiers pass. Focused suites (src/cli, src/main/runtime, src/main/ipc, src/main/persistence, src/shared, src/relay, renderer settings and web): 27118 tests, 0 failures at 8be3f060b4 on a quiet machine; the same run at the final head on a loaded machine (three other worktrees running vitest against the shared /tmp/workspaces fixtures) showed 5 failures in 4 files, none touched by this diff, two of which still collided when re-run alone under that load. The isolated CI shards are the verdict for those. CI: all 27 checks green on 1d3f75b201. The first run of that head went red on one test in the untouched worktree scan-cache suite (orca-runtime.test.ts, "expires scans per repo without coupling sibling repos": expected 3 scans, saw 4). That group passes alone at both origin/main and this head, the same runtime code was green on the two previous heads, and only a deleted test file changed the shard composition; the re-run of the failed jobs passed.

Things to know, not blockers

  • The settings-update RPC contract is strict, so a newer client writing a name to an older host is rejected. The CLI guards against it; the desktop field only writes to its own runtime; no web surface renders the field. A pushed rename event was considered and not added (no settings push to paired clients exists today, and the phone re-reads status on connect).
  • orca host name --name <more than 255 characters> is rejected by the RPC contract rather than truncated (the desktop field and file load truncate).
  • feat(mobile): name the machine after pairing #22104 still carries a trim-and-slice hunk in src/main/ipc/settings.ts that duplicates the boundary normalizer removed here; it should be dropped there before it lands.
  • Two full-suite failures seen during review (orca-runtime.test.ts lineage listing, structured-agent-session-lease-renewer.test.ts real-timer poll) pass alone, are untouched by this diff, and reproduce as load interference on a shared /tmp fixture path.

… a General home

The name other devices and hosts list this computer under is not a mobile
setting. Rename MobileMachineNameField to MachineNameField, give it a per-mount
id, and put its primary home in Settings > General under "This computer". The
Mobile pane keeps the same field. One shared search entry feeds General, the
Mobile pane, and the copy now says "other devices and hosts" in all six locales.

The web client has no machine of its own to name and its settings mirror cannot
persist one, so the field renders nothing there and General omits the section.
The "Pair this computer" step now shows the same machine name field above the
connection choice and code, so a user pairing a phone from the sidebar page can
name the computer right there.
Share this host produces the access link other devices use to reach this
machine, so it mounts the machine name field first. The pane's search entry
takes the shared machine-name keywords so a search lands there.
This desktop introduces itself to a new SSH host or remote server under its
machine name, so the Add Remote Host dialog mounts the field once, between the
header and the host fields, in both modes. Submit logic is unchanged.
The SSH pane's add form mounts the machine name field above the host fields.
Editing a saved host leaves it out; that host already met this computer.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — one minor suggestion inline.

Reviewed changes

This run re-reviewed the five commits pushed after the prior pullfrog review at 1d3f75b, which turn the machine-name field into a machine-wide setting and add five mount points.

  • Machine-name field is now machine-wide — MobileMachineNameField becomes MachineNameField with a per-mount id, hides itself on the web client, and commits on Enter without submitting an enclosing add-host form; Settings → General gains a "This computer" section and the search entry moves to a shared machine-name-search.ts catalog reused by the Mobile pane and Remote Orca Servers.
  • Mobile pairing step names this computer — MobileHeroPairingStep mounts the field above the Relay options, with new machine / relay grid areas in mobile-page.css.
  • Share this host names this host — RuntimeServerShareSection mounts the field above the link generator.
  • Add-host surfaces name this desktop — AddRemoteHostDialog threads the field into both the SSH and remote-server form panels via a new children slot, and SshTargetForm mounts it in add mode only.
  • Translations — the machine-name strings move to machine.name.search.* / MachineNameField.* across all six locales, and the old mobile.pane.search.machineName keys are removed.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

The pairing step wrapped MachineNameField in its own grid-area div. On the
web client the field renders nothing, so the wrapper left an empty row and
an extra row gap between the copy and the connection options. The field now
takes a className for its root, so the grid slot disappears with it.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — one minor suggestion inline.

Reviewed changes

This run re-reviewed the single commit pushed after the prior pullfrog review at 0042190, which reworks how the machine-name field is mounted in the mobile pairing layout.

  • The pairing grid class is passed to the field itself — MachineNameField gains an optional className merged onto its root via cn, and MobileHeroPairingStep drops the mp-pairing-machine wrapper <div> and passes the class directly, so the field's root is the grid item. Tests assert the class now rides on the field root rather than a parent.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

…ke sibling inputs

The field intercepted Enter to blur and commit instead of submitting the enclosing
SSH add form. The draft is already flushed on blur and on unmount, and the name is
read from the store whenever a peer asks, so nothing is lost when the form submits
first. Enter now behaves like the neighbouring inputs; the test proves the submit
fires and the name still commits when the form closes.
A dedicated grid row stayed in the template on the web client, where the field
renders nothing, adding an empty track and a second row gap between the copy and
the connection options. The field now sits at the end of the copy cell with the
same 18px rhythm, so an absent field leaves nothing behind.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No new issues in this delta — both commits address prior Pullfrog feedback cleanly. The earlier test-coverage thread (runtime-machine-name.test.ts's shared-lookup assertion is macOS-only, so CI never exercises it) remains open, so this is not an approval.

Reviewed changes

This run re-reviewed the two commits pushed after the prior pullfrog review at efa7dbb8.

  • Enter in the machine-name field now submits its enclosing form — MachineNameField drops its custom onKeyDown guard, so Enter behaves like sibling inputs; the typed draft still commits through the debounce/blur/unmount flush in use-debounced-settings-text-draft.ts. This resolves the prior IME-composition thread: no custom handler is left to bypass composition, and browsers suppress native implicit submission while composing.
  • The pairing grid no longer carries an empty machine row — the field moves inside .mp-pairing-copy and mobile-page.css drops the machine area from both .mp-pairing-layout and .has-failure; .mp-pairing-machine is now just margin-top: 18px. This is the structural fix the prior review asked for, so the web client no longer reserves the track.
  • Tests — the mobile pairing test now asserts the field's parent is .mp-pairing-copy, and the field test asserts Enter submits the enclosing form once while the name still commits on unmount.

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

…the hostname

On a loaded Mac the scutil lookup missed its 500 ms cap during app boot, and
because the fallback was memoized for the process, every status read and the
Settings caption showed the bare hostname for the rest of the session.

The lookup now gets a 5 s timeout, a failed attempt (timeout, spawn error,
non-zero exit, empty output) clears the shared memo so a later ready() retries
after a 30 s interval, and status publishers wait only up to a 750 ms publish
budget before answering with what read() has now. A friendly name and the
non-darwin hostname stay final.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — one minor correctness note inline. A slow macOS name lookup can leave a Settings caption on the bare hostname, and one new comment says otherwise.

Reviewed changes

This run re-reviewed the single commit pushed after the prior pullfrog review at b554034, which makes a failed friendly-name lookup retryable instead of latching the hostname for the process.

  • A failed lookup is retried, not memoized — detectRuntimeMachineName now returns { name, final }, and module state keeps sharedDetectedName / lastFailedLookupAt, so a scutil timeout, empty answer, or spawn error leaves the hostname in place but re-derives the name after MACHINE_NAME_RETRY_INTERVAL_MS (30 s) on a later ready().
  • Status waits are bounded — machineNameReady() becomes readyWithin(MACHINE_NAME_PUBLISH_WAIT_MS) (750 ms) and the scutil timeout rises to 5 s, so a status reply never blocks past 750 ms; a slow lookup lands in the background and the next read carries the friendly name.
  • Tests — status-machine-name-publish-budget.test.ts proves a status answered mid-lookup publishes the hostname and then the friendly name, and runtime-machine-name.test.ts gains retry, spawn-churn-bound, final-answer, and readyWithin cases that force darwin on every platform (retiring the prior macOS-only coverage gap).

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Comment thread src/main/ipc/runtime.ts
ipcMain.handle('runtime:getStatus', (): RuntimeStatus => {
ipcMain.handle('runtime:getStatus', async (): Promise<RuntimeStatus> => {
// Why: same capped wait as `status.get`, so the renderer caption sees the friendly name unless
// the lookup is still running; it refetches status and picks the name up on the next read.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment says the renderer caption "refetches status and picks the name up on the next read", but usePublishedMachineName (use-published-machine-name.ts:16-38) issues exactly one getStatus() per override change — no timer, subscription, or visibility re-read. Because machineNameReady() now stops waiting at 750 ms while scutil may run for 5 s, a caption mounted inside that window keeps the bare hostname until the component remounts. Either have the hook re-read once the lookup settles, or drop the self-correction claim from the comment.

Technical details
# Renderer caption never re-reads after the capped status wait

## Affected sites
- `src/main/ipc/runtime.ts:56-57` — the new comment claims the caption refetches and self-corrects.
- `src/renderer/src/components/settings/use-published-machine-name.ts:16-38` — a single `getRuntimeStatus()` call inside a `useEffect` keyed only on `override`; no polling, visibility listener, or runtime subscription.
- `src/main/runtime/orca-runtime-get-status.ts:155-157` — `machineNameReady()` calls `readyWithin(MACHINE_NAME_PUBLISH_WAIT_MS)`.
- `src/main/runtime/runtime-machine-name.ts:9,114-116` — `MACOS_COMPUTER_NAME_TIMEOUT_MS = 5_000` now outlives the 750 ms publish budget.

## Required outcome
- A caption mounted while a slow lookup is in flight must not stay on the hostname after the lookup lands: either re-read `runtime.getStatus()` when the lookup settles (poll once, or subscribe), or state in the comment that the value is a best-effort snapshot that only self-corrects on remount.

## Note
- The handoff `hostLabel` getter (`orca-runtime-get-worktree-ps.ts:184`) and the CLI both read `machineName.read()` live, so only the renderer caption has this gap.

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Update: machine-wide name, one field everywhere, and a latch fix — head 4f5edbcc73

Follow-up to the status above, after the product decision that this is one name for the whole machine, not a mobile setting.

What changed since 1d3f75b201 (9 commits)

  • The field is one component backed by one saved setting, and it now appears on every surface where this computer is about to meet another device or host: Settings → General under a new "This computer" heading (its home), Settings → Mobile, the Orca Mobile page's "Pair this computer" step, Settings → Remote Servers → Share this host, the Add Remote Host dialog (SSH and Orca server), and the SSH pane's add-host form. Caption everywhere: "Other devices and hosts see “…”". Settings search lands on it. The paired web client does not show it (a browser has no machine of its own to name). Translations added in all six catalogs.
  • Two review passes over those commits fixed an empty grid row the field left behind on the web client's pairing step, and removed an Enter-key interception in the field: the shared debounced input already commits a pending draft on blur and on unmount, so Enter now submits the enclosing form like every sibling input.
  • A real regression caught in the app and fixed. Validating the new surfaces on this Mac under heavy load (load average above 40), the cold-start status reply published Brennans-MacBook-Pro.local for the whole session: the earlier half-second lookup cap fired during Electron boot and the timed-out fallback was memoized forever. Now the lookup gets five seconds, a status read waits for it only up to 750 ms (under the CLI's one-second probe) and otherwise answers with the hostname until the next read, and a failed or timed-out lookup is retried after thirty seconds instead of being remembered. Each behaviour has a test that goes red when its line is removed.

Validation in the running app (hidden renderer, fresh profiles, same loaded machine)

  • All six surfaces show and edit the same value, with settings.machineName and status.get agreeing after each edit; a rename made on the Mobile page is what the Add Remote Host dialog then shows. Screenshots are in the PR body.
  • Cold start re-checked at 4f5edbcc73 on the same loaded machine (load average 35 to 42, not waited on): the first status read 1 ms after attach returned Brennan’s MacBook Pro, and ten seconds of polling never showed the .local hostname; Settings → General showed the detected name as placeholder and caption; override to Cold Desk and clearing it round-tripped through the store and status; Enter in the SSH pane's add form now submits (validation toast "Host or SSH config alias is required"), and Enter in the sidebar Add SSH host dialog does nothing, because the field there sits above the panel's own form and has no form to submit.

Gates at the final head: typecheck clean; renderer suites 4147 tests / 0 failures; main-process runtime and IPC suites 6866 tests / 0 failures; changed-code quality gate 0 findings; oxlint, oxfmt, and all four localization verifiers clean. CI: all 27 checks green on 4f5edbcc73, first run, no re-runs.

Things to know

  • On an overloaded Mac the very first status read after launch can still carry the hostname for up to 750 ms of waiting; the next read has the friendly name, and the Settings caption picks it up the next time it fetches status.
  • Nothing on the wire changed for pairing: server pairing sends no client display name today, and the SSH path already sends the machine name as the client name, so no new field was invented.

…in this computer

The Add Remote Host dialog, the SSH pane add form, and General all describe
another machine, so a field about this computer's own name read as a third
kind of label there. The field now mounts only where other devices pair with
or connect to this computer: the Mobile pane, the Orca Mobile pairing step,
and Remote Servers > Share this host.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — one minor test-comment note inline, plus a stale-description note below.

Reviewed changes

This run re-reviewed the single commit pushed after the prior pullfrog review at 4f5edbcc.

  • The field now mounts only where other devices join this computer — f11b974 removes MachineNameField from Settings → General ("This computer"), both Add Remote Host panels, and the SSH pane add form, leaving the Mobile pane, the Orca Mobile pairing step, and Remote Servers → Share this host. The children slots on the two add-host panels, GeneralPane's showDesktopOnlySettings prop, and the GeneralPane.thisComputer i18n key in all six locales go with them.
  • The shared search catalog is rehomed — machine-name-search.ts stays the single source; the Mobile pane keeps the entry with an added mobile keyword, and the desktop Remote Orca Servers entry spreads the shared keywords so hostname/rename still land there. getGeneralPaneSearchEntries no longer includes the machine name.
  • Paired-device revocation extracted — MobilePane's inline revokeDevice moves to useMobilePairedDeviceRevocation, keeping the pane under the max-lines ratchet as it gains the field.

ℹ️ PR description still documents the removed mount sites

The description says the field's home is Settings → General under a "This computer" heading and lists the Add Remote Host dialog and the SSH pane add form among the surfaces, with screenshots and Testing steps for each. f11b974 deliberately removed those three mounts, so the reviewer-facing spec, the "every surface, same field" screenshot table, and the validation steps no longer match the code. Worth updating the description before merge so it reflects the three remaining surfaces.

Technical details
# PR description lists surfaces the new commit removed

## Affected sites
- `src/renderer/src/components/settings/GeneralPane.tsx` — the "This computer" section and its `MachineNameField` mount were removed.
- `src/renderer/src/components/sidebar/AddRemoteHostDialog.tsx` — the `children` slot carrying the field was removed from both form panels.
- `src/renderer/src/components/settings/SshTargetForm.tsx` — the add-mode `MachineNameField` was removed.
- PR body — "Settings → General under a new 'This computer' heading (its home)", the Add Remote Host / SSH pane bullets, and the six-surface screenshot table.

## Required outcome
- The description, screenshots, and Testing steps name the three remaining mount sites (Mobile pane, Orca Mobile pairing step, Remote Servers → Share this host).

## Open questions for the human
- If any removed surface is meant to come back (e.g. a rename prompt when first connecting to a remote host), the description should say so; otherwise it can be dropped.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

})

it('lets Enter submit an enclosing host form like its sibling inputs, and still commits the name', async () => {
// Why: the SSH add form mounts this field beside inputs where Enter submits. A name typed just

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test still justifies itself with "the SSH add form mounts this field beside inputs where Enter submits", but f11b974 removed that mount and no remaining mount site sits inside a <form>. The behaviour guard is harmless to keep, but the rationale and the ssh-target-machine-name id (lines 88 and 105) now point at a surface that no longer exists.

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Update: field narrowed to where other devices join this computer — head f11b974ff8

Product correction after reading the description: the machine name belongs only next to the pairing it describes, not in General and not in the flows where this app joins another machine.

What changed (one commit, f11b974ff8)

  • Removed the field from Settings → General (and its "This computer" heading, search entry, and translations in all six catalogs), from the Add Remote Host dialog in both SSH and Orca-server modes (the children slot added for it is gone too), and from the SSH pane's add-host form. Those forms describe the remote host (Label, Host or alias), so a field about this computer's own name sat next to fields about a different machine and read as a third kind of name.
  • Kept, unchanged: Settings → Mobile, the Orca Mobile page's "Pair this computer" step, and Remote Servers → Share this host. Each is a place where other devices join this machine and the name is shown right where the pairing code is.
  • The diff for the removed surfaces is exactly the PR's own earlier hunks reversed; git diff against the merge base for those files is empty.

Validation in the running app (f11b974ff8, hidden renderer, fresh profile): cold start on a loaded machine (load average 33 to 41) answered Brennan’s MacBook Pro from the first read and never showed the .local hostname over ten seconds; Settings → Mobile, Remote Servers → Share this host, and the Orca Mobile page pairing step all showed the same value before and after an override to Narrow Desk and after clearing it; Settings → General, the SSH pane's add form, and both Add Remote Host modes have no machine-name field (their forms open on Label and Host or alias). Screenshots are in the PR body.

Gates: typecheck clean; renderer settings/mobile/sidebar/i18n suites 4136 passed with 5 load-induced WorktreeCard timeouts that pass alone and are untouched by the diff; changed-code quality gate 0 findings; oxlint, oxfmt, and the four localization verifiers clean. CI: 24 checks green on f11b974ff8; two red, neither from this PR. test / tests node 24 1/8 fails on Sidebar.test.tsx "does not start a full worktree scan while the startup session is hydrating" (TypeError in repo-scan-failure.ts:54), a file this PR does not touch: that test passes at this PR's head and fails at origin/main itself (7f203ad5aa, after #22552 changed repo-scan-failure.ts), so the merge ref inherits main's breakage; other open PRs are red on the same shard and a fix/sidebar-test-detected-worktrees-fixture branch is already up. e2e / ssh docker watcher isolation is not a required check and main's own E2E run on 7f203ad5aa failed as well. verify is red only because it aggregates those. Once the sidebar fix lands on main, a re-run of this PR's checks should go green without changes here.

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Conflict resolved and CI green — head f7879e4abf

Merged origin/main (7f203ad5aa) into the branch. One conflict, in normalize-loaded-global-settings.test.ts, where main added structured-chat shell-environment tests next to this PR's machine-name test; both blocks are kept. The five production files both sides touched merged cleanly and show only this PR's own lines against main.

On the merged tree: typecheck clean; 359 tests across every file both sides touched pass; all four localization verifiers pass. CI on f7879e4abf: all 26 required checks green on the first run, including the shard that was red before the merge. Only the non-required e2e / ssh docker watcher isolation job is still running.

Ready to merge.

@brennanb2025

Copy link
Copy Markdown
Contributor Author

SSH host dialogs re-shot at the merged head f7879e4abf

Verdict for these dialogs: the machine-name field was added in the reframe and then removed once it became clear every other field there describes the remote host (Label is what you call it in your sidebar, Host or alias is the SSH address or config alias). Against the merge base these files have no diff from this PR. An SSH host still learns this computer's name, but only as the client name in its connected-clients list, which is set from the main process with no UI in the dialog.

Settings → SSH Hosts → Add Target Sidebar → Add SSH host Sidebar → Add remote server
SSH pane add target Add SSH host Add remote server

Inputs in each, in order: Label, Host or alias, Username, Port, Identity file (Add SSH host); Server name, Pairing code (Add remote server). No machine-name id in any of them. For contrast, the field where it does live:

Settings → Mobile with the machine name field

@brennanb2025
brennanb2025 merged commit 800d33e into main Sep 24, 2026
43 checks passed
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