Repository navigation
feat(mobile): keep machine connections in a Zustand store and cache the last status per machine - #1551
Conversation
janicduplessis
left a comment
There was a problem hiding this comment.
Fresh review of #1551 (issue #1543). Local tsc --noEmit, jest (142 passed), oxfmt --check and expo lint pass in apps/mobile. Required CI is red, and both failures come from this PR.
Bugs
-
Windows CI: pnpm cannot parse the new patches. In
test (windows),setup-stimfails withERR_PNPM_INVALID_PATCH ... react-native-nitro-modules@0.37.1.patch ... invalid char in unquoted filename. The repo has no.gitattributes, so the Windows checkout convertspatches/*.patchto CRLF, and pnpm reads the\ras part of the file name. Fix: add a.gitattributeswithpatches/*.patch text eol=lf(or-text). -
test/e2e/release-prep.e2e.jsfails on Linux (node 22 and node 24). The failing test is "a bump rewrites all six versions and leaves the lockfile alone", withFailed to read patch file /tmp/stim-release-prep-*/patches/react-native-mmkv@4.3.2.patch.makeWorkspace()(around lines 29-43) copiespnpm-workspace.yamlandpnpm-lock.yamlinto a temp root, and both now referencepatches/, but the directory is never copied. Fix: addcpSync(join(REPO, 'patches'), join(root, 'patches'), { recursive: true })tomakeWorkspace. The realscripts/release-prep.mjsruns in the repo root, so a real release is not affected, but CI stays red until the fixture copies the directory.
Risks
-
"Last seen" can describe old cached data as fresh (
apps/mobile/src/hooks/mac-connection.tsx,useMachinePresence;apps/mobile/src/lib/attention.ts:51).lastSeenAtislink.disconnectedAt ?? cachedSeenAt. Take a machine whose cache is hours old. On cold launch it connects (wasOpenbecomes true) and then drops before its firststatuspush.disconnectedAtbecomes now while the rows still show the cached status. The rows then read "Last seen <1m ago", activity and build chips freeze at the drop time, and the attention item says the same.write()skips this snapshot becausecachedSeenAt !== null, so the disk entry stays correct and only the display is wrong. The window is small because the server pushes status on subscribe. Fix: prefer the cache time while the snapshot is still cached, i.e.lastSeenAt: cachedSeenAt ?? link?.disconnectedAt ?? null, and in attentionmac.seenAt ?? mac.disconnectedAt. -
Cold launch can briefly show machines the filters hide (
apps/mobile/src/screens/home.tsx,macIdsfrommacs ?? []). Until the pairings load,macIdsis[].filterWorkspacesthen drops everyfilters.macsentry, so the cached rows and tiles of machines excluded by the machine filter show untillistMacsresolves. Before this PR the list was empty in that window, so this is new. Fix: whilemacs === null, buildmacIdsfrom the snapshot ids (the keysworkspacesis built from). -
The
patchesrelease-QA exemption covers the whole directory (scripts/release-qa-matrix.data.mjs). The rule exempts all ofpatches/as "patches of Expo mobile app dependencies". A later pnpm patch to a dependency of a published package would also be exempt without anyone noticing. Suggestion: reword the exemption so reviewers check it when a patch targets a published package, or keep it and note that constraint in the PR.
Nits
apps/mobile/src/lib/attention.ts:20:seenAt?: number | nullis optional whiledisconnectedAtis required. Making it required keeps callers from silently leaving it out.apps/mobile/src/lib/status-cache.ts: the doc comment/** The subset of react-native-mmkv's MMKV the cache uses. */restates the interface. Under the comment policy it can go.apps/mobile/src/lib/status-share.test.ts, "treats an added or removed field as a change": the array assertionshare([1, 2], [1, 2, 3])).toEqual([1, 2, 3])checks only the value, so it passes whether or not identity is handled..not.toBe(prev)on a namedprevwould test the identity contract.
Checked, no issue found
- The
share/shareStatusmerge: it keys environments by path, handles added and removed keys throughhasOwn, and returnsprevfor a deep-equal push.receiveStatusmakes nosetfor an equal live push, and a cached-to-live transition keeps the sameworkspacesarray. - The selectors return stable references: raw record entries,
IDLE_LINK, oruseShallowover primitives. None of them builds a new object on every call. DeviceGridTile's comparator can skiptile.devicebecause the device comes fromitem.env.onOpenandonAspectare stable callbacks.- The write, debounce and prune paths behave correctly. On forget,
setMacsprunes the snapshot beforeMacLinkunmounts, soremoveLinkfinds nothing to write back. - Recents and the attention strip ignore a cached status. Read-only scope and the settings screens are unchanged.
- The AGP 9 guards match the open upstream PRs margelo/react-native-mmkv#1090 and margelo/nitro#1579, and the YAML comment that links them is allowed by the comment policy.
… prefer the cached time for last seen
janicduplessis
left a comment
There was a problem hiding this comment.
Re-review of d2f188d. It resolves all 8 earlier findings, but one new format failure keeps CI red.
Still open
test (node 22)andtest (node 24)fail at "Format check (oxfmt)" inscripts/release-qa-matrix.data.mjs:330. The newexemptstring is too long for one line; oxfmt breaks it after the key:Runexempt: 'pnpm patches of Expo mobile app dependencies; a patch of a published package dependency needs its own rule',
pnpm run format(oroxfmt scripts/release-qa-matrix.data.mjs) and push. Those two jobs stop at this step, so their unit and e2e steps did not run on this commit.
Verified fixed
.gitattributespatches/*.patch text eol=lf: the blobs are already LF, so a fresh Windows checkout keeps them LF.test (windows)was still pending when I checked.- The release-prep fixture now copies
patches/. lastSeenAtnow prefers the cached time, in bothuseMachinePresence(cachedSeenAt ?? disconnectedAt ?? null) and attention (seenAt ?? disconnectedAt).- Once a live push lands,
cachedSeenAtis null and the drop time is used again. The usual path, live then dropped, is therefore unchanged. - A link that opens and drops before its first push now shows the cache time.
- Once a live push lands,
- Home
macIdscomes from the items' machine ids while the pairings load. The machine filter now applies to cached rows at cold launch.- A cached machine with no environments has no items. Its id is left out, but it also has no rows to filter.
- If the only selected machine has no rows, the filter shows every machine. That is the same as main's behavior for a selected machine that is not paired, so it is not a regression.
- After the pairings load,
macIdscomes frommacsas before. Theitemsdependency only costs a recompute per push while the pairings are loading.
seenAtis required, the restating comment is gone, and thesharearray test now checks identity in both directions.
In apps/mobile, tsc --noEmit and jest pass (142 tests). The root pnpm run format:check fails only on the file above. With that line reformatted I have nothing else open.
Description
The phone app kept every machine connection in one React context, so every
statuspush re-rendered every consumer, even a push that changed nothing. Nothing survived a restart, so a cold launch showed a spinner, or "No machine connected" when the machine was down.This adds a native module (
react-native-mmkvonreact-native-nitro-modules), so it needs a new TestFlight build. The runtime fingerprint changes; an OTA update cannot carry it.Solution
Zustand store for the machine pool (
src/lib/machine-store.ts). Each machine has a link (connection, state, home,disconnectedAt), a snapshot (status), and usage, in separate records so a usage poll does not touch status consumers. TheMacLinklifecycle components are unchanged apart from writing to the store.shareStatusmatches environments by path and keeps the identity of every part equal to the previous push. A push equal to the current status makes nosetat all, the common case since perf(status): refresh only log facts on log appends and report activity to the minute #1540.HomeItem[], reusing an item while itsenvand names are unchanged.WorkspaceRowandDeviceGridTilearememoized and select their own machine state; workspace detail reads its workspace withuseWorkspace(macId, path).MMKV cache, one key per machine (
status:<macId>insrc/lib/status-cache.ts), without Zustand'spersist. - Written at most every 5 s, and at once when the connection drops or the app leaves the foreground.Where this differs from the agreed design:
seenAtis when the status was last known current (write time while connected, or the drop time), not when it was received; with rare pushes, receive time would make an open connection look stale.disconnectedAtis not seeded from the cache, so a cold launch does not flash an Offline attention item while connecting. The attention strip and recents ignore a still-cached status, as before (no status until the first push).AGP 9 patches (remove once upstream ships). React Native 0.88 uses AGP 9.2.1, which registers the Kotlin plugin itself;
react-native-mmkv@4.3.2andreact-native-nitro-modules@0.37.1applyorg.jetbrains.kotlin.androidunconditionally, which fails Gradle configuration. The pnpm patches guard that line like the open upstream fixes, margelo/react-native-mmkv#1090 and margelo/nitro#1579. Both packages are pinned exactly forpatchedDependencies.patches/is exempted in the release QA matrix, likeapps/mobile.Test plan
Re-renders per status push. I added uncommitted per-component render counters and counted renders in the 120 ms after each push. The run used the iOS simulator against
pnpm run mock-server(15 workspaces, default Live filter, 4 rows). For the "one workspace changed" case, a local mock tweak changed one workspace'serrorsSinceMarkeron every push.A push that lands with a usage poll adds 3 (machine chip and attention strip).
Unit tests:
status-share.test.ts: an identical push returns the previous status; one changed environment keeps the others' identity; path matching survives reorder and insert; items are reused.status-cache.test.ts: round trip, removal of unreadable entries, the size bound,keepOnly.machine-store.test.ts: hydration before the pairings load, noseton an identical push, the debounced write and itsseenAt, the immediate write on a drop, no write-back of a status that is still cached, and pruning, including no pruning after a failed pairing read.Devices (dev client built with this branch; mock server on its own port, paired with
dev:pair --mock):getAllKeys()on the MMKV instance is[]after the next reload.maindoes the same after a full reload.Fixes #1543