feat(memory): redesign settings ux#1901
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThis PR adds a manual memory update flow across presenter, service, route, contract, and renderer client layers, and replaces the memory settings UI with a redesigned shell built from inline configuration, list, diagnostics, inbox, persona, and empty-state components. It also updates shared status/result contracts, localization, and tests. ChangesManual Memory Update Backend Flow
Memory Settings UI Redesign
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![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
🧹 Nitpick comments (3)
src/main/presenter/sqlitePresenter/tables/agentMemory.ts (1)
1387-1422: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCentralize the conflict-pair predicate.
countConflictPairs()duplicates the same validity rules asConflictService.listConflicts(), so a future change can make the status count disagree with the list view. Extract the predicate into one shared helper or SQL fragment.🤖 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 `@src/main/presenter/sqlitePresenter/tables/agentMemory.ts` around lines 1387 - 1422, The conflict-pair validity logic is duplicated between countConflictPairs() and ConflictService.listConflicts(), so they can drift out of sync. Extract the shared predicate into a common helper or reusable SQL fragment, then have countConflictPairs() and the list-conflict path both use it so the challenger/target status checks stay identical.test/renderer/components/MemoryInlinePanel.test.ts (1)
145-360: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for archive, restore, and remove actions.
The suite thoroughly covers editing flows (create, update, noop, supersede, fold, stale responses, discard prompt, Escape handling), but the three footer action buttons — archive, restore, and delete — have no test coverage. These are destructive/irreversible user actions that warrant verification of correct
memoryClientcalls,changed/closeemission, and toast-on-error behavior.🧪 Suggested test additions
it('archives the memory and emits changed + close', async () => { const { wrapper, memoryClient } = await setup() const archiveButton = wrapper .findAll('button') .find((button) => button.text() === 'settings.memory.redesign.archive') await archiveButton!.trigger('click') await flushPromises() expect(memoryClient.archive).toHaveBeenCalledWith('deepchat', 'm1') expect(wrapper.emitted('changed')).toHaveLength(1) expect(wrapper.emitted('close')).toHaveLength(1) wrapper.unmount() }) it('restores an archived memory and emits changed', async () => { const { wrapper, memoryClient } = await setup({ item: memory({ status: 'archived' }) }) const restoreButton = wrapper .findAll('button') .find((button) => button.text() === 'settings.deepchatAgents.memoryManager.restore') await restoreButton!.trigger('click') await flushPromises() expect(memoryClient.restore).toHaveBeenCalledWith('deepchat', 'm1') expect(wrapper.emitted('changed')).toHaveLength(1) wrapper.unmount() }) it('removes the memory and emits changed + close', async () => { const { wrapper, memoryClient } = await setup() const deleteButton = wrapper .findAll('button') .find((button) => button.text() === 'settings.deepchatAgents.memoryManager.deletePermanent') await deleteButton!.trigger('click') await flushPromises() expect(memoryClient.remove).toHaveBeenCalledWith('deepchat', 'm1') expect(wrapper.emitted('changed')).toHaveLength(1) expect(wrapper.emitted('close')).toHaveLength(1) wrapper.unmount() })🤖 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 `@test/renderer/components/MemoryInlinePanel.test.ts` around lines 145 - 360, Add coverage for the footer actions in MemoryInlinePanel by testing the archive, restore, and delete flows. In the MemoryInlinePanel test suite, locate the existing setup-based cases and add assertions around the archive/restore/remove button handlers to verify the matching memoryClient methods are called with the current memory id, the expected changed and close events are emitted, and error paths show the toast behavior when those actions fail. Use the existing setup, memoryClient, and wrapper.emitted patterns to keep the new tests consistent with the other action-flow cases.test/renderer/components/MemoryDiagnosticsPanel.test.ts (1)
87-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for the
clearAlldestructive action.The
clearmock is configured but no test exercises the AlertDialog "Clear All" flow. This is a destructive operation that warrants verification of the correctmemoryClient.clearcall, subsequentload()reload, and error handling.🧪 Suggested test
it('calls clear and reloads on clearAll action', async () => { const { wrapper, memoryClient } = await setup() const clearButton = wrapper .findAll('button') .find((b) => b.text().includes('settings.deepchatAgents.memoryManager.clearAll'))! // AlertDialogAction is stubbed as ButtonStub, so clicking it triggers clearAll await clearButton.trigger('click') await flushPromises() expect(memoryClient.clear).toHaveBeenCalledWith('deepchat') expect(memoryClient.getHealth).toHaveBeenCalled() })🤖 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 `@test/renderer/components/MemoryDiagnosticsPanel.test.ts` around lines 87 - 122, Add test coverage for the destructive clearAll flow in MemoryDiagnosticsPanel.vue: the current setup() already mocks memoryClient.clear, but no test verifies the AlertDialog path. Add a test that locates the Clear All action, triggers the dialog action, and asserts memoryClient.clear is called with the agentId and that the panel reloads via getHealth/load afterward; also cover the error path if clear fails.
🤖 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 `@src/renderer/settings/components/MemoryDiagnosticsPanel.vue`:
- Around line 390-397: The clearAll flow in MemoryDiagnosticsPanel.vue should
match the loading/agent-snapshot handling used by load() and reindex(): capture
props.agentId at the start of clearAll, set loading while the clear is in
progress, and restore it in a finally block. Then call memoryClient.clear with
the captured agent id and invoke load() using that same captured id so a
mid-operation agent switch or refresh click cannot fetch the wrong agent’s data
or run concurrently.
In `@src/renderer/settings/components/MemoryListView.vue`:
- Around line 628-662: The MemoryListView handlers archive, restore, and
confirmRemove are only checking for thrown errors, but memoryClient methods can
also return false on failure. Update these functions to explicitly check the
boolean result from memoryClient.archive/restore/remove and call notifyFailed
when the result is false, not just in catch blocks. After a successful archive,
restore, or remove, refresh the list or update memories.value so the UI reflects
the change; use the existing archive, restore, confirmRemove, and deleteTarget
logic to locate the affected code.
In `@src/renderer/settings/components/MemorySettings.vue`:
- Around line 360-364: Reset the cached status when `selectedAgentId` changes so
counts don’t briefly show the previous agent’s data. In the
`watch(selectedAgentId, ...)` block, clear `status.value` before calling
`loadResolved()` and `loadStatus()`, so `activeMemoryCount`,
`archivedMemoryCount`, and the `MemoryInboxBar` props (`conflictCount`,
`personaDraftCount`) don’t render stale values during the switch. Keep the
existing single reload flow in place and apply the change in the
`selectedAgentId` watcher.
In `@src/renderer/src/i18n/zh-HK/settings.json`:
- Line 2917: The zh-HK translation for inboxTitle uses a simplified Chinese
character and should be updated to Traditional Chinese. In the zh-HK locale
settings JSON, fix the inboxTitle entry so it uses the traditional form “待處理”
instead of “待处理”, keeping the translation consistent with the locale.
In `@src/renderer/src/i18n/zh-TW/settings.json`:
- Line 2917: The zh-TW translation for inboxTitle uses a simplified Chinese
character and should be corrected to Traditional Chinese. Update the inboxTitle
entry in the zh-TW settings localization so it uses the traditional form “待處理”
instead of “待处理”, keeping the change localized to the i18n JSON entry with the
same key.
---
Nitpick comments:
In `@src/main/presenter/sqlitePresenter/tables/agentMemory.ts`:
- Around line 1387-1422: The conflict-pair validity logic is duplicated between
countConflictPairs() and ConflictService.listConflicts(), so they can drift out
of sync. Extract the shared predicate into a common helper or reusable SQL
fragment, then have countConflictPairs() and the list-conflict path both use it
so the challenger/target status checks stay identical.
In `@test/renderer/components/MemoryDiagnosticsPanel.test.ts`:
- Around line 87-122: Add test coverage for the destructive clearAll flow in
MemoryDiagnosticsPanel.vue: the current setup() already mocks
memoryClient.clear, but no test verifies the AlertDialog path. Add a test that
locates the Clear All action, triggers the dialog action, and asserts
memoryClient.clear is called with the agentId and that the panel reloads via
getHealth/load afterward; also cover the error path if clear fails.
In `@test/renderer/components/MemoryInlinePanel.test.ts`:
- Around line 145-360: Add coverage for the footer actions in MemoryInlinePanel
by testing the archive, restore, and delete flows. In the MemoryInlinePanel test
suite, locate the existing setup-based cases and add assertions around the
archive/restore/remove button handlers to verify the matching memoryClient
methods are called with the current memory id, the expected changed and close
events are emitted, and error paths show the toast behavior when those actions
fail. Use the existing setup, memoryClient, and wrapper.emitted patterns to keep
the new tests consistent with the other action-flow cases.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8b53bb3b-35e0-466b-a5d0-aa801cbd8b73
📒 Files selected for processing (55)
src/main/presenter/memoryPresenter/index.tssrc/main/presenter/memoryPresenter/services/managementService.tssrc/main/presenter/memoryPresenter/services/rowMutations.tssrc/main/presenter/memoryPresenter/types.tssrc/main/presenter/sqlitePresenter/tables/agentMemory.tssrc/main/routes/index.tssrc/renderer/api/MemoryClient.tssrc/renderer/settings/components/MemoryConfigInlinePanel.vuesrc/renderer/settings/components/MemoryConfigPanel.vuesrc/renderer/settings/components/MemoryDiagnosticsPanel.vuesrc/renderer/settings/components/MemoryEmptyState.vuesrc/renderer/settings/components/MemoryHealthSection.vuesrc/renderer/settings/components/MemoryInboxBar.vuesrc/renderer/settings/components/MemoryInlinePanel.vuesrc/renderer/settings/components/MemoryListView.vuesrc/renderer/settings/components/MemoryManagerDialog.vuesrc/renderer/settings/components/MemoryManagerPanel.vuesrc/renderer/settings/components/MemoryPersonaPanel.vuesrc/renderer/settings/components/MemorySettings.vuesrc/renderer/settings/components/memoryRedesignUtils.tssrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/de-DE/settings.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/es-ES/settings.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/id-ID/settings.jsonsrc/renderer/src/i18n/it-IT/settings.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/ms-MY/settings.jsonsrc/renderer/src/i18n/pl-PL/settings.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/tr-TR/settings.jsonsrc/renderer/src/i18n/vi-VN/settings.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/shared/contracts/events/memory.events.tssrc/shared/contracts/routes.tssrc/shared/contracts/routes/memory.routes.tstest/main/presenter/fakes/memoryFakes.tstest/main/presenter/memoryPresenter.test.tstest/main/presenter/memoryUpdate.test.tstest/main/routes/memoryDto.test.tstest/renderer/components/MemoryConfigInlinePanel.test.tstest/renderer/components/MemoryConfigPanel.test.tstest/renderer/components/MemoryDiagnosticsPanel.test.tstest/renderer/components/MemoryHealthSection.test.tstest/renderer/components/MemoryInlinePanel.test.tstest/renderer/components/MemoryListView.test.tstest/renderer/components/MemoryManagerDialog.test.tstest/renderer/components/MemorySettings.test.ts
💤 Files with no reviewable changes (7)
- src/renderer/settings/components/MemoryManagerDialog.vue
- test/renderer/components/MemoryConfigPanel.test.ts
- src/renderer/settings/components/MemoryManagerPanel.vue
- test/renderer/components/MemoryManagerDialog.test.ts
- src/renderer/settings/components/MemoryHealthSection.vue
- test/renderer/components/MemoryHealthSection.test.ts
- src/renderer/settings/components/MemoryConfigPanel.vue
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.
Summary by CodeRabbit
New Features
Bug Fixes