Skip to content

feat: sidebar enhancements and UI utilities from dpcode#65

Open
aaditagrawal wants to merge 4 commits intomainfrom
feat/dpcode-sidebar-utils
Open

feat: sidebar enhancements and UI utilities from dpcode#65
aaditagrawal wants to merge 4 commits intomainfrom
feat/dpcode-sidebar-utils

Conversation

@aaditagrawal
Copy link
Copy Markdown
Owner

@aaditagrawal aaditagrawal commented Apr 12, 2026

Summary

Ports sidebar features and UI utilities from the dpcode fork. All new files.

  • Thread pin toggle — pin/unpin threads + pinned threads store with persistence
  • Project sidebar icon — visual project icons in the sidebar
  • Disclosure chevron — animated expand/collapse chevron component
  • Shortcut kbd — keyboard shortcut display component
  • Toast route visibility — route-aware toast visibility helpers
  • Editor metadata — editor icons and display names
  • Confirm dialog fallback — browser confirm() for non-desktop environments
  • Task completion notifications — detect task completion and show notifications

Test plan

  • Pinned threads store tests
  • Editor metadata tests
  • Toast visibility tests
  • Task completion logic tests
  • Truncate title tests

Summary by CodeRabbit

Release Notes

  • New Features

    • Added task completion notifications with optional system/browser notifications support
    • Added thread pinning capability to organize conversations
    • Enhanced project sidebar with favicon display
    • Improved notification visibility for split-view layouts
  • Chores

    • Added new UI components for better visual consistency
    • Added shared utilities for editor metadata and terminal handling
    • Expanded test coverage for notification and pinning functionality

- Thread pin toggle button + pinned threads store
- Project sidebar icon component
- Disclosure chevron animation component
- Keyboard shortcut display component (shortcut-kbd)
- Toast route visibility helpers
- Editor metadata (icons, names) utility
- Confirm dialog fallback for non-desktop environments
- Truncate title tests
- Task completion notification system (logic + UI)
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 12, 2026

Warning

Rate limit exceeded

@aaditagrawal has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 6 minutes and 55 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 6 minutes and 55 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 932ca695-d48f-43c7-a6c0-dc4a61c23345

📥 Commits

Reviewing files that changed from the base of the PR and between cc15ee7 and 7672a24.

📒 Files selected for processing (3)
  • apps/web/src/notifications/taskCompletion.logic.test.ts
  • apps/web/src/notifications/taskCompletion.tsx
  • apps/web/src/splitViewStore.ts
📝 Walkthrough

Walkthrough

This PR introduces new UI components, state stores, notification handling, and shared terminal identity utilities. Key additions include ProjectSidebarIcon and ThreadPinToggleButton components, task completion notification logic with candidate collection and copy builders, pinned threads persistence store, toast visibility helpers, terminal thread identity derivation and reconciliation, editor metadata utilities, and supporting UI primitives like DisclosureChevron and ShortcutKbd.

Changes

Cohort / File(s) Summary
UI Components
apps/web/src/components/ProjectSidebarIcon.tsx, apps/web/src/components/ThreadPinToggleButton.tsx, apps/web/src/components/ui/DisclosureChevron.tsx, apps/web/src/components/ui/shortcut-kbd.tsx
New React components: ProjectSidebarIcon with favicon probing and caching; ThreadPinToggleButton with overlay/inline presentation modes; DisclosureChevron for rotating chevrons; ShortcutKbd for parsing and rendering keyboard shortcut labels with modifiers.
Icon and UI Utilities
apps/web/src/lib/icons.tsx, apps/web/src/components/ui/toastRouteVisibility.ts, apps/web/src/components/ui/toastRouteVisibility.test.ts
Consolidated icon re-exports from Lucide; toast visibility logic for split-view and thread-scoped rendering; test coverage for visible toast thread ID resolution and render decision rules.
Notification System
apps/web/src/notifications/taskCompletion.logic.ts, apps/web/src/notifications/taskCompletion.logic.test.ts, apps/web/src/notifications/taskCompletion.tsx
Task completion notification candidates and copy builders for completed threads, input-needed events, and terminal state changes; system notification bridging via desktop/web APIs with permission gating; test suite validating candidate collection and copy generation.
State Management
apps/web/src/pinnedThreadsStore.ts, apps/web/src/pinnedThreadsStore.test.ts, apps/web/src/splitViewStore.ts, apps/web/src/composerDraftStore.ts
Zustand store for persisted pinned threads with add/remove/toggle/prune actions; split-view thread ID resolution; composer model selection fallback chain; store tests.
Dialog and Utilities
apps/web/src/confirmDialogFallback.ts, apps/web/src/truncateTitle.test.ts
DOM-based confirm dialog fallback with keyboard/mouse interaction and Tailwind styling; truncateTitle function test coverage.
Editor Metadata
apps/web/src/editorMetadata.ts, apps/web/src/editorMetadata.test.ts
Editor UI metadata module with platform-specific labels (Finder/Explorer/Files), icon mapping, and available editor option resolution; test suite verifying label resolution and editor option filtering.
Terminal Identity Utilities
packages/shared/src/terminalThreads.ts, packages/shared/package.json
Comprehensive terminal thread identity derivation, parsing, and reconciliation with CLI kind detection, command normalization, shell tokenization, provider attribution, and visual state resolution; export subpath added to shared package.
Type Definitions
packages/contracts/src/environment.ts
ThreadEnvironmentMode schema as literal union of "local" and "worktree".

Sequence Diagram

sequenceDiagram
    participant ThreadState as Thread State
    participant Notifier as TaskCompletionNotifications
    participant Collector as taskCompletion.logic
    participant Toast as Toast System
    participant Browser as Browser/Desktop<br/>Notification

    ThreadState->>Notifier: State Update
    Notifier->>Collector: Compare prev/next threads
    Collector->>Collector: detectCompletedThreads()
    Collector->>Collector: detectInputNeededThreads()
    Collector-->>Notifier: Return Candidates
    
    alt Has Completed Thread Candidate
        Notifier->>Collector: buildTaskCompletionCopy()
        Collector-->>Notifier: {title, body}
        
        alt enableTaskCompletionToasts
            Notifier->>Toast: Show Toast
        end
        
        alt enableSystemNotifications<br/>& Window Backgrounded
            Notifier->>Browser: Show Notification
            Browser-->>Browser: User Clicks
            Browser->>Notifier: onMenuAction/notification-open
            Notifier->>ThreadState: Navigate to Thread
        end
    end
    
    alt Has Input Needed Candidate
        Notifier->>Collector: buildInputNeededCopy()
        Collector-->>Notifier: {title, body}
        
        alt enableTaskCompletionToasts<br/>& Suppress Check Passes
            Notifier->>Toast: Show Toast
        end
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Poem

🐰 Hoppy trails through task completion streams,
Pinned threads dance in notification dreams,
Terminal identities hop and reconcile,
Toast visibility checks with style,
From choosy composers to favicons bright,
This PR bounds forward—what a sight! 🎉

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 24.68% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main purpose: porting sidebar enhancements and UI utilities from the dpcode fork, which directly aligns with the changeset's primary focus.
Description check ✅ Passed The description covers what changed (enumerated features added), includes a test plan with checkmarks indicating completion, but lacks before/after UI screenshots and detailed reasoning for the approach.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/dpcode-sidebar-utils

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:XXL 1,000+ effective changed lines (test files excluded in mixed PRs). labels Apr 12, 2026
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 14

🧹 Nitpick comments (4)
apps/web/src/truncateTitle.test.ts (1)

14-16: Truncation path is covered correctly.

Optional follow-up: consider adding edge-case tests ("", maxLength = 0/1, and boundary cases after trimming) to harden regression protection.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/truncateTitle.test.ts` around lines 14 - 16, Add unit tests to
cover edge cases for the truncateTitle function: include an empty string input
(""), maxLength values 0 and 1, and boundary cases such as strings whose trimmed
length equals maxLength and just exceeds it (to ensure trimming logic behaves
correctly). Name tests to reflect scenarios (e.g., "returns empty for empty
input", "handles maxLength 0", "handles maxLength 1", "boundary after trimming")
and assert expected outputs (including ellipsis behavior) for each case.
apps/web/src/components/ProjectSidebarIcon.tsx (1)

9-10: Consider bounding projectFaviconPresence to avoid unbounded growth.

The module-level cache keeps every distinct favicon URL forever. In long-lived sessions with many projects, this can grow indefinitely.

♻️ Suggested bounded-cache helper
 const projectFaviconPresence = new Map<string, boolean>();
+const MAX_FAVICON_CACHE_ENTRIES = 500;
+
+function setProjectFaviconPresence(key: string, value: boolean) {
+  if (!projectFaviconPresence.has(key) && projectFaviconPresence.size >= MAX_FAVICON_CACHE_ENTRIES) {
+    const oldestKey = projectFaviconPresence.keys().next().value;
+    if (oldestKey) projectFaviconPresence.delete(oldestKey);
+  }
+  projectFaviconPresence.set(key, value);
+}

Then replace .set(...) callsites with setProjectFaviconPresence(...).

Also applies to: 64-70, 98-99

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/components/ProjectSidebarIcon.tsx` around lines 9 - 10, The
module-level Map projectFaviconPresence can grow unbounded; implement a small
bounded cache helper (e.g., MAX_PROJECT_FAVICON_CACHE_SIZE constant plus
functions getProjectFaviconPresence(key: string) and
setProjectFaviconPresence(key: string, value: boolean)) that evicts oldest
entries when the size limit is exceeded (LRU or FIFO), replace direct
projectFaviconPresence.set(...) calls with setProjectFaviconPresence, and read
via getProjectFaviconPresence wherever the Map is accessed (referencing
projectFaviconPresence, setProjectFaviconPresence, getProjectFaviconPresence and
MAX_PROJECT_FAVICON_CACHE_SIZE in your changes).
apps/web/src/confirmDialogFallback.ts (1)

23-26: Add dialog semantics and restore focus for accessibility.

This fallback should expose dialog semantics (role, aria-modal, labeling) and return focus to the previously active element on close.

Proposed refactor
 export function showConfirmDialogFallback(message: string): Promise<boolean> {
   return new Promise<boolean>((resolve) => {
+    const previouslyFocused =
+      document.activeElement instanceof HTMLElement ? document.activeElement : null;
+
     // Split message into title (first line) and description (rest)
     const lines = message.split("\n");
@@
     const popup = document.createElement("div");
@@
     popup.style.cssText = "animation:scaleIn .15s ease-out";
+    popup.setAttribute("role", "dialog");
+    popup.setAttribute("aria-modal", "true");
@@
     const titleEl = document.createElement("h2");
+    titleEl.id = "confirm-dialog-title";
@@
     if (description) {
       const descEl = document.createElement("p");
+      descEl.id = "confirm-dialog-description";
@@
       header.appendChild(descEl);
+      popup.setAttribute("aria-describedby", descEl.id);
     }
+    popup.setAttribute("aria-labelledby", titleEl.id);
@@
     function cleanup(result: boolean) {
       document.removeEventListener("keydown", onKeyDown);
       backdrop.remove();
       viewport.remove();
+      previouslyFocused?.focus();
       resolve(result);
     }

Also applies to: 32-41, 51-56, 97-97

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/confirmDialogFallback.ts` around lines 23 - 26, The popup
element created for the fallback dialog lacks ARIA dialog semantics and focus
management; update the created element (the variable popup where className and
style are set) to include role="dialog", aria-modal="true", and proper labeling
via aria-labelledby or aria-label, make it focusable (e.g., tabIndex=-1) and
move focus into it when opened, and ensure you capture the previously focused
element (document.activeElement) and restore focus to that element when the
dialog is closed; apply the same changes to the other dialog fallbacks at the
other creation sites (the blocks around lines 32-41, 51-56, and 97) so all
fallback popups expose consistent dialog semantics and restore focus on close.
apps/web/src/editorMetadata.test.ts (1)

13-23: Test scope doesn’t match the test name.

Line 13 says this covers every supported editor, but the assertion checks only a subset. Either rename the test or make it exhaustive from the shared catalog.

✅ Suggested update (exhaustive + future-proof)
 import { describe, expect, it } from "vitest";
+import { EDITORS } from "@t3tools/contracts";
 import { resolveAvailableEditorOptions, resolveEditorLabel } from "./editorMetadata";
@@
-  it("surfaces every supported available editor from the shared contracts catalog", () => {
-    expect(
-      resolveAvailableEditorOptions("MacIntel", [
-        "cursor",
-        "trae",
-        "vscode-insiders",
-        "vscodium",
-        "idea",
-        "file-manager",
-      ]).map((option) => option.value),
-    ).toEqual(["cursor", "trae", "vscode-insiders", "vscodium", "idea", "file-manager"]);
+  it("surfaces every supported editor from the shared contracts catalog", () => {
+    const allEditors = EDITORS.map((editor) => editor.id);
+    expect(resolveAvailableEditorOptions("MacIntel", allEditors).map((option) => option.value)).toEqual(
+      allEditors,
+    );
   });
 });
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/editorMetadata.test.ts` around lines 13 - 23, The test name
claims it covers "every supported available editor" but the assertion only
checks a hard-coded subset; update the test in editorMetadata.test.ts to either
rename the test to reflect the subset or (preferred) make it exhaustive by
importing the shared catalog of editor keys and asserting that
resolveAvailableEditorOptions("MacIntel", <fullCatalog>) .map(o => o.value)
equals the full catalog values; reference resolveAvailableEditorOptions to
locate the code under test and the shared catalog constant or export (use the
canonical exported list of editors from the shared contracts) so the test
remains future-proof.
🤖 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/ProjectSidebarIcon.tsx`:
- Around line 42-43: The favicon presence check in ProjectSidebarIcon (logic
around hasFavicon, url.searchParams.set("fallback","none"), and the Image
load/error handlers) is unreliable because /api/project-favicon always returns a
fallback SVG with 200; update either the server or client: ideally modify the
server handler for /api/project-favicon to respect the fallback query (when
fallback=none return 404/204 when no real favicon exists) so the Image onError
can be trusted, or alternatively add and call an explicit existence endpoint
(e.g., /api/project-favicon/existence) from ProjectSidebarIcon to return a
boolean before setting hasFavicon; adjust the component to use that existence
response instead of relying on load/error.
- Around line 48-50: The state initialization for hasFavicon can stay, but reset
it whenever faviconSrc changes to avoid showing a stale true value; update the
component to call setHasFavicon(false) (or set to
projectFaviconPresence.get(faviconSrc) === true if cached) inside a useEffect
that depends on faviconSrc before starting the probe, and ensure the probe logic
(the code interacting with projectFaviconPresence and any fetch/check routine)
sets setHasFavicon(true/false) when complete; reference useState/useEffect,
hasFavicon, setHasFavicon, faviconSrc, and projectFaviconPresence to locate and
modify the logic.

In `@apps/web/src/components/ThreadPinToggleButton.tsx`:
- Around line 41-46: The button currently only stops propagation in onMouseDown,
but the click event still bubbles and triggers parent row handlers; update the
component (ThreadPinToggleButton) so the onClick handler accepts the mouse
event, calls event.stopPropagation() (and optionally event.preventDefault() if
desired) before invoking onToggle, ensuring the parent row onClick isn't
triggered when this button is clicked.

In `@apps/web/src/components/ui/shortcut-kbd.tsx`:
- Around line 7-11: The split/filter pipeline in shortcut-kbd.tsx (the branch
that checks shortcutLabel.includes("+")) drops empty parts so labels like
"Ctrl++" lose the literal '+' key; update the logic around
.split("+")/.map((part) => part.trim())/.filter((part) => part.length > 0) to
preserve empty segments by mapping any empty trimmed part to the literal "+"
token instead of filtering it out (so sequence/ trailing '+' yield a "+" key).
Locate this behavior inside the ShortcutKbd component (or the function handling
shortcutLabel) and replace the filter with a mapping step that converts "" to
"+" while keeping non-empty parts unchanged.

In `@apps/web/src/confirmDialogFallback.ts`:
- Around line 20-21: The viewport element currently covers the screen and
intercepts clicks, preventing the backdrop's cancel handler from firing; update
the viewport's className in confirmDialogFallback.ts to include
"pointer-events-none" so it doesn't capture outside clicks, and ensure the
modal/dialog element inside (the popup element created after viewport)
explicitly has "pointer-events-auto" so it remains interactive; apply the same
change to the other occurrence referenced (the viewport assignment at the second
location).
- Around line 58-65: The onKeyDown handler currently treats every Enter keydown
as confirmation; update the onKeyDown function so it inspects
document.activeElement before calling cleanup(true): if the active element is a
form control that should handle Enter itself (e.g., tagName "BUTTON", "A",
"TEXTAREA", or "INPUT" with type "button", "submit", or "reset"), or is
contentEditable or has a data-cancel/data-ignore-enter attribute (for cancel
button identification), then do not call cleanup(true); otherwise proceed to
preventDefault and call cleanup(true). Keep the key checks (e.key ===
"Enter"/"Escape") and use cleanup(false)/cleanup(true) as before, but gate the
Enter branch by this activeElement check so pressing Enter while Cancel (or
other interactive controls) is focused won’t accidentally confirm.

In `@apps/web/src/notifications/taskCompletion.logic.ts`:
- Around line 68-80: The current summarizeLatestAssistantMessage function walks
backwards through thread.messages and can return an older assistant message;
instead, use the just-completed turn's assistant message id
(latestTurn.assistantMessageId) to locate the precise assistant message in
thread.messages (or a message map) and summarize that one only; if
latestTurn.assistantMessageId is missing, the referenced message is not found,
or its trimmed text is empty, return the default "Finished working." string;
update summarizeLatestAssistantMessage to accept the turn (or its
assistantMessageId) and reference that id to avoid returning stale assistant
text.

In `@apps/web/src/notifications/taskCompletion.tsx`:
- Around line 84-113: The async helper showSystemThreadNotification can throw if
window.desktopBridge.notifications.isSupported() or .show() rejects; wrap the
desktop-bridge branch in a try/catch inside showSystemThreadNotification
(catching errors from isSupported and show) and return false on error, and log
the error if appropriate, so callers (which use void fire-and-forget) won't
produce unhandled promise rejections; specifically update the calls to
window.desktopBridge.notifications.isSupported() and
window.desktopBridge.notifications.show(...) within showSystemThreadNotification
to be guarded by a single try/catch.

In `@apps/web/src/pinnedThreadsStore.ts`:
- Around line 92-99: The merge function in pinnedThreadsStore currently trusts
the persisted pinnedThreadIds and can break if storage is malformed; update the
merge implementation (the merge function that returns currentState with
pinnedThreadIds) to robustly validate and sanitize the persisted candidate
before calling normalizePinnedThreadIds: retrieve
persistedState?.pinnedThreadIds, check Array.isArray(candidate), coerce
non-array or null/undefined values to an empty array, and filter elements to
expected primitive types (e.g., strings or numbers) before passing to
normalizePinnedThreadIds so PinnedThreadsStoreState and consumers are never
hydrated with invalid values.

In `@apps/web/src/splitViewStore.ts`:
- Around line 1-12: The function resolvePreferredSplitViewIdForThread has the
wrong signature and SplitView is not exported; update this module to export the
SplitView type and change resolvePreferredSplitViewIdForThread to accept the
parameters used by TaskCompletionNotifications (an object containing
splitViewsById: Record<string, SplitView>, splitViewIdBySourceThreadId:
Record<ThreadId, string | undefined>, and threadId: ThreadId) and implement
logic that: (1) if splitViewIdBySourceThreadId[threadId] exists return it; (2)
otherwise scan splitViewsById and use resolveSplitViewThreadIds(splitView) to
find a split view containing threadId and return that split view's id; (3)
return null if none found. Ensure exports include the SplitView type and the
updated resolvePreferredSplitViewIdForThread signature so
toastRouteVisibility.ts and TaskCompletionNotifications compile.

In `@packages/shared/src/terminalThreads.ts`:
- Around line 357-363: The provider detection currently uses
deriveCliKindFromTokenList(tokens) which scans every token and misattributes
providers for commands like npm install or cat; change the logic to only detect
provider from the executable position after unwrapping (i.e., use the first
executable token — the current variables first/second or a normalized execToken
— or only handle explicit executor cases you support) instead of scanning the
whole tokens array, and update/createTerminalCommandIdentity calls accordingly
(keep createTerminalCommandIdentity("Codex CLI","codex") and "Claude Code" only
when the executable token matches codex/claude executables or supported executor
patterns).
- Around line 326-334: normalizePersistedTerminalTitle currently treats any
non-empty trimmed title as authoritative, which preserves the generic
placeholder "New terminal" even when cliKind is known; update
normalizePersistedTerminalTitle to treat the placeholder (e.g., "New terminal")
as empty by checking if normalizedTitle equals the generic placeholder constant
and, if so, fall through to return defaultTerminalTitleForCliKind(cliKind) or
GENERIC_TERMINAL_THREAD_TITLE; reference the function
normalizePersistedTerminalTitle and the helper defaultTerminalTitleForCliKind so
reconcileTerminalCommandIdentity will get an upgraded label when cliKind is
present.
- Around line 104-118: deriveCliKindFromNormalizedToken fails to recognize
versioned package specifiers (e.g. `@openai/codex`@latest); update the function to
strip any trailing version tag before matching. Specifically, after computing
normalizedToken (using stripScriptExtension), extract the package base by
removing a trailing `@version` component for both scoped and unscoped names (e.g.
use a regex to capture either scoped names like `@scope/name` or unscoped names
and drop anything after an `@version`), then test that base against
CODEX_COMMAND_NAMES, CLAUDE_COMMAND_NAMES and the literal "@openai/codex" /
"@anthropic-ai/claude-code". Keep stripScriptExtension and the same return
values ("codex"/"claude"/null) and only change the token normalization step
inside deriveCliKindFromNormalizedToken.

---

Nitpick comments:
In `@apps/web/src/components/ProjectSidebarIcon.tsx`:
- Around line 9-10: The module-level Map projectFaviconPresence can grow
unbounded; implement a small bounded cache helper (e.g.,
MAX_PROJECT_FAVICON_CACHE_SIZE constant plus functions
getProjectFaviconPresence(key: string) and setProjectFaviconPresence(key:
string, value: boolean)) that evicts oldest entries when the size limit is
exceeded (LRU or FIFO), replace direct projectFaviconPresence.set(...) calls
with setProjectFaviconPresence, and read via getProjectFaviconPresence wherever
the Map is accessed (referencing projectFaviconPresence,
setProjectFaviconPresence, getProjectFaviconPresence and
MAX_PROJECT_FAVICON_CACHE_SIZE in your changes).

In `@apps/web/src/confirmDialogFallback.ts`:
- Around line 23-26: The popup element created for the fallback dialog lacks
ARIA dialog semantics and focus management; update the created element (the
variable popup where className and style are set) to include role="dialog",
aria-modal="true", and proper labeling via aria-labelledby or aria-label, make
it focusable (e.g., tabIndex=-1) and move focus into it when opened, and ensure
you capture the previously focused element (document.activeElement) and restore
focus to that element when the dialog is closed; apply the same changes to the
other dialog fallbacks at the other creation sites (the blocks around lines
32-41, 51-56, and 97) so all fallback popups expose consistent dialog semantics
and restore focus on close.

In `@apps/web/src/editorMetadata.test.ts`:
- Around line 13-23: The test name claims it covers "every supported available
editor" but the assertion only checks a hard-coded subset; update the test in
editorMetadata.test.ts to either rename the test to reflect the subset or
(preferred) make it exhaustive by importing the shared catalog of editor keys
and asserting that resolveAvailableEditorOptions("MacIntel", <fullCatalog>)
.map(o => o.value) equals the full catalog values; reference
resolveAvailableEditorOptions to locate the code under test and the shared
catalog constant or export (use the canonical exported list of editors from the
shared contracts) so the test remains future-proof.

In `@apps/web/src/truncateTitle.test.ts`:
- Around line 14-16: Add unit tests to cover edge cases for the truncateTitle
function: include an empty string input (""), maxLength values 0 and 1, and
boundary cases such as strings whose trimmed length equals maxLength and just
exceeds it (to ensure trimming logic behaves correctly). Name tests to reflect
scenarios (e.g., "returns empty for empty input", "handles maxLength 0",
"handles maxLength 1", "boundary after trimming") and assert expected outputs
(including ellipsis behavior) for each case.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: aa54c906-069f-469f-9a7f-8f20213cb790

📥 Commits

Reviewing files that changed from the base of the PR and between b3bd5c3 and cc15ee7.

⛔ Files ignored due to path filters (1)
  • bun.lock is excluded by !**/*.lock
📒 Files selected for processing (21)
  • apps/web/src/components/ProjectSidebarIcon.tsx
  • apps/web/src/components/ThreadPinToggleButton.tsx
  • apps/web/src/components/ui/DisclosureChevron.tsx
  • apps/web/src/components/ui/shortcut-kbd.tsx
  • apps/web/src/components/ui/toastRouteVisibility.test.ts
  • apps/web/src/components/ui/toastRouteVisibility.ts
  • apps/web/src/composerDraftStore.ts
  • apps/web/src/confirmDialogFallback.ts
  • apps/web/src/editorMetadata.test.ts
  • apps/web/src/editorMetadata.ts
  • apps/web/src/lib/icons.tsx
  • apps/web/src/notifications/taskCompletion.logic.test.ts
  • apps/web/src/notifications/taskCompletion.logic.ts
  • apps/web/src/notifications/taskCompletion.tsx
  • apps/web/src/pinnedThreadsStore.test.ts
  • apps/web/src/pinnedThreadsStore.ts
  • apps/web/src/splitViewStore.ts
  • apps/web/src/truncateTitle.test.ts
  • packages/contracts/src/environment.ts
  • packages/shared/package.json
  • packages/shared/src/terminalThreads.ts

Comment on lines +42 to +43
url.searchParams.set("fallback", "none");
return origin.length > 0 ? url.toString() : `${url.pathname}${url.search}`;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Favicon presence detection is currently false-positive prone.

fallback=none is added to the URL, but the current /api/project-favicon server route always returns a fallback SVG with HTTP 200 when no favicon exists. That means Image often triggers load, and hasFavicon becomes true even for projects without a real favicon.

Please align the contract (e.g., server honors fallback=none with non-200/404, or expose an explicit existence endpoint) before relying on load/error as presence signal.

Also applies to: 53-74

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/components/ProjectSidebarIcon.tsx` around lines 42 - 43, The
favicon presence check in ProjectSidebarIcon (logic around hasFavicon,
url.searchParams.set("fallback","none"), and the Image load/error handlers) is
unreliable because /api/project-favicon always returns a fallback SVG with 200;
update either the server or client: ideally modify the server handler for
/api/project-favicon to respect the fallback query (when fallback=none return
404/204 when no real favicon exists) so the Image onError can be trusted, or
alternatively add and call an explicit existence endpoint (e.g.,
/api/project-favicon/existence) from ProjectSidebarIcon to return a boolean
before setting hasFavicon; adjust the component to use that existence response
instead of relying on load/error.

Comment on lines +48 to +50
const [hasFavicon, setHasFavicon] = useState<boolean>(
() => projectFaviconPresence.get(faviconSrc) === true,
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Reset stale hasFavicon on faviconSrc change.

When cwd changes to an uncached source, the previous hasFavicon=true can briefly persist until probe completes, causing a wrong badge flash.

💡 Suggested patch
  useEffect(() => {
    const cached = projectFaviconPresence.get(faviconSrc);
    if (cached !== undefined) {
      setHasFavicon(cached);
      return;
    }
+   setHasFavicon(false);

    let cancelled = false;
    const image = new Image();

Also applies to: 55-63

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/components/ProjectSidebarIcon.tsx` around lines 48 - 50, The
state initialization for hasFavicon can stay, but reset it whenever faviconSrc
changes to avoid showing a stale true value; update the component to call
setHasFavicon(false) (or set to projectFaviconPresence.get(faviconSrc) === true
if cached) inside a useEffect that depends on faviconSrc before starting the
probe, and ensure the probe logic (the code interacting with
projectFaviconPresence and any fetch/check routine) sets
setHasFavicon(true/false) when complete; reference useState/useEffect,
hasFavicon, setHasFavicon, faviconSrc, and projectFaviconPresence to locate and
modify the logic.

Comment on lines +41 to +46
onMouseDown={(event) => {
event.preventDefault();
event.stopPropagation();
}}
onClick={onToggle}
>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Stop click bubbling here to avoid triggering parent row handlers.

onMouseDown suppression alone doesn’t prevent click propagation. A parent row onClick can still run when this button is clicked.

Proposed fix
-      onClick={onToggle}
+      onClick={(event) => {
+        event.stopPropagation();
+        onToggle(event);
+      }}
📝 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
onMouseDown={(event) => {
event.preventDefault();
event.stopPropagation();
}}
onClick={onToggle}
>
onMouseDown={(event) => {
event.preventDefault();
event.stopPropagation();
}}
onClick={(event) => {
event.stopPropagation();
onToggle(event);
}}
>
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/components/ThreadPinToggleButton.tsx` around lines 41 - 46, The
button currently only stops propagation in onMouseDown, but the click event
still bubbles and triggers parent row handlers; update the component
(ThreadPinToggleButton) so the onClick handler accepts the mouse event, calls
event.stopPropagation() (and optionally event.preventDefault() if desired)
before invoking onToggle, ensuring the parent row onClick isn't triggered when
this button is clicked.

Comment on lines +7 to +11
if (shortcutLabel.includes("+")) {
return shortcutLabel
.split("+")
.map((part) => part.trim())
.filter((part) => part.length > 0);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Handle + as a literal key in shortcut labels.

Line 7-Line 11 drops empty split parts, so labels like Ctrl++/⌘+ lose the + key and render incorrectly.

💡 Proposed fix
   if (shortcutLabel.includes("+")) {
-    return shortcutLabel
-      .split("+")
-      .map((part) => part.trim())
-      .filter((part) => part.length > 0);
+    const rawParts = shortcutLabel.split("+").map((part) => part.trim());
+    const hasLiteralPlusKey = rawParts.at(-1) === "";
+    const parts = rawParts.filter((part) => part.length > 0);
+    return hasLiteralPlusKey ? [...parts, "+"] : 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
if (shortcutLabel.includes("+")) {
return shortcutLabel
.split("+")
.map((part) => part.trim())
.filter((part) => part.length > 0);
if (shortcutLabel.includes("+")) {
const rawParts = shortcutLabel.split("+").map((part) => part.trim());
const hasLiteralPlusKey = rawParts.at(-1) === "";
const parts = rawParts.filter((part) => part.length > 0);
return hasLiteralPlusKey ? [...parts, "+"] : parts;
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/components/ui/shortcut-kbd.tsx` around lines 7 - 11, The
split/filter pipeline in shortcut-kbd.tsx (the branch that checks
shortcutLabel.includes("+")) drops empty parts so labels like "Ctrl++" lose the
literal '+' key; update the logic around .split("+")/.map((part) =>
part.trim())/.filter((part) => part.length > 0) to preserve empty segments by
mapping any empty trimmed part to the literal "+" token instead of filtering it
out (so sequence/ trailing '+' yield a "+" key). Locate this behavior inside the
ShortcutKbd component (or the function handling shortcutLabel) and replace the
filter with a mapping step that converts "" to "+" while keeping non-empty parts
unchanged.

Comment on lines +20 to +21
viewport.className = "fixed inset-0 z-50 flex items-center justify-center p-4";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Backdrop dismiss won’t fire because the viewport intercepts outside clicks.

viewport fully covers the screen at the same z-index, so clicking outside the popup targets viewport, not backdrop. The cancel-on-backdrop path is effectively dead.

Proposed fix
-    document.addEventListener("keydown", onKeyDown);
-    backdrop.addEventListener("mousedown", () => cleanup(false));
+    document.addEventListener("keydown", onKeyDown);
+    viewport.addEventListener("mousedown", (e) => {
+      if (e.target === viewport) cleanup(false);
+    });

Also applies to: 69-69

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/confirmDialogFallback.ts` around lines 20 - 21, The viewport
element currently covers the screen and intercepts clicks, preventing the
backdrop's cancel handler from firing; update the viewport's className in
confirmDialogFallback.ts to include "pointer-events-none" so it doesn't capture
outside clicks, and ensure the modal/dialog element inside (the popup element
created after viewport) explicitly has "pointer-events-auto" so it remains
interactive; apply the same change to the other occurrence referenced (the
viewport assignment at the second location).

Comment on lines +1 to +12
export function resolveSplitViewThreadIds(splitView: SplitView): ThreadId[] {
const ids: ThreadId[] = [];
if (splitView.leftThreadId) ids.push(splitView.leftThreadId);
if (splitView.rightThreadId && splitView.rightThreadId !== splitView.leftThreadId) {
ids.push(splitView.rightThreadId);
}
return ids;
}

export function resolvePreferredSplitViewIdForThread(): string | null {
return null;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -e
rg -n --type=ts 'resolvePreferredSplitViewIdForThread|type SplitView|interface SplitView|export type SplitView|export interface SplitView' apps/web/src

Repository: aaditagrawal/t3code

Length of output: 1065


🏁 Script executed:

#!/bin/bash
# Check full contents of splitViewStore.ts to see type definitions and exports
cat -n apps/web/src/splitViewStore.ts

Repository: aaditagrawal/t3code

Length of output: 554


🏁 Script executed:

#!/bin/bash
# Check the call sites in taskCompletion.tsx around line 168
sed -n '160,180p' apps/web/src/notifications/taskCompletion.tsx

Repository: aaditagrawal/t3code

Length of output: 682


🏁 Script executed:

#!/bin/bash
# Check more call sites in taskCompletion.tsx
sed -n '220,230p' apps/web/src/notifications/taskCompletion.tsx

Repository: aaditagrawal/t3code

Length of output: 496


🏁 Script executed:

#!/bin/bash
# Check the full file size and see if there's more content
wc -l apps/web/src/splitViewStore.ts

Repository: aaditagrawal/t3code

Length of output: 96


🏁 Script executed:

#!/bin/bash
# Get the entire file content with more context
cat apps/web/src/splitViewStore.ts

Repository: aaditagrawal/t3code

Length of output: 470


🏁 Script executed:

#!/bin/bash
# Check if SplitView type is defined in the file
rg -A 5 'type SplitView|interface SplitView' apps/web/src/splitViewStore.ts

Repository: aaditagrawal/t3code

Length of output: 45


🏁 Script executed:

#!/bin/bash
# Show all content including any imports or type definitions
head -20 apps/web/src/splitViewStore.ts

Repository: aaditagrawal/t3code

Length of output: 470


🏁 Script executed:

#!/bin/bash
# Check what's actually imported or defined before the functions
rg '^(import|export|type|interface)' apps/web/src/splitViewStore.ts

Repository: aaditagrawal/t3code

Length of output: 212


🏁 Script executed:

#!/bin/bash
# Check the toastRouteVisibility file to see how it imports SplitView
cat -n apps/web/src/components/ui/toastRouteVisibility.ts | head -20

Repository: aaditagrawal/t3code

Length of output: 1017


🏁 Script executed:

#!/bin/bash
# Check imports at the very beginning of the file
head -1 apps/web/src/splitViewStore.ts | od -c | head -5

Repository: aaditagrawal/t3code

Length of output: 103


🏁 Script executed:

#!/bin/bash
# Look for where SplitView might be defined - check other related files
rg 'type SplitView|interface SplitView' apps/web/src --max-count=5

Repository: aaditagrawal/t3code

Length of output: 195


🏁 Script executed:

#!/bin/bash
# Check what TypeScript types are used (ThreadId)
rg 'type ThreadId|interface ThreadId' apps/web/src/splitViewStore.ts

Repository: aaditagrawal/t3code

Length of output: 45


🏁 Script executed:

#!/bin/bash
# Check if there's maybe a .d.ts file or other related files in splitViewStore area
fd 'splitViewStore' apps/web/src

Repository: aaditagrawal/t3code

Length of output: 93


🏁 Script executed:

#!/bin/bash
# Check the entire apps/web/src directory structure
git ls-files apps/web/src | grep -i splitview

Repository: aaditagrawal/t3code

Length of output: 146


🏁 Script executed:

#!/bin/bash
# Check GhosttyTerminalSplitView to see if SplitView type is defined there
grep -n 'type SplitView\|interface SplitView\|export.*SplitView' apps/web/src/components/GhosttyTerminalSplitView.tsx

Repository: aaditagrawal/t3code

Length of output: 170


🏁 Script executed:

#!/bin/bash
# Search entire codebase for SplitView definition
rg 'export (type|interface) SplitView' apps/web/src

Repository: aaditagrawal/t3code

Length of output: 45


🏁 Script executed:

#!/bin/bash
# Check if the file might have a different encoding or missing imports section
hexdump -C apps/web/src/splitViewStore.ts | head -20

Repository: aaditagrawal/t3code

Length of output: 108


🏁 Script executed:

#!/bin/bash
# Check entire apps/web/src for where SplitView might be defined (not just exported)
rg 'type SplitView|interface SplitView' apps/web/src

Repository: aaditagrawal/t3code

Length of output: 195


🏁 Script executed:

#!/bin/bash
# Look for SplitView in contracts or other dependency files
rg -t ts 'SplitView' apps/web/src | grep -i 'interface\|type\|export' | head -20

Repository: aaditagrawal/t3code

Length of output: 724


🏁 Script executed:

#!/bin/bash
# Check what the actual imports/types should be - look at useSplitViewStore usage
rg -B5 -A5 'useSplitViewStore' apps/web/src/notifications/taskCompletion.tsx | head -30

Repository: aaditagrawal/t3code

Length of output: 1224


Fix function signature and export SplitView type from this module.

The resolvePreferredSplitViewIdForThread function is currently defined with no parameters and always returns null, but it's called from TaskCompletionNotifications with an object containing splitViewsById, splitViewIdBySourceThreadId, and threadId. This is a compile-time signature mismatch. Additionally, toastRouteVisibility.ts imports type SplitView directly from this module, but the type is not defined or exported here. Update the function signature to accept the required parameters and implement logic to resolve the correct split view ID for a given thread. Ensure SplitView is properly exported from this module.

Proposed fix
 export function resolveSplitViewThreadIds(splitView: SplitView): ThreadId[] {
   const ids: ThreadId[] = [];
   if (splitView.leftThreadId) ids.push(splitView.leftThreadId);
   if (splitView.rightThreadId && splitView.rightThreadId !== splitView.leftThreadId) {
     ids.push(splitView.rightThreadId);
   }
   return ids;
 }
 
-export function resolvePreferredSplitViewIdForThread(): string | null {
-  return null;
+export function resolvePreferredSplitViewIdForThread(input: {
+  splitViewsById: Record<string, SplitView>;
+  splitViewIdBySourceThreadId: Record<string, string>;
+  threadId: ThreadId;
+}): string | null {
+  const directMatch = input.splitViewIdBySourceThreadId[input.threadId];
+  if (directMatch) {
+    return directMatch;
+  }
+
+  for (const [splitViewId, splitView] of Object.entries(input.splitViewsById)) {
+    if (resolveSplitViewThreadIds(splitView).includes(input.threadId)) {
+      return splitViewId;
+    }
+  }
+
+  return null;
 }
📝 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 resolveSplitViewThreadIds(splitView: SplitView): ThreadId[] {
const ids: ThreadId[] = [];
if (splitView.leftThreadId) ids.push(splitView.leftThreadId);
if (splitView.rightThreadId && splitView.rightThreadId !== splitView.leftThreadId) {
ids.push(splitView.rightThreadId);
}
return ids;
}
export function resolvePreferredSplitViewIdForThread(): string | null {
return null;
}
export function resolveSplitViewThreadIds(splitView: SplitView): ThreadId[] {
const ids: ThreadId[] = [];
if (splitView.leftThreadId) ids.push(splitView.leftThreadId);
if (splitView.rightThreadId && splitView.rightThreadId !== splitView.leftThreadId) {
ids.push(splitView.rightThreadId);
}
return ids;
}
export function resolvePreferredSplitViewIdForThread(input: {
splitViewsById: Record<string, SplitView>;
splitViewIdBySourceThreadId: Record<string, string>;
threadId: ThreadId;
}): string | null {
const directMatch = input.splitViewIdBySourceThreadId[input.threadId];
if (directMatch) {
return directMatch;
}
for (const [splitViewId, splitView] of Object.entries(input.splitViewsById)) {
if (resolveSplitViewThreadIds(splitView).includes(input.threadId)) {
return splitViewId;
}
}
return null;
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/splitViewStore.ts` around lines 1 - 12, The function
resolvePreferredSplitViewIdForThread has the wrong signature and SplitView is
not exported; update this module to export the SplitView type and change
resolvePreferredSplitViewIdForThread to accept the parameters used by
TaskCompletionNotifications (an object containing splitViewsById: Record<string,
SplitView>, splitViewIdBySourceThreadId: Record<ThreadId, string | undefined>,
and threadId: ThreadId) and implement logic that: (1) if
splitViewIdBySourceThreadId[threadId] exists return it; (2) otherwise scan
splitViewsById and use resolveSplitViewThreadIds(splitView) to find a split view
containing threadId and return that split view's id; (3) return null if none
found. Ensure exports include the SplitView type and the updated
resolvePreferredSplitViewIdForThread signature so toastRouteVisibility.ts and
TaskCompletionNotifications compile.

Comment on lines +104 to +118
function deriveCliKindFromNormalizedToken(token: string): TerminalCliKind | null {
const normalizedToken = stripScriptExtension(token.trim().toLowerCase());
if (normalizedToken.length === 0) {
return null;
}
if (CODEX_COMMAND_NAMES.has(normalizedToken) || normalizedToken === "@openai/codex") {
return "codex";
}
if (
CLAUDE_COMMAND_NAMES.has(normalizedToken) ||
normalizedToken === "@anthropic-ai/claude-code"
) {
return "claude";
}
return null;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Handle versioned package specifiers.

Lines 109-114 only match bare package names. Common launches like npx @openai/codex@latest or pnpm dlx @anthropic-ai/claude-code@1.2.3 will fall through to a generic title, so provider attribution never starts for those sessions.

💡 Suggested fix
+function stripPackageVersionSpecifier(token: string): string {
+  if (token.startsWith("@")) {
+    return token.replace(/^(@[^/]+\/[^@]+)(?:@.+)?$/, "$1");
+  }
+  return token.replace(/^([^@]+)(?:@.+)?$/, "$1");
+}
+
 function deriveCliKindFromNormalizedToken(token: string): TerminalCliKind | null {
-  const normalizedToken = stripScriptExtension(token.trim().toLowerCase());
+  const normalizedToken = stripPackageVersionSpecifier(
+    stripScriptExtension(token.trim().toLowerCase()),
+  );
   if (normalizedToken.length === 0) {
     return null;
   }
📝 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
function deriveCliKindFromNormalizedToken(token: string): TerminalCliKind | null {
const normalizedToken = stripScriptExtension(token.trim().toLowerCase());
if (normalizedToken.length === 0) {
return null;
}
if (CODEX_COMMAND_NAMES.has(normalizedToken) || normalizedToken === "@openai/codex") {
return "codex";
}
if (
CLAUDE_COMMAND_NAMES.has(normalizedToken) ||
normalizedToken === "@anthropic-ai/claude-code"
) {
return "claude";
}
return null;
function stripPackageVersionSpecifier(token: string): string {
if (token.startsWith("@")) {
return token.replace(/^(@[^/]+\/[^@]+)(?:@.+)?$/, "$1");
}
return token.replace(/^([^@]+)(?:@.+)?$/, "$1");
}
function deriveCliKindFromNormalizedToken(token: string): TerminalCliKind | null {
const normalizedToken = stripPackageVersionSpecifier(
stripScriptExtension(token.trim().toLowerCase()),
);
if (normalizedToken.length === 0) {
return null;
}
if (CODEX_COMMAND_NAMES.has(normalizedToken) || normalizedToken === "@openai/codex") {
return "codex";
}
if (
CLAUDE_COMMAND_NAMES.has(normalizedToken) ||
normalizedToken === "@anthropic-ai/claude-code"
) {
return "claude";
}
return null;
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/shared/src/terminalThreads.ts` around lines 104 - 118,
deriveCliKindFromNormalizedToken fails to recognize versioned package specifiers
(e.g. `@openai/codex`@latest); update the function to strip any trailing version
tag before matching. Specifically, after computing normalizedToken (using
stripScriptExtension), extract the package base by removing a trailing `@version`
component for both scoped and unscoped names (e.g. use a regex to capture either
scoped names like `@scope/name` or unscoped names and drop anything after an
`@version`), then test that base against CODEX_COMMAND_NAMES, CLAUDE_COMMAND_NAMES
and the literal "@openai/codex" / "@anthropic-ai/claude-code". Keep
stripScriptExtension and the same return values ("codex"/"claude"/null) and only
change the token normalization step inside deriveCliKindFromNormalizedToken.

Comment on lines +326 to +334
function normalizePersistedTerminalTitle(
title: string | null | undefined,
cliKind: TerminalCliKind | null,
): string {
const normalizedTitle = title?.trim();
if (normalizedTitle && normalizedTitle.length > 0) {
return normalizedTitle;
}
return cliKind ? defaultTerminalTitleForCliKind(cliKind) : GENERIC_TERMINAL_THREAD_TITLE;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Drop the generic placeholder once cliKind is known.

If cliKind is already set but title is still "New terminal", this helper preserves the placeholder. That means reconcileTerminalCommandIdentity() can keep a generic tab label instead of upgrading it to "Codex CLI" or "Claude Code".

💡 Suggested fix
 function normalizePersistedTerminalTitle(
   title: string | null | undefined,
   cliKind: TerminalCliKind | null,
 ): string {
   const normalizedTitle = title?.trim();
-  if (normalizedTitle && normalizedTitle.length > 0) {
+  if (
+    normalizedTitle &&
+    normalizedTitle.length > 0 &&
+    !isGenericTerminalThreadTitle(normalizedTitle)
+  ) {
     return normalizedTitle;
   }
   return cliKind ? defaultTerminalTitleForCliKind(cliKind) : GENERIC_TERMINAL_THREAD_TITLE;
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/shared/src/terminalThreads.ts` around lines 326 - 334,
normalizePersistedTerminalTitle currently treats any non-empty trimmed title as
authoritative, which preserves the generic placeholder "New terminal" even when
cliKind is known; update normalizePersistedTerminalTitle to treat the
placeholder (e.g., "New terminal") as empty by checking if normalizedTitle
equals the generic placeholder constant and, if so, fall through to return
defaultTerminalTitleForCliKind(cliKind) or GENERIC_TERMINAL_THREAD_TITLE;
reference the function normalizePersistedTerminalTitle and the helper
defaultTerminalTitleForCliKind so reconcileTerminalCommandIdentity will get an
upgraded label when cliKind is present.

Comment on lines +357 to +363
const detectedCliKind = deriveCliKindFromTokenList(tokens);
if (detectedCliKind === "codex") {
return createTerminalCommandIdentity("Codex CLI", "codex");
}
if (detectedCliKind === "claude" || (first === "claude" && second === "code")) {
return createTerminalCommandIdentity("Claude Code", "claude");
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Don't attribute the provider from arbitrary argv entries.

Line 357 scans every token, so commands like npm install @openai/codex`` or cat anthropic/claude-code/README.md get labeled as managed Codex/Claude sessions even though the CLI was never launched. In this path, provider detection should be limited to the executable position after unwrapping, plus any explicit executor cases you intentionally support.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/shared/src/terminalThreads.ts` around lines 357 - 363, The provider
detection currently uses deriveCliKindFromTokenList(tokens) which scans every
token and misattributes providers for commands like npm install or cat; change
the logic to only detect provider from the executable position after unwrapping
(i.e., use the first executable token — the current variables first/second or a
normalized execToken — or only handle explicit executor cases you support)
instead of scanning the whole tokens array, and
update/createTerminalCommandIdentity calls accordingly (keep
createTerminalCommandIdentity("Codex CLI","codex") and "Claude Code" only when
the executable token matches codex/claude executables or supported executor
patterns).

Comment on lines +423 to +427
for (const char of data) {
if (char === "\r" || char === "\n") {
nextIdentity = deriveTerminalCommandIdentity(nextBuffer);
nextBuffer = "";
continue;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

CRLF currently erases the derived identity.

Lines 424-426 derive on \r, clear the buffer, then immediately re-derive on \n from "" and overwrite the result with null. Any stream that submits Enter as \r\n will intermittently miss command detection.

💡 Suggested fix
   for (const char of data) {
     if (char === "\r" || char === "\n") {
-      nextIdentity = deriveTerminalCommandIdentity(nextBuffer);
-      nextBuffer = "";
+      if (nextBuffer.length > 0) {
+        nextIdentity = deriveTerminalCommandIdentity(nextBuffer);
+        nextBuffer = "";
+      }
       continue;
     }
📝 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
for (const char of data) {
if (char === "\r" || char === "\n") {
nextIdentity = deriveTerminalCommandIdentity(nextBuffer);
nextBuffer = "";
continue;
for (const char of data) {
if (char === "\r" || char === "\n") {
if (nextBuffer.length > 0) {
nextIdentity = deriveTerminalCommandIdentity(nextBuffer);
nextBuffer = "";
}
continue;

- Add SplitView type definition, useSplitViewStore hook stub, and proper
  imports to splitViewStore.ts (fixes TS2305/TS2304 in toastRouteVisibility
  and taskCompletion files)
- Add environmentId to test thread fixture in taskCompletion.logic.test.ts
  (fixes TS2322 Thread type mismatch)
- Stub taskCompletion.tsx component to return null, keeping only the
  browser notification permission helpers that compile against current
  APIs (fixes 20+ errors from missing store/bridge/settings APIs)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XXL 1,000+ effective changed lines (test files excluded in mixed PRs). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant