Conversation
Contributor
📝 WalkthroughWalkthroughThis change adds optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (3)
src/renderer/src/components/sidepanel/WorkspacePanel.vue (1)
258-281:⚠️ Potential issue | 🟠 MajorWatch the current-artifact fields too.
Line 272 derives
matchesCurrentArtifactfromartifactStore.current*, but the watcher only subscribes toartifactItemsandselectedArtifactContext. IfselectedArtifactContextis present first andsyncArtifact()fillscurrentMessageId/currentThreadIdlater, this callback still clears once and never re-evaluates when the store catches up. Please add the three current-artifact fields to the watch sources, or watch a computedmatchesCurrentArtifactinstead.🔧 Suggested change
watch( - [artifactItems, () => sessionState.value.selectedArtifactContext] as const, - ([items, context]) => { + [ + artifactItems, + () => sessionState.value.selectedArtifactContext, + () => artifactStore.currentArtifact?.id, + () => artifactStore.currentMessageId, + () => artifactStore.currentThreadId + ] as const, + ([items, context, currentArtifactId, currentMessageId, currentThreadId]) => { if (!context) { return } const existsInArtifactItems = items.some( @@ const matchesCurrentArtifact = - artifactStore.currentArtifact?.id === context.artifactId && - artifactStore.currentMessageId === context.messageId && - artifactStore.currentThreadId === context.threadId + currentArtifactId === context.artifactId && + currentMessageId === context.messageId && + currentThreadId === context.threadId if (!existsInArtifactItems && !matchesCurrentArtifact) { sidepanelStore.clearArtifact(props.sessionId) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/components/sidepanel/WorkspacePanel.vue` around lines 258 - 281, The watcher currently listens to artifactItems and () => sessionState.value.selectedArtifactContext but not the artifactStore.current* fields, causing a race where matchesCurrentArtifact (derived from artifactStore.currentArtifact/currentMessageId/currentThreadId) may change later without retriggering the watcher; update the watch sources to include artifactStore.currentArtifact, artifactStore.currentMessageId, and artifactStore.currentThreadId (or replace with a single computed matchesCurrentArtifact) so the callback re-evaluates when the store updates, keeping the existing logic that calls sidepanelStore.clearArtifact(props.sessionId) when neither existsInArtifactItems nor matchesCurrentArtifact are true.src/renderer/src/components/sidepanel/WorkspaceViewer.vue (1)
103-109:⚠️ Potential issue | 🟠 MajorForward the originating artifact
messageIdhere too.Line 105 passes the thread id, but the owning message id is still dropped at this boundary.
WorkspacePreviewPanenow falls back toartifact.id, andMarkdownRendereruses that value as the exact key forgetSearchResults()and as part of nested preview context. For artifacts opened fromWorkspacePanel,artifact.idis only the artifact identifier, so citations and child previews lose their original message context. Please passsessionState.value.selectedArtifactContext?.messageIdfor artifact previews and keep the path fallback only for raw workspace files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/components/sidepanel/WorkspaceViewer.vue` around lines 103 - 109, The WorkspacePreviewPane call drops the originating artifact message id, causing MarkdownRenderer/getSearchResults() to lose message context; update the WorkspacePreviewPane invocation to forward the originating message id (e.g. pass sessionState.value.selectedArtifactContext?.messageId) as a prop (named something like originatingMessageId or messageId) alongside the existing props (previewArtifact/previewFilePreview) and ensure WorkspacePreviewPane/MarkdownRenderer use that prop and only fall back to artifact.id for raw workspace files; reference WorkspacePreviewPane, previewArtifact, previewFilePreview, sessionState.value.selectedArtifactContext?.messageId, MarkdownRenderer, and getSearchResults() when making the change.src/renderer/src/components/markdown/MarkdownRenderer.vue (1)
66-134:⚠️ Potential issue | 🟠 MajorPass a custom-id to
setCustomComponents()to scope component handlers per renderer instance.Line 66 calls
setCustomComponents()without a custom-id, which registers handlers in the global/default mapping. In views with multipleMarkdownRendererinstances, each subsequent renderer overwrites the handlers used by previous ones, causing reference previews and code previews to be associated with the wrong message context. Pass a unique custom-id (derived fromeffectiveMessageIdand/oreffectiveThreadId) as the second parameter tosetCustomComponents()to isolate each renderer's handlers:setCustomComponents( { reference: (_props) => ..., mermaid: (_props) => ..., code_block: (_props) => ... }, `markdown-${effectiveMessageId.value}-${effectiveThreadId.value}` )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/components/markdown/MarkdownRenderer.vue` around lines 66 - 134, The handlers passed to setCustomComponents are being registered globally causing cross-instance collisions; update the setCustomComponents call to pass a per-renderer custom id (e.g. derived from effectiveMessageId.value and/or effectiveThreadId.value) as the second argument so each MarkdownRenderer instance scopes its handlers (keep using the same component factories reference, mermaid, code_block, but call setCustomComponents(components, uniqueId) where uniqueId uses effectiveMessageId/effectiveThreadId).
🧹 Nitpick comments (2)
test/renderer/components/WorkspacePanel.test.ts (1)
491-519: This regression test misses the ordering that still matters.Here the matching
artifactStore.current*fields are populated before mount. The remaining failure mode is the opposite order:selectedArtifactContextexists first, then the artifact store catches up later. Adding that sequence here would guard the asyncsyncArtifact()path as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/renderer/components/WorkspacePanel.test.ts` around lines 491 - 519, The test should also cover the ordering where sessionState.selectedArtifactContext is set before the artifact store values arrive so the async syncArtifact() path is exercised; modify the 'keeps the current temporary artifact selection...' spec to set sessionState.selectedArtifactContext first, then mount WorkspacePanel, then (after mount) assign artifactStore.currentArtifact, artifactStore.currentMessageId and artifactStore.currentThreadId, call await flushPromises(), and finally assert clearArtifactMock was not called; target the same symbols (sessionState.selectedArtifactContext, artifactStore.currentArtifact, artifactStore.currentMessageId, artifactStore.currentThreadId, WorkspacePanel and syncArtifact behavior) so the opposite-order timing is covered.test/renderer/components/message/MessageBlockContent.test.ts (1)
53-73: Consider adding theloadingprop to the stub for completeness.The stub includes
content,messageId, andthreadId, but the actualMarkdownRenderercomponent and its usage inMessageBlockContent.vuealso passes a:loadingprop. While the current tests don't require it, adding it to the stub would make the mock more representative of the real component.♻️ Optional: Add loading prop to stub
vi.mock('@/components/markdown/MarkdownRenderer.vue', () => ({ default: defineComponent({ name: 'MarkdownRenderer', props: { content: { type: String, default: '' }, + loading: { + type: Boolean, + default: false + }, messageId: { type: String, default: undefined }, threadId: { type: String, default: undefined } }, template: '<div class="markdown-stub" :data-message-id="messageId" :data-thread-id="threadId">{{ content }}</div>' }) }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/renderer/components/message/MessageBlockContent.test.ts` around lines 53 - 73, The MarkdownRenderer stub in the test mock is missing the loading prop used by MessageBlockContent.vue; update the mock definition (the default defineComponent named 'MarkdownRenderer' in the test) to declare a loading prop (type: Boolean, default: false) and expose it in the template (e.g., as a data attribute like :data-loading="loading") so the stub mirrors the real component's API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/renderer/src/components/markdown/MarkdownRenderer.vue`:
- Around line 66-134: The handlers passed to setCustomComponents are being
registered globally causing cross-instance collisions; update the
setCustomComponents call to pass a per-renderer custom id (e.g. derived from
effectiveMessageId.value and/or effectiveThreadId.value) as the second argument
so each MarkdownRenderer instance scopes its handlers (keep using the same
component factories reference, mermaid, code_block, but call
setCustomComponents(components, uniqueId) where uniqueId uses
effectiveMessageId/effectiveThreadId).
In `@src/renderer/src/components/sidepanel/WorkspacePanel.vue`:
- Around line 258-281: The watcher currently listens to artifactItems and () =>
sessionState.value.selectedArtifactContext but not the artifactStore.current*
fields, causing a race where matchesCurrentArtifact (derived from
artifactStore.currentArtifact/currentMessageId/currentThreadId) may change later
without retriggering the watcher; update the watch sources to include
artifactStore.currentArtifact, artifactStore.currentMessageId, and
artifactStore.currentThreadId (or replace with a single computed
matchesCurrentArtifact) so the callback re-evaluates when the store updates,
keeping the existing logic that calls
sidepanelStore.clearArtifact(props.sessionId) when neither existsInArtifactItems
nor matchesCurrentArtifact are true.
In `@src/renderer/src/components/sidepanel/WorkspaceViewer.vue`:
- Around line 103-109: The WorkspacePreviewPane call drops the originating
artifact message id, causing MarkdownRenderer/getSearchResults() to lose message
context; update the WorkspacePreviewPane invocation to forward the originating
message id (e.g. pass sessionState.value.selectedArtifactContext?.messageId) as
a prop (named something like originatingMessageId or messageId) alongside the
existing props (previewArtifact/previewFilePreview) and ensure
WorkspacePreviewPane/MarkdownRenderer use that prop and only fall back to
artifact.id for raw workspace files; reference WorkspacePreviewPane,
previewArtifact, previewFilePreview,
sessionState.value.selectedArtifactContext?.messageId, MarkdownRenderer, and
getSearchResults() when making the change.
---
Nitpick comments:
In `@test/renderer/components/message/MessageBlockContent.test.ts`:
- Around line 53-73: The MarkdownRenderer stub in the test mock is missing the
loading prop used by MessageBlockContent.vue; update the mock definition (the
default defineComponent named 'MarkdownRenderer' in the test) to declare a
loading prop (type: Boolean, default: false) and expose it in the template
(e.g., as a data attribute like :data-loading="loading") so the stub mirrors the
real component's API.
In `@test/renderer/components/WorkspacePanel.test.ts`:
- Around line 491-519: The test should also cover the ordering where
sessionState.selectedArtifactContext is set before the artifact store values
arrive so the async syncArtifact() path is exercised; modify the 'keeps the
current temporary artifact selection...' spec to set
sessionState.selectedArtifactContext first, then mount WorkspacePanel, then
(after mount) assign artifactStore.currentArtifact,
artifactStore.currentMessageId and artifactStore.currentThreadId, call await
flushPromises(), and finally assert clearArtifactMock was not called; target the
same symbols (sessionState.selectedArtifactContext,
artifactStore.currentArtifact, artifactStore.currentMessageId,
artifactStore.currentThreadId, WorkspacePanel and syncArtifact behavior) so the
opposite-order timing is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a5f987f-a9a0-4952-be9e-f34cd6aae31d
📒 Files selected for processing (10)
src/renderer/src/components/markdown/MarkdownRenderer.vuesrc/renderer/src/components/message/MessageBlockContent.vuesrc/renderer/src/components/sidepanel/WorkspacePanel.vuesrc/renderer/src/components/sidepanel/WorkspaceViewer.vuesrc/renderer/src/components/sidepanel/viewer/WorkspacePreviewPane.vuetest/renderer/components/MarkdownRenderer.test.tstest/renderer/components/WorkspacePanel.test.tstest/renderer/components/WorkspacePreviewPane.test.tstest/renderer/components/WorkspaceViewer.test.tstest/renderer/components/message/MessageBlockContent.test.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 by CodeRabbit
Release Notes
Bug Fixes
Improvements