fix(chat): resolve issues 1841-1849 - #1866
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR bounds chat message rendering to a scroll-driven window, reworks chat search into ordered result-based matching, converts skill discovery and sync backup IO to async filesystem operations, adds adaptive visibility-aware remote status polling, introduces a dev-only mock long chat session generator (backend route, renderer UI, i18n), and updates related docs and tests. ChangesChat windowing and search
Async filesystem IO
Adaptive remote polling
Debug mock chat session
Documentation
Unrelated test drift
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ChatPage
participant MessageList
participant chatSearch
User->>ChatPage: scroll or type search query
ChatPage->>ChatPage: syncMessageViewportMetrics()
ChatPage->>ChatPage: compute messageWindowRange / visibleDisplayMessages
ChatPage->>MessageList: pass visibleDisplayMessages + spacer heights
ChatPage->>chatSearch: collectChatSearchResults(displayMessages, query)
chatSearch-->>ChatPage: chatSearchResults
ChatPage->>chatSearch: setActiveChatSearchResult(target)
chatSearch-->>MessageList: activate/scroll matching DOM row
sequenceDiagram
participant Renderer as AboutUsSettings
participant DebugClient
participant MainRoute as RouteDispatcher
participant DB as SQLite
Renderer->>DebugClient: createMockChatSession()
DebugClient->>MainRoute: invoke debugCreateMockChatSessionRoute
alt not dev or packaged
MainRoute-->>DebugClient: created:false
else dev build
MainRoute->>DB: createDebugMockChatSession(db)
DB-->>MainRoute: sessionId, title, messageCount
MainRoute->>MainRoute: publish sessionsUpdatedEvent
MainRoute-->>DebugClient: created:true, sessionId, title, messageCount
end
DebugClient-->>Renderer: result
Renderer->>Renderer: show success/error toast
Possibly related issues
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
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (5)
src/renderer/src/pages/ChatPage.vue (1)
1222-1262: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winLinear scan for window bounds on every viewport-metric sync.
entries.findIndex(...)plus the trailingwhileloop are O(n) over the full message list, and this computed re-runs any timescrollViewportTop/scrollViewportHeightchange — i.e., on essentially every scroll frame once pastMESSAGE_WINDOWING_THRESHOLD. Sinceentriesare ordered bytop/bottom, a binary search would make this the same computation in O(log n), which is more in line with the "avoid O(n) rendering overhead" goal of issue#1841for very long sessions.♻️ Sketch of binary-search start lookup
- let start = entries.findIndex((entry) => entry.bottom >= windowTop) - if (start === -1) start = Math.max(0, total - MESSAGE_INITIAL_WINDOW_COUNT) + let start = lowerBoundByBottom(entries, windowTop) + if (start >= total) start = Math.max(0, total - MESSAGE_INITIAL_WINDOW_COUNT)
lowerBoundByBottomwould binary-searchentriesfor the first index whosebottom >= windowTop; the trailingendscan can similarly binary-search ontop <= windowBottominstead of incrementing one-by-one.🤖 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/renderer/src/pages/ChatPage.vue` around lines 1222 - 1262, The messageWindowRange computed in ChatPage.vue does a linear scan with entries.findIndex and a trailing while loop on every scroll update, which makes window bound calculation O(n) for large message lists. Replace the start/end lookup with binary-search helpers over the ordered entries array (for example in the message windowing logic around messageWindowRange), using bottom to find the first visible entry and top to find the last, while preserving the existing fallback behavior for empty/short lists and missing viewport height.test/main/shared/settingsNavigation.test.ts (1)
51-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant test case using invalid platform value.
'windows'isn't a real platform identifier (Node/Electron use'win32'for Windows), so this case duplicates the'win32'/'x64'assertion above (Line 47-50) without adding real coverage — it just falls into whatever default path unrecognized platforms take.🤖 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/main/shared/settingsNavigation.test.ts` around lines 51 - 55, The test in getSettingsNavigationItems is using an invalid platform value, so it does not add meaningful coverage and duplicates the existing Windows assertion. Replace the unsupported 'windows' case with a real platform identifier or remove the duplicate check, and keep the coverage focused on the platform/arch combinations handled by getSettingsNavigationItems.test/renderer/components/AboutUsSettings.test.ts (1)
407-445: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the new test to cover the success/pending UI, not just the API call.
The test only checks that
debugClientMock.createMockChatSessionwas invoked; it doesn't assert that the success toast is shown with the returned title/count, or that the button reflects theisCreatingMockChatpending state. Using a controllable/deferred mock resolution would let you assert the disabled/creating label mid-flight, closing a coverage gap for the new success path.🤖 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/AboutUsSettings.test.ts` around lines 407 - 445, The new AboutUsSettings test only verifies createMockChatSession was called, so extend it to cover the pending and success UI states as well. Use the AboutUsSettings.vue flow and debugClientMock.createMockChatSession with a deferred/controllable promise so you can assert the “isCreatingMockChat” button state changes while the request is in flight, then resolve it and verify the success toast uses the returned title/count. Keep the checks centered on the existing mock chat button and the creation action path in AboutUsSettings.src/main/routes/debug/createMockChatSession.ts (1)
188-219: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilent error swallowing hides sample/model read failures.
Both
readMessageSamplesandreadModelSamplecatch all errors and return empty defaults without any logging. Since this is a dev-only diagnostic tool, a failure here (e.g. schema mismatch) would go unnoticed, defeating the purpose of surfacing issues during manual testing.🔧 Suggested logging on failure
} catch { - return { user: [], assistant: [] } + console.warn('[DebugMockChatSession] Failed to read message samples') + return { user: [], assistant: [] } }🤖 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/routes/debug/createMockChatSession.ts` around lines 188 - 219, Both readMessageSamples and readModelSample are swallowing all database read errors and returning empty defaults, so add logging in each catch block to surface failures during debugging. Use the existing DebugMockChatDatabase access paths in readMessageSamples and readModelSample, and log the caught error with enough context before returning the fallback user/assistant arrays or empty model object. Keep the fallback behavior, but make the failure visible so schema or query issues don’t fail silently.src/main/routes/index.ts (1)
3975-3978: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded event name instead of typed event constant.
publishDeepchatEvent('sessions.updated', ...)uses a raw string literal, while other publishers in this same file use typed constants (e.g.publishProjectEnvironmentsChangedusesprojectEnvironmentsChangedEvent.name). Not a functional issue since the literal presumably matches theDeepchatEventNameunion, but inconsistent with the established convention.🤖 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/routes/index.ts` around lines 3975 - 3978, The event publish in the sessions update flow uses a hardcoded string literal instead of the typed event constant used elsewhere in this file. Update the `publishDeepchatEvent` call in the sessions creation/update path to use the corresponding `DeepchatEventName` constant (matching the pattern used by `publishProjectEnvironmentsChanged`) so the event name is centralized and consistent.
🤖 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/main/presenter/syncPresenter/index.ts`:
- Line 576: `performBackup()` in `syncPresenter/index.ts` is yielding on
`fs.promises.readFile(this.DB_PATH)`, which can let writes interleave after
`wal_checkpoint(TRUNCATE)` and before the ZIP copy. Update the backup read in
`performBackup()` to use a synchronous SQLite file read path such as
`fs.readFileSync`, or switch the backup flow to SQLite’s online backup/`VACUUM
INTO` approach so `agent.db` is copied atomically.
In `@src/main/routes/debug/createMockChatSession.ts`:
- Around line 132-137: The hidden side effect is in createDebugMockChatSession
where repairLegacyDebugAgent.run is executed as part of mock-session creation.
Move this legacy agent rewrite into a clearly named one-time migration/cleanup
helper, or add an explicit comment near repairLegacyDebugAgent and the
db.transaction block explaining why the update belongs here. Keep the
session-creation path focused on the new mock session and make the unrelated
mutation obvious by using the existing createDebugMockChatSession and
repairLegacyDebugAgent symbols.
In `@src/renderer/src/components/WindowSideBar.vue`:
- Around line 690-691: The remote control status polling in WindowSideBar.vue
can restart after unmount because runRemoteControlStatusRefresh() may schedule a
new timeout after refreshRemoteControlStatus() finishes even when cleanup has
already run. Add an unmounted/active guard around the scheduling path in
runRemoteControlStatusRefresh() and any timer setup/clear logic so no new
timeout is created once the component is torn down, and make the cleanup path
fully stop future reschedules using the existing remoteControlStatusTimer and
remoteControlStatusErrors state.
In `@src/renderer/src/i18n/da-DK/about.json`:
- Around line 17-22: The new mockChat* entries in the Danish locale are still in
English, so translate these strings to Danish to match the rest of about.json.
Update the values for mockChatButton, mockChatCreating, mockChatCreated,
mockChatCreatedDesc, mockChatCreateFailed, and mockChatCreateUnavailable in the
da-DK translation file, keeping the same placeholders and tone as nearby entries
like mockOnboardingButton.
In `@src/renderer/src/i18n/de-DE/about.json`:
- Around line 12-17: The mockChat* strings in about.json are still untranslated
English in the de-DE locale, unlike nearby localized keys such as
mockOnboardingButton. Update the mockChatButton, mockChatCreating,
mockChatCreated, mockChatCreatedDesc, mockChatCreateFailed, and
mockChatCreateUnavailable entries in the German translation file to proper
German text while preserving the existing message placeholders like {title} and
{count}.
In `@src/renderer/src/i18n/es-ES/about.json`:
- Around line 12-17: The new mockChat* entries in the es-ES about.json locale
are still in English, so translate them to Spanish to match the surrounding
localized strings. Update the strings for mockChatButton, mockChatCreating,
mockChatCreated, mockChatCreatedDesc, mockChatCreateFailed, and
mockChatCreateUnavailable so they are consistent with the existing Spanish
translations in this file.
In `@src/renderer/src/i18n/fa-IR/about.json`:
- Around line 16-21: The mockChat* entries in the fa-IR locale are still in
English, unlike the neighboring translated strings. Update the values in
about.json for mockChatButton, mockChatCreating, mockChatCreated,
mockChatCreatedDesc, mockChatCreateFailed, and mockChatCreateUnavailable to
proper Persian translations, keeping the same placeholders and meaning used by
the other i18n keys in this file.
In `@src/renderer/src/i18n/fr-FR/about.json`:
- Around line 16-21: The new mockChat* entries in the French locale file are
still in English; translate the strings in about.json to French while keeping
the existing keys and placeholder tokens unchanged. Update the values for
mockChatButton, mockChatCreating, mockChatCreated, mockChatCreatedDesc,
mockChatCreateFailed, and mockChatCreateUnavailable so they match the rest of
the fr-FR localization.
In `@src/renderer/src/i18n/he-IL/about.json`:
- Around line 16-21: The mockChat* entries in the Hebrew locale file are still
untranslated English strings, unlike the surrounding localized keys. Update the
values for mockChatButton, mockChatCreating, mockChatCreated,
mockChatCreatedDesc, mockChatCreateFailed, and mockChatCreateUnavailable in the
about.json locale object to proper Hebrew text, keeping the same placeholders
and message meaning consistent with the neighboring translations such as
mockOnboardingButton.
In `@src/renderer/src/i18n/id-ID/about.json`:
- Around line 12-17: The Indonesian locale entries in about.json are still in
English, so translate the mock chat strings to proper id-ID text. Update the
values for mockChatButton, mockChatCreating, mockChatCreated,
mockChatCreatedDesc, mockChatCreateFailed, and mockChatCreateUnavailable in this
locale file to match the translated style used by other locale files such as
zh-HK.
In `@src/renderer/src/i18n/it-IT/about.json`:
- Around line 12-17: The new mockChat strings in about.json are still in
English, so update the Italian locale entries to proper Italian translations.
Keep the same keys in the it-IT/about.json resource and replace each mockChat*
value with localized text, matching the existing wording and tone used by the
rest of the Italian translations.
In `@src/renderer/src/i18n/ja-JP/about.json`:
- Around line 8-13: The new mockChat* entries in the ja-JP about.json locale are
still in English; translate each of the affected keys (mockChatButton,
mockChatCreating, mockChatCreated, mockChatCreatedDesc, mockChatCreateFailed,
and mockChatCreateUnavailable) into natural Japanese while keeping the same
placeholders and meaning. Update the Japanese locale values in the about.json
resource so they match the rest of the ja-JP translations.
In `@src/renderer/src/i18n/ko-KR/about.json`:
- Around line 8-13: The new mockChat* entries in the ko-KR locale are still in
English and should be translated to Korean to match the surrounding localized
keys in about.json. Update the values for mockChatButton, mockChatCreating,
mockChatCreated, mockChatCreatedDesc, mockChatCreateFailed, and
mockChatCreateUnavailable so they use Korean wording consistent with the
existing locale, keeping the same placeholders and message intent.
In `@src/renderer/src/i18n/ms-MY/about.json`:
- Around line 12-17: The new mockChat* entries in the ms-MY about.json locale
are still in English, so translate each of the added keys to Malay to match the
rest of the locale file. Update the strings for mockChatButton,
mockChatCreating, mockChatCreated, mockChatCreatedDesc, mockChatCreateFailed,
and mockChatCreateUnavailable in the ms-MY translation resource.
In `@src/renderer/src/i18n/pl-PL/about.json`:
- Around line 20-25: The new mockChat* entries in the pl-PL locale file are
still in English; translate each of the six strings to Polish so the about.json
locale matches the rest of the Polish translations. Update the values for
mockChatButton, mockChatCreating, mockChatCreated, mockChatCreatedDesc,
mockChatCreateFailed, and mockChatCreateUnavailable in the pl-PL locale object.
In `@src/renderer/src/i18n/pt-BR/about.json`:
- Around line 16-21: The new mockChat* entries in the pt-BR locale file are
still in English, so translate each string to Portuguese and keep the wording
consistent with the existing about.json entries. Update the mockChatButton,
mockChatCreating, mockChatCreated, mockChatCreatedDesc, mockChatCreateFailed,
and mockChatCreateUnavailable values in the pt-BR translation file so they match
the locale language.
In `@src/renderer/src/i18n/ru-RU/about.json`:
- Around line 8-13: The new mockChat* entries in the ru-RU about.json locale are
still in English. Update the localized strings for mockChatButton,
mockChatCreating, mockChatCreated, mockChatCreatedDesc, mockChatCreateFailed,
and mockChatCreateUnavailable in the Russian translation file so they match the
rest of the ru-RU copy.
In `@src/renderer/src/i18n/tr-TR/about.json`:
- Around line 12-17: The new mockChat* entries in the tr-TR about.json locale
are still in English; update those keys with proper Turkish translations to
match the rest of the locale file. Keep the same identifiers (mockChatButton,
mockChatCreating, mockChatCreated, mockChatCreatedDesc, mockChatCreateFailed,
mockChatCreateUnavailable) and replace only the string values with localized
Turkish text.
In `@src/renderer/src/i18n/vi-VN/about.json`:
- Around line 20-25: The new mockChat* entries in the vi-VN locale are still in
English; translate these strings in the about.json Vietnamese locale so they
match the rest of the file. Update the values for mockChatButton,
mockChatCreating, mockChatCreated, mockChatCreatedDesc, mockChatCreateFailed,
and mockChatCreateUnavailable, keeping the same keys and placeholder tokens used
by the i18n messages.
In `@src/renderer/src/lib/chatSearch.ts`:
- Around line 178-193: The chat-search text collector is missing rendered
tool-call content, so search hits can be undercounted. Update collectUnknownText
in chatSearch.ts to also traverse the same tool-call payload fields that
MessageBlockToolCall.vue renders, especially tool_call.params (and any closely
related nested fields), so setActiveChatSearchResult sees the full message text
and selects the correct match.
---
Nitpick comments:
In `@src/main/routes/debug/createMockChatSession.ts`:
- Around line 188-219: Both readMessageSamples and readModelSample are
swallowing all database read errors and returning empty defaults, so add logging
in each catch block to surface failures during debugging. Use the existing
DebugMockChatDatabase access paths in readMessageSamples and readModelSample,
and log the caught error with enough context before returning the fallback
user/assistant arrays or empty model object. Keep the fallback behavior, but
make the failure visible so schema or query issues don’t fail silently.
In `@src/main/routes/index.ts`:
- Around line 3975-3978: The event publish in the sessions update flow uses a
hardcoded string literal instead of the typed event constant used elsewhere in
this file. Update the `publishDeepchatEvent` call in the sessions
creation/update path to use the corresponding `DeepchatEventName` constant
(matching the pattern used by `publishProjectEnvironmentsChanged`) so the event
name is centralized and consistent.
In `@src/renderer/src/pages/ChatPage.vue`:
- Around line 1222-1262: The messageWindowRange computed in ChatPage.vue does a
linear scan with entries.findIndex and a trailing while loop on every scroll
update, which makes window bound calculation O(n) for large message lists.
Replace the start/end lookup with binary-search helpers over the ordered entries
array (for example in the message windowing logic around messageWindowRange),
using bottom to find the first visible entry and top to find the last, while
preserving the existing fallback behavior for empty/short lists and missing
viewport height.
In `@test/main/shared/settingsNavigation.test.ts`:
- Around line 51-55: The test in getSettingsNavigationItems is using an invalid
platform value, so it does not add meaningful coverage and duplicates the
existing Windows assertion. Replace the unsupported 'windows' case with a real
platform identifier or remove the duplicate check, and keep the coverage focused
on the platform/arch combinations handled by getSettingsNavigationItems.
In `@test/renderer/components/AboutUsSettings.test.ts`:
- Around line 407-445: The new AboutUsSettings test only verifies
createMockChatSession was called, so extend it to cover the pending and success
UI states as well. Use the AboutUsSettings.vue flow and
debugClientMock.createMockChatSession with a deferred/controllable promise so
you can assert the “isCreatingMockChat” button state changes while the request
is in flight, then resolve it and verify the success toast uses the returned
title/count. Keep the checks centered on the existing mock chat button and the
creation action path in AboutUsSettings.
🪄 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: 0323b796-f811-42a0-bd33-49669e8b78df
📒 Files selected for processing (55)
docs/architecture/agent-runtime-presenter-split/spec.mddocs/architecture/agent-runtime-presenter-split/state-map.mddocs/architecture/agent-runtime-presenter-split/tasks.mddocs/features/mock-long-chat-debug-data/spec.mddocs/issues/harness-reliability-1841-1849/plan.mddocs/issues/harness-reliability-1841-1849/spec.mddocs/issues/harness-reliability-1841-1849/tasks.mddocs/issues/session-restore-scroll-intent/plan.mddocs/issues/session-restore-scroll-intent/spec.mddocs/issues/session-restore-scroll-intent/tasks.mdsrc/main/presenter/skillPresenter/index.tssrc/main/presenter/syncPresenter/index.tssrc/main/routes/debug/createMockChatSession.tssrc/main/routes/index.tssrc/renderer/api/DebugClient.tssrc/renderer/api/index.tssrc/renderer/settings/components/AboutUsSettings.vuesrc/renderer/settings/components/RemoteSettings.vuesrc/renderer/src/components/WindowSideBar.vuesrc/renderer/src/components/chat/MessageList.vuesrc/renderer/src/i18n/da-DK/about.jsonsrc/renderer/src/i18n/de-DE/about.jsonsrc/renderer/src/i18n/en-US/about.jsonsrc/renderer/src/i18n/es-ES/about.jsonsrc/renderer/src/i18n/fa-IR/about.jsonsrc/renderer/src/i18n/fr-FR/about.jsonsrc/renderer/src/i18n/he-IL/about.jsonsrc/renderer/src/i18n/id-ID/about.jsonsrc/renderer/src/i18n/it-IT/about.jsonsrc/renderer/src/i18n/ja-JP/about.jsonsrc/renderer/src/i18n/ko-KR/about.jsonsrc/renderer/src/i18n/ms-MY/about.jsonsrc/renderer/src/i18n/pl-PL/about.jsonsrc/renderer/src/i18n/pt-BR/about.jsonsrc/renderer/src/i18n/ru-RU/about.jsonsrc/renderer/src/i18n/tr-TR/about.jsonsrc/renderer/src/i18n/vi-VN/about.jsonsrc/renderer/src/i18n/zh-CN/about.jsonsrc/renderer/src/i18n/zh-HK/about.jsonsrc/renderer/src/i18n/zh-TW/about.jsonsrc/renderer/src/lib/chatSearch.tssrc/renderer/src/pages/ChatPage.vuesrc/shared/contracts/routes.tssrc/shared/contracts/routes/debug.routes.tstest/fixtures/mockMessages.tstest/main/presenter/deeplinkPresenter.test.tstest/main/routes/contracts.test.tstest/main/routes/debugMockChatSession.test.tstest/main/shared/settingsNavigation.test.tstest/renderer/components/AboutUsSettings.test.tstest/renderer/components/ChatPage.test.tstest/renderer/components/ModelProviderSettings.test.tstest/renderer/lib/chatSearch.test.tstest/renderer/message/performanceEvaluation.test.tstest/renderer/performance/chatRendering.perf.test.ts
💤 Files with no reviewable changes (1)
- test/renderer/message/performanceEvaluation.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/main/routes/debugMockChatSession.test.ts (1)
1-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest path doesn't mirror source structure.
Source lives at
src/main/routes/debug/createMockChatSession.ts, but this test sits directly undertest/main/routes/rather thantest/main/routes/debug/, missing thedebugsubdirectory mirror.As per coding guidelines, "Vitest test suites should mirror source structure under
test/main/**andtest/renderer/**with setup files."🤖 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/main/routes/debugMockChatSession.test.ts` around lines 1 - 6, The test suite location does not mirror the source structure for createDebugMockChatSession. Move or rename the vitest suite so it lives under the matching debug subdirectory to reflect src/main/routes/debug/createMockChatSession, and keep the existing imports for createDebugMockChatSession and DebugMockChatDatabase aligned with the relocated test file.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@test/main/routes/debugMockChatSession.test.ts`:
- Around line 1-6: The test suite location does not mirror the source structure
for createDebugMockChatSession. Move or rename the vitest suite so it lives
under the matching debug subdirectory to reflect
src/main/routes/debug/createMockChatSession, and keep the existing imports for
createDebugMockChatSession and DebugMockChatDatabase aligned with the relocated
test file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 97a84440-0c40-47ce-8605-5e99909da5c6
📒 Files selected for processing (2)
src/main/routes/debug/createMockChatSession.tstest/main/routes/debugMockChatSession.test.ts
💤 Files with no reviewable changes (1)
- src/main/routes/debug/createMockChatSession.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/main/routes/debug/createMockChatSession.test.ts (1)
33-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFake DB silently returns empty results for unmatched SQL.
prepare()dispatches via substring matching, and unrecognized SQL falls through tothis.statement()(line 107) with emptyall/get/run. If the real queries increateMockChatSession.ts(e.g., theSELECT role, contentorSELECT provider_id, model_idtext) ever change, this fake would silently start returning empty samples instead of failing the test, masking drift between the fake and the real implementation.Consider throwing on unmatched SQL so any drift fails loudly instead of silently degrading sample data.
♻️ Proposed fix
- return this.statement() + throw new Error(`FakeDebugDb: unrecognized SQL statement: ${sql}`) }🤖 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/main/routes/debug/createMockChatSession.test.ts` around lines 33 - 108, The Fake DB in prepare() silently falls back to an empty statement for unmatched SQL, which can hide drift between this test double and the real queries. Update the prepare() method in the mock so any SQL that does not match the known SELECT/INSERT cases throws an error instead of returning this.statement(), using the existing prepare dispatcher and statement helper to locate the change.
🤖 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.
Nitpick comments:
In `@test/main/routes/debug/createMockChatSession.test.ts`:
- Around line 33-108: The Fake DB in prepare() silently falls back to an empty
statement for unmatched SQL, which can hide drift between this test double and
the real queries. Update the prepare() method in the mock so any SQL that does
not match the known SELECT/INSERT cases throws an error instead of returning
this.statement(), using the existing prepare dispatcher and statement helper to
locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6af047cd-0210-4180-9f3f-fcae7b826e83
📒 Files selected for processing (26)
src/main/presenter/syncPresenter/index.tssrc/main/routes/debug/createMockChatSession.tssrc/main/routes/index.tssrc/renderer/src/components/WindowSideBar.vuesrc/renderer/src/i18n/da-DK/about.jsonsrc/renderer/src/i18n/de-DE/about.jsonsrc/renderer/src/i18n/es-ES/about.jsonsrc/renderer/src/i18n/fa-IR/about.jsonsrc/renderer/src/i18n/fr-FR/about.jsonsrc/renderer/src/i18n/he-IL/about.jsonsrc/renderer/src/i18n/id-ID/about.jsonsrc/renderer/src/i18n/it-IT/about.jsonsrc/renderer/src/i18n/ja-JP/about.jsonsrc/renderer/src/i18n/ko-KR/about.jsonsrc/renderer/src/i18n/ms-MY/about.jsonsrc/renderer/src/i18n/pl-PL/about.jsonsrc/renderer/src/i18n/pt-BR/about.jsonsrc/renderer/src/i18n/ru-RU/about.jsonsrc/renderer/src/i18n/tr-TR/about.jsonsrc/renderer/src/i18n/vi-VN/about.jsonsrc/renderer/src/lib/chatSearch.tssrc/renderer/src/pages/ChatPage.vuetest/main/routes/debug/createMockChatSession.test.tstest/main/shared/settingsNavigation.test.tstest/renderer/components/AboutUsSettings.test.tstest/renderer/lib/chatSearch.test.ts
💤 Files with no reviewable changes (1)
- test/main/shared/settingsNavigation.test.ts
✅ Files skipped from review due to trivial changes (13)
- src/renderer/src/i18n/ms-MY/about.json
- src/renderer/src/i18n/vi-VN/about.json
- src/renderer/src/i18n/he-IL/about.json
- src/renderer/src/i18n/it-IT/about.json
- src/renderer/src/i18n/ru-RU/about.json
- src/renderer/src/i18n/ko-KR/about.json
- src/renderer/src/i18n/tr-TR/about.json
- src/renderer/src/i18n/id-ID/about.json
- src/renderer/src/i18n/de-DE/about.json
- src/renderer/src/i18n/fr-FR/about.json
- src/renderer/src/i18n/es-ES/about.json
- src/renderer/src/i18n/pt-BR/about.json
- src/renderer/src/i18n/ja-JP/about.json
🚧 Files skipped from review as they are similar to previous changes (11)
- src/renderer/src/i18n/fa-IR/about.json
- src/renderer/src/i18n/da-DK/about.json
- src/renderer/src/i18n/pl-PL/about.json
- src/main/routes/index.ts
- test/renderer/lib/chatSearch.test.ts
- src/renderer/src/lib/chatSearch.ts
- test/renderer/components/AboutUsSettings.test.ts
- src/renderer/src/components/WindowSideBar.vue
- src/main/presenter/syncPresenter/index.ts
- src/main/routes/debug/createMockChatSession.ts
- src/renderer/src/pages/ChatPage.vue
This was referenced Jul 9, 2026
This was referenced Jul 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #1841, #1842, #1843, #1844, #1845, #1846, #1847, #1848, #1849
Validation
Notes
Summary by CodeRabbit
New Features
Bug Fixes
Tests