Merge upstream pingdotgg/t3code main (35 commits)#11
Merged
aaditagrawal merged 41 commits intomainfrom Mar 13, 2026
Merged
Conversation
… `useLocalStorage` helper (pingdotgg#662) Co-authored-by: Julius Marminge <julius0216@outlook.com>
Co-authored-by: Julius Marminge <julius0216@outlook.com>
…lan mode (pingdotgg#867) Co-authored-by: Julius Marminge <julius0216@outlook.com>
- move chat-wide key handling into `_chat` route-level shortcut handler - extract reusable `useHandleNewThread` hook and `isTerminalFocused` helper - update browser WS fixture to support `terminalOpen` RPC shape
- ensure `chat.new` creates a fresh draft after a promoted draft thread - enforce terminal cap per split group (4) while allowing additional terminal groups - refine sidebar row selected/active styling via shared class-name logic and tests
…otgg#900) Co-authored-by: Julius Marminge <julius0216@outlook.com>
Co-authored-by: Julius Marminge <julius0216@outlook.com>
Co-authored-by: hobostay <hobostay@users.noreply.github.com>
…gg#949) Co-authored-by: hobostay <hobostay@users.noreply.github.com>
Co-authored-by: Julius Marminge <julius0216@outlook.com>
…d Linux) (pingdotgg#841) Co-authored-by: Julius Marminge <julius0216@outlook.com>
…dotgg#1001) Co-authored-by: Julius Marminge <julius0216@outlook.com>
…ingdotgg#988) Co-authored-by: Julius Marminge <julius0216@outlook.com>
…dotgg#955) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: Julius Marminge <julius0216@outlook.com>
Co-authored-by: Julius Marminge <julius0216@outlook.com>
…tgg#1006) Co-authored-by: macroscopeapp[bot] <170038800+macroscopeapp[bot]@users.noreply.github.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Julius Marminge <juliusmarminge@users.noreply.github.com> Co-authored-by: codex <codex@users.noreply.github.com> Co-authored-by: cursor[bot] <206951365+cursor[bot]@users.noreply.github.com>
Upstream features: - Selective file staging in commit dialog (pingdotgg#872) - Compact Codex tool-call icons in chat timeline (pingdotgg#988) - Replace worktree toggle with Select dropdown (pingdotgg#1001) - Timestamp format override setting (pingdotgg#855) - Preferred editor resolver + useLocalStorage helper (pingdotgg#662) - Extract reusable clipboard hook and media queries (pingdotgg#1006) - Default thread env mode setting (pingdotgg#892) - Block image uploads during plan mode (pingdotgg#621) Upstream bug fixes: - Prompt editor + Plan mode cursor fixes (pingdotgg#867) - Composer @file autocomplete cursor/spacing bugs (pingdotgg#936) - Diff panel unclosable after retainSearchParams (pingdotgg#937) - Fix agent response duration always 1ms (pingdotgg#866) - Defer diff worker startup until diff opens (pingdotgg#934) - Fix new-thread shortcuts when terminal focused (pingdotgg#984) - PlanSidebar memory leak fix (pingdotgg#949) - WebSocket error logging (pingdotgg#948) - Desktop "Check for Updates" dialog (pingdotgg#955) - macOS logo alignment fix (pingdotgg#960) Upstream tooling: - Upgrade oxfmt/oxlint versions (pingdotgg#1010) - Update @vitejs/plugin-react to 6.0.0 (pingdotgg#1002) - Upgrade marketing app to Astro 6 (pingdotgg#1005) Conflict resolutions: - workspaceEntries.ts: kept our zortos-ported fuzzy search (better ranked insertion) over upstream's pingdotgg#256 - appSettings.ts: merged our display settings with upstream's timestamp format and env mode settings - MessagesTimeline.tsx: merged our showCommandOutput gating with upstream's compact tool-call icons - DiffPanel.tsx: fixed duplicate useAppSettings import - open.test.ts: added missing node:fs/os/path imports for new test
github-actions
bot
added
the
vouch:trusted
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds selective git staging and filePaths propagation, preferred-editor persistence and editor additions, per-group terminal limits, timestamp-format utilities, many new React hooks (localStorage, copy, media, new-thread), UI refinements, CI/workflow upgrades (PR-size labeling), contract extensions (filePaths, provider.connected, tool lifecycle types), and assorted tests and minor fixes across desktop, server, and web. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebUI as Web UI (client)
participant API as Server API
participant GitMgr as GitManager
participant GitCore as GitCore/Git CLI
User->>WebUI: Select files and trigger commit
WebUI->>API: POST /git/runStackedAction (includes filePaths)
API->>GitMgr: resolveCommitAndBranchSuggestion(filePaths)
GitMgr->>GitCore: prepareCommitContext(cwd, filePaths)
GitCore->>GitCore: git reset && git add -A -- <paths>
GitCore-->>GitMgr: preparedContext
GitMgr->>GitCore: runCommitStep / create commit
GitCore-->>API: commit result
API-->>WebUI: success/result
WebUI-->>User: show toast / update UI
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
apps/server/src/opencodeServerManager.ts (1)
1196-1209:⚠️ Potential issue | 🟡 MinorCarry
connectedIdsthrough the config-provider fallback.When
provider.list()returns noallentries, this falls back toconfig.providersbut drops theconnectedIdsyou just derived. Those models then reach the picker withconnectedunset, andapps/web/src/components/chat/ProviderModelPicker.tsxtreats them as connected.[suggested fix]
Apply the same connectivity metadata on the fallback path
- return parseProviderModels(configured.providers); + return parseProviderModels(configured.providers, connectedIds);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/opencodeServerManager.ts` around lines 1196 - 1209, The fallback path that reads configured providers drops the connectivity metadata because connectedIds (created from payload.connected) isn’t passed into parseProviderModels; update the fallback to apply the same connectivity mapping by passing connectedIds into parseProviderModels when processing configured providers (call parseProviderModels(configured.providers, connectedIds) or otherwise merge connectedIds into the provider entries before calling parseProviderModels) so models returned from readConfigProvidersResponse/readJsonData/client.config.providers preserve the connected state used by ProviderModelPicker.tsx.apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts (1)
445-455:⚠️ Potential issue | 🟡 MinorKeep completion wording out of the shared item title.
event.payload.titleis also used by theitem.startedanditem.updatedbranches above. Using it verbatim here forces providers to encode completion-tense wording in a field that is shared across the whole lifecycle, so in-progress command updates can now surface as “Ran command” before the command has actually finished. Derive the completion phrasing in this branch and keeppayload.titlelifecycle-neutral instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts` around lines 445 - 455, The item.completed branch currently uses event.payload.title (shared across lifecycle) directly for the completed summary, causing premature completion wording; in the case "item.completed" handler (the branch producing objects with id/eventId, createdAt, tone/kind, summary), replace use of event.payload.title verbatim with a derived completion phrase (e.g., synthesize "Finished: <payload.title>" or "Completed: <payload.title>") so payload.title remains lifecycle-neutral; update the summary construction in the case "item.completed" branch accordingly (leave event.payload.title unchanged elsewhere such as item.started or item.updated).apps/server/src/kiloServerManager.ts (1)
1137-1145:⚠️ Potential issue | 🟡 MinorPreserve
connectedstate in the config fallback path.On Line 1145, fallback parsing drops
connectedIds, so models returned fromconfig.providerslose the newconnectedindicator whenprovider.listis empty.Suggested fix
- return parseProviderModels(configured.providers); + return parseProviderModels(configured.providers, connectedIds);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/kiloServerManager.ts` around lines 1137 - 1145, The fallback path drops the connected state because parseProviderModels is called without the connectedIds set; preserve payload.connected by passing the same connectedIds set (created from payload.connected) into the fallback parsing. Update the call that currently does parseProviderModels(configured.providers) to use parseProviderModels(configured.providers, connectedIds) (or otherwise merge connectedIds into configured.providers before calling parseProviderModels), ensuring the same connectedIds variable created earlier is reused so providers from readConfigProvidersResponse retain the connected flag.apps/web/src/components/ui/sidebar.tsx (2)
539-551:⚠️ Potential issue | 🟠 MajorTreat persisted-width decode failures as non-fatal.
getLocalStorageItem(..., Schema.Finite)throws on malformed or stale values. Letting that escape from this effect can break sidebar mount for a single bad localStorage entry; catch and fall back to the default width instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/ui/sidebar.tsx` around lines 539 - 551, The effect assumes getLocalStorageItem(resolvedResizable.storageKey, Schema.Finite) won't throw, but a malformed/stale value can surface an exception and break mount; wrap the call to getLocalStorageItem in a try/catch, treat decode failures as non-fatal by ignoring the stored value (i.e., act as if storedWidth is null), and continue using clampSidebarWidth/resolvedResizable defaults and calling resolvedResizable.onResize as before; update the block referencing railRef, wrapper, getLocalStorageItem, Schema.Finite, clampSidebarWidth and resolvedResizable.onResize to handle and log (optional) the caught error while falling back to default behavior.
363-386:⚠️ Potential issue | 🟠 MajorGuarantee resize cleanup even if persistence throws.
The new storage write and
onResizecallback run before pointer-capture and body-style cleanup. If either throws, the rail can stay captured and the page can be left in resize mode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/ui/sidebar.tsx` around lines 363 - 386, The stopResize function can leave pointer capture and body styles in place if setLocalStorageItem or resolvedResizable.onResize throws; wrap the persistence and callback calls in a try/catch or move them into a safe block so cleanup always runs: ensure resizeState.rail.releasePointerCapture(pointerId), document.body.style.removeProperty("cursor"), document.body.style.removeProperty("user-select"), cancelAnimationFrame and transition-duration removals occur in a finally-like path before persisting (setLocalStorageItem) and invoking resolvedResizable.onResize, or alternatively catch errors from persistence/callback and log them while still performing the pointer capture and body-style cleanup; reference stopResize, resizeStateRef.current, resizeState.rail.releasePointerCapture, setLocalStorageItem, and resolvedResizable.onResize when making the change.apps/web/src/components/GitActionsControl.tsx (1)
398-409:⚠️ Potential issue | 🔴 CriticalMake the post-commit CTA truly push-only, or explicitly skip the commit phase.
After a partial commit, the excluded files remain in the worktree. Line 403 calls
commit_pushwithout preservingfilePaths, so clicking Push will stage and commit those excluded files instead of only pushing the commit that was just created. Either passfilePaths: selectedFiles.map((f) => f.path)to limit the commit to already-included files, or introduce a skip-commit flag if only a push is intended.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/GitActionsControl.tsx` around lines 398 - 409, The Push CTA currently calls runGitActionWithToast with action: "commit_push" but doesn't pass filePaths, so excluded files can be staged/committed; update the Push branch to either pass filePaths: selectedFiles.map(f => f.path) to restrict the commit to already-included files or add and pass a skipCommit flag (e.g., skipCommit: true) to signal runGitActionWithToast to only push without committing; modify the call site in GitActionsControl (the Push branch that sets actionProps children="Push" and onClick) and ensure the runGitActionWithToast handler respects the new filePaths or skipCommit parameter (and preserve existing params like forcePushOnlyProgress, onConfirmed: closeResultToast, statusOverride: actionStatus, isDefaultBranchOverride: actionIsDefaultBranch).
🧹 Nitpick comments (9)
apps/web/src/wsTransport.ts (1)
193-195: Avoid logging the full WebSocket URL in client warnings.This logs
this.urlverbatim; if the URL ever carries query tokens/credentials, they’ll be exposed in browser logs. Log a sanitized URL instead (e.g., origin + pathname only).Suggested patch
ws.addEventListener("error", (event) => { // Log WebSocket errors for debugging (close event will follow) - console.warn("WebSocket connection error", { type: event.type, url: this.url }); + let safeUrl = this.url; + try { + const parsed = new URL(this.url); + safeUrl = `${parsed.origin}${parsed.pathname}`; + } catch { + // keep fallback value + } + console.warn("WebSocket connection error", { type: event.type, url: safeUrl }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/wsTransport.ts` around lines 193 - 195, The WebSocket error handler logs this.url directly (in ws.addEventListener("error", ...)), which can leak query tokens; change the handler to log a sanitized URL instead by parsing this.url (e.g., const u = new URL(this.url)) and logging u.origin + u.pathname (or u.host + u.pathname) rather than the full href; ensure you guard against invalid URLs with a try/catch and fall back to a safe placeholder when parsing fails, and update the process of logging in the error listener to use that sanitized value.apps/server/src/open.test.ts (1)
135-152: Inconsistent use of rawnode:fsinstead of Effect FileSystem.This test uses raw
fs.mkdtempSync,fs.mkdirSync, andfs.writeFileStringwhile other tests in the same file (lines 268-332, 335-350) have been refactored to use Effect'sFileSystemwithmakeTempDirectoryScoped()andwriteFileString().Using
makeTempDirectoryScoped()would ensure automatic cleanup and maintain consistency with the rest of the file.♻️ Suggested refactor for consistency
it.effect("uses the containing directory when terminal editors receive a file path", () => Effect.gen(function* () { - const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "t3-open-ghostty-")); - const filePath = path.join(tempDir, "nested", "AGENTS.md"); - fs.mkdirSync(path.dirname(filePath), { recursive: true }); - fs.writeFileSync(filePath, "# test\n", "utf8"); + const fileSystem = yield* FileSystem.FileSystem; + const pathService = yield* Path.Path; + const tempDir = yield* fileSystem.makeTempDirectoryScoped({ prefix: "t3-open-ghostty-" }); + const filePath = pathService.join(tempDir, "nested", "AGENTS.md"); + yield* fileSystem.makeDirectory(pathService.dirname(filePath), { recursive: true }); + yield* fileSystem.writeFileString(filePath, "# test\n"); const ghosttyFileTarget = yield* resolveEditorLaunch( { cwd: `${filePath}:48`, editor: "ghostty" }, "linux", ); assert.deepEqual(ghosttyFileTarget, { command: "ghostty", - args: [`--working-directory=${path.dirname(filePath)}`], + args: [`--working-directory=${pathService.dirname(filePath)}`], }); }), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/open.test.ts` around lines 135 - 152, The test "uses the containing directory when terminal editors receive a file path" currently uses raw node:fs calls (fs.mkdtempSync, fs.mkdirSync, fs.writeFileSync); replace these with Effect FileSystem helpers: acquire a temp dir with makeTempDirectoryScoped(), create the nested directory and write the file using writeFileString() (or equivalent Effect FS ops) inside the Effect.gen flow so resource cleanup is automatic, then compute filePath and call resolveEditorLaunch({ cwd: `${filePath}:48`, editor: "ghostty" }, "linux") as before; ensure you import/use the same FileSystem helpers used elsewhere in this file and keep the assertion on ghosttyFileTarget unchanged.apps/server/src/workspaceEntries.ts (2)
40-43:RankedWorkspaceEntryis declared but not used.Either use it in
compareRankedEntries/insertRankedEntryand therankedarray, or remove it to avoid duplicate shapes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/workspaceEntries.ts` around lines 40 - 43, The declared interface RankedWorkspaceEntry is unused; either remove it or adopt it for the ranked array and related helpers—update the type of the ranked collection to RankedWorkspaceEntry[] and change the signatures/usage in compareRankedEntries and insertRankedEntry to accept/return RankedWorkspaceEntry (referencing the RankedWorkspaceEntry symbol and the functions compareRankedEntries and insertRankedEntry and the ranked variable) so you have a single canonical shape, or delete the interface if you prefer inline types.
68-75: Use precomputed normalized fields in scoring.
toSearchableWorkspaceEntryprecomputesnormalizedPath/normalizedName, butscoreEntryrecomputes them for every match. Wire the scoring path toSearchableWorkspaceEntryto avoid repeated per-entry string work.♻️ Suggested refactor
-function scoreEntry(entry: ProjectEntry, query: string): number { +function scoreEntry(entry: SearchableWorkspaceEntry, query: string): number { if (!query) { return entry.kind === "directory" ? 0 : 1; } - const normalizedPath = entry.path.toLowerCase(); - const normalizedName = basenameOf(normalizedPath); + const { normalizedPath, normalizedName } = entry;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/workspaceEntries.ts` around lines 68 - 75, The scoring currently re-normalizes strings per match; update scoreEntry to accept a SearchableWorkspaceEntry (instead of ProjectEntry) and use its precomputed normalizedPath and normalizedName fields (from toSearchableWorkspaceEntry) rather than recomputing basenameOf/ toLowerCase inside scoreEntry; update all call sites that pass ProjectEntry to first convert via toSearchableWorkspaceEntry and then call scoreEntry so the per-entry string work is done once when entries are created.apps/web/src/components/Sidebar.logic.test.ts (1)
179-201: Good coverage for active/selected combinations.The tests cover three of four state combinations well. Consider adding a test for the "neither active nor selected" case to complete the matrix:
💡 Suggested additional test
it("uses muted foreground for inactive unselected threads", () => { const className = resolveThreadRowClassName({ isActive: false, isSelected: false }); expect(className).toContain("text-muted-foreground"); expect(className).toContain("hover:bg-accent"); expect(className).not.toContain("bg-primary"); expect(className).not.toContain("bg-accent/85"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/Sidebar.logic.test.ts` around lines 179 - 201, Add a fourth unit test for resolveThreadRowClassName to cover the case where a thread is neither active nor selected (isActive: false, isSelected: false); assert it returns the muted/inactive styles (e.g., contains "text-muted-foreground" and "hover:bg-accent") and does not include primary/active backgrounds (e.g., does not contain "bg-primary" or "bg-accent/85") so the full 2x2 state matrix is tested.apps/web/src/hooks/useCopyToClipboard.ts (1)
22-26: Consider logging when clipboard API is unavailable without an error handler.When
navigator.clipboardis unavailable and noonErrorcallback is provided, the function silently returns. This could make debugging difficult in environments where the clipboard API is unexpectedly missing.💡 Suggested improvement
const copyToClipboard = React.useCallback((value: string, ctx: TContext): void => { if (typeof window === "undefined" || !navigator.clipboard?.writeText) { - onErrorRef.current?.(new Error("Clipboard API unavailable."), ctx); + const error = new Error("Clipboard API unavailable."); + if (onErrorRef.current) { + onErrorRef.current(error, ctx); + } else { + console.warn(error.message); + } return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/useCopyToClipboard.ts` around lines 22 - 26, The copyToClipboard callback in useCopyToClipboard silently returns when the Clipboard API is unavailable and no onError handler is registered; update copyToClipboard (and use onErrorRef) to emit a visible log (e.g., console.warn or console.error) when navigator.clipboard?.writeText is missing and onErrorRef.current is undefined so developers can see the failure during debugging — include a clear message and the ctx value for context and still call onErrorRef.current if present.apps/server/src/git/Layers/GitManager.test.ts (1)
887-891: Tighten porcelain assertion to reduce false positives.Line [890] currently checks only filename presence. As an optional improvement, assert exact porcelain state (
?? b.txt) so staged-but-uncommitted cases don’t pass accidentally.Optional test-hardening diff
- expect(statusStdout).toContain("b.txt"); + expect(statusStdout).toContain("?? b.txt"); expect(statusStdout).not.toContain("a.txt");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/git/Layers/GitManager.test.ts` around lines 887 - 891, The test currently asserts that statusStdout contains "b.txt" which can match other states; tighten the assertion in the Git status check by asserting the exact porcelain token and filename (e.g. "?? b.txt" or the exact staged flag you expect) instead of just the filename. Update the assertion around the runGit(...).pipe(Effect.map((r) => r.stdout)) result (statusStdout) to check the full porcelain line for b.txt so staged, modified, or other states don't cause false positives.apps/web/src/appSettings.ts (1)
253-336: Consider removing unused sync infrastructure.After switching
useAppSettingsto useuseLocalStorage(which handles its own persistence and cross-tab sync), the following appear to be orphaned:
listenersarray andemitChange()functionsubscribe()functionpersistSettings()functionThe
useLocalStoragehook already handles localStorage persistence and cross-tab synchronization via storage events, so this infrastructure is now redundant.
getAppSettingsSnapshotis still exported and may be used elsewhere, so it should be retained if needed for synchronous reads outside React components.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/appSettings.ts` around lines 253 - 336, Remove the now-orphaned localStorage sync/persistence helpers: delete the listeners array, emitChange(), subscribe(), and persistSettings() from appSettings.ts and update any internal references (search for "listeners", "emitChange", "subscribe", "persistSettings") so nothing calls them; keep and export getAppSettingsSnapshot (and parsePersistedSettings/migratePersistedAppSettings if still used) for synchronous reads, and ensure useAppSettings now relies solely on the useLocalStorage hook for persistence and cross-tab sync.apps/web/src/components/GitActionsControl.tsx (1)
514-517: Consider centralizing the commit-dialog reset state.The same reset sequence is duplicated across every close/submit path now. A small helper would keep
excludedFiles/isEditingFiles/ message cleanup in sync and make future changes less error-prone.♻️ Suggested refactor
+ const resetCommitDialogState = useCallback(() => { + setIsCommitDialogOpen(false); + setDialogCommitMessage(""); + setExcludedFiles(new Set()); + setIsEditingFiles(false); + }, []); ... - setIsCommitDialogOpen(false); - setDialogCommitMessage(""); - setExcludedFiles(new Set()); - setIsEditingFiles(false); + resetCommitDialogState();Also applies to: 587-589, 597-600, 776-777, 914-917
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/GitActionsControl.tsx` around lines 514 - 517, Duplicate commit-dialog teardown code should be extracted into a single helper (e.g., resetCommitDialogState or closeCommitDialog) that calls setIsCommitDialogOpen(false), setDialogCommitMessage(""), setExcludedFiles(new Set()), and setIsEditingFiles(false); replace each duplicated sequence (currently using those four setters in close/submit paths at locations around the handlers referencing setIsCommitDialogOpen, setDialogCommitMessage, setExcludedFiles, setIsEditingFiles) with a single call to that helper so all cleanup is centralized and future changes need only be made in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/pr-size.yml:
- Around line 3-5: The workflow only listens to pull_request_target so the
label-definition sync job ('label' job) can never create missing size:* labels
on fresh repos; update the workflow triggers and conditional checks so the job
is reachable: add pull_request (and/or workflow_dispatch) to the top-level on:
map so events delivered to the label job, and update any job if: expressions
that currently gate execution (the checks around lines referenced 57-60 and
115-130) to include github.event_name == 'pull_request' or github.event_name ==
'workflow_dispatch' (or remove the overly strict event-name guard) so the
'label' job can run and create labels when needed. Ensure the 'label' job
id/name remains the same so downstream steps keep references.
In @.github/workflows/release.yml:
- Around line 256-257: The "Publish CLI package" workflow step currently runs
"node apps/server/scripts/cli.ts publish ..." which will fail because Node
cannot execute a TypeScript file directly and the npm publish lacks
authentication; change the step to execute a built JS file or run the TypeScript
via ts-node (or compile first) so the command targets a runnable JavaScript
entrypoint (reference the "Publish CLI package" run command and the script at
apps/server/scripts/cli.ts), and ensure the job exposes the authentication token
by adding env: NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} (or configure the
setup-node step to use NODE_AUTH_TOKEN from the NPM_TOKEN secret) so npm publish
can authenticate when registry-url is set.
- Around line 250-251: The workflow step named "Align package versions to
release version" tries to run a TypeScript file with Node.js; replace the
failing invocation so the TypeScript script is executed the same way other steps
do (e.g., use bun). Update the run command for
scripts/update-release-package-versions.ts to use bun (e.g., change `node
scripts/update-release-package-versions.ts ...` to `bun run
scripts/update-release-package-versions.ts ...`) so the .ts file is executed
correctly.
In `@apps/web/package.json`:
- Around line 46-59: The package manifest is missing the required `@babel/core`
peer dependency for the React/Babel toolchain; update apps/web package.json by
adding `@babel/core` to the devDependencies alongside the existing
"@rolldown/plugin-babel", "babel-plugin-react-compiler" and
"@vitejs/plugin-react" entries (pick a compatible ^7.x version matching your
babel-related packages), then reinstall to ensure builds don’t rely on workspace
hoisting.
In `@apps/web/src/components/ChatMarkdown.tsx`:
- Line 258: The call to openInPreferredEditor(api, targetPath) can throw and
currently drops rejections; wrap the call in proper error handling—either make
the enclosing function async and await openInPreferredEditor(api, targetPath)
inside a try/catch that logs the error (or shows a user notification), or append
a .catch(handler) to the returned promise to handle and report failures;
reference the openInPreferredEditor(api, targetPath) invocation and ensure any
caught errors are surfaced via process/user-visible logging or a toast.
In `@apps/web/src/components/ChatView.browser.tsx`:
- Around line 1090-1142: The test fires the keyboard event immediately but the
mocked serverGetConfig that installs the chat.new keybinding hydrates
asynchronously, causing flakes; after mountChatView returns, await keybinding
hydration before dispatching the KeyboardEvent by using a waitFor that asserts
the test fixture/mounted state contains the installed keybinding (e.g.
waitFor(() => mounted.serverConfig?.keybindings?.some(k => k.command ===
"chat.new")) or waitFor(() => mounted.keybindings?.includes("chat.new"))) then
dispatch the KeyboardEvent and proceed; apply the same change to the other test
at lines 1191-1200.
In `@apps/web/src/components/ChatView.tsx`:
- Around line 2171-2176: The current early return inside addComposerImages()
discards co-pasted text because onComposerPaste() already called
preventDefault(); change addComposerImages() to not swallow the paste: either
(preferred) have addComposerImages() return a boolean (e.g., true = images
handled, false = images rejected) and remove the toast/return from it so
onComposerPaste() can decide to show the toast and allow text handling, or move
the pendingUserInputs guard and toast into onComposerPaste() before calling
addComposerImages(); update callers accordingly (notably onComposerPaste()) so
co-pasted text is preserved when images are disallowed and toastManager.add(...)
is invoked by the paste handler based on the returned flag.
In `@apps/web/src/components/GitActionsControl.tsx`:
- Around line 803-811: The master and per-file Checkbox components are missing
accessible names so screen readers announce anonymous checkboxes; add explicit
accessible labels (e.g., aria-label or aria-labelledby) to the master Checkbox
rendered where allSelected/noneSelected and setExcludedFiles are used—use a
label like "Select all files" for the master Checkbox—and to each per-file
Checkbox in the per-file list (lines around the per-file map that references
f.path), using a label such as "Select file {f.path}" or "Stage file {f.path}"
so each checkbox is announced uniquely and clearly to assistive tech.
In `@apps/web/src/components/Icons.tsx`:
- Around line 376-383: The file embeds a huge base64 string
(ANTIGRAVITY_ICON_DATA_URL) so every import of Icons.tsx inflates bundles; move
the PNG out of the shared module and load it only when needed: delete the long
ANTIGRAVITY_ICON_DATA_URL from Icons.tsx, create a dedicated asset import or
separate component (e.g., a new AntigravityIcon module) that references the
image file by static URL or dynamic import, and update AntigravityIcon to render
the external src (or lazy-load the component with React.lazy/dynamic import) so
only consumers that render AntigravityIcon pay the payload.
In `@apps/web/src/components/Sidebar.tsx`:
- Around line 885-887: The current call swallows failures from
handleNewThread(projectId, { envMode: appSettings.defaultThreadEnvMode }) by
using .catch(() => undefined); instead remove the silent catch so rejections
propagate or replace it with explicit error handling that surfaces the failure
to the user (e.g., show a UI notification or call the existing error handler)
and/or rethrow after logging; update the call site where handleNewThread is
invoked so any rejection is not ignored and the add-project flow can respond
(e.g., by reporting the error, retrying, or rolling back) rather than silently
completing.
In `@apps/web/src/hooks/useLocalStorage.ts`:
- Around line 5-20: The current isomorphicLocalStorage resolves
window.localStorage at import time (which can throw) and the fallback shim's
key() incorrectly uses Record.keys(store) on a Map; change to a lazy resolver
like a getStorage() accessor that tries to return window.localStorage inside a
try/catch and falls back to an in-memory Map-based shim only when needed, and
fix the shim to implement the Storage contract: use
Array.from(store.keys())[index] ?? null for key(), have getItem return
string|null, and make setItem/removeItem return void (do not return Map
booleans). Update references to use getStorage() (or an exported function that
returns the Storage) instead of the eagerly-initialized isomorphicLocalStorage
to avoid import-time throws.
- Around line 74-87: The functional updater inside setValue (in
useLocalStorage.ts: setValue calling setStoredValue) performs side effects—calls
to setLocalStorageItem, removeLocalStorageItem and queueMicrotask(() =>
dispatchLocalStorageChange(key))—which must be moved out because React may
replay updaters; change setValue so its updater only computes and returns the
new value, and add a useEffect that watches storedValue to perform persistence
(setLocalStorageItem/removeLocalStorageItem) and dispatchLocalStorageChange(key)
after state updates.
In `@apps/web/src/hooks/useMediaQuery.ts`:
- Around line 22-25: The resolveMax function currently subtracts 1px for all
inputs causing numeric max queries to be off-by-one; change resolveMax so it
only subtracts 1 when value is a named Breakpoint (use BREAKPOINTS[value] - 1)
and when value is a number return `(max-width: ${value}px)` without subtracting;
update the resolveMax implementation and any callers that rely on its output
(resolveMax, BREAKPOINTS) to use the corrected behavior.
In `@apps/web/src/routes/_chat`.$threadId.tsx:
- Line 224: The className on the SidebarInset component contains an extra space
between "h-dvh" and "min-h-0"; edit the SidebarInset element's className (the
JSX prop on SidebarInset in _chat.$threadId.tsx) to remove the double space so
classes read "h-dvh min-h-0 ..." to match the rest of the file's formatting.
In `@apps/web/src/terminalStateStore.ts`:
- Around line 293-299: The current check only enforces MAX_TERMINALS_PER_GROUP
for new terminals (isNewTerminal), allowing existing terminals to be moved into
a full group; change the guard to always block adding a terminal ID that isn't
already present when the group is full. Replace the conditional using
isNewTerminal with a check that if destinationGroup.terminalIds does not include
terminalId AND destinationGroup.terminalIds.length >= MAX_TERMINALS_PER_GROUP
then return normalized, so functions/variables to update: isNewTerminal usage,
destinationGroup.terminalIds, terminalId, MAX_TERMINALS_PER_GROUP, and the early
return of normalized.
In `@packages/contracts/src/providerRuntime.ts`:
- Around line 126-128: Remove the runtime predicate isToolLifecycleItemType from
packages/contracts and keep only the exported literal tuple/schema
TOOL_LIFECYCLE_ITEM_TYPES and its associated types; move the runtime check into
a consumer or shared utility module (e.g., create an isToolLifecycleItemType
function in the consumer package or shared utils) that imports
TOOL_LIFECYCLE_ITEM_TYPES and performs the includes(...) check, and update any
imports/usages to reference the predicate in the new location instead of
packages/contracts.
---
Outside diff comments:
In `@apps/server/src/kiloServerManager.ts`:
- Around line 1137-1145: The fallback path drops the connected state because
parseProviderModels is called without the connectedIds set; preserve
payload.connected by passing the same connectedIds set (created from
payload.connected) into the fallback parsing. Update the call that currently
does parseProviderModels(configured.providers) to use
parseProviderModels(configured.providers, connectedIds) (or otherwise merge
connectedIds into configured.providers before calling parseProviderModels),
ensuring the same connectedIds variable created earlier is reused so providers
from readConfigProvidersResponse retain the connected flag.
In `@apps/server/src/opencodeServerManager.ts`:
- Around line 1196-1209: The fallback path that reads configured providers drops
the connectivity metadata because connectedIds (created from payload.connected)
isn’t passed into parseProviderModels; update the fallback to apply the same
connectivity mapping by passing connectedIds into parseProviderModels when
processing configured providers (call parseProviderModels(configured.providers,
connectedIds) or otherwise merge connectedIds into the provider entries before
calling parseProviderModels) so models returned from
readConfigProvidersResponse/readJsonData/client.config.providers preserve the
connected state used by ProviderModelPicker.tsx.
In `@apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts`:
- Around line 445-455: The item.completed branch currently uses
event.payload.title (shared across lifecycle) directly for the completed
summary, causing premature completion wording; in the case "item.completed"
handler (the branch producing objects with id/eventId, createdAt, tone/kind,
summary), replace use of event.payload.title verbatim with a derived completion
phrase (e.g., synthesize "Finished: <payload.title>" or "Completed:
<payload.title>") so payload.title remains lifecycle-neutral; update the summary
construction in the case "item.completed" branch accordingly (leave
event.payload.title unchanged elsewhere such as item.started or item.updated).
In `@apps/web/src/components/GitActionsControl.tsx`:
- Around line 398-409: The Push CTA currently calls runGitActionWithToast with
action: "commit_push" but doesn't pass filePaths, so excluded files can be
staged/committed; update the Push branch to either pass filePaths:
selectedFiles.map(f => f.path) to restrict the commit to already-included files
or add and pass a skipCommit flag (e.g., skipCommit: true) to signal
runGitActionWithToast to only push without committing; modify the call site in
GitActionsControl (the Push branch that sets actionProps children="Push" and
onClick) and ensure the runGitActionWithToast handler respects the new filePaths
or skipCommit parameter (and preserve existing params like
forcePushOnlyProgress, onConfirmed: closeResultToast, statusOverride:
actionStatus, isDefaultBranchOverride: actionIsDefaultBranch).
In `@apps/web/src/components/ui/sidebar.tsx`:
- Around line 539-551: The effect assumes
getLocalStorageItem(resolvedResizable.storageKey, Schema.Finite) won't throw,
but a malformed/stale value can surface an exception and break mount; wrap the
call to getLocalStorageItem in a try/catch, treat decode failures as non-fatal
by ignoring the stored value (i.e., act as if storedWidth is null), and continue
using clampSidebarWidth/resolvedResizable defaults and calling
resolvedResizable.onResize as before; update the block referencing railRef,
wrapper, getLocalStorageItem, Schema.Finite, clampSidebarWidth and
resolvedResizable.onResize to handle and log (optional) the caught error while
falling back to default behavior.
- Around line 363-386: The stopResize function can leave pointer capture and
body styles in place if setLocalStorageItem or resolvedResizable.onResize
throws; wrap the persistence and callback calls in a try/catch or move them into
a safe block so cleanup always runs: ensure
resizeState.rail.releasePointerCapture(pointerId),
document.body.style.removeProperty("cursor"),
document.body.style.removeProperty("user-select"), cancelAnimationFrame and
transition-duration removals occur in a finally-like path before persisting
(setLocalStorageItem) and invoking resolvedResizable.onResize, or alternatively
catch errors from persistence/callback and log them while still performing the
pointer capture and body-style cleanup; reference stopResize,
resizeStateRef.current, resizeState.rail.releasePointerCapture,
setLocalStorageItem, and resolvedResizable.onResize when making the change.
---
Nitpick comments:
In `@apps/server/src/git/Layers/GitManager.test.ts`:
- Around line 887-891: The test currently asserts that statusStdout contains
"b.txt" which can match other states; tighten the assertion in the Git status
check by asserting the exact porcelain token and filename (e.g. "?? b.txt" or
the exact staged flag you expect) instead of just the filename. Update the
assertion around the runGit(...).pipe(Effect.map((r) => r.stdout)) result
(statusStdout) to check the full porcelain line for b.txt so staged, modified,
or other states don't cause false positives.
In `@apps/server/src/open.test.ts`:
- Around line 135-152: The test "uses the containing directory when terminal
editors receive a file path" currently uses raw node:fs calls (fs.mkdtempSync,
fs.mkdirSync, fs.writeFileSync); replace these with Effect FileSystem helpers:
acquire a temp dir with makeTempDirectoryScoped(), create the nested directory
and write the file using writeFileString() (or equivalent Effect FS ops) inside
the Effect.gen flow so resource cleanup is automatic, then compute filePath and
call resolveEditorLaunch({ cwd: `${filePath}:48`, editor: "ghostty" }, "linux")
as before; ensure you import/use the same FileSystem helpers used elsewhere in
this file and keep the assertion on ghosttyFileTarget unchanged.
In `@apps/server/src/workspaceEntries.ts`:
- Around line 40-43: The declared interface RankedWorkspaceEntry is unused;
either remove it or adopt it for the ranked array and related helpers—update the
type of the ranked collection to RankedWorkspaceEntry[] and change the
signatures/usage in compareRankedEntries and insertRankedEntry to accept/return
RankedWorkspaceEntry (referencing the RankedWorkspaceEntry symbol and the
functions compareRankedEntries and insertRankedEntry and the ranked variable) so
you have a single canonical shape, or delete the interface if you prefer inline
types.
- Around line 68-75: The scoring currently re-normalizes strings per match;
update scoreEntry to accept a SearchableWorkspaceEntry (instead of ProjectEntry)
and use its precomputed normalizedPath and normalizedName fields (from
toSearchableWorkspaceEntry) rather than recomputing basenameOf/ toLowerCase
inside scoreEntry; update all call sites that pass ProjectEntry to first convert
via toSearchableWorkspaceEntry and then call scoreEntry so the per-entry string
work is done once when entries are created.
In `@apps/web/src/appSettings.ts`:
- Around line 253-336: Remove the now-orphaned localStorage sync/persistence
helpers: delete the listeners array, emitChange(), subscribe(), and
persistSettings() from appSettings.ts and update any internal references (search
for "listeners", "emitChange", "subscribe", "persistSettings") so nothing calls
them; keep and export getAppSettingsSnapshot (and
parsePersistedSettings/migratePersistedAppSettings if still used) for
synchronous reads, and ensure useAppSettings now relies solely on the
useLocalStorage hook for persistence and cross-tab sync.
In `@apps/web/src/components/GitActionsControl.tsx`:
- Around line 514-517: Duplicate commit-dialog teardown code should be extracted
into a single helper (e.g., resetCommitDialogState or closeCommitDialog) that
calls setIsCommitDialogOpen(false), setDialogCommitMessage(""),
setExcludedFiles(new Set()), and setIsEditingFiles(false); replace each
duplicated sequence (currently using those four setters in close/submit paths at
locations around the handlers referencing setIsCommitDialogOpen,
setDialogCommitMessage, setExcludedFiles, setIsEditingFiles) with a single call
to that helper so all cleanup is centralized and future changes need only be
made in one place.
In `@apps/web/src/components/Sidebar.logic.test.ts`:
- Around line 179-201: Add a fourth unit test for resolveThreadRowClassName to
cover the case where a thread is neither active nor selected (isActive: false,
isSelected: false); assert it returns the muted/inactive styles (e.g., contains
"text-muted-foreground" and "hover:bg-accent") and does not include
primary/active backgrounds (e.g., does not contain "bg-primary" or
"bg-accent/85") so the full 2x2 state matrix is tested.
In `@apps/web/src/hooks/useCopyToClipboard.ts`:
- Around line 22-26: The copyToClipboard callback in useCopyToClipboard silently
returns when the Clipboard API is unavailable and no onError handler is
registered; update copyToClipboard (and use onErrorRef) to emit a visible log
(e.g., console.warn or console.error) when navigator.clipboard?.writeText is
missing and onErrorRef.current is undefined so developers can see the failure
during debugging — include a clear message and the ctx value for context and
still call onErrorRef.current if present.
In `@apps/web/src/wsTransport.ts`:
- Around line 193-195: The WebSocket error handler logs this.url directly (in
ws.addEventListener("error", ...)), which can leak query tokens; change the
handler to log a sanitized URL instead by parsing this.url (e.g., const u = new
URL(this.url)) and logging u.origin + u.pathname (or u.host + u.pathname) rather
than the full href; ensure you guard against invalid URLs with a try/catch and
fall back to a safe placeholder when parsing fails, and update the process of
logging in the error listener to use that sanitized value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d404e46a-1e28-4aa4-a23b-481e921e2f4a
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (81)
.github/VOUCHED.td.github/workflows/ci.yml.github/workflows/pr-size.yml.github/workflows/pr-vouch.yml.github/workflows/release.ymlCONTRIBUTING.mdKEYBINDINGS.mdapps/desktop/scripts/dev-electron.mjsapps/desktop/src/main.tsapps/marketing/package.jsonapps/server/integration/fixtures/providerRuntime.tsapps/server/src/git/Layers/GitCore.test.tsapps/server/src/git/Layers/GitCore.tsapps/server/src/git/Layers/GitManager.test.tsapps/server/src/git/Layers/GitManager.tsapps/server/src/git/Services/GitCore.tsapps/server/src/kiloServerManager.tsapps/server/src/open.test.tsapps/server/src/opencodeServerManager.tsapps/server/src/orchestration/Layers/ProviderRuntimeIngestion.tsapps/server/src/provider/Layers/CodexAdapter.tsapps/server/src/provider/Layers/ProviderService.test.tsapps/server/src/workspaceEntries.test.tsapps/server/src/workspaceEntries.tsapps/web/package.jsonapps/web/src/appSettings.test.tsapps/web/src/appSettings.tsapps/web/src/components/BranchToolbar.tsxapps/web/src/components/ChatMarkdown.tsxapps/web/src/components/ChatView.browser.tsxapps/web/src/components/ChatView.logic.tsapps/web/src/components/ChatView.tsxapps/web/src/components/ComposerPromptEditor.tsxapps/web/src/components/DiffPanel.tsxapps/web/src/components/DiffPanelShell.tsxapps/web/src/components/GitActionsControl.tsxapps/web/src/components/Icons.tsxapps/web/src/components/PlanSidebar.tsxapps/web/src/components/Sidebar.logic.test.tsapps/web/src/components/Sidebar.logic.tsapps/web/src/components/Sidebar.tsxapps/web/src/components/ThreadTerminalDrawer.tsxapps/web/src/components/chat/MessageCopyButton.tsxapps/web/src/components/chat/MessagesTimeline.logic.test.tsapps/web/src/components/chat/MessagesTimeline.logic.tsapps/web/src/components/chat/MessagesTimeline.tsxapps/web/src/components/chat/OpenInPicker.tsxapps/web/src/components/chat/ProviderModelPicker.tsxapps/web/src/components/ui/select.tsxapps/web/src/components/ui/sidebar.tsxapps/web/src/components/ui/switch.tsxapps/web/src/composer-logic.test.tsapps/web/src/composer-logic.tsapps/web/src/diffRouteSearch.tsapps/web/src/editorPreferences.tsapps/web/src/hooks/useCopyToClipboard.tsapps/web/src/hooks/useHandleNewThread.tsapps/web/src/hooks/useLocalStorage.tsapps/web/src/hooks/useMediaQuery.tsapps/web/src/lib/gitReactQuery.tsapps/web/src/lib/terminalFocus.tsapps/web/src/routes/__root.tsxapps/web/src/routes/_chat.$threadId.tsxapps/web/src/routes/_chat.settings.tsxapps/web/src/routes/_chat.tsxapps/web/src/session-logic.test.tsapps/web/src/session-logic.tsapps/web/src/terminal-links.tsapps/web/src/terminalStateStore.test.tsapps/web/src/terminalStateStore.tsapps/web/src/timestampFormat.test.tsapps/web/src/timestampFormat.tsapps/web/src/types.tsapps/web/src/wsTransport.tsapps/web/vite.config.tspackage.jsonpackages/contracts/src/editor.tspackages/contracts/src/git.tspackages/contracts/src/provider.tspackages/contracts/src/providerRuntime.tsscripts/build-desktop-artifact.ts
💤 Files with no reviewable changes (1)
- apps/web/src/terminal-links.ts
Comment on lines
+3
to
+5
| on: | ||
| pull_request_target: | ||
| types: [opened, reopened, synchronize, ready_for_review, converted_to_draft] |
There was a problem hiding this comment.
Make the label-definition sync job reachable.
Line 60 can never pass because this workflow only listens to pull_request_target. That leaves the label job assuming the size:* labels already exist, so a fresh repo state or deleted label set won't recover automatically.
Suggested fix
sync-label-definitions:
name: Sync PR size label definitions
needs: prepare-config
- if: github.event_name != 'pull_request_target'
+ if: github.event_name == 'pull_request_target'
runs-on: ubuntu-24.04
permissions:
contents: read
issues: write
@@
label:
name: Label PR size
- needs: prepare-config
+ needs:
+ - prepare-config
+ - sync-label-definitions
if: github.event_name == 'pull_request_target'
runs-on: ubuntu-24.04Also applies to: 57-60, 115-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/pr-size.yml around lines 3 - 5, The workflow only listens
to pull_request_target so the label-definition sync job ('label' job) can never
create missing size:* labels on fresh repos; update the workflow triggers and
conditional checks so the job is reachable: add pull_request (and/or
workflow_dispatch) to the top-level on: map so events delivered to the label
job, and update any job if: expressions that currently gate execution (the
checks around lines referenced 57-60 and 115-130) to include github.event_name
== 'pull_request' or github.event_name == 'workflow_dispatch' (or remove the
overly strict event-name guard) so the 'label' job can run and create labels
when needed. Ensure the 'label' job id/name remains the same so downstream steps
keep references.
.github/workflows/release.yml
Outdated
Comment on lines
+256
to
+257
| - name: Publish CLI package | ||
| run: node apps/server/scripts/cli.ts publish --tag latest --app-version "${{ needs.preflight.outputs.version }}" --verbose |
There was a problem hiding this comment.
Same TypeScript execution issue and missing npm auth token.
- Line 257 uses
nodeto run a.tsfile, which will fail for the same reason as line 251. - The npm publish step requires
NODE_AUTH_TOKENenvironment variable whenregistry-urlis configured in setup-node (line 245). Without it, npm authentication will fail.
🐛 Proposed fix
- name: Publish CLI package
- run: node apps/server/scripts/cli.ts publish --tag latest --app-version "${{ needs.preflight.outputs.version }}" --verbose
+ env:
+ NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}
+ run: bun run apps/server/scripts/cli.ts publish --tag latest --app-version "${{ needs.preflight.outputs.version }}" --verboseEnsure that NPM_TOKEN secret is configured in the repository settings.
📝 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
| - name: Publish CLI package | |
| run: node apps/server/scripts/cli.ts publish --tag latest --app-version "${{ needs.preflight.outputs.version }}" --verbose | |
| - name: Publish CLI package | |
| env: | |
| NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} | |
| run: bun run apps/server/scripts/cli.ts publish --tag latest --app-version "${{ needs.preflight.outputs.version }}" --verbose |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release.yml around lines 256 - 257, The "Publish CLI
package" workflow step currently runs "node apps/server/scripts/cli.ts publish
..." which will fail because Node cannot execute a TypeScript file directly and
the npm publish lacks authentication; change the step to execute a built JS file
or run the TypeScript via ts-node (or compile first) so the command targets a
runnable JavaScript entrypoint (reference the "Publish CLI package" run command
and the script at apps/server/scripts/cli.ts), and ensure the job exposes the
authentication token by adding env: NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}
(or configure the setup-node step to use NODE_AUTH_TOKEN from the NPM_TOKEN
secret) so npm publish can authenticate when registry-url is set.
Comment on lines
+46
to
+59
| "@rolldown/plugin-babel": "^0.2.0", | ||
| "@tailwindcss/vite": "^4.0.0", | ||
| "@tanstack/router-plugin": "^1.161.0", | ||
| "@types/babel__core": "^7.20.5", | ||
| "@types/react": "^19.0.0", | ||
| "@types/react-dom": "^19.0.0", | ||
| "@vitejs/plugin-react": "^5.1.4", | ||
| "@vitejs/plugin-react": "^6.0.0", | ||
| "@vitest/browser-playwright": "^4.0.18", | ||
| "babel-plugin-react-compiler": "^19.0.0-beta-e552027-20250112", | ||
| "msw": "^2.12.10", | ||
| "playwright": "^1.58.2", | ||
| "tailwindcss": "^4.0.0", | ||
| "typescript": "catalog:", | ||
| "vite": "^8.0.0-beta.12", | ||
| "vite": "^8.0.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python <<'PY'
import json
from pathlib import Path
found = []
for path in Path(".").rglob("package.json"):
if any(part in {"node_modules", "dist", "build", ".turbo"} for part in path.parts):
continue
try:
data = json.loads(path.read_text())
except Exception:
continue
for section in ("dependencies", "devDependencies", "peerDependencies", "optionalDependencies"):
deps = data.get(section) or {}
if "@babel/core" in deps:
found.append((str(path), section, deps["@babel/core"]))
if not found:
print("MISSING: `@babel/core` is not declared in any checked package.json")
else:
for path, section, version in found:
print(f"{path}: {section} -> {version}")
PYRepository: aaditagrawal/t3code
Length of output: 127
Add @babel/core to the dependencies for the React Compiler setup.
Lines 46–59 introduce @rolldown/plugin-babel, babel-plugin-react-compiler, and related types, but @babel/core is missing from this manifest. The @vitejs/plugin-react documentation lists @babel/core as a required peer dependency alongside these packages. Without it, the compiler toolchain is incomplete and relies on an incidental workspace hoist, making builds fragile across different installation scenarios.
Add @babel/core to devDependencies in apps/web/package.json.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/package.json` around lines 46 - 59, The package manifest is missing
the required `@babel/core` peer dependency for the React/Babel toolchain; update
apps/web package.json by adding `@babel/core` to the devDependencies alongside the
existing "@rolldown/plugin-babel", "babel-plugin-react-compiler" and
"@vitejs/plugin-react" entries (pick a compatible ^7.x version matching your
babel-related packages), then reinstall to ensure builds don’t rely on workspace
hoisting.
Comment on lines
+22
to
+25
| function resolveMax(value: Breakpoint | number): string { | ||
| const px = typeof value === "number" ? value : BREAKPOINTS[value]; | ||
| return `(max-width: ${px - 1}px)`; | ||
| } |
There was a problem hiding this comment.
Only subtract 1px for named breakpoint max queries.
The public API accepts numeric max values, but resolveMax(768) currently emits (max-width: 767px). That makes { max: 768 } an unexpected off-by-one for callers using exact numbers.
Suggested fix
function resolveMax(value: Breakpoint | number): string {
- const px = typeof value === "number" ? value : BREAKPOINTS[value];
- return `(max-width: ${px - 1}px)`;
+ if (typeof value === "number") {
+ return `(max-width: ${value}px)`;
+ }
+ return `(max-width: ${BREAKPOINTS[value] - 1}px)`;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/hooks/useMediaQuery.ts` around lines 22 - 25, The resolveMax
function currently subtracts 1px for all inputs causing numeric max queries to
be off-by-one; change resolveMax so it only subtracts 1 when value is a named
Breakpoint (use BREAKPOINTS[value] - 1) and when value is a number return
`(max-width: ${value}px)` without subtracting; update the resolveMax
implementation and any callers that rely on its output (resolveMax, BREAKPOINTS)
to use the corrected behavior.
| return ( | ||
| <> | ||
| <SidebarInset className="h-dvh min-h-0 overflow-hidden overscroll-y-none bg-background text-foreground"> | ||
| <SidebarInset className="h-dvh min-h-0 overflow-hidden overscroll-y-none bg-background text-foreground"> |
There was a problem hiding this comment.
Minor: Extra space in className.
There's a double space between h-dvh and min-h-0. Harmless but inconsistent with the rest of the file.
- <SidebarInset className="h-dvh min-h-0 overflow-hidden overscroll-y-none bg-background text-foreground">
+ <SidebarInset className="h-dvh min-h-0 overflow-hidden overscroll-y-none bg-background text-foreground">📝 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
| <SidebarInset className="h-dvh min-h-0 overflow-hidden overscroll-y-none bg-background text-foreground"> | |
| <SidebarInset className="h-dvh min-h-0 overflow-hidden overscroll-y-none bg-background text-foreground"> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/routes/_chat`.$threadId.tsx at line 224, The className on the
SidebarInset component contains an extra space between "h-dvh" and "min-h-0";
edit the SidebarInset element's className (the JSX prop on SidebarInset in
_chat.$threadId.tsx) to remove the double space so classes read "h-dvh min-h-0
..." to match the rest of the file's formatting.
Comment on lines
+293
to
+299
| if ( | ||
| isNewTerminal && | ||
| !destinationGroup.terminalIds.includes(terminalId) && | ||
| destinationGroup.terminalIds.length >= MAX_TERMINALS_PER_GROUP | ||
| ) { | ||
| return normalized; | ||
| } |
There was a problem hiding this comment.
Per-group terminal cap is bypassed for existing terminals.
Line [294] gates the limit with isNewTerminal, so moving an existing terminal into a full group can exceed MAX_TERMINALS_PER_GROUP.
Suggested fix
- if (
- isNewTerminal &&
- !destinationGroup.terminalIds.includes(terminalId) &&
- destinationGroup.terminalIds.length >= MAX_TERMINALS_PER_GROUP
- ) {
+ if (
+ !destinationGroup.terminalIds.includes(terminalId) &&
+ destinationGroup.terminalIds.length >= MAX_TERMINALS_PER_GROUP
+ ) {
return normalized;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/terminalStateStore.ts` around lines 293 - 299, The current check
only enforces MAX_TERMINALS_PER_GROUP for new terminals (isNewTerminal),
allowing existing terminals to be moved into a full group; change the guard to
always block adding a terminal ID that isn't already present when the group is
full. Replace the conditional using isNewTerminal with a check that if
destinationGroup.terminalIds does not include terminalId AND
destinationGroup.terminalIds.length >= MAX_TERMINALS_PER_GROUP then return
normalized, so functions/variables to update: isNewTerminal usage,
destinationGroup.terminalIds, terminalId, MAX_TERMINALS_PER_GROUP, and the early
return of normalized.
Comment on lines
+126
to
+128
| export function isToolLifecycleItemType(value: string): value is ToolLifecycleItemType { | ||
| return TOOL_LIFECYCLE_ITEM_TYPES.includes(value as ToolLifecycleItemType); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Keep packages/contracts schema-only.
isToolLifecycleItemType() introduces runtime behavior into the contracts package. The exported literal tuple/schema is fine here, but the predicate should live in a consumer or shared utility module instead.
As per coding guidelines, "packages/contracts schema-only — no runtime logic should be added to this package."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/contracts/src/providerRuntime.ts` around lines 126 - 128, Remove the
runtime predicate isToolLifecycleItemType from packages/contracts and keep only
the exported literal tuple/schema TOOL_LIFECYCLE_ITEM_TYPES and its associated
types; move the runtime check into a consumer or shared utility module (e.g.,
create an isToolLifecycleItemType function in the consumer package or shared
utils) that imports TOOL_LIFECYCLE_ITEM_TYPES and performs the includes(...)
check, and update any imports/usages to reference the predicate in the new
location instead of packages/contracts.
- Fix CI/release workflows: replace `node` with `bun run` for .ts files - Add NODE_AUTH_TOKEN env for npm publish step in release workflow - Fix pr-size.yml: sync-label-definitions condition was inverted (never ran) - Make label job depend on sync-label-definitions - Fix useLocalStorage: Record.keys() on Map -> Array.from(store.keys()) - Remove unused effect/Record import - Add aria-label to selective staging checkboxes in GitActionsControl - Handle rejected promise in ChatMarkdown openInPreferredEditor - Fix double space in className in _chat.$threadId.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/routes/_chat.$threadId.tsx (1)
177-202:⚠️ Potential issue | 🟠 MajorScope the warm diff state to the current
threadId.Line 179 stores a route-level boolean, and Line 219 uses it to keep rendering the diff once it has ever been opened. Because this route stays mounted across param-only navigations, opening diff in one thread makes
LazyDiffPanelmount eagerly in later threads even when their diff is still closed, which weakens the deferred-start behavior this patch is trying to add.💡 One way to key the warm state per thread
- const [hasOpenedDiff, setHasOpenedDiff] = useState(diffOpen); + const [openedDiffThreadId, setOpenedDiffThreadId] = useState(() => + diffOpen ? threadId : null, + ); useEffect(() => { if (diffOpen) { - setHasOpenedDiff(true); + setOpenedDiffThreadId(threadId); } - }, [diffOpen]); + }, [diffOpen, threadId]); - const shouldRenderDiffContent = diffOpen || hasOpenedDiff; + const shouldRenderDiffContent = diffOpen || openedDiffThreadId === threadId;Also applies to: 219-243
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/_chat`.$threadId.tsx around lines 177 - 202, The warm-diff boolean is currently route-scoped (hasOpenedDiff) so opening the diff on one thread keeps LazyDiffPanel eager on other threads; change this to be keyed by threadId: replace hasOpenedDiff/setHasOpenedDiff with a per-thread indicator (e.g., openedDiffThreadId or a Map keyed by threadId) and update the useEffect that watches diffOpen to set the current thread's marker when diffOpen becomes true (use setOpenedThreadId(threadId) or map.set(threadId, true) inside the effect). Then update all checks that use hasOpenedDiff (including where LazyDiffPanel is conditionally mounted) to compare the marker to the current threadId (openedDiffThreadId === threadId or map.get(threadId) === true) so the warm state is scoped to each thread.
♻️ Duplicate comments (2)
apps/web/src/hooks/useLocalStorage.ts (2)
73-107:⚠️ Potential issue | 🟠 MajorKeep the
setStoredValuefunctional updater pure (no storage/event side effects).On Lines 76-85, the functional updater writes storage and dispatches events. React may replay functional updaters in development/strict/concurrent paths, causing duplicate writes/events.
🔧 Proposed fix
const setValue = useCallback( (value: T | ((val: T) => T)) => { - try { - setStoredValue((prev) => { - const valueToStore = typeof value === "function" ? (value as (val: T) => T)(prev) : value; - if (valueToStore === null) { - removeLocalStorageItem(key); - } else { - setLocalStorageItem(key, valueToStore, schema); - } - // Dispatch event after state update completes to avoid nested state updates - queueMicrotask(() => dispatchLocalStorageChange(key)); - return valueToStore; - }); - } catch (error) { - console.error("[LOCALSTORAGE] Error:", error); - } + setStoredValue((prev) => + typeof value === "function" ? (value as (val: T) => T)(prev) : value, + ); }, - [key, schema], + [], ); const prevKeyRef = useRef(key); + const hasMountedRef = useRef(false); + + useEffect(() => { + if (!hasMountedRef.current) { + hasMountedRef.current = true; + return; + } + + try { + if (storedValue === null) { + removeLocalStorageItem(key); + } else { + setLocalStorageItem(key, storedValue, schema); + } + dispatchLocalStorageChange(key); + } catch (error) { + console.error("[LOCALSTORAGE] Error:", error); + } + }, [key, schema, storedValue]);#!/bin/bash set -euo pipefail cat -n apps/web/src/hooks/useLocalStorage.ts | sed -n '70,110p' rg -n 'setStoredValue\(\(prev\)|setLocalStorageItem\(|removeLocalStorageItem\(|queueMicrotask\(' apps/web/src/hooks/useLocalStorage.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/useLocalStorage.ts` around lines 73 - 107, The functional updater passed to setStoredValue currently performs side effects (writing via setLocalStorageItem/removeLocalStorageItem and dispatchLocalStorageChange), which can be replayed; change setValue so the updater only computes and returns newValue (no storage or event calls), then perform the storage write/remove and queueMicrotask(dispatchLocalStorageChange(key)) outside the updater—use the returned newValue (or compute it before calling setStoredValue when value is not a function) to decide whether to call setLocalStorageItem or removeLocalStorageItem; similarly ensure the key-change effect (getLocalStorageItem + setStoredValue) stays pure and any side-effectful sync to localStorage is done after state is set.
4-39:⚠️ Potential issue | 🟠 MajorAvoid import-time
window.localStorageaccess and fully honorStorageshim semantics.On Line 6,
window.localStorageis resolved during module initialization, which can throw in restricted environments and fail import. Also, on Lines 16-17 the shim methods returnMapvalues instead of behaving likeStoragevoid mutators.🔧 Proposed fix
-const isomorphicLocalStorage: Storage = - typeof window !== "undefined" - ? window.localStorage - : (function () { - const store = new Map<string, string>(); - return { - clear: () => store.clear(), - getItem: (_) => store.get(_) ?? null, - key: (_) => Array.from(store.keys()).at(_) ?? null, - get length() { - return store.size; - }, - removeItem: (_) => store.delete(_), - setItem: (_, value) => store.set(_, value), - }; - })(); +const memoryStorage: Storage = (() => { + const store = new Map<string, string>(); + return { + clear: () => { + store.clear(); + }, + getItem: (key) => store.get(key) ?? null, + key: (index) => Array.from(store.keys()).at(index) ?? null, + get length() { + return store.size; + }, + removeItem: (key) => { + store.delete(key); + }, + setItem: (key, value) => { + store.set(key, value); + }, + }; +})(); + +const getStorage = (): Storage => { + if (typeof window === "undefined") return memoryStorage; + try { + return window.localStorage; + } catch { + return memoryStorage; + } +}; @@ - const item = isomorphicLocalStorage.getItem(key); + const item = getStorage().getItem(key); @@ - isomorphicLocalStorage.setItem(key, valueToSet); + getStorage().setItem(key, valueToSet); @@ - isomorphicLocalStorage.removeItem(key); + getStorage().removeItem(key);#!/bin/bash set -euo pipefail cat -n apps/web/src/hooks/useLocalStorage.ts | sed -n '1,45p' rg -n 'window\.localStorage|removeItem:\s*\(_\)\s*=>\s*store\.delete|setItem:\s*\(_,\s*value\)\s*=>\s*store\.set' apps/web/src/hooks/useLocalStorage.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/useLocalStorage.ts` around lines 4 - 39, The module currently reads window.localStorage at import time via the isomorphicLocalStorage constant and the shim's setItem/removeItem return Map booleans instead of void; change this by making isomorphicLocalStorage a lazily-evaluated getter function (e.g., getIsomorphicLocalStorage) that accesses window.localStorage inside a try/catch when getLocalStorageItem/setLocalStorageItem/removeLocalStorageItem call it, and update the shim implementation used when window is unavailable so setItem and removeItem are void mutators (e.g., wrap store.set and store.delete in functions that do not return values) and ensure key(index) accepts a number and returns string | null to match Storage semantics; update references in getLocalStorageItem, setLocalStorageItem and removeLocalStorageItem to call the new getter.
🧹 Nitpick comments (2)
apps/web/src/components/ChatMarkdown.tsx (1)
296-312: Consider extracting shared handler logic to reduce duplication.The
onClickandonKeyDownhandlers contain identical logic for opening the file. This could be extracted to a shared function within the component scope.♻️ Suggested refactor
+ const openFile = () => { + const api = readNativeApi(); + if (api) { + void api.shell.openInEditor(pathForOpen, "file-manager").catch((error) => { + console.warn("Unable to open file.", error); + }); + } + }; + return ( <code {...props} className="cursor-pointer underline decoration-dotted underline-offset-2 hover:text-foreground" role="button" tabIndex={0} title={`Open ${text.trim()}`} onClick={(event) => { event.preventDefault(); event.stopPropagation(); - const api = readNativeApi(); - if (api) { - void api.shell.openInEditor(pathForOpen, "file-manager"); - } + openFile(); }} onKeyDown={(event) => { if (event.key === "Enter" || event.key === " ") { event.preventDefault(); - const api = readNativeApi(); - if (api) { - void api.shell.openInEditor(pathForOpen, "file-manager"); - } + openFile(); } }} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/ChatMarkdown.tsx` around lines 296 - 312, Extract the duplicated logic in the onClick and onKeyDown handlers into a single helper function (e.g., openFileInEditor) inside the ChatMarkdown component: move the readNativeApi() call and the void api.shell.openInEditor(pathForOpen, "file-manager") invocation into that helper, and have onClick call it after event.preventDefault()/event.stopPropagation(), while onKeyDown first checks event.key === "Enter" || event.key === " " then calls the same helper after event.preventDefault(); keep the existing void call semantics and preserve any TypeScript types for readNativeApi and pathForOpen..github/workflows/release.yml (1)
226-229: Consider gating desktop release onpublish_clito avoid partial releases.
publish_cliis parallel torelease, so desktop release/finalization can complete even if npm publish fails.♻️ Suggested dependency tightening
release: name: Publish GitHub Release - needs: [preflight, build] + needs: [preflight, build, publish_cli] runs-on: ubuntu-24.04🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 226 - 229, The desktop/final release is not gated on the npm publish job, so add the publish_cli job as a dependency to the release flow: update the workflow job that currently needs: [preflight, build] (or the specific desktop release job) to include publish_cli in its needs array so the desktop/finalization job will wait for publish_cli to succeed before running; reference the publish_cli job name and the release (or the desktop release job name) when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yml:
- Around line 272-286: The release job currently calls "bun run
scripts/merge-mac-update-manifests.ts" in the "Merge macOS updater manifests"
step but never installs Bun; add a prior step in the same release job to
install/setup Bun (e.g., using an official setup action or install script) so
that "bun run" is available at runtime; update the job to include this setup
step before invoking the merge script to prevent the runtime failure.
In `@apps/web/src/components/ChatMarkdown.tsx`:
- Around line 296-311: The onClick and onKeyDown handlers call readNativeApi()
and then call api.shell.openInEditor(pathForOpen, "file-manager") using void
which swallows rejections; update both handlers to append a .catch(...) to the
returned promise (or await in a try/catch) and log errors similarly to the
existing openInPreferredEditor handling so failures aren't silent—modify the
handlers that reference readNativeApi, api.shell.openInEditor and pathForOpen to
include error handling and logging.
---
Outside diff comments:
In `@apps/web/src/routes/_chat`.$threadId.tsx:
- Around line 177-202: The warm-diff boolean is currently route-scoped
(hasOpenedDiff) so opening the diff on one thread keeps LazyDiffPanel eager on
other threads; change this to be keyed by threadId: replace
hasOpenedDiff/setHasOpenedDiff with a per-thread indicator (e.g.,
openedDiffThreadId or a Map keyed by threadId) and update the useEffect that
watches diffOpen to set the current thread's marker when diffOpen becomes true
(use setOpenedThreadId(threadId) or map.set(threadId, true) inside the effect).
Then update all checks that use hasOpenedDiff (including where LazyDiffPanel is
conditionally mounted) to compare the marker to the current threadId
(openedDiffThreadId === threadId or map.get(threadId) === true) so the warm
state is scoped to each thread.
---
Duplicate comments:
In `@apps/web/src/hooks/useLocalStorage.ts`:
- Around line 73-107: The functional updater passed to setStoredValue currently
performs side effects (writing via setLocalStorageItem/removeLocalStorageItem
and dispatchLocalStorageChange), which can be replayed; change setValue so the
updater only computes and returns newValue (no storage or event calls), then
perform the storage write/remove and
queueMicrotask(dispatchLocalStorageChange(key)) outside the updater—use the
returned newValue (or compute it before calling setStoredValue when value is not
a function) to decide whether to call setLocalStorageItem or
removeLocalStorageItem; similarly ensure the key-change effect
(getLocalStorageItem + setStoredValue) stays pure and any side-effectful sync to
localStorage is done after state is set.
- Around line 4-39: The module currently reads window.localStorage at import
time via the isomorphicLocalStorage constant and the shim's setItem/removeItem
return Map booleans instead of void; change this by making
isomorphicLocalStorage a lazily-evaluated getter function (e.g.,
getIsomorphicLocalStorage) that accesses window.localStorage inside a try/catch
when getLocalStorageItem/setLocalStorageItem/removeLocalStorageItem call it, and
update the shim implementation used when window is unavailable so setItem and
removeItem are void mutators (e.g., wrap store.set and store.delete in functions
that do not return values) and ensure key(index) accepts a number and returns
string | null to match Storage semantics; update references in
getLocalStorageItem, setLocalStorageItem and removeLocalStorageItem to call the
new getter.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 226-229: The desktop/final release is not gated on the npm publish
job, so add the publish_cli job as a dependency to the release flow: update the
workflow job that currently needs: [preflight, build] (or the specific desktop
release job) to include publish_cli in its needs array so the
desktop/finalization job will wait for publish_cli to succeed before running;
reference the publish_cli job name and the release (or the desktop release job
name) when making this change.
In `@apps/web/src/components/ChatMarkdown.tsx`:
- Around line 296-312: Extract the duplicated logic in the onClick and onKeyDown
handlers into a single helper function (e.g., openFileInEditor) inside the
ChatMarkdown component: move the readNativeApi() call and the void
api.shell.openInEditor(pathForOpen, "file-manager") invocation into that helper,
and have onClick call it after event.preventDefault()/event.stopPropagation(),
while onKeyDown first checks event.key === "Enter" || event.key === " " then
calls the same helper after event.preventDefault(); keep the existing void call
semantics and preserve any TypeScript types for readNativeApi and pathForOpen.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59e37866-7b68-4612-b715-899bad5f02d0
📒 Files selected for processing (7)
.github/workflows/ci.yml.github/workflows/pr-size.yml.github/workflows/release.ymlapps/web/src/components/ChatMarkdown.tsxapps/web/src/components/GitActionsControl.tsxapps/web/src/hooks/useLocalStorage.tsapps/web/src/routes/_chat.$threadId.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci.yml
- Make localStorage resolution lazy to avoid import-time throws in sandboxed environments - Move persistence side-effects out of setState updater to avoid replays in React StrictMode - Use isInitialMount ref to skip persistence on first render
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/hooks/useLocalStorage.ts`:
- Around line 96-110: The hook currently dispatches same-tab localStorage events
(dispatchLocalStorageChange) that its own listener consumes and, because decode
returns new object references, can re-set state and loop; fix by attaching a
per-hook instance id (useRef instanceId) to the dispatched event and ignore
events whose event.detail.source matches this instanceId, and additionally in
the event handler (the code that calls decode and setStoredValue) perform a
deep-equality check between the decoded value and current storedValue before
calling setStoredValue so you never set state to an
equivalent-but-different-reference; update dispatchLocalStorageChange to include
the source id and update the listener to skip same-source events and only call
setStoredValue when values differ.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: baef4b3d-d133-4061-9b70-61efed59e886
📒 Files selected for processing (1)
apps/web/src/hooks/useLocalStorage.ts
- Add setup-bun step to release job (was using bun run without installing bun) - Prevent useLocalStorage self-triggered event feedback loop using isSelfDispatch ref guard - Handle rejected promises in ChatMarkdown openInEditor calls
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/web/src/hooks/useLocalStorage.ts (1)
100-113:⚠️ Potential issue | 🔴 Critical
isSelfDispatchstill allows peer echo loops.
syncFromStorage()always feeds the decoded value back into state, and for object/array schemas that produces a fresh reference. Another hook instance in the same tab—or another tab via the nativestorageevent—will then re-render and persist/dispatch the same payload again, so the update can bounce indefinitely. Track storage-originated updates separately and skip the next persist/dispatch, or compare the serialized payload before updating state. A regression test with two hook instances on the same key would reproduce this quickly.Also applies to: 135-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/useLocalStorage.ts` around lines 100 - 113, syncFromStorage() causes peer echo loops because decoded objects/arrays get new references and isSelfDispatch alone can't tell storage-originated updates apart; change the hook to track storage-origin updates separately (e.g., add a isStorageOriginRef or lastSerializedRef) and when syncFromStorage updates state set that flag or store the serialized payload, then on the subsequent effect that would persist/dispatch (the block using isSelfDispatch, setLocalStorageItem/removeLocalStorageItem and dispatchLocalStorageChange) skip persisting/dispatching if the update originated from storage or if the serialized payload matches the last persisted value; apply the same change to the other locations mentioned (the block at 135-156) so two instances in the same tab or across tabs won’t echo each other.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yml:
- Around line 226-229: The release/finalize jobs are not waiting for the
publish_cli job, so add publish_cli to their needs arrays to gate finalization
on successful npm publish; locate the job definitions named publish_cli and the
downstream jobs (e.g., release and finalize) and include "publish_cli" in each
downstream job's needs list (or replace parallel entries with an array that
includes publish_cli) so those jobs will only run after publish_cli completes
successfully.
- Around line 256-257: The workflow step "Publish CLI package" currently
hardcodes "--tag latest" which will promote prereleases; update the "Publish CLI
package" step so the npm tag is computed from the version
(needs.preflight.outputs.version) instead of always "latest" — detect prerelease
versions (e.g., version string contains a hyphen or semver prerelease
identifiers) and set the tag to a non-default tag like "next" or "prerelease",
then pass that variable into the publish command (replace the hardcoded "--tag
latest" in the bun run ... cli.ts publish invocation with the computed tag
variable).
In `@apps/web/src/hooks/useLocalStorage.ts`:
- Around line 100-116: The persistence effect in useLocalStorage (the useEffect
that watches storedValue, key, schema and uses isInitialMount,
removeLocalStorageItem, setLocalStorageItem, isSelfDispatch,
dispatchLocalStorageChange) must be guarded against key transitions: add a check
at the start of the effect to return early when prevKeyRef.current !== key so
the hook skips writing stale storedValue to a newly selected key and lets the
key-sync effect establish the correct state first; keep the existing
isInitialMount handling and error catch as-is.
---
Duplicate comments:
In `@apps/web/src/hooks/useLocalStorage.ts`:
- Around line 100-113: syncFromStorage() causes peer echo loops because decoded
objects/arrays get new references and isSelfDispatch alone can't tell
storage-originated updates apart; change the hook to track storage-origin
updates separately (e.g., add a isStorageOriginRef or lastSerializedRef) and
when syncFromStorage updates state set that flag or store the serialized
payload, then on the subsequent effect that would persist/dispatch (the block
using isSelfDispatch, setLocalStorageItem/removeLocalStorageItem and
dispatchLocalStorageChange) skip persisting/dispatching if the update originated
from storage or if the serialized payload matches the last persisted value;
apply the same change to the other locations mentioned (the block at 135-156) so
two instances in the same tab or across tabs won’t echo each other.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fcd8829e-8f72-4a65-bb18-0d54aa691a51
📒 Files selected for processing (3)
.github/workflows/release.ymlapps/web/src/components/ChatMarkdown.tsxapps/web/src/hooks/useLocalStorage.ts
- Guard useLocalStorage persistence effect against key transitions to prevent writing stale values under a new key - Use dynamic npm tag (next for prereleases, latest for stable) in release workflow - Gate release and finalize jobs on publish_cli success
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/web/src/hooks/useLocalStorage.ts (2)
16-24:⚠️ Potential issue | 🟡 MinorAlign the fallback shim with
StorageAPI semantics.Line 16 uses
.at(index), which allows negative indices (e.g.,-1returns the last key), unlikeStorage.key(index). Line 23 also returns theMapinstance fromsetItem, whileStorage.setItemshould returnvoid.Proposed fix
- key: (_) => Array.from(store.keys()).at(_) ?? null, + key: (index) => + Number.isInteger(index) && index >= 0 + ? (Array.from(store.keys())[index] ?? null) + : null, @@ - setItem: (_, value) => store.set(_, value), + setItem: (key, value) => { + store.set(key, value); + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/useLocalStorage.ts` around lines 16 - 24, The fallback Storage shim currently allows negative indices and returns a Map from setItem; update the key, length and setItem behaviors to match the Storage API: in the exported object (functions key, length getter, removeItem, setItem) change key to use a plain index lookup over Array.from(store.keys())[index] (or equivalent) and return null for out-of-range or negative indices so negative values do not access from the end; keep length as store.size and removeItem as store.delete; and make setItem (_, value) return void (i.e., do not return the Map or any value) so it matches Storage.setItem semantics. Ensure you reference the existing symbols: key, length (getter), removeItem, setItem, and the store Map when making these changes.
117-119:⚠️ Potential issue | 🔴 CriticalPrevent cross-instance same-tab rebroadcast loops.
This still loops when two
useLocalStorageinstances share the samekey: one instance syncs from the event, persists again, then dispatches again, causing ping-pong updates (especially for decoded object values with new references).Proposed fix
const isInitialMount = useRef(true); const isSelfDispatch = useRef(false); + const isSyncFromEvent = useRef(false); @@ useEffect(() => { if (isInitialMount.current) { isInitialMount.current = false; return; } + if (isSyncFromEvent.current) { + isSyncFromEvent.current = false; + return; + } @@ const syncFromStorage = () => { try { const newValue = getLocalStorageItem(key, schema); + isSyncFromEvent.current = true; setStoredValue(newValue ?? initialValue); } catch (error) { console.error("[LOCALSTORAGE] Error:", error); } };Also applies to: 154-161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/useLocalStorage.ts` around lines 117 - 119, The handler is causing same-tab ping-pong between two useLocalStorage instances because writing back triggers dispatchLocalStorageChange and the other instance re-persisting causes a loop; fix by (1) surrounding the write/persist logic in the setter (function that writes to localStorage) with setting isSelfDispatch.current = true before writing and resetting it to false in a finally block so event handlers can ignore self-originated writes, and (2) avoid rebroadcasting when the stored value did not actually change by comparing the previous serialized value (JSON.stringify or stable serialize) to the new serialized value and only calling dispatchLocalStorageChange(key) when they differ. Apply the same guard and comparison in both locations referenced (the block around isSelfDispatch.current = true / dispatchLocalStorageChange at the shown try/catch and the analogous block at lines 154-161).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/web/src/hooks/useLocalStorage.ts`:
- Around line 16-24: The fallback Storage shim currently allows negative indices
and returns a Map from setItem; update the key, length and setItem behaviors to
match the Storage API: in the exported object (functions key, length getter,
removeItem, setItem) change key to use a plain index lookup over
Array.from(store.keys())[index] (or equivalent) and return null for out-of-range
or negative indices so negative values do not access from the end; keep length
as store.size and removeItem as store.delete; and make setItem (_, value) return
void (i.e., do not return the Map or any value) so it matches Storage.setItem
semantics. Ensure you reference the existing symbols: key, length (getter),
removeItem, setItem, and the store Map when making these changes.
- Around line 117-119: The handler is causing same-tab ping-pong between two
useLocalStorage instances because writing back triggers
dispatchLocalStorageChange and the other instance re-persisting causes a loop;
fix by (1) surrounding the write/persist logic in the setter (function that
writes to localStorage) with setting isSelfDispatch.current = true before
writing and resetting it to false in a finally block so event handlers can
ignore self-originated writes, and (2) avoid rebroadcasting when the stored
value did not actually change by comparing the previous serialized value
(JSON.stringify or stable serialize) to the new serialized value and only
calling dispatchLocalStorageChange(key) when they differ. Apply the same guard
and comparison in both locations referenced (the block around
isSelfDispatch.current = true / dispatchLocalStorageChange at the shown
try/catch and the analogous block at lines 154-161).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: da55513a-208a-44cb-9571-2e257f6f359d
📒 Files selected for processing (2)
.github/workflows/release.ymlapps/web/src/hooks/useLocalStorage.ts
- Clamp negative indices in fallback Storage.key() to match spec - Make fallback setItem return void instead of Map instance - Compare encoded values before writing to storage to prevent cross-instance dispatch loops when multiple hooks share the same key
This was referenced Mar 16, 2026
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
18 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
Sync with upstream pingdotgg/t3code, bringing in 35 commits since our last merge round (PR #9).
New features from upstream
useLocalStoragehelper (fix(web): resolve preferred editor from available editors & introduceuseLocalStoragehelper pingdotgg/t3code#662)Bug fixes from upstream
Tooling updates
Conflict resolutions
workspaceEntries.ts: Kept our zortos-ported fuzzy search (ranked insertion with path tiebreaker) over upstream's feat: add fuzzy workspace entry search pingdotgg/t3code#256appSettings.ts/_chat.settings.tsx: Merged our display settings (showCommandOutput, showFileChangeDiffs) with upstream's timestamp format and env mode settingsMessagesTimeline.tsx: Merged our showCommandOutput gating with upstream's compact tool-call iconsDiffPanel.tsx: Deduplicated useAppSettings importopen.test.ts: Added missing node:fs/os/path imports for upstream's new testTest plan
cmpin @-mention picker)Summary by CodeRabbit
New Features
Improvements
Documentation