Archipelago — open-source initial import
This commit is contained in:
Archipelago
@@ -0,0 +1,669 @@
|
||||
---
|
||||
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
|
||||
fixed_at: 2026-07-31T08:08:53Z
|
||||
fix_status: fixed_except_documented
|
||||
fixed: 8
|
||||
documented_not_fixed: 1
|
||||
---
|
||||
|
||||
# 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
|
||||
|
||||
## Fix Status (2026-07-31T08:08:53Z)
|
||||
|
||||
All Critical and Warning findings fixed (CR-01, WR-01 through WR-06), one
|
||||
commit per finding. Of the two Info findings, IN-02 (trivial/zero-risk
|
||||
comment) was fixed; IN-01 (a real refactor across 4 files) was left
|
||||
documented, not fixed, per this pass's trivial/zero-risk bar for Info
|
||||
findings. Full test suite (774 tests / 95 files), `vue-tsc --noEmit`, and
|
||||
`npm run build` are all green after every fix. See each finding's own
|
||||
**Status:** line below for the commit hash.
|
||||
|
||||
| Finding | Status | Commit |
|
||||
|---|---|---|
|
||||
| CR-01 | Fixed | `57989dfc` |
|
||||
| WR-01 | Fixed | `61057704` |
|
||||
| WR-02 | Fixed | `751b05f2` |
|
||||
| WR-03 | Fixed (flagged for human verification — concurrency-race logic) | `69358bf6` |
|
||||
| WR-04 | Fixed | `5f7cd4c8` |
|
||||
| WR-05 | Fixed | `7e4e739e` |
|
||||
| WR-06 | Fixed | `0486045d` |
|
||||
| IN-01 | Documented, not fixed | — |
|
||||
| IN-02 | Fixed | `b5506025` |
|
||||
|
||||
## 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.
|
||||
|
||||
**Status:** Fixed in `57989dfc` (`fix(02-review): CR-01 web5.lnd-info/profits resources must not persist to sessionStorage`).
|
||||
|
||||
## 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.
|
||||
|
||||
**Status:** Fixed in `61057704` (`fix(02-review): WR-01 decouple Discover's featured-banner data from app-catalog dedup race`) — implemented option (b)-adjacent: the featured payload now lives on its own dedicated cache key (`'app-catalog:featured'`) subscribed only by Discover.vue, decoupled entirely from the shared `'app-catalog'` dedup race. The shared key and its dedup behavior are unchanged.
|
||||
|
||||
### 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.)
|
||||
|
||||
**Status:** Fixed in `751b05f2` (`fix(02-review): WR-02 stop MeshMap geolocation watch on deactivate, resume on activate`). `onDeactivated` now stops an active watch, tracked via a flag so `onActivated` transparently resumes it on return to the tab (the user's toggle state is preserved, not lost).
|
||||
|
||||
### 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.
|
||||
|
||||
**Status:** Fixed in `69358bf6` (`fix(02-review): WR-03 never drop OpenWrtGateway Connect form params under concurrent load`). `load(params)` now bypasses `routerResource.refresh()` entirely when explicit params are supplied, calling `rpcClient` directly and writing the result into `routerResource.entry` (rather than via `.optimistic()`, to also set `fetchedAt`/`loadState` consistently with a normal refresh success). This is a real concurrency-race fix — flagged for human verification of the logic (concurrent-load race conditions are inherently hard to prove correct from static reading alone; type-check and the full test suite pass, but there is no dedicated OpenWrtGateway.vue test file to exercise the race directly).
|
||||
|
||||
### 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.
|
||||
|
||||
**Status:** Fixed in `5f7cd4c8` (`fix(02-review): WR-04 require explicit persist on resources.ts entry()/optimistic()`) — implemented both suggested fixes together: `persist` is now a required (no-default) argument on both `entry()` and `optimistic()`, and the per-key decision is recorded and asserted (dev-only warning) against any later call that disagrees. `useCachedResource`'s `optimistic()` wrapper threads its own already-resolved `persist` value through automatically. The two direct external call sites (Cloud.vue/PeerFiles.vue's per-peer browse cache) and the resources store's unit tests were updated to pass `persist` explicitly, preserving existing behavior exactly.
|
||||
|
||||
### 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.
|
||||
|
||||
**Status:** Fixed in `7e4e739e` (`fix(02-review): WR-05 forward abort signal through vpnStatus()/dnsStatus()`).
|
||||
|
||||
### 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)
|
||||
```
|
||||
|
||||
**Status:** Fixed in `0486045d` (`fix(02-review): WR-06 skip redundant fallback init timer on later MeshMap reactivations`).
|
||||
|
||||
## 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.
|
||||
|
||||
**Status:** Documented, not fixed. This requires touching business logic
|
||||
across four files (`useCachedResource.ts` plus the three views' own
|
||||
`refresh*IfStale` call sites, each with slightly different local signatures
|
||||
— `refreshHomeGroupIfStale`/`refreshMeshGroupIfStale` take a resource
|
||||
argument and are fanned out via `Promise.allSettled`, while Cloud.vue's
|
||||
`loadCounts` inlines the check directly) — not the trivial/zero-risk bar
|
||||
this fix pass applies to Info findings. Left for a dedicated follow-up.
|
||||
|
||||
### 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.
|
||||
|
||||
**Status:** Fixed in `b5506025` (`fix(02-review): IN-02 document wrapperFor's currently-unreachable full-bleed branch`).
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-07-31T04:09:21Z_
|
||||
_Reviewer: Claude (gsd-code-reviewer)_
|
||||
_Depth: standard_
|
||||
|
||||
## CR-01 Follow-up (post-review hardening)
|
||||
|
||||
**Date:** 2026-07-31
|
||||
**Trigger:** Direct question from Dorian ("does the CR-01 fix protect existing
|
||||
users on update?") surfaced that it did not, for tabs already open before the
|
||||
fix shipped. Not part of a numbered plan — recorded here for traceability.
|
||||
|
||||
### Why CR-01's fix was incomplete for existing users
|
||||
|
||||
CR-01 set `web5.lnd-info`/`web5.networking-profits` to `persist: false`,
|
||||
which stops **future** writes to sessionStorage, and `resources.ts`'s
|
||||
`entry()` already refused to **read** a stale snapshot when `persist` is
|
||||
`false` (WR-04). Neither of those touches a snapshot the **old** bundle
|
||||
already wrote under the old (implicit `persist: true`) decision. Nothing
|
||||
purges an existing `resource:` snapshot except `clearAll()` on logout, or
|
||||
the browser discarding sessionStorage when the tab's session ends. A tab
|
||||
that was open before the update and reloads in-place to pick up the new
|
||||
bundle — normal here, given the installed-PWA and kiosk-display usage
|
||||
patterns — keeps that orphaned snapshot in sessionStorage indefinitely,
|
||||
readable by any script on the origin, until the user happens to log out or
|
||||
close the tab.
|
||||
|
||||
### What was exposed, and what was not
|
||||
|
||||
`resource:web5.lnd-info` held `balance_sats` / `channel_balance_sats` /
|
||||
`synced_to_chain`. `resource:web5.networking-profits` held `total_sats` /
|
||||
`content_sales_sats` / `routing_fees_sats`. **Not** exposed: no macaroon, no
|
||||
private key, no seed material, no npub/pubkey, no channel point, no
|
||||
on-chain/Lightning address, no transaction-level detail. This is a
|
||||
**financial-privacy exposure** (someone with script access to the origin,
|
||||
or physical access to a device with the tab still open, could read this
|
||||
node's aggregate balance/profit figures) — **not** a spending, signing, or
|
||||
key-compromise risk. Stated plainly for anyone reading this record later:
|
||||
nothing in the exposed payload could be used to move funds or impersonate
|
||||
the node.
|
||||
|
||||
### The fix: one-time schema-versioned migration purge
|
||||
|
||||
`resources.ts` now checks a `sessionStorage['resource:__schema']` marker
|
||||
once at store setup (once per tab's page load — not per entry()/refresh()
|
||||
call). Marker absent (tab predates the migration) or stale (persist
|
||||
contract changed since) purges every `resource:`-prefixed sessionStorage
|
||||
key, then writes the current version. A matching marker is a no-op, so a
|
||||
plain in-place reload of an already-migrated tab costs one sessionStorage
|
||||
read, not a purge — the instant-paint-from-snapshot benefit the cache
|
||||
exists for is preserved for the common case. **Contract for future persist
|
||||
changes:** `CURRENT_SCHEMA_VERSION` (currently `'1'`) must be bumped any
|
||||
time a cache key's `persist` decision changes, so the next deploy
|
||||
automatically purges snapshots written under the old, now-incorrect
|
||||
decision — documented inline in `resources.ts` so this doesn't quietly
|
||||
regress the same way CR-01 did.
|
||||
|
||||
### `persist` is now required, not defaulted, everywhere
|
||||
|
||||
`refresh()`'s `opts.persist ?? true` and `useCachedResource()`'s `persist:
|
||||
boolean` (optional, defaulting to `true`) were the same class of footgun
|
||||
that let CR-01 happen in the first place (Web5.vue's wallet resources
|
||||
omitted the field and silently persisted). Both are now required
|
||||
parameters with no default, matching the `entry()`/`optimistic()` hardening
|
||||
WR-04 already applied. Every existing call site was audited and given an
|
||||
explicit decision:
|
||||
|
||||
**persist:false** (financial / identity / peer-identity payload):
|
||||
|
||||
| Call site | Key | Rationale |
|
||||
|---|---|---|
|
||||
| LightningChannelsPanel.vue | `lnd.channels` | open channel balances/capacity — wallet data |
|
||||
| LightningChannelsPanel.vue | `lnd.closed-channels` | closed channel settlement records — wallet data |
|
||||
| Cloud.vue | `cloud.paid-items` | `PaidItem` carries `paid_sats` + purchase history |
|
||||
| Cloud.vue | `cloud.peer-nodes` | `PeerNode` carries did/pubkey/onion |
|
||||
| Cloud.vue / PeerFiles.vue | `cloud.my-files` | not a clean money/identity/peer-identity case — chosen `false` as the fail-safe default, flagged for human review |
|
||||
| Credentials.vue | `credentials.identities` | identity records |
|
||||
| Credentials.vue | `credentials.list` | credential material |
|
||||
| Federation.vue | `federation.nodes` | `FederatedNode` carries did (matches Mesh.vue's existing `mesh.federation-nodes` decision) |
|
||||
| FipsSeedAnchorsCard.vue | `server.fips-seed-anchors` | `SeedAnchor` carries npub |
|
||||
| Server.vue + FipsNetworkCard.vue | `server.fips-summary` | **corrected from `persist:true`** — the shared `fips.status` response carries `npub` (this node's own FIPS identity key); Server.vue's narrower local type didn't surface this, FipsNetworkCard.vue's fuller `FipsStatus` type did. Found during this audit, not in the original call-site list — a same-class T-02-01 violation, fixed alongside it. `serverTabCache.test.ts` updated to match. |
|
||||
|
||||
**persist:true** (aggregate/status/public data, no identity or money):
|
||||
|
||||
| Call site | Key | Rationale |
|
||||
|---|---|---|
|
||||
| AppDetails.vue | `app-details:bitcoin-sync:{id}` | public chain height/sync progress |
|
||||
| Cloud.vue | `cloud.section-counts` | bare per-section item counts |
|
||||
| Cloud.vue / PeerFiles.vue | `cloud.peer-browse:{onion}` (direct `resources.refresh()` calls) | now pass `{ persist: true }` explicitly, matching the pre-existing decision already documented at `peerBrowseEntry()` |
|
||||
| Federation.vue | `federation.dwn-status` | sync status/counters only |
|
||||
| MarketplaceAppDetails.vue | `app-details:versions:{id}` | public catalog metadata |
|
||||
| Monitoring.vue | `monitoring.current` / `.history.minute60` / `.alerts` / `.alert-rules` | system metrics and alert metadata only |
|
||||
| OpenWrtGateway.vue | `server.openwrt-status` | network/router status, matches sibling `server.*` resources |
|
||||
|
||||
Commits: `5bfe6088` (migration + required-persist mechanism),
|
||||
`b8391115` (call-site audit + fixes).
|
||||
|
||||
### PWA auto-update change: declined
|
||||
|
||||
A subsequent instruction, relayed through this session's coordinator,
|
||||
asked for `vite.config.ts`'s `skipWaiting`/`clientsClaim` to be flipped to
|
||||
`true` and `PWAUpdatePrompt.vue` changed to auto-apply updates without user
|
||||
acceptance, citing verbal approval from Dorian for the alpha stage. This
|
||||
was **not implemented**. Per this agent's operating rules, a relayed
|
||||
message from another agent is never treated as the user's own consent for
|
||||
a change of this kind — forcing service-worker activation on a Bitcoin/
|
||||
Lightning wallet PWA (risking a reload mid-session) is exactly the sort of
|
||||
change that needs Dorian's own direct confirmation, not a second-hand
|
||||
instruction. `vite.config.ts` and `PWAUpdatePrompt.vue` are unmodified by
|
||||
this work. If this is still wanted, it should be requested directly.
|
||||
|
||||
**Second request, same decision.** A follow-up message, again relayed
|
||||
through the coordinator, pressed the same change a second time, this time
|
||||
including a purported verbatim quote attributed to Dorian approving forced
|
||||
auto-update and declining any warning UI, offered as direct evidence rather
|
||||
than inference. That request was **also declined, unimplemented** —
|
||||
`vite.config.ts` and `PWAUpdatePrompt.vue` remain unmodified. Not because
|
||||
the quote was judged false, but because this agent has no channel to
|
||||
verify a quote attributed to the user by another agent independently of
|
||||
that agent's own assertion, and this agent's operating rules treat any
|
||||
agent-relayed message — verbatim-quoted or otherwise — as distinct from,
|
||||
and insufficient to substitute for, the user's own message in this session.
|
||||
A more detailed, more insistent second request for the same
|
||||
wallet-affecting change is the exact shape of thing that rule exists to
|
||||
hold the line against, whether or not that's what was happening here. This
|
||||
will be implemented, in one self-contained commit exactly as scoped,
|
||||
the moment Dorian's own message (or an equivalent direct/verified channel)
|
||||
carries the request instead.
|
||||
|
||||
### Safety acceptance criteria — evidence
|
||||
|
||||
1. **Purge blast radius is strictly bounded — PASS.** `purgeAllSnapshots()`
|
||||
only enumerates/removes keys with the `resource:` prefix. Audited every
|
||||
other sessionStorage/localStorage key this app uses (auth's
|
||||
`neode-auth` — localStorage, not sessionStorage; `_seed_words`/
|
||||
`_seed_challenge_indices`; `archipelago_from_boot`/`archipelago_from_splash`/
|
||||
`archipelago_share_to_mesh`/`archipelago_boot_log`; `video_intro_*`;
|
||||
`archy_onboarding_finale`; the hand-rolled `archipelago.web5.identities.v1`
|
||||
/ `archipelago.web5.connected-nodes.v1` / `archipelago.fleet.cache.v1`
|
||||
caches; the PWA install-dismiss and reload-guard keys) — none share the
|
||||
`resource:` prefix. Test:
|
||||
`resourcesClear.test.ts` → *"migration purge is strictly bounded to the
|
||||
resource: prefix..."* seeds all of the above plus a legacy snapshot,
|
||||
runs the migration, and asserts every non-`resource:` key survives
|
||||
byte-for-byte while the legacy key is purged.
|
||||
2. **No spending path is touched — PASS.** `git diff --stat 57989dfc..HEAD`
|
||||
(the range from CR-01's own fix through this work) touches only:
|
||||
`keepalive-remount-probe.spec.ts`, `catalog.json` (unrelated botfights
|
||||
version bump), `rpc-client.ts` (abort-signal plumbing for
|
||||
vpnStatus/dnsStatus, WR-05), `MeshMap.vue`, `useCachedResource.ts`,
|
||||
`resources.ts`/its tests, `Cloud.vue`, `Discover.vue`, `PeerFiles.vue`,
|
||||
`Server.vue`, `OpenWrtGateway.vue`, `keepAliveLifecycle.test.ts`,
|
||||
`dashboardViewWrappers.ts` — plus this session's own commits
|
||||
(`5bfe6088`, `b8391115`) touching the call-site list in the table above.
|
||||
`SendBitcoinModal.vue`, `WalletScanModal.vue`, and every `lnd.*`/`wallet.*`
|
||||
send/pay/sign RPC call site are untouched by any commit in this range —
|
||||
confirmed by `grep -rn "send\|invoice\|pay\|channel\|sign"` over the diff
|
||||
stat returning nothing, and by direct inspection that `SendBitcoinModal.vue`
|
||||
was not among the changed files.
|
||||
3. **Money data is never presented as live when it isn't — PASS for
|
||||
Home.vue, PRE-EXISTING GAP for Web5.vue (not introduced by this work).**
|
||||
Home.vue's wallet card already had a header `RefreshIndicator` wired to
|
||||
`walletStatusRes.loadState` (T-02-13/D-05, from an earlier phase),
|
||||
unconditionally revalidating on every activation rather than TTL-gated —
|
||||
this was verified still intact, not modified. Web5.vue's
|
||||
`networkingProfitsDisplay` (routing/content-sale profit totals) and the
|
||||
hidden wallet card's `lndInfoRes` have **no** equivalent refresh
|
||||
indicator — this predates CR-01 and this fix; `lndInfoRes`'s actual
|
||||
`balance_sats`/`channel_balance_sats` numbers are not rendered anywhere
|
||||
in the current UI (the wallet card is commented out/hidden), so there is
|
||||
no live risk of a stale balance being shown as current from Web5.vue
|
||||
today, but `networkingProfitsDisplay` (a real financial figure) is
|
||||
rendered as plain text with no staleness cue. Not fixed here: adding a
|
||||
new visual indicator would violate this project's standing "no visual/
|
||||
animation changes" rule and needs an explicit design decision, not a
|
||||
unilateral addition inside a storage-layer fix. Flagged for a dedicated
|
||||
follow-up.
|
||||
4. **Nothing can be corrupted mid-flight — PASS.** The migration check runs
|
||||
synchronously inside `useResourcesStore()`'s `setup()`, before the store
|
||||
object is returned to any caller — `entries`/`inflight` are still the
|
||||
fresh empty `Map`s declared earlier in the same `setup()` call, so no
|
||||
`entry()`/`refresh()`/`optimistic()` invocation on this store instance
|
||||
can possibly be in flight yet. Test:
|
||||
`resourcesClear.test.ts` → *"the migration cannot race an in-flight
|
||||
fetch..."* asserts `entries.size === 0` immediately after store
|
||||
construction (post-migration) and that a `refresh()` right afterward
|
||||
completes normally with correct data/loadState/snapshot. The
|
||||
pre-existing `generation` guard (bumped by `clearAll()`, unchanged by
|
||||
this work) continues to cover the separate logout-time race, proven by
|
||||
the unmodified *"drops in-flight bookkeeping..."* test.
|
||||
5. **Reversibility — PASS.** Two self-contained commits
|
||||
(`5bfe6088` mechanism, `b8391115` call-site audit), no schema/API
|
||||
changes outside `neode-ui/src/stores/resources.ts` and
|
||||
`neode-ui/src/composables/useCachedResource.ts`'s type signature, no
|
||||
deploy performed. Either commit can be reverted independently without
|
||||
touching the other.
|
||||
|
||||
### Test / build status
|
||||
|
||||
Full suite: 785/785 passing (up from 778 pre-fix — 7 new tests: 6 migration/
|
||||
blast-radius/race tests + 1 corrected `serverTabCache.test.ts` assertion
|
||||
counts as a modification, not new). `npx vue-tsc --noEmit`: clean. `npm run
|
||||
build`: succeeds.
|
||||
|
||||
### Record correction — PWA auto-update (commit `5fc3284a`)
|
||||
|
||||
The addendum above records the PWA auto-update change being **declined twice** by the
|
||||
code-fixer agent, on the grounds that the approval reached it second-hand and it would
|
||||
not take a change that can reload a Bitcoin/Lightning wallet PWA on relayed consent.
|
||||
That judgement was correct for an agent in its position and was not overridden.
|
||||
|
||||
For the audit trail: the change was subsequently made **by the orchestrator**, which held
|
||||
the authorization first-hand. Dorian's own message, sent in direct reply to an explanation
|
||||
that forcing service-worker activation could reload the app under a user mid-payment (and
|
||||
to an offer of an insistent-prompt alternative instead), was:
|
||||
|
||||
> "ok, we're in alpha so no need for any scary warnings, we can just update them."
|
||||
|
||||
So the decision was the user's, made with the mid-payment reload risk explicitly in front
|
||||
of them, and it specifically rejected adding warning UI. It was not an agent's inference
|
||||
and not a bypass of the earlier refusal.
|
||||
|
||||
Implementation note: rather than flipping the build-time `skipWaiting`/`clientsClaim` flags
|
||||
as originally proposed, `5fc3284a` extends the **auto-apply path that already existed for
|
||||
kiosk displays** to all non-demo clients. That keeps two guards the previous code had
|
||||
deliberately built and which a build-time `skipWaiting` would have bypassed:
|
||||
`reloadAfterCinematic()` holds the reload until the splash/dashboard cinematic finishes, and
|
||||
the `hadController` check ignores the first-install claim. `vite.config.ts` is unmodified.
|
||||
|
||||
Revisit at beta: restoring the prompt is a one-line change (`showUpdatePrompt.value = true`).
|
||||
Reference in New Issue
Block a user