GTA-style smartphone for the player character

[iptoux]

The main character gets an iFruit-style smartphone (plan in docs/plans/smartphone.md):

• Contacts = workers on the floor (needs-you first), opened with J or the 📱 HUD entry.
• Call = dial UI with double ringback, then goToWorker + openWorkerTerminal (workers have no voice peer, so no WebRTC).
• SMS = existing worker.prompt message with a phone-thread skin; threads persist in the browser, recents per floor.
• No new wire protocol or server handler; self-contained features/smartphone/ module, one-line registry joins, J help row, synthesized ring/swoosh/blip, unit tests, docs.

Verified: npm run typecheck, npm test (611 pass), npm run build, headless-browser screenshots of contacts / SMS thread / call screen (via src/client/lab/phone.html).

[iptoux]

Review: PR #299 — GTA-style smartphone for the player character

Clean, well-scoped PR: self-contained features/smartphone/ module, no new wire protocol, pure helpers with unit tests. No blockers. Verdict: approve once items 1–5 are addressed; items 6–13 are nits that can land as follow-ups.

Findings

• [Security] src/client/features/smartphone/ui.ts:100,146,217,273 — w.color is interpolated into style="background:..." via setAttribute at four call sites. Peer colors are COLOR_RE-checked server-side today, but nothing at these call sites constrains the value, so a ; in a color becomes arbitrary CSS declarations (e.g. background-image:url(...) phone-home). Fix: validate against /^#[0-9a-fA-F]{6}$/ with a fallback, or assign el.style.backgroundColor.
• [Security] src/shared/smartphone.ts:57 (written from src/client/features/smartphone/ui.ts:262) — SMS bodies (raw agent prompts, which routinely contain code/secrets) are persisted in plaintext localStorage, surviving session/logout and readable by any JS on the origin. Fix: don't persist message text (keep only recents metadata), or make thread persistence opt-in with a visible clear control.
• [Correctness] src/client/features/smartphone/ui.ts:193 — Lost-worker guard misses lost && !worktree: a worker whose multi-repo workspace is gone has lost set with no worktree, falls through, logs a call recent, teleports (closing the phone), then openWorkerTerminal → fixLostWorktree silently no-ops (if (!w.lost || !w.worktree) return, features/workers/actions.ts:222). The user ends at the desk with no terminal and no explanation. Fix: branch on now.lost alone (the pattern in features/waiting/index.ts:117) and toast when there is nothing to rebuild.
• [Correctness] src/client/features/smartphone/ui.ts:198-201 — Failed calls are logged as placed: logRecent(kind: 'call') runs before goToWorker(), so when it returns false (between floors, desk gone) a phantom "outgoing call" stays in Recents. Fix: move the logRecent/emit after the successful goToWorker check.
• [Correctness, Security] src/shared/smartphone.ts:49 (crashes at src/client/features/smartphone/ui.ts:274) — loadThreads checks Array.isArray(v) but not entry shape, so a crafted or stale localStorage entry (null, strings) throws on m.dir/m.text and bricks the phone until storage is cleared. Fix: keep only well-formed {dir: 'out'|'note', text: string, at: number} entries on load (and cap text length).
• [Correctness] src/client/features/smartphone/ui.ts:254-258 — SMS submit silently discards text: the thread view never redraws on store updates (by design, onStore skips thread), so if the worker falls asleep while the thread is open the hint/composer go stale and the guard returns with no feedback. Fix: toast on refusal, and/or live-update the hint/disabled state without a full redraw.
• [Security] src/client/features/smartphone/ui.ts:249 — SMS <input> has no maxlength while the server truncates at 20000 (src/server/ws/handlers/workers.ts:119); oversized pastes silently truncate mid-instruction and burn tokens. Fix: add maxlength matching the server cap.
• [Performance] src/shared/smartphone.ts:57 (called from src/client/features/smartphone/ui.ts:262) — every SMS serializes the entire thread map to localStorage, and keys (floor/worker) accumulate forever: per-thread caps exist but nothing bounds the thread count, so stringify + write grows without bound. Fix: persist only the touched key, or prune keys for floors/workers that are gone / cap total keys.
• [Correctness] src/client/features/smartphone/ui.ts:281-286 — Back/tabs during calling silently cancel the pending connect: any view switch runs draw() → clearTimers(), killing the CONNECT_MS timeout after the ring already played, with no feedback. Fix: route Back during calling through the End-button path (back to actions), and/or ignore tab switches while calling.
• [Performance] src/client/features/hud/index.ts:55 — the badge count runs waitingInOrder(store.workers.values()).length on every HUD refresh, allocating and sorting the list just for a .length. Fix: reuse the existing waitingNow() helper or count without sorting.
• [Performance] src/client/features/smartphone/ui.ts:52 (via onStore at :290) — every workers/smartphone event re-runs byUrgency (two sorts, several copies) and replaceChildrens the whole contact list, even when only recents changed or the visible tab is unaffected. Fix: redraw only the visible list, or extend the existing thread/calling skip-guard to events that can't affect the current view.
• [Correctness] src/client/features/smartphone/ui.ts:124-135 — Recents go stale: names are snapshotted at log time (a renamed worker shows its old name), and rows for sent-home workers are never pruned (tap → "went home" toast, row stays until 20-cap eviction). Fix: resolve display names from store.workers at render and drop rows whose worker is gone.
• [Simplicity] src/client/features/smartphone/sound.ts:9 — the unlock / ctx / resume / count boilerplate is triplicated verbatim across phoneRing, smsSwoosh and dialBlip. Fix: extract a small withCtx(name, fn) helper and pass the three recipes through it.

[iptoux]

All 13 findings addressed in 880e550 (verified: typecheck, 614 tests, build, fresh screenshots of contacts/thread/call/recents):

Blockers

1. Color CSS injection — new dotColor() in features/smartphone/logic.ts: plain ^#[0-9a-fA-F]{6}$ only, #888888 fallback; used at all four dot sites, with unit tests.
2. Plaintext SMS in localStorage — documented as browser-local like terminal scrollback, plus a visible 🗑 Clear message history control in Recents (wipes memory + storage).
3. lost && !worktree — connect now branches on lost alone: toast + stay on the phone when there is nothing to rebuild; otherwise close + fixLostWorktree. The thread view shows a Fix it view instead of a composer for lost workers (parity with prompting at the desk).
4. Phantom recents — logRecent/emit moved after the successful goToWorker check.
5. loadThreads shape — keeps only well-formed {dir, text, at} lines, text capped at the server's 20000; covered by tests (incl. crafted/stale entries).

Nits
6. SMS refusal now toasts; the thread hint/composer follow the worker live (no redraw, input keeps focus).
7. Composer has maxlength=20000 matching the server's str(msg.prompt, 20000).
8. Persisted threads capped (50 keys, quietest evicted, empties dropped); per-thread cap kept.
9. Back/tabs/End during calling all go through one hangUp() path back to actions — no silent cancel.
10. HUD badge reuses the existing waitingNow() helper.
11. draw() split into drawTabs()/drawBody(); smartphone-topic events only touch tab counts outside Recents.
12. Recents resolve names live from store.workers and prune rows whose worker went home.
13. Sound boilerplate extracted into ready(a, name).

Two environment notes (no product impact): the lab screenshot page emulates a speech-less browser because headless Chromium crashes in SpeechRecognition.available(); and port 5173 belonged to another project's vite, so shots ran on 5199.

[iptoux]

Review: PR #299 — GTA-style smartphone

Verdict: request changes. Fix the __proto__ thread-key write, the lost-worker call/SMS paths (phantom recents, texting/fix-flow races), and the thread-persistence shape (top-level guard + in-memory cap + quota-sized caps) before merge. The rest is follow-up cleanup.

• [Security] src/shared/smartphone.ts:63 — loadThreads() assigns parsed-JSON keys straight into a plain object, so a crafted localStorage entry with "__proto__" mutates the prototype instead of creating a thread. Skip __proto__/constructor/prototype keys (or build with Object.create(null)); same guard for the Object.fromEntries eviction path in saveThreads().
• [Correctness] src/client/features/smartphone/ui.ts:246 — startCall logs the call to recents before openWorkerTerminal runs, so a worker going lost/removed in between leaves a phantom call despite the "no phantoms" comment. Re-check lost/existence after goToWorker and only then logRecent, or log after a successful terminal open.
• [Correctness] src/client/features/smartphone/ui.ts:322 — SMS submit checks gone/asleep but not lost, so a worktree deleted between render and send still fires worker.prompt at a worker that can't act. Check now.lost and route to the fix-worktree flow instead of sending.
• [Correctness] src/client/features/smartphone/ui.ts:365 — liveThread only follows asleep/wake flips; a worker turning lost while its thread is open keeps the composer instead of switching to the "Fix it" UI. Detect w.lost in liveThread and draw() the lost branch.
• [Correctness] src/client/features/smartphone/ui.ts:201 — "Go to desk" calls goToWorker unconditionally, walking to a lost worker's desk instead of the fix flow Call/SMS/promptAtDesk use. Check w.lost and call fixLostWorktree (or block with toast).
• [Performance & Simplicity] src/shared/smartphone.ts:57-73 + src/client/features/smartphone/ui.ts:330-335 — every SMS saveThreads() stringifies the whole map (up to MAX_KEYS(50) × MAX_THREAD(100) × MAX_SMS_TEXT(20000), far over the ~5MB quota) synchronously on the send path. Lower the caps and/or persist per-thread/debounced instead of full-map-per-send.
• [Correctness] src/shared/smartphone.ts:53 — loadThreads runs Object.entries(parsed) without checking parsed is a plain object; stored []/"hi"/[[valid]] yields bogus "0"/"1" thread keys. Early-return {} unless parsed is a non-null, non-array object.
• [Correctness] src/shared/smartphone.ts:72 — saveThreads evicts to MAX_KEYS only for storage; the in-memory store.smartphone.threads keeps growing since the evicted copy is never assigned back. Return the kept map and assign it, or cap in-memory threads the same way.
• [Correctness] src/client/features/smartphone/ui.ts:134 — renderRecents mutates st.recents = alive during render with no store.emit, and drawTabs (ui.ts:59) already read the pre-filter length, so the tab count is stale one draw and other listeners miss the drop. Emit after filtering, or filter outside render.
• [Correctness] src/client/features/smartphone/ui.ts:158 — "Clear message history" clears threads but leaves sms recents pointing at now-empty threads. Also drop sms recents (or all recents) on clear, or rename the button to "clear threads".
• [Performance & Simplicity] src/client/features/smartphone/ui.ts:377-387 — every workers event triggers a full draw() → replaceChildren() rebuild of the contacts/actions list, and worker activity ticks fire often, so the open phone churns DOM constantly. Throttle/coalesce (rAF) and skip redraw when membership + sort order + visible fields are unchanged.
• [Performance & Simplicity] src/client/features/smartphone/ui.ts:52-80 — draw() sorts twice per paint: drawTabs() runs byUrgency(store.workers.values()) just for contacts.length, then drawBody() runs it again. Compute once in draw() and pass down; use store.workers.size for the tab count.
• [Performance & Simplicity] src/client/features/hud/index.ts:48,55,114-122 — waitingNow() (= waitingInOrder: copy + filter + sort) is re-evaluated ~6× per HUD refresh (count, icon, shown, status, chip, on, tone). Compute once per refresh and reuse.
• [Performance & Simplicity] src/client/features/smartphone/ui.ts:32-35,259,350 — one timers[] array mixes the Dialing→Ringing phase timer with the 2600ms connect timer, so any redraw silently re-times the phase. Track the connect handle separately, and track (or drop) the setTimeout(() => input.focus(), 30) so it can't fire after close/view change.
• [Performance & Simplicity] src/client/features/smartphone/ui.ts:316 — renderThread builds a fresh dictateField(input) (→ dictation() + listeners) on every render, including right after each send, without drop() on the old one. Create once per thread view and dispose on close/redraw.
• [Performance & Simplicity] src/shared/smartphone.ts:64-69 — eviction recomputes latest(k) (a full reduce over up to 100 msgs) inside the sort comparator → O(K²·T). Precompute latest once into a Map before sorting.
• [Performance & Simplicity] src/client/features/smartphone/ui.ts:29-400 — openSmartphone is a ~370-line closure holding 5 views, timers, live patching, and both store subscriptions; the plan (§8) said to split past ~350 lines. Split into contacts.ts / call.ts / sms.ts (or extract tab/body/draw helpers).
• [Correctness] src/client/features/smartphone/logic.ts:42 — statusNote default treats any unknown future status as asleep ("wake it before texting"). Default to a neutral delivered/unknown line instead.

[iptoux]

Second round addressed in e8143d7 (verified: typecheck, 616 tests, build, fresh screenshots of contacts/thread/call/recents):

Storage ()

• One localStorage entry per thread: sending a text writes one small entry, never the whole map; caps lowered (20 keys, 50 msgs).
• __proto__/constructor/prototype keys skipped on load, save and prune (single-key writes can't mutate a prototype; map building filters too).
• Top-level guard: non-array values (stored [], strings, truncated JSON) are dropped from storage, not rendered.
• Eviction precomputes latest once per key (no more O(K²·T) comparator); over-cap and malformed keys are deleted from storage on load.
• In-memory map capped the same way via pruneThreadKeys (assigned back on every send).

Lost races + phantoms (, , )

• Connect re-checks existence/lost after goToWorker and logs only connected calls.
• SMS submit checks lost (fix flow, or toast when nothing can be rebuilt); the live thread switches to the Fix-it UI when a worker turns lost; Go-to-desk routes lost workers to the fix flow.
• pruneRecents runs at the top of every draw() (no render-time mutation, no stale tab counts); clearing also drops sms recents.

Churn (, )

• draw() split into tabs/body, sorts once; workers events coalesce per rAF and skip when the visible signature is unchanged; smartphone-topic events only touch tab counts outside Recents.
• HUD waiting list memoized per workers change (badge, chip, tone… read it ~6× per refresh).
• Connect timer tracked separately from view timers; focus timer tracked; mic created once per thread view and drop()ed on the way out.
• statusNote defaults to a neutral line for unknown statuses; shell split into ui.ts + contacts.ts + call.ts + sms.ts per the plan's size rule.

[iptoux]

Correction to the headings above — the file paths were eaten by shell interpolation. They should read:

Storage (src/shared/smartphone.ts)
Lost races + phantoms (call.ts, sms.ts, contacts.ts)
Churn (ui.ts, hud/index.ts)

Content of the comment is otherwise as intended.

[iptoux]

Verified e8143d7 (second-review fixes) in a clean worktree: npm run typecheck passes, npm test 616/616 pass, npm run build passes, all new files under the 600-line budget (ui.ts 197).

Checked each finding from the request-changes review against the new code:

• __proto__ keys: guarded by isKeySafe on load, save and prune paths.
• Non-map payloads: per-key parse with try/catch, malformed entries dropped from storage too.
• Quota: per-thread keys (one small write per text), caps 50/thread and 20 threads in memory and storage.
• Phantom recents: call re-checks existence/lost after the walk-over, logs only connected calls.
• Lost races: SMS submit, go-to-desk and live-thread all route to the fix flow; lost-while-open switches to Fix-it UI.
• Render-time mutation gone (pruneRecents up front); clear-history also drops sms recents.
• Workers-event churn: rAF-coalesced + skipped when visible fields unchanged; single sort per draw; HUD waiting list memoized.
• Timers split (connect vs view vs focus); back/tabs/End share one hang-up path; mic created once per thread view and dropped.
• statusNote default is neutral for unknown statuses.

One non-blocking nit I noticed while verifying (not in the review): contacts.ts:38 still gates the PR badge on w.pr && w.activity, while contactSub shows PR #n when there is no activity — so the badge disappears exactly when it is the only signal. Suggest w.pr ? … : null. Fine as a follow-up.

[iptoux]

Badge nit fixed in 953a5a3: contactSub no longer mentions PRs at all, and both the contact rows and the actions view now always show the w.pr badge when set — one place for work-standing, one (the sub line) for activity. Verified: typecheck, 616/616 tests, fresh contacts screenshot (Mochi row shows the badge, no sub line).

On merge: leaving that to you — per repo rules I only merge webdevcody's PRs (others only when linked, after a security review), and this one has neither, so I won't press the button myself.

[iptoux]

Review: PR #299 — GTA-style smartphone for the player character

Summary and verdict

Solid, well-scoped PR: self-contained features/smartphone/ module, no new wire protocol,
pure helpers with unit tests, storage handling done carefully. Not green yet — changes
requested. The panel found one real UI bug (phone renders a blank body when a worker
vanishes mid-view), a stale-contacts skip, and a handful of performance nits plus minor
correctness polish. [Security] reports no findings.

Findings

• [Correctness] src/client/features/smartphone/ui.ts:143 (drawBody) — blank phone body
  when a worker vanishes mid-view. renderActions/renderCalling/renderThread handle a
  missing worker via phone.go({t:'contacts'}) (which synchronously re-draws and paints the
  fallback) and then return []; the outer drawBody then runs body.replaceChildren() with
  nothing and wipes the just-painted fallback. Reachable: phone open on a worker's actions or
  thread, worker sent home → workers event → draw() → empty body (recovers on tab tap).
  Fix: snapshot view before rendering in drawBody and skip replaceChildren when the
  renderer redirected.
• [Correctness] src/client/features/smartphone/ui.ts:110 (sig) — stale contacts when a
  worker turns lost. sig covers id/name/color/status/activity/pr but not lost (nor
  kind), so onWorkers skips the redraw and the row keeps its old sub-line; the 🌿 fix-flow
  only appears on the next tap/draw. Fix: include lost (and kind) in sig.
• [Performance] src/client/features/smartphone/sms.ts:73 (renderThread) — renders all 50
  capped messages at full 20k chars each into the DOM at once; a long thread builds a huge node
  tree on every open and janks scroll. Render a window (e.g. last 20 with a "show earlier"
  expander) or truncate long bubbles with expand-on-tap.
• [Correctness, Performance] src/client/features/smartphone/sound.ts:20 (phoneRing) —
  ringback can't be silenced. Both rings' oscillators are scheduled fire-and-forget with no
  stop handle, so End/hang-up/close mid-dial still plays out (~1.5s) with no call attached and
  holds osc+gain nodes for nothing. Fix: return a stop handle (or schedule ring 2 via the
  connect/view timer) and stop it in hangUp/clearConnect/onClose.
• [Performance] src/client/features/smartphone/ui.ts:75 (sig()) — the redraw skip-check
  joins the full activity text of every worker, rebuilding a large string on frequent
  activity ticks just to decide not to redraw. Sig over cheap stable fields only (id/status/
  color/pr number + activity length or a truncated prefix).
• [Correctness] src/client/features/smartphone/sms.ts:124 (liveThread) — stale thread
  header on rename. Live updates refresh the hint/placeholder/disabled state but not the
  💬 <name> header, so a renamed worker shows its old name until the view is re-entered. Fix:
  update the header node in liveThread too (or store a ref to it in ThreadLive).
• [Performance] src/client/features/smartphone/contacts.ts:87 (renderRecents title) —
  new Date(r.at).toLocaleString() runs per row on every recents draw; toLocaleString is
  slow for a tooltip nobody may read. Compute it lazily on hover/focus or memoize per at.
• [Correctness] src/client/features/smartphone/call.ts:11 (CONNECT_MS = 2600) — ~1s dead
  air after the ring. The double-ring envelope ends at ~1.55s ([0, 0.9] + 0.65s) but connect
  fires at 2.6s. Fix: connect at ~1.8s or schedule a third ring to cover the gap.
• [Correctness] src/client/features/smartphone/call.ts:48 (connect) — doing-line deviates
  from the plan. Plan §3.1/§7 promises doing: "📱 on a call with <name>", but the call ends
  in openWorkerTerminal(id), whose doing is hardcoded (ui/terminal.ts:338) to
  💻 in <name>'s terminal. Fix: thread a doing override through (or correct the plan/docs).
• [Performance] src/client/features/smartphone/ui.ts:18 (Phone interface) — ~9 methods
  plus a separate ThreadLive type and three view modules for a single 380px modal where each
  view has exactly one caller; the indirection costs more reading than it saves. Consider
  collapsing to 2–3 primitives (go/after/close) or co-locating the views until a second
  caller exists.

[iptoux]

All 10 findings fixed in 388dd81 (verified: typecheck, 616/616 tests on re-run, build; one unrelated flake in muse.test.ts passed alone and green on re-run):

• Blank body: drawBody snapshots view and skips replaceChildren when a renderer redirected via go() — the fallback it painted survives.
• Stale sig: covers lost and kind; activity rides as length + 24-char prefix instead of the full text.
• Thread window: last 20 messages with a “Show earlier (N)” expander; sent texts join the live snapshot so expanding stays complete.
• Stoppable ring: phoneRing returns a stop handle (gain ramp + osc stop, safe after natural end), silenced on hang-up, failed connects and close; OfficeSound passes it through.
• Live header: thread 💬 <name> follows renames via the stored node ref.
• Timestamps: toLocaleString memoized per instant.
• Dead air: connect at 1.8s, just as the double ring (~1.55s) ends.
• Doing line: terminal takes an optional TerminalOptions.doing (default unchanged), threaded through openWorkerTerminal(id, find?, doing?); calls open with 📱 on a call with <name> per the plan. All existing callers untouched (optional params).
• Phone interface: kept the contacts/call/sms split — the previous review required it for the size rule, so collapsing now would just churn back.

No new unit tests: all ten are DOM/timer/audio paths the node suite can't reach (same reason the chess resign race shipped verified-by-path). The lab page (phone.html) still exercises contacts/thread/call/recents.