From b448232fb5b900f0b01fb551b3753ff054a18c3f Mon Sep 17 00:00:00 2001 From: archipelago Date: Fri, 31 Jul 2026 00:11:52 -0400 Subject: [PATCH] docs(02): code review findings Adversarial review of phase 02's KeepAlive/useCachedResource architecture: 1 blocker (Web5.vue wallet balances default-persisted to sessionStorage, violating T-02-01) and 6 warnings (app-catalog cache-key race silently drops Discover's featured banner, MeshMap geolocation watch survives tab deactivation, OpenWrtGateway connect-form params droppable under concurrent load, resources.ts persist-argument footgun, partial abort-signal coverage on Server.vue's network-summary fetch, redundant timer re-arm). Co-Authored-By: Claude --- .../phases/02-ui-performance/02-REVIEW.md | 393 ++++++++++++++++++ 1 file changed, 393 insertions(+) create mode 100644 .planning/phases/02-ui-performance/02-REVIEW.md diff --git a/.planning/phases/02-ui-performance/02-REVIEW.md b/.planning/phases/02-ui-performance/02-REVIEW.md new file mode 100644 index 00000000..99deb51a --- /dev/null +++ b/.planning/phases/02-ui-performance/02-REVIEW.md @@ -0,0 +1,393 @@ +--- +phase: 02-ui-performance +reviewed: 2026-07-31T04:09:21Z +depth: standard +files_reviewed: 26 +files_reviewed_list: + - neode-ui/src/api/rpc-client.ts + - neode-ui/src/components/MeshMap.vue + - neode-ui/src/components/RefreshIndicator.vue + - neode-ui/src/composables/useCachedResource.ts + - neode-ui/src/stores/auth.ts + - neode-ui/src/stores/homeStatus.ts + - neode-ui/src/stores/mesh.ts + - neode-ui/src/stores/resources.ts + - neode-ui/src/stores/transport.ts + - neode-ui/src/views/AppDetails.vue + - neode-ui/src/views/Apps.vue + - neode-ui/src/views/Chat.vue + - neode-ui/src/views/Cloud.vue + - neode-ui/src/views/Dashboard.vue + - neode-ui/src/views/Discover.vue + - neode-ui/src/views/Home.vue + - neode-ui/src/views/Marketplace.vue + - neode-ui/src/views/MarketplaceAppDetails.vue + - neode-ui/src/views/Mesh.vue + - neode-ui/src/views/Server.vue + - neode-ui/src/views/server/OpenWrtGateway.vue + - neode-ui/src/views/dashboard/DashboardRouterView.vue + - neode-ui/src/views/dashboard/dashboardViewWrappers.ts + - neode-ui/src/views/dashboard/keepAliveRoutes.ts + - neode-ui/src/views/dashboard/useRouteTransitions.ts + - neode-ui/src/views/web5/Web5.vue +findings: + critical: 1 + warning: 6 + info: 2 + total: 9 +status: issues_found +--- + +# Phase 02: Code Review Report + +**Reviewed:** 2026-07-31T04:09:21Z +**Depth:** standard +**Files Reviewed:** 26 source files (diffed against `a75b6709~1`, the commit before phase 02's first commit) + test files skimmed for correctness of what they pin +**Status:** issues_found + +## Summary + +Phase 02 layers a KeepAlive instance cache and a stale-while-revalidate resource +composable (`useCachedResource`) onto ~15 views, plus an activate/deactivate +lifecycle audit across the main tabs. The architecture itself +(`keepAliveRoutes.ts` classifier, `dashboardViewWrappers.ts` memoized wrapper +factory, `useCachedResource.ts`'s onActivated hook, the Cloud.vue browse-peer +concurrency pool) is sound: no unbounded-growth bugs, no wrapper name +collisions, and the concurrency-capped fan-out in Cloud.vue is race-free +(cursor increments are synchronous, no double-processing of a peer). + +The real defects are concentrated in three places the task specifically asked +to scrutinize: (1) one cache key genuinely violates T-02-01 (wallet data +persisted to sessionStorage by default) in a file this phase directly edited, +(2) a cache-key-sharing decision made in 02-04 has a real, previously +unrecognized race that silently degrades a UI section, and (3) one +onActivated/onDeactivated pair (MeshMap.vue) doesn't fully mirror the +"only-while-visible" discipline applied everywhere else in the same rewrite. +None of these were already called out in 02-FINDINGS.md's Outstanding section +or the task's exclusion list (Server.vue remount gap, timing regressions, +UIFIX-01..06), so they're reported fresh below. + +## Critical Issues + +### CR-01: Web5.vue's wallet balance resources persist to sessionStorage by default (T-02-01 violation) + +**File:** `neode-ui/src/views/web5/Web5.vue:140-143, 293-300` +**Issue:** `profitsRes` (`web5.networking-profits`) and, more importantly, +`lndInfoRes` (`web5.lnd-info`, holding `balance_sats`/`channel_balance_sats`/ +`synced_to_chain`) are declared with no `persist` field at all: + +```ts +const lndInfoRes = useCachedResource<{ + balance_sats: number + channel_balance_sats: number + synced_to_chain: boolean +}>({ + key: 'web5.lnd-info', + fetcher: (signal) => rpcClient.call({ method: 'lnd.getinfo', signal, dedup: true, maxRetries: 1 }), +}) +``` + +`useCachedResource`'s default is `persist = opts.persist ?? true` +(`useCachedResource.ts:64`), so every successful fetch calls +`writeSnapshot('web5.lnd-info', {balance_sats, channel_balance_sats, +synced_to_chain}, fetchedAt)`, writing the node's live on-chain and Lightning +channel balances into `sessionStorage` as plaintext JSON +(`resource:web5.lnd-info`). This is exactly the class of data T-02-01 exists +to keep out of sessionStorage, and it directly contradicts the pattern this +same phase established everywhere else — every other resource in this phase +(`home.wallet-status`, `mesh.self-onion`, `mesh.self-did`, `mesh.contacts`, +`server.vpn-peers`, `server.tor-services`, `AppDetails.vue`'s credentials +resource, …) makes an *explicit* `persist: false` decision precisely because +"never defaulted" was the hard rule (`02-02-SUMMARY.md` key-decisions: +`"persist decided explicitly per cache key (never defaulted) per T-02-01"`). + +This is not a hypothetical: `neode-ui/src/views/Home.vue:527-544`'s own code +comment, added by this phase, explicitly documents the gap and declines to +close it: *"web5.lnd-info's default persist:true (Web5.vue is out of this +plan's file scope to fix) would leak balance data to sessionStorage via its +own independent refresh cycle regardless of what Home declares."* Web5.vue +**is** in this review's file scope (63 lines changed by this phase, including +the onActivated/onDeactivated lifecycle wrapped directly around these two +resources), so the fix belongs here now rather than being deferred again. +`02-FINDINGS.md`'s `## Outstanding` section does not list this gap. + +**Fix:** +```ts +const profitsRes = useCachedResource({ + key: 'web5.networking-profits', + fetcher: (signal) => rpcClient.call({ method: 'wallet.networking-profits', signal, dedup: true, maxRetries: 1 }), + persist: false, // routing/content-sale profit totals — financial data (T-02-01) +}) +const lndInfoRes = useCachedResource<{ + balance_sats: number + channel_balance_sats: number + synced_to_chain: boolean +}>({ + key: 'web5.lnd-info', + fetcher: (signal) => rpcClient.call({ method: 'lnd.getinfo', signal, dedup: true, maxRetries: 1 }), + persist: false, // wallet balance — must never land in sessionStorage (T-02-01) +}) +``` +Also update Home.vue's comment once fixed — it currently documents this as a +known, deliberately-unfixed gap. + +## Warnings + +### WR-01: `app-catalog` cache key shared by two non-equivalent fetchers — dedup races silently drop Discover's featured-banner data + +**File:** `neode-ui/src/views/Marketplace.vue:242-247`, `neode-ui/src/views/Discover.vue:273-291` +**Issue:** Both views register a `useCachedResource` against the same key +`'app-catalog'`, but with different fetchers: + +```ts +// Marketplace.vue +const catalogResource = useCachedResource({ + key: 'app-catalog', + fetcher: async () => getCuratedAppList(), // static hardcoded list only + ... +}) + +// Discover.vue +const catalogResource = useCachedResource({ + key: 'app-catalog', + fetcher: async () => { + const catalog = await fetchAppCatalog() // dynamic registry fetch + if (catalog) { + catalogFeatured.value = catalog.featured // <- Discover-local side effect + return catalog.apps + } + catalogFeatured.value = null + return getCuratedAppList() + }, + ... +}) +``` + +`resources.ts`'s `refresh()` dedupes by key via an `inflight` map: whichever +caller's `refresh()` reaches `store.refresh()` first (synchronously, before +the other subscriber's call) sets `inflight`, and every other concurrent +caller for the same key just awaits that *same* promise — its own fetcher +never runs. Since both views are simultaneously KeepAlive-eligible +(`/dashboard/marketplace` and `/dashboard/discover` are both in +`KEEP_ALIVE_PATHS`), and both re-subscribe/re-revalidate on every activation +and TTL lapse, whichever view's fetcher wins a given race governs the shared +cache entry for both. When Marketplace's simpler fetcher wins, Discover's +`catalogFeatured.value` side effect is silently skipped for that cycle — +`featuredBanner` (`Discover.vue:450`) falls back to the static +`FEATURED_DEFINITIONS` entry with no error, no stale indicator, and no way +for the user to tell the dynamic catalog's featured banner was dropped. +Concretely: visit Marketplace first (within its 300s TTL), then Discover — +Discover hydrates the already-populated `entries` Map entry, sees it isn't +stale, and never calls its own fetcher at all, so `catalogFeatured` stays at +its initial `null` for the rest of that TTL window. + +02-04-SUMMARY.md's own rationale ("both are valid producers of the shared +'app-catalog' cache key") is the flawed premise here — the two fetchers are +not interchangeable because only one carries the `catalogFeatured` side +effect, and `store.refresh()`'s dedup silently privileges whichever one wins. + +**Fix:** Either (a) give Discover.vue its own cache key +(`'app-catalog:discover'`) so its richer fetcher always runs on its own +schedule, or (b) move the `catalogFeatured` derivation out of the fetcher and +into a `computed`/store-level cache so it doesn't depend on which of the two +subscribers' fetcher happened to execute, or (c) make Marketplace.vue call +`fetchAppCatalog()` too (unifying the two fetchers) so both producers are +genuinely interchangeable as the design comment assumes. + +### WR-02: MeshMap.vue's geolocation watch keeps running after the Mesh tab is deactivated + +**File:** `neode-ui/src/components/MeshMap.vue:69-76, 412-429` +**Issue:** `armMapVisibility()`/`disarmMapVisibility()` correctly follow the +"only-while-visible" pattern for the resize listener and `ResizeObserver` +(added by this phase specifically because MeshMap now survives a tab switch +under KeepAlive), but `onDeactivated` does not call `stopSharing()`: + +```ts +onActivated(() => { if (mapMountFresh) { mapMountFresh = false; return }; armMapVisibility() }) +onMounted(() => armMapVisibility()) +onDeactivated(() => disarmMapVisibility()) // <- geolocation watch NOT stopped here +``` + +If the user has "Share Location" enabled (`sharingLocation.value = true`, +`geoWatchId` set via `navigator.geolocation.watchPosition`) and then switches +away from the Mesh tab to any other main tab, the browser's location watch +keeps firing in the background indefinitely — `mesh.updateSelfPosition()` +keeps getting called, the browser's location indicator stays active, and GPS +polling continues to drain battery — for as long as the session lasts (or +until `KEEP_ALIVE_MAX` evicts Mesh.vue's whole subtree and `onUnmounted` +finally calls `stopSharing()`). Every other resource this phase added +"only-while-visible" handling for in this exact file (resize listener, +ResizeObserver) is torn down on deactivate; the geolocation watch — arguably +the most expensive/privacy-sensitive of the three — is not. + +**Fix:** +```ts +onDeactivated(() => { + disarmMapVisibility() + if (sharingLocation.value) stopSharing() +}) +``` +(If keeping location live across a tab switch is actually desired, that +should be a deliberate, documented decision like the other exceptions in this +phase — not a gap in an otherwise-systematic "only-while-visible" rewrite.) + +### WR-03: OpenWrtGateway.vue's `load(params)` can silently drop a caller's params under concurrent load + +**File:** `neode-ui/src/views/server/OpenWrtGateway.vue:85-96, 152-166` +**Issue:** +```ts +let pendingParams: Record | undefined +const routerResource = useCachedResource({ + key: 'server.openwrt-status', + fetcher: (signal) => rpcClient.call({ + method: 'openwrt.get-status', + params: pendingParams ?? {}, + ... + }), + ... +}) +async function load(params?: Record) { + error.value = '' + pendingParams = params + await routerResource.refresh() + const err = routerResource.error.value + ... +} +``` +`routerResource.refresh()` goes through `resources.ts`'s `store.refresh()`, +which dedupes concurrent calls for the same key via its `inflight` map: if a +refresh is already in flight (e.g. an auto-revalidation from +`useCachedResource`'s own TTL-gated `onActivated`, or the plain `load()` this +component's own `onMounted` fires on a stale cache), a second call to +`load({host, ssh_user, ssh_password})` (the Connect form's submit handler, +`OpenWrtGateway.vue:174`) sets `pendingParams` to the new connect +credentials, but `routerResource.refresh()` just returns the *first* call's +already-in-flight promise — the fetcher never re-runs, so +`params: pendingParams ?? {}` for that in-flight request was already resolved +against whatever `pendingParams` held when *that* call started (typically +`{}` from a background reconnect). The Connect form's submit `await +load({host,...})` then resolves against that unrelated result: the entered +host/credentials were never actually sent, and the caller has no way to tell. + +**Fix:** Give each `load()` call its own request instead of routing through +the shared cache's dedup when explicit params are supplied — e.g. bypass +`routerResource.refresh()` for the params-carrying path and call +`rpcClient.call(...)` directly (then write the result into `routerResource` +via `.optimistic()`), or track an explicit "params in flight" flag and reject/ +queue overlapping calls with different params instead of silently coalescing +them. + +### WR-04: `resources.ts`'s `entry()` silently ignores `persist` after the first call for a key + +**File:** `neode-ui/src/stores/resources.ts:68-81` +**Issue:** +```ts +function entry(key: string, persist = true): ResourceEntry { + let e = entries.get(key) + if (!e) { + const snap = persist ? readSnapshot(key) : null + e = reactive({ ... }) + entries.set(key, e) + } + return e as ResourceEntry +} +``` +Only the *first* caller for a given key's `persist` argument has any effect; +every subsequent call (from `entry()` itself, or transitively from +`optimistic()`, which calls `entry(key)` with no `persist` arg at all — +defaulting to `true`) silently reuses whatever was decided the first time. +Every current call site happens to be safe because `useCachedResource()` +always creates the entry (with the correct, explicit `persist`) before any UI +code can call `.optimistic()` on it — but this is a fragile invariant, not an +enforced one, and it is exactly the kind of interaction T-02-01 asks this +phase to get right. A future resource that calls `store.optimistic(key, ...)` +before any `useCachedResource({key, persist: false, ...})` has run in the +same tick (e.g. from a Pinia store action fired at app-init, before any +component mounts) would silently get `persist: true` and start writing to +sessionStorage with no indication anything is wrong. +**Fix:** Make `persist` a property of the entry that's set once and asserted +consistent, or have `optimistic()` require an explicit `persist` argument +(no default) so silent fallback-to-`true` can't happen by omission. + +### WR-05: `server.network-summary`'s abort-on-unmount contract is only half-honored + +**File:** `neode-ui/src/views/Server.vue:475-482` +**Issue:** `networkRes`'s fetcher batches four RPCs, but only two forward the +`signal` `useCachedResource` provides for abort-on-unmount: +```ts +fetcher: async (signal) => { + const [diagRes, fwdRes, vpnRes, dnsRes] = await Promise.allSettled([ + rpcClient.call<...>({ method: 'network.diagnostics', signal, ... }), + rpcClient.call<...>({ method: 'router.list-forwards', signal, ... }), + rpcClient.vpnStatus(), // <- no signal parameter exists on this method + rpcClient.dnsStatus(), // <- no signal parameter exists on this method + ]) + ... +} +``` +`rpcClient.vpnStatus()`/`dnsStatus()` are convenience wrappers with no +`signal` parameter at all, so `aborter.abort()` (fired from +`useCachedResource`'s `onScopeDispose`) cannot cancel these two calls. +This partially defeats the documented "Abort-on-unmount: the fetcher receives +an AbortSignal that fires when the last subscribed component unmounts" +contract in `useCachedResource.ts`'s own header comment, for this one +resource. +**Fix:** Add an optional `signal` parameter to `rpcClient.vpnStatus()`/ +`dnsStatus()` (mirroring the pattern already used everywhere else in +`rpc-client.ts`) and forward it here. + +### WR-06: MeshMap.vue re-arms a redundant 300ms fallback timer on every reactivation + +**File:** `neode-ui/src/components/MeshMap.vue:399-401` +**Issue:** `armMapVisibility()` unconditionally calls `setTimeout(initMap, +300)` every time it runs — on the initial mount *and* on every later +reactivation. `initMap()`'s own guard (`if (!mapContainer.value || map) +return`) makes this harmless once the map exists, but it means every +tab-switch back into Mesh (with the Map sub-tab open) schedules a throwaway +300ms timer purely to no-op. Low severity (matches the file's own comment +acknowledging this), but it's dead weight that a one-line `if (!map)` guard +around the `setTimeout` call would remove, and it makes the intent ("fallback +init for the very first mount") not actually match what the code does +("fallback init on every arm"). +**Fix:** +```ts +if (!map) setTimeout(initMap, 300) +``` + +## Info + +### IN-01: `refreshXIfStale` helper duplicated near-verbatim across three views + +**File:** `neode-ui/src/views/Home.vue:552-555`, `neode-ui/src/views/Mesh.vue` (`refreshMeshGroupIfStale`), `neode-ui/src/views/Cloud.vue` (`loadCounts`'s staleness check) +**Issue:** The "refresh this `CachedResource` only if it has never resolved or +is past its own TTL" pattern is re-implemented independently in at least +three views with the same three-line body (`if (res.entry.data === null || +res.isStale.value) return res.refresh(); return Promise.resolve()`). Not a +bug, but worth lifting into `useCachedResource.ts` itself (e.g. exposing +`refreshIfStale()` on the returned object, mirroring the internal helper the +composable already has) now that three call sites independently reinvented +it. +**Fix:** Add `refreshIfStale: () => Promise` to `CachedResource`'s +return shape and have the three views call that instead of their local +copies. + +### IN-02: `wrapperFor`'s full-bleed/non-cacheable branch is currently dead code + +**File:** `neode-ui/src/views/dashboard/dashboardViewWrappers.ts:108-117` +**Issue:** `wrapperFor()`'s key derivation (`cacheable || isFullBleedPath(path) +? path : DEFAULT_WRAPPER_KEY`) has a branch for "full-bleed but not +cacheable" paths, but `isFullBleedPath()` only ever returns true for +`/dashboard/chat` and `/dashboard/mesh`, both of which are always in +`KEEP_ALIVE_PATHS` today (derived from `TAB_ORDER`, which both belong to). +The branch is defensively correct (and cheap), just currently unreachable — +worth a one-line comment noting it's intentional defense against a future +`TAB_ORDER`/`WITHHELD_FROM_CACHE` change that could withhold a full-bleed +path from the cache, so a future reader doesn't mistake it for dead code to +delete. +**Fix:** Non-blocking; a comment is sufficient. + +--- + +_Reviewed: 2026-07-31T04:09:21Z_ +_Reviewer: Claude (gsd-code-reviewer)_ +_Depth: standard_