From 64aafa789956afbe015cdd6af987b2a66230f8a1 Mon Sep 17 00:00:00 2001 From: archipelago Date: Fri, 31 Jul 2026 04:16:41 -0400 Subject: [PATCH] docs(02-review): mark phase 02 review findings fixed/documented with commit hashes CR-01 and WR-01 through WR-06 fixed (one commit each); IN-02 fixed (trivial comment); IN-01 left documented, not fixed (requires a real refactor across 4 files, outside this pass's trivial/zero-risk bar for Info findings). Full test suite, vue-tsc --noEmit, and npm run build are all green after every fix. Co-Authored-By: Claude --- .../phases/02-ui-performance/02-REVIEW.md | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/.planning/phases/02-ui-performance/02-REVIEW.md b/.planning/phases/02-ui-performance/02-REVIEW.md index 99deb51a..13ae146e 100644 --- a/.planning/phases/02-ui-performance/02-REVIEW.md +++ b/.planning/phases/02-ui-performance/02-REVIEW.md @@ -36,6 +36,10 @@ findings: 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 @@ -45,6 +49,28 @@ status: issues_found **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 | `69e45554` | +| WR-01 | Fixed | `f34ef922` | +| WR-02 | Fixed | `f5cd9294` | +| WR-03 | Fixed (flagged for human verification — concurrency-race logic) | `467f53e8` | +| WR-04 | Fixed | `2005c8c7` | +| WR-05 | Fixed | `40f8cf67` | +| WR-06 | Fixed | `013704a9` | +| IN-01 | Documented, not fixed | — | +| IN-02 | Fixed | `952f6102` | + ## Summary Phase 02 layers a KeepAlive instance cache and a stale-while-revalidate resource @@ -131,6 +157,8 @@ const lndInfoRes = useCachedResource<{ Also update Home.vue's comment once fixed — it currently documents this as a known, deliberately-unfixed gap. +**Status:** Fixed in `69e45554` (`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 @@ -194,6 +222,8 @@ 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 `f34ef922` (`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` @@ -231,6 +261,8 @@ onDeactivated(() => { 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 `f5cd9294` (`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` @@ -277,6 +309,8 @@ 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 `467f53e8` (`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` @@ -309,6 +343,8 @@ sessionStorage with no indication anything is wrong. consistent, or have `optimistic()` require an explicit `persist` argument (no default) so silent fallback-to-`true` can't happen by omission. +**Status:** Fixed in `2005c8c7` (`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` @@ -336,6 +372,8 @@ resource. `dnsStatus()` (mirroring the pattern already used everywhere else in `rpc-client.ts`) and forward it here. +**Status:** Fixed in `40f8cf67` (`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` @@ -354,6 +392,8 @@ init for the very first mount") not actually match what the code does if (!map) setTimeout(initMap, 300) ``` +**Status:** Fixed in `013704a9` (`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 @@ -371,6 +411,14 @@ it. 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` @@ -386,6 +434,8 @@ 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 `952f6102` (`fix(02-review): IN-02 document wrapperFor's currently-unreachable full-bleed branch`). + --- _Reviewed: 2026-07-31T04:09:21Z_