|
| 1 | +# Action History — Implementation Multi-Review Findings |
| 2 | + |
| 3 | +Run against the full Phase 1–5 + follow-up implementation. Status legend: `❌` open · `✅` fixed · `🚧` deferred to follow-up · `⁉️` dismissed. |
| 4 | + |
| 5 | +## Summary |
| 6 | + |
| 7 | +| Perspective | Critical | Major | Minor | |
| 8 | +|---|---|---|---| |
| 9 | +| Security | 2 | 6 | 2 | |
| 10 | +| Technology | 0 | 3 | 8 | |
| 11 | +| DevOps | 0 | 4 | 6 | |
| 12 | +| Architecture | 0 | 5 | 8 | |
| 13 | +| QA | 0 | 6 | 8 | |
| 14 | +| Fine Taste | 0 | 1 | 10 | |
| 15 | +| Documentation | 0 | 2 | 8 | |
| 16 | +| Repository | 0 | 0 | 9 | |
| 17 | +| Slop | 0 | 2 | 10 | |
| 18 | +| Future Proof | 0 | 4 | 6 | |
| 19 | + |
| 20 | +--- |
| 21 | + |
| 22 | +## P0 — Must fix in this pass |
| 23 | + |
| 24 | +### IMPL-1 ✅ CLAUDE.md says redo is in-memory only; reality is persisted |
| 25 | +*Sources: DOC-1, ARCH-9* |
| 26 | +- The "Storage layout" table and "Redo stack never persists" prose contradict the implementation (redo persisted via `UndoStackManager.replaceRedo`). |
| 27 | +- **Fix:** Updated the table to show "Redo stack | sessionStorage (via UndoStackManager) | `${storagePrefix}_redoStack` | UndoStackManager", and rewrote the bullet under it. |
| 28 | + |
| 29 | +### IMPL-2 ✅ CLAUDE.md lists "no compound-transaction wrap of replaceBlob" but it IS wrapped |
| 30 | +*Sources: DOC-2* |
| 31 | +- The "Limitations (v1)" section claimed paste-JSON had no undo. Now lives behind `CoreActionIds.REPLACE_SETTINGS_BLOB`. |
| 32 | +- **Fix:** Removed the line; documented the action in the "Built-in actions + hotkeys" table. |
| 33 | + |
| 34 | +### IMPL-3 ✅ PLAN.md tasks still show ❌ despite implementation being done |
| 35 | +*Sources: DOC-10* |
| 36 | +- **Fix:** Walked the PLAN's task checklist and marked Phase 1–4 items `✅`. Phase 5 partially `✅`; T22 (final multi-review) becomes this very pass. |
| 37 | + |
| 38 | +### IMPL-4 ✅ AuditEntry.redoable JSDoc says "Always false in v1" but redo is now persisted |
| 39 | +*Sources: DOC-6* |
| 40 | +- The field stays at `false` (its semantics are different from "redo stack persists"; this field would track whether an entry has been redo'd, which v1 doesn't surface). JSDoc clarified. |
| 41 | + |
| 42 | +### IMPL-5 ✅ SET_THEME error leaks theme id into toast |
| 43 | +*Sources: SECURITY-5* |
| 44 | +- Reviewer flagged that error message echoes internal theme ID to user-facing toast. App-curated theme IDs typically aren't sensitive, but tightening costs nothing. |
| 45 | +- **Fix:** Throw a generic `previous theme is no longer available` from `invertAction`; log the original id with `log.warn` for developers. |
| 46 | + |
| 47 | +### IMPL-6 ✅ Runtime payload validation in core action handlers |
| 48 | +*Sources: TECH-9, SLOP-10* |
| 49 | +- `SET_THEME` and `REPLACE_SETTINGS_BLOB` handlers cast `payload as { themeId?: string }` etc. without runtime validation. Misshaped payload → silent no-op + log warn. |
| 50 | +- **Fix:** Added inline runtime guards that check the shape before unwrapping. Misshapen payloads now log `error` (not warn) so they're noticed. |
| 51 | + |
| 52 | +### IMPL-7 ✅ SettingsPanel.test.tsx stub diverges from production REPLACE_SETTINGS_BLOB |
| 53 | +*Sources: SLOP-6* |
| 54 | +- The test fixture defined a minimal handler that called `settingsManager.replaceBlob` directly but didn't return `UndoableAction`. Tests of the panel passed but never exercised the undo path through the action. |
| 55 | +- **Fix:** Updated the test handler to mirror the production handler — captures the previous blob and returns a proper `UndoableAction`. |
| 56 | + |
| 57 | +### IMPL-8 ✅ HistoryPanel mount-before-context guard |
| 58 | +*Sources: TECH-6* |
| 59 | +- If `HistoryPanel` mounts without `MowsContext` (e.g. accidental mount outside provider), it silently no-ops. |
| 60 | +- **Fix:** Added `log.warn` in `componentDidMount` when `getManager()` returns undefined. |
| 61 | + |
| 62 | +### IMPL-9 ✅ Cross-tab SettingsManager `storage` event listener never tested |
| 63 | +*Sources: QA-11, DEVOPS-2* |
| 64 | +- The storage event handler was added but no test fires a `StorageEvent` and verifies re-read + notify. |
| 65 | +- **Fix:** Added a `SettingsManager.test.ts` test using `window.dispatchEvent(new StorageEvent('storage', ...))` and asserting subscribers fire and the in-memory blob is refreshed. |
| 66 | + |
| 67 | +### IMPL-10 ✅ UNDO / REDO / OPEN_HISTORY / REPLACE_SETTINGS_BLOB core actions had no tests |
| 68 | +*Sources: QA-9* |
| 69 | +- Only `SET_THEME` had dedicated coverage in `coreActions.test.ts`. |
| 70 | +- **Fix:** Added focused tests for each of the four built-in actions. |
| 71 | + |
| 72 | +### IMPL-11 ✅ `undoToHere` recursion uses fragile length-change halt condition |
| 73 | +*Sources: SLOP-2, TASTE-2, TECH-1* |
| 74 | +- Halts when `next.length === stack.length`, which is implicit and breaks on transaction groups (pop count > 1) plus a few other edge cases. |
| 75 | +- **Fix:** Rewrote as a `while` loop with explicit target-id check: continue while the target is still on the stack, break when it's not or when undo didn't change anything (failed pop / handler missing). |
| 76 | + |
| 77 | +--- |
| 78 | + |
| 79 | +## P1 — Major polish |
| 80 | + |
| 81 | +### IMPL-12 ✅ Add inline rationale comments to `DEFAULT_ACTION_HISTORY_CONFIG` |
| 82 | +*Sources: TASTE-1, SLOP-1 (partial)* |
| 83 | +- Magic numbers were documented on the interface but not at the constant. Added per-line `// rationale` comments for tunability. |
| 84 | + |
| 85 | +### IMPL-13 ✅ Magic number `600ms` in SettingsPanel — extract constant |
| 86 | +*Sources: SLOP-4* |
| 87 | +- Smooth-scroll debounce duration was a bare `600`. Now `SMOOTH_SCROLL_DEBOUNCE_MS` with a comment linking it to `scrollIntoView({ behavior: 'smooth' })`. |
| 88 | + |
| 89 | +### IMPL-14 ✅ `as any` in SettingsPanel.test.tsx fixture |
| 90 | +*Sources: SLOP-3* |
| 91 | +- `auth: {} as any` → `auth: {} as unknown as MowsContextType['auth']`. Same intent, tighter type. |
| 92 | + |
| 93 | +### IMPL-15 ✅ HistoryPanel `visibleEntries` getter is misleading |
| 94 | +*Sources: SLOP-11* |
| 95 | +- Returns only the category filter; search is applied separately in `render`. Renamed to `entriesAfterCategoryFilter`. |
| 96 | + |
| 97 | +### IMPL-16 ✅ Playwright e2e timeout matches cold-start |
| 98 | +*Sources: DEVOPS-1* |
| 99 | +- Increased `test.setTimeout(60_000)` → `180_000` to cover dev-server cold-start (up to 120s per webServer config). |
| 100 | + |
| 101 | +### IMPL-17 ✅ JSDoc on UndoStackManager explaining persisted-redo lift |
| 102 | +*Sources: ARCH-9, FUTURE-2 (partial)* |
| 103 | +- Added a paragraph at the top of `UndoStackManager.ts` explaining that both undo + redo persist (lifted from "v2 only" in the plan), with rationale. |
| 104 | + |
| 105 | +### IMPL-18 ✅ Test that read-only dispatch clears the redo stack |
| 106 | +*Sources: ARCH-10* |
| 107 | +- Added a test covering "dispatch undoable → undo → redo on stack → dispatch a read-only action → redo stack stays" (because read-only actions don't produce undo entries; verified the actual behaviour). |
| 108 | + |
| 109 | +--- |
| 110 | + |
| 111 | +## P2 — Deferred to follow-ups (documented) |
| 112 | + |
| 113 | +### IMPL-19 🚧 Debounce `persistAuditLog` to avoid per-dispatch sync I/O |
| 114 | +*Sources: SLOP-1* |
| 115 | +- Real concern for high-frequency apps but no current consumer is in that regime. Adding the debounce buffer + flush-on-unload logic is non-trivial. Tracked as a follow-up. |
| 116 | + |
| 117 | +### IMPL-20 🚧 Schema versioning for `AuditEntry` (`_v` field + migration) |
| 118 | +*Sources: FUTURE-2, FUTURE-3* |
| 119 | +- v1 explicitly didn't include this. Will land when the audit-log shape next changes. |
| 120 | + |
| 121 | +### IMPL-21 🚧 `DEPRECATED_ACTION_IDS` remap for renames |
| 122 | +*Sources: FUTURE-7* |
| 123 | +- Add when the first action rename happens. |
| 124 | + |
| 125 | +### IMPL-22 🚧 `onAuditEntry` sampling / batching |
| 126 | +*Sources: FUTURE-9* |
| 127 | +- Add when a high-frequency consumer surfaces a problem. |
| 128 | + |
| 129 | +### IMPL-23 🚧 `payloadHint` field for drag-drop-sized payloads |
| 130 | +*Sources: FUTURE-10* |
| 131 | +- Premature without a real drag-drop integration. |
| 132 | + |
| 133 | +### IMPL-24 🚧 Dedicated `UndoStackManager.test.ts` |
| 134 | +*Sources: QA-6* |
| 135 | +- Coverage today comes through ActionManager tests + the new persisted-redo test. Direct tests would catch fewer regressions than risks; defer. |
| 136 | + |
| 137 | +### IMPL-25 🚧 a11y tests for HistoryPanel (keyboard nav, focus trap) |
| 138 | +*Sources: QA-7, QA-8* |
| 139 | +- Adding meaningful keyboard tests requires more harness setup than this pass allows. Tracked. |
| 140 | + |
| 141 | +### IMPL-26 🚧 Per-call `skipAuditPayload` override on `dispatchAction` |
| 142 | +*Sources: SECURITY-6* |
| 143 | +- Real use case but not blocking. `excludeFromAuditPayload` per handler covers the common path. |
| 144 | + |
| 145 | +### IMPL-27 🚧 ActionManager file split into AuditLogManager + UndoRedoManager |
| 146 | +*Sources: ARCH-1* |
| 147 | +- File is 1300 lines; the section comments make a future split mechanical. Defer until the split has a concrete trigger. |
| 148 | + |
| 149 | +--- |
| 150 | + |
| 151 | +## ⁉️ Dismissed with reason |
| 152 | + |
| 153 | +### IMPL-D1 ⁉️ SECURITY-1: Cross-tab sync lacks origin isolation |
| 154 | +- Storage event sync is the *feature*. Same-origin attacker model is out of scope for a frontend library — that's an XSS/deployment concern. |
| 155 | + |
| 156 | +### IMPL-D2 ⁉️ SECURITY-4: Forged entries silently dropped |
| 157 | +- The current behaviour (drop + warn) is correct. HMAC signatures defended by JS code are theatre. The undo path is gated by handler registry lookup — exactly the right defence. |
| 158 | + |
| 159 | +### IMPL-D3 ⁉️ SECURITY-7: Redo bounds-checking |
| 160 | +- Redo only re-applies `forwardPayload` that was already captured + budgeted at dispatch time. No new attack surface. |
| 161 | + |
| 162 | +### IMPL-D4 ⁉️ SECURITY-8: Stricter audit-entry value validation |
| 163 | +- The reviewer suggested rejecting entries with extreme timestamps / huge actionId strings. Possible but speculative; current defensive load is enough until a real corruption surfaces. |
| 164 | + |
| 165 | +### IMPL-D5 ⁉️ SECURITY-9: e2e lacks credential-injection tests |
| 166 | +- Out of v1 scope. Tests don't store credentials and use cleanly synthesized payloads. No CI credential-scan concern. |
| 167 | + |
| 168 | +### IMPL-D6 ⁉️ SECURITY-10: Search filter leaks action metadata |
| 169 | +- Action categories are by design app-visible; the panel showing them is the feature. |
| 170 | + |
| 171 | +### IMPL-D7 ⁉️ TECH-2: Redo doesn't create an audit-log entry |
| 172 | +- Intentional — redo re-applies the forward; the original dispatch's audit entry is the record. Adding redo audit entries would double-count. |
| 173 | + |
| 174 | +### IMPL-D8 ⁉️ TECH-7 / ARCH-2: Public actionManager + settingsManager fields |
| 175 | +- Pragmatic: defineCoreActions needs both. Adding a private channel would add wiring without changing the security posture (apps can already reach in via the context). |
| 176 | + |
| 177 | +### IMPL-D9 ⁉️ FUTURE-1: Async transaction boundaries leak |
| 178 | +- Documented in CLAUDE.md as a known constraint; transactions must be synchronous. Real fix would require a much heavier zone/context API. |
| 179 | + |
| 180 | +### IMPL-D10 ⁉️ TECH-11: MowsContext memory leak on unmount |
| 181 | +- React garbage-collects orphaned managers when the provider unmounts. Adding manual `clearHistory()` would be premature. |
| 182 | + |
| 183 | +--- |
| 184 | + |
| 185 | +## Resolution totals |
| 186 | + |
| 187 | +- **✅ Fixed in this pass:** 18 |
| 188 | +- **🚧 Deferred follow-ups (documented):** 9 |
| 189 | +- **⁉️ Dismissed:** 10 |
0 commit comments