Split marm-console's Memory.tsx into per-tab component files#101
Merged
Conversation
Pure refactor, no behavior changes. Extracted via exact index-based line-slicing (all 4 moved bodies verified byte-identical to the original before any import cleanup) -- same methodology as the memory_ops.py split. - src/pages/Memory.tsx keeps only the MemoryPage shell: 1075 -> 53 lines. - src/components/memory/shared.tsx (new, 52 lines): ActionNotice type, mutationErrorMessage, deleteNotice, ActionNoticePanel -- used by all 5 tabs. - src/components/memory/MemoriesTab.tsx (new, 403 lines): MemoryRow + MemoriesTab. - src/components/memory/SessionsAndLogsTabs.tsx (new, 297 lines): SessionSummaryDialog + SessionsTab + LogsTab. - src/components/memory/NotebookAndCompactionTabs.tsx (new, 289 lines): HISTORY_STATUS_LABEL + CompactionHistoryRow + NotebookTab + CompactionTab. Split granularity (MemoriesTab alone, smaller tabs grouped in 2 files) confirmed with user directly before implementation. New components/memory/ directory mirrors the existing components/layout/ and components/ui/ convention, chosen over pages/memory/ specifically to avoid a case-only collision with pages/Memory.tsx on case-insensitive filesystems. Each extracted component referenced across file boundaries gained `export` (mechanical requirement -- TS won't let you import an unexported binding); components that stayed file-internal (MemoryRow, SessionSummaryDialog, CompactionHistoryRow) were left unexported, same visibility as before. Validated: tsc --noEmit clean, npm run build succeeds, and a real Playwright smoke pass against the running dev server confirmed all 5 tabs render and are clickable with zero JS console errors.
Contributor
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughChangesMemory console
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@marm-console/artifacts/marm-console/src/components/memory/MemoriesTab.tsx`:
- Around line 56-71: The selection logic around selectedIds, toggleSelect, and
toggleAll must discard IDs no longer present in the current data.items result.
Reconcile selectedIds whenever the displayed results change, and determine the
all-selected state by verifying every displayed memory ID is selected rather
than comparing set sizes. Ensure bulk deletion can only operate on currently
visible memories.
- Around line 124-145: Update handleUpdate so the mutation payload’s
context_type uses the same blank-to-null normalization as the local
selectedMemory update, rather than falling back to selectedMemory.context_type.
Preserve the existing fallback to 'general' only when that is the intended
non-blank default, and ensure clearing the field sends null to the server.
In
`@marm-console/artifacts/marm-console/src/components/memory/NotebookAndCompactionTabs.tsx`:
- Around line 17-28: The handleSave function must preserve the selected
notebook’s identity when editing an existing entry. Prevent changes to
editing.name, editing.project, or editing.platform for existing entries, or
route such edits through an atomic rename operation so the original record is
updated rather than leaving a duplicate; keep the current upsert behavior for
new entries.
In
`@marm-console/artifacts/marm-console/src/components/memory/SessionsAndLogsTabs.tsx`:
- Around line 98-107: Replace the independent mutation-error precedence with the
single latest-operation notice source, actionNotice. In
marm-console/artifacts/marm-console/src/components/memory/SessionsAndLogsTabs.tsx
lines 98-107 and 214-220, and
marm-console/artifacts/marm-console/src/components/memory/NotebookAndCompactionTabs.tsx
lines 42-49, route session, log, save, and delete outcomes through actionNotice
so stale mutation errors cannot mask newer results.
In `@marm-console/artifacts/marm-console/src/components/memory/shared.tsx`:
- Around line 13-36: Update deleteNotice so its base message reflects
deletedCount, including zero and partial deletions, instead of falling back to
“Memory deleted.” Return kind “warning” whenever missingCount > 0, while
preserving the existing concept-cleanup warning handling and success status only
when all requested IDs are deleted without cleanup issues.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cedeaaca-1105-4773-a17e-9f66626b9ac8
📒 Files selected for processing (5)
marm-console/artifacts/marm-console/src/components/memory/MemoriesTab.tsxmarm-console/artifacts/marm-console/src/components/memory/NotebookAndCompactionTabs.tsxmarm-console/artifacts/marm-console/src/components/memory/SessionsAndLogsTabs.tsxmarm-console/artifacts/marm-console/src/components/memory/shared.tsxmarm-console/artifacts/marm-console/src/pages/Memory.tsx
📜 Review details
🔇 Additional comments (5)
marm-console/artifacts/marm-console/src/pages/Memory.tsx (1)
1-4: LGTM!marm-console/artifacts/marm-console/src/components/memory/shared.tsx (1)
1-12: LGTM!Also applies to: 38-52
marm-console/artifacts/marm-console/src/components/memory/MemoriesTab.tsx (1)
1-55: LGTM!Also applies to: 72-123, 146-403
marm-console/artifacts/marm-console/src/components/memory/SessionsAndLogsTabs.tsx (1)
1-97: LGTM!Also applies to: 108-213, 221-297
marm-console/artifacts/marm-console/src/components/memory/NotebookAndCompactionTabs.tsx (1)
1-16: LGTM!Also applies to: 29-41, 50-78, 97-289
Comment on lines
+98
to
+107
| <ActionNoticePanel | ||
| notice={ | ||
| createSession.error | ||
| ? { kind: 'error', message: mutationErrorMessage(createSession.error) } | ||
| : deleteSession.error | ||
| ? { kind: 'error', message: mutationErrorMessage(deleteSession.error) } | ||
| : deleteAllSessions.error | ||
| ? { kind: 'error', message: mutationErrorMessage(deleteAllSessions.error) } | ||
| : actionNotice | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use one latest-operation notice source.
Independent mutation errors persist and can mask a newer success or failure from another action.
marm-console/artifacts/marm-console/src/components/memory/SessionsAndLogsTabs.tsx#L98-L107: route all session mutation outcomes throughactionNotice.marm-console/artifacts/marm-console/src/components/memory/SessionsAndLogsTabs.tsx#L214-L220: route all log mutation outcomes throughactionNotice.marm-console/artifacts/marm-console/src/components/memory/NotebookAndCompactionTabs.tsx#L42-L49: route save and delete outcomes throughactionNotice.
📍 Affects 2 files
marm-console/artifacts/marm-console/src/components/memory/SessionsAndLogsTabs.tsx#L98-L107(this comment)marm-console/artifacts/marm-console/src/components/memory/SessionsAndLogsTabs.tsx#L214-L220marm-console/artifacts/marm-console/src/components/memory/NotebookAndCompactionTabs.tsx#L42-L49
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@marm-console/artifacts/marm-console/src/components/memory/SessionsAndLogsTabs.tsx`
around lines 98 - 107, Replace the independent mutation-error precedence with
the single latest-operation notice source, actionNotice. In
marm-console/artifacts/marm-console/src/components/memory/SessionsAndLogsTabs.tsx
lines 98-107 and 214-220, and
marm-console/artifacts/marm-console/src/components/memory/NotebookAndCompactionTabs.tsx
lines 42-49, route session, log, save, and delete outcomes through actionNotice
so stale mutation errors cannot mask newer results.
All 5 findings were confirmed real, pre-existing bugs in the original Memory.tsx (carried over verbatim by the split). Fixing here since this branch is the only place with easy access to the touched code right now, same reasoning as PRs #99/#100's follow-up fixes. - MemoriesTab.tsx: selectedIds now drops IDs no longer in the visible result set whenever search/filter results change, so bulk delete can't act on hidden stale selections. toggleAll's 'all selected' check now verifies every displayed ID is selected instead of just comparing set sizes. - MemoriesTab.tsx: clearing the context type field now sends null to the server instead of falling back to the memory's previous value. - NotebookAndCompactionTabs.tsx: name/project/platform are the notebook entry's identity key, so they're now locked once editing an existing entry -- editing them on save previously created a duplicate record instead of updating the original. - SessionsAndLogsTabs.tsx + NotebookAndCompactionTabs.tsx: session, log, notebook, and compaction mutations now all route their success/error outcomes through the single actionNotice state instead of independent per-mutation .error checks that could mask a newer result behind a stale one. - shared.tsx: deleteNotice now derives its base message from the actual deletedCount (including zero) and returns 'warning' whenever any requested ID was missing, instead of always reporting success. Verified: tsc --noEmit clean, npm run build succeeds, Playwright smoke pass against the live dev server hit all 5 tabs with zero JS errors. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011s8tBNnQTmb1q5nRAKwhvv
CodeRabbit follow-up: selection cleanup runs in a useEffect that fires after render, so a stale ID could still be present in selectedIds at the moment handleBulkDelete reads it, if the result set changed on that same render. Recompute the target IDs against data.items directly inside handleBulkDelete instead of trusting the effect-reconciled state, so the confirmation count and the mutation payload can never include a hidden/stale memory even in that race window. tsc --noEmit clean, npm run build succeeds. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011s8tBNnQTmb1q5nRAKwhvv
Lyellr88
pushed a commit
that referenced
this pull request
Jul 18, 2026
Reverts the behavior change from 0d69abc. An independent review caught what I missed: marm-mcp-server's internal /internal/memories endpoint declares context_type as a required, non-empty str (min_length=1), and its recall path calls .upper() on it unconditionally. Forwarding an explicit null (what my earlier fix did) doesn't clear the field -- it gets rejected with a 422 from the real MCP server. My commit's test only stubbed mcp_client.request, so it verified the Console forwarded null correctly without ever touching the real cross-server contract that rejects it. Supporting a genuinely nullable context_type would need an intentional MCP/core contract change (nullable field, guard the .upper() call, audit other consumers) -- out of scope here. Restored the coercion, with a comment explaining why it's required rather than defensive. The actual frontend bug (PR #101 finding: fallback used stale selectedMemory.context_type instead of a fixed default) gets fixed correctly on that branch by falling back to 'general', not null. ruff check/format clean, full suite 27 passed / 0 failed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011s8tBNnQTmb1q5nRAKwhvv
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
2 participants
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.
Pure refactor, no behavior changes. Extracted via exact index-based line-slicing (all 4 moved bodies verified byte-identical to the original before any import cleanup) -- same methodology as the memory_ops.py split.
Split granularity (MemoriesTab alone, smaller tabs grouped in 2 files) confirmed with user directly before implementation. New components/memory/ directory mirrors the existing components/layout/ and components/ui/ convention, chosen over pages/memory/ specifically to avoid a case-only collision with pages/Memory.tsx on case-insensitive filesystems.
Each extracted component referenced across file boundaries gained
export(mechanical requirement -- TS won't let you import an unexported binding); components that stayed file-internal (MemoryRow, SessionSummaryDialog, CompactionHistoryRow) were left unexported, same visibility as before.Validated: tsc --noEmit clean, npm run build succeeds, and a real Playwright smoke pass against the running dev server confirmed all 5 tabs render and are clickable with zero JS console errors.
Summary by CodeRabbit