Merge upstream/main into fork (batch 3)#6
Merged
aaditagrawal merged 12 commits intomainfrom Mar 11, 2026
Merged
Conversation
…der (pingdotgg#649) Co-authored-by: Julius Marminge <julius0216@outlook.com>
* fix(web): avoid push and PR defaults without origin * fmt * object here too --------- Co-authored-by: Julius Marminge <julius0216@outlook.com>
Integrates 8 upstream commits: downloads page, app version display, terminal restart type fix, custom provider auth skip, default git action fix, desktop theme sync, error handling cleanup, and Codex overrides overflow fix. Resolves 5 merge conflicts preserving fork's multi-provider support.
📝 WalkthroughWalkthroughAdds desktop–web theme sync via a new Electron IPC channel; exposes APP_VERSION to the web app; surfaces git origin presence across contracts, server, and UI; switches terminal restart input and related contracts; enhances Codex provider-health detection and tests; standardizes error formatting; adds marketing release fetching and download pages. Changes
Sequence DiagramsequenceDiagram
participant Web as Web App
participant Preload as Preload Bridge
participant IPC as Electron IPC
participant Main as Main Process
participant Native as nativeTheme
Web->>Preload: desktopBridge.setTheme(theme)
Preload->>IPC: ipcRenderer.invoke("desktop:set-theme", theme)
IPC->>Main: handle "desktop:set-theme"
Main->>Main: getSafeTheme(rawTheme)
alt theme valid
Main->>Native: nativeTheme.themeSource = theme
Native-->>Main: updated
Main-->>IPC: success
else invalid
Main-->>IPC: no-op
end
IPC-->>Preload: response
Preload-->>Web: result
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly Related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
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/server/src/orchestration/Layers/ProviderCommandReactor.ts (1)
77-90:⚠️ Potential issue | 🟠 MajorRestore the missing
unknown pending approval requestbranch.Line 574 is currently a no-op: the failure activity has already been appended, and the generator ends immediately afterward. As written,
unknown pending approval requesterrors now fall through the generic failure path, so this merge dropped the special-case handling entirely. Move the check before the failure append or reintroduce the intended follow-up action.Also applies to: 559-575
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/orchestration/Layers/ProviderCommandReactor.ts` around lines 77 - 90, The special-case handling for "unknown pending approval request" was lost because the check runs after the generic failure activity is appended and the generator returns; find the logic that appends the failure and returns (the code path around the generator in ProviderCommandReactor where failures are handled) and move the isUnknownPendingApprovalRequestError(cause) check (or reintroduce its branch) to before the failure append so the specific follow-up action for unknown pending approval/permission requests is executed instead of falling through to the generic failure path; reference the isUnknownPendingApprovalRequestError function and the failure-append code in ProviderCommandReactor to locate and restore the intended branch.
🧹 Nitpick comments (2)
apps/server/src/provider/Layers/ProviderHealth.ts (1)
20-20: Remove unusedArrayimport to avoid shadowing the global.The
Arrayimport fromeffectshadows the globalArrayproperty but is not used in the new code. This was flagged by static analysis. While effect'sArray.isArrayon line 77 would still function correctly, removing the unused import eliminates the shadowing concern.♻️ Proposed fix
-import { Array, Effect, Fiber, FileSystem, Layer, Option, Path, Result, Stream } from "effect"; +import { Effect, Fiber, FileSystem, Layer, Option, Path, Result, Stream } from "effect";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/ProviderHealth.ts` at line 20, Importing Array from "effect" shadows the global Array and is unused; remove Array from the named import list in ProviderHealth.ts (the import that currently reads import { Array, Effect, Fiber, FileSystem, Layer, Option, Path, Result, Stream } ...) so the global Array (and Array.isArray usage in this file) is no longer shadowed and static analysis warnings disappear.apps/web/public/mockServiceWorker.js (1)
1-336: Regenerate this file using MSW CLI instead of manual formatting changes.This file explicitly states "Please do NOT modify this file" (line 7) as it's auto-generated by Mock Service Worker. Manual formatting changes, while harmless functionally, can cause merge conflicts during MSW version updates and create drift from the canonical distribution.
🔄 Recommended approach
Regenerate the file using the MSW CLI:
npx msw init apps/web/public --saveThis ensures you have the exact canonical version for MSW 2.12.10 and prevents future merge conflicts.
Verify whether MSW version 2.12.10 is current and check for any security advisories:
#!/bin/bash # Check latest MSW version and security status # Check npm for latest version npm view msw version # Check for known vulnerabilities npm audit --package-lock-only --prefix apps/web 2>/dev/null | grep -A5 "msw" || echo "No MSW vulnerabilities in quick scan"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/public/mockServiceWorker.js` around lines 1 - 336, This file (mockServiceWorker.js, identified by the PACKAGE_VERSION constant and the header comment "Please do NOT modify this file") was manually reformatted but must be regenerated from the official MSW distribution; replace the file by running the MSW CLI (npx msw init apps/web/public --save) to restore the canonical worker that matches PACKAGE_VERSION (2.12.10), ensuring you do not manually edit functions like handleRequest, getResponse, respondWithMock, or serializeRequest; verify the regenerated file's PACKAGE_VERSION/integrity and commit that generated file instead of hand edits to avoid future merge drift.
🤖 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/marketing/src/lib/releases.ts`:
- Around line 19-29: The fetchLatestRelease function currently parses the fetch
response without checking HTTP status, so non-2xx GitHub responses get treated
as valid Release objects; update fetchLatestRelease to first await
fetch(API_URL), check response.ok, and if not ok throw or reject with a
descriptive error (include response.status and statusText or the error body)
before calling response.json(); only parse and cache the JSON when response.ok
and then keep the existing assets check and sessionStorage.setItem logic.
In `@apps/web/src/components/GitActionsControl.tsx`:
- Around line 173-177: The code defaults hasOriginRemote to false while
branchList is loading, causing a transient UI state; change the fallback so
hasOriginRemote uses branchList?.hasOriginRemote ?? true (i.e., default to true
while loading) in the destructuring that reads from
useQuery(gitBranchesQueryOptions(gitCwd)), and apply the same fix to the other
identical destructuring/usage of branchList later in the file so both places
default to true until branchList resolves.
---
Outside diff comments:
In `@apps/server/src/orchestration/Layers/ProviderCommandReactor.ts`:
- Around line 77-90: The special-case handling for "unknown pending approval
request" was lost because the check runs after the generic failure activity is
appended and the generator returns; find the logic that appends the failure and
returns (the code path around the generator in ProviderCommandReactor where
failures are handled) and move the isUnknownPendingApprovalRequestError(cause)
check (or reintroduce its branch) to before the failure append so the specific
follow-up action for unknown pending approval/permission requests is executed
instead of falling through to the generic failure path; reference the
isUnknownPendingApprovalRequestError function and the failure-append code in
ProviderCommandReactor to locate and restore the intended branch.
---
Nitpick comments:
In `@apps/server/src/provider/Layers/ProviderHealth.ts`:
- Line 20: Importing Array from "effect" shadows the global Array and is unused;
remove Array from the named import list in ProviderHealth.ts (the import that
currently reads import { Array, Effect, Fiber, FileSystem, Layer, Option, Path,
Result, Stream } ...) so the global Array (and Array.isArray usage in this file)
is no longer shadowed and static analysis warnings disappear.
In `@apps/web/public/mockServiceWorker.js`:
- Around line 1-336: This file (mockServiceWorker.js, identified by the
PACKAGE_VERSION constant and the header comment "Please do NOT modify this
file") was manually reformatted but must be regenerated from the official MSW
distribution; replace the file by running the MSW CLI (npx msw init
apps/web/public --save) to restore the canonical worker that matches
PACKAGE_VERSION (2.12.10), ensuring you do not manually edit functions like
handleRequest, getResponse, respondWithMock, or serializeRequest; verify the
regenerated file's PACKAGE_VERSION/integrity and commit that generated file
instead of hand edits to avoid future merge drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be2d620e-1048-4276-92f8-7faf7ef2d35f
📒 Files selected for processing (33)
CLAUDE.mdapps/desktop/src/main.tsapps/desktop/src/preload.tsapps/marketing/src/layouts/Layout.astroapps/marketing/src/lib/releases.tsapps/marketing/src/pages/download.astroapps/marketing/src/pages/index.astroapps/server/src/git/Layers/GitCore.test.tsapps/server/src/git/Layers/GitCore.tsapps/server/src/orchestration/Layers/OrchestrationEngine.tsapps/server/src/orchestration/Layers/ProviderCommandReactor.tsapps/server/src/provider/Layers/CodexAdapter.tsapps/server/src/provider/Layers/ProviderHealth.test.tsapps/server/src/provider/Layers/ProviderHealth.tsapps/server/src/terminal/Layers/Manager.test.tsapps/server/src/terminal/Layers/Manager.tsapps/server/src/terminal/Services/Manager.tsapps/server/src/wsServer.test.tsapps/server/src/wsServer.tsapps/web/public/mockServiceWorker.jsapps/web/src/branding.tsapps/web/src/components/ChatView.browser.tsxapps/web/src/components/GitActionsControl.logic.test.tsapps/web/src/components/GitActionsControl.logic.tsapps/web/src/components/GitActionsControl.tsxapps/web/src/components/Sidebar.tsxapps/web/src/hooks/useTheme.tsapps/web/src/routes/_chat.settings.tsxapps/web/src/vite-env.d.tsapps/web/vite.config.tspackages/contracts/src/git.tspackages/contracts/src/ipc.tspackages/contracts/src/terminal.ts
Comment on lines
173
to
177
| const { data: branchList = null } = useQuery(gitBranchesQueryOptions(gitCwd)); | ||
| // Default to true while loading so we don't flash init controls. | ||
| const isRepo = branchList?.isRepo ?? true; | ||
| const hasOriginRemote = branchList?.hasOriginRemote ?? false; | ||
| const currentBranch = branchList?.branches.find((branch) => branch.current)?.name ?? null; |
There was a problem hiding this comment.
Avoid defaulting hasOriginRemote to false while the branches query is still loading.
The logic helpers default this flag to true, but Line 176 forces false until branchList resolves. If gitStatus wins that race, the toolbar briefly downgrades push/PR actions into the “add origin remote” path even for repos that already have origin.
Also applies to: 206-213
🤖 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 173 - 177, The
code defaults hasOriginRemote to false while branchList is loading, causing a
transient UI state; change the fallback so hasOriginRemote uses
branchList?.hasOriginRemote ?? true (i.e., default to true while loading) in the
destructuring that reads from useQuery(gitBranchesQueryOptions(gitCwd)), and
apply the same fix to the other identical destructuring/usage of branchList
later in the file so both places default to true until branchList resolves.
- Add response.ok check in fetchLatestRelease to handle API errors - Move isUnknownPendingApprovalRequestError check before failure append - Remove unused Array import from ProviderHealth.ts - Default hasOriginRemote to true while loading in GitActionsControl
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/server/src/provider/Layers/ProviderHealth.ts (1)
220-225: Minor optimization: break early after leaving top level.Once
inTopLevelbecomes false (line 222), the loop continues iterating through remaining lines but will skip all of them. For large config files, breaking early would be more efficient.♻️ Suggested optimization
// Detect section headers — once we leave the top level, stop. if (trimmed.startsWith("[")) { - inTopLevel = false; - continue; + break; } - if (!inTopLevel) continue;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/ProviderHealth.ts` around lines 220 - 225, The loop that scans config lines sets inTopLevel to false when a section header is found (trimmed.startsWith("[")) but then uses continue on subsequent iterations, wasting work; update the loop in ProviderHealth (the block using the inTopLevel flag and trimmed.startsWith("[")) to break out immediately once inTopLevel becomes false so remaining lines are not processed, i.e., replace the continue-after-setting-inTopLevel logic with a break (or add a conditional break when !inTopLevel) to stop the loop early.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/server/src/provider/Layers/ProviderHealth.ts`:
- Around line 220-225: The loop that scans config lines sets inTopLevel to false
when a section header is found (trimmed.startsWith("[")) but then uses continue
on subsequent iterations, wasting work; update the loop in ProviderHealth (the
block using the inTopLevel flag and trimmed.startsWith("[")) to break out
immediately once inTopLevel becomes false so remaining lines are not processed,
i.e., replace the continue-after-setting-inTopLevel logic with a break (or add a
conditional break when !inTopLevel) to stop the loop early.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6f62de09-14d4-4af0-9916-11c8325c7104
📒 Files selected for processing (4)
apps/marketing/src/lib/releases.tsapps/server/src/orchestration/Layers/ProviderCommandReactor.tsapps/server/src/provider/Layers/ProviderHealth.tsapps/web/src/components/GitActionsControl.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/marketing/src/lib/releases.ts
- apps/web/src/components/GitActionsControl.tsx
Replaces inTopLevel flag with direct break when a section header is found, avoiding unnecessary iteration through remaining config lines.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/provider/Layers/ProviderHealth.ts`:
- Around line 205-207: The current read of configPath uses
fileSystem.readFileString(...).pipe(Effect.orElseSucceed(() => undefined)) which
swallows all errors; change it so only a "file not found" / missing-file error
is mapped to undefined while any other I/O error is surfaced (logged or
returned) — detect the error from the Effect failure (e.g., check errno/Code
like ENOENT or the file-not-found error class) and in the failure handler return
undefined only for that case, otherwise rethrow or call processLogger.warn/error
with the error; update the handling around fileSystem.readFileString and the
variable content to reflect this selective error mapping instead of using
Effect.orElseSucceed for all failures.
- Around line 342-350: The branch that handles hasCustomModelProvider
incorrectly returns status: "ready" for CODEX_PROVIDER without verifying
external credentials; update the return in ProviderHealth (the block referencing
hasCustomModelProvider and CODEX_PROVIDER) to use status: "warning" (and set
available accordingly, e.g., available: false) and adjust the message to
indicate external auth wasn’t verified, or alternatively perform
provider-specific env var checks before returning "ready" so only validated
providers report ready. Ensure checkedAt and authStatus remain present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 798e394b-15bd-45b3-aef5-532ead48f894
📒 Files selected for processing (1)
apps/server/src/provider/Layers/ProviderHealth.ts
Comment on lines
+205
to
+207
| const content = yield* fileSystem | ||
| .readFileString(configPath) | ||
| .pipe(Effect.orElseSucceed(() => undefined)); |
There was a problem hiding this comment.
Only suppress missing-config reads here.
Lines 205-207 currently turn every readFileString failure into undefined. That hides unreadable or broken Codex configs and can fall through to the OpenAI auth probe, which reintroduces the false negative this change is trying to avoid. Please only treat “file not found” as absent config and surface other I/O errors as a warning/error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/provider/Layers/ProviderHealth.ts` around lines 205 - 207,
The current read of configPath uses
fileSystem.readFileString(...).pipe(Effect.orElseSucceed(() => undefined)) which
swallows all errors; change it so only a "file not found" / missing-file error
is mapped to undefined while any other I/O error is surfaced (logged or
returned) — detect the error from the Effect failure (e.g., check errno/Code
like ENOENT or the file-not-found error class) and in the failure handler return
undefined only for that case, otherwise rethrow or call processLogger.warn/error
with the error; update the handling around fileSystem.readFileString and the
variable content to reflect this selective error mapping instead of using
Effect.orElseSucceed for all failures.
- Only suppress file-not-found errors when reading Codex config, log warnings for other I/O errors instead of silently swallowing - Return status "warning" for custom model providers since external credentials are not verified
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/server/src/provider/Layers/ProviderHealth.ts (1)
231-232: Consider matching quote pairs for robustness.The regex allows mismatched quotes (
"value'). While unlikely in practice, capturing the opening quote and backreferencing ensures consistency.♻️ Optional fix
- const match = trimmed.match(/^model_provider\s*=\s*["']([^"']+)["']/); + const match = trimmed.match(/^model_provider\s*=\s*(["'])([^"']+)\1/); + if (match) return match[2]; - if (match) return match[1];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/ProviderHealth.ts` around lines 231 - 232, The current regex in ProviderHealth.ts that extracts model_provider from trimmed (const match = trimmed.match(/^model_provider\s*=\s*["']([^"']+)["']/)) allows mismatched quotes; update the pattern to capture the opening quote and use a backreference so quotes must match (e.g. capture group for quote and use \1), then adjust which capture index you read (use the group that holds the value). Ensure the variable name trimmed and the const match are used as before and return the proper capture.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/server/src/provider/Layers/ProviderHealth.ts`:
- Around line 231-232: The current regex in ProviderHealth.ts that extracts
model_provider from trimmed (const match =
trimmed.match(/^model_provider\s*=\s*["']([^"']+)["']/)) allows mismatched
quotes; update the pattern to capture the opening quote and use a backreference
so quotes must match (e.g. capture group for quote and use \1), then adjust
which capture index you read (use the group that holds the value). Ensure the
variable name trimmed and the const match are used as before and return the
proper capture.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 162c4e48-6044-4f27-a346-a84e9e4ba9fb
📒 Files selected for processing (1)
apps/server/src/provider/Layers/ProviderHealth.ts
This was referenced Mar 13, 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
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pingdotgg/t3codemain branchConflict Resolutions
index.astro— adopted upstream's Layout component refactorProviderCommandReactor.ts— kept fork's interrupt guards, adopted upstream'sCause.pretty()error formatProviderHealth.ts— merged both sides' imports (fork's Copilot + upstream's FileSystem/Path)Sidebar.tsx— kept fork'sresolveThreadProvider+ upstream's version tooltip_chat.settings.tsx— kept fork's provider Select imports + upstream's APP_VERSIONVerification
bun typecheckpasses (7/7 packages)bun lintpasses (0 errors)Summary by CodeRabbit