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 <noreply@anthropic.com>
2026-07-31 00:11:52 -04:00
parent 092a37332f
commit b448232fb5
1 changed files with 393 additions and 0 deletions
@@ -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<ProfitsData>({
+  key: 'web5.networking-profits',
+  fetcher: (signal) => rpcClient.call<ProfitsData>({ 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<MarketplaceApp[]>({
+  key: 'app-catalog',
+  fetcher: async () => getCuratedAppList(),   // static hardcoded list only
+  ...
+})
+
+// Discover.vue
+const catalogResource = useCachedResource<MarketplaceApp[]>({
+  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<string, string> | undefined
+const routerResource = useCachedResource<RouterStatus>({
+  key: 'server.openwrt-status',
+  fetcher: (signal) => rpcClient.call<RouterStatus>({
+    method: 'openwrt.get-status',
+    params: pendingParams ?? {},
+    ...
+  }),
+  ...
+})
+async function load(params?: Record<string, string>) {
+  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<T>(key: string, persist = true): ResourceEntry<T> {
+  let e = entries.get(key)
+  if (!e) {
+    const snap = persist ? readSnapshot<T>(key) : null
+    e = reactive<ResourceEntry>({ ... })
+    entries.set(key, e)
+  }
+  return e as ResourceEntry<T>
+}
+```
+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<T>(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<void>` to `CachedResource<T>`'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_