refactor(renderer): split scopes and fix review-confirmed races#2000
Open
zhangmo8 wants to merge 11 commits into
Open
refactor(renderer): split scopes and fix review-confirmed races#2000zhangmo8 wants to merge 11 commits into
zhangmo8 wants to merge 11 commits into
Conversation
Restructure renderer scope boundaries per docs/architecture/renderer-scope-optimization: - Move ChatTabView into apps/chat-main; drop unused views/SettingsTabView - Extract foundation/appearance/documentAppearance shared by all renderer entries - Session store: paginated list with cursor, tombstoned deletes, targeted activate/deactivate IPC; refetch when a pending first page is invalidated - Chat search: canonical debounced query, incremental per-row highlighting with a MutationObserver; drain pending observer work on same-query re-apply and count only rendered user-message content - Message window fast path returns a fresh array so totalHeight and the window range react to tail growth; batch measures no longer force per-row full layout rebuilds - Scroll state machine rejects a late session-restore scroll after the user explicitly navigated (search/spotlight) - Split row-level isStreamingMessage (activity grouping) from thread-level isInGeneratingThread (fork gating, variant auto-switch) - Always clear plan snapshots for deleted messages even after switching views
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRenderer changes add race-safe session, project, deeplink, composer, search, streaming, appearance, and route-host behavior, alongside Markstream integration, metadata refreshes, architecture documentation, and regression tests. ChangesRenderer optimization and reliability
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
Sidebar search is a pure client-side filter, so pagination must keep auto-filling while a query is active or matches are limited to already loaded pages. Collapsed-group guards still apply when not searching.
Preserve targeted session updates across stale page loads and fence environment reorder rollbacks. Align search indexing with activatable DOM content and refresh the renderer architecture baseline.
zhangmo8
marked this pull request as ready for review
July 20, 2026 07:49
![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: 1
🧹 Nitpick comments (5)
test/renderer/features/chat-page/composables/useComposerSubmit.test.ts (1)
5-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a shared deferred-promise test helper.
Both new test files define their own local "deferred" utility with the same shape (a promise plus externally-controlled
resolve); this mirrors a pattern that already exists inuseToolInteraction.test.ts. Consolidating into one shared helper (e.g.test/renderer/utils/deferred.ts) avoids drift between the two implementations (only one currently exposesreject) and reduces duplication introduced in this PR.
test/renderer/features/chat-page/composables/useComposerSubmit.test.ts#L5-L11: replace localcreateDeferredwith the shared helper (which should includerejectsupport for parity).test/renderer/features/chat-page/composables/useMessageActions.test.ts#L5-L14: replace localdeferredwith the same shared helper.🤖 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/features/chat-page/composables/useComposerSubmit.test.ts` around lines 5 - 11, Extract the duplicated deferred-promise utilities into a shared helper at test/renderer/utils/deferred.ts, including externally controlled resolve and reject functions. In test/renderer/features/chat-page/composables/useComposerSubmit.test.ts:5-11, replace createDeferred with the shared helper; in test/renderer/features/chat-page/composables/useMessageActions.test.ts:5-14, replace deferred with the same helper and update usages as needed.src/renderer/src/stores/ui/session.ts (2)
250-274: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
isNewerSessionUpdatename is inverted from its actual semantics.The function returns
truewhen the update is not newer (i.e., should be skipped), but its name reads as "is this update newer".mergeSessionsrelies on this inverted polarity (if (existing && isNewerSessionUpdate(existing, update)) continue). Given this logic guards cross-window causal ordering, a future edit that "fixes" the name without updating the call site (or vice versa) could silently invert the merge behavior.♻️ Suggested rename for clarity
-function isNewerSessionUpdate(existing: UISession, update: UISession): boolean { +function isStaleOrSameSessionUpdate(existing: UISession, update: UISession): boolean { const existingUpdatedAt = existing.updatedAt const updateUpdatedAt = update.updatedAt // ... return ( Number.isFinite(existingUpdatedAt) && Number.isFinite(updateUpdatedAt) && updateUpdatedAt <= existingUpdatedAt ) } function mergeSessions(current: UISession[], updates: UISession[]): UISession[] { const next = new Map(current.map((session) => [session.id, session])) for (const update of updates) { const existing = next.get(update.id) - if (existing && isNewerSessionUpdate(existing, update)) { + if (existing && isStaleOrSameSessionUpdate(existing, update)) { continue }🤖 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/stores/ui/session.ts` around lines 250 - 274, Rename isNewerSessionUpdate to a predicate that clearly indicates the update is stale or should be skipped, and update the corresponding guard in mergeSessions to use the renamed symbol without changing its existing behavior: equal or older finite timestamps must continue to be ignored.
394-452: 🚀 Performance & Scalability | 🔵 TrivialTombstone/revision maps grow unbounded for the store's lifetime.
removedSessionIds,sessionByIdRefreshRevisions, andtargetedSessionCommitRevisionsentries are added on every delete/refresh/targeted-commit but never pruned (onlyobservedSessionStatuses.delete(sessionId)is cleaned up inremoveSessions). The in-code comment explicitly accepts this as a lifetime tradeoff for correctness, but in a long-running renderer session with many created/deleted sessions over days, these collections will grow indefinitely.Worth confirming this is an accepted tradeoff for the renderer's expected session lifetime, or considering a bounded eviction (e.g., cap size / evict oldest tombstones once no in-flight request could plausibly reference them).
🤖 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/stores/ui/session.ts` around lines 394 - 452, Bound the lifetime of removedSessionIds, sessionByIdRefreshRevisions, and targetedSessionCommitRevisions in the session store. Add pruning that evicts the oldest entries once no in-flight list, per-ID refresh, or targeted-commit operation can still reference them, while preserving tombstone and revision checks for active requests and keeping removeSessions cleanup correct.test/renderer/composables/useMessageVirtualization.test.ts (1)
164-185: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
layoutSegments.valueresolving tonullat runtime.Test 3 only covers the "option omitted" fallback.
useDisplayMessages.ts'slayoutSegmentscomputed can also emitnullat runtime while the option is present (inline-stream reinsertion case) — that specific transition isn't exercised here, even though it's the most fragile new fallback path introduced by this PR.🤖 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/composables/useMessageVirtualization.test.ts` around lines 164 - 185, Extend the useMessageVirtualization test suite to cover a provided layoutSegments option whose computed value resolves to null at runtime, mirroring the inline-stream reinsertion transition. Assert that visibleDisplayMessages.value falls back to the complete displayMessages list, while preserving the existing omitted-option fallback test.test/renderer/components/SettingsApp.test.ts (1)
1483-1489: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReset
document.documentElement.lang/dirafter this test.The test mutates
document.documentElement.lang/dirbut doesn't restore them, unlike the siblingdocumentAppearance.test.tswhich resets these inafterEach. If any later test in this file relies on defaultlang/dir, this could leak state across tests.Suggested cleanup
expect(configClientMock.getLanguage).not.toHaveBeenCalled() wrapper.unmount() + document.documentElement.lang = '' + document.documentElement.dir = '' })🤖 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/SettingsApp.test.ts` around lines 1483 - 1489, Restore document.documentElement.lang and dir to their default values after the test covering the fa-IR locale, matching the cleanup pattern used by sibling tests. Add the cleanup alongside the existing wrapper.unmount() flow without changing the assertions.
🤖 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 `@test/renderer/components/ChatInputToolbar.test.ts`:
- Around line 131-146: Update the “renders progress and blocks repeated steer
clicks while pending” test for ChatInputToolbar to trigger a click on the steer
button and assert that no steer event is emitted, while preserving its existing
disabled, aria-busy, status, and icon assertions.
---
Nitpick comments:
In `@src/renderer/src/stores/ui/session.ts`:
- Around line 250-274: Rename isNewerSessionUpdate to a predicate that clearly
indicates the update is stale or should be skipped, and update the corresponding
guard in mergeSessions to use the renamed symbol without changing its existing
behavior: equal or older finite timestamps must continue to be ignored.
- Around line 394-452: Bound the lifetime of removedSessionIds,
sessionByIdRefreshRevisions, and targetedSessionCommitRevisions in the session
store. Add pruning that evicts the oldest entries once no in-flight list, per-ID
refresh, or targeted-commit operation can still reference them, while preserving
tombstone and revision checks for active requests and keeping removeSessions
cleanup correct.
In `@test/renderer/components/SettingsApp.test.ts`:
- Around line 1483-1489: Restore document.documentElement.lang and dir to their
default values after the test covering the fa-IR locale, matching the cleanup
pattern used by sibling tests. Add the cleanup alongside the existing
wrapper.unmount() flow without changing the assertions.
In `@test/renderer/composables/useMessageVirtualization.test.ts`:
- Around line 164-185: Extend the useMessageVirtualization test suite to cover a
provided layoutSegments option whose computed value resolves to null at runtime,
mirroring the inline-stream reinsertion transition. Assert that
visibleDisplayMessages.value falls back to the complete displayMessages list,
while preserving the existing omitted-option fallback test.
In `@test/renderer/features/chat-page/composables/useComposerSubmit.test.ts`:
- Around line 5-11: Extract the duplicated deferred-promise utilities into a
shared helper at test/renderer/utils/deferred.ts, including externally
controlled resolve and reject functions. In
test/renderer/features/chat-page/composables/useComposerSubmit.test.ts:5-11,
replace createDeferred with the shared helper; in
test/renderer/features/chat-page/composables/useMessageActions.test.ts:5-14,
replace deferred with the same helper and update usages as needed.
🪄 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: c616963f-ab08-4b28-96e9-c59a5b09a9f8
📒 Files selected for processing (59)
docs/architecture/baselines/renderer-application-boundaries-baseline.jsondocs/architecture/renderer-scope-optimization/plan.mddocs/architecture/renderer-scope-optimization/spec.mddocs/architecture/renderer-scope-optimization/tasks.mddocs/issues/chat-interrupt-actions-stall/spec.mdsrc/renderer/floating/main.tssrc/renderer/settings/App.vuesrc/renderer/src/apps/chat-main/ChatMainApp.vuesrc/renderer/src/apps/chat-main/ChatTabView.vuesrc/renderer/src/components/WindowSideBar.vuesrc/renderer/src/components/chat/ChatInputToolbar.vuesrc/renderer/src/components/chat/MessageList.vuesrc/renderer/src/components/chat/MessageListRow.vuesrc/renderer/src/components/message/MessageItemAssistant.vuesrc/renderer/src/composables/chat/chatScrollState.tssrc/renderer/src/composables/message/useMessageWindow.tssrc/renderer/src/features/chat-page/ChatPage.vuesrc/renderer/src/features/chat-page/composables/useChatSearch.tssrc/renderer/src/features/chat-page/composables/useComposerSubmit.tssrc/renderer/src/features/chat-page/composables/useDisplayMessages.tssrc/renderer/src/features/chat-page/composables/useMessageActions.tssrc/renderer/src/features/chat-page/composables/useMessageVirtualization.tssrc/renderer/src/features/chat-page/composables/useToolInteraction.tssrc/renderer/src/foundation/appearance/documentAppearance.tssrc/renderer/src/lib/chatSearch.tssrc/renderer/src/pages/NewThreadPage.vuesrc/renderer/src/router/index.tssrc/renderer/src/stores/language.tssrc/renderer/src/stores/theme.tssrc/renderer/src/stores/ui/draft.tssrc/renderer/src/stores/ui/message.tssrc/renderer/src/stores/ui/project.tssrc/renderer/src/stores/ui/session.tssrc/renderer/src/stores/ui/sessionIpc.tssrc/renderer/src/views/SettingsTabView.vuetest/renderer/components/App.startup.test.tstest/renderer/components/ChatInputToolbar.test.tstest/renderer/components/ChatPage.test.tstest/renderer/components/ChatTabView.test.tstest/renderer/components/MessageList.test.tstest/renderer/components/SettingsApp.test.tstest/renderer/components/WindowSideBar.test.tstest/renderer/components/message/MessageItemAssistant.test.tstest/renderer/composables/chat/chatScrollState.test.tstest/renderer/composables/useMessageVirtualization.test.tstest/renderer/composables/useMessageWindow.test.tstest/renderer/features/chat-page/composables/useChatSearch.test.tstest/renderer/features/chat-page/composables/useComposerSubmit.test.tstest/renderer/features/chat-page/composables/useMessageActions.test.tstest/renderer/features/chat-page/composables/useToolInteraction.test.tstest/renderer/foundation/appearance/documentAppearance.test.tstest/renderer/lib/chatSearch.test.tstest/renderer/pages/NewThreadPage.test.tstest/renderer/stores/languageStore.test.tstest/renderer/stores/memoryActivityStore.test.tstest/renderer/stores/messageStore.test.tstest/renderer/stores/projectStore.test.tstest/renderer/stores/sessionStore.test.tstest/renderer/stores/themeStore.test.ts
💤 Files with no reviewable changes (2)
- src/renderer/src/views/SettingsTabView.vue
- src/renderer/src/stores/ui/draft.ts
…e-optimization Resolve language/i18n conflicts to preserve this PR's scope-boundary race fixes (languageInitialization promise cache, listener-before-snapshot registration, documentAppearance abstraction) alongside the lazy renderer locale loading (#2003) and dashboard CPU reduction (#2004) perf wins from dev.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/renderer/src/stores/language.ts (2)
59-67: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClear initialization cache when locale application fails.
applyLanguageStatecatches its own errors and returns a boolean rather than throwing. If loading locale messages fails, it returnsfalse, which bypasses thecatchblock here. As a result,languageInitializationincorrectly retains a successfully resolved promise, preventing any future initialization retries.Check the return value of
applyLanguageStateto accurately clear the initialization cache upon failure.🐛 Proposed fix
const initialization = (async () => { try { const languageState = await configClient.getLanguageState() - await applyLanguageState(languageState, revision) + const success = await applyLanguageState(languageState, revision) + if (!success) { + languageInitialization = null + } } catch (error) { languageInitialization = null console.error('初始化语言失败:', error) } })()🤖 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/stores/language.ts` around lines 59 - 67, Update the initialization flow around applyLanguageState to inspect its boolean result and set languageInitialization to null when it returns false, while retaining the existing catch handling for thrown errors and successful initialization behavior.
83-85: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAvoid using component lifecycle hooks in Pinia setup stores.
Vue lifecycle hooks like
onMountedare bound to the component that first invokesuseLanguageStore(). If the store is first instantiated outside a component context (such as in a router navigation guard or a background service module), theonMountedhook will be silently ignored, andinitLanguagewill never execute.Trigger initialization directly in the store setup to ensure reliable execution regardless of where the store is first used.
🐛 Proposed fix
- onMounted(async () => { - await initLanguage() - }) + void initLanguage()(You can also remove
onMountedfrom the Vue imports at the top of the file if it is no longer used).🤖 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/stores/language.ts` around lines 83 - 85, Remove the onMounted wrapper from the language store setup and invoke initLanguage directly so initialization runs regardless of where useLanguageStore is first instantiated. Remove the now-unused onMounted import while preserving the existing async initialization behavior.
🧹 Nitpick comments (1)
src/renderer/src/stores/language.ts (1)
9-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the shared appearance helper to resolve document direction.
The
RTL_LISTduplicates the logic already provided byresolveDocumentDirection. To fully align with the PR objective of using a shared appearance projection, removeRTL_LISTand use the exportedresolveDocumentDirectionhelper.♻️ Proposed refactor
Add the import at the top of the file:
import { resolveDocumentDirection } from '`@/foundation/appearance/documentAppearance`'And update the code accordingly:
-const RTL_LIST = ['fa-IR', 'he-IL'] - export const useLanguageStore = defineStore('language', () => { const { locale, setLocaleMessage } = useI18n({ useScope: 'global' }) const language = shallowRef<string>('system') const configClient = createConfigClient() const initialLocale = resolveSupportedLocale(locale.value) - const dir = shallowRef<'auto' | 'rtl'>(RTL_LIST.includes(initialLocale) ? 'rtl' : 'auto') + const dir = shallowRef<'auto' | 'rtl'>(resolveDocumentDirection(initialLocale)) let transitionRevision = 0 let updateRequestRevision = 0 let removeLanguageListener: (() => void) | undefined let languageInitialization: Promise<void> | null = null const applyLanguageState = async (state: RendererLanguageState, revision: number) => { const resolvedLocale = resolveSupportedLocale(state.locale) try { const messages = await loadLocaleMessages(resolvedLocale) if (revision !== transitionRevision) return false setLocaleMessage(resolvedLocale, messages) locale.value = resolvedLocale language.value = state.requestedLanguage || 'system' - dir.value = state.direction === 'rtl' || RTL_LIST.includes(resolvedLocale) ? 'rtl' : 'auto' + dir.value = state.direction === 'rtl' || resolveDocumentDirection(resolvedLocale) === 'rtl' ? 'rtl' : 'auto' return true🤖 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/stores/language.ts` around lines 9 - 33, Replace the local RTL_LIST-based direction logic in useLanguageStore with the shared resolveDocumentDirection helper, importing it from the appearance module. Use the helper for both initial dir setup and the direction assignment in applyLanguageState, then remove RTL_LIST while preserving the existing resolved-locale and explicit state.direction behavior.
🤖 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.
Outside diff comments:
In `@src/renderer/src/stores/language.ts`:
- Around line 59-67: Update the initialization flow around applyLanguageState to
inspect its boolean result and set languageInitialization to null when it
returns false, while retaining the existing catch handling for thrown errors and
successful initialization behavior.
- Around line 83-85: Remove the onMounted wrapper from the language store setup
and invoke initLanguage directly so initialization runs regardless of where
useLanguageStore is first instantiated. Remove the now-unused onMounted import
while preserving the existing async initialization behavior.
---
Nitpick comments:
In `@src/renderer/src/stores/language.ts`:
- Around line 9-33: Replace the local RTL_LIST-based direction logic in
useLanguageStore with the shared resolveDocumentDirection helper, importing it
from the appearance module. Use the helper for both initial dir setup and the
direction assignment in applyLanguageState, then remove RTL_LIST while
preserving the existing resolved-locale and explicit state.direction behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d81600b2-b27f-4246-a3fa-6bcf33882046
📒 Files selected for processing (3)
docs/architecture/baselines/renderer-application-boundaries-baseline.jsonsrc/renderer/floating/main.tssrc/renderer/src/stores/language.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/architecture/baselines/renderer-application-boundaries-baseline.json
Address coderabbit review comments on PR #2000: - ChatInputToolbar.test: trigger steer click and assert no emission while pending (was only checking static attributes) - session store: rename isNewerSessionUpdate -> isStaleOrSameSessionUpdate so the predicate name matches its "skip stale/equal update" semantics - extract shared createDeferred helper (with reject) to test/renderer/utils, replacing three local copies in useComposerSubmit/useMessageActions/ useToolInteraction tests - useMessageVirtualization.test: cover layoutSegments provided but returning null at runtime (inline-stream reinsertion fallback) - SettingsApp.test: reset document.documentElement lang/dir in afterEach to match sibling documentAppearance cleanup
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/renderer/stores/floatingButtonStore.test.ts (1)
25-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
deferredhelper — reuse the shared test utility.This appears to duplicate
test/renderer/utils/deferred.ts, which other renderer store tests in this PR already use for the identical pattern.♻️ Suggested fix
-function deferred<T>() { - let resolve!: (value: T) => void - const promise = new Promise<T>((resolvePromise) => { - resolve = resolvePromise - }) - return { promise, resolve } -} +import { deferred } from '../utils/deferred'#!/bin/bash # Confirm the shared deferred utility exists and matches this local implementation fd deferred.ts test/renderer cat -n test/renderer/utils/deferred.ts 2>/dev/null🤖 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/stores/floatingButtonStore.test.ts` around lines 25 - 31, Remove the local deferred helper from the floating button store test and import and reuse the shared deferred utility from test/renderer/utils/deferred.ts, updating its call sites as needed while preserving the existing test behavior.
🤖 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 `@docs/architecture/renderer-scope-optimization/spec.md`:
- Line 66: Update the Chinese sentence in the renderer-scope optimization
specification to use the clearer wording “不得覆盖用户在等待期间手动选择的新项目”, preserving the
original meaning.
---
Nitpick comments:
In `@test/renderer/stores/floatingButtonStore.test.ts`:
- Around line 25-31: Remove the local deferred helper from the floating button
store test and import and reuse the shared deferred utility from
test/renderer/utils/deferred.ts, updating its call sites as needed while
preserving the existing test behavior.
🪄 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: 754cdb1f-3a65-4a7c-8265-fc4939045818
📒 Files selected for processing (10)
docs/architecture/renderer-scope-optimization/spec.mddocs/architecture/renderer-scope-optimization/tasks.mdsrc/renderer/src/apps/chat-main/ChatMainApp.vuesrc/renderer/src/pages/NewThreadPage.vuesrc/renderer/src/stores/floatingButton.tssrc/renderer/src/stores/language.tstest/renderer/pages/NewThreadPage.test.tstest/renderer/stores/floatingButtonStore.test.tstest/renderer/stores/languageStore.test.tstest/renderer/stores/projectStore.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- test/renderer/stores/projectStore.test.ts
- src/renderer/src/stores/language.ts
- test/renderer/pages/NewThreadPage.test.ts
- src/renderer/src/apps/chat-main/ChatMainApp.vue
zhangmo8
marked this pull request as draft
July 21, 2026 03:33
zhangmo8
marked this pull request as ready for review
July 21, 2026 10:46
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/src/components/markdown/MarkdownRenderer.vue (1)
195-198: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClear the cached search-results promise on failure
searchResultsPromisestays stuck on a rejected promise untilcustomRendererIdchanges, so a transient failure can leave reference hover/click handling broken for the rest of the render cycle. Reset the cache on rejection and add a.catch()to the.then()chains so the rejection doesn’t bubble as unhandled.🤖 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/components/markdown/MarkdownRenderer.vue` around lines 195 - 198, Update getSearchResults so a rejected sessionClient.getSearchResults promise clears searchResultsPromise, allowing a later request to retry. Add rejection handling to every .then() chain consuming this promise so failures are handled and do not surface as unhandled rejections, while preserving successful result processing.
🤖 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 `@resources/model-db/providers.json`:
- Around line 258751-258754: Update the meituan/longcat-2.0 provider entry’s
limit.context value from 1048756 to 1048576, while leaving its output limit
unchanged.
---
Outside diff comments:
In `@src/renderer/src/components/markdown/MarkdownRenderer.vue`:
- Around line 195-198: Update getSearchResults so a rejected
sessionClient.getSearchResults promise clears searchResultsPromise, allowing a
later request to retry. Add rejection handling to every .then() chain consuming
this promise so failures are handled and do not surface as unhandled rejections,
while preserving successful result processing.
🪄 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: 766311ad-8b4e-4ea4-bb6d-0dad93275785
📒 Files selected for processing (20)
docs/architecture/chat-markstream-rendering-pipeline/plan.mddocs/architecture/chat-markstream-rendering-pipeline/spec.mddocs/architecture/chat-markstream-rendering-pipeline/tasks.mddocs/issues/markstream-code-block-rendering/spec.mdpackage.jsonresources/acp-registry/registry.jsonresources/model-db/providers.jsonsrc/renderer/src/components/markdown/MarkdownRenderer.vuesrc/renderer/src/components/message/MessageBlockContent.vuesrc/renderer/src/components/message/MessageItemAssistant.vuesrc/renderer/src/features/chat-page/composables/useDisplayMessages.tssrc/renderer/src/stores/ui/message.tssrc/renderer/src/stores/ui/stream.tstest/main/agent/deepchat/runtime/echo.test.tstest/renderer/components/MarkdownRenderer.test.tstest/renderer/components/MarkstreamDomContract.test.tstest/renderer/components/message/MessageBlockContent.test.tstest/renderer/components/message/MessageItemAssistant.test.tstest/renderer/features/chat-page/composables/useDisplayMessages.test.tstest/renderer/stores/messageStore.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/renderer/src/components/message/MessageItemAssistant.vue
- src/renderer/src/features/chat-page/composables/useDisplayMessages.ts
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
Renderer scope-boundary restructuring per
docs/architecture/renderer-scope-optimization(spec/plan/tasks included), plus fixes for every issue confirmed by a final four-boundary independent review (stores/foundation, chat search, message window/virtualization, page shell/router).Structure
ChatTabViewintoapps/chat-main; remove unusedviews/SettingsTabView(settings owned by the dedicated settings renderer)foundation/appearance/documentAppearanceshared by chat-main, settings, and floating entries (theme / font size / locale / direction projection)Review-confirmed fixes
textand rendered content blocks are no longer double-counted (match counter desynced from DOM marks, causing dead Enter-navigation slots)totalHeight/ window range never updated and the pending-assistant placeholder row was invisible; batch measurements no longer trigger a full layout rebuild per rowhasExplicitNavigation)sessionIpctargeted-update payload narrowed explicitlyisInGeneratingThread(fork gating, variant auto-switch) split from new row-levelisStreamingMessage(activity grouping only), restoring fork-disabled-while-generating; time-group workspace sessions sort by recency againTests
useChatSearch,useMessageVirtualization,documentAppearance,languageStore,themeStorelocale, globalrestoreAllMocksstripping listener-mock implementations, full-page restore mockstypecheck/lint/format/i18nclean; renderer suite 179 files / 1410 tests green.🤖 Generated with Claude Code
Summary by CodeRabbit