Conversation
Analyzed last 100+ pull requests to identify best formatting patterns and create a comprehensive template that covers: - Clear structure with Summary → Why → How → Changes flow - Performance impact tables with measurable metrics - Configuration examples for new features - Breaking change documentation and migration guides - Testing checklists and environment details - Design decisions and alternatives considered - Security and rollback planning sections - Review checklist aligned with project standards Template captures patterns from high-quality PRs while remaining flexible for different types of changes.
Add clickable file links for inline markdown path references and edit/write/apply_patch file entries so desktop and web users can open changed files directly from the timeline.
Introduce a new session-modal package and desktop tauri dev target so cross-project sessions can be reviewed separately while still deep-linking into the main app.
Add to launch the desktop app via deep link and normalize worktree paths when opening projects so trailing slash variants do not create duplicate project entries.
Add normalizeWorktree tests for trailing separators and root paths, harden normalizeWorktree root handling, and rename the desktop CLI command module from open.ts to desktop.ts for command/file consistency.
# Conflicts: # packages/opencode/src/config/config.ts
# Conflicts: # .github/pull_request_template.md
# Conflicts: # packages/desktop/src/menu.ts
The focus effect only triggered when terminal.active() changed, not when the panel opened. This meant if a terminal was already active (restored from previous session), toggling the panel open wouldn't shift focus from the chat input to the terminal. Added a new effect that watches panel.open() and focuses the active terminal whenever the panel transitions from closed to open.
The terminal panel focus effect should only run for an actual closed to open toggle. Skip the initial opened state so session load and desktop media query changes do not steal focus from the composer.
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
📝 WalkthroughWalkthroughAdds desktop-first project workflows: Git clone support (including WSL), default clone directory management, a project-open dialog with URL parsing, session selection/search dialogs, model favorites/quick slots, rich clipboard copy, file-link handling, and related UI, platform, and backend bindings. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User
participant Dialog as DialogOpenProject
participant Platform as PlatformImpl
participant Host as DesktopHost
participant CloneUI as CloneProgress
User->>Dialog: open dialog
Dialog->>Platform: getDefaultCloneDirectory()
Platform->>Host: invoke get_default_clone_directory
Host-->>Platform: path | null
Platform-->>Dialog: default path
User->>Dialog: enter repo input (github:owner/repo)
Dialog->>Dialog: resolveCloneRepositoryUrl -> https://.../.git
Dialog->>Dialog: suggestCloneTargetPath
Dialog-->>User: show suggested target
User->>Dialog: click Clone
Dialog->>CloneUI: show busy
Dialog->>Platform: cloneGitRepository(url, directory)
Platform->>Host: invoke clone_git_repository
Host->>Host: perform git/WSL clone
Host-->>Platform: cloned path
Platform-->>Dialog: cloned path
CloneUI->>Dialog: hide busy
Dialog->>User: close & trigger onSelect(cloned path)
sequenceDiagram
autonumber
participant User as User
participant SessionDlg as DialogSelectSession
participant SDK as ServerSDK
participant Data as DataLayer
participant UI as SessionList
User->>SessionDlg: open dialog
SessionDlg->>SDK: fetch sessions
SDK-->>SessionDlg: sessions + metadata
SessionDlg->>Data: normalize & rank sessions
Data-->>SessionDlg: ranked list
SessionDlg->>UI: render sessions
User->>UI: type query
UI->>Data: filter by title/desc/dir
Data-->>UI: filtered results
User->>UI: select session
UI->>User: navigate via deep-link (opencode://open-session?directory=...&id=...)
SessionDlg->>User: close dialog
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
|
# Conflicts: # packages/app/src/pages/layout.tsx # packages/app/src/pages/session/message-timeline.tsx # packages/ui/src/components/message-part.tsx
…into local/all-work
…to local/all-work # Conflicts: # packages/app/src/context/local.tsx # packages/app/src/context/models.tsx # packages/app/src/i18n/en.ts # packages/app/src/pages/session/use-session-commands.tsx
…into local/all-work
…into local/all-work # Conflicts: # packages/app/e2e/projects/workspaces.spec.ts # packages/app/src/pages/layout/helpers.ts
…al/all-work # Conflicts: # packages/app/src/pages/session/message-timeline.tsx
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/opencode/src/server/routes/session.ts (1)
264-289:⚠️ Potential issue | 🟡 MinorReject
time.updatedunlesstime.archivedis present.This schema accepts
{ "time": { "updated": ... } }, but Line 284 only applies the update whenarchivedexists. That means a valid request can be silently ignored. Either makeupdatedconditional onarchived, or add a separate handler path forupdated-only payloads.Suggested fix
validator( "json", z.object({ title: z.string().optional(), time: z .object({ archived: z.number().nullable().optional(), updated: z.number().optional(), }) + .refine((value) => value.archived !== undefined || value.updated === undefined, { + message: "`time.updated` requires `time.archived`", + path: ["updated"], + }) .optional(), }), ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/server/routes/session.ts` around lines 264 - 289, The time object schema currently allows time.updated without time.archived, which leads to requests being accepted but ignored; update the validator for the "time" z.object to enforce that if time.updated is present then time.archived must also be present (e.g., use .refine or .superRefine on the z.object to reject when updated is set but archived is missing), so the route handler logic around Session.setArchived({ sessionID, time: updates.time.archived ?? null, updated: updates.time.updated }) receives a consistent payload and no valid-but-ignored requests are allowed.packages/app/src/context/server.tsx (1)
251-257:⚠️ Potential issue | 🟠 MajorPersist the normalized worktree, or normalize every lookup.
open()dedupes onnormalizeWorktree(...), but Line 257 still stores the rawdirectory. The rest of this API comparesx.worktree === directory, soopen("/repo/")followed byclose("/repo")orcollapse("/repo")won't round-trip.Also applies to: 259-292
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/src/context/server.tsx` around lines 251 - 257, The open() implementation dedupes using normalizeWorktree(...) but still stores the raw directory string, causing mismatches on later lookups; update open() to persist the normalized path (store worktree: normalizeWorktree(directory)) so stored entries match subsequent comparisons, and apply the same change to the sibling functions that modify project entries (the routines around lines 259-292 that use collapse/close semantics and setStore on store.projects) so every insertion/update uses normalizeWorktree(...) for the worktree property and all comparisons use normalizeWorktree(...) consistently.packages/opencode/src/config/config.ts (1)
153-167:⚠️ Potential issue | 🟠 MajorFresh installs won't contribute package agents until the next config reload.
needsInstall()/installDependencies()is only queued intodeps, but bothloadPackageAgents()calls run immediately against the pre-install filesystem. Becausestate()memoizesresult.agent,Config.waitForDependencies()never re-scans after the install completes, so newly added package agents stay invisible for the rest of the instance.Also applies to: 218-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/config/config.ts` around lines 153 - 167, The package-agent scan runs before queued installs finish, so newly installed package agents never get merged into result.agent (state() memoizes it and Config.waitForDependencies() won't re-scan); fix by ensuring installs complete before calling loadPackageAgents—either include the loadPackageAgents call inside the same async job you push to deps (the iife that calls needsInstall/installDependencies) or await completion of those deps (e.g., Promise.all over the pushed async tasks) before invoking loadPackageAgents and merging into result.agent; update both occurrences where loadPackageAgents is called so it runs after installDependencies has finished.packages/app/src/context/models.tsx (1)
153-155:⚠️ Potential issue | 🟠 MajorClear quick slots when a model is hidden.
setVisibility(..., false)can hide a model that is still stored instore.quick.a/b. That leaves quick-switch state pointing at a disabled model, and the quick selector no longer contains its current value. Clear affected slots here, or refuse the hide.🧹 Suggested fix
const setVisibility = (model: ModelKey, state: boolean) => { + if (!state) { + if (same(store.quick.a, model)) setStore("quick", "a", undefined) + if (same(store.quick.b, model)) setStore("quick", "b", undefined) + } update(model, state ? "show" : "hide") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/src/context/models.tsx` around lines 153 - 155, The setVisibility function currently calls update(model, state ? "show" : "hide") but doesn’t clear quick-switch slots that reference a model being hidden; modify setVisibility so that when state is false it checks store.quick.a and store.quick.b for the given ModelKey and clears (or resets) any slot that equals that model before/after calling update (or alternatively reject the hide if you prefer) — update the logic in setVisibility and use the store.quick.a / store.quick.b setters to remove the hidden model from quick slots.
🟠 Major comments (23)
packages/util/src/session-transcript.ts-48-57 (1)
48-57:⚠️ Potential issue | 🟠 MajorEscape code fences before embedding tool payloads.
If a tool input/output/error contains triple backticks, this markdown closes the fence early and corrupts the rest of the transcript. Please wrap these sections with a helper that picks a fence longer than any fence present in the payload.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/util/src/session-transcript.ts` around lines 48 - 57, The current transcript builder in session-transcript.ts constructs markdown code fences directly from part.state.input/output/error which breaks if those strings contain ```; modify the logic around the input, output, and error blocks (the variables input, output, error built from part.state) to wrap payloads with a dynamic fence: scan the payload for runs of backticks, pick a fence of backticks longer than any run found (e.g., repeat '`' maxRun+3), and use that fence instead of the fixed ``` so embedded backticks won’t close the fence; update the return assembly (the `**Tool: ${part.tool}**` block) to use this fence helper for all three sections.packages/util/src/session-transcript.ts-48-56 (1)
48-56:⚠️ Potential issue | 🟠 MajorDon't drop falsy tool payloads.
false,0,null, and""are all valid serialized values here, but these truthiness checks suppress them and make the copied transcript lossy. Check forundefinedexplicitly instead.🩹 Proposed fix
- const input = part.state.input + const input = part.state.input !== undefined ? `\n**Input:**\n\`\`\`json\n${JSON.stringify(part.state.input, null, 2)}\n\`\`\`\n` : "" const output = - part.state.status === "completed" && part.state.output + part.state.status === "completed" && part.state.output !== undefined ? `\n**Output:**\n\`\`\`\n${part.state.output}\n\`\`\`\n` : "" const error = - part.state.status === "error" && part.state.error ? `\n**Error:**\n\`\`\`\n${part.state.error}\n\`\`\`\n` : "" + part.state.status === "error" && part.state.error !== undefined ? `\n**Error:**\n\`\`\`\n${part.state.error}\n\`\`\`\n` : ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/util/src/session-transcript.ts` around lines 48 - 56, The transcript builder is dropping valid falsy tool payloads by using truthiness checks; update the conditions in session-transcript.ts to test explicitly for undefined (or null if desired) instead of truthiness: replace checks like `part.state.input ? ... : ""`, `part.state.output` and `part.state.error` with explicit `part.state.input !== undefined`, `part.state.output !== undefined` and `part.state.error !== undefined` (or `!= null` if you want to allow both null/undefined), keeping the status checks (`part.state.status === "completed"` / `"error"`) as-is so false, 0, empty string, and null payloads are preserved in the serialized transcript.script/check-preload.ts-7-8 (1)
7-8:⚠️ Potential issue | 🟠 MajorSkip
node_modulesby path segment, not by"/node_modules/"substring.A root-level path like
node_modules/pkg/bunfig.tomldoes not contain"/node_modules/", so this will start validating vendored configs after dependencies are installed and can fail on third-party preload entries.♻️ Proposed fix
for await (const file of new Bun.Glob("**/bunfig.toml").scan(".")) { - if (file.includes("/node_modules/")) continue + if (file.split(/[\\/]/).includes("node_modules")) continue const text = await Bun.file(file).text()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script/check-preload.ts` around lines 7 - 8, The skip check is using substring matching on file (from new Bun.Glob("**/bunfig.toml").scan(".")) which misses root-level "node_modules" entries; update the condition that currently reads if (file.includes("/node_modules/")) continue to check path segments instead (e.g., split the path into segments and test segment === "node_modules" or use path utilities) so any file whose path contains a node_modules segment is skipped; modify the code around the for-await loop and the variable file to perform a segment-aware check.script/check-preload.ts-10-13 (1)
10-13:⚠️ Potential issue | 🟠 MajorUse a TOML parser instead of regex extraction.
The regex only captures double-quoted strings, missing single-quoted strings which are valid in TOML v1.0 arrays (e.g.,
['string']). Additionally, it matchespreload = [...]inside comments, while a real TOML parser correctly ignores commented lines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script/check-preload.ts` around lines 10 - 13, The current regex-based extraction for the preload variable in script/check-preload.ts misses single-quoted TOML strings and can match commented lines; replace this with a proper TOML parse of the file content and read the preload array from the parsed document (instead of using the preload = [...matchAll...] logic). Use a TOML library (e.g., toml or `@iarna/toml`) to parse the file, validate that the parsed.preload is an array of strings, and map those values into the existing preload variable so comments are ignored and both single- and double-quoted strings are handled correctly.packages/app/src/context/command.tsx-12-12 (1)
12-12:⚠️ Potential issue | 🟠 Major
mod+pwill hijack the browser print shortcut globally.This default now flows into the global
keydownhandler below, which callspreventDefault()for the palette binding. In the web app that replaces the native print shortcut everywhere, including editable fields. Consider keepingmod+shift+pon web or making the default platform-specific.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/src/context/command.tsx` at line 12, DEFAULT_PALETTE_KEYBIND currently set to "mod+p" will override the browser print shortcut; change the constant DEFAULT_PALETTE_KEYBIND in packages/app/src/context/command.tsx to a safer default (e.g., "mod+shift+p") or make it platform-specific (use a small helper to choose "mod+p" for mac and "mod+shift+p" for other platforms). Also update the global keydown handler in the same file to ignore the palette shortcut when the event target is an editable element (input, textarea, or contentEditable) so you don't call preventDefault() for native shortcuts inside editable fields.packages/opencode/src/cli/cmd/tui/component/prompt/autocomplete.tsx-340-343 (1)
340-343:⚠️ Potential issue | 🟠 MajorStrip the leading
@before callinginsertPart().
insertPart()always prefixes the inserted text with"@", so scoped agents still get inserted as@@scope/agent. The display fix only hides the bug in the menu.Proposed fix
- .map( - (agent): AutocompleteOption => ({ - // Avoid double @ for scoped package agents (e.g., `@openpets/coder/pr-review`) - display: agent.name.startsWith("@") ? agent.name : "@" + agent.name, - onSelect: () => { - insertPart(agent.name, { + .map((agent): AutocompleteOption => { + const mention = agent.name.startsWith("@") ? agent.name.slice(1) : agent.name + return { + display: "@" + mention, + onSelect: () => { + insertPart(mention, { type: "agent", name: agent.name, source: { start: 0, end: 0, value: "", }, }) - }, - }), - ) + }, + } + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/cli/cmd/tui/component/prompt/autocomplete.tsx` around lines 340 - 343, The onSelect handler is inserting scoped agents with a double "@". Before calling insertPart(from the onSelect in the autocomplete component), strip a leading "@" from agent.name (e.g., use a sanitizedName = agent.name.replace(/^@/, "") or similar) so insertPart receives the name without "@"; keep the display logic unchanged (display uses agent.name.startsWith("@") ? agent.name : "@" + agent.name) and only pass the sanitizedName to insertPart to avoid producing "@@scope/agent".packages/app/src/pages/session/use-session-commands.tsx-286-300 (1)
286-300:⚠️ Potential issue | 🟠 MajorResolve the new keybind collisions.
message.top/message.bottomusemod+shift+arrowupandmod+shift+arrowdown, and the quick-model commands reuse those exact shortcuts. Only one pair can win in the command registry, so the other commands become unreachable.Also applies to: 313-343
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/src/pages/session/use-session-commands.tsx` around lines 286 - 300, The message.top and message.bottom command keybinds collide with the quick-model commands (they both use "mod+shift+arrowup"/"mod+shift+arrowdown"), so update the bindings to unique, non-conflicting shortcuts: locate the sessionCommand invocations with id "message.top" and "message.bottom" and change their keybind strings to alternative combos (or change the quick-model command keybinds) so only one command wins in the registry; ensure any changed keys are also reflected where those commands are referenced or documented to keep behavior consistent.packages/app/src/components/settings-general.tsx-62-67 (1)
62-67:⚠️ Potential issue | 🟠 MajorDon't resync from the old clone-path value until the refetch finishes.
Line 77 and Line 94 clear
cloneDirtybeforerefetch()completes, so the effect at Line 62 can immediately copy the previous resource value back into the field. That briefly shows the old directory again, and if the refetch fails the UI stays out of sync even thoughsetDefaultCloneDirectorymay already have succeeded.Also applies to: 77-79, 94-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/src/components/settings-general.tsx` around lines 62 - 67, The effect is resyncing clonePath from clonePathResource.latest while refetch() is still underway because store.cloneDirty is cleared before refetch resolves; change the flow in setDefaultCloneDirectory and the places that call refetch() so they await the refetch promise and only clear store.cloneDirty after refetch completes (or after handling the error), or alternatively keep cloneDirty true while clonePathResource is loading and only clear it once clonePathResource.loading is false; update the code paths that currently clear cloneDirty (the blocks around setDefaultCloneDirectory and refetch calls) to clear it in the refetch.then/await resolution (and in the catch) so createEffect (which reads clonePathResource.latest and store.cloneDirty) will not copy the old value back prematurely.packages/app/src/context/server.tsx-37-43 (1)
37-43:⚠️ Potential issue | 🟠 MajorPreserve Windows drive roots during normalization.
normalizeWorktree("C:\\")andnormalizeWorktree("C:/")currently returnC:because Line 39 removes the trailing separator and Line 40 returns early. That is not the same path, so drive-root worktrees won't compare correctly on Windows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/src/context/server.tsx` around lines 37 - 43, In normalizeWorktree, handle Windows drive-root inputs before you strip trailing separators: after computing value = input.trim(), detect a drive-root with a regex like /^[A-Za-z]:[\/\\]*$/ and if it matches return the drive root preserving the original separator (e.g. "C:\" if input used backslash or "C:/" if it used forward slash); otherwise continue with the existing logic that computes next and trims trailing separators (references: function normalizeWorktree, local vars value and next).packages/ui/src/components/markdown-file-ref.ts-13-21 (1)
13-21:⚠️ Potential issue | 🟠 MajorKeep absolute POSIX paths absolute when there is no project root.
With
directory === "", Line 18 treats every/...path as project-relative becauseroot + "/"collapses to/.parseCodeFileRef("/tmp/app.ts", "")currently becomestmp/app.ts, which points at the wrong file and also breaksfile://URLs outside a project root.Proposed guard
function normalizeProjectPath(path: string, directory: string) { if (!path) return path const file = path.replace(/\\/g, "/") const root = directory.replace(/\\/g, "/") if (/^\/[a-zA-Z]:\//.test(file)) return file.slice(1) - if (file.startsWith(root + "/")) return file.slice(root.length + 1) + if (root && file.startsWith(root + "/")) return file.slice(root.length + 1) if (file === root) return "" if (file.startsWith("./")) return file.slice(2) return file }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/markdown-file-ref.ts` around lines 13 - 21, normalizeProjectPath incorrectly treats absolute POSIX paths as project-relative when directory is empty because root + "/" becomes "/" — update normalizeProjectPath to skip the "startsWith(root + '/')" and "file === root" checks when directory (or root) is an empty string so leading '/' is preserved; locate the function normalizeProjectPath and add a guard that only applies the project-root trimming logic if directory (root) is non-empty, leaving absolute paths (e.g., file starting with "/") unchanged when no project root is provided.packages/ui/src/components/message-part.tsx-205-213 (1)
205-213:⚠️ Potential issue | 🟠 MajorDon't strip the leading slash from non-project absolute paths.
relativizeProjectPath()returns the original absolute path when the file is outsidedirectoryor whendirectory === "/". Line 211 then removes the leading slash anyway, so/tmp/file.tsbecomestmp/file.tsand opens the wrong target.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/message-part.tsx` around lines 205 - 213, openProjectFile currently strips a leading slash from the result of relativizeProjectPath unconditionally, which turns non-project absolute paths like /tmp/file.ts into tmp/file.ts; change the logic in openProjectFile so you only remove a leading slash for paths that are not absolute — i.e., call relativizeProjectPath(path, directory), then if the returned string starts with "/" treat it as an absolute path and do not remove the slash, otherwise strip a leading slash before calling openFilePath; reference the openProjectFile function and the relativizeProjectPath call to locate where to adjust this behavior.packages/app/src/components/dialog-select-session.tsx-101-103 (1)
101-103:⚠️ Potential issue | 🟠 MajorUse
Showinstead of standaloneMatch.In SolidJS,
<Match>is only valid as a child of<Switch>. Replace this block with<Show when={showLatest() && latest()}>for proper standalone conditional rendering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/src/components/dialog-select-session.tsx` around lines 101 - 103, The JSX uses <Match> as a standalone conditional which is invalid; replace the <Match when={showLatest() && latest()}>... block with a <Show when={showLatest() && latest()}> wrapper so the content renders correctly outside of a <Switch>. Update the component in dialog-select-session.tsx to use the Show component around the div that references latest(), keeping the same condition (showLatest() && latest()) and preserving the inner div (text-12-regular ... truncate pr-4 mt-0.5) and its content latest().packages/opencode/src/config/config.ts-698-699 (1)
698-699:⚠️ Potential issue | 🟠 MajorA malformed package prompt can abort config loading.
Unlike
loadAgent()andloadCommand(), these package paths callConfigMarkdown.parse()without a local catch. One bad markdown/frontmatter file anywhere undernode_moduleswill rejectloadPackageAgents()and fail startup instead of logging and skipping that package.Also applies to: 724-725
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/config/config.ts` around lines 698 - 699, Wrap calls to ConfigMarkdown.parse(...) inside a try/catch in the loops used by loadPackageAgents() (and the similar loop that starts at the other occurrence around lines 724-725) so a single malformed markdown/frontmatter file doesn't reject the whole load; on catch, log the package path/item and the error (include error.message/stack) and continue to the next item instead of rethrowing. Ensure you update both places where ConfigMarkdown.parse(item) is called (the variable md and its consumers) to skip processing when parse fails.packages/opencode/src/config/config.ts-281-284 (1)
281-284:⚠️ Potential issue | 🟠 MajorThe
*fallback never satisfiesneedsInstall().
installDependencies()now writes@opencode-ai/plugin: "*"whenInstallation.VERSIONisn't semver, butneedsInstall()still compares the installed dependency against rawInstallation.VERSION. For builds like0.0.0-opencode/jolly-river-..., every config load will keep re-runningbun install.Possible fix
- const targetVersion = Installation.isLocal() ? "latest" : Installation.VERSION + const targetVersion = Installation.isLocal() ? "latest" : pluginVersion(Installation.VERSION) if (targetVersion === "latest") { const isOutdated = await PackageRegistry.isOutdated("@opencode-ai/plugin", depVersion, dir) if (!isOutdated) return false log.info("Cached version is outdated, proceeding with install", { pkg: "@opencode-ai/plugin", cachedVersion: depVersion, }) return true } if (depVersion === targetVersion) return falseAlso applies to: 288-289
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/config/config.ts` around lines 281 - 284, The current pluginVersion fallback returns "*" for non-semver VERSIONs which never matches needsInstall, causing repeated installs; change pluginVersion to return the original value when semver.test(value) fails (i.e., return value instead of "*") so install metadata stores the actual VERSION string and needsInstall can compare correctly; apply the same fix to the sibling function (pluginPeerVersion / the other occurrence at lines ~288-289) so both places preserve non-semver VERSIONs rather than substituting "*".packages/app/src/pages/session/message-timeline.tsx-352-360 (1)
352-360:⚠️ Potential issue | 🟠 MajorRename is a no-op for untitled root sessions.
This command is enabled for any
sessionID(), but on a root session with notitleValue()and noparentID()the header never renders, soopenTitleEditor()flipstitle.editingwith nothing visible to focus. Either gate the command the same way as the header, or make the header render while editing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/src/pages/session/message-timeline.tsx` around lines 352 - 360, The "session.rename" command is a no-op for untitled root sessions because openTitleEditor() toggles title.editing when the header isn't rendered; update the command registration in the command.register("session-title") block to use the same enablement logic as the header: only enable the command when sessionID() is truthy AND (titleValue() is truthy OR parentID() is truthy). In other words, replace the current disabled condition (!sessionID()) with the header's gating check so openTitleEditor() is only callable when the title editor will actually render; alternatively (if you prefer editing to force-render the header) implement logic in the header rendering code to render the title editor when title.editing is true, but pick one approach and apply it consistently around sessionID(), titleValue(), parentID(), and openTitleEditor().packages/app/src/pages/session.tsx-821-831 (1)
821-831:⚠️ Potential issue | 🟠 Major
jumpToTop()stops at the current history window.When
historyWindow.turnStart() > 0, older turns are still windowed out. Scrolling the current scroller totop: 0only triggers one backfill pass, and the scroll-preservation logic then snaps the viewport back down, so the new “first message” command never reaches the actual oldest message. Reveal/reset the history window before scrolling, and fetch remaining history ifhistoryMore()is still true.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/src/pages/session.tsx` around lines 821 - 831, Modify jumpToTop so it first expands the history window and fully backfills before forcing the scroller to the top: if historyWindow.turnStart() > 0 call historyWindow.reveal(0) to reset the window start, then while historyMore() is true trigger the same backfill/fetch loop used elsewhere (awaiting each fetch) until historyMore() becomes false; only then call autoScroll.pause() and scroller.scrollTo({ top: 0, behavior: "auto" }) so the viewport stays at the true oldest message (keep using the existing symbols jumpToTop, scroller, autoScroll.pause, historyWindow.turnStart, historyWindow.reveal, and historyMore()).packages/session-modal/src/main.tsx-212-220 (1)
212-220:⚠️ Potential issue | 🟠 MajorReload drops the desktop-provided credentials.
After Tauri initialization,
connection()may carryusernameandpassword. Submitting this form replaces the whole object with{ baseUrl, source: "manual" }, so a no-op reload immediately strips auth and subsequent SDK calls to the protected sidecar start failing.Possible fix
- setConnection({ - baseUrl: value, - source: "manual", - }) + setConnection((prev) => ({ + baseUrl: value, + username: prev.baseUrl === value ? prev.username : undefined, + password: prev.baseUrl === value ? prev.password : undefined, + source: prev.baseUrl === value ? prev.source : "manual", + }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/session-modal/src/main.tsx` around lines 212 - 220, The saveBaseUrl handler currently overwrites the entire connection state with only { baseUrl, source: "manual" }, which discards any existing username/password coming from connection(); update saveBaseUrl to merge the new baseUrl and source into the existing connection object instead of replacing it — e.g., read the current connection (connection()), preserve its username/password (and any other fields), then call setConnection with { ...existingConnection, baseUrl: value, source: "manual" } and keep the localStorage and log behavior unchanged.packages/session-modal/src/main.tsx-146-187 (1)
146-187:⚠️ Potential issue | 🟠 MajorOne bad directory request blanks the entire modal.
These per-directory fetches are wrapped in a single
Promise.all, so onesession.list()/permission.list()/question.list()failure rejects the whole resource and the UI drops into the global error state. For a cross-project triage view, it's better to log and skip the broken directory than lose every other session.Possible fix
- const rows = await Promise.all( + const rows = await Promise.allSettled( directories.map(async (directory) => { const at = Date.now() const scoped = client(value.baseUrl, auth, directory) const [sessions, permissions, questions, statuses] = await Promise.all([ scoped.session.list({ limit: 200 }).then((x) => x.data ?? []), scoped.permission.list().then((x) => x.data ?? []), scoped.question.list().then((x) => x.data ?? []), scoped.session.status().then((x) => x.data ?? {}), ]) ... }), ) - const sorted = rows - .flat() + const sorted = rows + .flatMap((result) => { + if (result.status === "fulfilled") return result.value + log("directory:error", result.reason) + return [] + }) .sort((a, b) => b.updated - a.updated) .sort((a, b) => a.priority - b.priority)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/session-modal/src/main.tsx` around lines 146 - 187, The current Promise.all over directories causes a single failing per-directory fetch (e.g., scoped.session.list, scoped.permission.list, scoped.question.list, or scoped.session.status) to reject the entire rows computation and blank the modal; wrap each directory's fetch logic inside a try/catch (inside the directories.map async callback) or use Promise.allSettled for the outer aggregation so that on error you log the directory and the caught error (use the existing log call) and return an empty array for that directory so other directories still populate rows; update references in this block (rows, directories.map, scoped.session.list / scoped.permission.list / scoped.question.list / scoped.session.status, bySession, rootSession, updatedAt) to gracefully skip failed directories instead of letting the whole Promise.all reject.packages/app/src/components/settings-models.tsx-130-168 (1)
130-168:⚠️ Potential issue | 🟠 MajorLabel the quick-slot clear buttons.
Both
circle-xactions are icon-only and currently unnamed, so assistive tech can't distinguish “clear first quick model” from “clear second quick model.” Add localizedaria-labels to both buttons.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/src/components/settings-models.tsx` around lines 130 - 168, The two IconButton instances used to clear quick slots are icon-only and lack accessible names; update the first IconButton (the one that calls models.quick.set("a", undefined)) and the second IconButton (the one that calls models.quick.set("b", undefined)) to include localized aria-labels via language.t (e.g., language.t("settings.models.quick.first.clear") and language.t("settings.models.quick.second.clear") or similar keys), ensuring each button has a distinct, translated aria-label so assistive tech can distinguish "clear first quick model" from "clear second quick model."packages/app/src/components/dialog-select-model.tsx-96-113 (1)
96-113:⚠️ Potential issue | 🟠 MajorExpose the favorite action as a real toggle button.
This new control is icon-only, but it doesn't expose either an accessible name or pressed state. Screen-reader users won't know what it does or whether the model is already favorited.
♿ Suggested fix
<IconButton icon="circle-check" variant="ghost" size="small" class="shrink-0" + aria-label={language.t( + favorite(i.provider.id, i.id) ? "dialog.model.unfavorite" : "dialog.model.favorite", + )} + aria-pressed={favorite(i.provider.id, i.id)} classList={{ "opacity-100": favorite(i.provider.id, i.id), "opacity-30": !favorite(i.provider.id, i.id), }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/src/components/dialog-select-model.tsx` around lines 96 - 113, The icon-only control isn't exposed as a toggle for assistive tech; update the IconButton rendered in dialog-select-model.tsx to include an accessible name and pressed state: pass an aria-label (use language.t with the same keys "dialog.model.favorite" / "dialog.model.unfavorite") and an aria-pressed attribute bound to favorite(i.provider.id, i.id), ensure the element is a proper button (type="button" or role="button" if IconButton requires it), and keep the existing onClick handler toggleFavorite(event, i.provider.id, i.id) so screen-reader users can both discover the action and perceive its current state.packages/app/src/pages/directory-layout.tsx-27-33 (1)
27-33:⚠️ Potential issue | 🟠 MajorNormalize the full file ref before calling
openPath.This only picks the join separator. A ref like
src/foo.tsstill becomesC:\repo\src/foo.ts, which is brittle in Windows/WSL desktop flows. Normalize the whole relative segment, or run the joined path throughplatform.normalizeProjectPath()before opening.🛠️ Suggested fix
onOpenFilePath={async (input) => { - const file = input.path.replace(/^[\\/]+/, "") - const separator = props.directory.includes("\\") ? "\\" : "/" - const path = props.directory.endsWith(separator) ? props.directory + file : props.directory + separator + file + const separator = platform.os === "windows" || props.directory.includes("\\") ? "\\" : "/" + const file = input.path.replace(/^[\\/]+/, "").replace(/[\\/]+/g, separator) + const joined = props.directory.endsWith(separator) ? props.directory + file : props.directory + separator + file + const path = platform.normalizeProjectPath ? await platform.normalizeProjectPath(joined) : joined if (platform.platform === "desktop" && platform.openPath) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/src/pages/directory-layout.tsx` around lines 27 - 33, The code in onOpenFilePath builds a path by concatenating props.directory and input.path but only normalizes the separator, which can leave mixed separators (e.g., C:\repo/src/foo.ts) on Windows/WSL; fix by running the composed path through the platform normalization helper before calling platform.openPath — after computing path from props.directory and input.path, call platform.normalizeProjectPath(path) (or otherwise fully normalize the relative segment) and use that normalized value in the platform.openPath call to ensure a consistent, platform-correct path.packages/app/src/components/dialog-select-model-unpaid.tsx-87-104 (1)
87-104:⚠️ Potential issue | 🟠 MajorExpose the favorite action as a real toggle button.
This icon-only control still needs button semantics. Without an accessible name and
aria-pressed, screen-reader users can't tell what the action does or whether the model is already favorited.♿ Suggested fix
<IconButton icon="circle-check" variant="ghost" size="small" class="shrink-0" + aria-label={language.t( + favorite(i.provider.id, i.id) ? "dialog.model.unfavorite" : "dialog.model.favorite", + )} + aria-pressed={favorite(i.provider.id, i.id)} classList={{ "opacity-100": favorite(i.provider.id, i.id), "opacity-30": !favorite(i.provider.id, i.id), }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/src/components/dialog-select-model-unpaid.tsx` around lines 87 - 104, The icon-only favorite control lacks button semantics for screen readers; update the IconButton usage (the Tooltip/IconButton block) to provide an accessible name and pressed state by passing an aria-label derived from language.t(...) (same text as the Tooltip value) and an aria-pressed prop set to favorite(i.provider.id, i.id), and ensure the IconButton renders as a real button (e.g., type="button" if the component supports it) so toggleFavorite(event, i.provider.id, i.id) correctly exposes the toggle state to assistive tech.packages/desktop/src/index.tsx-463-478 (1)
463-478:⚠️ Potential issue | 🟠 MajorThe drag-drop prevention is too broad—add a file type check to match the pattern in
dialog.tsx.The
handleDragOverandhandleDropfunctions prevent default for all dragover and drop events globally. Compare this withpackages/ui/src/context/dialog.tsx(lines 71–74), which implementspreventFileDropNavigationwith a selective check:if (!event.dataTransfer?.types.includes("Files")) return. The dialog context only prevents defaults for file drops, not all drag operations. Update the handlers here to check for file types before preventing defaults to avoid suppressing legitimate drag interactions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop/src/index.tsx` around lines 463 - 478, The global drag handlers (handleDragOver and handleDrop) are preventing defaults for all drag events; mirror the selective behavior from preventFileDropNavigation in dialog.tsx by first checking if event.dataTransfer?.types includes "Files" and only then call e.preventDefault() (and any other suppression like stopPropagation if present); update both handleDragOver and handleDrop to perform this file-type check before preventing default so only file drags/drops are blocked.
🟡 Minor comments (6)
packages/desktop/src/menu.ts-72-90 (1)
72-90:⚠️ Potential issue | 🟡 MinorRoute the new File menu labels through i18n.
"Search All Sessions..."and"Clone Project..."are hardcoded while the surrounding menu entries uset(...). That leaves these two items untranslated and inconsistent with the rest of the desktop menu.🌐 Suggested change
await MenuItem.new({ - text: "Search All Sessions...", + text: t("desktop.menu.file.searchAllSessions"), accelerator: "Shift+Cmd+P", action: () => trigger("session.search.all"), }), @@ await MenuItem.new({ - text: "Clone Project...", + text: t("desktop.menu.file.cloneProject"), accelerator: "Shift+Cmd+O", action: () => trigger("project.clone"), }),Add matching keys in the desktop i18n dictionaries as well.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop/src/menu.ts` around lines 72 - 90, Replace the two hardcoded menu labels in the MenuItem.new calls with i18n lookups like the surrounding entries use: change the "Search All Sessions..." and "Clone Project..." texts to t(...) keys (e.g., t("desktop.menu.file.searchAllSessions") and t("desktop.menu.file.cloneProject") or your preferred key names) inside the MenuItem.new invocations, and add those corresponding keys and translations to the desktop i18n dictionaries so the entries are translated consistently with the other menu items.packages/opencode/src/session/index.ts-399-411 (1)
399-411:⚠️ Potential issue | 🟡 MinorRequire
timeinsetArchived.
setArchivedis now callable with{ sessionID, updated }or even just{ sessionID }becausetimeis optional. That broadens the contract from “set archive state” to “maybe touchtime_updated/ maybe no-op”, which is easy to misuse from internal callers.Suggested fix
export const setArchived = fn( z.object({ sessionID: Identifier.schema("session"), - time: z.number().nullable().optional(), + time: z.number().nullable(), updated: z.number().optional(), }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/session/index.ts` around lines 399 - 411, The setArchived RPC currently allows omitting time because the input schema marks time as optional/nullable; change the zod schema for setArchived so time is required (remove .nullable().optional()), ensuring the input to setArchived always includes a concrete time value, and keep the updated field optional as-is; then update any callers of setArchived to pass a time value if they don't already. Locate the setArchived function and modify its input schema (Identifier.schema("session"), time: z.number()...) and leave the DB update logic using SessionTable.time_archived and time_updated unchanged.packages/app/src/context/server.test.ts-10-18 (1)
10-18:⚠️ Potential issue | 🟡 MinorAdd a Windows drive-root case to the root-stability coverage.
C:\\is also a root path, and it's the easiest one for a trailing-separator normalizer to accidentally collapse toC:. This suite should pin that behavior down too.Proposed test addition
test("keeps root separators stable", () => { expect(normalizeWorktree("/")).toBe("/") expect(normalizeWorktree("\\")).toBe("\\") + expect(normalizeWorktree("C:\\")).toBe("C:\\") })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/src/context/server.test.ts` around lines 10 - 18, Test suite for normalizeWorktree misses the Windows drive-root case; add a test asserting normalizeWorktree("C:\\\\") returns "C:\\\\" to ensure drive-root separators are preserved (similar to existing root tests for "/" and "\\"). Update the test block that contains "keeps root separators stable" to include the Windows drive-root expectation using the normalizeWorktree function name.packages/ui/src/components/message-part.tsx-1761-1764 (1)
1761-1764:⚠️ Potential issue | 🟡 MinorOnly render a clickable filename when
openFilePathexists.These call sites always provide a truthy click path, but
openProjectFile()is a no-op whendata.openFilePathis missing. In hosts that don't wire that capability, the filename becomes a focusable control that does nothing.Also applies to: 1831-1834, 1948-1954, 2038-2040
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/message-part.tsx` around lines 1761 - 1764, ToolFileAccordion currently always gets an onPathClick that calls openProjectFile(path(), data.directory, data.openFilePath) even when data.openFilePath is falsy, producing a focusable no-op; change the prop so onPathClick is only supplied when data.openFilePath is truthy (e.g., conditionalize passing onPathClick or pass undefined) so filenames are not rendered as clickable when openProjectFile is effectively unavailable. Update all similar call sites that pass path(), openProjectFile, data.directory and data.openFilePath (including the other ToolFileAccordion usages in this component) to follow the same conditional pattern.packages/app/src/pages/layout/helpers.ts-182-186 (1)
182-186:⚠️ Potential issue | 🟡 MinorDon't hardcode the "All projects" label in the helper.
This bypasses the new
sidebar.group.allProjectstranslation key, so localized builds will still render this group in English. Thread the label in from the caller or translate the"all"group at render time instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/src/pages/layout/helpers.ts` around lines 182 - 186, The helper currently returns a group with a hardcoded label "All projects" (id "all"); instead, wire the label through the caller or use the translation key sidebar.group.allProjects when building the group so it is localized. Update the function in helpers.ts that returns the group (the object with id "all", label currently "All projects", projects) to accept a label parameter (or return the id only and resolve label at render time) and ensure callers pass the translated string (or change the renderer to call i18n.t('sidebar.group.allProjects') for id "all") so the label is not hardcoded.packages/desktop/src-tauri/src/lib.rs-443-446 (1)
443-446:⚠️ Potential issue | 🟡 MinorLogic issue: Directory is created only when it already exists.
The condition
if path.exists()on line 443 meanscreate_dir_allis only called when the path already exists, which is a no-op. The intended logic appears to be finding a unique subdirectory name within an existing parent directory vs. using the path directly as the clone target.However, the
create_dir_allon line 444 is inside theif path.exists()block, meaning if the parent path doesn't exist, the code proceeds to use it directly without creating parent directories, which may causegit cloneto fail.🛠️ Consider restructuring the logic
let target = if let Some(directory) = directory.filter(|v| !v.trim().is_empty()) { let path = PathBuf::from(directory.trim()); if path.exists() { - std::fs::create_dir_all(&path) - .map_err(|e| format!("Failed to create clone destination directory: {e}"))?; - let mut index = 1usize; loop { let candidate = if index == 1 { path.join(&name) } else { path.join(format!("{name}-{index}")) }; if !candidate.exists() { break candidate; } index += 1; } } else { if let Some(parent) = path.parent() { if !parent.as_os_str().is_empty() { std::fs::create_dir_all(parent) .map_err(|e| format!("Failed to create clone destination directory: {e}"))?; } } path }The
create_dir_all(&path)insideif path.exists()is unnecessary since the path already exists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop/src-tauri/src/lib.rs` around lines 443 - 446, The code currently calls std::fs::create_dir_all(&path) only when path.exists(), which is backwards and can leave parent directories missing before running git clone; change the logic so that you create the directory when it does NOT exist (i.e., if !path.exists() { std::fs::create_dir_all(&path).map_err(...) } ), or—if the intent is to choose a unique subdirectory inside an existing parent—ensure you detect the parent directory (e.g., parent_path = path.parent()), create the parent if missing with std::fs::create_dir_all(parent_path) and then compute a unique child name before using path as the clone target; update uses of path.exists() and the std::fs::create_dir_all call accordingly to guarantee necessary directories exist prior to cloning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 809b352c-32df-4688-80fb-aa91dbd21a4c
⛔ Files ignored due to path filters (3)
bun.lockis excluded by!**/*.lockpackages/sdk/js/src/v2/gen/sdk.gen.tsis excluded by!**/gen/**packages/sdk/js/src/v2/gen/types.gen.tsis excluded by!**/gen/**
📒 Files selected for processing (78)
.github/pull_request_template.mdpackage.jsonpackages/app/e2e/projects/projects-switch.spec.tspackages/app/src/app.tsxpackages/app/src/components/dialog-open-project.helpers.test.tspackages/app/src/components/dialog-open-project.helpers.tspackages/app/src/components/dialog-open-project.tsxpackages/app/src/components/dialog-select-model-unpaid.tsxpackages/app/src/components/dialog-select-model.tsxpackages/app/src/components/dialog-select-session.tsxpackages/app/src/components/dialog-settings.tsxpackages/app/src/components/session/session-header.tsxpackages/app/src/components/settings-general.tsxpackages/app/src/components/settings-keybinds.tsxpackages/app/src/components/settings-models.tsxpackages/app/src/context/command.tsxpackages/app/src/context/local.tsxpackages/app/src/context/models.tsxpackages/app/src/context/platform.tsxpackages/app/src/context/server.test.tspackages/app/src/context/server.tsxpackages/app/src/i18n/en.tspackages/app/src/index.csspackages/app/src/pages/directory-layout.tsxpackages/app/src/pages/layout.tsxpackages/app/src/pages/layout/deep-links.tspackages/app/src/pages/layout/helpers.test.tspackages/app/src/pages/layout/helpers.tspackages/app/src/pages/layout/sidebar-project.tsxpackages/app/src/pages/layout/sidebar-shell.tsxpackages/app/src/pages/session.tsxpackages/app/src/pages/session/helpers.test.tspackages/app/src/pages/session/helpers.tspackages/app/src/pages/session/message-timeline.tsxpackages/app/src/pages/session/use-session-commands.tsxpackages/desktop/package.jsonpackages/desktop/scripts/predev.tspackages/desktop/src-tauri/capabilities/default.jsonpackages/desktop/src-tauri/src/constants.rspackages/desktop/src-tauri/src/lib.rspackages/desktop/src-tauri/src/server.rspackages/desktop/src-tauri/tauri.session-modal.conf.jsonpackages/desktop/src/bindings.tspackages/desktop/src/i18n/en.tspackages/desktop/src/index.tsxpackages/desktop/src/menu.tspackages/opencode/script/preload.jspackages/opencode/src/cli/cmd/desktop.tspackages/opencode/src/cli/cmd/tui/component/prompt/autocomplete.tsxpackages/opencode/src/config/config.tspackages/opencode/src/index.tspackages/opencode/src/server/routes/session.tspackages/opencode/src/session/index.tspackages/opencode/src/util/which.tspackages/opencode/test/config/config.test.tspackages/script/src/index.tspackages/script/src/version.test.tspackages/script/src/version.tspackages/session-modal/.gitignorepackages/session-modal/README.mdpackages/session-modal/index.htmlpackages/session-modal/package.jsonpackages/session-modal/src/desktop-bindings.tspackages/session-modal/src/main.tsxpackages/session-modal/src/styles.csspackages/session-modal/tsconfig.jsonpackages/session-modal/vite.config.tspackages/ui/src/components/markdown-file-ref.test.tspackages/ui/src/components/markdown-file-ref.tspackages/ui/src/components/markdown.csspackages/ui/src/components/markdown.tsxpackages/ui/src/components/message-part.csspackages/ui/src/components/message-part.tsxpackages/ui/src/context/data.tsxpackages/ui/src/context/dialog.tsxpackages/util/src/session-transcript.tspackages/web/src/content/docs/keybinds.mdxscript/check-preload.ts
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/app/src/components/dialog-edit-project.tsx (1)
83-97:⚠️ Potential issue | 🟡 MinorUse the same icon property name (
url) in both SDK and sync API calls for consistency.The SDK update path (line 87) uses
url:icon: { color: store.color, url: store.iconUrl }But
globalSync.project.meta()(line 97) usesoverride:icon: { color: store.color, override: store.iconUrl || undefined }Both properties are valid and sync to the same database field, so no data loss occurs. However, they should use the same property name for consistency. Use
urlin both places to match the SDK schema.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/src/components/dialog-edit-project.tsx` around lines 83 - 97, The icon property name is inconsistent between the SDK update path and the sync call; change the icon object passed to globalSync.project.meta(...) to use url instead of override so it matches the shape used by globalSDK.client.project.update. Update the call in the dialog-edit-project component where globalSync.project.meta(...) is invoked (referencing props.project, globalSDK.client.project.update, and globalSync.project.meta) to pass icon: { color: store.color, url: store.iconUrl || undefined }.
🧹 Nitpick comments (9)
packages/app/src/pages/session/terminal-panel.tsx (1)
120-133: Consider adding cleanup for thesetTimeoutto match the pattern used elsewhere.The existing
focusfunction (lines 86-107) properly cleans up its timers via the returned cleanup function, but this effect'ssetTimeoutat line 130 has no cleanup. If the effect re-runs or the component unmounts before the timeout fires, the callback will still execute.While likely benign, adding cleanup would maintain consistency with the established pattern in this file and prevent stale callbacks.
♻️ Proposed fix to add cleanup
createEffect( on( () => opened(), (isOpen, wasOpen) => { if (platform.platform !== "desktop" || !isOpen || wasOpen !== false) return const activeId = terminal.active() if (!activeId) return if (document.activeElement instanceof HTMLElement) { document.activeElement.blur() } - setTimeout(() => focusTerminalById(activeId), 0) + const timer = setTimeout(() => focusTerminalById(activeId), 0) + onCleanup(() => clearTimeout(timer)) }, ), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/src/pages/session/terminal-panel.tsx` around lines 120 - 133, The createEffect using on(opened) schedules focusTerminalById via setTimeout without cleanup; modify that effect to capture the timeout ID (from setTimeout) and return a cleanup function that calls clearTimeout on it so the timer is cleared if the effect re-runs or the component unmounts, mirroring the cleanup pattern used in the existing focus function; keep the change local to the createEffect block that references opened(), platform.platform, terminal.active(), and focusTerminalById.packages/ui/src/hooks/filter-search.ts (1)
33-44: Consider normalizingneedleinsidefuzzy()for a safer API.The
startsWithcomparison on line 37 compares the rawneedleagainst normalizedtext. Sincetextis already normalized viabuild(), callers must remember to normalize the needle before callingfuzzy(). All current usages do this correctly, but the API could be more defensive by normalizing internally.♻️ Proposed refactor to normalize needle internally
export const fuzzy = <T>(needle: string, list: T[], keys?: string[]) => { + const normalizedNeedle = normalize(needle) const rows = build(list, keys) - return Array.from(fuzzysort.go(needle, rows, { key: "text" })) + return Array.from(fuzzysort.go(normalizedNeedle, rows, { key: "text" })) .sort((a, b) => { - const ab = Number(a.obj.text.startsWith(needle)) - const bb = Number(b.obj.text.startsWith(needle)) + const ab = Number(a.obj.text.startsWith(normalizedNeedle)) + const bb = Number(b.obj.text.startsWith(normalizedNeedle)) if (ab !== bb) return bb - ab if (a.score !== b.score) return b.score - a.score return a.obj.ord - b.obj.ord }) .map((hit) => hit.obj.val) }This would also require updating callers to remove their explicit
normalize()calls on the needle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/hooks/filter-search.ts` around lines 33 - 44, Normalize the incoming search string inside fuzzy() before calling build and performing comparisons: in the fuzzy function, compute a normalizedNeedle (using the same normalize logic build() uses) and pass that to fuzzysort.go and to the startsWith check (replace references to raw needle with normalizedNeedle); update callers to remove their external normalize() calls so they rely on fuzzy() to normalize internally; ensure you use the same normalization helper used by build() to keep behavior consistent.packages/app/src/components/settings-general.tsx (2)
87-102: Same refactor opportunity assaveClonePath.The
Promise.resolve().then()pattern can be replaced withtry/catch/finallyfor consistency and readability.♻️ Suggested refactor
const resetClonePath = async () => { const setClonePath = platform.setDefaultCloneDirectory if (!setClonePath) return setStore("cloneBusy", true) - await Promise.resolve() - .then(async () => { - await setClonePath(null) - setStore("cloneDirty", false) - await clonePathActions.refetch() - }) - .catch((err: unknown) => { - const message = err instanceof Error ? err.message : String(err) - showToast({ title: language.t("common.requestFailed"), description: message }) - }) - .finally(() => setStore("cloneBusy", false)) + try { + await setClonePath(null) + setStore("cloneDirty", false) + await clonePathActions.refetch() + } catch (err: unknown) { + const message = err instanceof Error ? err.message : String(err) + showToast({ title: language.t("common.requestFailed"), description: message }) + } finally { + setStore("cloneBusy", false) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/src/components/settings-general.tsx` around lines 87 - 102, The resetClonePath function uses Promise.resolve().then(...).catch(...).finally(...) which is less readable than async/await; replace the Promise chain in resetClonePath with a try { await setClonePath(null); setStore("cloneDirty", false); await clonePathActions.refetch(); } catch (err) { const message = err instanceof Error ? err.message : String(err); showToast({ title: language.t("common.requestFailed"), description: message }); } finally { setStore("cloneBusy", false); } keeping the existing guard for setClonePath and the initial setStore("cloneBusy", true).
69-85: Consider using async/await directly instead ofPromise.resolve().then().The
Promise.resolve().then()pattern adds unnecessary indirection. SincesaveClonePathis already async, you can usetry/catch/finallydirectly for cleaner code.♻️ Suggested refactor
const saveClonePath = async () => { const setClonePath = platform.setDefaultCloneDirectory if (!setClonePath) return setStore("cloneBusy", true) const path = store.clonePath.trim() - await Promise.resolve() - .then(async () => { - await setClonePath(path || null) - setStore("cloneDirty", false) - await clonePathActions.refetch() - }) - .catch((err: unknown) => { - const message = err instanceof Error ? err.message : String(err) - showToast({ title: language.t("common.requestFailed"), description: message }) - }) - .finally(() => setStore("cloneBusy", false)) + try { + await setClonePath(path || null) + setStore("cloneDirty", false) + await clonePathActions.refetch() + } catch (err: unknown) { + const message = err instanceof Error ? err.message : String(err) + showToast({ title: language.t("common.requestFailed"), description: message }) + } finally { + setStore("cloneBusy", false) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/src/components/settings-general.tsx` around lines 69 - 85, The saveClonePath function currently uses Promise.resolve().then().catch().finally(), which is unnecessary because saveClonePath is already async; refactor it to use an explicit try/catch/finally block: call platform.setDefaultCloneDirectory (setClonePath) with await inside try, set cloneDirty to false and await clonePathActions.refetch() there, handle errors in catch by deriving the message from err (as done now) and calling showToast, and ensure cloneBusy is reset in finally; keep the same calls and state updates (setStore("cloneBusy"), setStore("cloneDirty"), setClonePath, clonePathActions.refetch, showToast) and preserve the early-return when setClonePath is falsy.packages/opencode/src/project/project.ts (2)
140-143: Consider clarifying the semantic difference betweenurlandoverride.In
fromRow, bothurlandoverrideare set to the same database value (row.icon_url). While this appears intentional for the current implementation, the distinction between these fields isn't immediately obvious from the code.A brief inline comment would help future maintainers understand when
urlvsoverrideshould be used (e.g.,urlfor auto-discovered icons,overridefor user-specified icons that should persist across re-discovery).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/project/project.ts` around lines 140 - 143, fromRow currently sets both icon.url and icon.override to row.icon_url which obscures their intended semantics; add a brief inline comment near the icon construction (the icon variable in fromRow) clarifying the difference (e.g., url = auto-discovered icon, override = user-specified icon that should persist across rediscovery) and when each should be populated, and adjust code only if needed to reflect that semantic (ensure fromRow still uses row.icon_url for override when appropriate and document why url and override may share the same value).
329-329: Fire-and-forgetdiscover()may cause subtle race conditions.The
discover(seeded)call runs withoutawait, meaning icon discovery happens asynchronously while the function continues to persist and emit events with potentially stale icon data. If discovery completes and callsupdate()shortly after, the emitted event and returned result may not reflect the discovered icon.Since this is behind
Flag.OPENCODE_EXPERIMENTAL_ICON_DISCOVERY, it may be acceptable for now. Consider documenting this behavior or adding a follow-up to await discovery when the feature graduates from experimental.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/project/project.ts` at line 329, The call to discover(seeded) is fire-and-forget and can cause race conditions where persistence and events are emitted before discovery completes; change the code to await discover(seeded) (or explicitly handle its Promise) so discovery finishes before calling update()/emitting events, and add try/catch to log or surface discovery errors; if you intentionally want async behavior while experimental, add a clear comment and a follow-up TODO to remove the fire-and-forget and switch to awaiting discover when Flag.OPENCODE_EXPERIMENTAL_ICON_DISCOVERY is promoted.packages/ui/src/components/session-turn.tsx (1)
22-22: ImportMarkdownCopyModeas a type here too.This symbol is exported as
export type, so a value import can fail in import-preserving TypeScript emit modes. Useimport { type MarkdownCopyMode } from "./markdown-copy"instead.Suggested fix
-import { MarkdownCopyMode } from "./markdown-copy" +import { type MarkdownCopyMode } from "./markdown-copy"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/session-turn.tsx` at line 22, The import of MarkdownCopyMode in session-turn.tsx should be a type-only import to avoid runtime import issues; change the import statement that currently reads importing MarkdownCopyMode from "./markdown-copy" to use a type import (import { type MarkdownCopyMode } from "./markdown-copy") so TypeScript treats MarkdownCopyMode as a type-only symbol when referenced in the SessionTurn component.packages/ui/src/components/message-part.tsx (2)
1175-1189: Verify that the filename control isn't nested inside a native accordion button.Lines 1179-1188 and 2010-2019 add filename
<button>elements insideAccordion.Trigger. If the trigger renders a native button, this is invalid interactive nesting and can break keyboard/click behavior for both the accordion and the new open-file action. Please verify the trigger markup and move the filename control outside it if needed.Also applies to: 2010-2019
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/message-part.tsx` around lines 1175 - 1189, The filename button inside the Show block (rendering via getFilename(props.path) when props.onPathClick exists) may be nested inside Accordion.Trigger which could itself render a native <button>, causing invalid interactive nesting; locate the MessagePart component (the Show wrapper around getFilename(props.path) and the button created when props.onPathClick is present) and verify whether Accordion.Trigger produces a native button; if it does, move the filename control out of the Accordion.Trigger (place it adjacent to the trigger) or replace the inner <button> with a non-nested accessible control (e.g., a <div> or <span> with role="button" and keyboard handlers that call props.onPathClick and stopPropagation) so you avoid nested native buttons while preserving click/keyboard behavior and calling props.onPathClick and event.stopPropagation().
57-57: ImportMarkdownCopyModeas a type.
./markdown-copyexportsMarkdownCopyModeviaexport type, so keeping it in the value import list can break underverbatimModuleSyntaxor other emit modes that preserve imports. Usetype MarkdownCopyModehere.Suggested fix
-import { MarkdownCopyMode, serializeMarkdownClipboardHTML, writeClipboardPayload } from "./markdown-copy" +import { type MarkdownCopyMode, serializeMarkdownClipboardHTML, writeClipboardPayload } from "./markdown-copy"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/message-part.tsx` at line 57, The import currently brings MarkdownCopyMode as a value; change it to a type-only import so the emitted JS won’t include a preserved value import: import MarkdownCopyMode as a type (e.g., use "import type { MarkdownCopyMode }") while keeping serializeMarkdownClipboardHTML and writeClipboardPayload as regular imports; update the import statement that references MarkdownCopyMode, serializeMarkdownClipboardHTML, and writeClipboardPayload accordingly so only MarkdownCopyMode is imported with "type".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/app/e2e/actions.ts`:
- Around line 716-727: The helper setWorkspacePinned infers pin state from
localized label text (name.includes("unpin")), which is brittle; update it to
read a stable attribute/test-id on the toggle returned by
workspacePinToggleSelector (e.g. aria-pressed, data-pinned or data-test-pinned)
instead of parsing text, use openWorkspaceMenu and the toggle locator to inspect
that attribute to decide whether to act, and when toggling call toggle.click({
force: true }) then explicitly wait for the attribute to flip (using
expect(toggle).toHaveAttribute(...) or locator.waitFor with the new value)
before returning; only press Escape when no change was needed or after the state
has settled.
In `@packages/app/e2e/projects/workspaces.spec.ts`:
- Line 462: The test uses new RegExp(`/${otherSlug}/session`) which treats regex
metacharacters in otherSlug as special; import and use the existing escape
function (from utils.ts) to escape otherSlug before building the RegExp, e.g.,
call escape(otherSlug) when constructing the pattern passed to
expect(page).toHaveURL so the URL assertion matches the literal slug; ensure the
escape import is added and replace the inline RegExp construction around
otherSlug accordingly.
In `@packages/app/src/i18n/en.ts`:
- Line 35: Update the i18n entry for the key "dialog.project.open.path.hint" so
the hint points to the correct settings location; change the text value that
currently reads "Settings > General > Projects." to reference "Desktop >
Projects." (Locate the "dialog.project.open.path.hint" entry in
packages/app/src/i18n/en.ts and update its string accordingly.)
In `@packages/ui/src/components/markdown-copy.test.ts`:
- Around line 33-65: The test mutates globalThis.navigator and
globalThis.ClipboardItem and currently restores them at the end of the test
body, which can leak if an assertion throws; wrap the mutation/restoration in a
try/finally (or move restoration to an afterEach) so the original values are
always restored; specifically update the test "writes both plain text and html
mime types" (and the other test covering lines 68-91) to store originalNavigator
and originalClipboardItem, set the fake navigator/ClipboardItem, call
writeClipboardPayload, and then restore originalNavigator and
originalClipboardItem in a finally block (or register an afterEach that restores
them) so FakeClipboardItem, globalThis.navigator, and globalThis.ClipboardItem
never leak to other tests.
In `@packages/ui/src/components/markdown-copy.ts`:
- Around line 7-14: The addStyle function appends new CSS to an element's style
string without ensuring declaration boundaries, which can merge the new rules
into the previous declaration; update addStyle (and its use of
el.getAttribute("style") / el.setAttribute("style", ...)) so that when an
existing style string is present you first ensure it ends with a semicolon (or
only whitespace) before appending the new value—i.e., if style does not already
end with ';' (ignoring trailing whitespace) add a ';' (and a space) before
concatenating the new value to preserve declaration boundaries.
- Around line 42-55: The writeClipboardPayload function currently calls
clipboard.write(...) without handling runtime failures; update
writeClipboardPayload to wrap the rich clipboard.write call in a try-catch and,
on any error, fall back to await clipboard.writeText(input.text) so plain text
is still copied; locate the call inside writeClipboardPayload where
clipboard.write is invoked and add the try-catch around that call, logging or
silently ignoring the error as appropriate and ensuring the fallback uses the
existing input.text.
In `@packages/ui/src/components/markdown.tsx`:
- Around line 267-275: The selection-cleanup removes copy buttons but doesn't
unwrap file-link buttons so their <button> markup gets serialized; update the
block that operates on wrap (the variable derived from selection.getRangeAt(0))
to also find all button.file-link elements and replace each button element with
its inner HTML (preserving the inner code/content) before calling
serializeMarkdownClipboardHTML; ensure you perform this unwrap on
wrap.querySelectorAll('button.file-link') so serializeMarkdownClipboardHTML
receives plain content rather than <button> tags.
- Around line 176-185: normalizeProjectPath currently returns unmodified paths
in the default case, allowing absolute paths or paths with "../" to escape the
workspace; update normalizeProjectPath so it resolves the candidate path against
the workspace root (the directory variable) using path resolution/normalization
and then verify the resolved absolute path is inside the workspace root (e.g.,
by checking the resolved path startsWith or is within path.resolve(directory));
if the resolved path is outside the workspace, return null/undefined or
otherwise reject it; apply this validation to all branches (file: URLs,
root-relative, ./ prefixes) so parseCodeFileRef and other callers cannot
reference files outside directory.
In `@packages/ui/src/components/message-part.tsx`:
- Around line 209-218: The current openProjectFile unconditionally removes a
leading slash/backslash from the result of relativizeProjectPath, which breaks
absolute paths; update openProjectFile so it detects absolute paths and only
strip a leading separator for relative project paths: call
relativizeProjectPath(path, directory) into file, then if file matches an
absolute pattern (starts with "/" or matches Windows drive-letter like
/^[A-Za-z]:[\\/]/ or starts with "\\"), leave it as-is; otherwise normalize
backslashes to "/" and remove a single leading "/" or "\" before calling
openFilePath?. Use the existing function names openProjectFile,
relativizeProjectPath, and openFilePath to locate and change the logic.
---
Outside diff comments:
In `@packages/app/src/components/dialog-edit-project.tsx`:
- Around line 83-97: The icon property name is inconsistent between the SDK
update path and the sync call; change the icon object passed to
globalSync.project.meta(...) to use url instead of override so it matches the
shape used by globalSDK.client.project.update. Update the call in the
dialog-edit-project component where globalSync.project.meta(...) is invoked
(referencing props.project, globalSDK.client.project.update, and
globalSync.project.meta) to pass icon: { color: store.color, url: store.iconUrl
|| undefined }.
---
Nitpick comments:
In `@packages/app/src/components/settings-general.tsx`:
- Around line 87-102: The resetClonePath function uses
Promise.resolve().then(...).catch(...).finally(...) which is less readable than
async/await; replace the Promise chain in resetClonePath with a try { await
setClonePath(null); setStore("cloneDirty", false); await
clonePathActions.refetch(); } catch (err) { const message = err instanceof Error
? err.message : String(err); showToast({ title:
language.t("common.requestFailed"), description: message }); } finally {
setStore("cloneBusy", false); } keeping the existing guard for setClonePath and
the initial setStore("cloneBusy", true).
- Around line 69-85: The saveClonePath function currently uses
Promise.resolve().then().catch().finally(), which is unnecessary because
saveClonePath is already async; refactor it to use an explicit try/catch/finally
block: call platform.setDefaultCloneDirectory (setClonePath) with await inside
try, set cloneDirty to false and await clonePathActions.refetch() there, handle
errors in catch by deriving the message from err (as done now) and calling
showToast, and ensure cloneBusy is reset in finally; keep the same calls and
state updates (setStore("cloneBusy"), setStore("cloneDirty"), setClonePath,
clonePathActions.refetch, showToast) and preserve the early-return when
setClonePath is falsy.
In `@packages/app/src/pages/session/terminal-panel.tsx`:
- Around line 120-133: The createEffect using on(opened) schedules
focusTerminalById via setTimeout without cleanup; modify that effect to capture
the timeout ID (from setTimeout) and return a cleanup function that calls
clearTimeout on it so the timer is cleared if the effect re-runs or the
component unmounts, mirroring the cleanup pattern used in the existing focus
function; keep the change local to the createEffect block that references
opened(), platform.platform, terminal.active(), and focusTerminalById.
In `@packages/opencode/src/project/project.ts`:
- Around line 140-143: fromRow currently sets both icon.url and icon.override to
row.icon_url which obscures their intended semantics; add a brief inline comment
near the icon construction (the icon variable in fromRow) clarifying the
difference (e.g., url = auto-discovered icon, override = user-specified icon
that should persist across rediscovery) and when each should be populated, and
adjust code only if needed to reflect that semantic (ensure fromRow still uses
row.icon_url for override when appropriate and document why url and override may
share the same value).
- Line 329: The call to discover(seeded) is fire-and-forget and can cause race
conditions where persistence and events are emitted before discovery completes;
change the code to await discover(seeded) (or explicitly handle its Promise) so
discovery finishes before calling update()/emitting events, and add try/catch to
log or surface discovery errors; if you intentionally want async behavior while
experimental, add a clear comment and a follow-up TODO to remove the
fire-and-forget and switch to awaiting discover when
Flag.OPENCODE_EXPERIMENTAL_ICON_DISCOVERY is promoted.
In `@packages/ui/src/components/message-part.tsx`:
- Around line 1175-1189: The filename button inside the Show block (rendering
via getFilename(props.path) when props.onPathClick exists) may be nested inside
Accordion.Trigger which could itself render a native <button>, causing invalid
interactive nesting; locate the MessagePart component (the Show wrapper around
getFilename(props.path) and the button created when props.onPathClick is
present) and verify whether Accordion.Trigger produces a native button; if it
does, move the filename control out of the Accordion.Trigger (place it adjacent
to the trigger) or replace the inner <button> with a non-nested accessible
control (e.g., a <div> or <span> with role="button" and keyboard handlers that
call props.onPathClick and stopPropagation) so you avoid nested native buttons
while preserving click/keyboard behavior and calling props.onPathClick and
event.stopPropagation().
- Line 57: The import currently brings MarkdownCopyMode as a value; change it to
a type-only import so the emitted JS won’t include a preserved value import:
import MarkdownCopyMode as a type (e.g., use "import type { MarkdownCopyMode }")
while keeping serializeMarkdownClipboardHTML and writeClipboardPayload as
regular imports; update the import statement that references MarkdownCopyMode,
serializeMarkdownClipboardHTML, and writeClipboardPayload accordingly so only
MarkdownCopyMode is imported with "type".
In `@packages/ui/src/components/session-turn.tsx`:
- Line 22: The import of MarkdownCopyMode in session-turn.tsx should be a
type-only import to avoid runtime import issues; change the import statement
that currently reads importing MarkdownCopyMode from "./markdown-copy" to use a
type import (import { type MarkdownCopyMode } from "./markdown-copy") so
TypeScript treats MarkdownCopyMode as a type-only symbol when referenced in the
SessionTurn component.
In `@packages/ui/src/hooks/filter-search.ts`:
- Around line 33-44: Normalize the incoming search string inside fuzzy() before
calling build and performing comparisons: in the fuzzy function, compute a
normalizedNeedle (using the same normalize logic build() uses) and pass that to
fuzzysort.go and to the startsWith check (replace references to raw needle with
normalizedNeedle); update callers to remove their external normalize() calls so
they rely on fuzzy() to normalize internally; ensure you use the same
normalization helper used by build() to keep behavior consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 05c5dfe1-a290-449e-b05b-a722944986f7
📒 Files selected for processing (29)
packages/app/e2e/actions.tspackages/app/e2e/projects/workspaces.spec.tspackages/app/e2e/selectors.tspackages/app/src/components/dialog-edit-project.tsxpackages/app/src/components/settings-general.tsxpackages/app/src/context/global-sync/bootstrap.tspackages/app/src/context/layout.test.tspackages/app/src/context/layout.tsxpackages/app/src/context/local.tsxpackages/app/src/context/models.tsxpackages/app/src/context/settings.tsxpackages/app/src/i18n/en.tspackages/app/src/pages/layout.tsxpackages/app/src/pages/layout/helpers.tspackages/app/src/pages/layout/sidebar-items.tsxpackages/app/src/pages/layout/sidebar-workspace.tsxpackages/app/src/pages/session/message-timeline.tsxpackages/app/src/pages/session/terminal-panel.tsxpackages/opencode/src/project/project.tspackages/opencode/test/project/project.test.tspackages/ui/src/components/markdown-copy.test.tspackages/ui/src/components/markdown-copy.tspackages/ui/src/components/markdown.tsxpackages/ui/src/components/message-part.tsxpackages/ui/src/components/session-turn.tsxpackages/ui/src/hooks/filter-search.test.tspackages/ui/src/hooks/filter-search.tspackages/ui/src/hooks/use-filtered-list.tsxpackages/ui/src/i18n/en.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/app/src/context/models.tsx
| export async function setWorkspacePinned(page: Page, workspaceSlug: string, enabled: boolean) { | ||
| const menu = await openWorkspaceMenu(page, workspaceSlug) | ||
| const toggle = menu.locator(workspacePinToggleSelector(workspaceSlug)).first() | ||
| await expect(toggle).toBeVisible() | ||
| const name = await toggle.textContent() | ||
| const pinned = (name ?? "").toLowerCase().includes("unpin") | ||
| if (pinned === enabled) { | ||
| await page.keyboard.press("Escape") | ||
| return | ||
| } | ||
| await toggle.click({ force: true }) | ||
| } |
There was a problem hiding this comment.
Use a non-localized signal for workspace pinning.
Line 721 infers the current state from the menu label (includes("unpin")), so this helper will start failing as soon as the action text is reworded or localized. Returning immediately after Line 726 also leaves no explicit settle point for the UI. Prefer a stable attribute/test id for the pinned state and wait for that state to change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/app/e2e/actions.ts` around lines 716 - 727, The helper
setWorkspacePinned infers pin state from localized label text
(name.includes("unpin")), which is brittle; update it to read a stable
attribute/test-id on the toggle returned by workspacePinToggleSelector (e.g.
aria-pressed, data-pinned or data-test-pinned) instead of parsing text, use
openWorkspaceMenu and the toggle locator to inspect that attribute to decide
whether to act, and when toggling call toggle.click({ force: true }) then
explicitly wait for the attribute to flip (using
expect(toggle).toHaveAttribute(...) or locator.waitFor with the new value)
before returning; only press Escape when no change was needed or after the state
has settled.
| const otherButton = page.locator(projectSwitchSelector(otherSlug)).first() | ||
| await expect(otherButton).toBeVisible() | ||
| await otherButton.click() | ||
| await expect(page).toHaveURL(new RegExp(`/${otherSlug}/session`)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the relevant section of the file to understand context
sed -n '440,480p' packages/app/e2e/projects/workspaces.spec.tsRepository: anduimagui/opencode
Length of output: 1394
🏁 Script executed:
# Find where otherSlug is defined - likely at the start of the test function
sed -n '400,470p' packages/app/e2e/projects/workspaces.spec.tsRepository: anduimagui/opencode
Length of output: 2406
🏁 Script executed:
# Find dirSlug function definition
rg -n "dirSlug" packages/app/e2e/projects/workspaces.spec.ts -B 2 -A 5Repository: anduimagui/opencode
Length of output: 814
🏁 Script executed:
# Find dirSlug function definition in utils
rg -n "dirSlug" packages/app/e2e/utils.ts -B 2 -A 10Repository: anduimagui/opencode
Length of output: 695
🏁 Script executed:
# Check base64Encode function to confirm it uses standard base64
rg -n "base64Encode|base64Decode" packages/app/e2e/utils.ts -B 2 -A 5Repository: anduimagui/opencode
Length of output: 609
🏁 Script executed:
# Check if there are regex escaping utilities in the codebase
rg -n "escapeRegex|escapeRegExp|RegExp|replace.*\\\\\|replace.*\[\]" packages/app/e2e/ -t ts | head -30Repository: anduimagui/opencode
Length of output: 4033
🏁 Script executed:
# Check what the escape function is in utils.ts
sed -n '1,40p' packages/app/e2e/utils.tsRepository: anduimagui/opencode
Length of output: 1477
Escape otherSlug before embedding it in RegExp.
The slug is base64-encoded and can contain regex metacharacters like + and /, which would be interpreted as regex syntax rather than literal characters. This makes the test brittle. An escape function already exists in utils.ts that can be applied here.
🧪 Proposed fix
- await expect(page).toHaveURL(new RegExp(`/${otherSlug}/session`))
+ const escapedOtherSlug = otherSlug.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")
+ await expect(page).toHaveURL(new RegExp(`/${escapedOtherSlug}/session`))📝 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.
| await expect(page).toHaveURL(new RegExp(`/${otherSlug}/session`)) | |
| const escapedOtherSlug = otherSlug.replace(/[.*+?^${}()|[\]\\]/g, "\\$&") | |
| await expect(page).toHaveURL(new RegExp(`/${escapedOtherSlug}/session`)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/app/e2e/projects/workspaces.spec.ts` at line 462, The test uses new
RegExp(`/${otherSlug}/session`) which treats regex metacharacters in otherSlug
as special; import and use the existing escape function (from utils.ts) to
escape otherSlug before building the RegExp, e.g., call escape(otherSlug) when
constructing the pattern passed to expect(page).toHaveURL so the URL assertion
matches the literal slug; ensure the escape import is added and replace the
inline RegExp construction around otherSlug accordingly.
| "dialog.project.open.git.helper": "gitlab.com/group/repo, github.com/org/repo, org/repo", | ||
| "dialog.project.open.path.label": "Project path", | ||
| "dialog.project.open.path.placeholder": "~/projects/my-app", | ||
| "dialog.project.open.path.hint": "Default location is set in Settings > General > Projects.", |
There was a problem hiding this comment.
Point this hint at the Desktop settings section.
Line 35 says Settings > General > Projects, but the new clone-path settings added below live under Desktop > Projects. As written, the dialog sends users to the wrong place.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/app/src/i18n/en.ts` at line 35, Update the i18n entry for the key
"dialog.project.open.path.hint" so the hint points to the correct settings
location; change the text value that currently reads "Settings > General >
Projects." to reference "Desktop > Projects." (Locate the
"dialog.project.open.path.hint" entry in packages/app/src/i18n/en.ts and update
its string accordingly.)
| test("writes both plain text and html mime types", async () => { | ||
| const originalNavigator = globalThis.navigator | ||
| const originalClipboardItem = globalThis.ClipboardItem | ||
|
|
||
| const writes: unknown[][] = [] | ||
| class FakeClipboardItem { | ||
| constructor(public data: Record<string, Blob>) {} | ||
| } | ||
|
|
||
| Object.defineProperty(globalThis, "navigator", { | ||
| value: { | ||
| clipboard: { | ||
| write: async (items: unknown[]) => { | ||
| writes.push(items) | ||
| }, | ||
| writeText: async () => {}, | ||
| }, | ||
| }, | ||
| configurable: true, | ||
| }) | ||
| Object.defineProperty(globalThis, "ClipboardItem", { value: FakeClipboardItem, configurable: true }) | ||
|
|
||
| await writeClipboardPayload({ text: "hello", html: "<p>hello</p>" }) | ||
|
|
||
| expect(writes.length).toBe(1) | ||
| const item = writes[0]?.[0] as FakeClipboardItem | ||
| expect(item.data["text/plain"]).toBeInstanceOf(Blob) | ||
| expect(item.data["text/html"]).toBeInstanceOf(Blob) | ||
| expect(await item.data["text/plain"]?.text()).toBe("hello") | ||
| expect(await item.data["text/html"]?.text()).toBe("<p>hello</p>") | ||
|
|
||
| Object.defineProperty(globalThis, "navigator", { value: originalNavigator, configurable: true }) | ||
| Object.defineProperty(globalThis, "ClipboardItem", { value: originalClipboardItem, configurable: true }) |
There was a problem hiding this comment.
Restore the clipboard globals in finally or afterEach.
If an assertion throws before Lines 64-65 or 90-91, the fake navigator/ClipboardItem leak into later tests and make failures order-dependent.
Also applies to: 68-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/components/markdown-copy.test.ts` around lines 33 - 65, The
test mutates globalThis.navigator and globalThis.ClipboardItem and currently
restores them at the end of the test body, which can leak if an assertion
throws; wrap the mutation/restoration in a try/finally (or move restoration to
an afterEach) so the original values are always restored; specifically update
the test "writes both plain text and html mime types" (and the other test
covering lines 68-91) to store originalNavigator and originalClipboardItem, set
the fake navigator/ClipboardItem, call writeClipboardPayload, and then restore
originalNavigator and originalClipboardItem in a finally block (or register an
afterEach that restores them) so FakeClipboardItem, globalThis.navigator, and
globalThis.ClipboardItem never leak to other tests.
| function addStyle(el: Element, value: string) { | ||
| const style = el.getAttribute("style") | ||
| if (!style) { | ||
| el.setAttribute("style", value) | ||
| return | ||
| } | ||
| el.setAttribute("style", `${style} ${value}`) | ||
| } |
There was a problem hiding this comment.
Preserve declaration boundaries when appending styles.
Line 13 joins the old and new style strings with a bare space. If the existing attribute does not already end with ;, the appended clipboard rules become part of the previous declaration and are ignored.
Suggested fix
function addStyle(el: Element, value: string) {
const style = el.getAttribute("style")
if (!style) {
el.setAttribute("style", value)
return
}
- el.setAttribute("style", `${style} ${value}`)
+ const separator = /;\s*$/.test(style) ? " " : "; "
+ el.setAttribute("style", `${style}${separator}${value}`)
}📝 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.
| function addStyle(el: Element, value: string) { | |
| const style = el.getAttribute("style") | |
| if (!style) { | |
| el.setAttribute("style", value) | |
| return | |
| } | |
| el.setAttribute("style", `${style} ${value}`) | |
| } | |
| function addStyle(el: Element, value: string) { | |
| const style = el.getAttribute("style") | |
| if (!style) { | |
| el.setAttribute("style", value) | |
| return | |
| } | |
| const separator = /;\s*$/.test(style) ? " " : "; " | |
| el.setAttribute("style", `${style}${separator}${value}`) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/components/markdown-copy.ts` around lines 7 - 14, The
addStyle function appends new CSS to an element's style string without ensuring
declaration boundaries, which can merge the new rules into the previous
declaration; update addStyle (and its use of el.getAttribute("style") /
el.setAttribute("style", ...)) so that when an existing style string is present
you first ensure it ends with a semicolon (or only whitespace) before appending
the new value—i.e., if style does not already end with ';' (ignoring trailing
whitespace) add a ';' (and a space) before concatenating the new value to
preserve declaration boundaries.
| export async function writeClipboardPayload(input: { text: string; html?: string }) { | ||
| const clipboard = navigator.clipboard | ||
| if (!clipboard) return | ||
| if (!input.html || typeof ClipboardItem === "undefined" || typeof clipboard.write !== "function") { | ||
| await clipboard.writeText(input.text) | ||
| return | ||
| } | ||
| await clipboard.write([ | ||
| new ClipboardItem({ | ||
| "text/plain": new Blob([input.text], { type: "text/plain" }), | ||
| "text/html": new Blob([input.html], { type: "text/html" }), | ||
| }), | ||
| ]) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "markdown-copy.ts" -type fRepository: anduimagui/opencode
Length of output: 108
🏁 Script executed:
cat -n ./packages/ui/src/components/markdown-copy.tsRepository: anduimagui/opencode
Length of output: 2486
🏁 Script executed:
# Check if there are any tests or other uses of clipboard in the codebase
rg "clipboard\." --type ts --type tsx -A 3 -B 1 | head -50Repository: anduimagui/opencode
Length of output: 90
🏁 Script executed:
# Check if there are any tests or other uses of clipboard in the codebase
rg "clipboard\." -t ts -A 3 -B 1Repository: anduimagui/opencode
Length of output: 9323
🏁 Script executed:
# Check how markdown-copy is used to understand impact
rg "writeClipboardPayload|from.*markdown-copy" -A 2 -B 1Repository: anduimagui/opencode
Length of output: 3548
🏁 Script executed:
cat -n ./packages/ui/src/components/markdown-copy.test.tsRepository: anduimagui/opencode
Length of output: 4100
🏁 Script executed:
# Check the message-part.tsx to see how writeClipboardPayload is called
rg -A 10 "writeClipboardPayload" ./packages/ui/src/components/message-part.tsx | head -30Repository: anduimagui/opencode
Length of output: 896
Add error handling for clipboard.write() with fallback to plain text.
The function currently has no protection against clipboard.write() throwing at runtime. While the code checks for API availability (line 45), browsers and webviews can still reject the write operation at runtime—for example, due to permission denials or HTTPS-only restrictions. When this happens, the user is silently left with nothing copied, which breaks the core functionality.
Other clipboard operations throughout the codebase (e.g., packages/web/src/components/share/copy-button.tsx, packages/console/app/src/component/header.tsx) all implement error handling, which indicates clipboard operations are expected to fail at runtime.
Wrap the clipboard.write() call in a try-catch block (lines 49–54) and fall back to clipboard.writeText(input.text) if the rich write fails.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/components/markdown-copy.ts` around lines 42 - 55, The
writeClipboardPayload function currently calls clipboard.write(...) without
handling runtime failures; update writeClipboardPayload to wrap the rich
clipboard.write call in a try-catch and, on any error, fall back to await
clipboard.writeText(input.text) so plain text is still copied; locate the call
inside writeClipboardPayload where clipboard.write is invoked and add the
try-catch around that call, logging or silently ignoring the error as
appropriate and ensuring the fallback uses the existing input.text.
| const file = parseCodeFileRef(code.textContent ?? "", directory) | ||
| if (!file) continue | ||
|
|
||
| const link = document.createElement("a") | ||
| link.href = href | ||
| link.className = "external-link" | ||
| link.target = "_blank" | ||
| link.rel = "noopener noreferrer" | ||
| code.parentNode?.replaceChild(link, code) | ||
| link.appendChild(code) | ||
| const button = document.createElement("button") | ||
| button.type = "button" | ||
| button.className = "file-link" | ||
| button.setAttribute("data-file-path", file.path) | ||
| if (file.line) button.setAttribute("data-file-line", String(file.line)) | ||
| code.parentNode?.replaceChild(button, code) | ||
| button.appendChild(code) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' packages/ui/src/components/markdown-file-ref.ts
rg -n 'parseCodeFileRef|resolve|normalize|relative|startsWith|isAbsolute' packages/ui/src/components/markdown-file-ref.tsRepository: anduimagui/opencode
Length of output: 2341
🏁 Script executed:
rg -n 'openFilePath' packages/ui/src/components/markdown.tsx -B 3 -A 3Repository: anduimagui/opencode
Length of output: 416
🏁 Script executed:
rg -n 'openFilePath' packages/ui/src/ -lRepository: anduimagui/opencode
Length of output: 179
🏁 Script executed:
fd markdown-file-ref packages/ui/src/ --type fRepository: anduimagui/opencode
Length of output: 163
🏁 Script executed:
sed -n '200,230p' packages/ui/src/components/markdown.tsxRepository: anduimagui/opencode
Length of output: 1242
🏁 Script executed:
rg -n 'openFilePath\s*[=:({]' packages/ui/src/ -A 5Repository: anduimagui/opencode
Length of output: 262
🏁 Script executed:
cat -n packages/ui/src/context/data.tsx | head -80Repository: anduimagui/opencode
Length of output: 1787
🏁 Script executed:
cat -n packages/ui/src/components/markdown-file-ref.test.ts | head -150Repository: anduimagui/opencode
Length of output: 1813
🏁 Script executed:
rg -n '\.\./|/etc|absolute|escape' packages/ui/src/components/markdown-file-ref.test.tsRepository: anduimagui/opencode
Length of output: 45
Fix path validation to prevent workspace escape through absolute paths and .. sequences.
The normalizeProjectPath function has a critical security gap. While it handles specific cases (file URLs, root-relative paths, ./ prefixes), its default case returns the input path unmodified. This allows:
- Absolute paths like
/etc/passwdto pass through - Relative escapes like
../../../sensitive-fileto bypass the workspace boundary
The function must validate that the final resolved path remains within the workspace directory and reject any paths that attempt to escape it. Without this check, assistant-authored markdown can deep-link to arbitrary files on the system instead of restricting access to workspace files.
Add path normalization/resolution plus an explicit boundary check before returning a valid path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/components/markdown.tsx` around lines 176 - 185,
normalizeProjectPath currently returns unmodified paths in the default case,
allowing absolute paths or paths with "../" to escape the workspace; update
normalizeProjectPath so it resolves the candidate path against the workspace
root (the directory variable) using path resolution/normalization and then
verify the resolved absolute path is inside the workspace root (e.g., by
checking the resolved path startsWith or is within path.resolve(directory)); if
the resolved path is outside the workspace, return null/undefined or otherwise
reject it; apply this validation to all branches (file: URLs, root-relative, ./
prefixes) so parseCodeFileRef and other callers cannot reference files outside
directory.
| const range = selection.getRangeAt(0) | ||
| const wrap = document.createElement("div") | ||
| wrap.appendChild(range.cloneContents()) | ||
| for (const item of wrap.querySelectorAll('[data-slot="markdown-copy-button"]')) { | ||
| item.remove() | ||
| } | ||
|
|
||
| const html = serializeMarkdownClipboardHTML(wrap.innerHTML) | ||
| if (!html) return |
There was a problem hiding this comment.
Unwrap button.file-link before serializing selection HTML.
The cleanup only removes the copy buttons. If the selection includes one of the new file-link buttons, the clipboard HTML still contains <button> markup, which rich paste targets often drop or mangle. Preserve the inner code content instead.
✂️ Proposed fix
const range = selection.getRangeAt(0)
const wrap = document.createElement("div")
wrap.appendChild(range.cloneContents())
for (const item of wrap.querySelectorAll('[data-slot="markdown-copy-button"]')) {
item.remove()
}
+ for (const item of wrap.querySelectorAll("button.file-link")) {
+ item.replaceWith(...Array.from(item.childNodes))
+ }
const html = serializeMarkdownClipboardHTML(wrap.innerHTML)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/components/markdown.tsx` around lines 267 - 275, The
selection-cleanup removes copy buttons but doesn't unwrap file-link buttons so
their <button> markup gets serialized; update the block that operates on wrap
(the variable derived from selection.getRangeAt(0)) to also find all
button.file-link elements and replace each button element with its inner HTML
(preserving the inner code/content) before calling
serializeMarkdownClipboardHTML; ensure you perform this unwrap on
wrap.querySelectorAll('button.file-link') so serializeMarkdownClipboardHTML
receives plain content rather than <button> tags.
| function openProjectFile( | ||
| path: string | undefined, | ||
| directory: string, | ||
| openFilePath?: (input: { path: string }) => void, | ||
| ) { | ||
| if (!path) return | ||
| const file = relativizeProjectPath(path, directory).replace(/^\//, "") | ||
| if (!file) return | ||
| openFilePath?.({ path: file }) | ||
| } |
There was a problem hiding this comment.
Don't strip leading separators from every absolute path.
Line 215 blindly removes a leading slash/backslash after relativizeProjectPath(). That breaks both sides of the new file-link flow: Windows project paths still arrive as \src\file.ts, and absolute non-project paths like /etc/hosts are turned into etc/hosts, which can open the wrong file.
Suggested fix
function openProjectFile(
path: string | undefined,
directory: string,
openFilePath?: (input: { path: string }) => void,
) {
if (!path) return
- const file = relativizeProjectPath(path, directory).replace(/^\//, "")
+ const relative = relativizeProjectPath(path, directory)
+ if (relative === path && /^(?:[A-Za-z]:[\\/]|[\\/])/.test(path)) return
+ const file = relative.replace(/^[/\\]+/, "")
if (!file) return
openFilePath?.({ path: file })
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/components/message-part.tsx` around lines 209 - 218, The
current openProjectFile unconditionally removes a leading slash/backslash from
the result of relativizeProjectPath, which breaks absolute paths; update
openProjectFile so it detects absolute paths and only strip a leading separator
for relative project paths: call relativizeProjectPath(path, directory) into
file, then if file matches an absolute pattern (starts with "/" or matches
Windows drive-letter like /^[A-Za-z]:[\\/]/ or starts with "\\"), leave it
as-is; otherwise normalize backslashes to "/" and remove a single leading "/" or
"\" before calling openFilePath?. Use the existing function names
openProjectFile, relativizeProjectPath, and openFilePath to locate and change
the logic.
|
This pull request has been automatically closed because it was not updated to meet our contributing guidelines within the 2-hour window. Feel free to open a new pull request that follows our guidelines. |
2 participants
Summary
local/all-workintegration branch againstdevin my fork so the full branch can be reviewed and shared as one stack.anomalyco/opencode, including session UX, desktop flows, model switching, clipboard actions, and file-link improvements.Upstream PRs referenced
desktopcommand for deep-linking into desktop app anomalyco/opencode#15479 - feat(opencode): adddesktopcommand for deep-linking into desktop appSummary by CodeRabbit
New Features
Improvements