refactor(web): decompose MemorySection into a features/memory vertical slice - #5228
refactor(web): decompose MemorySection into a features/memory vertical slice#5228leonaburime-ucla wants to merge 37 commits into
Conversation
The 6 uncovered branches (99.15% → not 100%) are intentionalEvery one is a genuinely-unreachable defensive/SSR/concurrency guard. Covering them would mean deleting real robustness for a coverage digit, so they're left as-is (no
For context: the reachable dead branches were removed properly rather than ignored — a redundant component guard that duplicated its formatter's check, an over-broad helper signature (nullable param its only caller never passes null), and a couple of TS-required fallbacks the runtime guarantees (regex-captured groups) turned into asserted accesses. The SSR guards in |
lefarcen
added
size/XXL
lefarcen
added
the
skip-validation
|
A generated Nix hash refresh is available for this PR. Apply the diff below with diff --git a/nix/pnpm-deps.nix b/nix/pnpm-deps.nix
index baac30c..be31fd5 100644
--- a/nix/pnpm-deps.nix
+++ b/nix/pnpm-deps.nix
@@ -9,6 +9,6 @@
# 1. Temporarily set the consuming `hash = lib.fakeHash;`
# 2. Run the relevant nix build/flake check
# 3. Copy the expected hash printed by Nix into the matching field below
- daemonHash = "sha256-k2mLkO5BXrVXpqYNuJbSxwQ4TzR+Hd35UG5tLxLvwww=";
- webHash = "sha256-mKqQx7UcW1hBli9iSJiSv+JJte++nKJC81+NchP29k4=";
+ daemonHash = "sha256-qaEKwHilBDyetmfE1WN6rzXgtGMyO+FImOXjuvb+Dkw=";
+ webHash = "sha256-IALVCmzaRtCMSbmXAHEeoq5tEZ4DuKnV4It1NG5tLxM=";
}
|
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
@leonaburime-ucla I reviewed the memory-slice extraction, the new provider seams, and the added hook/component coverage in the changed ranges. I didn't find any actionable issues in this refactor; the behavior-preserving split looks coherent, and the expanded tests cover the connector, extraction, advanced-editor, and orchestration paths well. I couldn't rerun the web Vitest suite in this prepared worktree because dependencies are not installed here, but the code and test changes themselves look solid. Nice work landing a large decomposition without widening the product surface.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.
Visual regression reviewHead: 0 changed · 53 unchanged · 0 new without baseline · 0 failed Unchanged cases
Visual diff is advisory only and does not block merging. |
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
I found two issues that should be fixed before this lands: one correctness bug in the new master-toggle flow, and one gap between the slice boundary this PR documents and what the new guard actually enforces.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.Location: scripts/check-web-slice-boundaries.ts RIGHT line 177
This guard only walks collectSourceFiles(featuresDir), so it never inspects imports from non-feature files into features/<slice>/.... In this same PR, MemorySection.tsx now deep-imports ../features/memory/components/*, hooks/*, formatters, and rules, even though ADR 0002 says the slice root index.ts is the public boundary and pnpm guard should enforce that contract. As written, the canary stays coupled to slice internals while the new guard still reports success. Please either move the orchestrator behind the slice public API (or into the slice) and extend this check to reject deep imports from outside features/**, or narrow the ADR/PR claim so the boundary being introduced matches what the code actually enforces.
Inline comment could not be anchored: inline anchor is outside the PR diff anchorable ranges
| const onToggleEnabled = useCallback( | ||
| async (next: boolean) => { | ||
| setEnabled(next); | ||
| await port.patchConfig(enabledPatch(next)); |
There was a problem hiding this comment.
onToggleEnabled() updates local state optimistically but never rolls it back when patchConfig() rejects the write. In this slice the provider now uses false to signal a non-2xx PATCH, and onToggleHook() already handles that case by restoring the previous flag value; the master toggle ignores it. If /api/memory/config fails here, the UI can show memory enabled or disabled while the daemon keeps the old value. Please mirror the per-flag path in this handler: keep the previous enabled value, await patchConfig(enabledPatch(next)), and restore the prior state on false (and on thrown transport errors as well).
…the slice barrel from outside Addresses @nettee's CHANGES_REQUESTED on nexu-io#5228. 1. onToggleEnabled optimistically flips the master switch but never rolled back a rejected write. The provider signals a non-2xx PATCH with `false` (the per-flag path already handles this); the master toggle ignored it, so a failed /api/memory/config write could leave the UI and daemon out of sync. Capture the prior value and restore it on both a `false` result and a thrown transport error, mirroring onToggleHook. Adds two red-first tests. 2. The slice boundary guard only walked features/, so MemorySection.tsx — the orchestrator, which lives outside the slice — deep-imported features/memory/{components,hooks,formatters,rules} while the guard still reported success, contradicting ADR 0002's claim that the slice index.ts is the public boundary. Route the orchestrator through the expanded features/memory barrel and extend the guard to reject deep imports into a slice from any file outside features/**. ADR wording updated to match. web typecheck + pnpm guard pass; 235 memory/orchestrator tests green. Guard verified to fail on a reintroduced deep import.
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
I found two issues that should be fixed before this lands: one state-consistency bug in the config hook, and one remaining hole in the new slice-boundary guard.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.Location: scripts/check-web-slice-boundaries.ts RIGHT line 244
This new outside-in guard still skips every non-relative specifier, but apps/web/tsconfig.json defines the @/* path alias. That means @/features/memory/hooks/useMemoryConfig.hooks or @/features/other-slice/components/... bypasses both the cross-slice and orchestrator checks even though the ADR and the comments in this file now claim the barrel boundary is enforced for every consumer. Because the guard is part of the canary contract for future slices, this leaves the advertised boundary trivially unenforced in the normal import style supported by the package. Please resolve tsconfig path aliases here (at least @/features/...) before applying the barrel/deep-import checks, and add a guard test/fixture that proves both relative and aliased deep imports are rejected.
Inline comment could not be anchored: inline anchor is outside the PR diff anchorable ranges
| async (key: MemoryConfigFlagKey, next: boolean) => { | ||
| const setter = setters[key]; | ||
| setter(() => next); | ||
| const ok = await port.patchConfig(singleFlagPatch(key, next)); |
There was a problem hiding this comment.
onToggleHook() only rolls the optimistic state back when patchConfig() resolves false. The provider added in this PR still delegates straight to fetch() (apps/web/src/providers/memory/config.ts), so a network failure rejects instead of returning false. In that case this handler leaves the local flag flipped even though the daemon never applied the change, which is the same UI/server divergence the master-toggle fix just addressed. Please mirror onToggleEnabled() here as well: keep the previous value, wrap the await in try/finally (or catch + rethrow), restore the prior flag on false and on thrown errors, and add a hook test covering the thrown-error path next to the new master-toggle regression test.
There was a problem hiding this comment.
Apologies, notifications werent on. Fixing now
…the slice barrel across @/ aliases Addresses the two remaining blocking review items on PR nexu-io#5228. useMemoryConfig.onToggleHook previously restored the optimistic per-hook flag only when patchConfig() resolved false; a thrown transport error (the provider delegates to fetch(), which rejects on network failure) left the flag flipped, diverging the UI from the daemon. Mirror onToggleEnabled: capture the prior value inside the functional update and restore it in a finally on both a false result and a thrown error, letting the error propagate. Adds a regression test for the thrown-error path next to the master-toggle one. check-web-slice-boundaries previously skipped every non-relative specifier, so the @/* tsconfig path alias (rooted at apps/web) let @/src/features/<slice>/... deep-import slice internals past both the cross-slice and outside-in checks the ADR says the barrel enforces. Resolve the @/ alias the same way scripts/guard.ts does, fold the two walkers into one pure, disk-free collectImportBoundaryViolations so the rules are unit-testable, and add a guard test proving both relative and aliased deep imports are rejected (cross-slice and outside-in) while barrels — including a trailing-slash form — stay allowed. Wire the test into pnpm guard.
|
@leonaburime-ucla I'm holding off on generating review comments for #5228 because this pull request has merge conflicts right now. Please resolve the conflicts with main and push the updated branch. Once that's done, request or wait for the review to run again and I'll take another look. 🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos. |
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
@leonaburime-ucla I re-reviewed the current MemorySection slice extraction, with particular focus on the two earlier blocker themes: the config-hook rollback paths and the slice-boundary guard. This head closes those gaps cleanly: the optimistic config toggles now roll back on both rejected and thrown PATCHes, the orchestrator consumes the slice through the barrel, and the guard now covers both outside-in imports and @/-aliased deep imports with targeted tests. I wasn’t able to rerun the Vitest suite in this prepared worktree because node_modules is not installed here, but the changed ranges and added coverage look coherent. Nice work tightening the canary contract and carrying the follow-up fixes through without widening the refactor surface.
CI: single failure is an unrelated flaky Playwright timeoutThe two red checks are actually one root cause:
Why this looks like a flake rather than a regression from this PR:
I don't have rerun rights on this repo, so I'm pushing an empty commit to force a fresh CI run to confirm the flake clears. If it recurs deterministically I'll dig further, but I expect it to go green. |
|
Hey @leonaburime-ucla — yes, behavior-preserving frontend refactors are in scope here. The main guardrails are the same ones we discussed in #5201: keep each follow-up scoped to one surface, preserve behavior, and carry the tests / boundary checks with the refactor instead of turning it into a repo-wide reshape. For the rest of the candidates in #5201, I'd use #5228 as the reference once it lands and then open them one surface at a time. That makes it much easier to review the next refactors against a merged canary instead of having multiple slice patterns moving in parallel. |
|
Ok @lefarcen I think Ill do one more refactor of a frontend canary and then wait to see if these gets merged so Im not doing work that wont be ultimately merged. I was very disappointed to see that the barrel fixes were discarded. I really think a lot of this code is brittle and needs to be drastically refactored for maintainability, bug fixes, and to stop accruing technical debt |
|
That makes sense. Let's use #5228 to settle the canary first, then treat the next refactors the same way: one surface at a time, behavior-preserving, with the slice boundaries and tests landing together. I agree the brittleness concern is real — the main thing we want to avoid is reopening the same boundary questions across multiple parallel PRs before this pattern is fully accepted. |
|
Ok @lefarcen. To be clear, I should wait on if this gets merged before doing more refactors correct? I will not start another one then. Im wondering when this would be merged though...I know you may not have a timeline but want to make sure that Amy Shang-alt has plans to actually merge this before continuing. |
|
Yes — if your goal is to avoid building on an unaccepted pattern, waiting for #5228 to actually land before starting the next refactor is the safer call. I can't promise a merge timeline or speak for anyone else's queue from here. There is already a current-head approval on the PR, but until it's actually merged I'd still use this canary as the reference point rather than stack more follow-up refactors on top of it. |
|
Will do @lefarcen, will also wait on Maintainer status news |
|
Thanks @leonaburime-ucla — sounds good. Holding on the current head until a maintainer picks it up is the right next step here. |
lefarcen
added
needs-validation
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
I found two correctness regressions that are still present on this head: one stale-state merge in the extraction-history reload path, and one required-read provider path that still masks a malformed success response.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.Location: apps/web/src/providers/memory/entries.ts RIGHT lines 31-34
The port contract in features/memory/ports.ts says fetchMemoryEntry() may return null only for a genuine 404, but this still maps any 2xx body without entry to null. If the daemon ever regresses to 200 {} here, openPreview() / startEdit() will silently treat a broken required-read response as "entry not found" instead of surfacing the backend failure. That is the same masking problem this PR is trying to remove from the read path. Please fail fast on malformed 2xx payloads as well: require json.entry to exist once resp.ok is true, throw if it does not, and update providers-entries.test.ts so the malformed-success case rejects instead of expecting null.
Inline comment could not be anchored: inline anchor is outside the PR diff anchorable ranges
| : confirmed.filter((row) => { | ||
| if (pending.has(row.id)) return false; | ||
| const deletedAt = deletedAtRevisionRef.current.get(row.id); | ||
| if (deletedAt !== undefined && deletedAt > sinceRevision) return false; | ||
| // Any row the live stream touched after the recovery read began | ||
| // is newer than the snapshot — keep the local copy, not the | ||
| // fetched one, so a phase transition never regresses. | ||
| return (rowRevisionRef.current.get(row.id) ?? 0) <= sinceRevision; |
There was a problem hiding this comment.
reconcileConfirmedExtractions() drops any fetched row whose id was touched after sinceRevision, so a same-id confirmed row never reaches acceptedById here and the survivor path below keeps the local copy instead. That means reloadExtractions() can get stuck on stale phase data: if a reload starts, an SSE frame updates extraction a to running, and the fetch then resolves with a already at success, we keep showing running even though the confirmed read is newer. This breaks the new reconciliation path the refactor introduced for manual refreshes and recovery reloads. Please compare same-id local vs fetched rows during reconciliation and keep whichever snapshot is actually newer for that id, then add a regression for reload starts -> SSE running -> fetch returns success.
|
Hey @leonaburime-ucla — @nettee's latest blocking review is the one to work from on this head: the extraction-history reload path can still keep a stale local row when the fetched row for the same id is newer, and 💡 To drive this PR to merge hands-free, paste this to your AI coding agent (Claude Code / Codex / opencode / Cursor …): |
…tion order Addresses the review at nexu-io#5228 (review) reconcileConfirmedExtractions() infers "which version of a row is newer" from CLIENT-SIDE reception order (rowRevisionRef: was this id touched locally after sinceRevision?). That works for races the client itself orders (reload vs reload, delete vs reload) because the client genuinely knows which one it started first. It does not work here: a reload's GET and the live SSE stream are BOTH SERVER-originated, racing over two independent channels, so client-side arrival order tells you nothing about which server-side event actually happened first. A GET can resolve with a MORE advanced state (e.g. 'success') than an SSE frame that was sent earlier but arrives later (e.g. 'running') — the old code kept 'running' solely because it arrived after sinceRevision, even though it was strictly less progressed. Same-id rows are now reconciled by their own content: extraction phases only ever move running -> exactly one terminal result (success/failed/skipped), so a terminal phase always outranks 'running' regardless of arrival order; a completion-timestamp (finishedAt) tiebreak covers the (should-not-happen) case of two terminal payloads for the same id. A second, related gap: MemoryExtractionCard exposes delete for 'running' rows (no phase gating), so an SSE phase-transition frame for an id can race a pending delete for that SAME id. Phase frames for an id with an in-flight delete are now buffered and only replayed if the delete fails (discarded if it succeeds); an id that has ever been tombstoned deleted is permanently ignored by future phase frames (extraction ids are attempt UUIDs, never reused, per packages/contracts's own doc comment). Investigated and implemented independently by codex (gpt-5.6-terra, high effort), briefed only with the reported finding + the actual severity rubric the automated reviewer bot (nettee/Looper) uses — not my own diagnosis, to avoid biasing its analysis. Re-verified here: read the full diff, confirmed the MemoryExtractionCard claim by reading the component directly, confirmed `finishedAt`/phase enum against packages/contracts, and independently re-ran all 3 new regression tests red-then-green (reverted just the hook file against the new tests to confirm each fails without the fix). Full memory + MemorySection suite (312 tests) and typecheck clean; guard unaffected and still passes.
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
I found three blocking correctness issues that are still present on the current head.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.Location: apps/web/src/providers/memory/entries.ts RIGHT line 34
fetchMemoryEntry() still breaks the new port contract: the port says null means a genuine 404, but this branch also maps any successful response without an entry field to null. That turns a malformed success payload into a silent "not found" path in preview/edit, so a required read failure is rendered as empty state instead of surfacing the backend regression. Please fail fast once resp.ok is true: require json.entry to exist, throw when it does not, and update the provider test so the malformed-success case rejects instead of expecting null.
Inline comment could not be anchored: inline anchor is outside the PR diff anchorable ranges
Location: scripts/check-web-slice-boundaries.ts RIGHT line 480
Rule 4 still has a concrete false negative here: a provider directory is recognized only when its root file is literally index.ts. In the same script, sourceExtensions was widened to include .js, .mjs, .cts, and friends, and the header comment says suffix changes should not become a bypass, but a provider folder rooted at index.js or index.mjs is still invisible to providerResourceHomes(). That means duplicate transport-home detection silently stops working for one of the source shapes this guard now claims to support, and the added JS coverage test only exercises flat provider files, not a JS-backed provider folder. Please look for any supported index.* extension when deciding whether a provider directory is a declared home, and add a regression fixture for a provider folder rooted at a non-TS index file.
Inline comment could not be anchored: inline anchor is outside the PR diff anchorable ranges
| // See the delete path above for why this reads extractionsRef instead of | ||
| // the render-closure `extractions` state. | ||
| const previous = extractionsRef.current; | ||
| const optimisticRevision = updateExtractions([]); |
There was a problem hiding this comment.
clearExtractions() still leaves a stale-reload window open. The optimistic wipe happens here, but the clear tombstone is only stamped later in the success branch after await port.clearExtractionHistory(). If a concurrent reloadExtractions() starts during that gap, it captures the post-optimistic revision, fetches a pre-clear server snapshot, and the reconciler accepts those rows because it still cannot tell that a clear is in flight. The result is that rows the user just cleared can temporarily repopulate until the DELETE settles, which is the same stale-read-wins race this PR is otherwise hardening against. Please mark the clear as authoritative before awaiting the DELETE, or track an in-flight clear generation that the reconciler honors, and add a regression for clear starts -> reload starts -> stale pre-clear read resolves before DELETE.
|
Hey @leonaburime-ucla — the current head is blocked on @nettee's latest review plus his extraction-history follow-up above: Once those three points are fixed on a new push, that's the review thread to work from for the next pass. |
…ing model Supersedes the ~13 incremental race-fix commits on this file (06aef19 through 88f9219 plus the uncommitted clear-vs-SSE fix). Each of those patched one reported interleaving by bolting on another ad hoc ref — revision counters, per-row revisions, destructive-event revision stamps, tombstone maps compared with strict/non-strict boundaries, a reload generation, a refcounted pending set, a deferred-frame buffer, a pending- clear refcount — until the file was correct only by enumeration: every guard existed because a reviewer had found the one scenario it blocks. The rewrite replaces that accretion with a single pure store (createExtractionHistoryStore) built on three ordering rules, each matched to what can actually be ordered: - Client-ordered facts share ONE logical clock. Optimistic writes and GET starts are genuinely sequenced by this client, so every commit advances one clock and every snapshot reconciles against the clock captured at its start. Reload-vs-reload ordering reuses the slice's shared AsyncCommitGuard (latest-started call wins outright). - Server-content-ordered facts are compared by content, never arrival. A GET row and an SSE frame for the same id race over two independent server channels, so reception order proves nothing; phase progression (running -> exactly one terminal result) plus a finishedAt tiebreak decides. - Confirmed removals are permanent. Attempt ids are UUIDs and never reused, so a removal the server confirmed (SSE deleted/cleared frame, or an acknowledged local delete/clear) tombstones the id forever. Any payload still carrying a tombstoned id is stale for it by definition. These rules handle the historical race classes by construction rather than one-guard-per-scenario: reload-vs-reload, delete-vs-reload (including the same-clock equality boundary), clear-vs-reload (including a reload spanning the in-flight clear window), delete-vs-delete (same and different ids, via one refcount), SSE-phase-vs-GET-content in both directions, delete-vs-SSE (buffer/replay/tombstone), clear-vs-SSE (the clear owns every id observed before its success), clear-vs-delete, and failed-mutation recovery with every combination of newer additive, destructive, and same-id-progressing events landing mid-recovery. The public MemoryExtractionsController contract and the MemoryExtractionsPort are unchanged. The test file is reorganized as the interleaving matrix itself — one describe block per event-source pair — carrying every scenario from the prior commits' regression suites and adding new coverage the old structure never pinned: superseded-reload rejection not surfacing a load error, late running frames not regressing terminal rows, terminal-vs-terminal finishedAt tiebreaks, a never-displayed id first observed during a pending clear staying gone, a remote cleared frame tombstoning an id hidden by a pending local delete, a clear succeeding over an overlapping same-row delete that later fails, and a post-clear SSE arrival surviving a rejected pre-clear snapshot. Validated: apps/web memory + MemorySection suites green (325 tests), package typecheck introduces no errors in changed files (pre-existing stale-contracts failures unrelated), and the web slice-boundary guard passes.
…unrelated to this PR)
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
I found two blocking issues that are still present on the current head.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.Location: apps/web/src/providers/memory/entries.ts RIGHT line 34
fetchMemoryEntry() still breaks the port contract here. MemoryEntriesPort says null means only a genuine 404, and useMemoryEntries.openPreview() / startEdit() rely on that distinction to treat other failures as required-read errors. Returning json.entry ?? null turns a malformed 200 {} into the same not-found path, so a backend regression silently renders an empty preview or skips opening the editor instead of surfacing the broken read. The added provider test currently codifies that bad case by expecting null for {} as well. Please fail fast once resp.ok is true: require json.entry to exist, throw if it does not, and update apps/web/tests/features/memory/providers-entries.test.ts so malformed 2xx payloads reject instead of being treated as not-found.
Inline comment could not be anchored: inline anchor is outside the PR diff anchorable ranges
Location: scripts/check-web-slice-boundaries.ts RIGHT lines 479-484
providerResourceHomes() still recognizes provider folders only when they contain index.ts. Earlier in this file, sourceExtensions and the header comment explicitly say JS/MJS/CTS/etc. source files are part of the supported scan surface, but a folder rooted at providers/<name>/index.js or index.mjs is skipped entirely here. That leaves rule 4 with a concrete false negative: duplicate transport homes inside a non-TS provider folder will pass pnpm guard even though this PR advertises suffix changes as a hardened case. Please detect any supported index.* file when declaring a provider folder home, and add a regression fixture in scripts/check-web-slice-boundaries.test.ts for a JS-backed provider directory so this boundary stays enforced across the file types the guard claims to support.
Inline comment could not be anchored: inline anchor is outside the PR diff anchorable ranges
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
I found three blocking issues that are still present on the current head.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.Location: apps/web/src/providers/memory/entries.ts RIGHT line 34
MemoryEntriesPort says fetchMemoryEntry() returns null only for a genuine 404, but this still maps any successful body without entry to null. That turns a malformed 200 {} into the same path as a real not-found, so openPreview() / startEdit() silently render empty state or no-op instead of surfacing a broken required-read response. The test added in apps/web/tests/features/memory/providers-entries.test.ts currently codifies that malformed-success case as null too, so this regression is now locked in. Please fail fast once resp.ok is true: require json.entry to exist, throw if it does not, and update the malformed-2xx test to expect rejection.
Inline comment could not be anchored: inline anchor is outside the PR diff anchorable ranges
Location: scripts/check-web-slice-boundaries.ts RIGHT line 480
providerResourceHomes() still recognizes provider folders only when they contain index.ts, even though sourceExtensions above explicitly says .js, .mjs, .cts, and the other supported source suffixes are part of this scan surface. A provider home rooted at providers/<name>/index.js or index.mjs is therefore invisible here, so duplicate transport-home detection silently stops working for one of the file shapes this guard now claims to harden. Please treat any supported index.* file as the folder barrel and add a regression fixture in scripts/check-web-slice-boundaries.test.ts for a non-TS provider directory root.
Inline comment could not be anchored: inline anchor is outside the PR diff anchorable ranges
| }, [pendingConnectorAuthIds, port]); | ||
|
|
||
| const reloadConnectors = useCallback(async () => { | ||
| const revision = connectorStatusCommitGuardRef.current.begin(); |
There was a problem hiding this comment.
reloadConnectors() and refreshConnectorStatuses() currently share connectorStatusCommitGuardRef, so a later status-only refresh invalidates an earlier catalogue reload mid-flight. In the reloadConnectors() path that means the discovery response at lines 167-169 is discarded even though the later refresh never fetched replacement details. If OAuth polling/callback fires while the Connected tab is loading, the hook can clear connectorsLoading and leave the old or synthetic connector metadata (tool counts, tool lists, labels) on screen indefinitely with no error. Please order status snapshots and catalogue discovery independently: let the latest discovery reload commit its details, merged with the latest status map, and add a regression for reload starts -> refresh starts -> refresh resolves -> reload discovery resolves.
|
Hey @leonaburime-ucla — the current head is still blocked on @nettee's latest review + follow-up comment: Once those are addressed on a new push, that's the review thread to work from for the next pass. |
Follow-up to 429ac90. useMemoryExtractions.hooks.ts was 582 lines: a 76-line concurrency-model doc comment, the pure store (createExtractionHistoryStore + mergeSameAttempt/phaseProgress + the op-token types), and the React shell. Since the store is owned by exactly one hook (unlike the slice-shared rules.ts/formatters.ts), it is extracted to a sibling module using the hooks/ directory's existing suffix convention: useMemoryExtractions.store.ts alongside useMemoryExtractions.hooks.ts, rather than a shared slice-root file. The hook file is now a 206-line thin shell: port I/O for reload/delete/clear, the reload-vs-reload AsyncCommitGuard wiring, the derived UI state (no-provider banner, connector partition, relative-time clock), and pairing store transitions with setState. The concurrency model's doc comment (the three ordering rules) now lives with the code that implements it, in the store file. Also corrected one comment while moving it: settleDeleteSuccess's re-assert commit does not, as the prior comment claimed, guard against a concurrent recovery snapshot resurrecting a pending-delete id — commitSnapshot's own pending-delete filter already makes that unreachable. The re-assert is genuinely redundant against `rows` content, but the commit it performs still matters: it advances the clock so a sibling operation's restoreIfUnchanged correctly treats this confirmation as new information. Verified this reframing with a direct test (store.test.ts: "never lets a concurrent commitSnapshot resurrect an id while its delete is still pending"). Added a dedicated pure-logic test file, useMemoryExtractions.store.test.ts, mirroring the slice's rules.test.ts/formatters.test.ts pattern (plain function calls, no renderHook/act, no jsdom pragma). Given how much of this rewrite's correctness argument lives in the store's three ordering rules, testing them directly — synchronous calls against the store's own methods rather than through React's act()/promise-timing machinery — makes a store regression fail at the exact rule it violates instead of surfacing as a harder-to-diagnose hook-level timing failure. The hook-level useMemoryExtractions.test.tsx is unchanged and remains the proof that the React shell wires the store correctly end-to-end. Validated: apps/web memory + MemorySection suites green (367 tests, up from 325 — 42 new direct store tests), typecheck introduces no errors in changed files, and the web slice-boundary guard passes.
… provider folders Addresses the review at nexu-io#5228 (review) Two findings, both previously identified in an earlier review round but not yet landed: - providers/memory/entries.ts's fetchMemoryEntry() mapped ANY 2xx response without an `entry` field to null, collapsing a malformed success payload into the same "not found" path a genuine 404 takes. useMemoryEntries' openPreview()/startEdit() rely on that null meaning "doesn't exist" to silently render an empty preview or skip opening the editor — a backend regression would go unnoticed instead of surfacing as a required-read failure. Now throws when resp.ok is true but json.entry is missing. The provider test previously codified the bad behavior (expected null for a 200 {} body); fixed to expect a rejection, plus a new dedicated test for the malformed-2xx case. - scripts/check-web-slice-boundaries.ts's providerResourceHomes() only recognized a provider folder when it contained index.ts specifically, even though sourceExtensions (and the guard's own header comment) were already widened to scan .js/.jsx/.mjs/.cjs/.mts/.cts. A provider folder rooted at index.js or index.mjs was invisible to rule 4's "one transport home per route" check — a real false negative for exactly the file shapes this guard advertises supporting. Now checks for any supported index.<ext>. Added a regression fixture (a JS-backed provider folder duplicating a route with a flat provider file) that spies on the guard's violation output to confirm the JS folder is named as one of the route's homes, verified red against the pre-fix code before this change. Both regressions confirmed red-then-green; typecheck clean; full guard test suite (28/28) and the entries/useMemoryEntries suites pass.
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
I found two blocking correctness issues that are still present on the current head: one stale-commit race in the connector catalogue reload path, and one remaining malformed-success path in the extracted memory-config provider.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.Location: apps/web/src/providers/memory/config.ts RIGHT line 48
patchMemoryExtractionConfig() still treats a malformed 2xx payload as if the daemon intentionally cleared the extraction config. The caller (MemoryModelInline.persist()) uses undefined for a failed PATCH and null for a real clear, so a response like 200 { enabled: true } will silently clear the saved override and run the normal success flow instead of surfacing a broken /api/memory/config echo. That is the same contract-masking bug class this PR already fixed for fetchMemoryEntry(). Please require the extraction field to be present on every successful response, throw when it is missing, and update apps/web/tests/features/memory/config-provider.test.ts so the malformed-success case expects failure instead of null.
Inline comment could not be anchored: inline anchor is outside the PR diff anchorable ranges
| setConnectorStatuses(statuses); | ||
| setConnectors((prev) => applyMemoryConnectorStatuses(prev, statuses)); | ||
| const next = await port.fetchMemoryConnectors(); | ||
| if (!connectorStatusCommitGuardRef.current.isCurrent(revision)) return; |
There was a problem hiding this comment.
reloadConnectors() and refreshConnectorStatuses() still invalidate each other through the same connectorStatusCommitGuardRef. A concrete case is: the Connected tab opens and starts reloadConnectors(), then an OAuth poll/callback fires refreshConnectorStatuses() while fetchMemoryConnectors() is still in flight. When discovery resolves here, isCurrent(revision) is false, so the real catalogue details are discarded, connectorsLoading still drops to false, and the UI is left on stale or synthetic connector rows until a manual reload. Please order discovery reloads independently from status-only refreshes so the latest catalogue response can still commit its details merged with the latest status map, and add a regression that interleaves reloadConnectors() with a later refreshConnectorStatuses().
|
Hey @leonaburime-ucla — the current head is still blocked on @nettee's latest two findings: the stale-commit race between Once those two paths are fixed on this head, we can re-check the canary from there. |
…rmed config PATCH echo useMemoryConnectors.hooks.ts: reloadConnectors and refreshConnectorStatuses shared one commit guard, so a status-only refresh could invalidate an in-flight catalogue fetch and silently discard the discovery response while loading still cleared. Split into independent status and catalogue guards, merge the catalogue commit against the latest status ref (kept in sync synchronously, not just via effect), and invalidate the catalogue guard on a successful connect upsert so a stale reload can't overwrite it. Adds 3 regression tests for the reload/refresh/connect interleavings. providers/memory/config.ts: patchMemoryExtractionConfig() collapsed a malformed 2xx response missing the extraction field into the same null used for a real clear, masking a broken /api/memory/config echo as an intentional clear. Now distinguishes field-absent (throws) from field-present-null (legitimate clear), matching the fix already applied to fetchMemoryEntry(). Reached via a 2-round multi-model debate (host, codex, Gemini 3.1 Pro, a Fable subagent) that converged unanimously on this fix shape, then verified by an independent codex audit that found and fixed one residual gap (the status ref sync timing) before this commit.
…eModuleName scripts/check-web-slice-boundaries.ts had 10+ rounds of an automated reviewer finding a new bypass in its hand-rolled specifier resolution (missed aliases, extensions, directories, syntax forms) because rules 2-3 answered "what does this specifier resolve to" with a growing list of special cases instead of the real algorithm. Rebuilt on ts.resolveModuleName against the actual parsed apps/web/tsconfig.json: - resolveWebImport tries real resolution first for every non-relative specifier (including @/*, so a remapped alias is never silently misresolved via a hardcoded assumption); falls back to the old hand-mapped @/* join only when real resolution fails outright (e.g. a case-mismatched specifier on a case-sensitive filesystem, which must still be caught, not silently dropped). - Rule 3's outside-in file enumeration now derives its scan set from the real compiler-resolved file list (parsed fresh, not the memoized per-specifier cache) instead of a hardcoded [src, app] root pair — sidecar/ and standalone-included files (next.config.ts) are now covered. tests/ is deliberately excluded: this repo's convention is that test files reach into slice internals directly to unit-test them, which is not the outside-in barrel escape rule 3 exists to catch. - Rule 1 (transport-free slice files) extended to catch self.fetch/self[...] alongside globalThis, and destructuring a forbidden global out of either object — as a renamed, computed, or string-literal key, from a variable initializer, a parameter default, or a plain assignment expression, and through parenthesized/as-expression/computed-key wrappers. - Kept the already-hardened AST specifier walk and rule 4 unchanged, per the debate's verdict that a full ts.Program/type-checker isn't warranted here — bare module resolution is the load-bearing piece. Reached via a 2-round multi-model debate (host, codex, Gemini 3.1 Pro, a Fable subagent), unanimous on ts.resolveModuleName over a full ts.Program or an ESLint-plugin migration (this repo has no ESLint anywhere). Verified by three rounds of independent codex (gpt-5.6-terra) audits, each of which found and got real bugs fixed before this commit: a rule-1 destructuring bypass (computed/string-literal keys, parameter defaults), a case-sensitive node_modules exclusion, a remapped-@/*-alias false negative, unwrapped expression forms hiding a destructure, incomplete config-driven file enumeration, and an assignment-expression destructuring variant. 40 guard tests pass (12 new), typecheck clean, pnpm guard's slice-boundary check passes.
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
I found two blocking contract-handling issues that are still present on this head.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.Location: apps/web/src/providers/memory/entries.ts RIGHT line 53
saveMemoryEntry() currently collapses a malformed 200 {} into the same null path as a real non-2xx save failure. In this slice null is treated as an ordinary "save did not happen" result (useMemoryEntries.onSave() just skips reload/flash, and connector-suggestion saves count it as a partial miss), so a broken daemon response is silently downgraded to a no-op instead of surfacing the contract regression. Please require json.entry on every successful POST/PUT response, throw when it is missing, and update providers-entries.test.ts so the malformed-success case rejects rather than expecting null.
Inline comment could not be anchored: inline anchor is outside the PR diff anchorable ranges
Location: apps/web/src/providers/memory/extractions.ts RIGHT line 13
fetchExtractions() currently treats a 200 {} as an empty history. That turns a malformed success payload into an authoritative "there are no extractions" snapshot, so useMemoryExtractions.reloadExtractions() can clear the visible history and suppress loadError instead of surfacing a broken /api/memory/extractions response. Please require json.extractions on successful reads (allow [] only when the field is actually present) and update providers-extractions.test.ts so the missing-field case rejects instead of expecting an empty list.
Inline comment could not be anchored: inline anchor is outside the PR diff anchorable ranges
|
Hey @leonaburime-ucla — @nettee's latest blocking review on this head is the one to work from now: Once those two contract-handling paths reject instead of silently falling back, reviewers can take another pass on this head. |
Six functions across apps/web/src/providers/memory/*.ts independently hand-rolled the same malformed-2xx check: entries.ts's fetchMemoryTree, fetchMemoryEntry, and saveMemoryEntry; config.ts's patchMemoryExtractionConfig; extractions.ts's fetchExtractions; connectors.ts's fetchMemoryConnectors. Each one, separately, collapsed a successful response missing a required field into the same fallback value used for a genuinely empty/null/cleared result, so a broken backend response silently read as "nothing here" instead of surfacing the contract regression. nettee found this shape six times, one function at a time. Added response-fields.ts: requiredField() (field must be present, its value may legitimately be null/empty) and requiredNonNullField() (field must be present AND non-null, for entities with no legitimate empty result). All six call sites now route through one of these instead of their own ad-hoc check, so the "field absent vs. field present with a falsy value" distinction is made once. 70 tests pass (12 new, across a dedicated response-fields.test.ts plus updated provider tests), typecheck clean.
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
I found three blocking state-flow regressions that are still present on the current head; the concrete fixes are in the inline comments below.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.| if (previewRequestTokenRef.current !== token) return; | ||
| setPreviewBody(entry?.body ?? ''); | ||
| }, | ||
| [previewId, port], | ||
| ); | ||
|
|
||
| const startEdit = useCallback( | ||
| async (id: string) => { | ||
| const token = ++editRequestTokenRef.current; | ||
| let entry; | ||
| try { | ||
| entry = await port.fetchMemoryEntry(id); | ||
| } catch { | ||
| // A stale request's failure must not clobber a newer action's state. | ||
| if (editRequestTokenRef.current !== token) return; | ||
| setLoadError(LOAD_ERROR_MESSAGE); | ||
| return; | ||
| } | ||
| if (!entry || editRequestTokenRef.current !== token) return; | ||
| openEditor(); | ||
| setEditing({ | ||
| id: entry.id, | ||
| name: entry.name, | ||
| description: entry.description, | ||
| type: entry.type, | ||
| body: entry.body, |
There was a problem hiding this comment.
openPreview() and startEdit() both set loadError on a rejected fetchMemoryEntry(), but neither success path clears that error again. After one failed preview/edit read, the next successful read updates previewBody or editing here while the old banner stays visible, so the section still looks broken even though the retry succeeded. The new tests exercise the failure cases, but there is no coverage for failure -> success clears the banner. Please clear loadError once a current preview/edit request completes successfully (before or alongside setPreviewBody / setEditing), and add a regression test for a failed read followed by a successful retry.
| const refreshConnectorStatuses = useCallback(async () => { | ||
| const revision = connectorStatusGuardRef.current.begin(); | ||
| const statuses = await port.fetchConnectorStatuses(); | ||
| if (!connectorStatusGuardRef.current.isCurrent(revision)) return; | ||
| const statusChanged = connectorStatusesChanged(connectorsRef.current, statuses); | ||
| commitConnectorStatuses(statuses); | ||
| setConnectors((prev) => applyMemoryConnectorStatuses(prev, statuses)); | ||
| setPendingConnectorAuthIds((prev) => { | ||
| const next = new Set(prev); | ||
| for (const connectorId of prev) { | ||
| if (statuses[connectorId]?.status === 'connected') next.delete(connectorId); | ||
| } | ||
| return next.size === prev.size ? prev : next; | ||
| }); | ||
| setConnectorConnectErrors((prev) => { | ||
| let changed = false; | ||
| const next = { ...prev }; | ||
| for (const [connectorId, status] of Object.entries(statuses)) { | ||
| if (status.status === 'connected' && next[connectorId] !== undefined) { | ||
| delete next[connectorId]; | ||
| changed = true; | ||
| } | ||
| } | ||
| return changed ? next : prev; | ||
| }); | ||
| if (statusChanged) port.notifyConnectorsChanged(); | ||
| }, [port, commitConnectorStatuses]); | ||
|
|
||
| // NOTE: the OAuth mid-auth status poll and popup-callback message listener are | ||
| // deliberately NOT effects here — they open accumulating browser | ||
| // subscriptions, so the orchestrator (a guaranteed single instance) owns them | ||
| // and drives `refreshConnectorStatuses` below. See the file header. | ||
|
|
||
| const onConnectMemoryConnector = useCallback(async (connectorId: string) => { | ||
| if (connectingConnectorIds.has(connectorId)) return; | ||
| setConnectingConnectorIds((prev) => new Set(prev).add(connectorId)); | ||
| setConnectorConnectErrors((prev) => { | ||
| if (prev[connectorId] === undefined) return prev; | ||
| const next = { ...prev }; | ||
| delete next[connectorId]; | ||
| return next; | ||
| }); | ||
| try { | ||
| const result = await port.connectConnector(connectorId); | ||
| if (result.connector?.status === 'connected') port.notifyConnectorsChanged(); | ||
| const requiresAuthorizationCompletion = | ||
| result.auth?.kind === 'redirect_required' || result.auth?.kind === 'pending'; | ||
| setConnectors((prev) => | ||
| upsertMemoryConnector( | ||
| prev, | ||
| requiresAuthorizationCompletion && result.connector | ||
| ? connectorWithPendingAuthorization(result.connector) | ||
| : result.connector, | ||
| ), | ||
| ); | ||
| // This upsert just committed newer-than-any-in-flight-discovery truth | ||
| // for this connector. Invalidate any older `reloadConnectors` catalogue | ||
| // fetch so its (now stale) wholesale replace can't land after this and | ||
| // silently overwrite what was just connected. | ||
| connectorCatalogueGuardRef.current.invalidate(); | ||
| if (result.error) { | ||
| setConnectorConnectErrors((prev) => ({ ...prev, [connectorId]: result.error! })); | ||
| setPendingConnectorAuthIds((prev) => { | ||
| if (!prev.has(connectorId)) return prev; | ||
| const next = new Set(prev); | ||
| next.delete(connectorId); | ||
| return next; | ||
| }); | ||
| return; | ||
| } | ||
| if (result.auth?.kind === 'redirect_required' || result.auth?.kind === 'pending') { | ||
| setPendingConnectorAuthIds((prev) => new Set(prev).add(connectorId)); | ||
| } else { | ||
| setPendingConnectorAuthIds((prev) => { | ||
| if (!prev.has(connectorId)) return prev; | ||
| const next = new Set(prev); | ||
| next.delete(connectorId); | ||
| return next; | ||
| }); | ||
| } | ||
| await refreshConnectorStatuses(); |
There was a problem hiding this comment.
refreshConnectorStatuses() is driven from the background OAuth poll/message subscriptions, but it has no rejection handling at all, and onConnectMemoryConnector() awaits it after this hook has already upserted connector state locally. If fetchConnectorStatuses() throws transiently, the connect flow rejects after a partial local state update, pendingConnectorAuthIds / connectorConnectErrors never reconcile, and the failure is not surfaced through connectorLoadError or a connector-scoped error. Please catch rejected status refreshes inside this path, keep the local optimistic/upserted state intact, and report the refresh failure through explicit connector UI state instead of letting the whole action reject mid-handshake; a targeted test for connect succeeds, refresh rejects would pin the recovery path.
| const onSaveConnectorSuggestions = useCallback(async () => { | ||
| if (selectedConnectorSuggestions.length === 0) return; | ||
| setConnectorSaving(true); | ||
| setConnectorError(null); | ||
| try { | ||
| const saved: MemoryEntry[] = []; | ||
| const savedSuggestionIds = new Set<string>(); | ||
| for (const suggestion of selectedConnectorSuggestions) { | ||
| const entry = await port.saveMemoryEntry({ | ||
| id: memoryEntryIdForConnectorSuggestion(suggestion), | ||
| name: suggestion.name, | ||
| description: suggestion.description, | ||
| type: suggestion.type, | ||
| body: suggestion.body, | ||
| }); | ||
| if (entry) { | ||
| saved.push(entry); | ||
| savedSuggestionIds.add(suggestion.id); | ||
| } | ||
| } | ||
| await reload(); | ||
| const savedEntriesById = new Map(saved.map((entry) => [entry.id, entry])); | ||
| setConnectorSuggestions((prev) => | ||
| prev.filter((suggestion) => !savedSuggestionIds.has(suggestion.id)), | ||
| ); | ||
| setSelectedSuggestionIds( | ||
| new Set( | ||
| selectedConnectorSuggestions | ||
| .filter((suggestion) => !savedSuggestionIds.has(suggestion.id)) | ||
| .map((suggestion) => suggestion.id), | ||
| ), | ||
| ); | ||
| setConnectorStatus( | ||
| `Saved ${savedEntriesById.size} memor${savedEntriesById.size === 1 ? 'y' : 'ies'} from connected apps.`, | ||
| ); | ||
| if (savedEntriesById.size !== selectedConnectorSuggestions.length) { | ||
| setConnectorError( | ||
| `Saved ${savedEntriesById.size} of ${selectedConnectorSuggestions.length} selected memories. Please try the remaining items again.`, | ||
| ); | ||
| } | ||
| } catch (err) { | ||
| setConnectorError(err instanceof Error ? err.message : String(err)); |
There was a problem hiding this comment.
This loop only reconciles savedSuggestionIds after all saves finish. If one suggestion saves successfully and a later saveMemoryEntry() throws, control jumps to the outer catch before reload(), before pruning the saved suggestions, and before recomputing selection. That leaves already-persisted suggestions rendered as if they were still unsaved and retryable, so the UI diverges from server state after a mixed success, then throw batch. Please preserve the successes even when a later save throws: reload, remove the already-saved suggestions from local state, keep only the unsaved ones selected, and add a regression test for the mixed-outcome case (the current suite covers null partial saves and all throw, but not some saved before throw).
|
Hey @leonaburime-ucla — @nettee's current-head review already covers the blockers that need changing here: clearing the stale load-error banner after a successful retry, preserving already-saved connector suggestions across mixed save outcomes, and handling connector-status refresh failures without leaving auth state half-updated. Once those are addressed on this head, we can take another look. |
…verage to ~97% Three nettee-reported bugs fixed (all with regression tests): - useMemoryEntries.hooks.ts: openPreview/startEdit never cleared a stale loadError on a subsequent successful read. - useMemoryConnectors.hooks.ts: refreshConnectorStatuses had no rejection handling when driven from the background OAuth poll/callback, and onConnectMemoryConnector awaited it after already upserting optimistic state, so a transient failure rejected the whole connect action mid-flight. - useMemoryConnectors.hooks.ts: onSaveConnectorSuggestions only reconciled saved suggestions after ALL saves finished, so a save that succeeded before a LATER save threw still rendered as unsaved/retryable. Two more races found via an independent codex trace of every async path in these hooks (framed as: does each throw/race match that file's OWN established contract, and is it already proven by an existing test): - onConnectMemoryConnector's connectConnector() call itself had no catch (only its trailing refreshConnectorStatuses did) — a thrown connect propagated as an unhandled rejection with no connect-error state set. - Two synchronous onConnectMemoryConnector calls in the same React batch both passed the re-entrancy guard, since it read un-committed state — fixed with a synchronously-updated ref alongside the state. - onSaveIndex's success handler unconditionally cleared indexDraft, so editing the draft again while an earlier save was still in flight silently discarded the newer, unsaved edit. One incorrect fix attempted and reverted: initially "fixed" useMemoryConfig .hooks.ts's onToggleEnabled/onToggleHook to swallow a thrown PATCH instead of rethrowing after rollback — reverted after finding two pre-existing tests proving the rethrow was an intentional, already-tested contract. Structural fix for a 6x-repeated bug class: providers/memory/response- fields.ts adds requiredField/requiredNonNullField, and all six memory provider functions that previously hand-rolled their own malformed-2xx check now route through it. Coverage pushed from ~95% branches / 98% functions to 96.8% branches / 100% functions / 99.82% statements / 99.9% lines across the memory feature slice (features/memory + providers/memory), via ~30 new tests plus two behavior-preserving refactors: MEMORY_CONNECTOR_APP_LABELS is now typed to require every MEMORY_CONNECTOR_APP_IDS entry (removing a dead `?? id` fallback), and a genuinely-unreachable branch in onSaveConnectorSuggestions (guaranteed dead by the function's own early return) was simplified away instead of tested. 370 tests pass, typecheck clean.
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
I found one blocking issue in the current head: the new strict /api/memory/config provider can now reject on malformed 2xx responses, but MemoryModelInline still calls it as if it were the old non-throwing helper.
| setBusy(true); | ||
| try { | ||
| const result = await saveMemoryExtraction(next); | ||
| const result = await patchMemoryExtractionConfig(next); |
There was a problem hiding this comment.
patchMemoryExtractionConfig() now throws when a 2xx /api/memory/config response is missing the required extraction field, but persist() still treats this call as if it only returned undefined on failure. That leaves both save paths here broken on the new contract: the click handlers (await persist(...)) reject out of the React event with no user-visible failure state, and the debounced BYOK re-sync below (void persist(...)) becomes an unhandled promise rejection. The provider test added in this PR already locks in the thrown malformed-success case, so this caller needs to absorb it explicitly. Please wrap the await in try/catch, keep the prior config on rejection, surface a local failure state instead of throwing out of the component, and add a component test that drives a malformed-success response through both a user-triggered save and the debounced re-sync path.
|
Hey @leonaburime-ucla — @nettee's latest current-head review is the blocker set to work from now: Once that path is updated on a new push, that's the review thread to use for the next pass. |
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
I found one blocking issue that is still present on the current head.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.Location: apps/web/src/providers/memory/entries.ts RIGHT lines 18-23
fetchMemoryList() now validates only entries, but the slice immediately consumes the rest of MemoryListResponse as required data. useMemoryConfig.hydrate() reads enabled plus the four hook flags, and useMemoryEntries.reload() commits rootDir and index; with the current code a malformed 200 like { entries: [] } is treated as success, clears loadError, and then hydrates those fields to undefined instead of surfacing a broken /api/memory response. Please either validate the full MemoryListResponse contract here (at least the fields this shared read path actually commits) or narrow this helper to a smaller DTO and adjust callers/tests accordingly. providers-entries.test.ts and wired-hooks.test.tsx should then cover the missing-field case so this malformed-success path stays closed.
Inline comment could not be anchored: inline anchor is outside the PR diff anchorable ranges
|
@nettee @AmyShang-alt @lefarcen @kokisanai Why this PR accumulated so many review rounds - Before this closes for good, I want to leave a clear record of what actually happened here, because I think it matters beyond this one PR. The refactor did not introduce these bugs. It exposed pre-existing ones. I verified this directly against the original monolithic MemorySection.tsx (2,636 lines, pre-refactor) via git show, and every major bug class @nettee found in the decomposed code has a byte-for-byte analogous version already sitting in the original file:
These errors are multiplied throughout the code Why none of this was ever caught: the original component has a 2,312-line test file with 29 tests, and every one of them is happy-path. creates a new memory entry and refreshes the list, suggests and saves memory from selected connected apps, clears extraction history after clicking Clear — the suite proves the feature works, but there is not a single test for a failed fetch, a malformed response, two operations racing, or a retry after failure. This is a real, structural gap in what the existing tests exercise. The scale of what this means: MemorySection.tsx is one settings tab. If a component of this size and age has this many undetected async-correctness bugs, and the reason they went undetected is a systemic pattern (large components, happy-path-only test suites), there's a reasonable chance this isn't isolated to memory. This is worth treating as a signal about poor test quality and bad code that works but is horribly unstable on the frontend I'm not going to attempt to reopen or resubmit this — that's not the point of this comment. Just wanted the actual root cause on the record before it's gone: bad in original code, correctly exposed by decomposition + a genuinely thorough reviewer, not caused by the refactor itself. Ive worked all day on this and gotten pretty much nowhere because theres always an infinite set of edge cases the Looper is finding. Its fundamentally flawed but being hidden due to how the original code was compiled. Hopefully you guys can take this as a signal to seriously think about how this code needs to be rebuilt in a new version. I think there are these problems all throughout the project |
|
@leonaburime-ucla Thanks for writing this up clearly. I checked the current head and the changed memory-provider paths before replying. I think the important distinction is the one you're drawing: this PR did not invent the underlying fragility in memory, but it still needed to close the remaining blockers on the branch it was asking us to merge. On the current head, the latest blocking point is narrower than "the refactor caused all of this": So I don't read this thread as "the decomposition created fake problems." I read it as "the decomposition exposed a real backlog, and this PR still needed that backlog closed all the way through before it could land." The broader point about large happy-path-heavy frontend tests missing async and contract failures is fair, and it's useful to have that context left on the record. |
4 participants
Part of #5201
Why
Use case: I'm decomposing the large
apps/web"god-components" into vertical slices — the direction agreed in #5201 and written up indocs/adr/0002-frontend-vertical-slice-decomposition.md.MemorySection.tsxwas the maintainer-approved canary: a 2,617-line component with ~30 pieces of state, inline transport, pure rules, and presentational markup all fused together.Pain: These god-components are hard to test, review, and change safely — section state leaks up into the shell, transport is duplicated across surfaces, and there's no seam to unit-test logic. The disease is state relocation, not file length. This PR proves the slice pattern end-to-end on one component so the rest of the effort has a concrete, reviewed template.
What users will see
Nothing — this is a behavior-preserving refactor. The Memory settings section renders and behaves identically: same DOM, same class names, same flows (Work profile / Add manually / Import-from-apps tabs, OAuth connect, scan → suggest → save, extraction history, the advanced
MEMORY.mdindex editor). No visual or functional change.Surface area
Note: the
@vitest/coverage-v8devDep was added to the workspaceapps/web/package.json, not the root — so it's out of scope for the root-dependency box, but flagged here for transparency. Nocoverage.thresholdsCI gate is added in this PR; enforcing the bar is a planned follow-up so it doesn't change CI behavior for the whole package inside this canary.Structure
MemorySection.tsx: 2,617 → 552 lines, decomposed into a vertical slice.Each hook follows one paradigm: a real
useX(port, coord?)with transport injected as the slice port and pure logic imported directly, plus a per-hook wireruseWiredX()binding the real port. The orchestrator injects each hook as a prop defaulted to its wired version, so tests swap in a fake.scripts/check-web-slice-boundaries.ts(wired intopnpm guard) checks the boundaries: feature files never touchfetch/window/document; only a slice's rootdependencies.tsimportsproviders/; no cross-slice deep imports. This is a best-effort static-analysis guard for realistic, non-adversarial violations (hardened over review against path aliases, dynamic imports,require(),globalThisbracket access, template routes, flat/folder provider homes, JS/JSX/symlinks) — not an adversarial security boundary; see its own header comment for the exact scope. Full rationale in ADR 0002.Test metrics
Full unit coverage added for the slice — 233 tests, and every one of the 31 feature + associated files is ≥95% on all four metrics:
The only 6 remaining uncovered branches are intentional defensive/SSR/concurrency guards — enumerated in a comment below.
Bug fix verification
Not a bug fix — no red spec required. Behavior preservation is covered by the pre-existing
apps/web/tests/components/MemorySection.test.tsxend-to-end suite (extended, still green) plus the new slice unit tests.Validation
pnpm guard— pass (includes the new slice-boundary check)pnpm --filter @open-design/web typecheck— 0 errorspnpm --filter @open-design/web test— 233 passing (memory slice + orchestrator)🤖 Generated with Claude Code