sync: merge 6 new upstream commits + ancestry#62
Merged
aaditagrawal merged 7 commits intomainfrom Apr 12, 2026
Merged
Conversation
Co-authored-by: Hugo Blom <[email protected]> Co-authored-by: eggfriedrice <[email protected]> Co-authored-by: Jono Kemball <[email protected]> Co-authored-by: Julius Marminge <[email protected]> Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
Co-authored-by: codex <[email protected]>
) Co-authored-by: Julius Marminge <[email protected]> Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
|
Warning Rate limit exceeded
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 14 minutes and 48 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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 configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (33)
📝 WalkthroughWalkthroughAdds a global command palette (toggleable via Mod+K when terminal unfocused), implements palette UI, search/filter/sort logic, state store, sidebar trigger, thread-sorting utilities, command execution plumbing, tests, and related server/adapter and platform adjustments. Changes
Sequence DiagramsequenceDiagram
actor User
participant ChatView as ChatView
participant Store as CommandPaletteStore
participant Palette as CommandPalette
participant Logic as CommandPalette.logic
participant Handler as ThreadActionHandler
User->>ChatView: Press Mod+K
ChatView->>Store: getState().open
alt Closed
ChatView->>Store: toggleOpen()
Store->>Palette: open=true
else Open
ChatView->>Store: toggleOpen()
Store->>Palette: open=false
end
Palette->>Logic: buildRootGroups(...) / filterCommandPaletteGroups(query)
Logic->>Palette: groups/items (ranked)
User->>Palette: Select item
Palette->>Handler: item.run()
Handler->>ChatView: handleNewThread(...)
Handler->>Store: setOpen(false)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
size:XXL
aaditagrawal
force-pushed
the
sync/upstream-main-2026-04-13
branch
from
2d2cce0 to
0123393
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/src/git/Layers/GitCore.ts (1)
921-935:⚠️ Potential issue | 🟠 MajorRemote-only fetch can leave some tracked upstream refs stale.
Switching this refresh from an explicit upstream ref fetch to plain
git fetch <remote>changes behavior: it now only updates refs covered byremote.<name>.fetch. Branches tracking a narrowed/custom upstream ref can keep stale ahead/behind data, and the remote-level cache will then suppress another refresh for ~15s across sibling worktrees.Please keep the cache key remote-based if you want the coalescing, but preserve the old explicit upstream refspec when the caller has a resolved upstream.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/git/Layers/GitCore.ts` around lines 921 - 935, fetchRemoteForStatus currently runs git fetch <remote> which only updates refs covered by remote.<name>.fetch and can leave tracked upstream refs stale; keep the remote-based cache/coalescing but when the caller has a resolved upstream, call git fetch with the explicit upstream refspec instead of the plain remote to ensure that specific upstream ref is refreshed. Update fetchRemoteForStatus (and its callers) to accept an optional upstream refspec (or detect a resolved upstream) and, when present, pass ["--git-dir", gitCommonDir, "fetch", "--quiet", "--no-tags", remoteName, "<refspec>"] (or replace <remote> with "<remote> <refspec>" as a single fetch target) to executeGit; otherwise keep the existing remote-only fetch and preserve the timeout and allowNonZeroExit options.
🧹 Nitpick comments (4)
apps/desktop/src/main.ts (1)
107-107: Centralize desktop app identity to avoid runtime/build drift.Line 107 correctly splits
APP_USER_MODEL_IDby environment, but build config still appears hardcoded tocom.t3tools.t3code(scripts/build-desktop-artifact.ts, Lines 493-501 in provided context). Consider using one shared source for app identity across launcher, main process, and build script.♻️ Proposed follow-up alignment (build config)
-const buildConfig: Record<string, unknown> = { - appId: "com.t3tools.t3code", +const APP_BUNDLE_ID = isDevelopment ? "com.t3tools.t3code.dev" : "com.t3tools.t3code"; +const buildConfig: Record<string, unknown> = { + appId: APP_BUNDLE_ID, productName, artifactName: "T3-Code-${version}-${arch}.${ext}", asarUnpack: ["node_modules/@github/copilot*/**/*"], directories: { buildResources: "apps/desktop/resources", }, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main.ts` at line 107, APP_USER_MODEL_ID is environment-dependent in the main process but the build script still uses the hardcoded string "com.t3tools.t3code", causing drift; fix by extracting a single exported constant (e.g., APP_USER_MODEL_ID or APP_ID) into a shared config module and import that constant wherever the app identity is needed (replace the hardcoded literal in the build-script logic and replace the inline ternary in main.ts with the shared constant), ensuring both the launcher/main process and build tooling read the same value.package.json (1)
59-61: Update the stale SDK version reference in ClaudeProvider.ts.The dependency was upgraded to
^0.2.104but the inline comment atapps/server/src/provider/Layers/ClaudeProvider.ts:441still references SDK0.2.77. Update it to reflect the current version to prevent misleading maintenance context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 59 - 61, The inline version comment in apps/server/src/provider/Layers/ClaudeProvider.ts that mentions SDK version "0.2.77" is now stale; update that comment to reference the current dependency version ^0.2.104 to match package.json. Locate the comment inside the ClaudeProvider file (near the ClaudeProvider class / relevant provider setup block) and replace the old version string with "^0.2.104" so maintenance notes reflect the actual installed SDK.packages/contracts/src/keybindings.test.ts (1)
50-54: Remove the duplicatecommandPalette.toggleassertion.This case is already covered at Lines 26-30, so the extra block adds noise without increasing coverage.
Proposed cleanup
- const parsedCommandPalette = yield* decode(KeybindingRule, { - key: "mod+k", - command: "commandPalette.toggle", - }); - assert.strictEqual(parsedCommandPalette.command, "commandPalette.toggle");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/contracts/src/keybindings.test.ts` around lines 50 - 54, The test contains a duplicate assertion for the command "commandPalette.toggle" in the block that decodes KeybindingRule into parsedCommandPalette; remove the redundant assert.strictEqual(parsedCommandPalette.command, "commandPalette.toggle") (the decode call and variable parsedCommandPalette can remain if other assertions exist) because the same assertion is already covered earlier; locate the decode(KeybindingRule, {...}) call and delete only the duplicate assertion line referencing parsedCommandPalette.command.packages/contracts/src/keybindings.ts (1)
30-38: Drop the duplicated"commandPalette.toggle"literal.It does not change the schema, but it makes the canonical command list harder to scan and easier to let drift.
♻️ Suggested cleanup
const STATIC_KEYBINDING_COMMANDS = [ "commandPalette.toggle", "terminal.toggle", "terminal.split", "terminal.new", "terminal.close", "diff.toggle", - "commandPalette.toggle", "chat.new", "chat.newLocal", "editor.openFavorite",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/contracts/src/keybindings.ts` around lines 30 - 38, STATIC_KEYBINDING_COMMANDS currently contains a duplicate "commandPalette.toggle" entry; remove the duplicate from the STATIC_KEYBINDING_COMMANDS array in packages/contracts/src/keybindings.ts so the list contains each command literal only once (leave the other entries and ordering intact).
🤖 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/server/src/keybindings.ts`:
- Line 65: Remove the duplicate default keybinding entry for mod+k in the
DEFAULT_KEYBINDINGS array so the same { key: "mod+k", command:
"commandPalette.toggle", when: "!terminalFocus" } isn't declared twice; locate
the duplicate object (the entry shown in keybindings.ts) and delete it (or
deduplicate DEFAULT_KEYBINDINGS) so syncDefaultKeybindingsOnStartup() doesn't
persist a redundant binding.
In `@apps/server/src/provider/Layers/ClaudeAdapter.ts`:
- Around line 1427-1431: The comparison/guard currently uses
lastGoodUsage.usedTokens as the floor which can regress totalProcessedTokens;
change the check to compare accumulatedTotalProcessedTokens against the previous
total (lastGoodUsage.totalProcessedTokens ?? lastGoodUsage.usedTokens) and only
set totalProcessedTokens when accumulatedTotalProcessedTokens is a finite number
greater than that previousTotal; update the conditional around
accumulatedTotalProcessedTokens accordingly in the block that builds the result
(referencing accumulatedTotalProcessedTokens and
lastGoodUsage.totalProcessedTokens/usedTokens).
- Around line 350-358: In maxClaudeContextWindowFromModelUsage, guard the
extracted contextWindow values before passing to Math.max: iterate over
Object.values(modelUsage), coerce or validate that value.contextWindow is a
finite number (e.g., Number.isFinite), skip entries that are missing or
non-finite, and only call Math.max with valid numbers; if no valid contextWindow
is found return undefined so you don't propagate NaN into
context.lastKnownContextWindow (which completeTurn later relies on from the
SDK-cast payload).
In `@apps/web/src/components/Sidebar.tsx`:
- Around line 2146-2165: The global window keydown/keyup handlers that implement
thread traversal and jump hints in this component should be disabled while the
command palette is open: update the handlers (the functions registered on window
in this file that manage thread traversal/jump hints) to early-return when
useCommandPaletteStore.getState().open is true, or wrap their registration so
they are not active while the CommandDialogTrigger opens the palette; reference
CommandDialogTrigger and useCommandPaletteStore.getState().open to locate where
to add the guard.
In `@apps/web/src/keybindings.test.ts`:
- Around line 102-106: The test fixture still includes an unconditional
DEFAULT_BINDINGS entry mapping mod+k to "commandPalette.toggle", which overrides
the new conditional rule; remove or modify the unconditional shortcut object so
there is no plain { shortcut: modShortcut("k"), command: "commandPalette.toggle"
} entry (or replace it with the conditional rule using whenAst:
whenNot(whenIdentifier("terminalFocus"))), ensuring the only binding for mod+k
is the conditional one so resolveShortcutCommand(..., { context: {
terminalFocus: true } }) no longer resolves to "commandPalette.toggle".
In `@apps/web/src/lib/chatThreadActions.ts`:
- Around line 63-67: buildDefaultThreadOptions currently returns envMode:
context.defaultThreadEnvMode so calls like startNewLocalThreadFromContext (and
the similar path at 88-97) can still create worktree drafts; change the code so
the local-thread path forces a true local envMode: set envMode to "local"
(either by hardcoding in buildDefaultThreadOptions when used for local creation
or by adding a new buildLocalThreadOptions helper) and ensure callers
startNewLocalThreadFromContext and the other similar function(s) use this
local-only options object so no worktree metadata is inherited.
- Around line 52-74: startNewThreadInProjectFromContext currently always passes
buildContextualThreadOptions(context) which copies branch/worktree/env from the
active thread/draft even when projectRef is different; change it to detect
whether projectRef matches context.activeThread?.projectRef or
context.activeDraftThread?.projectRef and only use
buildContextualThreadOptions(context) in that case, otherwise call
buildDefaultThreadOptions(context) (ensuring branch/worktree are cleared and
envMode falls back to context.defaultThreadEnvMode) before invoking
context.handleNewThread; update references in startNewThreadInProjectFromContext
to use activeThread, activeDraftThread, buildContextualThreadOptions,
buildDefaultThreadOptions, defaultThreadEnvMode, and handleNewThread.
---
Outside diff comments:
In `@apps/server/src/git/Layers/GitCore.ts`:
- Around line 921-935: fetchRemoteForStatus currently runs git fetch <remote>
which only updates refs covered by remote.<name>.fetch and can leave tracked
upstream refs stale; keep the remote-based cache/coalescing but when the caller
has a resolved upstream, call git fetch with the explicit upstream refspec
instead of the plain remote to ensure that specific upstream ref is refreshed.
Update fetchRemoteForStatus (and its callers) to accept an optional upstream
refspec (or detect a resolved upstream) and, when present, pass ["--git-dir",
gitCommonDir, "fetch", "--quiet", "--no-tags", remoteName, "<refspec>"] (or
replace <remote> with "<remote> <refspec>" as a single fetch target) to
executeGit; otherwise keep the existing remote-only fetch and preserve the
timeout and allowNonZeroExit options.
---
Nitpick comments:
In `@apps/desktop/src/main.ts`:
- Line 107: APP_USER_MODEL_ID is environment-dependent in the main process but
the build script still uses the hardcoded string "com.t3tools.t3code", causing
drift; fix by extracting a single exported constant (e.g., APP_USER_MODEL_ID or
APP_ID) into a shared config module and import that constant wherever the app
identity is needed (replace the hardcoded literal in the build-script logic and
replace the inline ternary in main.ts with the shared constant), ensuring both
the launcher/main process and build tooling read the same value.
In `@package.json`:
- Around line 59-61: The inline version comment in
apps/server/src/provider/Layers/ClaudeProvider.ts that mentions SDK version
"0.2.77" is now stale; update that comment to reference the current dependency
version ^0.2.104 to match package.json. Locate the comment inside the
ClaudeProvider file (near the ClaudeProvider class / relevant provider setup
block) and replace the old version string with "^0.2.104" so maintenance notes
reflect the actual installed SDK.
In `@packages/contracts/src/keybindings.test.ts`:
- Around line 50-54: The test contains a duplicate assertion for the command
"commandPalette.toggle" in the block that decodes KeybindingRule into
parsedCommandPalette; remove the redundant
assert.strictEqual(parsedCommandPalette.command, "commandPalette.toggle") (the
decode call and variable parsedCommandPalette can remain if other assertions
exist) because the same assertion is already covered earlier; locate the
decode(KeybindingRule, {...}) call and delete only the duplicate assertion line
referencing parsedCommandPalette.command.
In `@packages/contracts/src/keybindings.ts`:
- Around line 30-38: STATIC_KEYBINDING_COMMANDS currently contains a duplicate
"commandPalette.toggle" entry; remove the duplicate from the
STATIC_KEYBINDING_COMMANDS array in packages/contracts/src/keybindings.ts so the
list contains each command literal only once (leave the other entries and
ordering intact).
🪄 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: 05a0dc15-0731-467f-b08f-05237f2adb46
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
KEYBINDINGS.mdapps/desktop/src/main.tsapps/server/src/git/Layers/GitCore.test.tsapps/server/src/git/Layers/GitCore.tsapps/server/src/keybindings.tsapps/server/src/open.tsapps/server/src/provider/Layers/ClaudeAdapter.test.tsapps/server/src/provider/Layers/ClaudeAdapter.tsapps/web/src/commandPaletteStore.tsapps/web/src/components/ChatView.browser.tsxapps/web/src/components/ChatView.tsxapps/web/src/components/CommandPalette.logic.test.tsapps/web/src/components/CommandPalette.logic.tsapps/web/src/components/CommandPalette.tsxapps/web/src/components/CommandPaletteResults.tsxapps/web/src/components/Sidebar.logic.test.tsapps/web/src/components/Sidebar.logic.tsapps/web/src/components/Sidebar.tsxapps/web/src/components/ui/command.tsxapps/web/src/components/ui/toast.tsxapps/web/src/composerHandleContext.tsapps/web/src/keybindings.test.tsapps/web/src/lib/chatThreadActions.test.tsapps/web/src/lib/chatThreadActions.tsapps/web/src/lib/threadSort.test.tsapps/web/src/lib/threadSort.tsapps/web/src/routes/__root.tsxapps/web/src/routes/_chat.tsxapps/web/src/types.tsapps/web/vitest.browser.config.tspackage.jsonpackages/contracts/src/keybindings.test.tspackages/contracts/src/keybindings.ts
💤 Files with no reviewable changes (1)
- apps/web/src/components/Sidebar.logic.test.ts
Comment on lines
+350
to
358
| function maxClaudeContextWindowFromModelUsage( | ||
| modelUsage: Record<string, ModelUsage> | undefined, | ||
| ): number | undefined { | ||
| if (!modelUsage) return undefined; | ||
|
|
||
| let maxContextWindow: number | undefined; | ||
| for (const value of Object.values(modelUsage as Record<string, unknown>)) { | ||
| if (!value || typeof value !== "object") { | ||
| continue; | ||
| } | ||
| const contextWindow = (value as { contextWindow?: unknown }).contextWindow; | ||
| if ( | ||
| typeof contextWindow !== "number" || | ||
| !Number.isFinite(contextWindow) || | ||
| contextWindow <= 0 | ||
| ) { | ||
| continue; | ||
| } | ||
| for (const value of Object.values(modelUsage)) { | ||
| const contextWindow = value.contextWindow; | ||
| maxContextWindow = Math.max(maxContextWindow ?? 0, contextWindow); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l apps/server/src/provider/Layers/ClaudeAdapter.tsRepository: aaditagrawal/t3code
Length of output: 116
🏁 Script executed:
# First, let's see the specific lines mentioned in the review
sed -n '340,370p' apps/server/src/provider/Layers/ClaudeAdapter.tsRepository: aaditagrawal/t3code
Length of output: 920
🏁 Script executed:
# Also check lines around 1401-1431 for the monotonicity issue mentioned in scratchpad
sed -n '1395,1435p' apps/server/src/provider/Layers/ClaudeAdapter.tsRepository: aaditagrawal/t3code
Length of output: 1930
🏁 Script executed:
# Look at the complete function to understand context better
sed -n '350,380p' apps/server/src/provider/Layers/ClaudeAdapter.tsRepository: aaditagrawal/t3code
Length of output: 1055
🏁 Script executed:
# Find the ModelUsage type definition
rg -n "type ModelUsage\|interface ModelUsage" apps/server/src/provider/Layers/ClaudeAdapter.tsRepository: aaditagrawal/t3code
Length of output: 45
🏁 Script executed:
# Check imports to understand where ModelUsage comes from
head -50 apps/server/src/provider/Layers/ClaudeAdapter.tsRepository: aaditagrawal/t3code
Length of output: 1359
🏁 Script executed:
# Verify if there are any other calls to maxClaudeContextWindowFromModelUsage
rg -n "maxClaudeContextWindowFromModelUsage" apps/server/src/provider/Layers/ClaudeAdapter.tsRepository: aaditagrawal/t3code
Length of output: 188
🏁 Script executed:
# Check how resultContextWindow is used after being set
sed -n '1401,1415p' apps/server/src/provider/Layers/ClaudeAdapter.tsRepository: aaditagrawal/t3code
Length of output: 838
Guard contextWindow before feeding it into Math.max.
completeTurn() force-casts the SDK payload at line 1402, so this helper is effectively parsing untrusted runtime data. If any entry is missing or has a non-finite contextWindow, Math.max returns NaN, and since NaN !== undefined, the result gets stored in context.lastKnownContextWindow at line 1404, poisoning downstream token usage snapshots.
🛡️ Proposed fix
function maxClaudeContextWindowFromModelUsage(
modelUsage: Record<string, ModelUsage> | undefined,
): number | undefined {
if (!modelUsage) return undefined;
let maxContextWindow: number | undefined;
for (const value of Object.values(modelUsage)) {
- const contextWindow = value.contextWindow;
+ const contextWindow =
+ typeof value?.contextWindow === "number" && Number.isFinite(value.contextWindow)
+ ? value.contextWindow
+ : undefined;
+ if (contextWindow === undefined || contextWindow <= 0) {
+ continue;
+ }
maxContextWindow = Math.max(maxContextWindow ?? 0, contextWindow);
}
return maxContextWindow;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/provider/Layers/ClaudeAdapter.ts` around lines 350 - 358, In
maxClaudeContextWindowFromModelUsage, guard the extracted contextWindow values
before passing to Math.max: iterate over Object.values(modelUsage), coerce or
validate that value.contextWindow is a finite number (e.g., Number.isFinite),
skip entries that are missing or non-finite, and only call Math.max with valid
numbers; if no valid contextWindow is found return undefined so you don't
propagate NaN into context.lastKnownContextWindow (which completeTurn later
relies on from the SDK-cast payload).
Comment on lines
+1427
to
1431
| ...(typeof accumulatedTotalProcessedTokens === "number" && | ||
| Number.isFinite(accumulatedTotalProcessedTokens) && | ||
| accumulatedTotalProcessedTokens > lastGoodUsage.usedTokens | ||
| ? { totalProcessedTokens: accumulatedTotalProcessedTokens } | ||
| : {}), |
There was a problem hiding this comment.
Compare against the previous total, not the clamped usedTokens.
Line 1429 can regress totalProcessedTokens. If lastGoodUsage is already { usedTokens: 200000, totalProcessedTokens: 400000 } and the result later normalizes to 250000, this branch overwrites 400000 with 250000 just because 250000 > 200000. Since task_progress and result come from different SDK messages, the safer floor here is lastGoodUsage.totalProcessedTokens ?? lastGoodUsage.usedTokens.
🔧 Proposed fix
+ const previousTotalProcessedTokens =
+ lastGoodUsage?.totalProcessedTokens ?? lastGoodUsage?.usedTokens;
const usageSnapshot: ThreadTokenUsageSnapshot | undefined = lastGoodUsage
? {
...lastGoodUsage,
...(typeof maxTokens === "number" && Number.isFinite(maxTokens) && maxTokens > 0
? { maxTokens }
: {}),
...(typeof accumulatedTotalProcessedTokens === "number" &&
Number.isFinite(accumulatedTotalProcessedTokens) &&
- accumulatedTotalProcessedTokens > lastGoodUsage.usedTokens
+ accumulatedTotalProcessedTokens > (previousTotalProcessedTokens ?? 0)
? { totalProcessedTokens: accumulatedTotalProcessedTokens }
: {}),
}
: accumulatedSnapshot;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/provider/Layers/ClaudeAdapter.ts` around lines 1427 - 1431,
The comparison/guard currently uses lastGoodUsage.usedTokens as the floor which
can regress totalProcessedTokens; change the check to compare
accumulatedTotalProcessedTokens against the previous total
(lastGoodUsage.totalProcessedTokens ?? lastGoodUsage.usedTokens) and only set
totalProcessedTokens when accumulatedTotalProcessedTokens is a finite number
greater than that previousTotal; update the conditional around
accumulatedTotalProcessedTokens accordingly in the block that builds the result
(referencing accumulatedTotalProcessedTokens and
lastGoodUsage.totalProcessedTokens/usedTokens).
Comment on lines
+52
to
+74
| function buildContextualThreadOptions(context: ChatThreadActionContext): NewThreadOptions { | ||
| return { | ||
| branch: context.activeThread?.branch ?? context.activeDraftThread?.branch ?? null, | ||
| worktreePath: | ||
| context.activeThread?.worktreePath ?? context.activeDraftThread?.worktreePath ?? null, | ||
| envMode: | ||
| context.activeDraftThread?.envMode ?? | ||
| (context.activeThread?.worktreePath ? "worktree" : "local"), | ||
| }; | ||
| } | ||
|
|
||
| function buildDefaultThreadOptions(context: ChatThreadActionContext): NewThreadOptions { | ||
| return { | ||
| envMode: context.defaultThreadEnvMode, | ||
| }; | ||
| } | ||
|
|
||
| export async function startNewThreadInProjectFromContext( | ||
| context: ChatThreadActionContext, | ||
| projectRef: ScopedProjectRef, | ||
| ): Promise<void> { | ||
| await context.handleNewThread(projectRef, buildContextualThreadOptions(context)); | ||
| } |
There was a problem hiding this comment.
Don't reuse thread context when the target project is different.
startNewThreadInProjectFromContext() always forwards buildContextualThreadOptions(context), so choosing project B from the palette still copies branch/worktree/env mode from the active thread or draft in project A. That seeds the new draft with the wrong repo context and bypasses the target project's default env-mode fallback.
Only apply buildContextualThreadOptions() when the explicit projectRef matches the active thread/draft project; otherwise start from default options and clear branch/worktree metadata.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/lib/chatThreadActions.ts` around lines 52 - 74,
startNewThreadInProjectFromContext currently always passes
buildContextualThreadOptions(context) which copies branch/worktree/env from the
active thread/draft even when projectRef is different; change it to detect
whether projectRef matches context.activeThread?.projectRef or
context.activeDraftThread?.projectRef and only use
buildContextualThreadOptions(context) in that case, otherwise call
buildDefaultThreadOptions(context) (ensuring branch/worktree are cleared and
envMode falls back to context.defaultThreadEnvMode) before invoking
context.handleNewThread; update references in startNewThreadInProjectFromContext
to use activeThread, activeDraftThread, buildContextualThreadOptions,
buildDefaultThreadOptions, defaultThreadEnvMode, and handleNewThread.
Comment on lines
+63
to
+67
| function buildDefaultThreadOptions(context: ChatThreadActionContext): NewThreadOptions { | ||
| return { | ||
| envMode: context.defaultThreadEnvMode, | ||
| }; | ||
| } |
There was a problem hiding this comment.
startNewLocalThreadFromContext() does not actually force local mode.
This path delegates to buildDefaultThreadOptions(), which uses context.defaultThreadEnvMode. If the default is "worktree", the "local" helper will still create a worktree draft.
Either hardcode envMode: "local" here (and avoid inheriting worktree metadata), or rename the API so callers do not assume local-thread semantics.
Also applies to: 88-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/lib/chatThreadActions.ts` around lines 63 - 67,
buildDefaultThreadOptions currently returns envMode:
context.defaultThreadEnvMode so calls like startNewLocalThreadFromContext (and
the similar path at 88-97) can still create worktree drafts; change the code so
the local-thread path forces a true local envMode: set envMode to "local"
(either by hardcoding in buildDefaultThreadOptions when used for local creation
or by adding a new buildLocalThreadOptions helper) and ensure callers
startNewLocalThreadFromContext and the other similar function(s) use this
local-only options object so no worktree metadata is inherited.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/src/git/Layers/GitCore.ts (1)
921-935:⚠️ Potential issue | 🟠 MajorDo not treat non-zero
git fetchexits as a successful refresh.With
allowNonZeroExit: true, a fetch that exits non-zero still resolves successfully here, andEffect.asVoidmakes the cache entry use the success TTL. That breaks the intended failure cooldown path and can leave behind/ahead counts stale for longer after transient fetch failures.Suggested fix
return executeGit( "GitCore.fetchRemoteForStatus", fetchCwd, ["--git-dir", gitCommonDir, "fetch", "--quiet", "--no-tags", remoteName], { - allowNonZeroExit: true, timeoutMs: Duration.toMillis(STATUS_UPSTREAM_REFRESH_TIMEOUT), + fallbackErrorMessage: `git fetch ${remoteName} failed`, }, ).pipe(Effect.asVoid);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/git/Layers/GitCore.ts` around lines 921 - 935, fetchRemoteForStatus currently calls executeGit with allowNonZeroExit: true which treats non-zero git fetch exits as success; change that option so non-zero exits surface as GitCommandError (e.g., remove allowNonZeroExit or set it to false) so failures propagate instead of being converted to success by Effect.asVoid, leaving the cache to use the failure cooldown; keep the same timeout (STATUS_UPSTREAM_REFRESH_TIMEOUT) and otherwise return the Effect as before so successful fetches still map to void.
♻️ Duplicate comments (5)
apps/web/src/lib/chatThreadActions.ts (2)
63-67:⚠️ Potential issue | 🟠 Major
newLocalpath should force local mode.Line 65 uses
defaultThreadEnvMode, sostartNewLocalThreadFromContext(Line 96) can still create non-local drafts when default is"worktree".Suggested fix
function buildDefaultThreadOptions(context: ChatThreadActionContext): NewThreadOptions { return { + branch: null, + worktreePath: null, envMode: context.defaultThreadEnvMode, }; } + +function buildLocalThreadOptions(): NewThreadOptions { + return { + branch: null, + worktreePath: null, + envMode: "local", + }; +} @@ export async function startNewLocalThreadFromContext( context: ChatThreadActionContext, ): Promise<boolean> { @@ - await context.handleNewThread(projectRef, buildDefaultThreadOptions(context)); + await context.handleNewThread(projectRef, buildLocalThreadOptions()); return true; }Also applies to: 88-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/chatThreadActions.ts` around lines 63 - 67, The default builder buildDefaultThreadOptions uses context.defaultThreadEnvMode which allows non-local drafts; ensure the "newLocal" path forces local mode by overriding envMode to "local" when creating local threads—either add a dedicated builder (e.g., buildLocalThreadOptions) or change startNewLocalThreadFromContext to call buildDefaultThreadOptions but then set options.envMode = "local" (or pass an explicit envMode override) so newLocal always creates a local draft; update the analogous creation path referenced around lines 88–97 as well.
69-74:⚠️ Potential issue | 🟠 MajorAvoid leaking thread context across projects.
Line 73 always applies
buildContextualThreadOptions(context). IfprojectRefpoints to a different project, this can carry branch/worktree/env from the wrong project into the new draft.Suggested fix
export async function startNewThreadInProjectFromContext( context: ChatThreadActionContext, projectRef: ScopedProjectRef, ): Promise<void> { - await context.handleNewThread(projectRef, buildContextualThreadOptions(context)); + const activeProjectRef = context.activeThread + ? scopeProjectRef(context.activeThread.environmentId, context.activeThread.projectId) + : context.activeDraftThread + ? scopeProjectRef( + context.activeDraftThread.environmentId, + context.activeDraftThread.projectId, + ) + : null; + + const options = + activeProjectRef === projectRef + ? buildContextualThreadOptions(context) + : buildDefaultThreadOptions(context); + + await context.handleNewThread(projectRef, options); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/chatThreadActions.ts` around lines 69 - 74, The function startNewThreadInProjectFromContext currently always passes buildContextualThreadOptions(context) which can leak branch/worktree/env from the current context into a different project; change it to only use the contextual options when the context refers to the same project (compare context.projectRef or equivalent) or construct/clean a new ThreadOptions for the target project that resets project-scoped fields (branch, worktree, env) before calling context.handleNewThread; update references to buildContextualThreadOptions, startNewThreadInProjectFromContext, and handleNewThread to implement this conditional/cleaning behavior.apps/server/src/provider/Layers/ClaudeAdapter.ts (2)
350-358:⚠️ Potential issue | 🟡 MinorFilter invalid
contextWindowvalues before callingMath.max.This helper still trusts a force-cast SDK payload. A single missing or non-finite
contextWindowturns the accumulator intoNaN, and Line 1405 then persists that bad value intocontext.lastKnownContextWindow.Suggested fix
function maxClaudeContextWindowFromModelUsage( modelUsage: Record<string, ModelUsage> | undefined, ): number | undefined { if (!modelUsage) return undefined; let maxContextWindow: number | undefined; for (const value of Object.values(modelUsage)) { - const contextWindow = value.contextWindow; + const contextWindow = + typeof value?.contextWindow === "number" && Number.isFinite(value.contextWindow) + ? value.contextWindow + : undefined; + if (contextWindow === undefined || contextWindow <= 0) { + continue; + } maxContextWindow = Math.max(maxContextWindow ?? 0, contextWindow); } return maxContextWindow; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/ClaudeAdapter.ts` around lines 350 - 358, In maxClaudeContextWindowFromModelUsage, guard against invalid contextWindow values before using Math.max: skip any value.contextWindow that is undefined, null or not a finite number (use Number.isFinite or equivalent) and only feed valid finite numbers into Math.max so maxContextWindow cannot become NaN; keep references to the existing variables/function names (maxClaudeContextWindowFromModelUsage, maxContextWindow, contextWindow, ModelUsage) and initialize/compare against 0 when computing the maximum.
1417-1430:⚠️ Potential issue | 🟡 MinorCompare against the previous total, not
usedTokens.This merge can still regress
totalProcessedTokens. IflastGoodUsagealready has a largertotalProcessedTokensthanusedTokens, the current guard lets a smaller accumulated total overwrite it.Suggested fix
+ const previousTotalProcessedTokens = + lastGoodUsage?.totalProcessedTokens ?? lastGoodUsage?.usedTokens; const usageSnapshot: ThreadTokenUsageSnapshot | undefined = lastGoodUsage ? { ...lastGoodUsage, ...(typeof maxTokens === "number" && Number.isFinite(maxTokens) && maxTokens > 0 ? { maxTokens } : {}), ...(typeof accumulatedTotalProcessedTokens === "number" && Number.isFinite(accumulatedTotalProcessedTokens) && - accumulatedTotalProcessedTokens > lastGoodUsage.usedTokens + accumulatedTotalProcessedTokens > (previousTotalProcessedTokens ?? 0) ? { totalProcessedTokens: accumulatedTotalProcessedTokens } : {}), } : accumulatedSnapshot;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/ClaudeAdapter.ts` around lines 1417 - 1430, The merge incorrectly allows accumulatedTotalProcessedTokens to overwrite a larger existing totalProcessedTokens because it only compares against lastGoodUsage.usedTokens; update the conditional that builds usageSnapshot so that accumulatedTotalProcessedTokens is compared against lastGoodUsage.totalProcessedTokens when present (fallback to lastGoodUsage.usedTokens only if totalProcessedTokens is undefined), i.e., in ClaudeAdapter.ts where accumulatedTotalProcessedTokens, lastGoodUsage (context.lastKnownTokenUsage) and usageSnapshot are constructed, guard setting totalProcessedTokens by ensuring accumulatedTotalProcessedTokens > (lastGoodUsage.totalProcessedTokens ?? lastGoodUsage.usedTokens) before assigning.apps/web/src/components/Sidebar.tsx (1)
2887-2914:⚠️ Potential issue | 🟠 MajorStill gate sidebar window shortcuts while command palette is open.
onWindowKeyDown/onWindowKeyUpstill run traversal and jump-hint logic when the palette is open, so navigation can fire behind the dialog. Add an early return guard in both handlers. (Line 2887 and Line 2964 paths.)🛡️ Suggested patch
+import { useCommandPaletteStore } from "../commandPaletteStore"; @@ const onWindowKeyDown = (event: globalThis.KeyboardEvent) => { + if (useCommandPaletteStore.getState().open) { + return; + } if (shouldIgnoreThreadJumpHintUpdate(event)) { return; } @@ const onWindowKeyUp = (event: globalThis.KeyboardEvent) => { + if (useCommandPaletteStore.getState().open) { + return; + } if (shouldIgnoreThreadJumpHintUpdate(event)) { return; }Also applies to: 2964-2987
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/Sidebar.tsx` around lines 2887 - 2914, Add an early-return guard at the top of both onWindowKeyDown and onWindowKeyUp so none of the traversal or jump-hint logic runs while the command palette is open: call getCurrentSidebarShortcutContext() (or the existing context getter used in these handlers) and if its commandPaletteOpen (or equivalent flag) is true, return immediately; do this before calling shouldIgnoreThreadJumpHintUpdate, shouldShowThreadJumpHints, setThreadJumpLabelByKey, clearThreadJumpHints, updateThreadJumpHintsVisibility, etc., so the handlers do nothing while the palette is open.
🤖 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/ChatView.browser.tsx`:
- Around line 569-579: createProjectlessSnapshot() doesn't fully remove
bootstrap context because mountChatView() still calls buildFixture() which sets
welcome.bootstrapProjectId/bootstrapThreadId to PROJECT_ID/THREAD_ID; update the
snapshot or build helper so the bootstrap fields are cleared. Specifically,
change createProjectlessSnapshot (or modify buildFixture) to override
welcome.bootstrapProjectId and welcome.bootstrapThreadId to undefined/null (or
remove them) in the returned OrchestrationReadModel, and ensure mountChatView()
uses this snapshot path (or accept an overrides parameter in buildFixture and
pass through) so tests run with no bootstrap project/thread context.
In `@apps/web/src/components/CommandPalette.logic.ts`:
- Around line 73-123: buildThreadActionItems currently uses bare projectId and
thread.id which can collide across environments; change lookups and item
identities to be environment-scoped by incorporating thread.environmentId into
keys. Specifically, update logic that reads from projectTitleById (and any
map/population code elsewhere) to use a composite key that includes
environmentId (e.g., `${environmentId}:${projectId}`) when calling
projectTitleById.get, and change the action item value from
`thread:${thread.id}` to include environment (e.g.,
`thread:${thread.environmentId}:${thread.id}`) so items and labels are unique
per environment; adjust searchTerms/title/description handling only if they rely
on the bare id. Use the function and symbol names buildThreadActionItems,
projectTitleById, thread.environmentId, and value to locate and apply the
changes.
In `@apps/web/src/lib/threadSort.ts`:
- Around line 16-18: The function getLatestUserMessageTimestamp currently
returns Number.NEGATIVE_INFINITY whenever thread.latestUserMessageAt is present
but toSortableTimestamp(thread.latestUserMessageAt) fails to parse; change the
logic so you only short-circuit and return when toSortableTimestamp returns a
valid numeric timestamp (non-null/undefined), otherwise fall through to the
existing fallbacks (e.g., examine thread.messages or thread.updatedAt). Update
the branch that references latestUserMessageAt to call toSortableTimestamp once,
test the result for success, and only return that value when valid; if parsing
fails, continue with the rest of the function instead of returning -Infinity.
---
Outside diff comments:
In `@apps/server/src/git/Layers/GitCore.ts`:
- Around line 921-935: fetchRemoteForStatus currently calls executeGit with
allowNonZeroExit: true which treats non-zero git fetch exits as success; change
that option so non-zero exits surface as GitCommandError (e.g., remove
allowNonZeroExit or set it to false) so failures propagate instead of being
converted to success by Effect.asVoid, leaving the cache to use the failure
cooldown; keep the same timeout (STATUS_UPSTREAM_REFRESH_TIMEOUT) and otherwise
return the Effect as before so successful fetches still map to void.
---
Duplicate comments:
In `@apps/server/src/provider/Layers/ClaudeAdapter.ts`:
- Around line 350-358: In maxClaudeContextWindowFromModelUsage, guard against
invalid contextWindow values before using Math.max: skip any value.contextWindow
that is undefined, null or not a finite number (use Number.isFinite or
equivalent) and only feed valid finite numbers into Math.max so maxContextWindow
cannot become NaN; keep references to the existing variables/function names
(maxClaudeContextWindowFromModelUsage, maxContextWindow, contextWindow,
ModelUsage) and initialize/compare against 0 when computing the maximum.
- Around line 1417-1430: The merge incorrectly allows
accumulatedTotalProcessedTokens to overwrite a larger existing
totalProcessedTokens because it only compares against lastGoodUsage.usedTokens;
update the conditional that builds usageSnapshot so that
accumulatedTotalProcessedTokens is compared against
lastGoodUsage.totalProcessedTokens when present (fallback to
lastGoodUsage.usedTokens only if totalProcessedTokens is undefined), i.e., in
ClaudeAdapter.ts where accumulatedTotalProcessedTokens, lastGoodUsage
(context.lastKnownTokenUsage) and usageSnapshot are constructed, guard setting
totalProcessedTokens by ensuring accumulatedTotalProcessedTokens >
(lastGoodUsage.totalProcessedTokens ?? lastGoodUsage.usedTokens) before
assigning.
In `@apps/web/src/components/Sidebar.tsx`:
- Around line 2887-2914: Add an early-return guard at the top of both
onWindowKeyDown and onWindowKeyUp so none of the traversal or jump-hint logic
runs while the command palette is open: call getCurrentSidebarShortcutContext()
(or the existing context getter used in these handlers) and if its
commandPaletteOpen (or equivalent flag) is true, return immediately; do this
before calling shouldIgnoreThreadJumpHintUpdate, shouldShowThreadJumpHints,
setThreadJumpLabelByKey, clearThreadJumpHints, updateThreadJumpHintsVisibility,
etc., so the handlers do nothing while the palette is open.
In `@apps/web/src/lib/chatThreadActions.ts`:
- Around line 63-67: The default builder buildDefaultThreadOptions uses
context.defaultThreadEnvMode which allows non-local drafts; ensure the
"newLocal" path forces local mode by overriding envMode to "local" when creating
local threads—either add a dedicated builder (e.g., buildLocalThreadOptions) or
change startNewLocalThreadFromContext to call buildDefaultThreadOptions but then
set options.envMode = "local" (or pass an explicit envMode override) so newLocal
always creates a local draft; update the analogous creation path referenced
around lines 88–97 as well.
- Around line 69-74: The function startNewThreadInProjectFromContext currently
always passes buildContextualThreadOptions(context) which can leak
branch/worktree/env from the current context into a different project; change it
to only use the contextual options when the context refers to the same project
(compare context.projectRef or equivalent) or construct/clean a new
ThreadOptions for the target project that resets project-scoped fields (branch,
worktree, env) before calling context.handleNewThread; update references to
buildContextualThreadOptions, startNewThreadInProjectFromContext, and
handleNewThread to implement this conditional/cleaning behavior.
🪄 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: bcb3968e-bb7b-444e-bc85-bcbe88be7b44
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
KEYBINDINGS.mdapps/desktop/src/main.tsapps/server/src/git/Layers/GitCore.test.tsapps/server/src/git/Layers/GitCore.tsapps/server/src/keybindings.tsapps/server/src/open.tsapps/server/src/provider/Layers/ClaudeAdapter.test.tsapps/server/src/provider/Layers/ClaudeAdapter.tsapps/web/src/commandPaletteStore.tsapps/web/src/components/ChatView.browser.tsxapps/web/src/components/ChatView.tsxapps/web/src/components/CommandPalette.logic.test.tsapps/web/src/components/CommandPalette.logic.tsapps/web/src/components/CommandPalette.tsxapps/web/src/components/CommandPaletteResults.tsxapps/web/src/components/Sidebar.logic.test.tsapps/web/src/components/Sidebar.logic.tsapps/web/src/components/Sidebar.tsxapps/web/src/components/ui/command.tsxapps/web/src/components/ui/toast.tsxapps/web/src/composerHandleContext.tsapps/web/src/keybindings.test.tsapps/web/src/lib/chatThreadActions.test.tsapps/web/src/lib/chatThreadActions.tsapps/web/src/lib/threadSort.test.tsapps/web/src/lib/threadSort.tsapps/web/src/routes/__root.tsxapps/web/src/routes/_chat.tsxapps/web/src/types.tsapps/web/vitest.browser.config.tspackage.jsonpackages/contracts/src/keybindings.test.tspackages/contracts/src/keybindings.ts
💤 Files with no reviewable changes (1)
- apps/web/src/components/Sidebar.logic.test.ts
✅ Files skipped from review due to trivial changes (9)
- KEYBINDINGS.md
- apps/web/src/routes/__root.tsx
- apps/web/src/components/ui/toast.tsx
- packages/contracts/src/keybindings.test.ts
- package.json
- apps/web/src/composerHandleContext.ts
- apps/web/src/lib/chatThreadActions.test.ts
- apps/web/src/lib/threadSort.test.ts
- apps/web/src/commandPaletteStore.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/contracts/src/keybindings.ts
- apps/server/src/keybindings.ts
- apps/web/src/keybindings.test.ts
- apps/web/vitest.browser.config.ts
- apps/web/src/components/ui/command.tsx
- apps/web/src/routes/_chat.tsx
- apps/server/src/provider/Layers/ClaudeAdapter.test.ts
- apps/web/src/components/CommandPalette.logic.test.ts
Comment on lines
+73
to
+123
| export function buildThreadActionItems(input: { | ||
| threads: ReadonlyArray< | ||
| Pick< | ||
| SidebarThreadSummary, | ||
| "archivedAt" | "branch" | "createdAt" | "environmentId" | "id" | "projectId" | "title" | ||
| > & { | ||
| updatedAt?: string | undefined; | ||
| latestUserMessageAt?: string | null; | ||
| } | ||
| >; | ||
| activeThreadId?: Thread["id"]; | ||
| projectTitleById: ReadonlyMap<Project["id"], string>; | ||
| sortOrder: SidebarThreadSortOrder; | ||
| icon: ReactNode; | ||
| runThread: (thread: Pick<SidebarThreadSummary, "environmentId" | "id">) => Promise<void>; | ||
| limit?: number; | ||
| }): CommandPaletteActionItem[] { | ||
| const sortedThreads = sortThreads( | ||
| input.threads.filter((thread) => thread.archivedAt === null), | ||
| input.sortOrder, | ||
| ); | ||
| const visibleThreads = | ||
| input.limit === undefined ? sortedThreads : sortedThreads.slice(0, input.limit); | ||
|
|
||
| return visibleThreads.map((thread) => { | ||
| const projectTitle = input.projectTitleById.get(thread.projectId); | ||
| const descriptionParts: string[] = []; | ||
|
|
||
| if (projectTitle) { | ||
| descriptionParts.push(projectTitle); | ||
| } | ||
| if (thread.branch) { | ||
| descriptionParts.push(`#${thread.branch}`); | ||
| } | ||
| if (thread.id === input.activeThreadId) { | ||
| descriptionParts.push("Current thread"); | ||
| } | ||
|
|
||
| return { | ||
| kind: "action", | ||
| value: `thread:${thread.id}`, | ||
| searchTerms: [thread.title, projectTitle ?? "", thread.branch ?? ""], | ||
| title: thread.title, | ||
| description: descriptionParts.join(" · "), | ||
| timestamp: formatRelativeTimeLabel(thread.updatedAt ?? thread.createdAt), | ||
| icon: input.icon, | ||
| run: async () => { | ||
| await input.runThread(thread); | ||
| }, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Use environment-scoped identity for thread search items.
This builder keys project titles by bare projectId and gives each thread item the bare thread.id in value, even though the palette is populated across environments. If two environments reuse ids, project labels can be overwritten and command rows can collide in the list. Please carry an environment-scoped key for both the project-title lookup and the thread item identity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/CommandPalette.logic.ts` around lines 73 - 123,
buildThreadActionItems currently uses bare projectId and thread.id which can
collide across environments; change lookups and item identities to be
environment-scoped by incorporating thread.environmentId into keys.
Specifically, update logic that reads from projectTitleById (and any
map/population code elsewhere) to use a composite key that includes
environmentId (e.g., `${environmentId}:${projectId}`) when calling
projectTitleById.get, and change the action item value from
`thread:${thread.id}` to include environment (e.g.,
`thread:${thread.environmentId}:${thread.id}`) so items and labels are unique
per environment; adjust searchTerms/title/description handling only if they rely
on the bare id. Use the function and symbol names buildThreadActionItems,
projectTitleById, thread.environmentId, and value to locate and apply the
changes.
Comment on lines
+16
to
+18
| function getLatestUserMessageTimestamp(thread: ThreadSortInput): number { | ||
| if (thread.latestUserMessageAt) { | ||
| return toSortableTimestamp(thread.latestUserMessageAt) ?? Number.NEGATIVE_INFINITY; |
There was a problem hiding this comment.
Fall back when latestUserMessageAt is present but invalid.
A truthy but unparsable latestUserMessageAt currently returns -Infinity, which pushes the thread to the end even if messages or updatedAt are valid. Only short-circuit when parsing succeeds.
Suggested fix
function getLatestUserMessageTimestamp(thread: ThreadSortInput): number {
if (thread.latestUserMessageAt) {
- return toSortableTimestamp(thread.latestUserMessageAt) ?? Number.NEGATIVE_INFINITY;
+ const latestUserMessageTimestamp = toSortableTimestamp(thread.latestUserMessageAt);
+ if (latestUserMessageTimestamp !== null) {
+ return latestUserMessageTimestamp;
+ }
}📝 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 getLatestUserMessageTimestamp(thread: ThreadSortInput): number { | |
| if (thread.latestUserMessageAt) { | |
| return toSortableTimestamp(thread.latestUserMessageAt) ?? Number.NEGATIVE_INFINITY; | |
| function getLatestUserMessageTimestamp(thread: ThreadSortInput): number { | |
| if (thread.latestUserMessageAt) { | |
| const latestUserMessageTimestamp = toSortableTimestamp(thread.latestUserMessageAt); | |
| if (latestUserMessageTimestamp !== null) { | |
| return latestUserMessageTimestamp; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/lib/threadSort.ts` around lines 16 - 18, The function
getLatestUserMessageTimestamp currently returns Number.NEGATIVE_INFINITY
whenever thread.latestUserMessageAt is present but
toSortableTimestamp(thread.latestUserMessageAt) fails to parse; change the logic
so you only short-circuit and return when toSortableTimestamp returns a valid
numeric timestamp (non-null/undefined), otherwise fall through to the existing
fallbacks (e.g., examine thread.messages or thread.updatedAt). Update the branch
that references latestUserMessageAt to call toSortableTimestamp once, test the
result for success, and only return that value when valid; if parsing fails,
continue with the rest of the function instead of returning -Infinity.
Upstream additions: - fix: Align token usage metrics for both Claude and Codex (pingdotgg#1943) - fix(web): allow concurrent browser tests to retry ports (pingdotgg#1951) - fix: quote editor launch args on Windows for paths with spaces (pingdotgg#1805) - Coalesce status refreshes by remote (pingdotgg#1940) - chore(desktop): separate dev AppUserModelID on Windows (pingdotgg#1934) - feat(web): add extensible command palette (pingdotgg#1103) Fork adaptations: - Took upstream's extensible command palette (replaces fork's simpler version) - Took upstream's extracted thread sort logic (Sidebar.logic.ts → lib/threadSort.ts) - Inline ModelUsage/NonNullableUsage types (not yet re-exported from SDK) - Updated claude-agent-sdk to 0.2.104 - Made Thread.archivedAt required (matches upstream) - Removed duplicate commandPalette.toggle keybinding - Gate sidebar shortcuts when command palette is open (CodeRabbit review)
aaditagrawal
force-pushed
the
sync/upstream-main-2026-04-13
branch
from
0123393 to
b7f2969
Compare
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
6 participants
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
Merges 6 new upstream commits from
pingdotgg/t3codewith proper ancestry recording.Upstream additions:
Fork adaptations:
Sidebar.logic.ts→lib/threadSort.ts)ModelUsage/NonNullableUsagetypes (not yet re-exported from SDK entry)@anthropic-ai/claude-agent-sdkto 0.2.104Thread.archivedAtrequired (matches upstream)git log main..upstream/main→ 0 commits after mergeTest plan
bun typecheck— 0 errorsbun run build— 0 errorsbun run fmt— cleanSummary by CodeRabbit
New Features
Improvements
Documentation