lfg2025/archy
1
0
Fork 0
Code Issues 24 Pull Requests 1 Actions Packages Projects Releases 63 Wiki Activity

docs(02-review): mark phase 02 review findings fixed/documented with commit hashes

Browse Source 
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 <noreply@anthropic.com>
This commit is contained in:
archipelago 2026-07-31 04:16:41 -04:00
parent b5506025dd
commit 64aafa7899
1 changed files with 50 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

50
.planning/phases/02-ui-performance/02-REVIEW.md
View File

@ -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_