fix: honor session metro hints and expo dev-client bundle urls#1199
Conversation
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Independent review — needs changes
Reviewed in a scratch worktree off origin/fix/metro-session-hints (commit 94ada3191), not the checked-out branches on this machine. All gates run there; live probes only on port 8083 (killed after each), ports 8081/8082 and all devices untouched.
Blocking findings
1. open's new --metro-host/--metro-port/--bundle-url/--launch-url are not actually consumed by a later metro reload — the help text promises behavior the code doesn't deliver.
The open help text (src/commands/management/app.ts:123) says: "a later plain reload in the same session reuses whichever of these were set." That's not what happens. open's hint flags fold into SessionRuntimeHints and get written to the daemon's session store via setSessionRuntimeHintsForOpen (src/daemon/handlers/session-runtime.ts, called from session-open.ts:328) — applied to the device's native dev-server prefs (Android ReactNativeDevPrefs.xml / iOS simulator plist via src/daemon/runtime-hints.ts). metro reload is local-only and never touches the daemon; its hint resolution (client.metro.reload() in src/client/client.ts:289-296) only reads resolveMetroSessionHints(config), which reads <state-dir>/metro-sessions/<session>.json — a file written exclusively by metro prepare (persistMetroSessionHints, client.ts:286,456-469). These are two disjoint storage layers that never sync.
Live repro (fresh client instances, matching real CLI invocation-per-process):
open MyApp --metro-host 10.0.0.5 --metro-port 8090
-> daemon request runtime payload: { metroHost: '10.0.0.5', metroPort: 8090, ... } ✓ (as designed)
metro reload (new client instance, same session/state-dir, no flags)
-> resolveMetroReloadUrl(...) => http://localhost:8081/reload ✗ (falls back to the hard default,
ignoring what `open` just set)
Either fix the wiring (have open's hint-setters also call writeMetroSessionHints) or correct the help text/PR description — right now item 3 of the PR ("no reload-first workaround is needed") doesn't hold for the CLI's actual metro reload path.
2. Stale session hints are never cleared — a same-name session reuse can make metro reload silently hit the wrong (or dead) dev server.
clearMetroSessionHints (src/metro/metro-session-hints.ts:56) is defined and unit-tested in isolation (metro-session-hints.test.ts:78) but has zero production call sites — confirmed by grep across src/. handleCloseCommand (src/daemon/handlers/session-close.ts:109-115) clears the daemon's SessionRuntimeHints on close via clearRuntimeHintsFromApp, but never touches the metro-session-hints file. Nothing else (idle-reap, daemon shutdown) touches <state-dir>/metro-sessions/ either — it's write-once-per-prepare, read-forever.
Live repro:
metro prepare --port 8083 ... # session "proj-foo" -> hints = {host: 127.0.0.1, port: 8083}
close --session proj-foo # hint file untouched
<project A's dev server on 8083 stops>
<an unrelated project B later starts its own dev server, happens to grab 8083>
metro reload --session proj-foo # NEW session, same name, no flags
-> resolves to http://127.0.0.1:8083/reload
-> hits PROJECT B's server, gets 200 OK, body confirms it's a different app entirely
This is exactly the failure class the PR sets out to fix, just moved to a new trigger (session-name reuse instead of "never prepared"), and it's silent — the caller gets a 200 and no signal anything is wrong. This is the "bug's original shape inverted" scenario called out for this review.
Should-fix (non-blocking)
3. Bun's current default lockfile (bun.lock, text format, default since Bun 1.2) isn't in LOCKFILE_PACKAGE_MANAGERS (src/metro/client-metro.ts:185-190) — only the older binary bun.lockb is recognized. A Bun-workspace project on the current lockfile format falls through to npm detection and can hit the same EUNSUPPORTEDPROTOCOL-class failure this PR exists to fix, just for Bun users.
4. findLockfilePackageManager's walk-up has no boundary (stops only at filesystem root, src/metro/client-metro.ts:192-204). No stop at .git or similar repo marker, so a project with no lockfile of its own could walk into an unrelated ancestor directory and silently pick the wrong package manager. Low real-world odds, but zero test coverage of the boundary case. "Nearest wins" ordering (pnpm > yarn > bun > npm per directory level) is otherwise a sound, defensible heuristic for the nested-repo case.
Notes / low severity
- Hint-file writes aren't atomic (plain
writeFileSync, no temp+rename) — low risk sincereadMetroSessionHintsalready treats a corrupt/partial file as "no hint" and falls back gracefully rather than crashing. - Pre-existing (not introduced by this PR): live-verified that
metro reload's/reloadcall against a real Expo dev server returns 200 but with the SPAindex.htmlbody, not a real Metro reload trigger — Expo's dev server doesn't appear to expose/reloadthe way plain Metro does. The reload-path logic is unchanged by this diff, but since this PR is the first to officially wire upkind=expo, worth a follow-up so callers don't believe reload worked when it didn't. fallow auditflagsresolveMetroPrepareSettings(client-metro.ts:788, cyclomatic 14) as high-complexity, but the gate itself confirms this is inherited/pre-existing, not introduced by this diff.
Verified sound
- Priority resolution (explicit flag > session hint > default) is correctly implemented and tested per-field, including mixed cases (explicit port + hinted host) —
resolveReloadMetroHost/resolveReloadMetroPort(client-metro.ts:398-422) andclient-metro.test.ts. - Expo virtual-entry bundle fix is real and live-verified against
react-navigation/example(expo dev-client, yarn workspaces) on port 8083:index.bundle→ 404UnableToResolveError,.expo/.virtual-metro-entry.bundle→ 200 with an 11 MB real bundle. - Expo-kind detection keys only on
package.json'sexpodependency (src/utils/project-runtime.ts:17-28), not on a.expo/directory's presence — a bare-RN app with a stale.expo/dir from a pastexpo runwill not misdetect. platformquery param is correctly plumbed for both android/ios through the sharedbuildBundleUrlfor both the new expo entry path and the existingindex.bundlepath.- Session-name sanitization for the hint-file path (
safeSessionName) is correct and tested against unsafe/path-traversal-shaped session names. - Exposing the four new flags unconditionally on
openfor every platform (macOS/web/tvOS included) matches existing convention — there's no per-flag platform gate anywhere in this codebase (only command-levelcapability, from #1137); precedent flags likeactivity/deviceHubare likewise accepted everywhere and silently no-op off-platform, andapplyRuntimeHintsToApp(src/daemon/runtime-hints.ts:42-61) does the same for the new metro/launch hints on macOS/web/tvOS/physical iOS. - Architecture claim holds:
metro prepare/metro reloadnever call the daemon transport (explicitly asserted and tested — a throwing fake transport inclient-metro.test.ts's session-hints test passes).
Gates (scratch worktree, isolated)
pnpm build/pnpm lint/pnpm format:check/pnpm typecheck/pnpm check:layering— all clean.fallow audit --base origin/main— 0 new issues in the 10 changed files; 1 pre-existing complexity finding correctly excluded as inherited.- PR-relevant unit tests run in isolation to avoid known process-spawn-under-contention flakiness:
client-metro.test.ts(14),metro-session-hints.test.ts(6),commands/management/app.test.ts(4),cli-help-command-usage.test.ts(38) — all 62 pass. gh pr checks 1199— 21/21 green (Coverage, Layering Guard, Lint & Format, Typecheck, Fallow, Smoke Tests ×4, CodeQL, etc.).
Verdict
Needs changes. The Expo virtual-entry fix and the package-manager walk-up (yarn-workspace case) are solid, live-verified wins — keep those. But findings #1 and #2 mean two of the PR's three stated fixes don't fully hold: the open hint-setters don't feed the reload path they're documented to feed, and the core "don't silently reload the wrong project" guarantee this PR exists to deliver has a real gap on session-name reuse. Recommend wiring open's hint-setters into writeMetroSessionHints (or at minimum correcting the help text to stop promising that), and clearing the hint file on session close the same way clearRuntimeHintsFromApp already does for the daemon-side hints.
Review follow-up (PR #1199): - open's --metro-host/--metro-port/--bundle-url now also record the session's dev-server binding in the metro-sessions file (the one local store metro reload resolves against), so a later plain reload actually reuses what open set. The daemon runtime-hints write stays for device-native dev-server prefs. - The binding now carries bundleUrl (prepare persists the local-flow bundle URL; bridge runtimes are excluded). - clearMetroSessionHints is wired: session close drops the binding (teardown intent — even when the daemon call fails), and a hintless open that creates the session clears a leftover same-name binding. The open result carries sessionReused so the client can tell fresh from reused sessions. Regression test pins prepare -> close -> flagless reload resolving to the default, never the stale port. - Package-manager detection recognizes bun.lock (text lockfile, default since Bun 1.2) and bounds the lockfile walk-up at the nearest .git entry. - Comment audit per maintainer directive: multi-line acceptability arguments trimmed to one-line constraint statements; the store's lifecycle is documented once on MetroSessionHints.
Review response — all findings addressed in 740d99aPoint-by-point: Blocker 1 (open's hint flags never fed Blocker 2 (stale hints never cleared) —
Should-fix 3 (bun.lock) — added ahead of Should-fix 4 (unbounded walk-up) — the lockfile walk now stops at the nearest Notes acknowledged:
Additionally (maintainer directive): audited the whole diff for paragraph-length acceptability arguments — the expo monorepo-root cwd note is now one line ("cwd is --project-root; monorepo-root module resolution beyond that is Expo's own behavior"), and every other comment is a one-to-two-line constraint statement. The store's lifecycle is documented exactly once, on the Gates (all green): build, lint, format:check, typecheck, layering, MCP metadata, |
|
Two remaining issues on current head
CI is green, but live evidence still does not prove that an Expo app actually reloads through this path; the current HTTP 200 probe can also be an Expo HTML response. |
Three exported-and-unit-tested-but-unreferenced-in-production incidents this week (#1166 getNearestCommandNames, #1167 buildSettleTail, #1199 clearMetroSessionHints) — the first two were caught by fallow's dead-code check because they had zero importers anywhere; #1199 was missed because a test file imports the export, and fallow's default reachability graph counts a test import as "used". Adds a second, stricter pass reusing fallow's own --production mode (entry.exclude test/story/dev files) via scripts/test-only-exports/check.ts: an export alive in fallow's default graph but dead in its production graph, with no other reference anywhere in its own file, has no production call site — exactly the #1199 shape. Ratchets against a checked-in baseline (scripts/test-only-exports-baseline.json, 77 entries); new findings fail `pnpm check:test-only-exports` (wired into CI's Fallow job and check:tooling). A `// test-seam: <reason>` comment above an export is the escape hatch for intentional test seams. Also extends .fallowrc.json's ignoreExports for seven daemon route handlers (src/daemon/handlers/*.ts) that are genuinely production-reachable through request-handler-chain.ts's `typeof import()` lazy-load pattern, which fallow's static import graph can't trace as a named-export consumer — without this they were false positives in the production-mode pass.
* ci: ratchet against test-only exports Three exported-and-unit-tested-but-unreferenced-in-production incidents this week (#1166 getNearestCommandNames, #1167 buildSettleTail, #1199 clearMetroSessionHints) — the first two were caught by fallow's dead-code check because they had zero importers anywhere; #1199 was missed because a test file imports the export, and fallow's default reachability graph counts a test import as "used". Adds a second, stricter pass reusing fallow's own --production mode (entry.exclude test/story/dev files) via scripts/test-only-exports/check.ts: an export alive in fallow's default graph but dead in its production graph, with no other reference anywhere in its own file, has no production call site — exactly the #1199 shape. Ratchets against a checked-in baseline (scripts/test-only-exports-baseline.json, 77 entries); new findings fail `pnpm check:test-only-exports` (wired into CI's Fallow job and check:tooling). A `// test-seam: <reason>` comment above an export is the escape hatch for intentional test seams. Also extends .fallowrc.json's ignoreExports for seven daemon route handlers (src/daemon/handlers/*.ts) that are genuinely production-reachable through request-handler-chain.ts's `typeof import()` lazy-load pattern, which fallow's static import graph can't trace as a named-export consumer — without this they were false positives in the production-mode pass. * fix: harden test-only-exports ratchet per review Addresses the two should-fixes and all five minors from the independent review of #1202: - Replace the regex own-file occurrence count with an oxc-parser AST walk (typescript@7 ships no JS scanner API, so the review's fallback tool suggestion is the primary): identifiers are counted as AST nodes deduped by source span, so mentions in JSDoc/block comments, strings, and template-literal text no longer masquerade as call sites (review finding 1, both constructed cases re-verified fixed), and a `//` inside a string no longer hides real usages (finding 6). Span dedupe keeps barrel re-exports (`export { x } from`) counting once. The sharper count surfaced one organic false negative on main: `selector` in src/commands/index.ts was previously exempted because the regex matched "selector" inside the './...selector-read.ts' import path string; it is now baselined alongside its sibling `ref` (same re-export line). - Make the baseline shrink-only (finding 2): --update-baseline refuses new findings with the same wire/delete/annotate message, so the `// test-seam:` annotation in the reviewed source diff is the only acceptance path; CONTRIBUTING no longer documents baseline regeneration as an acceptance option and now describes baseline growth as a deliberate manual edit. - Stale baseline entries now emit a `::warning` CI annotation (finding 3). - Commit a re-runnable fixture test (finding 4): check.test.ts mirrors scripts/layering/model.test.ts, builds a synthetic package with a clearMetroSessionHints-shaped export (JSDoc self-mention included), asserts it is flagged, and asserts the annotated twin passes; wired before the check in pnpm check:test-only-exports. - Mark the unreadable/unparseable-file fallbacks CONSERVATIVE: per CONTRIBUTING's convention (finding 5). - Document the dynamic property access (obj[name]) blind spot in the script header and CONTRIBUTING (finding 7). * fix: harden test-only export ratchet * refactor: use native Fallow export gate * chore: refresh production export baseline
- metro reload now resolves against the dev server the session's last metro prepare bound (via a per-session hint file), instead of silently defaulting to localhost:8081 and reloading an unrelated project. Explicit --metro-host/--metro-port/--bundle-url still win. - metro prepare --metro-kind expo (detected or forced) now hints the virtual-metro-entry bundle URL instead of index.bundle, which 404s against Expo dev servers in monorepos (live-verified against react-navigation's example app). Package-manager detection for --install-deps now walks up from --project-root to the nearest lockfile so Yarn/pnpm workspace monorepos don't wrongly fall back to npm and hit EUNSUPPORTEDPROTOCOL on workspace: deps; install failures now hint at --no-install-deps and the detected PM. - open now accepts --metro-host/--metro-port/--bundle-url/--launch-url as session-hint setters (folded into the same runtime object the daemon already persists for open), so a fresh session doesn't need a throwaway reload-first call just to seed hints. Updates help text for metro and open to match.
Review follow-up (PR #1199): - open's --metro-host/--metro-port/--bundle-url now also record the session's dev-server binding in the metro-sessions file (the one local store metro reload resolves against), so a later plain reload actually reuses what open set. The daemon runtime-hints write stays for device-native dev-server prefs. - The binding now carries bundleUrl (prepare persists the local-flow bundle URL; bridge runtimes are excluded). - clearMetroSessionHints is wired: session close drops the binding (teardown intent — even when the daemon call fails), and a hintless open that creates the session clears a leftover same-name binding. The open result carries sessionReused so the client can tell fresh from reused sessions. Regression test pins prepare -> close -> flagless reload resolving to the default, never the stale port. - Package-manager detection recognizes bun.lock (text lockfile, default since Bun 1.2) and bounds the lockfile walk-up at the nearest .git entry. - Comment audit per maintainer directive: multi-line acceptability arguments trimmed to one-line constraint statements; the store's lifecycle is documented once on MetroSessionHints.
… /message Maintainer review follow-ups (PR #1199): - Reload/message endpoint URLs keep the bound bundle URL's mount prefix (e.g. /tenant-42/index.bundle -> /tenant-42/reload) instead of collapsing to the host root. The Expo virtual entry (.expo/.virtual-metro-entry.bundle) is an entry-module path, not a server mount, so it maps to the server-root endpoints (verified live: Expo serves /message at the root). - When the dev server has no HTTP /reload route and answers with the app page (Expo), metro reload now broadcasts {"version":2,"method":"reload"} over the server's /message websocket (the channel dev-server CLIs use for the r key) instead of reporting the app-page 200 as a successful reload. The result carries a transport field (http | message-socket). Live-verified: a flagless metro reload against the running Expo server made the app re-fetch its JS bundle. - Endpoint resolution moved to src/metro/metro-reload-endpoints.ts so the seam tests use is production-imported (keeps the new production-unused-exports ratchet clean). - Docs reconciled: help metro and website commands.md describe the single session binding store, open's hint flags, prefix preservation, and the message-socket fallback.
thymikee
force-pushed
the
fix/metro-session-hints
branch
from
740d99a to
5e34471
Compare
Maintainer review response — all three blockers addressed in 5e34471 (rebased onto 0.19.3 main)Blocker 1 (path-prefixed reload drops the prefix) — fixed, with one correction discovered live. The old derivation only recognized While proving this live I found the deeper issue behind it: Expo's dev server has no HTTP reload route at all — any Blocker 2 (stale docs) — reconciled. Blocker 3 (live expo reload proof) — done, app actually reloaded. Against the running Metro on 8082 (react-navigation example, expo dev-client) with the playground on the free iPhone 17 sim (PagerView sim untouched), own session/state-dir: App-side proof it actually reloaded (session The app re-fetched its JS bundle from 8082 with the virtual-entry path in direct response to the plain Gates (rebased onto |
|
1 participant
Summary
Three metro-command gaps found live on 2026-07-10 while moving the RN example dev server from port 8081 to 8082.
1.
metro reloadignored session hints. Aftermetro prepare --port 8082 --public-base-url http://127.0.0.1:8082bound a session, a plainmetro reload --session <s>still hithttp://localhost:8081/reload— reloading a different project's server on the default port.metro prepare/metro reloadare local-only (they never round-trip through the daemon), so there was nowhere for session-scoped Metro coordinates to live.metro preparenow persists{ metroHost, metroPort }to a small per-session file (<state-dir>/metro-sessions/<session>.json), andmetro reloadresolves against it when no explicit flag is given. Priority: explicit flag > session hint > default (localhost:8081).2.
metro prepare --metro-kind expomisbehaved for expo dev-client monorepos, reproduced live againstreact-navigation/example(expo dev-client, yarn workspaces):bundleUrlhint used.../index.bundle?..., which 404s against the expo server ("Unable to resolve module ./index from<monorepo root>"). The working URL is the virtual entry.../.expo/.virtual-metro-entry.bundle?.... Verified live on port 8083:index.bundle→ 404, virtual-entry → 200 with an 11 MB bundle. Fixed by mappingkind=expoto the virtual-entry path.cwdalready equals--project-root(confirmed vialsof -p <pid>), so the monorepo-root module resolution is Expo's own workspace/watchFolders behavior, not an agent-device bug — documented in code and help text instead of "fixed".node_modulesis missing in a workspace using yarnworkspace:protocol, dependency install failed with rawnpm error EUNSUPPORTEDPROTOCOLbecause package-manager detection only checked the leaf project root. It now walks up from--project-rootto find the nearestyarn.lock/pnpm-lock.yaml/bun.lockb/package-lock.json, and install failures now carry a hint pointing at--no-install-depsand the detected package manager.3.
openrejected--bundle-url/--metro-host/--metro-portwithINVALID_ARGS, even though the daemon already accepts and persists aruntimehint object foropen(used for a fresh session before its first reload). Chose to add these three flags (plus--launch-url) toopendirectly as session-hint setters — they fold into the sameruntimeobject the daemon already handles, so no reload-first workaround is needed. This was the clean option:open's command definition already builds its client-call options from CLI flags in one place, so wiring these through was a small, well-contained change with no ordering blocker.Test plan
pnpm build,pnpm lint,pnpm typecheck,pnpm format:check,pnpm check:layeringall passnpx fallow audit --base origin/mainpasses (0 issues in changed files; 1 pre-existing complexity finding correctly excluded as inherited)client-metro.test.ts,metro-session-hints.test.ts,commands/management/app.test.ts,cli-help-command-usage.test.tsreact-navigation/exampleon port 8083 (killed after); did not touch ports 8081/8082 or any simulator/device