Show device creation and boot progress outside the dialog - #109
krystofwoldrich-agent wants to merge 2 commits into
Conversation
krystofwoldrich-agent
left a comment
krystofwoldrich-agent
left a comment
There was a problem hiding this comment.
🤖 This is an automated review. Addressing it doesn't guarantee a merge.
Review: Show device creation and boot progress outside the dialog
Verdict: REQUEST_CHANGES (posted as a comment because the reviewer account owns this PR)
Findings: 1 critical · 3 design · 3 suggestion · 2 nit
PR 2/2 in the stack; this review covers only the commit on top of #108 (b8f12e8a..0e5a4b6a). The background-start flow is well structured: createDeviceStarter owns the operation, the store reconciles local rows against discovery, and the dialog no longer owns the result. The tests cover the main races (dismiss/reopen, Android serial reconciliation, retry of create vs. boot). One reconciliation rule is wrong: a failed local entry is kept forever and shadows the real device when discovery later reports it running, which blocks streaming until the user clicks "Try again". The remaining findings concern the shared data model, sticky failure rows, and placeholder ids in the URL. Like #108, this PR adds no .changeset/*.md entry.
Findings
- [critical] A failed startup entry hides the same device once it comes online.
packages/expo-device-hub/src/dashboard/deviceSessionStore.ts:70 - [design] Browser-only lifecycle state (
startup) is added to the sharedDevicemodel.packages/@expo/hub-components/src/dashboard/data.ts:57 - [design] Failed rows cannot be dismissed.
packages/@expo/hub-components/src/dashboard/PhoneFrame.tsx:296 - [design] Placeholder
pending-…ids are written to the URL and browser history.packages/expo-device-hub/src/dashboard/startDevice.ts:74 - [suggestion] No upper bound for "Booting…" after HTTP success.
packages/expo-device-hub/src/dashboard/deviceSessionStore.ts:69 - [suggestion] Integration test in hub-components imports expo-device-hub source.
packages/@expo/hub-components/src/__tests__/device-start-dialog.test.tsx:12 - [suggestion] The 400 response does not mention the new
bootfield.packages/expo-device-hub/src/server/device-actions.ts:101 - [nit]
entry.requestId = failed.requestIdmutates the entry afterdevice.idwas built from the newrequestIdon line 48, so the store key and thepending-…suffix no longer match. It works, but the alias chain is harder to read. Compute the key before building the device, or leave a comment.packages/expo-device-hub/src/dashboard/startDevice.ts:72 - [nit]
maxHeight: '35cqh'resolves against the nearestcontainer-type: sizeancestor. The framedscreenStylesets that (deviceFrame.ts:196), but the unframed branch (PhoneFrame.tsx:166-172) does not, and the wrapper on line 122 isinline-sizeonly. In that pathcqhfalls back to the viewport height.packages/@expo/hub-components/src/dashboard/PhoneFrame.tsx:291
Details are in the inline comments.
Tests run: bun test on the five PR test files in a scratch copy of the branch (dependencies installed, hub-android-utils, hub-apple-utils and hub-client built): 30 pass, 0 fail. A one-off repro test for the critical finding failed as described (selectedAvailable stays false after a running snapshot). tsc --noEmit did not complete in the copy because other workspace packages lack dist output; not attributable to the PR.
| const retained = Object.values(previous.startups).filter( | ||
| (entry) => | ||
| entry.pending || | ||
| entry.device.startup?.phase === 'failed' || |
There was a problem hiding this comment.
[critical] A failed startup entry hides the same device once it comes online.
The retained filter keeps every entry whose startup.phase === 'failed', regardless of running. displayed (lines 80-83) and withLocalDevices (line 99) then remove any running device that matches the local entry by id or by Android AVD name, and selectedAvailable (line 94) is false because the selected device carries startup. Result: the sidebar shows "Boot failed", the frame shows the error, and useActiveDeviceClient in Dashboard.tsx:190-195 never connects, even though discovery lists the device as booted.
This is reachable in normal use. bootAndroidHubDevice returns ok: false after BOOT_READY_TIMEOUT_MS (server/device-actions.ts:374), and a slow emulator can come online after that. For a recent Android device the local id is the AVD name (server/devices.ts:110), so the name-based matches clause hides the emulator's serial row. The same applies to iOS when simctl boot fails for a device that is already booted. The only exit is "Try again", which then short-circuits through alreadyRunning (startDevice.ts:110-119).
Reproduced in a scratch copy: start a recent device, resolve boot with an error, then push a snapshot with { ...iphone, booted: true }. selectedAvailable stays false and simulators[0].startup.phase stays 'failed'.
Fix: drop a failed entry when running contains a match (id, or Android name), the same rule already applied to completed entries on lines 71-73. Add the scenario as a test in startDevice.test.ts.
| */ | ||
| lastUsedAt?: number; | ||
| /** Present only on locally requested devices while starting or after a failure. */ | ||
| startup?: DeviceStartup; |
There was a problem hiding this comment.
[design] Browser-only lifecycle state is added to the shared Device model.
Device is the data shape shared with the website (AGENTS.md: components were extracted "so the website can consume the same code"). #108 modelled viewer-local state as separate props (offlineDeviceId on DeviceSection, available on PhoneFrame / StreamPanel, offline on DeviceListItem). This PR instead embeds startup in Device, so DeviceTitle, PhoneFrame, DeviceSection and DeviceListItem now branch on a field that only the Hub dashboard ever populates.
The field also leaks into the retry target: createdDevice at startDevice.ts:99-107 includes startup: { phase: 'booting' } and becomes target.device, while the success path at line 131 strips it. So AddDeviceTarget.device sometimes carries a startup value that this comment says is "present only on locally requested devices".
Either keep the field and document it as dashboard-owned (and strip it consistently at startDevice.ts:107), or pass startup state as a prop keyed by id, following the offlineDeviceId precedent.
| }}> | ||
| {message} | ||
| </p> | ||
| {failed && onRetry && ( |
There was a problem hiding this comment.
[design] Failed rows cannot be dismissed.
A failed entry is retained until the same target is started again (startDevice.ts:59-72) or the page reloads. "Try again" is the only control. For a failed create with a placeholder id (pending-…) there is no matching recent, so the row is a permanent ghost in the sidebar. Example: creation fails because the AVD name already exists; retry repeats the failure; the user cannot remove the row.
Add a removeStartup(requestId) store action and a "Dismiss" control next to "Try again". This also gives the user an exit for the critical finding in deviceSessionStore.ts:70 until it is fixed.
| ); | ||
| if (failed) entry.requestId = failed.requestId; | ||
| store.getState().putStartup(entry, true); | ||
| navigate(device.id); |
There was a problem hiding this comment.
[design] Placeholder ids are written to the URL and browser history.
navigate(device.id) pushes /device/pending-<hex> (id built on line 48). Within the session the alias map redirects it (DeviceDiscovery.tsx:15-19). After a reload, or when the URL is shared, the id is unknown. unknownDevice (deviceSessionStore.ts:43-55) then guesses android for any non-UUID id and renders a "Device / pending-…" offline row, also for an iOS creation. History entries from publish (line 82) and from retries (this line again, with a new id) accumulate.
Options: select the placeholder in the store only and navigate when a real id exists; or treat an unresolvable pending- route as no selection in DeviceDiscovery.
| (local.id === device.id || (local.platform === 'android' && local.name === device.name)); | ||
| const retained = Object.values(previous.startups).filter( | ||
| (entry) => | ||
| entry.pending || |
There was a problem hiding this comment.
[suggestion] No upper bound for "Booting…" after HTTP success.
After boot resolves, the entry has pending: false and startup: booting (startDevice.ts:121-132). It is retained until running contains the id (lines 71-73). If discovery never lists the device, the row shows "Booting…" indefinitely and "Try again" is not offered. iOS discovery filters simulators by lastUsedAt !== undefined (server/devices.ts:95), so listing depends on usage history being present. Consider converting the entry to failed after a timeout with a clear message, so the user has a retry path.
| import { PhoneFrame } from '../dashboard/PhoneFrame'; | ||
| import { DeviceTitle } from '../dashboard/DeviceTitle'; | ||
| import { DeviceSection } from '../dashboard/DeviceSection'; | ||
| import { createDeviceStarter } from '../../../../expo-device-hub/src/dashboard/startDevice'; |
There was a problem hiding this comment.
[suggestion] Lines 12-14 import ../../../../expo-device-hub/src/dashboard/{startDevice,deviceSessionStore,deviceActions}. This inverts the package dependency: hub-components is the shared library and should not depend on the dashboard. tsconfig.json includes src, so tsc --noEmit in hub-components now compiles expo-device-hub source with hub-components' compiler options, and bun test in this package alone requires the sibling package. Move the first test to expo-device-hub/src/dashboard/__tests__/ (which already depends on hub-components), and keep the modal tests here with a fake onAdd. Same concern as raised on #108.
| !isNonEmptyString(name) || | ||
| !isNonEmptyString(runtime) || | ||
| !isNonEmptyString(deviceType) || | ||
| (boot !== undefined && typeof boot !== 'boolean') || |
There was a problem hiding this comment.
[suggestion] A body with boot: "false" is rejected here, but the route answers Expected { platform, name, runtime, deviceType } JSON body (server/index.ts:168). The message does not say which field failed or that boot must be a boolean. Update the message, or return a field-specific error.
Co-authored-by: Krystof Woldrich <31292499+krystofwoldrich@users.noreply.github.com> Co-authored-by: Codex <codex@openai.com>
Co-authored-by: Krystof Woldrich <31292499+krystofwoldrich@users.noreply.github.com> Co-authored-by: Codex <codex@openai.com>
0e5a4b6 to
3fb28d7
Compare
1 participant
Boot and Create now immediately add and select a local device. Users can close the dialog while the operation continues; the sidebar and phone frame show Creating… or Booting…, then connect when discovery confirms the device is online. Failures remain in the frame with the error and a Try again button.
Creation and boot are separate steps so a failed boot retries the existing device. The create API accepts optional
boot: falsewhile preserving its default create-and-boot behavior. Local-to-real device IDs are reconciled without duplicate Android rows or navigating away from another selected device. A dismissed dialog's response cannot close a newly opened dialog.Depends on #108 and includes its commits. Merge #108 first, then rebase this PR onto main. Both target main because the branches live in a fork and cannot use GitHub stacks here.
Base PR: URL selection and offline views · Focused diff for this follow-up
Validation: full Device Hub build; 296 passing dashboard, server, and component tests; component typecheck and lint;
git diff --check. Tests cover immediate state updates, dismiss/reopen races, creation and boot failures, retries, Android ID changes, discovery reconciliation, and render isolation.