feat(mobile): add composer-first threads and daemon discovery (CODE-587) - #520
feat(mobile): add composer-first threads and daemon discovery (CODE-587)#520Zerlight wants to merge 9 commits into
Conversation
There was a problem hiding this comment.
Important
The composer-first rework and the runtime-hook extraction are clean, and the attachment-before-dispatch tests are genuinely tight (they assert transport.sent[0] is session.attach and exactly one agent.input, not just "it worked"). My one substantive question is whether the discovery half of this PR can find anything yet.
Nothing in the repo publishes _linkcode._tcp
Both new native modules browse for _linkcode._tcp, but I can't find a publisher anywhere:
- A repo-wide search across every language, plus every
package.jsonandCargo.toml, returns zero mDNS/Bonjour/NSD publishing code or dependency. - The daemon's only "advertise local discovery" path —
apps/daemon/src/index.ts:375— writesruntime.jsonto the local filesystem. That's a same-machine handoff, not a LAN advert. apps/mobile/app.jsonandapps/desktop/electron-builder.ymlmention the service type only underNSBonjourServices, which grants permission to browse, not to publish.- I also checked
origin/ruocheng/code-588(#521, the stack tip): no publisher there either.
So as far as I can tell the "Discovered hosts" section ships permanently empty. That reading lines up with the PR body's own note — "Runtime smoke used the in-process mock host, not a live remote harness or LAN discovery session" — and with the // TODO: Verify daemon identity and carry its paired capability once the advert contract exists at LinkCodeDaemonDiscoveryModule.swift:7, which implies the advert contract doesn't exist yet.
If the daemon-side advert is deliberately landing in a later, non-mobile PR, that's completely reasonable — could you confirm that and link it? I'd just want the client half not to read as a finished feature in the meantime. Nothing here needs to block on it.
Verification at this branch tip
The body says the stack tip 33bd138a was verified but this branch wasn't rerun, so I ran it here:
pnpm typecheck— passes.eslint apps/mobile packages/presentation/ui— 0 errors. 25 warnings, of which 2 are new (use-session-actions.ts:66-67,set-state-in-effect); both are false positives, since those calls sit in promise callbacks rather than the synchronous effect body. No action needed.vitest run apps/mobile packages/presentation/ui— 733 tests pass.
One note for anyone else checking out this branch: typecheck fails on a fresh checkout with Cannot find module '@linkcode/providers'. That's a stale-node_modules artifact, not a defect — the dep is correctly added to both apps/mobile/package.json and pnpm-lock.yaml. Re-linking the workspace package fixes it.
Minor, non-blocking
apps/mobile/modules/linkcode-daemon-discovery/LICENSEstill carriesCopyright (c) 2015-present 650 Industries, Inc. (aka Expo)— thecreate-expo-module --localscaffold default. Probably wants to be ArcBox's.- Once a publisher does exist, the Swift TODO at line 7 becomes security-relevant: any host on the LAN answering
_linkcode._tcpwill render as a tappable daemon with no identity check. Worth making sure that lands with the advert contract rather than after it.
Everything I flagged inline is verified against the real source rather than inferred from the diff; details are in each comment.
Claude Opus | 𝕏
| export function formatDiscoveryUrl(host: string, port: number): string | undefined { | ||
| const normalizedHost = unbracketHost(host.trim().replaceAll(rTrailingDot, '')); | ||
| const urlHost = normalizedHost.includes(':') | ||
| ? `[${normalizedHost.replaceAll(rUnescapedScope, '%25')}]` |
There was a problem hiding this comment.
This %25 escaping can never change the outcome — it's dead code.
I checked against whatwg-url-minimum, which is the exact URL Expo 57 installs on native (expo/src/winter/runtime.native.ts:21), so this is the real runtime behavior and not just Node's:
[fe80::1%en0] -> THROWS TypeError
[fe80::1%25en0] -> THROWS TypeError
[fe80::1] -> OK, hostname "[fe80::1]"
The WHATWG IPv6 parser rejects zone IDs outright, escaped or not. So any host containing % returns undefined regardless of whether this replaceAll runs.
The rejection itself is clearly intentional — daemon-discovery.test.ts:12 asserts formatDiscoveryUrl('fe80::1234%en0', 19523) is undefined. My concern is only that this branch reads as though scoped IPv6 is supported, which sent me looking for a bug that isn't here. Dropping rUnescapedScope and the replaceAll would make the intent match the test.
Worth flagging the downstream consequence though: iOS hostName() returns address.debugDescription for IPv6, which carries the %en0 scope for link-local addresses. toDiscoveredDaemon then returns undefined and use-daemon-discovery.ts:35 skips it silently. A daemon reachable only over link-local IPv6 — common on a LAN with no IPv4 — would vanish with no diagnostic. A one-line log on the dropped-host path would make that debuggable.
|
|
||
| private fun retryStop(listener: NsdManager.DiscoveryListener) { | ||
| if (discoveryListener !== listener) return | ||
| handler.postDelayed({ requestStop(listener) }, 500) |
There was a problem hiding this comment.
This retry loop has no attempt counter and no deadline, and the wedged state it can reach is unrecoverable within the process.
The cycle is onStopDiscoveryFailed (line 156) → retryStop → postDelayed(requestStop, 500) → and requestStop's RuntimeException catch routes straight back to retryStop (line 206). Nothing bounds it.
What makes that costly is that completeStop (line 215) is the only nominal path that does both of these:
discoveryListener = null
releaseMulticastLock()So while the loop spins: discoveryListener stays non-null, which means startDiscovery's discoveryListener != null guard makes every subsequent start a no-op that only re-emits the stale snapshot — discovery is off for good. And the MulticastLock is held the whole time, which is a battery cost and can interfere with other apps' multicast reception.
This needs a genuinely broken NSD stack to trigger, which does happen on some OEM builds, but the failure mode is silent and permanent rather than degraded. Capping the attempts (or a bounded overall deadline) and calling completeStop on exhaustion would bound the damage to "discovery stopped" instead of "discovery stopped and the lock is still held."
| } | ||
|
|
||
| val nsdManager = manager ?: return | ||
| isResolving = true |
There was a problem hiding this comment.
isResolving is set here but has no timeout backstop — it's cleared only inside onResolveFailed (248), onServiceResolved (256), and the two exception catches (279, 284).
The deprecated callback form of resolveService is documented to occasionally drop calls without invoking either callback. If that happens, isResolving stays true and resolveNext's guard at line 227 makes every later call return immediately, so the serial queue stalls and newly-found services enqueue but never resolve. The user sees a host list that just stops updating.
To be fair to the current code, this does self-heal: startDiscovery resets pendingServices and isResolving at lines 105-106, so a background/foreground cycle recovers it. That caps the severity at "stale until the app is backgrounded," not a permanent wedge. Still, a bounded postDelayed that clears isResolving and calls resolveNext if neither callback has fired would keep discovery live without the user having to do anything.
|
|
||
| OnAppEntersForeground { [weak self] in | ||
| self?.queue.async { [weak self] in | ||
| guard self?.isObserved == true else { return } |
There was a problem hiding this comment.
Minor consistency point against the Android sibling: this guard checks only isObserved, and OnAppEntersBackground (line 47) calls stopDiscovery() without recording that it backgrounded. So the iOS module keeps no notion of foreground state.
The Kotlin module tracks isInForeground alongside isObserved and gates its restart on both (completeStop, line 219). The consequence of the asymmetry is that if OnStartObserving ever fires while the app is backgrounded, iOS starts a NWBrowser in the background where Android would not — wasted radio, and it's the state most likely to hit the local-network policy denial you already handle.
In practice the listener is added from useFocusEffect, which normally implies foreground, so I couldn't construct a definitely-reachable path to it — treat this as a symmetry nit rather than a known bug. I mention it mainly because the two modules are meant to be behavioral mirrors, and a reader comparing them will wonder which one is right.
For what it's worth, I did have the iOS connection lifecycle audited separately: cleanup across success / failure / 5s-timeout / stopDiscovery mid-resolve is correctly guarded by the connections[id] === connection identity check, and the timeout-vs-completion race is safe because both hops land on the same serial queue. That part looks solid.

Summary
Replace the new-thread sheet with a composer-first page and share start-option presentation with live sessions. Expose account-backed model, effort, and approval controls, add native LAN daemon discovery on iOS and Android, and keep draft/session orchestration in the mobile runtime.
The first message waits for the session attachment before dispatch; failed creation or dispatch preserves the draft for retry. Only explicit start-option picks override harness defaults.
Part 2 of the mobile stack:
master→ #519 → #520 → #521. Depends on #519; base:ruocheng/code-581. Review and merge after #519.Refs CODE-587.
Verification
33bd138a); this lower branch was not independently rerun.Checklist
pnpm check:ciandpnpm testpass at the complete stack tip