feat(8f-followup): close 19 of 20 retrospective findings (single-PR cleanup workstream)#166
Merged
Merged
Conversation
Closes 8f followup HIGH #3 — pre-8f users carried an orphan IndexedDB forever after the wallLayoutStore → canvasViewportStore shape change. Mirror PwaHubRepository's legacy-DB cleanup pattern. Tightens the existing test that lied about asserting deletion. Co-Authored-By: ruflo <ruv@ruv.net>
…alogs Closes 8f followup HIGH #5 — 47 hardcoded English strings across SystemLevelView, CanvasLensPicker, MobileLevelPicker, NoFocalStepPrompt, AuthorL3View, LocalMechanismView now route through MessageCatalog. CANVAS_LENS_REGISTRY labels/descriptions translated at render time in CanvasLensPicker via LENS_LABEL_KEY / LENS_DESC_KEY maps; the hooks registry keeps English for non-UI consumers. Non-English locales receive English placeholders pending a translation pass (TODO(i18n) comment). Co-Authored-By: ruflo <ruv@ruv.net>
Closes the i18n reviewer's flagged follow-up: Canvas/index.tsx:1042
rendered the lens registry's English label and a local
CANVAS_LEVEL_LABELS map as user-facing copy, bypassing i18n.
- Export LENS_LABEL_KEY from CanvasLensPicker for cross-component reuse
- Reuse canvas.mobile.{system,process,step} for level labels
- Add canvas.lensPicker.invalidAtLevel parameterized key (32 locales,
English placeholder elsewhere per the catalog's translation pass plan)
- Drop CANVAS_LEVEL_LABELS + remove now-unused CANVAS_LENS_REGISTRY import
Co-Authored-By: ruflo <ruv@ruv.net>
Closes 8f followup LOW #20 — load-bearing CanvasLensPicker had no dedicated test. 18 enabled/disabled cells (3 levels × 6 lenses) + click-dispatch + aria-label assertions. Co-Authored-By: ruflo <ruv@ruv.net>
…comments Closes 8f followup LOW #18 — viewStore.ts:140 + preferencesStore.ts:178 still mentioned wallLayoutStore in doc-strings after the PR1 rename to canvasViewportStore. Co-Authored-By: ruflo <ruv@ruv.net>
…/core/frame Closes 8f followup HIGH #2 — AuthorL3View's private focalStepColumns helper duplicates business logic that should live in core/frame. The helper now lives at packages/core/src/frame/stepColumns.ts with 6 unit tests; AuthorL3View imports it. Per ADR-074 amendment + ADR-081: Canvas embeds owner-surface computation, doesn't re-derive. Co-Authored-By: ruflo <ruv@ruv.net>
Closes 8f followup MEDIUM #8 — SystemLevelView trusted a flat specLimits prop without ADR-073-anchored contract. Now accepts measureSpecs keyed by column and derives from measureSpecs[map.ctsColumn] internally; old prop renamed to specLimitsOverride (advisory/debug only, deprecated). Canvas passes { [ctsColumn]: { usl, lsl, target, cpkTarget } } as measureSpecs. Two regression tests assert the leak scenario: wrong-column measureSpecs key produces '--' Cpk, not a silently wrong value. Co-Authored-By: ruflo <ruv@ruv.net>
…duplicate Same ADR-074 amendment violation that PR2 fixed in AuthorL3View. The private helper now delegates to getStepColumnAssignments from @variscout/core/frame (introduced in the prior commit), flattening the structured result into the string[] this view needs. Co-Authored-By: ruflo <ruv@ruv.net>
Closes 8f followup HIGH #4 via AMEND path (not expand). Git blame shows both `performance` and `yamazumi` lenses were introduced with `enabled: false` AND registry descriptions explicitly labeling them as "Future ... lens" — intentional V2 placeholders, not bugs. Spec §10 was over-promised at original ship. - Spec §10 matrix amended: 6 cells marked `(V2 — deferred; lens not enabled in V1)` instead of pretending they ship enabled - V2 expansion path documented inline - investigations.md entry marked RESOLVED 2026-05-13 with rationale Co-Authored-By: ruflo <ruv@ruv.net>
…4.4) l3 without focalStepId now emits console.warn and returns the viewport unchanged instead of throwing. fitToContent guards the same path. Updated test asserts warn was called and state did not change. Co-Authored-By: ruflo <ruv@ruv.net>
…viewport (4.7) Move FIT_TO_CONTENT_ZOOM_BY_LEVEL from canvasViewportStore into @variscout/core/canvas/viewport.ts and re-export it through the barrel. Add LOD_SNAP_BOUNDARIES (L2_OVERVIEW_LOW=0.5, L2_DETAIL_HIGH=1.8) for the upcoming snap-to-LOD feature. Single source of truth for level math. Co-Authored-By: ruflo <ruv@ruv.net>
…ce(6) (4.3) Adds .clickDistance(6) to the d3-zoom behavior in useCanvasViewportInput. Pointer moves ≤5px are treated as clicks; ≥6px as drags. Matches spec §6.3. Co-Authored-By: ruflo <ruv@ruv.net>
…ort (4.5/4.6) worldToWallSvg was an identity function with no callers outside its own test; deleted function and test. CanvasViewport is used in Canvas/index.tsx — added JSDoc comment explaining its role so the seam is documented. Co-Authored-By: ruflo <ruv@ruv.net>
…(4.2) Adds a 'end' listener to the zoom behavior. When the user releases the wheel with zoom in [0.3, 0.5) or [1.8, 2.0), the viewport eases back to 0.5 or 1.8 respectively over 150ms. Exports snapTarget() for unit testing. LOD_SNAP_BOUNDARIES lives in @variscout/core/canvas/viewport alongside LOD_THRESHOLDS. Co-Authored-By: ruflo <ruv@ruv.net>
…final) LODSwitcher now renders both outgoing and incoming renderers in stacked absolute divs during a 150ms window, then unmounts the outgoing. Uses useState+useEffect+setTimeout — no external animation library needed. useCanvasViewportInput snap-to-LOD uses d3-transition via selection.transition().duration(150).call(zoomBehavior.transform, ...). Adds d3-transition + @types/d3-transition to @variscout/hooks deps. Tests: 4 LODSwitcher tests assert dual-render during transition and single-render after 150ms via fake timers. Co-Authored-By: ruflo <ruv@ruv.net>
Closes 8f followup HIGH #1 part 1/2 — adds loadBlobCanvasViewport + saveBlobCanvasViewport mirroring the updateBlobEvidenceSnapshots ETag-conditional pattern. Per ADR-081 §2 (Azure = IndexedDB + Blob sync with ETag per ADR-079) and ADR-079. Also adds getLocalViewportUpdatedAt to @variscout/stores so the Azure lifecycle hook can compare timestamps without reading Dexie directly. Co-Authored-By: ruflo <ruv@ruv.net>
Closes 8f followup HIGH #1 part 2/2 — useCanvasViewportLifecycle (Azure) now rehydrates from Blob after Dexie cache, debounced-persists to both Dexie and Blob with ETag, and treats precondition-failed as last-write- wins per spec §11 with App Insights telemetry on the conflict path. Co-Authored-By: ruflo <ruv@ruv.net>
… granularity Closes 8f followup MEDIUM #9 — LocalMechanismView previously only exposed Quick Action. Spec §5.3.a lists 5 CTAs at column-mechanism granularity (Quick Action / Focused Investigation / IP / Sustainment / Handoff). Threaded the 4 step-level callbacks already on CanvasProps through to LocalMechanismView; per-column button row added with new i18n keys (8 new MessageCatalog keys across 32 locale files). Parent callbacks are step-only; column is visible only within the card row. Co-Authored-By: ruflo <ruv@ruv.net>
Closes 8f followup MEDIUM #10 — MobileLevelPicker previously called setLevel('l2') before setZoom(2.5) as an explicit l2 redirect comment implied. The final committed state was already l3 (setZoom(2.5) fires inferLevel→l3), but the intent was undocumented. Updated comment to clarify the atomicity: both calls run synchronously before React re-renders, so the final state is l3 with no focalStepId, and canvas renders NoFocalStepPrompt (the step-list surface) per spec §7. Co-Authored-By: ruflo <ruv@ruv.net>
… hook Closes 8f followup MEDIUM #11 — useCanvasViewportInput previously subscribed to the whole canvasViewportStore; every unrelated mutation (setRailOpen, setViewMode, openChartCluster, etc.) fired syncElementToStoreViewport (which has its own diff-check guard, but still did needless work). Now tracks prevViewportRef and short-circuits on reference equality of state.viewports[hubId] — sync is skipped entirely when the hub's viewport slice hasn't changed. Test added: setRailOpen → d3 element __zoom unchanged. Co-Authored-By: ruflo <ruv@ruv.net>
…er-hub Closes 8f followup LOW #15 — canvasViewportStore state is keyed by hubId not projectId since the 8f shape change. The annotation-per-project label was technically truthful (per-project umbrella, hub-keyed inside) but invited confusion. Renamed to annotation-per-hub; layerBoundary test + packages/stores/CLAUDE.md table updated. 'annotation-per-project' is now in the reserved/unused set; 'annotation-per-hub' is live. Co-Authored-By: ruflo <ruv@ruv.net>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
jukka-matti
added a commit
that referenced
this pull request
May 14, 2026
- investigations.md "8f followups" entry: STATUS → RESOLVED 2026-05-14, PR #166 squash-merged as cd93691. LOW #16 (Canvas/index.tsx 1122-line refactor) + LOW #19 (brand ProcessHubId) explicitly carried forward. - investigations.md gains two new entries from the --chrome walk: (1) pre-existing setState-in-render warning from AppMain across canvas transitions (App.tsx untouched by #166), (2) designer-lens canvas journey-clarity observations (9 UX items: Frame→Canvas rename, desktop LODSwitcher parity, Lock-canvas mode-toggle relabeling, L1 capability empty-state copy, L3 CTA visual weight, etc.). - decision-log.md "8f canvas viewport SHIPPED" entry: Amendment 2026-05-14 followups complete block prepended; original 2026-05-13 amendment retained as history. Co-Authored-By: ruflo <ruv@ruv.net>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A 3-agent retrospective on shipped 8f (PRs #160–#165) surfaced 20 findings — 5 HIGH that qualified the "shipped" verdict, 8 MEDIUM spec-vs-shipped drift, 7 LOW cleanups. This PR closes 19 of them across 22 commits on
canvas-viewport-8f-followups. Plan:docs/superpowers/plans/2026-05-13-canvas-viewport-8f-followups.md. Tracking:docs/investigations.mdentry "8f canvas viewport — followup findings from 3-agent retrospective".HIGH findings closed
useCanvasViewportLifecycle(Azure) now rehydrates from Blob after Dexie cache, debounced-persists to both with ETag-conditional PUT, last-write-wins per spec §11 with App Insights telemetry on conflict. NewloadBlobCanvasViewport/saveBlobCanvasViewportinblobClient.tsmirrorupdateBlobEvidenceSnapshotsConditional. ADR-081 §2 now honored.getStepColumnAssignmentsto@variscout/core/frame; both AuthorL3View and LocalMechanismView now consume the shared helper. ADR-074 amendment + ADR-081 honored.variscout-wall-layoutDexie never deleted —deleteLegacyWallLayoutDb()exported fromcanvasViewportStore; fires fire-and-forget at module init. Misleading test tightened to assert deletion.performance+yamazumiwere intentional V2 placeholders ("Future ... lens" descriptions). Spec §10 amended; matrix reflects as-shipped truth.MessageCatalog(70 new keys across 32 locales; non-en use English placeholders pending translation pass). Plus the Canvas/index.tsx empty-state leak caught by review.MEDIUM findings closed (all 8)
'end'handlermeasureSpecs[map.ctsColumn]with ADR-073 regression testsetViewportLevelreplaces throw with warn + no-opclickDistance(6)LOW findings closed (6 of 7)
STORE_LAYERrenamedannotation-per-project→annotation-per-hubworldToWallSvgidentity deletedCanvasViewport.tsxconfirmed alive — JSDoc addedwallLayoutStoredoc-string references inviewStore.ts+preferencesStore.tsrefreshedCanvasLensPickertest added (33 new tests; 18-cell predicate matrix)FIT_TO_CONTENT_ZOOM_BY_LEVELco-located withLOD_THRESHOLDSin@variscout/core/canvas/viewportDeferred
ProcessHubId— 18-file refactor, LOW priority, low value vs risk. Tracked for its own future micro-PR.Canvas/index.tsx1122-line refactor — defer to next viewport feature per slice-cap discipline.Workflow
Executed via
superpowers:subagent-driven-developmentperfeedback_subagent_driven_default. Sonnet implementer + Sonnet reviewer per task; the user accepted single-PR-at-the-end mode partway through, so per-task PR ceremony was dropped for inline commits on this branch.Test plan
pnpm --filter @variscout/core test— 3392 passed (+6 stepColumns)pnpm --filter @variscout/stores test— 257 passed (+2 deletion-assertion)pnpm --filter @variscout/hooks test— 1192 passedpnpm --filter @variscout/ui test— 2039 passed (+39 across CanvasLensPicker, SystemLevelView, LODSwitcher, LocalMechanismView)pnpm --filter @variscout/azure-app test— 1304 passed (+17 across blobClient.viewport + useCanvasViewportLifecycle.blob)pnpm --filter @variscout/pwa test— 335 passed (no regression)pnpm --filter @variscout/ui build— greenpnpm --filter @variscout/azure-app build— green--chromewalk completed 2026-05-14 (PWA on worktree)--chrome walk summary (2026-05-14)
Walked PWA at desktop (1440×900) + mobile (420×900) on the Showcase: Fill Weight Investigation dataset against the followup-branch worktree.
Verified visually / live:
getStepColumnAssignmentsgeometry: AuthorL3 (author mode) + LocalMechanismView (read mode) both render CTQ-first + tributaries-in-order column layout from same helper.variscout-wall-layoutlegacy Dexie absent after init (verified viaindexedDB.databases());variscout-canvas-viewportpersists viewport state (zoom,pan,focalStepId,currentLevel) across reload.opacity:0.4,cursor:not-allowed, and the spec §10 amended-truth tooltip copy ("Future within-step channel lens." / "Future time-study lens.").[MISSING:…]/[key.path]/MessageCatalogpatterns; zero leaks.Lock canvastoggle to read mode reveals all 4 response-path CTAs (Investigate / Charter / Sustain / Handoff) on every column card; Investigate CTA correctly routes to Investigation tab + hub composer.focalStepId.Verified by code + test evidence (not visually reachable in Showcase):
apps/azure/server.js+ Vite proxy = 3-tier dev setup not pre-configured); 522 new test lines (useCanvasViewportLifecycle.blob.test.ts:291+blobClient.viewport.test.ts:231) cover rehydrate, ETag-conditional PUT, last-write-wins on 412, and App Insights conflict telemetry.--(expected; contract is wired per ADR-073 regression test in feat(8f-followup): close 19 of 20 retrospective findings (single-PR cleanup workstream) #166).NoFocalStepPromptonly renders when nofirstStepIdexists; auto-focal-step logic inCanvas/index.tsx:502-504short-circuits the path on any non-empty map.clickDistance(6)confirmed on d3-zoom call atuseCanvasViewportInput.ts:87, +69 lines of input-mode tests.setViewportLevelwarn behavior: store not exposed globally for runtime probe; covered by unit test.Side findings (NOT introduced by this PR, NOT blocking):
setState-in-renderwarning fires fromAppMainrepeatedly across LOD/mode/tab transitions:Cannot update a component (AppMain) while rendering a different component (AppMain).apps/pwa/src/App.tsxis not touched on this branch (git log origin/main..HEAD -- apps/pwa/src/App.tsxis empty), so this is pre-existing — likely a 70+-selector AppMain calling a hook that triggers setState during render. Deserves andocs/investigations.mdentry post-merge.🤖 Generated with ruflo