Fix desktop release/update flow and improve terminal link handling - #55
Conversation
Standardize prop access in PanelLayout and QuickOpen by using destructuring syntax. This improves code readability and aligns with modern React conventions across the layout components.
Grant read/write permissions to the repository contents and configure the workflow to always attempt artifact upload. This prevents silent failures during the release-desktop job and ensures the build output is accessible.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds terminal link routing with project-aware resolution and user preferences; editor navigation targets and file-tree reveal support; centralizes quick-open apps; moves right-sidebar toggle to Header; replaces update popover with toast flows and release notes links; tweaks macOS window positions; makes desktop release workflow platform-selectable with a dynamic matrix. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Terminal
participant Routing as terminal-link-routing
participant FS as fsApi
participant Store as useEditorStore
participant Editor as CodeMirrorEditor
User->>Terminal: Click/activate link
Terminal->>Routing: resolveTerminalLink(token, { projectRootPath })
Routing->>FS: probe candidates (readFile/listDir/getHomeDir)
FS-->>Routing: resolution (file / dir / external / null)
Routing-->>Terminal: ResolvedTerminalLink
Terminal->>Store: openFile(path, workspaceId, { line, column })
Terminal->>Store: requestFileTreeReveal(path, workspaceId)
Store-->>Editor: set navigationTarget for file
Editor->>Editor: apply selection & scrollIntoView
sequenceDiagram
actor User
participant Terminal
participant Store as useEditorStore
participant LeftSidebar
participant FileTree
participant FS as fsApi
User->>Terminal: Open file link
Terminal->>Store: openFile(...) & requestFileTreeReveal(...)
Store->>LeftSidebar: fileTreeRevealTarget updated
LeftSidebar->>LeftSidebar: switch to 'files' tab (if needed)
LeftSidebar->>FileTree: reveal target path
FileTree->>FS: listDir / load children along path
FileTree->>User: focus + timed highlight
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
🟡 Release notes URL uses wrong tag prefix, generating broken links Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/src/hooks/use-updater.ts (1)
55-65: 🛠️ Refactor suggestion | 🟠 MajorMove the Tauri updater/process I/O behind
src/api/.This change grows the shared updater state here, but the module still owns the actual
check,downloadAndInstall, andrelaunchcalls. Please move that I/O into anapps/web/src/api/*module and keepuse-updater.tsfocused on client-side status orchestration.As per coding guidelines, "All API interaction logic must live in
src/api/" and "Usehooks/directory for client-side state logic".Also applies to: 88-99, 113-139
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/use-updater.ts` around lines 55 - 65, Move all Tauri I/O out of the hook into an API module: create an apps/web/src/api/updater.ts that exports async wrapper functions (e.g., checkForUpdateIO/checkUpdate, downloadAndInstallUpdate, relaunchApp) which internally import '@tauri-apps/plugin-updater' and invoke the actual check(), install/download, and relaunch calls (and handle isTauriRuntime guard inside those wrappers). In use-updater.ts keep the client-side orchestration (the exported checkForUpdate hook function and any status callbacks) but replace direct calls to the Tauri plugin with calls to the new api functions (preserve the same UpdateInfo/UpdateStatus types and function names used in the hook), and ensure fallbacks/null returns when not running in Tauri.apps/web/src/components/agent/AgentManagerView.tsx (1)
277-309:⚠️ Potential issue | 🟠 MajorFilter custom agents before including them in Installed.
This view now mixes
mgr.installedAgentswithmgr.customAgents, and the latter still appears to be the raw list because it also drives the tab counts. That means a search that matches no installed agents will still render every custom agent here, and theNo installed agents matching...state never appears once any custom agent exists. Please render a query-filtered custom list in this tab as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/agent/AgentManagerView.tsx` around lines 277 - 309, The tab is rendering unfiltered mgr.customAgents alongside mgr.installedAgents, so search queries still show all custom agents; filter the custom agents the same way as installed ones before rendering and counting: create a filteredCustom list (e.g., const filteredCustom = mgr.customAgents.filter(a => matchesQuery(a, query)) or reuse the existing installed-agent filtering predicate), replace uses of mgr.customAgents in the AgentManagerView render with filteredCustom (map to <CustomAgentCard ... index={mgr.installedAgents.length + i} ...>), and update any tab-count logic that references mgr.customAgents to use filteredCustom.length so the "No installed agents matching..." empty state appears correctly.
🧹 Nitpick comments (4)
.github/workflows/release-desktop.yml (1)
257-272: Consider using env vars for safer secret handling.Direct interpolation of
${{ secrets.TAURI_SIGNING_PRIVATE_KEY }}into the shell condition (line 262) is fragile if the secret contains shell metacharacters. Additionally, the hardcoded heredoc delimiter__EOF__would break if the secret itself contains that string.♻️ Safer pattern using environment variables
- name: Export updater signing env shell: bash + env: + _SIGNING_KEY: ${{ secrets.TAURI_SIGNING_PRIVATE_KEY }} + _SIGNING_KEY_PASSWORD: ${{ secrets.TAURI_SIGNING_PRIVATE_KEY_PASSWORD }} run: | set -euo pipefail - if [ -z "${{ secrets.TAURI_SIGNING_PRIVATE_KEY }}" ]; then + if [ -z "$_SIGNING_KEY" ]; then echo "❌ Missing secret: TAURI_SIGNING_PRIVATE_KEY" exit 1 fi + DELIMITER="__EOF_$(date +%s)__" { - echo 'TAURI_SIGNING_PRIVATE_KEY<<__EOF__' - echo "${{ secrets.TAURI_SIGNING_PRIVATE_KEY }}" - echo '__EOF__' - echo "TAURI_SIGNING_PRIVATE_KEY_PASSWORD=${{ secrets.TAURI_SIGNING_PRIVATE_KEY_PASSWORD }}" + echo "TAURI_SIGNING_PRIVATE_KEY<<$DELIMITER" + echo "$_SIGNING_KEY" + echo "$DELIMITER" + echo "TAURI_SIGNING_PRIVATE_KEY_PASSWORD=$_SIGNING_KEY_PASSWORD" } >> "$GITHUB_ENV"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release-desktop.yml around lines 257 - 272, The step "Export updater signing env" currently interpolates `${{ secrets.TAURI_SIGNING_PRIVATE_KEY }}` directly into the shell which can break with shell metacharacters or if the secret contains the `__EOF__` delimiter; instead, pass TAURI_SIGNING_PRIVATE_KEY and TAURI_SIGNING_PRIVATE_KEY_PASSWORD into the step via the step's env: mapping (e.g. env: TAURI_SIGNING_PRIVATE_KEY: ${{ secrets.TAURI_SIGNING_PRIVATE_KEY }}), check the presence using the shell variable `$TAURI_SIGNING_PRIVATE_KEY`, and write them into GITHUB_ENV safely (e.g. avoid brittle heredoc delimiters by encoding the key (base64) or using a delimiter generated at runtime) so the variables TAURI_SIGNING_PRIVATE_KEY and TAURI_SIGNING_PRIVATE_KEY_PASSWORD are exported without risk of shell injection or delimiter collision.apps/web/src/components/terminal/Terminal.tsx (1)
1020-1020: IncludingprojectRootPathin dependencies causes unnecessary terminal recreation.When
projectRootPathchanges, the entire terminal is torn down and recreated, disconnecting the WebSocket and losing any in-progress session state. However,projectRootPathonly affects link resolution in the link provider.Consider extracting the link provider registration into a separate effect that can update without destroying the terminal, or use a ref to hold
projectRootPathfor the link handlers.♻️ Alternative approach using ref
+const projectRootPathRef = useRef(projectRootPath); +useEffect(() => { + projectRootPathRef.current = projectRootPath; +}, [projectRootPath]); // In createTerminalLinkProvider call: -createTerminalLinkProvider(terminal, { projectRootPath }, (event, target) => { +createTerminalLinkProvider(terminal, { get projectRootPath() { return projectRootPathRef.current; } }, (event, target) => { // Remove projectRootPath from the main effect's dependencies -}, [sessionId, workspaceId, cwd, projectRootPath]); +}, [sessionId, workspaceId, cwd]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/terminal/Terminal.tsx` at line 1020, The effect that recreates the terminal (the useEffect whose dependency array includes sessionId, workspaceId, cwd, projectRootPath) is tearing down the terminal when projectRootPath changes; move link provider registration out of that effect so changes to projectRootPath don't recreate the terminal. Specifically, keep the terminal creation/cleanup logic in the existing effect but remove any registerLinkProvider/registerLinkHandlers code from it and create a second useEffect (or store projectRootPath in a ref via useRef and read the ref inside the link handler) that solely updates the link provider when projectRootPath changes; update registerLinkProvider/registerLinkHandlers to read projectRootPath from the ref (or be run by the separate effect) so the WebSocket/terminal instance (terminal, socket, etc.) isn't recreated on projectRootPath updates.apps/web/src/components/terminal/terminal-link-routing.ts (1)
263-273: Consider a lightweight existence check instead of reading file content.
fsApi.readFile(candidatePath)reads the entire file content just to determine if it exists. For large files, this is wasteful. If the API supports it, a stat or exists check would be more efficient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/terminal/terminal-link-routing.ts` around lines 263 - 273, The code currently calls fsApi.readFile(candidatePath) inside the lookup loop (see candidatePath handling) just to detect existence; replace that with a lightweight existence/stat check (e.g., fsApi.stat, fsApi.exists, or an fsApi.access-style method if provided) so we don't load file contents for large files, and update the result handling to check existence/nullability from that API (returning the same { type: "file", path: candidatePath } when it exists). Keep the try/catch behavior to ignore lookup failures and fallback to other candidates.apps/web/src/components/agent/AgentManagerView.tsx (1)
51-53: Drop the unnecessaryuseCallbackwrapper onhandleCustomDialogSaved.The
mgr.loadDatacallback is already memoized in theuseAgentManagerhook (has empty dependency array), so wrapping it in anotheruseCallbackadds no value. Simplify to a direct arrow function:const handleCustomDialogSaved = () => { void mgr.loadData(); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/agent/AgentManagerView.tsx` around lines 51 - 53, Remove the unnecessary React.useCallback wrapper around handleCustomDialogSaved: replace the memoized wrapper with a plain arrow function that calls mgr.loadData() directly (keep using void mgr.loadData() to preserve the original async intent). Update the declaration of handleCustomDialogSaved (referenced in AgentManagerView.tsx) to be a simple const handleCustomDialogSaved = () => { void mgr.loadData(); }; and remove useCallback import if it becomes unused.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/components/agent/AgentManagerView.tsx`:
- Around line 216-223: The Installed badge currently hardcodes emerald color
classes inside the TabsTrigger block (see the span rendering the count using
mgr.installedCount + mgr.customAgents.length); replace those hardcoded classes
with the app's semantic theme tokens (e.g., use bg-[semantic-token] /
text-[semantic-token] / border-[semantic-token] equivalents such as
bg-background variants, text-muted-foreground or the project's semantic emerald
tokens and border-border) so the badge respects light/dark theming and the
project's style guidelines; update the span's className accordingly while
preserving sizing, rounding and font rules.
In `@apps/web/src/components/terminal/Terminal.tsx`:
- Around line 713-731: Replace the private xterm.js access (the use of
terminal._core/_mouseService/getCoords in Terminal.tsx) with a calculation using
public APIs: attach your mouse listener to the terminal.element (or viewport),
get the element bounding rect via getBoundingClientRect(), read cell
width/height from terminal.renderer.dimensions.css.cell, compute column =
floor((clientX - rect.left) / cell.width) and row = floor((clientY - rect.top) /
cell.height) and add terminal.buffer.active.ydisp to the row to account for
scroll; optionally cache terminal.renderer.dimensions.css.cell to avoid repeated
measurements for performance.
In `@apps/web/src/hooks/use-updater.ts`:
- Around line 92-95: The current early return when isInstallingUpdate is true
silently drops callers; change the check in the installer function in
use-updater.ts (the block using isInstallingUpdate) to return an explicit
in-progress result or re-play the current updater status to the caller instead
of a bare return. Concretely, replace "if (isInstallingUpdate) return;" with a
branch that either rejects/returns a Promise with a clearly typed status (e.g.
Promise.resolve({ status: 'in-progress' }) or Promise.reject(new
Error('update-in-progress'))) or returns the same status object/state_ref the
hook exposes so callers (e.g. the SettingsModal and UpdateNotification
consumers) can observe and react to the updater being busy. Ensure the returned
value matches the hook's existing return type so callers can handle the
in-progress signal.
---
Outside diff comments:
In `@apps/web/src/components/agent/AgentManagerView.tsx`:
- Around line 277-309: The tab is rendering unfiltered mgr.customAgents
alongside mgr.installedAgents, so search queries still show all custom agents;
filter the custom agents the same way as installed ones before rendering and
counting: create a filteredCustom list (e.g., const filteredCustom =
mgr.customAgents.filter(a => matchesQuery(a, query)) or reuse the existing
installed-agent filtering predicate), replace uses of mgr.customAgents in the
AgentManagerView render with filteredCustom (map to <CustomAgentCard ...
index={mgr.installedAgents.length + i} ...>), and update any tab-count logic
that references mgr.customAgents to use filteredCustom.length so the "No
installed agents matching..." empty state appears correctly.
In `@apps/web/src/hooks/use-updater.ts`:
- Around line 55-65: Move all Tauri I/O out of the hook into an API module:
create an apps/web/src/api/updater.ts that exports async wrapper functions
(e.g., checkForUpdateIO/checkUpdate, downloadAndInstallUpdate, relaunchApp)
which internally import '@tauri-apps/plugin-updater' and invoke the actual
check(), install/download, and relaunch calls (and handle isTauriRuntime guard
inside those wrappers). In use-updater.ts keep the client-side orchestration
(the exported checkForUpdate hook function and any status callbacks) but replace
direct calls to the Tauri plugin with calls to the new api functions (preserve
the same UpdateInfo/UpdateStatus types and function names used in the hook), and
ensure fallbacks/null returns when not running in Tauri.
---
Nitpick comments:
In @.github/workflows/release-desktop.yml:
- Around line 257-272: The step "Export updater signing env" currently
interpolates `${{ secrets.TAURI_SIGNING_PRIVATE_KEY }}` directly into the shell
which can break with shell metacharacters or if the secret contains the
`__EOF__` delimiter; instead, pass TAURI_SIGNING_PRIVATE_KEY and
TAURI_SIGNING_PRIVATE_KEY_PASSWORD into the step via the step's env: mapping
(e.g. env: TAURI_SIGNING_PRIVATE_KEY: ${{ secrets.TAURI_SIGNING_PRIVATE_KEY }}),
check the presence using the shell variable `$TAURI_SIGNING_PRIVATE_KEY`, and
write them into GITHUB_ENV safely (e.g. avoid brittle heredoc delimiters by
encoding the key (base64) or using a delimiter generated at runtime) so the
variables TAURI_SIGNING_PRIVATE_KEY and TAURI_SIGNING_PRIVATE_KEY_PASSWORD are
exported without risk of shell injection or delimiter collision.
In `@apps/web/src/components/agent/AgentManagerView.tsx`:
- Around line 51-53: Remove the unnecessary React.useCallback wrapper around
handleCustomDialogSaved: replace the memoized wrapper with a plain arrow
function that calls mgr.loadData() directly (keep using void mgr.loadData() to
preserve the original async intent). Update the declaration of
handleCustomDialogSaved (referenced in AgentManagerView.tsx) to be a simple
const handleCustomDialogSaved = () => { void mgr.loadData(); }; and remove
useCallback import if it becomes unused.
In `@apps/web/src/components/terminal/terminal-link-routing.ts`:
- Around line 263-273: The code currently calls fsApi.readFile(candidatePath)
inside the lookup loop (see candidatePath handling) just to detect existence;
replace that with a lightweight existence/stat check (e.g., fsApi.stat,
fsApi.exists, or an fsApi.access-style method if provided) so we don't load file
contents for large files, and update the result handling to check
existence/nullability from that API (returning the same { type: "file", path:
candidatePath } when it exists). Keep the try/catch behavior to ignore lookup
failures and fallback to other candidates.
In `@apps/web/src/components/terminal/Terminal.tsx`:
- Line 1020: The effect that recreates the terminal (the useEffect whose
dependency array includes sessionId, workspaceId, cwd, projectRootPath) is
tearing down the terminal when projectRootPath changes; move link provider
registration out of that effect so changes to projectRootPath don't recreate the
terminal. Specifically, keep the terminal creation/cleanup logic in the existing
effect but remove any registerLinkProvider/registerLinkHandlers code from it and
create a second useEffect (or store projectRootPath in a ref via useRef and read
the ref inside the link handler) that solely updates the link provider when
projectRootPath changes; update registerLinkProvider/registerLinkHandlers to
read projectRootPath from the ref (or be run by the separate effect) so the
WebSocket/terminal instance (terminal, socket, etc.) isn't recreated on
projectRootPath updates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 89228bf1-c74d-4122-a4fc-c5b0fb0eed50
📒 Files selected for processing (15)
.github/workflows/release-desktop.ymlapps/web/src/components/agent/AgentManagerView.tsxapps/web/src/components/dialogs/SettingsModal.tsxapps/web/src/components/editor/CodeMirrorEditor.tsxapps/web/src/components/files/FileTree.tsxapps/web/src/components/layout/CenterStage.tsxapps/web/src/components/layout/LeftSidebar.tsxapps/web/src/components/layout/QuickOpen.tsxapps/web/src/components/layout/quick-open-apps.tsxapps/web/src/components/terminal/Terminal.tsxapps/web/src/components/terminal/TerminalGrid.tsxapps/web/src/components/terminal/terminal-link-routing.tsapps/web/src/hooks/use-editor-store.tsapps/web/src/hooks/use-terminal-link-settings.tsapps/web/src/hooks/use-updater.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- apps/web/src/components/layout/LeftSidebar.tsx
- apps/web/src/components/editor/CodeMirrorEditor.tsx
- apps/web/src/hooks/use-terminal-link-settings.ts
- apps/web/src/hooks/use-editor-store.ts
- apps/web/src/components/terminal/TerminalGrid.tsx
- apps/web/src/components/layout/QuickOpen.tsx
- apps/web/src/components/files/FileTree.tsx
- apps/web/src/components/layout/quick-open-apps.tsx
- apps/web/src/components/dialogs/SettingsModal.tsx
| <TabsTrigger value="installed"> | ||
| <Download className="size-4" /> | ||
| Installed | ||
| {!mgr.loading && mgr.installedCount + mgr.customAgents.length > 0 && ( | ||
| <span className="ml-1 rounded-full bg-emerald-500/10 border border-emerald-500/20 px-1.5 text-[10px] font-medium text-emerald-600 dark:text-emerald-400 tabular-nums"> | ||
| {mgr.installedCount + mgr.customAgents.length} | ||
| </span> | ||
| )} |
There was a problem hiding this comment.
Avoid hardcoded palette classes in the new Installed badge.
The added badge styling hardcodes emerald-* values instead of the shared theme tokens used elsewhere in this view. Please switch this to semantic color classes so it stays aligned in both light and dark themes.
As per coding guidelines "ALWAYS use semantic CSS variables (bg-background, text-muted-foreground, border-border, etc.) instead of hardcoded Tailwind colors like bg-zinc-900 or text-gray-500 for layout components".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/agent/AgentManagerView.tsx` around lines 216 - 223,
The Installed badge currently hardcodes emerald color classes inside the
TabsTrigger block (see the span rendering the count using mgr.installedCount +
mgr.customAgents.length); replace those hardcoded classes with the app's
semantic theme tokens (e.g., use bg-[semantic-token] / text-[semantic-token] /
border-[semantic-token] equivalents such as bg-background variants,
text-muted-foreground or the project's semantic emerald tokens and
border-border) so the badge respects light/dark theming and the project's style
guidelines; update the span's className accordingly while preserving sizing,
rounding and font rules.
| const core = (terminal as unknown as { | ||
| _core?: { | ||
| _mouseService?: { | ||
| getCoords: ( | ||
| event: MouseEvent, | ||
| element: HTMLElement, | ||
| colCount: number, | ||
| rowCount: number, | ||
| ) => [number, number] | undefined; | ||
| }; | ||
| }; | ||
| })._core; | ||
|
|
||
| const coords = core?._mouseService?.getCoords( | ||
| event, | ||
| terminal.element ?? container, | ||
| terminal.cols, | ||
| terminal.rows, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
xterm.js get mouse coordinates from click event public API
💡 Result:
xterm.js does not provide a public API method to directly get grid (row/col) coordinates from a raw DOM MouseEvent. Mouse events are primarily handled internally for terminal protocol reporting (e.g., CSI sequences for backend apps like vim/tmux), not exposed as public events with coordinates. To get mouse coordinates from a click event using public APIs: 1. Attach a listener to the viewport element: term.element.querySelector('.xterm-viewport') or term.viewport.viewportElement (noted as non-public in older docs but accessible). 2. In the handler, compute relative position using getBoundingClientRect and event clientX/clientY. 3. Convert pixel offsets to grid coordinates using terminal dimensions: term.renderer.dimensions.css.cell.width and .cell.height for col/row. Example code: const term = new Terminal; term.open(document.getElementById('terminal')); const viewport = term.element.querySelector('.xterm-viewport'); viewport.addEventListener('click', (e) => { const rect = viewport.getBoundingClientRect; const x = e.clientX - rect.left; const y = e.clientY - rect.top; const cellWidth = term.renderer.dimensions.css.cell.width; const cellHeight = term.renderer.dimensions.css.cell.height; const col = Math.floor(x / cellWidth); const row = Math.floor(y / cellHeight) + term.buffer.active.ydisp; // ydisp for absolute buffer row console.log(Clicked at buffer row ${row}, col ${col}); }); Notes: - ydisp accounts for scroll position to get absolute buffer coordinates. - Coordinates are 0-based. - This matches internal logic (e.g., getBoundingClientRect used in selection/mouse handling). - No dedicated public onMouseClick or IMouseEvent API exists (proposed but not implemented). - For selection-related positions, use term.getSelectionPosition which returns IBufferRange with start/end {start: {x, y}, end: {x, y}} (viewport-relative y). This approach is stable, as element, renderer.dimensions, and buffer.ydisp are public.
Citations:
- 1: API mouse support xtermjs/xterm.js#2336
- 2: [Question] Listen for mouse and scroll events xtermjs/xterm.js#657
- 3: Get the mouse position when onSelectionChange xtermjs/xterm.js#5152
- 4: http://xtermjs.org/docs/api/terminal/classes/terminal/
- 5: https://github.com/xtermjs/xterm.js/
- 6: https://xtermjs.org/docs/api/terminal/classes/terminal
- 7: getCoordsRelativeToElement: use getBoundingClientRect and clientX/Y. xtermjs/xterm.js#1788
- 8: https://github.com/xtermjs/xterm.js/blob/master/src/browser/Viewport.ts
- 9: https://xtermjs.org/docs/api/terminal/interfaces/iviewportrangeposition/
Replace private xterm.js API with public alternatives for stability.
The code accesses terminal._core._mouseService.getCoords(), a private internal API with no public equivalent in xterm.js. While xterm.js provides no dedicated public API for getting grid coordinates from mouse events, you can achieve this using stable public APIs:
Use term.element, term.renderer.dimensions.css.cell, and term.buffer.active.ydisp to manually calculate coordinates instead:
- Attach listener to viewport element
- Get pixel offsets via
getBoundingClientRect() - Divide by cell dimensions to get grid position
- Add
ydispto account for scroll position
This approach avoids private API fragility while remaining stable across xterm.js versions. If performance is critical, consider caching cell dimensions to reduce recalculation overhead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/terminal/Terminal.tsx` around lines 713 - 731,
Replace the private xterm.js access (the use of
terminal._core/_mouseService/getCoords in Terminal.tsx) with a calculation using
public APIs: attach your mouse listener to the terminal.element (or viewport),
get the element bounding rect via getBoundingClientRect(), read cell
width/height from terminal.renderer.dimensions.css.cell, compute column =
floor((clientX - rect.left) / cell.width) and row = floor((clientY - rect.top) /
cell.height) and add terminal.buffer.active.ydisp to the row to account for
scroll; optionally cache terminal.renderer.dimensions.css.cell to avoid repeated
measurements for performance.
| if (isInstallingUpdate) return; | ||
|
|
||
| try { | ||
| isInstallingUpdate = true; |
There was a problem hiding this comment.
Don’t silently drop callers when an install is already running.
Line 92 returns without notifying the caller, so other entry points never learn that the updater is busy. apps/web/src/components/dialogs/SettingsModal.tsx at Lines 101-177 only clears installInFlightRef.current from callback stages, and apps/web/src/components/layout/UpdateNotification.tsx at Lines 30-38 disables its button from the same status stream. Replay the current updater status here, or return an explicit in-progress result, instead of a silent no-op.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/hooks/use-updater.ts` around lines 92 - 95, The current early
return when isInstallingUpdate is true silently drops callers; change the check
in the installer function in use-updater.ts (the block using isInstallingUpdate)
to return an explicit in-progress result or re-play the current updater status
to the caller instead of a bare return. Concretely, replace "if
(isInstallingUpdate) return;" with a branch that either rejects/returns a
Promise with a clearly typed status (e.g. Promise.resolve({ status:
'in-progress' }) or Promise.reject(new Error('update-in-progress'))) or returns
the same status object/state_ref the hook exposes so callers (e.g. the
SettingsModal and UpdateNotification consumers) can observe and react to the
updater being busy. Ensure the returned value matches the hook's existing return
type so callers can handle the in-progress signal.
| const loadDirectoryChildren = useCallback(async (itemPath: string): Promise<string[]> => { | ||
| const existingItem = initialItemsMap.get(itemPath) || lazyItemsMap.get(itemPath); | ||
| if (!existingItem?.isDir) return []; | ||
|
|
||
| if (existingItem.children && existingItem.children.length > 0) { | ||
| return existingItem.children; | ||
| } | ||
|
|
||
| const response = await fsApi.listDir(itemPath, { showHidden: true, dirsOnly: false }); | ||
|
|
||
| const newChildren = response.entries.map((entry) => entry.path); | ||
| const newEntriesMap = new Map<string, FileTreeItem>(); | ||
|
|
||
| response.entries.forEach((entry) => { | ||
| newEntriesMap.set(entry.path, { | ||
| id: entry.path, | ||
| name: entry.name, | ||
| path: entry.path, | ||
| isDir: entry.is_dir, | ||
| isSymlink: entry.is_symlink, | ||
| isIgnored: entry.is_ignored, | ||
| symlinkTarget: entry.symlink_target, | ||
| }); | ||
| }); | ||
|
|
||
| setLazyItemsMap((prev: Map<string, FileTreeItem>) => { | ||
| const next = new Map(prev); | ||
| newEntriesMap.forEach((val, key) => next.set(key, val)); | ||
|
|
||
| const parent = initialItemsMap.get(itemPath) || next.get(itemPath); | ||
| if (parent) { | ||
| next.set(itemPath, { ...parent, children: newChildren }); | ||
| } | ||
|
|
||
| return next; | ||
| }); | ||
|
|
||
| return newChildren; | ||
| }, [initialItemsMap, lazyItemsMap]); |
There was a problem hiding this comment.
🟡 loadDirectoryChildren dependency on lazyItemsMap causes reveal effect to restart on each directory load
loadDirectoryChildren includes lazyItemsMap in its useCallback dependency array (FileTree.tsx:138). Every call to loadDirectoryChildren updates lazyItemsMap via setLazyItemsMap (FileTree.tsx:125), which creates a new Map, causing the callback reference to change. Since loadDirectoryChildren is in the reveal effect's dependency array (FileTree.tsx:283), the effect restarts on each directory load.
Within a single effect execution, when the loop calls loadDirectoryChildren for a parent directory, the resulting setLazyItemsMap update hasn't been committed yet. The next iteration tries to load a child directory, but the stale lazyItemsMap closure doesn't contain the just-loaded children — so existingItem is undefined and the function returns [] for a valid directory. The effect then gets cancelled when React processes the batched state update and restarts from scratch. For a path N levels deep, this requires ~N restarts, each re-traversing from the root.
Prompt for agents
In apps/web/src/components/files/FileTree.tsx, the loadDirectoryChildren useCallback at line 100-138 depends on lazyItemsMap (line 138), which changes every time a directory is lazily loaded. This causes the callback reference to change on each load, triggering the reveal effect (line 206-283) to restart.
Fix: Use a ref to access lazyItemsMap inside the callback instead of capturing it in the closure. Add a ref like:
const lazyItemsMapRef = useRef(lazyItemsMap);
useEffect(() => { lazyItemsMapRef.current = lazyItemsMap; }, [lazyItemsMap]);
Then in loadDirectoryChildren, replace:
const existingItem = initialItemsMap.get(itemPath) || lazyItemsMap.get(itemPath);
with:
const existingItem = initialItemsMap.get(itemPath) || lazyItemsMapRef.current.get(itemPath);
And change the dependency array from [initialItemsMap, lazyItemsMap] to [initialItemsMap].
This keeps the callback reference stable while still reading the latest lazyItemsMap, preventing the reveal effect from restarting on each directory load.
Was this helpful? React with 👍 or 👎 to provide feedback.
This branch was successfully deployed
1 participant
Summary
This PR stabilizes the desktop release/update pipeline, fixes desktop updater install behavior, and improves terminal link routing and related settings in the web app.
Related completed issue:
Feature
Bugfix
Installreuses the checked update instead of silently no-oping.Refactor
Validation
cd apps/web && bun typecheckNotes
Summary by CodeRabbit
New Features
Improvements