Ghostty Web#2115
Closed
Kitenite wants to merge 10 commits into
Closed
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR transitions the terminal implementation from xterm.js with multiple addons to ghostty-web, introduces Claude notification hook support in the agent setup with event type mapping, and refactors terminal font management, search handling, and lifecycle coordination. Multiple terminal utilities are updated to support the new architecture. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Terminal Client
participant Hook as notify-hook.sh
participant Server as Notification Server
Client->>Hook: Execute hook with event data<br/>(hook_event_name, notificationType, etc.)
Hook->>Hook: Extract fields via extract_json_field()<br/>SESSION_ID, EVENT_TYPE, NOTIFICATION_TYPE
Hook->>Hook: Map notification subtypes<br/>(permission_prompt → PermissionRequest,<br/>idle_prompt → Stop, etc.)
Hook->>Server: POST /dispatch with eventType<br/>and notificationType parameter
Server->>Server: mapEventType(eventType)
Server->>Server: If falsy, fallback to<br/>mapEventType(notificationType)
Server->>Server: Route to appropriate<br/>notification handler
Server-->>Hook: 200 OK
Hook-->>Client: Hook execution complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
apps/desktop/src/main/lib/notifications/server.test.ts (1)
46-51: Cover the raw normalized subtype forms too.
mapEventType()now accepts no-underscore variants after lowercasing (permissionprompt,elicitationdialog,idleprompt,authsuccess), but this case only exercises the underscored inputs. Add at least one mixed-case/no-underscore sample per bucket so the server-side fallback path can't regress silently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/notifications/server.test.ts` around lines 46 - 51, The test only checks underscored Claude subtypes but mapEventType also accepts lowercased no-underscore variants (e.g., permissionprompt, elicitationdialog, idleprompt, authsuccess), so extend the test in server.test.ts to include at least one no-underscore/mixed-case sample per mapping bucket to ensure the server-side fallback path is exercised; update the "should map Claude notification subtypes" test to call mapEventType with both underscored and normalized forms (referencing mapEventType) and assert the same expected outputs.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/ghostty-runtime.ts (1)
10-26: Add a small regression test for the init gate.This module’s behavior lives in the shared module state: concurrent callers should share one
init(), failures should clear the cached promise, and the next call should retry. A focused test around those three cases would make this bootstrap path much safer to change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/ghostty-runtime.ts` around lines 10 - 26, Add a focused regression test for the init gate: write tests that call ensureGhosttyReady concurrently to verify multiple callers share a single init() invocation, simulate init() rejecting to ensure ghosttyInitPromise is cleared and ghosttyReady stays false, and then call ensureGhosttyReady again to verify a retry invokes init() a second time; reference ensureGhosttyReady, init, ghosttyInitPromise and ghosttyReady in the test to assert the shared-promise behavior, failure-clearance, and retry semantics.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.test.ts (1)
116-158: Remove the wall-clock dependency from this regression test.The retry assertion still depends on a real-time sleep, which makes it slower and more timing-sensitive in CI. If
makeScheduler()accepts injected time/timer primitives, this case can drive the retry path deterministically without waiting on wall clock time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.test.ts` around lines 116 - 158, The test depends on real wall-clock sleep; change it to use deterministic timers by updating makeScheduler to accept injected time/timer primitives (e.g., a mock setTimeout/Date.now or a clock object) and in the test provide a fake clock so you can advance time instead of awaiting setTimeout; specifically, call makeScheduler with the injected clock, set state.lastRunAt relative to the fake clock, call schedule(false) and flush(), then advance the fake clock by the remaining throttle window (e.g., 70ms) to trigger the scheduled retry and call flush() again to assert calls === 1, referencing makeScheduler, schedule, flush and state.lastRunAt to locate the changes.apps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.sh (1)
45-59: Avoid keeping a second notification mapping table here.This block re-encodes part of
mapEventType()in the hook, and the two tables are already different (permissionprompt/authsuccessare only handled server-side). Forward the rawhook_event_name/notificationTypeand let the server stay authoritative; otherwise hook debug output and final classification can drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.sh` around lines 45 - 59, The hook currently re-maps notification subtypes (using NOTIFICATION_TYPE, EVENT_TYPE and the case block after calling extract_json_field) which duplicates and diverges from server-side mapEventType(); remove the case mapping that overrides EVENT_TYPE (the permission_prompt/idle_prompt branches) so the script preserves the original EVENT_TYPE="Notification" and raw NOTIFICATION_TYPE (from extract_json_field or matcher) and forwards hook_event_name/notificationType to the server unchanged; keep the extraction logic (NOTIFICATION_TYPE=$(extract_json_field ...)) but do not mutate EVENT_TYPE in this template.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (1)
448-454: LetResizeObserverown resize detection here.The extra
window.resizelistener makes each window resize hit this path even when the terminal container size did not change, and duplicates the observer path when it did. Dropping it keeps resize work aligned with actual pane size changes.💡 Suggested simplification
const resizeObserver = new ResizeObserver(handleResize); resizeObserver.observe(container); - window.addEventListener("resize", handleResize); return () => { - window.removeEventListener("resize", handleResize); resizeObserver.disconnect(); }; }Based on learnings, in the superset desktop app's flex-based mosaic layout (WorkspaceView/BrowserPane), every position change of a pane also triggers a size change, so ResizeObserver alone is sufficient to keep the persistent webview bounds synchronized without needing additional scroll or window resize listeners.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts` around lines 448 - 454, Remove the extra global window resize listener and rely solely on ResizeObserver for resize detection: delete the window.addEventListener("resize", handleResize) call and its corresponding window.removeEventListener("resize", handleResize) cleanup, keeping ResizeObserver(handleResize), resizeObserver.observe(container), and resizeObserver.disconnect() intact so handleResize is only invoked by the ResizeObserver.
🤖 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/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/font-family.ts`:
- Around line 31-35: The splitFontFamilyList function currently uses
value.split(",") which breaks quoted font names like "My Font, VF"; update
splitFontFamilyList to parse the CSS font-family string by iterating characters
and splitting only on commas that are not inside single or double quotes and not
escaped — i.e., implement a small state machine tracking current quote char and
escape sequences, accumulate characters into a buffer, on an unquoted comma push
the trimmed buffer (and strip surrounding quotes) into the result array, and
after the loop push any remaining buffer; ensure handling of both single and
double quotes and backslash escapes so quoted families remain intact.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts`:
- Around line 243-249: The deferred zero-delay blur can steal focus if the pane
becomes focused before the timer fires; inside the setTimeout callback in
useTerminalLifecycle, re-check the current focus state via isFocusedRef.current
(in addition to existing isUnmounted and xtermRef.current checks) and bail out
if the pane is now focused so blurTerminalInput(xterm) is skipped; reference
blurTerminalInput, xtermRef, isUnmounted and isFocusedRef to locate and update
the timeout guard.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalStream.ts`:
- Around line 59-72: The flushPendingData function can return early without
clearing pendingWriteBufferRef, allowing buffered bytes to later flush into a
new terminal; to fix, capture the terminal instance and readiness at the top
(e.g., const activeTerminal = xtermRef.current; const ready =
isStreamReadyRef.current) and if activeTerminal is null or ready is false then
clear/discard pendingWriteBufferRef.current before returning, or alternatively
bind the pending buffer to the captured terminal instance and only flush when
the same instance is still current; update references to updateModesRef and
updateCwdRef so they run only after confirming the captured activeTerminal is
valid.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx`:
- Around line 130-140: The current useEffect swallows ensureGhosttyReady()
rejections and never updates component state, leaving the terminal blank; update
the effect to capture the error and set a renderer error state (e.g., add or use
a state setter like setRendererError / setIsRendererError) inside the .catch so
the component can render an explicit fallback and a retry action; also expose a
retry handler (e.g., retryInitialize that calls ensureGhosttyReady and updates
setIsRendererReady and clears the error) and ensure the render path checks
isRendererReady and rendererError to show either the terminal, an error message,
and a retry button.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/TerminalSearch/terminal-search-adapter.ts`:
- Around line 128-133: The previous-search logic in findPrevious is reselecting
the current match because startColumn is set to the match end; change the anchor
so the search begins before the current match: when hasSameQuery && lastMatch
and direction === "previous" set startColumn = lastMatch.column - 1 (or
otherwise compare against lastMatch.column + lastMatch.length to exclude the
current span) so findPreviousInLine will skip the current match and advance to
the prior hit; update references in findPrevious/lastMatch/startColumn
accordingly.
---
Nitpick comments:
In `@apps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.sh`:
- Around line 45-59: The hook currently re-maps notification subtypes (using
NOTIFICATION_TYPE, EVENT_TYPE and the case block after calling
extract_json_field) which duplicates and diverges from server-side
mapEventType(); remove the case mapping that overrides EVENT_TYPE (the
permission_prompt/idle_prompt branches) so the script preserves the original
EVENT_TYPE="Notification" and raw NOTIFICATION_TYPE (from extract_json_field or
matcher) and forwards hook_event_name/notificationType to the server unchanged;
keep the extraction logic (NOTIFICATION_TYPE=$(extract_json_field ...)) but do
not mutate EVENT_TYPE in this template.
In `@apps/desktop/src/main/lib/notifications/server.test.ts`:
- Around line 46-51: The test only checks underscored Claude subtypes but
mapEventType also accepts lowercased no-underscore variants (e.g.,
permissionprompt, elicitationdialog, idleprompt, authsuccess), so extend the
test in server.test.ts to include at least one no-underscore/mixed-case sample
per mapping bucket to ensure the server-side fallback path is exercised; update
the "should map Claude notification subtypes" test to call mapEventType with
both underscored and normalized forms (referencing mapEventType) and assert the
same expected outputs.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/ghostty-runtime.ts`:
- Around line 10-26: Add a focused regression test for the init gate: write
tests that call ensureGhosttyReady concurrently to verify multiple callers share
a single init() invocation, simulate init() rejecting to ensure
ghosttyInitPromise is cleared and ghosttyReady stays false, and then call
ensureGhosttyReady again to verify a retry invokes init() a second time;
reference ensureGhosttyReady, init, ghosttyInitPromise and ghosttyReady in the
test to assert the shared-promise behavior, failure-clearance, and retry
semantics.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts`:
- Around line 448-454: Remove the extra global window resize listener and rely
solely on ResizeObserver for resize detection: delete the
window.addEventListener("resize", handleResize) call and its corresponding
window.removeEventListener("resize", handleResize) cleanup, keeping
ResizeObserver(handleResize), resizeObserver.observe(container), and
resizeObserver.disconnect() intact so handleResize is only invoked by the
ResizeObserver.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.test.ts`:
- Around line 116-158: The test depends on real wall-clock sleep; change it to
use deterministic timers by updating makeScheduler to accept injected time/timer
primitives (e.g., a mock setTimeout/Date.now or a clock object) and in the test
provide a fake clock so you can advance time instead of awaiting setTimeout;
specifically, call makeScheduler with the injected clock, set state.lastRunAt
relative to the fake clock, call schedule(false) and flush(), then advance the
fake clock by the remaining throttle window (e.g., 70ms) to trigger the
scheduled retry and call flush() again to assert calls === 1, referencing
makeScheduler, schedule, flush and state.lastRunAt to locate the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 58ed4b39-7e68-40ff-808e-97d6361783f5
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
apps/desktop/package.jsonapps/desktop/src/main/lib/agent-setup/agent-wrappers-claude-codex-opencode.tsapps/desktop/src/main/lib/agent-setup/agent-wrappers.test.tsapps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.shapps/desktop/src/main/lib/notifications/map-event-type.tsapps/desktop/src/main/lib/notifications/server.test.tsapps/desktop/src/main/lib/notifications/server.tsapps/desktop/src/renderer/globals.cssapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/ScrollToBottomButton/ScrollToBottomButton.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/TerminalSearch/TerminalSearch.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/TerminalSearch/terminal-search-adapter.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/config.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/font-family.test.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/font-family.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/ghostty-runtime.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.test.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalColdRestore.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalConnection.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalHotkeys.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.test.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalRestore.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalStream.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/suppressQueryResponses.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/types.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/utils.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsxapps/desktop/src/renderer/stores/hotkeys/store.tsapps/desktop/src/renderer/stores/tabs/store.ts
💤 Files with no reviewable changes (2)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalHotkeys.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/suppressQueryResponses.ts
Comment on lines
+31
to
+35
| export function splitFontFamilyList(value: string): string[] { | ||
| return value | ||
| .split(",") | ||
| .map((part) => part.trim()) | ||
| .filter(Boolean); |
Contributor
There was a problem hiding this comment.
Don't split CSS font lists on raw commas.
Quoted families like "My Font, VF", monospace are valid, but value.split(",") turns them into broken entries. That cascades into wrong formatting, bad availability probes, and fallback reordering across the rest of this module.
💡 Suggested parser fix
export function splitFontFamilyList(value: string): string[] {
- return value
- .split(",")
- .map((part) => part.trim())
- .filter(Boolean);
+ const parts: string[] = [];
+ let current = "";
+ let quote: '"' | "'" | null = null;
+ let escaped = false;
+
+ for (const char of value) {
+ if (escaped) {
+ current += char;
+ escaped = false;
+ continue;
+ }
+
+ if (quote) {
+ current += char;
+ if (char === "\\") escaped = true;
+ else if (char === quote) quote = null;
+ continue;
+ }
+
+ if (char === '"' || char === "'") {
+ quote = char;
+ current += char;
+ continue;
+ }
+
+ if (char === ",") {
+ const part = current.trim();
+ if (part) parts.push(part);
+ current = "";
+ continue;
+ }
+
+ current += char;
+ }
+
+ const part = current.trim();
+ if (part) parts.push(part);
+ return parts;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function splitFontFamilyList(value: string): string[] { | |
| return value | |
| .split(",") | |
| .map((part) => part.trim()) | |
| .filter(Boolean); | |
| export function splitFontFamilyList(value: string): string[] { | |
| const parts: string[] = []; | |
| let current = ""; | |
| let quote: '"' | "'" | null = null; | |
| let escaped = false; | |
| for (const char of value) { | |
| if (escaped) { | |
| current += char; | |
| escaped = false; | |
| continue; | |
| } | |
| if (quote) { | |
| current += char; | |
| if (char === "\\") escaped = true; | |
| else if (char === quote) quote = null; | |
| continue; | |
| } | |
| if (char === '"' || char === "'") { | |
| quote = char; | |
| current += char; | |
| continue; | |
| } | |
| if (char === ",") { | |
| const part = current.trim(); | |
| if (part) parts.push(part); | |
| current = ""; | |
| continue; | |
| } | |
| current += char; | |
| } | |
| const part = current.trim(); | |
| if (part) parts.push(part); | |
| return parts; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/font-family.ts`
around lines 31 - 35, The splitFontFamilyList function currently uses
value.split(",") which breaks quoted font names like "My Font, VF"; update
splitFontFamilyList to parse the CSS font-family string by iterating characters
and splitting only on commas that are not inside single or double quotes and not
escaped — i.e., implement a small state machine tracking current quote char and
escape sequences, accumulate characters into a buffer, on an unquoted comma push
the trimmed buffer (and strip surrounding quotes) into the result array, and
after the loop push any remaining buffer; ensure handling of both single and
double quotes and backslash escapes so quoted families remain intact.
Comment on lines
+243
to
+249
| } else { | ||
| // ghostty-web focuses during open(); counter that for background panes. | ||
| blurTerminalInput(xterm); | ||
| setTimeout(() => { | ||
| if (isUnmounted || xtermRef.current !== xterm) return; | ||
| blurTerminalInput(xterm); | ||
| }, 0); |
Contributor
There was a problem hiding this comment.
Re-check focus before the deferred blur.
The zero-delay blur still runs if this pane becomes focused before the timer fires, so a newly activated terminal can immediately lose focus again. Guard the timeout with the latest isFocusedRef.current.
💡 Suggested fix
setTimeout(() => {
- if (isUnmounted || xtermRef.current !== xterm) return;
+ if (
+ isUnmounted ||
+ xtermRef.current !== xterm ||
+ isFocusedRef.current
+ ) {
+ return;
+ }
blurTerminalInput(xterm);
}, 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts`
around lines 243 - 249, The deferred zero-delay blur can steal focus if the pane
becomes focused before the timer fires; inside the setTimeout callback in
useTerminalLifecycle, re-check the current focus state via isFocusedRef.current
(in addition to existing isUnmounted and xtermRef.current checks) and bail out
if the pane is now focused so blurTerminalInput(xterm) is skipped; reference
blurTerminalInput, xtermRef, isUnmounted and isFocusedRef to locate and update
the timeout guard.
Comment on lines
+59
to
+72
| const flushPendingData = useCallback(() => { | ||
| isWriteFlushScheduledRef.current = false; | ||
|
|
||
| const pending = pendingWriteBufferRef.current; | ||
| if (!pending) return; | ||
|
|
||
| const activeTerminal = xtermRef.current; | ||
| if (!activeTerminal || !isStreamReadyRef.current) return; | ||
|
|
||
| pendingWriteBufferRef.current = ""; | ||
| updateModesRef.current(pending); | ||
| activeTerminal.write(pending); | ||
| updateCwdRef.current(pending); | ||
| }, [xtermRef, isStreamReadyRef]); |
Contributor
There was a problem hiding this comment.
Don't let buffered output survive terminal replacement.
If a queued microtask reaches this function after xtermRef.current is cleared/replaced or isStreamReadyRef.current flips false, the function returns without clearing pendingWriteBufferRef. Those bytes can then flush into the next terminal instance on a later event, reordering output across sessions. Clear/discard the buffer on this path, or associate the buffer with the terminal instance that produced it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalStream.ts`
around lines 59 - 72, The flushPendingData function can return early without
clearing pendingWriteBufferRef, allowing buffered bytes to later flush into a
new terminal; to fix, capture the terminal instance and readiness at the top
(e.g., const activeTerminal = xtermRef.current; const ready =
isStreamReadyRef.current) and if activeTerminal is null or ready is false then
clear/discard pendingWriteBufferRef.current before returning, or alternatively
bind the pending buffer to the captured terminal instance and only flush when
the same instance is still current; update references to updateModesRef and
updateCwdRef so they run only after confirming the captured activeTerminal is
valid.
Comment on lines
+130
to
+140
| useEffect(() => { | ||
| let cancelled = false; | ||
| ensureGhosttyReady() | ||
| .then(() => { | ||
| if (!cancelled) { | ||
| setIsRendererReady(true); | ||
| } | ||
| }) | ||
| .catch((error) => { | ||
| console.error("[Terminal] Failed to initialize ghostty-web:", error); | ||
| }); |
Contributor
There was a problem hiding this comment.
Don't turn renderer init failures into a silent blank terminal.
If ensureGhosttyReady() rejects, this branch only logs and leaves isRendererReady false. The component never surfaces that failure or offers a retry path, so terminal startup can fail with an empty pane and no actionable feedback. Please route this through terminal error state or render an explicit fallback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx`
around lines 130 - 140, The current useEffect swallows ensureGhosttyReady()
rejections and never updates component state, leaving the terminal blank; update
the effect to capture the error and set a renderer error state (e.g., add or use
a state setter like setRendererError / setIsRendererError) inside the .catch so
the component can render an explicit fallback and a retry action; also expose a
retry handler (e.g., retryInitialize that calls ensureGhosttyReady and updates
setIsRendererReady and clears the error) and ensure the render path checks
isRendererReady and rendererError to show either the terminal, an error message,
and a retry button.
Comment on lines
+128
to
+133
| } else if (hasSameQuery && lastMatch) { | ||
| startRow = lastMatch.row; | ||
| startColumn = | ||
| direction === "next" | ||
| ? lastMatch.column + 1 | ||
| : lastMatch.column + lastMatch.length - 1; |
Contributor
There was a problem hiding this comment.
findPrevious can reselect the current match forever.
When the same query is repeated, the previous-search anchor is set to the end of lastMatch. findPreviousInLine() only checks the match start column, so the current match still qualifies and repeated “previous” calls never move backward. Start from lastMatch.column - 1 (or compare against the match end) so the search actually advances to the prior hit.
♻️ Minimal fix
} else if (hasSameQuery && lastMatch) {
startRow = lastMatch.row;
startColumn =
direction === "next"
? lastMatch.column + 1
- : lastMatch.column + lastMatch.length - 1;
+ : lastMatch.column - 1;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } else if (hasSameQuery && lastMatch) { | |
| startRow = lastMatch.row; | |
| startColumn = | |
| direction === "next" | |
| ? lastMatch.column + 1 | |
| : lastMatch.column + lastMatch.length - 1; | |
| } else if (hasSameQuery && lastMatch) { | |
| startRow = lastMatch.row; | |
| startColumn = | |
| direction === "next" | |
| ? lastMatch.column + 1 | |
| : lastMatch.column - 1; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/TerminalSearch/terminal-search-adapter.ts`
around lines 128 - 133, The previous-search logic in findPrevious is reselecting
the current match because startColumn is set to the match end; change the anchor
so the search begins before the current match: when hasSameQuery && lastMatch
and direction === "previous" set startColumn = lastMatch.column - 1 (or
otherwise compare against lastMatch.column + lastMatch.length to exclude the
current span) so findPreviousInLine will skip the current match and advance to
the prior hit; update references in findPrevious/lastMatch/startColumn
accordingly.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
4 issues found across 33 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/font-family.ts">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/font-family.ts:33">
P2: Font-family parsing is incorrect for quoted names containing commas because the list is split on every comma.</violation>
</file>
<file name="apps/desktop/src/main/lib/notifications/server.ts">
<violation number="1" location="apps/desktop/src/main/lib/notifications/server.ts:139">
P2: Validate `notificationType` is a string before passing it to `mapEventType`; the current type assertion can trigger a runtime `trim` error on non-string query values.</violation>
</file>
<file name="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/TerminalSearch/terminal-search-adapter.ts">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/TerminalSearch/terminal-search-adapter.ts:122">
P2: Selection coordinates are treated as 0-based even though xterm returns 1-based buffer positions, causing off-by-one search navigation after a selection.</violation>
</file>
<file name="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts:563">
P2: Add `.catch` handler to the `runResize()` floating promise.
(Based on your team's feedback about handling async calls with proper await and catch.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| export function splitFontFamilyList(value: string): string[] { | ||
| return value | ||
| .split(",") |
Contributor
There was a problem hiding this comment.
P2: Font-family parsing is incorrect for quoted names containing commas because the list is split on every comma.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/font-family.ts, line 33:
<comment>Font-family parsing is incorrect for quoted names containing commas because the list is split on every comma.</comment>
<file context>
@@ -0,0 +1,180 @@
+
+export function splitFontFamilyList(value: string): string[] {
+ return value
+ .split(",")
+ .map((part) => part.trim())
+ .filter(Boolean);
</file context>
| const mappedEventType = mapEventType(eventType as string | undefined); | ||
| const mappedEventType = | ||
| mapEventType(eventType as string | undefined) ?? | ||
| mapEventType(notificationType as string | undefined); |
Contributor
There was a problem hiding this comment.
P2: Validate notificationType is a string before passing it to mapEventType; the current type assertion can trigger a runtime trim error on non-string query values.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/lib/notifications/server.ts, line 139:
<comment>Validate `notificationType` is a string before passing it to `mapEventType`; the current type assertion can trigger a runtime `trim` error on non-string query values.</comment>
<file context>
@@ -133,7 +134,9 @@ app.get("/hook/complete", (req, res) => {
- const mappedEventType = mapEventType(eventType as string | undefined);
+ const mappedEventType =
+ mapEventType(eventType as string | undefined) ??
+ mapEventType(notificationType as string | undefined);
// Unknown or missing eventType: return success but don't process
</file context>
Suggested change
| mapEventType(notificationType as string | undefined); | |
| mapEventType( | |
| typeof notificationType === "string" ? notificationType : undefined, | |
| ); |
|
|
||
| if (selection) { | ||
| if (direction === "next") { | ||
| startRow = selection.end.y; |
Contributor
There was a problem hiding this comment.
P2: Selection coordinates are treated as 0-based even though xterm returns 1-based buffer positions, causing off-by-one search navigation after a selection.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/TerminalSearch/terminal-search-adapter.ts, line 122:
<comment>Selection coordinates are treated as 0-based even though xterm returns 1-based buffer positions, causing off-by-one search navigation after a selection.</comment>
<file context>
@@ -0,0 +1,174 @@
+
+ if (selection) {
+ if (direction === "next") {
+ startRow = selection.end.y;
+ startColumn = selection.end.x + 1;
+ } else {
</file context>
| }); | ||
| }; | ||
|
|
||
| void runResize().finally(() => { |
Contributor
There was a problem hiding this comment.
P2: Add .catch handler to the runResize() floating promise.
(Based on your team's feedback about handling async calls with proper await and catch.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts, line 563:
<comment>Add `.catch` handler to the `runResize()` floating promise.
(Based on your team's feedback about handling async calls with proper await and catch.) </comment>
<file context>
@@ -516,14 +490,94 @@ export function useTerminalLifecycle({
+ });
+ };
+
+ void runResize().finally(() => {
+ resizeInFlight = false;
+ if (!pendingResize) return;
</file context>
Suggested change
| void runResize().finally(() => { | |
| void runResize() | |
| .catch((error) => console.warn("[Terminal] Failed to run resize:", error)) | |
| .finally(() => { |
Contributor
There was a problem hiding this comment.
4 issues found across 55 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/main/lib/terminal-host/types.ts">
<violation number="1" location="apps/desktop/src/main/lib/terminal-host/types.ts:176">
P2: `CreateOrAttachResponse.sessionGeneration` is required even though protocol changes are supposed to be additive-only. If a new client talks to an older daemon that doesn’t send this field, the type promises a string but runtime will be `undefined`. Make this optional (or bump the protocol version and handle missing fields explicitly).</violation>
</file>
<file name="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/TerminalSearch/terminal-search-adapter.ts">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/TerminalSearch/terminal-search-adapter.ts:138">
P2: Search wrap-around is incomplete: the starting row is only partially scanned once, so valid matches can be missed.</violation>
</file>
<file name="apps/desktop/src/main/lib/notifications/server.ts">
<violation number="1" location="apps/desktop/src/main/lib/notifications/server.ts:139">
P2: Validate `notificationType` is a string before mapping; otherwise malformed query input can throw at runtime in `mapEventType`.</violation>
</file>
<file name="apps/desktop/src/lib/trpc/routers/terminal/terminal.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/terminal/terminal.ts:480">
P1: The `onExit` handler signature was changed to expect a `payload` object, but `terminal.emit` in the `write` procedure still uses positional arguments.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| signal?: number, | ||
| reason?: "killed" | "exited" | "error", | ||
| ) => { | ||
| const onExit = (payload: { |
Contributor
There was a problem hiding this comment.
P1: The onExit handler signature was changed to expect a payload object, but terminal.emit in the write procedure still uses positional arguments.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/lib/trpc/routers/terminal/terminal.ts, line 480:
<comment>The `onExit` handler signature was changed to expect a `payload` object, but `terminal.emit` in the `write` procedure still uses positional arguments.</comment>
<file context>
@@ -437,52 +438,64 @@ export const createTerminalRouter = () => {
- signal?: number,
- reason?: "killed" | "exited" | "error",
- ) => {
+ const onExit = (payload: {
+ exitCode: number;
+ signal?: number;
</file context>
| isNew: boolean; | ||
| snapshot: TerminalSnapshot; | ||
| wasRecovered: boolean; | ||
| sessionGeneration: string; |
Contributor
There was a problem hiding this comment.
P2: CreateOrAttachResponse.sessionGeneration is required even though protocol changes are supposed to be additive-only. If a new client talks to an older daemon that doesn’t send this field, the type promises a string but runtime will be undefined. Make this optional (or bump the protocol version and handle missing fields explicitly).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/lib/terminal-host/types.ts, line 176:
<comment>`CreateOrAttachResponse.sessionGeneration` is required even though protocol changes are supposed to be additive-only. If a new client talks to an older daemon that doesn’t send this field, the type promises a string but runtime will be `undefined`. Make this optional (or bump the protocol version and handle missing fields explicitly).</comment>
<file context>
@@ -173,6 +173,7 @@ export interface CreateOrAttachResponse {
isNew: boolean;
snapshot: TerminalSnapshot;
wasRecovered: boolean;
+ sessionGeneration: string;
/** PTY process ID for port scanning (null if not yet spawned or exited) */
pid: number | null;
</file context>
|
|
||
| startRow = Math.max(0, Math.min(totalRows - 1, startRow)); | ||
|
|
||
| for (let index = 0; index < totalRows; index++) { |
Contributor
There was a problem hiding this comment.
P2: Search wrap-around is incomplete: the starting row is only partially scanned once, so valid matches can be missed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/TerminalSearch/terminal-search-adapter.ts, line 138:
<comment>Search wrap-around is incomplete: the starting row is only partially scanned once, so valid matches can be missed.</comment>
<file context>
@@ -0,0 +1,174 @@
+
+ startRow = Math.max(0, Math.min(totalRows - 1, startRow));
+
+ for (let index = 0; index < totalRows; index++) {
+ const row =
+ direction === "next"
</file context>
| const mappedEventType = mapEventType(eventType as string | undefined); | ||
| const mappedEventType = | ||
| mapEventType(eventType as string | undefined) ?? | ||
| mapEventType(notificationType as string | undefined); |
Contributor
There was a problem hiding this comment.
P2: Validate notificationType is a string before mapping; otherwise malformed query input can throw at runtime in mapEventType.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/lib/notifications/server.ts, line 139:
<comment>Validate `notificationType` is a string before mapping; otherwise malformed query input can throw at runtime in `mapEventType`.</comment>
<file context>
@@ -133,7 +134,9 @@ app.get("/hook/complete", (req, res) => {
- const mappedEventType = mapEventType(eventType as string | undefined);
+ const mappedEventType =
+ mapEventType(eventType as string | undefined) ??
+ mapEventType(notificationType as string | undefined);
// Unknown or missing eventType: return success but don't process
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by cubic
Switched the terminal to Ghostty Web for faster rendering and lower latency. Bundled a Nerd Font and tightened event handling so restores, focus, and desktop notifications are more reliable.
New Features
Bug Fixes
Written for commit 45ec31b. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Performance