Conversation
Contributor
📝 WalkthroughWalkthroughAdds a hidden-by-default right-side Sidepanel (workspace + embedded browser), replaces ArtifactDialog, refactors workspace presenter for file previews and git/diff APIs, removes plan/terminal snippet plumbing and command process tracking, consolidates embedded browser lifecycle, updates renderer stores/components, i18n, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MessageBlockContent
participant ArtifactStore
participant SidepanelStore
participant ChatPage
User->>MessageBlockContent: Artifact generation completes
MessageBlockContent->>ArtifactStore: completeArtifact(artifact, messageId, threadId)
ArtifactStore->>SidepanelStore: selectArtifact(context, { open: true, viewMode })
SidepanelStore->>ChatPage: openWorkspace(sessionId)
ChatPage->>User: Slide open sidepanel and focus artifact
sequenceDiagram
participant User
participant WorkspacePanel
participant WorkspacePresenter
participant FileSystem
participant Git
participant WorkspaceViewer
User->>WorkspacePanel: Click file
WorkspacePanel->>WorkspacePresenter: readFilePreview(filePath)
WorkspacePresenter->>FileSystem: read file
WorkspacePresenter-->>WorkspacePanel: WorkspaceFilePreview
WorkspacePanel->>WorkspaceViewer: render preview
User->>WorkspacePanel: Click changed file
WorkspacePanel->>WorkspacePresenter: getGitDiff(workspacePath, filePath)
WorkspacePresenter->>Git: run git diff
WorkspacePresenter-->>WorkspacePanel: WorkspaceGitDiff
WorkspacePanel->>WorkspaceViewer: render diff
sequenceDiagram
participant BrowserPanel
participant YoBrowserPresenter
participant WindowHost
participant WebContentsView
BrowserPanel->>YoBrowserPresenter: ensureEmbeddedState / navigate(url)
YoBrowserPresenter->>WebContentsView: create or reuse embedded view (rgba(0,128,255,0.5))
YoBrowserPresenter->>WindowHost: attachEmbeddedToWindow(hostWindowId)
BrowserPanel->>YoBrowserPresenter: resize observer -> updateEmbeddedBounds(bounds)
YoBrowserPresenter->>WebContentsView: update bounds & visibility
BrowserPanel->>YoBrowserPresenter: close/switch -> detachEmbedded()
YoBrowserPresenter->>WindowHost: remove embedded view
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/presenter/agentPresenter/acp/agentBashHandler.ts (1)
91-104:⚠️ Potential issue | 🟠 MajorChange to
'close'event to ensure all stdio is fully drained.The foreground execution awaits the complete buffered output from
runShellProcess(), but the helper currently resolves on the'exit'event. In Node.js,'exit'fires when the process terminates but before all stdio streams are fully closed. This creates a race condition where final output chunks can be lost even when the command succeeds. Use the'close'event instead, which only fires after all stdio streams are fully drained.Suggested fix
- child.on('exit', (code, signal) => { + child.on('close', (code, signal) => { if (timeoutId) clearTimeout(timeoutId) if (killTimeoutId) clearTimeout(killTimeoutId) if (signal && timedOut) { exitCode = null } else { exitCode = code ?? null } resolve({ output, exitCode, timedOut, truncated }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/agentPresenter/acp/agentBashHandler.ts` around lines 91 - 104, The helper runShellProcess currently resolves when the child process emits 'exit', which can fire before stdio is fully drained and lose trailing output; update runShellProcess (in agentBashHandler.ts) to listen for and resolve on the child's 'close' event instead of 'exit' so the promise only settles after all stdio streams are closed, ensuring result.output contains complete buffered output and preserving the existing handling of exitCode, timedOut, and truncated flags.src/renderer/src/stores/artifact.ts (1)
111-126:⚠️ Potential issue | 🟠 MajorScope artifact syncing to the artifact id, not just the message context.
Lines 111-113 only validate
messageIdandthreadId. On Line 126, a second artifact from the same assistant message can replacecurrentArtifacteven if the sidepanel is still pointing at the first artifact, so selection and rendered content drift apart.Proposed fix
- const validateContext = (messageId: string, threadId: string) => { - return currentMessageId.value === messageId && currentThreadId.value === threadId + const validateContext = (messageId: string, threadId: string, artifactId?: string) => { + if (currentMessageId.value !== messageId || currentThreadId.value !== threadId) { + return false + } + return artifactId == null || currentArtifact.value?.id === artifactId } @@ const syncArtifact = (artifact: ArtifactState, messageId: string, threadId: string) => { - if (!currentArtifact.value || validateContext(messageId, threadId)) { + if (!currentArtifact.value || validateContext(messageId, threadId, artifact.id)) { currentArtifact.value = artifact currentMessageId.value = messageId currentThreadId.value = threadId } } @@ - const currentMatches = - validateContext(messageId, threadId) && currentArtifact.value?.id === artifact.id + const currentMatches = validateContext(messageId, threadId, artifact.id)Also applies to: 133-139
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/stores/artifact.ts` around lines 111 - 126, The current context check (validateContext) only compares currentMessageId and currentThreadId, allowing a different artifact from the same assistant message to replace the visible artifact; update sync logic to also guard by artifact id: when syncing in syncArtifact (and the similar block around lines 133-139), only replace or merge into currentArtifact when currentArtifact.value is null/undefined OR when currentArtifact.value.id === artifact.id (and the existing message/thread check still passes). Modify validateContext or add a small artifact id check in syncArtifact so selection stays scoped to the selected artifact id (reference symbols: validateContext, syncArtifact, currentArtifact, currentMessageId, currentThreadId, artifact.id).
🧹 Nitpick comments (5)
src/renderer/src/i18n/zh-TW/chat.json (1)
133-137: Keep the workspace “files” label consistent.Line 134 uses
檔案, but the existingworkspace.files.sectionkey still says文件. If both render in the unified sidepanel, users will see two names for the same section.📝 Suggested alignment
"sections": { "files": "檔案", "git": "Git", "artifacts": "Artifacts" }, ... - "section": "文件" + "section": "檔案"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/zh-TW/chat.json` around lines 133 - 137, The "sections.files" translation currently uses "檔案" while elsewhere the key workspace.files.section uses "文件", causing inconsistent labels; update one of these entries so both use the same Chinese term (choose either "檔案" or "文件") to ensure the unified sidepanel shows a single consistent label—edit the "sections" object (key "files") or the "workspace.files.section" entry so both keys map to the identical string.src/renderer/src/composables/useArtifacts.ts (1)
26-38: Avoid duplicating the artifact MIME union.
ParsedArtifactPart.typerepeats the same literals already expressed byProcessedPart/ArtifactType, so the next MIME-type addition has to be updated in multiple places. Reusing the shared type keeps these shapes in sync.Suggested simplification
export interface ParsedArtifactPart { identifier: string title: string - type: - | 'application/vnd.ant.code' - | 'text/markdown' - | 'text/html' - | 'image/svg+xml' - | 'application/vnd.ant.mermaid' - | 'application/vnd.ant.react' + type: ArtifactType language?: string content: string loading: boolean }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/composables/useArtifacts.ts` around lines 26 - 38, The ParsedArtifactPart.type union duplicates the MIME literals already defined elsewhere; update ParsedArtifactPart to reuse the shared type (e.g., ArtifactType or ProcessedPart['type']) instead of re-declaring the union so new MIME types only need to be added in one place; locate ParsedArtifactPart in useArtifacts.ts and replace its explicit union type with the shared identifier (ArtifactType or ProcessedPart['type']), updating any imports if necessary.src/shared/types/presenters/workspace.d.ts (2)
30-36: Consider usingnumber(timestamp) instead ofDatefor IPC compatibility.
fileCreatedandfileModifiedare typed asDate, butDateobjects don't serialize properly over Electron IPC (they become plain strings). Consider usingnumber(Unix timestamp) orstring(ISO 8601) for consistent serialization.♻️ Proposed fix
export type WorkspaceFileMetadata = { fileName: string fileSize: number fileDescription?: string - fileCreated: Date - fileModified: Date + fileCreated: number + fileModified: number }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/types/presenters/workspace.d.ts` around lines 30 - 36, The WorkspaceFileMetadata type uses Date for fileCreated and fileModified which do not serialize cleanly over Electron IPC; change both properties to number (Unix timestamp) instead of Date in the WorkspaceFileMetadata type definition and update any callers/consumers that construct or read WorkspaceFileMetadata (e.g., where timestamps are created or parsed) to use Date.getTime()/new Date(timestamp) as needed so serialization across IPC becomes consistent.
46-46: Simplify optional union type.
language?: string | nullcombines optional (?) with| null, which is redundant. Either uselanguage?: string(can be undefined) orlanguage: string | null(must be present but nullable).♻️ Suggested simplification
- language?: string | null + language?: string🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/types/presenters/workspace.d.ts` at line 46, The property declaration language?: string | null is redundant; update the type to a single nullable/optional form — either change it to language?: string (optional, may be undefined) or to language: string | null (required but may be null); pick the form consistent with the surrounding presenter types and update the property declaration for language accordingly.src/renderer/src/components/sidepanel/WorkspacePanel.vue (1)
280-314: Consider adding error handling for file preview and git diff loading.The watch handlers for
selectedFilePathandselectedDiffPathcall async presenter methods without try-catch blocks. IfreadFilePrevieworgetGitDiffthrows an error, the loading state will remaintrueand the user will see a perpetual loading indicator.♻️ Proposed fix to add error handling
watch( () => sessionState.value.selectedFilePath, async (filePath) => { selectedFilePreview.value = null if (!filePath) { return } loadingFilePreview.value = true try { selectedFilePreview.value = await workspacePresenter.readFilePreview(filePath) - } finally { + } catch (error) { + console.error('[WorkspacePanel] Failed to load file preview:', error) + } finally { loadingFilePreview.value = false } }, { immediate: true } ) watch( () => sessionState.value.selectedDiffPath, async (filePath) => { selectedGitDiff.value = null if (!filePath || !props.workspacePath) { return } loadingGitDiff.value = true try { selectedGitDiff.value = await workspacePresenter.getGitDiff(props.workspacePath, filePath) - } finally { + } catch (error) { + console.error('[WorkspacePanel] Failed to load git diff:', error) + } finally { loadingGitDiff.value = false } }, { immediate: true } )🤖 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 280 - 314, The watch handlers for sessionState.value.selectedFilePath and sessionState.value.selectedDiffPath must catch errors so loading flags don't hang; wrap the async calls to workspacePresenter.readFilePreview(filePath) and workspacePresenter.getGitDiff(props.workspacePath, filePath) in try-catch-finally blocks, set the respective loadingFilePreview.value and loadingGitDiff.value back to false in finally, assign null or a user-facing error state to selectedFilePreview.value/selectedGitDiff.value on error, and log the error (e.g., via console.error or an existing logger) inside the catch so failures are visible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/specs/right-sidepanel/plan.md`:
- Around line 82-90: Remove the nonexistent method name "clearWorkspaceData"
from the deletion list that currently contains getPlanEntries,
updatePlanEntries, emitTerminalSnippet, terminateCommand, clearWorkspaceData,
and PlanStateManager; update the documentation block so the deletion list only
includes the real items (getPlanEntries, updatePlanEntries, emitTerminalSnippet,
terminateCommand, PlanStateManager).
In `@src/main/presenter/browser/YoBrowserPresenter.ts`:
- Around line 148-165: detachEmbedded currently removes the WebContentsView but
leaves state.visible possibly true, causing show()/toggleVisibility() to report
visible when nothing is attached; update detachEmbedded (in
YoBrowserPresenter.detachEmbedded) to also set state.visible = false and ensure
setWindowVisibility is called accordingly, and modify the embedded branches of
show() and toggleVisibility() (the methods handling state.visible changes) to
check state.attachedWindowId and/or state.view attachment before setting
state.visible = true or emitting visibility events — if not attached, either
attach the view first or prevent reporting visible until reattachment.
- Around line 77-122: attachEmbeddedToWindow currently moves the content view
but never wires the new host's lifecycle handlers, so after a reattach the new
BrowserWindow never gets attachWindowListeners() and its focus/show/hide/close
events aren't observed; update attachEmbeddedToWindow to call
this.attachWindowListeners(window) whenever you successfully add the embedded
view to a new window (both the initial add and the reattach branch where
state.attachedWindowId !== windowId), and optionally call a corresponding
detach/cleanup for previousWindow (identified by state.attachedWindowId) before
removing its child view to avoid duplicate listeners; use the existing symbols
attachEmbeddedToWindow, attachWindowListeners, state.attachedWindowId and
previousWindow to locate and modify the code.
- Around line 512-518: The WebContentsView is created with
webPreferences.sandbox set to false which weakens isolation for arbitrary web
content; update the WebContentsView instantiation (the webPreferences object
passed to the WebContentsView constructor) to set sandbox: true while keeping
devTools and session (getYoBrowserSession()) as-is so CDP and the existing
session permission/security settings continue to work without disabling the
Electron sandbox.
In `@src/renderer/src/components/sidepanel/BrowserPanel.vue`:
- Around line 4-27: The toolbar controls (icon Buttons invoking goBack,
goForward, reloadPage and the address Input bound to urlInput/navigate) lack
accessible names; add aria-label attributes (or an associated visually-hidden
<label>) for each control and source the strings from vue-i18n via t(), e.g. use
keys like t('common.browser.back'), t('common.browser.forward'),
t('common.browser.reload') and t('common.browser.addressLabel') so screen
readers announce the purpose of the Buttons and the address field; ensure you
update the Input and each Button markup to reference those t() keys rather than
hardcoded text.
In `@src/renderer/src/components/sidepanel/WorkspaceViewer.vue`:
- Around line 135-146: The viewer currently renders nothing when
previewComponent is null; update the conditional rendering in the activeSource
=== 'artifact' branches (the block that checks previewBlock and previewComponent
around the <component :is="previewComponent" ... /> usage) to provide a fallback
UI: if previewComponent is null render a DefaultArtifactPreview (or a simple
raw-content fallback) that accepts :block="previewBlock" and :is-preview="true"
and shows raw text or a download link; implement the same fallback change in the
analogous block referenced at lines 233-253 so unsupported artifact types show
raw content instead of a blank viewer.
- Around line 96-132: The branch that renders the file viewer is gated by
"activeSource === 'file' && props.filePreview", which hides the whole file pane
until props.filePreview arrives and prevents the loading spinner
(props.loadingFilePreview) from ever showing; change the outer conditional to
only check "activeSource === 'file'" (keep the inner templates that check
props.loadingFilePreview, props.filePreview.kind, previewBlock,
previewComponent, etc.) so the file branch remains active while preview data
loads and the spinner or fallback content can render correctly.
In `@src/renderer/src/i18n/fr-FR/chat.json`:
- Around line 129-133: The "sections.artifacts" translation in chat.json is
still English; update the value for the JSON key "artifacts" under the
"sections" object (workspace.sections.artifacts) from "Artifacts" to the French
translation "Artefacts" so it matches the other localized labels.
In `@src/renderer/src/i18n/ko-KR/chat.json`:
- Around line 129-133: The "sections.artifacts" entry in the Korean locale file
is left in English; update the value for the key "artifacts" inside the
"sections" object (workspace.sections.artifacts) to the Korean transliteration
"아티팩트" so the label matches the other translated section names and the locale
consistency.
In `@src/renderer/src/i18n/pt-BR/chat.json`:
- Around line 129-133: The translation for the workspace section key "artifacts"
is still in English; update the value for the JSON key "sections.artifacts"
(inside pt-BR chat.json) from "Artifacts" to the Portuguese translation
"Artefatos" so it matches the other localized section labels.
In `@src/renderer/src/i18n/ru-RU/chat.json`:
- Around line 129-133: The translation key "sections.artifacts" in
src/renderer/src/i18n/ru-RU/chat.json is left in English ("Artifacts"); update
the value for "sections": { "artifacts": "Артефакты" } so it matches the other
translated labels (e.g., "files": "Файлы") and maintains consistency across
workspace section names.
In `@src/renderer/src/stores/artifact.ts`:
- Around line 133-147: completeArtifact is forcing the side panel into 'preview'
before checking completedContexts, which snaps users back to Preview when
reprocessing an already completed artifact; move or guard the
sidepanelStore.setViewMode(threadId, 'preview') call so it only runs when the
context is not already completed (i.e., after checking
completedContexts.value.has(contextKey) or by adding a pre-check that returns
early), preserving the existing panel state if the contextKey is present;
reference completeArtifact, makeContextKey, completedContexts, and
sidepanelStore.setViewMode to locate and adjust the logic.
In `@src/renderer/src/stores/ui/sidepanel.ts`:
- Around line 46-52: The clamp logic in computed normalizedWidth currently
applies the minimum (420) after capping to resolveMaxWidth(), which causes it to
return 420 even when resolveMaxWidth() is smaller; update normalizedWidth (and
the analogous block at lines 68-69) to first get the resolved maximum
(resolveMaxWidth()), then clamp the rounded current width between the minimum
(420) and that resolved maximum so the final value never exceeds the viewport;
adjust the expression in normalizedWidth (referencing width.value and
resolveMaxWidth()) and the other similar computed to enforce max before min.
---
Outside diff comments:
In `@src/main/presenter/agentPresenter/acp/agentBashHandler.ts`:
- Around line 91-104: The helper runShellProcess currently resolves when the
child process emits 'exit', which can fire before stdio is fully drained and
lose trailing output; update runShellProcess (in agentBashHandler.ts) to listen
for and resolve on the child's 'close' event instead of 'exit' so the promise
only settles after all stdio streams are closed, ensuring result.output contains
complete buffered output and preserving the existing handling of exitCode,
timedOut, and truncated flags.
In `@src/renderer/src/stores/artifact.ts`:
- Around line 111-126: The current context check (validateContext) only compares
currentMessageId and currentThreadId, allowing a different artifact from the
same assistant message to replace the visible artifact; update sync logic to
also guard by artifact id: when syncing in syncArtifact (and the similar block
around lines 133-139), only replace or merge into currentArtifact when
currentArtifact.value is null/undefined OR when currentArtifact.value.id ===
artifact.id (and the existing message/thread check still passes). Modify
validateContext or add a small artifact id check in syncArtifact so selection
stays scoped to the selected artifact id (reference symbols: validateContext,
syncArtifact, currentArtifact, currentMessageId, currentThreadId, artifact.id).
---
Nitpick comments:
In `@src/renderer/src/components/sidepanel/WorkspacePanel.vue`:
- Around line 280-314: The watch handlers for
sessionState.value.selectedFilePath and sessionState.value.selectedDiffPath must
catch errors so loading flags don't hang; wrap the async calls to
workspacePresenter.readFilePreview(filePath) and
workspacePresenter.getGitDiff(props.workspacePath, filePath) in
try-catch-finally blocks, set the respective loadingFilePreview.value and
loadingGitDiff.value back to false in finally, assign null or a user-facing
error state to selectedFilePreview.value/selectedGitDiff.value on error, and log
the error (e.g., via console.error or an existing logger) inside the catch so
failures are visible.
In `@src/renderer/src/composables/useArtifacts.ts`:
- Around line 26-38: The ParsedArtifactPart.type union duplicates the MIME
literals already defined elsewhere; update ParsedArtifactPart to reuse the
shared type (e.g., ArtifactType or ProcessedPart['type']) instead of
re-declaring the union so new MIME types only need to be added in one place;
locate ParsedArtifactPart in useArtifacts.ts and replace its explicit union type
with the shared identifier (ArtifactType or ProcessedPart['type']), updating any
imports if necessary.
In `@src/renderer/src/i18n/zh-TW/chat.json`:
- Around line 133-137: The "sections.files" translation currently uses "檔案"
while elsewhere the key workspace.files.section uses "文件", causing inconsistent
labels; update one of these entries so both use the same Chinese term (choose
either "檔案" or "文件") to ensure the unified sidepanel shows a single consistent
label—edit the "sections" object (key "files") or the "workspace.files.section"
entry so both keys map to the identical string.
In `@src/shared/types/presenters/workspace.d.ts`:
- Around line 30-36: The WorkspaceFileMetadata type uses Date for fileCreated
and fileModified which do not serialize cleanly over Electron IPC; change both
properties to number (Unix timestamp) instead of Date in the
WorkspaceFileMetadata type definition and update any callers/consumers that
construct or read WorkspaceFileMetadata (e.g., where timestamps are created or
parsed) to use Date.getTime()/new Date(timestamp) as needed so serialization
across IPC becomes consistent.
- Line 46: The property declaration language?: string | null is redundant;
update the type to a single nullable/optional form — either change it to
language?: string (optional, may be undefined) or to language: string | null
(required but may be null); pick the form consistent with the surrounding
presenter types and update the property declaration for language accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed6266fd-4acf-491f-a24b-e8b66cc304d0
📒 Files selected for processing (47)
docs/specs/right-sidepanel/plan.mddocs/specs/right-sidepanel/spec.mddocs/specs/right-sidepanel/tasks.mdsrc/main/events.tssrc/main/presenter/agentPresenter/acp/agentBashHandler.tssrc/main/presenter/agentPresenter/acp/commandProcessTracker.tssrc/main/presenter/agentPresenter/acp/index.tssrc/main/presenter/browser/YoBrowserPresenter.tssrc/main/presenter/index.tssrc/main/presenter/workspacePresenter/index.tssrc/main/presenter/workspacePresenter/planStateManager.tssrc/renderer/src/components/WindowSideBar.vuesrc/renderer/src/components/artifacts/ArtifactDialog.vuesrc/renderer/src/components/artifacts/ArtifactPreview.vuesrc/renderer/src/components/chat/ChatInputBox.vuesrc/renderer/src/components/chat/ChatTopBar.vuesrc/renderer/src/components/chat/MessageList.vuesrc/renderer/src/components/message/MessageBlockContent.vuesrc/renderer/src/components/sidepanel/BrowserPanel.vuesrc/renderer/src/components/sidepanel/ChatSidePanel.vuesrc/renderer/src/components/sidepanel/WorkspacePanel.vuesrc/renderer/src/components/sidepanel/WorkspaceViewer.vuesrc/renderer/src/components/workspace/WorkspaceBrowserTabs.vuesrc/renderer/src/composables/useArtifacts.tssrc/renderer/src/events.tssrc/renderer/src/i18n/da-DK/chat.jsonsrc/renderer/src/i18n/en-US/chat.jsonsrc/renderer/src/i18n/fa-IR/chat.jsonsrc/renderer/src/i18n/fr-FR/chat.jsonsrc/renderer/src/i18n/he-IL/chat.jsonsrc/renderer/src/i18n/ja-JP/chat.jsonsrc/renderer/src/i18n/ko-KR/chat.jsonsrc/renderer/src/i18n/pt-BR/chat.jsonsrc/renderer/src/i18n/ru-RU/chat.jsonsrc/renderer/src/i18n/zh-CN/chat.jsonsrc/renderer/src/i18n/zh-HK/chat.jsonsrc/renderer/src/i18n/zh-TW/chat.jsonsrc/renderer/src/pages/ChatPage.vuesrc/renderer/src/stores/artifact.tssrc/renderer/src/stores/ui/sidepanel.tssrc/renderer/src/stores/yoBrowser.tssrc/renderer/src/views/ChatTabView.vuesrc/shared/types/presenters/index.d.tssrc/shared/types/presenters/legacy.presenters.d.tssrc/shared/types/presenters/workspace.d.tstest/renderer/components/MessageActionButtons.test.tstest/renderer/components/WindowSideBar.test.ts
💤 Files with no reviewable changes (10)
- src/main/presenter/agentPresenter/acp/index.ts
- src/renderer/src/components/WindowSideBar.vue
- test/renderer/components/WindowSideBar.test.ts
- src/renderer/src/stores/yoBrowser.ts
- src/renderer/src/events.ts
- src/main/presenter/workspacePresenter/planStateManager.ts
- src/main/events.ts
- src/renderer/src/components/workspace/WorkspaceBrowserTabs.vue
- src/main/presenter/agentPresenter/acp/commandProcessTracker.ts
- src/renderer/src/components/artifacts/ArtifactDialog.vue
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
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/artifact.ts (1)
111-113:⚠️ Potential issue | 🟠 MajorMatch artifact updates by
artifact.idtoo.
syncArtifact()still collapses identity down tomessageId/threadId. The rest of this store and the sidepanel context key onartifactId, so a later update from a different artifact in the same message can replacecurrentArtifactand desync the selected context from the content being rendered.Proposed fix
const syncArtifact = (artifact: ArtifactState, messageId: string, threadId: string) => { - if (!currentArtifact.value || validateContext(messageId, threadId)) { + const matchesCurrentArtifact = + validateContext(messageId, threadId) && currentArtifact.value?.id === artifact.id + + if (!currentArtifact.value || matchesCurrentArtifact) { currentArtifact.value = artifact currentMessageId.value = messageId currentThreadId.value = threadId } }Also applies to: 125-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/stores/artifact.ts` around lines 111 - 113, The current validateContext/syncArtifact logic only compares currentMessageId and currentThreadId and ignores artifact identity, which allows updates for different artifacts in the same message to replace currentArtifact; update validateContext (and the matching logic inside syncArtifact) to also compare artifact.id (and/or currentArtifact.id/currentArtifactId) so that updates are only applied when messageId, threadId, AND artifact.id match the currently selected artifact, ensuring currentArtifact and the sidepanel context keyed by artifactId stay in sync.src/main/presenter/browser/YoBrowserPresenter.ts (1)
327-340:⚠️ Potential issue | 🟠 Major
navigateWindowdoesn't handle embedded state consistently with other window methods.The method only checks
browserWindowsfor the window ID, but other similar methods likefocusWindowandcloseWindowfirst check if the ID matchesembeddedState.id. When called with an embedded window ID,navigateWindowfalls back to callingopenWindow, which indirectly navigates throughensureEmbeddedState. This works but is inefficient and inconsistent with the established pattern.Add the embedded state check at the beginning of
navigateWindowto match the pattern used infocusWindow(line 185) andcloseWindow(line 201):async navigateWindow(windowId: number, url: string, timeoutMs?: number): Promise<void> { + if (this.embeddedState?.id === windowId) { + await this.embeddedState.page.navigate(url, timeoutMs) + this.embeddedState.updatedAt = Date.now() + this.emitWindowUpdated(this.embeddedState) + return + } + const state = this.browserWindows.get(windowId) if (!state) { const created = await this.openWindow(url)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/browser/YoBrowserPresenter.ts` around lines 327 - 340, Add an embedded-state check at the start of navigateWindow to mirror focusWindow/closeWindow: if this.embeddedState exists and windowId === this.embeddedState.id, call await this.ensureEmbeddedState(), then navigate using this.embeddedState.page.navigate(url, timeoutMs), update this.embeddedState.updatedAt = Date.now(), and call this.emitWindowUpdated(this.embeddedState); otherwise continue using the browserWindows lookup and existing logic. Reference: navigateWindow, embeddedState, ensureEmbeddedState, browserWindows, openWindow, emitWindowUpdated.
♻️ Duplicate comments (1)
src/renderer/src/components/sidepanel/WorkspaceViewer.vue (1)
96-132:⚠️ Potential issue | 🟡 MinorKeep the file view mounted while previews load.
The loading branch still cannot render on the initial fetch because both
activeSourceand the outerv-else-ifwait forfilePreview. Selecting a file falls back to the empty state instead of showing the spinner.Also applies to: 185-191
🤖 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 96 - 132, The template currently hides the entire file view unless props.filePreview exists, so the loading spinner (props.loadingFilePreview) never appears on initial fetch; change the outer conditional that uses activeSource === 'file' && props.filePreview to only check activeSource === 'file' so the file pane stays mounted while previews load, then guard individual preview branches (image, binary, previewComponent, fallback) with v-if checks that reference props.filePreview (and its properties) to avoid runtime errors; apply the same change to the other occurrence around lines 185-191, ensuring you rely on activeSource, props.loadingFilePreview, previewBlock and previewComponent to control inner rendering while keeping the container mounted.
🧹 Nitpick comments (3)
test/renderer/stores/sidepanelAndArtifact.test.ts (1)
21-23: ThedefineStoremock may be fragile.The mock returns the setup function directly (
setup()), bypassing Pinia's actual store creation. This works if the artifact store's setup function doesn't rely on Pinia-specific features (like$patch,$reset, etc.). If the store implementation changes to use these features, this test will break silently.Consider using
createTestingPiniafrom@pinia/testingfor more robust store testing, or document why this simplified mock is sufficient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/renderer/stores/sidepanelAndArtifact.test.ts` around lines 21 - 23, The current vi.doMock for 'pinia' returning defineStore as the raw setup function is fragile; replace it with a more realistic test Pinia (e.g., use createTestingPinia from '@pinia/testing' or createPinia and install it) so stores used in tests (the mocked defineStore/setup) get Pinia instance methods like $patch and $reset, or alternatively update the mock of defineStore to return an object that wraps the setup result and exposes $patch/$reset/$state to mirror Pinia behavior; update references to vi.doMock('pinia')/defineStore in the test to use the testing pinia helper or the enhanced mock and add a brief comment explaining why the testing pinia is used.test/main/presenter/YoBrowserPresenter.test.ts (1)
88-88: Consider typingviewConfigsmore strictly.Using
Record<string, any>loses type safety for the webPreferences structure being tested.♻️ Suggested improvement
- const viewConfigs: Array<Record<string, any>> = [] + const viewConfigs: Array<{ webPreferences?: { sandbox?: boolean; devTools?: boolean; session?: unknown } }> = []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/YoBrowserPresenter.test.ts` at line 88, The test's viewConfigs variable is too loosely typed as Array<Record<string, any>>; tighten it by using the concrete shape used by the presenter (e.g. define or import an explicit interface/type that includes webPreferences and its expected fields) and change the declaration of viewConfigs to Array<YourViewConfigType> (or the exact imported type) so the test statically validates the webPreferences structure referenced in the assertions in YoBrowserPresenter.test.ts; update any test data to conform to that type.src/renderer/src/components/sidepanel/BrowserPanel.vue (1)
133-152: Consider adding error handling for navigation failures.The
navigatefunction doesn't handle potential errors fromnavigateWindow. Invalid URLs or network failures could throw, leaving the UI in an inconsistent state.🛡️ Proposed improvement
const navigate = async () => { if (hostWindowId.value == null) { return } const nextUrl = normalizeUrl(urlInput.value) if (!nextUrl) { return } browserWindowId.value = await yoBrowserPresenter.attachEmbeddedToWindow(hostWindowId.value) if (browserWindowId.value == null) { return } - await yoBrowserPresenter.navigateWindow(browserWindowId.value, nextUrl) - await loadState() - await nextTick() - await syncBounds() + try { + await yoBrowserPresenter.navigateWindow(browserWindowId.value, nextUrl) + } catch { + // Navigation failed - URL may be invalid or unreachable + } + await loadState() + await nextTick() + await syncBounds() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/components/sidepanel/BrowserPanel.vue` around lines 133 - 152, The navigate function lacks error handling around the call to yoBrowserPresenter.navigateWindow, so wrap the navigation steps in a try/catch (e.g., around await yoBrowserPresenter.navigateWindow(browserWindowId.value, nextUrl)) to catch navigation failures; on error, log or surface the error (via processLogger/console or a UI error state), reset or clear browserWindowId.value if the attach succeeded but navigation failed to avoid inconsistent state, and move cleanup actions like await loadState() and await syncBounds() into a finally block (or only run them on success as appropriate) so state is consistent whether navigation succeeds or throws; reference functions/variables: navigate, yoBrowserPresenter.navigateWindow, yoBrowserPresenter.attachEmbeddedToWindow, loadState, syncBounds, browserWindowId, hostWindowId, normalizeUrl, urlInput.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/src/components/sidepanel/BrowserPanel.vue`:
- Line 62: Update the import of BrowserPlaceholder in BrowserPanel.vue to use
the project path alias `@browser` instead of the relative path; locate the
existing import statement that brings in BrowserPlaceholder and replace the
'../../../browser/components/BrowserPlaceholder.vue' path with the `@browser`
alias path to the same component so imports remain consistent with the alias
configuration.
In `@src/renderer/src/components/sidepanel/WorkspaceViewer.vue`:
- Around line 111-116: The template hardcodes the English suffix "bytes" in the
binary preview; replace it with a vue-i18n key from the renderer i18n files so
the suffix is localized. Update the template that renders
props.filePreview.mimeType and the size (where you currently use Math.max(... )
+ " bytes") to call the translator (e.g. $t or useI18n) with a key from
src/renderer/src/i18n (create a key like "size.bytes" if missing) and pass the
numeric value as a param or concatenate via the localized pattern so the file
size label uses i18n instead of the hardcoded "bytes". Ensure you reference the
template block rendering props.filePreview and
props.filePreview.metadata.fileSize in WorkspaceViewer.vue.
In `@src/renderer/src/stores/ui/sidepanel.ts`:
- Around line 33-45: The clamp uses resolveMaxWidth() which reads
window.innerWidth non-reactively, so normalizedWidth can exceed the new max when
the viewport shrinks; make the sidepanel react to resizes by tracking viewport
width and re-clamping on resize: add a reactive source (e.g., a viewportWidth
ref updated in a window 'resize' listener) used by resolveMaxWidth()/clampWidth,
and on resize call setWidth(clampWidth(width.value)) (or recompute
normalizedWidth) to enforce the new limits; ensure the listener is registered
when the store/component initializes and removed on cleanup to avoid leaks.
---
Outside diff comments:
In `@src/main/presenter/browser/YoBrowserPresenter.ts`:
- Around line 327-340: Add an embedded-state check at the start of
navigateWindow to mirror focusWindow/closeWindow: if this.embeddedState exists
and windowId === this.embeddedState.id, call await this.ensureEmbeddedState(),
then navigate using this.embeddedState.page.navigate(url, timeoutMs), update
this.embeddedState.updatedAt = Date.now(), and call
this.emitWindowUpdated(this.embeddedState); otherwise continue using the
browserWindows lookup and existing logic. Reference: navigateWindow,
embeddedState, ensureEmbeddedState, browserWindows, openWindow,
emitWindowUpdated.
In `@src/renderer/src/stores/artifact.ts`:
- Around line 111-113: The current validateContext/syncArtifact logic only
compares currentMessageId and currentThreadId and ignores artifact identity,
which allows updates for different artifacts in the same message to replace
currentArtifact; update validateContext (and the matching logic inside
syncArtifact) to also compare artifact.id (and/or
currentArtifact.id/currentArtifactId) so that updates are only applied when
messageId, threadId, AND artifact.id match the currently selected artifact,
ensuring currentArtifact and the sidepanel context keyed by artifactId stay in
sync.
---
Duplicate comments:
In `@src/renderer/src/components/sidepanel/WorkspaceViewer.vue`:
- Around line 96-132: The template currently hides the entire file view unless
props.filePreview exists, so the loading spinner (props.loadingFilePreview)
never appears on initial fetch; change the outer conditional that uses
activeSource === 'file' && props.filePreview to only check activeSource ===
'file' so the file pane stays mounted while previews load, then guard individual
preview branches (image, binary, previewComponent, fallback) with v-if checks
that reference props.filePreview (and its properties) to avoid runtime errors;
apply the same change to the other occurrence around lines 185-191, ensuring you
rely on activeSource, props.loadingFilePreview, previewBlock and
previewComponent to control inner rendering while keeping the container mounted.
---
Nitpick comments:
In `@src/renderer/src/components/sidepanel/BrowserPanel.vue`:
- Around line 133-152: The navigate function lacks error handling around the
call to yoBrowserPresenter.navigateWindow, so wrap the navigation steps in a
try/catch (e.g., around await
yoBrowserPresenter.navigateWindow(browserWindowId.value, nextUrl)) to catch
navigation failures; on error, log or surface the error (via
processLogger/console or a UI error state), reset or clear browserWindowId.value
if the attach succeeded but navigation failed to avoid inconsistent state, and
move cleanup actions like await loadState() and await syncBounds() into a
finally block (or only run them on success as appropriate) so state is
consistent whether navigation succeeds or throws; reference functions/variables:
navigate, yoBrowserPresenter.navigateWindow,
yoBrowserPresenter.attachEmbeddedToWindow, loadState, syncBounds,
browserWindowId, hostWindowId, normalizeUrl, urlInput.
In `@test/main/presenter/YoBrowserPresenter.test.ts`:
- Line 88: The test's viewConfigs variable is too loosely typed as
Array<Record<string, any>>; tighten it by using the concrete shape used by the
presenter (e.g. define or import an explicit interface/type that includes
webPreferences and its expected fields) and change the declaration of
viewConfigs to Array<YourViewConfigType> (or the exact imported type) so the
test statically validates the webPreferences structure referenced in the
assertions in YoBrowserPresenter.test.ts; update any test data to conform to
that type.
In `@test/renderer/stores/sidepanelAndArtifact.test.ts`:
- Around line 21-23: The current vi.doMock for 'pinia' returning defineStore as
the raw setup function is fragile; replace it with a more realistic test Pinia
(e.g., use createTestingPinia from '@pinia/testing' or createPinia and install
it) so stores used in tests (the mocked defineStore/setup) get Pinia instance
methods like $patch and $reset, or alternatively update the mock of defineStore
to return an object that wraps the setup result and exposes $patch/$reset/$state
to mirror Pinia behavior; update references to vi.doMock('pinia')/defineStore in
the test to use the testing pinia helper or the enhanced mock and add a brief
comment explaining why the testing pinia is used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c8155ea7-eb8a-4373-ad99-1532e6f0a480
📒 Files selected for processing (22)
src/main/presenter/browser/YoBrowserPresenter.tssrc/renderer/src/components/sidepanel/BrowserPanel.vuesrc/renderer/src/components/sidepanel/WorkspaceViewer.vuesrc/renderer/src/i18n/da-DK/common.jsonsrc/renderer/src/i18n/en-US/common.jsonsrc/renderer/src/i18n/fa-IR/common.jsonsrc/renderer/src/i18n/fr-FR/common.jsonsrc/renderer/src/i18n/he-IL/common.jsonsrc/renderer/src/i18n/ja-JP/common.jsonsrc/renderer/src/i18n/ko-KR/common.jsonsrc/renderer/src/i18n/pt-BR/common.jsonsrc/renderer/src/i18n/ru-RU/common.jsonsrc/renderer/src/i18n/zh-CN/common.jsonsrc/renderer/src/i18n/zh-HK/common.jsonsrc/renderer/src/i18n/zh-TW/common.jsonsrc/renderer/src/stores/artifact.tssrc/renderer/src/stores/ui/sidepanel.tstest/main/presenter/YoBrowserPresenter.test.tstest/renderer/components/BrowserPanel.test.tstest/renderer/components/WorkspaceViewer.test.tstest/renderer/stores/sidepanel.test.tstest/renderer/stores/sidepanelAndArtifact.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/renderer/src/i18n/fr-FR/common.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/renderer/src/components/sidepanel/WorkspaceViewer.vue (1)
96-137:⚠️ Potential issue | 🟡 MinorKeep the file pane mounted while the preview request is in flight.
selectedFilePathcan be set beforefilePreviewarrives, but this code dropsactiveSourceback tonulland also hides the entire file branch behindprops.filePreview. The loading state on Lines 97-100 therefore never renders for a newly selected file, and the panel falls back to the empty state or a blank body instead.Suggested fix
- <div v-else-if="activeSource === 'file' && props.filePreview" class="h-full overflow-auto"> + <div v-else-if="activeSource === 'file'" class="h-full overflow-auto"> <template v-if="props.loadingFilePreview"> <div class="flex h-full items-center justify-center text-sm text-muted-foreground"> {{ t('chat.workspace.files.loading') }} </div> </template> - <template v-else-if="props.filePreview.kind === 'image'"> + <template v-else-if="props.filePreview?.kind === 'image'"> ... - <template v-else-if="props.filePreview.kind === 'binary'"> + <template v-else-if="props.filePreview?.kind === 'binary'"> ... - <template v-else-if="previewBlock && previewComponent"> + <template v-else-if="props.filePreview && previewBlock && previewComponent"> ... - <pre class="whitespace-pre-wrap break-words p-4 text-sm leading-6">{{ - props.filePreview.content - }}</pre> + <pre + v-if="props.filePreview" + class="whitespace-pre-wrap break-words p-4 text-sm leading-6" + >{{ props.filePreview.content }}</pre> </template> </div> @@ if (sessionState.value.selectedFilePath) { - return props.filePreview ? 'file' : null + return 'file' }Also applies to: 189-195
🤖 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 96 - 137, The file pane currently unmounts when props.filePreview is null so selecting a file before its preview arrives hides the loading UI; change the outer condition that controls the pane (currently "activeSource === 'file' && props.filePreview") to only check "activeSource === 'file'" so the pane stays mounted while awaiting props.filePreview, keep the inner loading branch using props.loadingFilePreview to show the spinner, and ensure the existing branches that rely on props.filePreview (image/binary/text/previewComponent) still guard on props.filePreview being present; apply the same change to the other file-pane block referenced (lines ~189-195) and keep references to activeSource, props.filePreview, props.loadingFilePreview, previewBlock and previewComponent to locate the places to update.
🧹 Nitpick comments (1)
src/renderer/src/components/sidepanel/BrowserPanel.vue (1)
182-187: Consider debouncing rapid state changes.When the sidepanel open/activeTab state changes rapidly (e.g., during animations or quick tab switches), multiple
syncBounds()calls may queue up. While not critical, a short debounce could reduce unnecessary IPC overhead.♻️ Optional: Debounce using VueUse
+import { useDebounceFn } from '@vueuse/core' +const debouncedSyncBounds = useDebounceFn(syncBounds, 16) watch( () => [sidepanelStore.open, sidepanelStore.activeTab] as const, () => { - void syncBounds() + void debouncedSyncBounds() } )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/components/sidepanel/BrowserPanel.vue` around lines 182 - 187, The watcher on [sidepanelStore.open, sidepanelStore.activeTab] calls syncBounds() directly and can trigger many IPC calls; create a debounced wrapper (e.g., debouncedSyncBounds) around syncBounds (using debounceFn from `@vueuse/core` or lodash.debounce) and call that from the watcher instead of syncBounds, ensuring you configure the debounce delay (e.g., 100–200ms) and desired edge behavior (leading/trailing) so rapid state changes coalesce while still updating bounds timely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/src/components/sidepanel/BrowserPanel.vue`:
- Around line 189-200: The call to getWindowId is not protected if window.api
itself is undefined; change the assignment in onMounted so it uses optional
chaining on window.api too — e.g. replace hostWindowId.value =
window.api.getWindowId?.() ?? null with hostWindowId.value =
window.api?.getWindowId?.() ?? null (update the onMounted block where
hostWindowId is set) so the code safely handles a missing preload API.
In `@src/renderer/src/components/sidepanel/WorkspaceViewer.vue`:
- Around line 34-42: The Open File button currently renders whenever
props.filePreview is truthy, which lets it appear even when the viewer shows a
diff or artifact; update the render condition to require the file viewer be
active as well by gating the Button on both props.filePreview and activeSource
=== 'file' (or the component's equivalent reactive property) so the Button (the
element using `@click`="handleOpenFile") only appears when the file viewer is the
active source.
- Around line 231-236: Remove the three unnecessary "as never" casts when
calling useArtifactCodeEditor; call useArtifactCodeEditor(codeArtifact,
codeEditorRef, computed(() => viewMode.value === 'preview'), computed(() =>
true)) so the actual Ref/ComputedRef types flow through, or alternatively adjust
the useArtifactCodeEditor parameter types to accept Ref | ComputedRef for the
relevant arguments; target the call site where useArtifactCodeEditor is invoked
and the composable signature to ensure type compatibility without casting.
In `@src/renderer/src/i18n/da-DK/common.json`:
- Around line 91-98: The browser translations are inconsistent: replace the
English values for keys "back", "forward", "reload", and "addressPlaceholder"
with Danish equivalents (e.g., "back" -> "Tilbage", "forward" -> "Frem",
"reload" -> "Genindlæs", "addressPlaceholder" -> "Indtast en URL") and correct
the "name" value (replace "Yo browser" with a proper product name such as
"Browser" or a localized variant) in the same browser JSON object; update the
entries "back", "forward", "reload", "addressPlaceholder", and "name" so they
match the rest of the Danish translations (leave other keys like "addressLabel",
"enterUrlDescription", "enterUrlToStart" intact unless further localization is
desired).
In `@src/renderer/src/i18n/zh-HK/common.json`:
- Line 94: Pick one term and make it consistent: replace the other occurrence of
the address-field string so both use the same Chinese phrase; update the
"addressLabel" entry (currently "網址列") and any other key whose value is "地址欄" so
they all use the chosen term (e.g., change "地址欄" → "網址列"), ensuring all browser
strings in this locale are identical.
---
Duplicate comments:
In `@src/renderer/src/components/sidepanel/WorkspaceViewer.vue`:
- Around line 96-137: The file pane currently unmounts when props.filePreview is
null so selecting a file before its preview arrives hides the loading UI; change
the outer condition that controls the pane (currently "activeSource === 'file'
&& props.filePreview") to only check "activeSource === 'file'" so the pane stays
mounted while awaiting props.filePreview, keep the inner loading branch using
props.loadingFilePreview to show the spinner, and ensure the existing branches
that rely on props.filePreview (image/binary/text/previewComponent) still guard
on props.filePreview being present; apply the same change to the other file-pane
block referenced (lines ~189-195) and keep references to activeSource,
props.filePreview, props.loadingFilePreview, previewBlock and previewComponent
to locate the places to update.
---
Nitpick comments:
In `@src/renderer/src/components/sidepanel/BrowserPanel.vue`:
- Around line 182-187: The watcher on [sidepanelStore.open,
sidepanelStore.activeTab] calls syncBounds() directly and can trigger many IPC
calls; create a debounced wrapper (e.g., debouncedSyncBounds) around syncBounds
(using debounceFn from `@vueuse/core` or lodash.debounce) and call that from the
watcher instead of syncBounds, ensuring you configure the debounce delay (e.g.,
100–200ms) and desired edge behavior (leading/trailing) so rapid state changes
coalesce while still updating bounds timely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd68ae68-21de-4a11-8593-796e154db732
📒 Files selected for processing (16)
src/renderer/src/components/sidepanel/BrowserPanel.vuesrc/renderer/src/components/sidepanel/WorkspaceViewer.vuesrc/renderer/src/i18n/da-DK/common.jsonsrc/renderer/src/i18n/en-US/common.jsonsrc/renderer/src/i18n/fa-IR/common.jsonsrc/renderer/src/i18n/fr-FR/common.jsonsrc/renderer/src/i18n/he-IL/common.jsonsrc/renderer/src/i18n/ja-JP/common.jsonsrc/renderer/src/i18n/ko-KR/common.jsonsrc/renderer/src/i18n/pt-BR/common.jsonsrc/renderer/src/i18n/ru-RU/common.jsonsrc/renderer/src/i18n/zh-CN/common.jsonsrc/renderer/src/i18n/zh-HK/common.jsonsrc/renderer/src/i18n/zh-TW/common.jsonsrc/renderer/src/stores/ui/sidepanel.tstest/renderer/stores/sidepanel.test.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- src/renderer/src/i18n/ja-JP/common.json
- test/renderer/stores/sidepanel.test.ts
- src/renderer/src/i18n/en-US/common.json
- src/renderer/src/i18n/pt-BR/common.json
- src/renderer/src/i18n/ru-RU/common.json
- src/renderer/src/i18n/he-IL/common.json
- src/renderer/src/i18n/fr-FR/common.json
- src/renderer/src/stores/ui/sidepanel.ts
- src/renderer/src/i18n/zh-CN/common.json
- src/renderer/src/i18n/fa-IR/common.json
- src/renderer/src/i18n/zh-TW/common.json
Comment on lines
+189
to
+200
| onMounted(async () => { | ||
| hostWindowId.value = window.api.getWindowId?.() ?? null | ||
| await loadState() | ||
| await nextTick() | ||
| await syncBounds() | ||
|
|
||
| window.electron.ipcRenderer.on(YO_BROWSER_EVENTS.WINDOW_CREATED, handleBrowserEvent) | ||
| window.electron.ipcRenderer.on(YO_BROWSER_EVENTS.WINDOW_UPDATED, handleBrowserEvent) | ||
| window.electron.ipcRenderer.on(YO_BROWSER_EVENTS.WINDOW_CLOSED, handleBrowserEvent) | ||
| window.electron.ipcRenderer.on(YO_BROWSER_EVENTS.WINDOW_FOCUSED, handleBrowserEvent) | ||
| window.electron.ipcRenderer.on(YO_BROWSER_EVENTS.WINDOW_VISIBILITY_CHANGED, handleBrowserEvent) | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how window.api is accessed elsewhere in the codebase for consistency
rg -n "window\.api\." src/renderer --type vue --type ts -C 1 | head -40Repository: ThinkInAIXYZ/deepchat
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Search for window.api usage patterns - more broadly
rg "window\.api" src/renderer --type ts -C 2 | head -50Repository: ThinkInAIXYZ/deepchat
Length of output: 3434
🏁 Script executed:
#!/bin/bash
# Read the BrowserPanel.vue file to see the context
cat -n src/renderer/src/components/sidepanel/BrowserPanel.vue | sed -n '180,210p'Repository: ThinkInAIXYZ/deepchat
Length of output: 1569
🏁 Script executed:
#!/bin/bash
# Check all window.api usages in BrowserPanel.vue
rg "window\.api" src/renderer/src/components/sidepanel/BrowserPanel.vue -n
# Also check for window.api type definition in the renderer
rg "window\.api" src/renderer/src --type ts -A 2 | grep -E "(declare|interface|type|window\.api)" | head -30Repository: ThinkInAIXYZ/deepchat
Length of output: 1678
🏁 Script executed:
#!/bin/bash
# Look for type definitions of window.api
find src/renderer -name "*.ts" -o -name "*.tsx" | xargs grep -l "declare.*window\.api\|interface.*api" | head -5
# Also check env.d.ts or similar declaration files
find src/renderer -name "*.d.ts" -o -name "env.d.ts" | xargs cat 2>/dev/nullRepository: ThinkInAIXYZ/deepchat
Length of output: 1685
🏁 Script executed:
#!/bin/bash
# Search more broadly for window.api declaration
rg "window\.api" src/renderer --type ts -B 5 | grep -E "(declare|interface|type)" | head -20
# Also look in src/main or preload files
find src -name "*preload*" -o -name "*main*" | head -10Repository: ThinkInAIXYZ/deepchat
Length of output: 292
🏁 Script executed:
#!/bin/bash
# Check the preload directory
ls -la src/preload/
# Read preload files to see how window.api is exposed
cat src/preload/*.ts 2>/dev/null | head -100Repository: ThinkInAIXYZ/deepchat
Length of output: 3417
🏁 Script executed:
#!/bin/bash
# Read the complete preload type definition
cat src/preload/index.d.ts
# Read the complete preload index
cat src/preload/index.tsRepository: ThinkInAIXYZ/deepchat
Length of output: 4245
🏁 Script executed:
#!/bin/bash
# Check if getWindowId?.() is the only optional chaining on window.api in the file
rg "window\." src/renderer/src/components/sidepanel/BrowserPanel.vue -nRepository: ThinkInAIXYZ/deepchat
Length of output: 1052
🏁 Script executed:
#!/bin/bash
# Double-check: are there other instances of optional chaining on window.api methods elsewhere?
rg "window\.api.*\?" src/renderer --type ts -nRepository: ThinkInAIXYZ/deepchat
Length of output: 323
🏁 Script executed:
#!/bin/bash
# Check if window.api can be undefined in any legitimate scenario
# Look for error handling around window.api access
rg "window\.api" src/renderer --type ts -B 3 -A 1 | grep -E "(try|catch|if.*window|undefined)" | head -20Repository: ThinkInAIXYZ/deepchat
Length of output: 337
Add optional chaining on window.api to guard against undefined preload API.
The optional chaining on getWindowId?.() only protects against the method being undefined; it does not protect against window.api itself being undefined. If the preload script fails to expose the API, accessing window.api.getWindowId will throw before the optional chaining applies.
🛡️ Proposed fix
onMounted(async () => {
- hostWindowId.value = window.api.getWindowId?.() ?? null
+ hostWindowId.value = window.api?.getWindowId?.() ?? null
await loadState()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| onMounted(async () => { | |
| hostWindowId.value = window.api.getWindowId?.() ?? null | |
| await loadState() | |
| await nextTick() | |
| await syncBounds() | |
| window.electron.ipcRenderer.on(YO_BROWSER_EVENTS.WINDOW_CREATED, handleBrowserEvent) | |
| window.electron.ipcRenderer.on(YO_BROWSER_EVENTS.WINDOW_UPDATED, handleBrowserEvent) | |
| window.electron.ipcRenderer.on(YO_BROWSER_EVENTS.WINDOW_CLOSED, handleBrowserEvent) | |
| window.electron.ipcRenderer.on(YO_BROWSER_EVENTS.WINDOW_FOCUSED, handleBrowserEvent) | |
| window.electron.ipcRenderer.on(YO_BROWSER_EVENTS.WINDOW_VISIBILITY_CHANGED, handleBrowserEvent) | |
| }) | |
| onMounted(async () => { | |
| hostWindowId.value = window.api?.getWindowId?.() ?? null | |
| await loadState() | |
| await nextTick() | |
| await syncBounds() | |
| window.electron.ipcRenderer.on(YO_BROWSER_EVENTS.WINDOW_CREATED, handleBrowserEvent) | |
| window.electron.ipcRenderer.on(YO_BROWSER_EVENTS.WINDOW_UPDATED, handleBrowserEvent) | |
| window.electron.ipcRenderer.on(YO_BROWSER_EVENTS.WINDOW_CLOSED, handleBrowserEvent) | |
| window.electron.ipcRenderer.on(YO_BROWSER_EVENTS.WINDOW_FOCUSED, handleBrowserEvent) | |
| window.electron.ipcRenderer.on(YO_BROWSER_EVENTS.WINDOW_VISIBILITY_CHANGED, handleBrowserEvent) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/src/components/sidepanel/BrowserPanel.vue` around lines 189 -
200, The call to getWindowId is not protected if window.api itself is undefined;
change the assignment in onMounted so it uses optional chaining on window.api
too — e.g. replace hostWindowId.value = window.api.getWindowId?.() ?? null with
hostWindowId.value = window.api?.getWindowId?.() ?? null (update the onMounted
block where hostWindowId is set) so the code safely handles a missing preload
API.
Comment on lines
+34
to
+42
| <Button | ||
| v-if="props.filePreview" | ||
| variant="outline" | ||
| size="sm" | ||
| class="h-7 text-xs" | ||
| @click="handleOpenFile" | ||
| > | ||
| {{ t('chat.workspace.files.contextMenu.openFile') }} | ||
| </Button> |
Contributor
There was a problem hiding this comment.
Hide the Open File action unless the file viewer is active.
This button is keyed only off props.filePreview, so it can stay visible while the body is showing a diff or artifact and still open the last selected file. Gating it on activeSource === 'file' keeps the header action aligned with the current viewer state.
🤖 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 34 -
42, The Open File button currently renders whenever props.filePreview is truthy,
which lets it appear even when the viewer shows a diff or artifact; update the
render condition to require the file viewer be active as well by gating the
Button on both props.filePreview and activeSource === 'file' (or the component's
equivalent reactive property) so the Button (the element using
`@click`="handleOpenFile") only appears when the file viewer is the active source.
Comment on lines
+231
to
+236
| useArtifactCodeEditor( | ||
| codeArtifact as never, | ||
| codeEditorRef, | ||
| computed(() => viewMode.value === 'preview') as never, | ||
| computed(() => true) as never | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd useArtifactCodeEditor.ts src/renderer/src --exec sed -n '1,220p' {}
echo '--- call sites ---'
rg -n -C2 'useArtifactCodeEditor\(' src/renderer/srcRepository: ThinkInAIXYZ/deepchat
Length of output: 6015
🏁 Script executed:
# Get full context around the call site in WorkspaceViewer.vue
sed -n '200,260p' src/renderer/src/components/sidepanel/WorkspaceViewer.vue
# Get variable declarations at the top of the file
head -100 src/renderer/src/components/sidepanel/WorkspaceViewer.vue | tail -100Repository: ThinkInAIXYZ/deepchat
Length of output: 5194
🏁 Script executed:
# Find the full script section to see all variable declarations and imports
sed -n '1,240p' src/renderer/src/components/sidepanel/WorkspaceViewer.vue | grep -E "^(import|const|ref|computed|let|var|declare)" -A 2
# Also search for codeEditorRef declaration
rg -n "codeEditorRef" src/renderer/src/components/sidepanel/WorkspaceViewer.vue -B 2 -A 2
# Check the actual type of ArtifactState to ensure we understand the full picture
rg -n "export.*type ArtifactState|interface ArtifactState" src/renderer/srcRepository: ThinkInAIXYZ/deepchat
Length of output: 2419
🏁 Script executed:
# Verify Vue 3 type hierarchy by checking the node_modules or type definitions
find . -name "*.d.ts" -path "*/vue/*" | head -5 | xargs grep -l "ComputedRef" | head -1 | xargs sed -n '1,100p'
# Alternative: search for ComputedRef type definition in the codebase's type references
rg "interface ComputedRef|type ComputedRef" --type ts --type tsx -A 5 | head -20Repository: ThinkInAIXYZ/deepchat
Length of output: 92
🏁 Script executed:
# Try a simpler approach - just search TypeScript files
rg "ComputedRef" --type ts -A 3 | head -30
# Or check if there's a tsconfig that might help us understand the type checking context
cat tsconfig.json 2>/dev/null || echo "No tsconfig.json found"Repository: ThinkInAIXYZ/deepchat
Length of output: 2383
🏁 Script executed:
# Check if ComputedRef values are passed to Ref-typed parameters elsewhere without casting
rg "useArtifactCodeEditor|computed\(\)" src/renderer/src/composables --type ts -A 1 -B 1 | grep -v "as never" | head -40
# Search for other function calls that accept Ref parameters with ComputedRef values
rg "function.*Ref<" src/renderer/src/composables --type ts -A 3 | head -30
# Check if there are similar patterns elsewhere in the codebase
rg "computed\(.*\) as never" src/renderer/src --type ts -C 1Repository: ThinkInAIXYZ/deepchat
Length of output: 686
🏁 Script executed:
# Search for other instances of "as never" pattern in the repo to see if this is a common antipattern
rg "as never" src/renderer/src --type ts --type vue -C 1 | head -50
# Also check if there's documentation or comments explaining why these casts exist
rg -B 5 "useArtifactCodeEditor\(" src/renderer/src/components/sidepanel/WorkspaceViewer.vue | head -20Repository: ThinkInAIXYZ/deepchat
Length of output: 191
🏁 Script executed:
# Let's check git history to see if these casts were added for a reason
git log -p --all -S "as never" -- src/renderer/src/components/sidepanel/WorkspaceViewer.vue | head -100
# Also check the full WorkspaceViewer.vue for any comments about this pattern
sed -n '220,245p' src/renderer/src/components/sidepanel/WorkspaceViewer.vueRepository: ThinkInAIXYZ/deepchat
Length of output: 628
Remove unnecessary as never casts—they suppress type checking at the composable boundary.
The three as never casts mask the real types here. In Vue 3, ComputedRef<T> properly extends Ref<T>, so all three parameters are already type-compatible without casting. This is evident from codeEditorRef (line 233) being passed without a cast. The casts allow signature drift to compile silently and break only at runtime. Either remove the casts entirely or, if there's a real type mismatch preventing removal, loosen the composable's parameter types to accept Ref | ComputedRef explicitly.
🤖 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 231 -
236, Remove the three unnecessary "as never" casts when calling
useArtifactCodeEditor; call useArtifactCodeEditor(codeArtifact, codeEditorRef,
computed(() => viewMode.value === 'preview'), computed(() => true)) so the
actual Ref/ComputedRef types flow through, or alternatively adjust the
useArtifactCodeEditor parameter types to accept Ref | ComputedRef for the
relevant arguments; target the call site where useArtifactCodeEditor is invoked
and the composable signature to ensure type compatibility without casting.
Comment on lines
91
to
98
| "back": "Back", | ||
| "forward": "Forward", | ||
| "reload": "Reload", | ||
| "addressLabel": "Adresselinje", | ||
| "addressPlaceholder": "Enter a URL", | ||
| "enterUrlDescription": "Indtast URL'en i adresselinjen ovenfor for at få adgang til webstedet", | ||
| "enterUrlToStart": "Indtast URL'en for at begynde at besøge", | ||
| "name": "Yo browser" |
Contributor
There was a problem hiding this comment.
Inconsistent translations and potential typo in the browser object.
Several issues in the existing browser object that may warrant attention:
- Untranslated keys:
back,forward,reload(lines 91-93), andaddressPlaceholder(line 95) contain English text instead of Danish translations. - Likely typo:
"name": "Yo browser"on line 98 appears to be a placeholder or typo — should this be"Browser"or a Danish equivalent?
These are pre-existing issues, but since this PR touches the browser object, consider fixing them for consistency.
🌐 Suggested Danish translations
"browser": {
- "back": "Back",
- "forward": "Forward",
- "reload": "Reload",
+ "back": "Tilbage",
+ "forward": "Frem",
+ "reload": "Genindlæs",
"addressLabel": "Adresselinje",
- "addressPlaceholder": "Enter a URL",
+ "addressPlaceholder": "Indtast en URL",
"enterUrlDescription": "Indtast URL'en i adresselinjen ovenfor for at få adgang til webstedet",
"enterUrlToStart": "Indtast URL'en for at begynde at besøge",
- "name": "Yo browser"
+ "name": "Browser"
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/src/i18n/da-DK/common.json` around lines 91 - 98, The browser
translations are inconsistent: replace the English values for keys "back",
"forward", "reload", and "addressPlaceholder" with Danish equivalents (e.g.,
"back" -> "Tilbage", "forward" -> "Frem", "reload" -> "Genindlæs",
"addressPlaceholder" -> "Indtast en URL") and correct the "name" value (replace
"Yo browser" with a proper product name such as "Browser" or a localized
variant) in the same browser JSON object; update the entries "back", "forward",
"reload", "addressPlaceholder", and "name" so they match the rest of the Danish
translations (leave other keys like "addressLabel", "enterUrlDescription",
"enterUrlToStart" intact unless further localization is desired).
| "back": "後退", | ||
| "forward": "前進", | ||
| "reload": "重新載入", | ||
| "addressLabel": "網址列", |
Contributor
There was a problem hiding this comment.
Standardize the address-field term in this browser copy.
Line 94 uses 網址列, while Line 96 still says 地址欄. Mixing both for the same control reads inconsistent in zh-HK; please pick one term and use it across these browser strings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/src/i18n/zh-HK/common.json` at line 94, Pick one term and make
it consistent: replace the other occurrence of the address-field string so both
use the same Chinese phrase; update the "addressLabel" entry (currently "網址列")
and any other key whose value is "地址欄" so they all use the chosen term (e.g.,
change "地址欄" → "網址列"), ensuring all browser strings in this locale are
identical.
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
New Features
Removed Features