sync: port upstream web files, desktop bounds, defaults (#3996–#4244)#182
Conversation
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR updates release automation, persists and restores desktop window state, adds bounded orchestration shell streaming, refines provider model defaults and Claude handling, improves web workspace interactions, and prioritizes remote default Git refs. ChangesRelease automation
Desktop window state
Orchestration and providers
Web workspace interactions
Git refs
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
vouch:trusted
1ea182a to
71eedba
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ea182a18f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| with: | ||
| app-id: ${{ secrets.RELEASE_APP_ID }} | ||
| private-key: ${{ secrets.RELEASE_APP_PRIVATE_KEY }} | ||
| owner: ${{ github.repository_owner }} |
There was a problem hiding this comment.
When this workflow is run from any repository context other than the fork—for example after an accidental upstream sync—using github.repository_owner here, implicit repository targeting in action-gh-release, and the later git push origin directs privileged writes to that repository. Restore the GITHUB_REPOSITORY == aaditagrawal/t3code preflight guard and explicitly target the fork before minting or using the release token.
AGENTS.md reference: AGENTS.md:L5-L10
Useful? React with 👍 / 👎.
| # Node ABI, not libc ABI. Use a standard GitHub-hosted runner + Node bullseye | ||
| # container instead of Blacksmith Ubuntu 2004 labels, which can queue forever. | ||
| runs-on: ubuntu-24.04 | ||
| runs-on: blacksmith-8vcpu-ubuntu-2404 |
There was a problem hiding this comment.
Windows releases now compile the bundled WSL pty.node directly on Ubuntu 24.04, so the native addon may require newer glibc/libstdc++ symbols and fail to load on common Ubuntu 20.04 or 22.04 WSL distributions. N-API only stabilizes the Node ABI, not these system-library ABIs; retain the previous Debian Bullseye container (or another oldest-supported sysroot) for this build.
Useful? React with 👍 / 👎.
| resolveSelectableModel(entry.driverKind, selectedModel, options) ?? | ||
| resolveSelectableModel(entry.driverKind, defaultModel, options) ?? | ||
| options.find((option) => option.isDefault)?.slug ?? | ||
| options[0]?.slug ?? |
There was a problem hiding this comment.
For providers that do not emit model-level isDefault metadata, an absent, hidden, or unavailable selection now falls straight to catalog order. Claude's catalog has no model-level default and starts with claude-fable-5, so this bypasses the declared claude-sonnet-5 chat default; the same helper also makes text generation choose Fable instead of the declared Haiku default. Keep the DEFAULT_MODEL_BY_PROVIDER and DEFAULT_GIT_TEXT_GENERATION_MODEL_BY_PROVIDER fallbacks when no live default is present.
AGENTS.md reference: AGENTS.md:L22-L24
Useful? React with 👍 / 👎.
| needs: [preflight, relay_public_config, build_wsl_node_pty] | ||
| if: ${{ !cancelled() && needs.preflight.result == 'success' && needs.relay_public_config.result == 'success' }} |
There was a problem hiding this comment.
The release graph now proceeds directly from preflight to the desktop matrix after deleting build_android_apk; a repo-wide search finds no remaining assembleRelease, android-apk, or APK artifact producer. Consequently every release silently stops publishing the signed Android APK, preventing Android users from installing or upgrading from these releases.
Useful? React with 👍 / 👎.
| - name: Publish CLI package | ||
| run: node apps/server/scripts/cli.ts publish --tag "${{ needs.preflight.outputs.cli_dist_tag }}" --app-version "${{ needs.preflight.outputs.version }}" --verbose |
There was a problem hiding this comment.
Every fork release now publishes the globally named t3 package before creating the GitHub release, and the latter explicitly depends on this job. In aaditagrawal/t3code, the OIDC identity will not match the official package's upstream trusted-publisher configuration, so publishing fails and blocks all desktop releases; if credentials do authorize it, this instead publishes the fork build as the official upstream CLI. Disable this job for the fork or publish to a fork-owned package without gating desktop releases on it.
Useful? React with 👍 / 👎.
| }); | ||
| context.currentApiModelId = apiModelId; |
There was a problem hiding this comment.
When sendTurn changes the effective Claude model or its 200k/1M option, this path now updates only currentApiModelId and leaves lastKnownContextWindow from the previous selection. Subsequent stream and task-progress usage events therefore report the previous model's maximum—or omit maxTokens when the session started without a model—until a later result or context-usage query arrives, making the context banner incorrect throughout the active turn. Set the resolved context window after setModel succeeds.
AGENTS.md reference: AGENTS.md:L22-L24
Useful? React with 👍 / 👎.
| for x64_manifest in release-assets/*-mac-x64.yml; do | ||
| arm64_manifest="${x64_manifest/-x64.yml/.yml}" | ||
| if [[ ! -f "$arm64_manifest" ]]; then | ||
| echo "Missing matching arm64 macOS manifest for $x64_manifest" >&2 | ||
| exit 1 | ||
| arm64_manifest="${x64_manifest%-x64.yml}.yml" | ||
| if [[ -f "$arm64_manifest" ]]; then | ||
| node scripts/merge-update-manifests.ts --platform mac "$arm64_manifest" "$x64_manifest" | ||
| rm -f "$x64_manifest" | ||
| fi |
There was a problem hiding this comment.
If either macOS architecture stops producing its updater manifest while still producing a DMG, this loop now silently skips the merge and the release continues. A missing arm64 manifest leaves only the suffixed x64 file, while a missing x64 manifest leaves an unmerged arm64-only canonical manifest; either case publishes a release whose auto-update metadata is absent or incomplete. Restore the previous checks that require both manifests and fail when no matching pair is found.
Useful? React with 👍 / 👎.
| const [sidebarWidth, setSidebarWidth] = useState(readInitialThreadSidebarWidth); | ||
| const sidebarMaximumWidth = resolveThreadSidebarMaximumWidth(window.innerWidth); |
There was a problem hiding this comment.
The new maxWidth is derived from window.innerWidth only when this component renders, with no resize subscription. If the user enlarges a desktop window without crossing the mobile breakpoint, the sidebar rail retains the old smaller maximum—for example, opening at 1000px and then maximizing still prevents resizing beyond roughly 360px—even though shouldAcceptWidth previously used the wrapper's live width. Track viewport changes or rely on the existing dynamic acceptance predicate.
Useful? React with 👍 / 👎.
| const assetUrl = useAssetUrlState(props.environmentId, { | ||
| _tag: "workspace-file", | ||
| threadId: props.threadRef.threadId, | ||
| path: props.absolutePath, |
There was a problem hiding this comment.
On a new unsent draft, threadRef.threadId exists only in the client draft store; the server's assetsCreateUrl handler resolves workspace-file resources through getThreadShellById, so it returns AssetWorkspaceContextNotFoundError until the first turn creates the thread. Because image paths now disable the normal project-file query and always use this asset URL, opening any image from the file panel in a fresh draft shows “Unable to load workspace image.” Use the known project/cwd context for drafts or retain a file-query/blob fallback.
Useful? React with 👍 / 👎.
cf43415 to
21b9602
Compare
Co-authored-by: Rhiz3K <rhiz3k@protonmail.com> Co-authored-by: Julius Marminge <julius0216@outlook.com> (cherry picked from commit 0936fd2)
…otgg#4140) Co-authored-by: Julius Marminge <julius0216@outlook.com> Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com> (cherry picked from commit 8ca4eec)
Co-authored-by: Julius Marminge <julius0216@outlook.com> Co-authored-by: codex <codex@users.noreply.github.com> (cherry picked from commit 2c199aa)
…up (pingdotgg#4177) Co-authored-by: codex <codex@users.noreply.github.com> Co-authored-by: Julius Marminge <julius0216@outlook.com> (cherry picked from commit db4b2d8)
Co-authored-by: codex <codex@users.noreply.github.com> (cherry picked from commit 5d34f9f)
Co-authored-by: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit 23c18fd)
…om origin main (pingdotgg#4240) Co-authored-by: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit 62cf461)
…arning rows (pingdotgg#4244) Co-authored-by: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit 6f34ad3)
21b9602 to
4bd87dd
Compare
Upstream pingdotgg#4177 added latestSequence and dropped subscribeDomainEvents; the fork needs both so shell/thread live buffers do not race PubSub subscribe. Also mark Claude Sonnet 5 as the default model, update model-selection tests for isDefault fallbacks, and stub openPath in desktop fixtures. Co-authored-by: aaditagrawal <aaditagrawal@users.noreply.github.com>
Local oxfmt rewrites the Nayuki for-loop spacing differently than CI. Keep the upstream/main form and ignore the vendored file in fmt. Co-authored-by: aaditagrawal <aaditagrawal@users.noreply.github.com>
Upstream pingdotgg#4240 made Sonnet 5 expose a contextWindow selector (200k default, 1M opt-in). Update fork tests that still assumed native 1M with no selector, and wire subscribeDomainEvents into the live-burst shell coalesce fixture. Co-authored-by: aaditagrawal <aaditagrawal@users.noreply.github.com>
Model selection on sendTurn already calls setModel for gateway ids like sonnet[1m], but lastKnownContextWindow was left stale after pingdotgg#4240, so token-usage maxTokens ignored mid-session context switches. Co-authored-by: aaditagrawal <aaditagrawal@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/release.yml (1)
570-580: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
release/*.ymlalso sweeps in electron-builder debug output.electron-builder writes
builder-debug.yml(andbuilder-effective-config.yaml) into the output dir, so this glob publishes them as release assets, and because every platform emits the samebuilder-debug.ymlthemerge-multiple: truedownload inreleasewill collide on it. Narrowing the glob avoids both.🔧 Narrow the manifest glob
- "release/*.yml"; do + "release/latest*.yml" \ + "release/nightly*.yml"; do🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml around lines 570 - 580, Update the asset-copy loop in the release workflow to replace the broad release/*.yml glob with a narrowly scoped manifest pattern that matches only the intended release manifest files, excluding electron-builder debug and effective-config outputs such as builder-debug.yml and builder-effective-config.yaml.apps/server/src/provider/Layers/ClaudeAdapter.ts (1)
3737-3750: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStale
lastKnownContextWindowafter mid-turn model switch.
sendTurnupdatescontext.currentApiModelId/context.session.modelon a model switch but no longer refreshescontext.lastKnownContextWindow, unlikestartSessionwhich seeds it viaselectedClaudeContextWindow(modelSelection). Until the turn completes (viacompleteTurn/queryCurrentContextUsage), everymessage_delta-driventhread.token-usage.updatedevent will report the previous model'smaxTokens, misrepresenting context usage for the whole turn.🐛 Proposed fix
if (modelSelection?.model) { const apiModelId = resolveClaudeApiModelId(modelSelection); if (context.currentApiModelId !== apiModelId) { yield* Effect.tryPromise({ try: () => context.query.setModel(apiModelId), catch: (cause) => toRequestError(input.threadId, "turn/setModel", cause), }); context.currentApiModelId = apiModelId; + context.lastKnownContextWindow = + selectedClaudeContextWindow(modelSelection) ?? context.lastKnownContextWindow; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/provider/Layers/ClaudeAdapter.ts` around lines 3737 - 3750, Update the model-switch branch in sendTurn to refresh context.lastKnownContextWindow using selectedClaudeContextWindow(modelSelection) after successfully switching models, matching startSession behavior. Keep the existing currentApiModelId and session.model updates unchanged so token-usage events immediately use the new model’s context window.
🧹 Nitpick comments (10)
apps/server/src/vcs/GitVcsDriverCore.test.ts (1)
599-617: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert first-page ordering, not only the flags.
This test can pass even if
origin/<initialBranch>is returned after other refs. CalllistRefswithlimit: 1and assert that the first ref is the remote default, reusing the existingfeature/only-localbranch as the competing ref.As per coding guidelines, backend changes must include and run focused tests for the changed behavior.
Suggested test adjustment
- const refs = yield* driver.listRefs({ cwd }); + const refs = yield* driver.listRefs({ cwd, limit: 1 }); + assert.equal(refs.refs[0]?.name, `origin/${initialBranch}`);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/vcs/GitVcsDriverCore.test.ts` around lines 599 - 617, The test case around GitVcsDriver.listRefs should verify ordering as well as default flags. Call listRefs with limit: 1, assert the returned first ref is origin/${initialBranch} and is marked remote/default, using the existing feature/only-local branch as the competing ref; add or update the focused test coverage for this behavior.Source: Coding guidelines
.github/workflows/release.yml (4)
610-616: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument why this job stays on the GitHub-hosted runner.
The inline
# blacksmith-8vcpu-ubuntu-2404reads as an accidental leftover. If the reason is npm trusted publishing needing a GitHub-hosted OIDC issuer, say so explicitly so a future cleanup doesn't switch it back and breakid-token: write.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml around lines 610 - 616, Update the inline runner comment on the release job’s runs-on setting to explicitly document that it must remain GitHub-hosted because npm trusted publishing requires the GitHub-hosted OIDC issuer. Remove or replace the ambiguous Blacksmith reference while preserving the existing runner and id-token: write permission.
363-375: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame tracing-config load block is copy-pasted into three jobs. Identical download +
sed+ mask +>> $GITHUB_ENVlogic appears here, inpublish_cli(Lines 639-651) and indeploy_web(Lines 816-828). A local composite action (.github/actions/load-relay-tracing) would keep the masking rule in one place — if the mask line ever drifts in one copy, the token leaks in that job's logs.Also note
cat "$config_path" >> "$GITHUB_ENV"assumes single-line values; a multi-line value in the generated file would corrupt the env file. Awhile readloop with heredoc delimiters is safer if the generator can ever emit those.As per coding guidelines: "extract shared logic instead of duplicating local implementations."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml around lines 363 - 375, The relay tracing configuration download and loading logic is duplicated across three workflow jobs and writes values unsafely to GITHUB_ENV. Extract the shared download, token masking, and environment-loading steps into the local composite action .github/actions/load-relay-tracing, update the affected jobs to invoke it, and use delimiter-based environment-file writes to preserve multiline values.Source: Coding guidelines
662-663: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftnpm publish now precedes the GitHub Release, and it isn't reversible.
releaseruns afterpublish_cli, so a failure in manifest merging or asset upload leaves annpmversion live that points at desktop artifacts and release notes that don't exist. Consider publishing to npm last, or publishing the CLI under a throwaway dist-tag and promoting the tag (npm dist-tag add) only afterreleasesucceeds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml around lines 662 - 663, Update the release workflow around the “Publish CLI package” step so the CLI is not permanently published under its final npm dist-tag until the GitHub Release completes successfully. Either move final npm publication after the release job or publish under a temporary tag and promote it with npm dist-tag add only after release succeeds, while preserving the existing version and package inputs.
741-780: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse the duplicate release publish steps
Both
Publish releaseandPublish first releasecallsoftprops/action-gh-release@v2with the same payload exceptprevious_tag; omitting it has the same effect as an empty comparison base, so keep one step and pass${{ needs.preflight.outputs.previous_tag }}.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml around lines 741 - 780, Collapse the duplicate “Publish release” and “Publish first release” steps into one softprops/action-gh-release@v2 step, using the shared payload and retaining the release condition. Include previous_tag as needs.preflight.outputs.previous_tag; its empty value must preserve first-release behavior.apps/server/src/provider/Layers/ClaudeProvider.ts (1)
142-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
contextWindowoption descriptors instead of reusing a shared factory.
claude-sonnet-5(246-254) andclaude-sonnet-4-6(273-277) manually re-declare the exact same200k-default options already encapsulated inCLAUDE_CONTEXT_WINDOW_OPTIONS/buildClaudeContextWindowDescriptor()(113-124), andclaude-fable-5(142-145)/claude-opus-4-6(207-210) duplicate an equivalent1m-default variant with no shared helper at all. A future change to the option set risks silently diverging across these four sites.Consider extracting a small factory, e.g.
buildClaudeContextWindowDescriptor({ default: "200k" | "1m" }), and reusing it at all four call sites (and forwithClaudeSonnet5ContextWindowSelector).Also applies to: 207-210, 246-254, 273-277
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/provider/Layers/ClaudeProvider.ts` around lines 142 - 145, Extract or extend a shared context-window descriptor factory near CLAUDE_CONTEXT_WINDOW_OPTIONS, such as buildClaudeContextWindowDescriptor, to support both "200k" and "1m" defaults. Replace the duplicated option arrays in withClaudeFable5ContextWindowSelector, withClaudeOpus46ContextWindowSelector, withClaudeSonnet5ContextWindowSelector, and withClaudeSonnet46ContextWindowSelector, preserving each model’s current default.apps/desktop/src/settings/DesktopAppSettings.ts (1)
53-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExport
MIN_MAIN_WINDOW_SIZEso the window creation options stop duplicating 840/620.
DesktopWindow.tshardcodesminWidth: 840/minHeight: 620, which must stay in lockstep with this schema check — otherwise Electron can produce a window whose bounds silently failDesktopWindowBoundsSchemaand never persist.As per coding guidelines, "extract shared logic instead of duplicating local implementations".
♻️ Proposed change
-const MIN_MAIN_WINDOW_SIZE = { +export const MIN_MAIN_WINDOW_SIZE = { width: 840, height: 620, } as const;Then in
apps/desktop/src/window/DesktopWindow.ts:- minWidth: 840, - minHeight: 620, + minWidth: DesktopAppSettings.MIN_MAIN_WINDOW_SIZE.width, + minHeight: DesktopAppSettings.MIN_MAIN_WINDOW_SIZE.height,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/settings/DesktopAppSettings.ts` around lines 53 - 62, Export MIN_MAIN_WINDOW_SIZE from DesktopAppSettings.ts, then update the DesktopWindow creation options to reuse its width and height values for minWidth and minHeight instead of hardcoding 840 and 620. Preserve the existing DesktopWindowBoundsSchema validation and ensure both window constraints remain synchronized.Source: Coding guidelines
apps/desktop/src/window/DesktopWindow.ts (1)
108-118: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFull containment in a single display discards valid multi-monitor layouts.
A window straddling two adjacent displays fits neither one, so it is reset to the default centered size on every launch even though it was fully visible. Consider accepting bounds whose visible intersection with some display is substantial (e.g. title bar / a minimum overlap area) instead of requiring total containment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/window/DesktopWindow.ts` around lines 108 - 118, Update windowFitsWithinDisplay to accept bounds that meaningfully overlap a display, rather than requiring complete containment within one display. Preserve rejection for windows with only negligible visibility by validating a substantial intersection, such as a visible title bar or minimum overlap area, so valid windows spanning adjacent monitors are retained.apps/web/src/components/AppSidebarLayout.tsx (1)
161-169: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMemoize the sidebar
resizableconfig to stop redundant effect reruns. Theresizableobject passed to<Sidebar>is rebuilt with a new identity on everyAppSidebarLayoutrender (inlineshouldAcceptWidtharrow), andsidebar.tsx's persisted-width hydration effect keys off that same object's identity, so it fires far more often than needed.
apps/web/src/components/AppSidebarLayout.tsx#L161-L169: wrap theresizableobject (and ideallysidebarProviderStyle) inuseMemokeyed onsidebarMaximumWidth,THREAD_SIDEBAR_MIN_WIDTH,THREAD_MAIN_CONTENT_MIN_WIDTH,THREAD_SIDEBAR_WIDTH_STORAGE_KEY, andsetSidebarWidthso its identity is stable across unrelated re-renders.apps/web/src/components/ui/sidebar.tsx#L565-L585: no change needed here once the caller'sresizablereference is stable — this effect's[resolvedResizable]dependency will then only fire on genuine config changes instead of every parent re-render.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/AppSidebarLayout.tsx` around lines 161 - 169, Memoize the resizable configuration in AppSidebarLayout using useMemo with dependencies on sidebarMaximumWidth, the referenced width constants/storage key, and setSidebarWidth; also memoize sidebarProviderStyle if needed for stable identity. In apps/web/src/components/AppSidebarLayout.tsx:161-169, update the resizable construction while preserving its existing behavior. In apps/web/src/components/ui/sidebar.tsx:565-585, make no direct changes because stabilizing the caller’s resizable reference fixes the resolvedResizable effect reruns.Source: Coding guidelines
apps/web/src/components/chat/ChangedFilesTree.tsx (1)
44-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated changed-files header into a shared component.
ChangedFilesCardandAssistantChangedFilesSectionInnerrender the same header (container classes, sticky header, pluralized file-count label,DiffStatLabel, tooltip-wrapped expand/collapse and view-diff icon buttons) with near-identical class strings, introduced in the same PR — a clear candidate for a single shared subcomponent instead of two copies that must be kept in sync manually.
apps/web/src/components/chat/ChangedFilesTree.tsx#L44-L112: extract this header (props: file count, summary stat,allDirectoriesExpanded,onToggleAllDirectories,onOpenTurnDiff) into a sharedChangedFilesCardHeadercomponent.apps/web/src/components/chat/MessagesTimeline.tsx#L1281-L1341: replace this header block with the same sharedChangedFilesCardHeadercomponent, passingturnSummary/checkpointFiles-derived props.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/chat/ChangedFilesTree.tsx` around lines 44 - 112, The duplicated changed-files header should be centralized in a shared ChangedFilesCardHeader component. In apps/web/src/components/chat/ChangedFilesTree.tsx lines 44-112, extract the container, file-count/stat display, and tooltip-wrapped expand/collapse and view-diff controls into that component with props for file count, summary stat, allDirectoriesExpanded, onToggleAllDirectories, and onOpenTurnDiff. In apps/web/src/components/chat/MessagesTimeline.tsx lines 1281-1341, replace the existing header block with ChangedFilesCardHeader, passing the corresponding turnSummary/checkpointFiles-derived values; do not change the surrounding tree or section behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/release.yml:
- Around line 215-221: Remove the “Upload relay client tracing config” artifact
step and stop transferring relay-client-tracing.env between jobs. Update each
consuming job to receive only non-secret tracing fields through job outputs and
source T3CODE_RELAY_CLIENT_OTLP_TRACES_TOKEN directly from the configured
secrets or relay state, preserving the existing tracing configuration behavior
without exposing the token as an artifact.
- Around line 285-293: The WSL `pty.node` build in the release workflow now
links against the Ubuntu 24.04 runner’s newer glibc, breaking older supported
distributions. Update the build block around `pty_pkg`, `pty_dir`, and `node-gyp
rebuild` to compile in an older-glibc environment such as the existing Bullseye
container or an equivalent `zig cc`/sysroot setup, while preserving the
generated `wsl-prebuild/pty.node` artifact flow.
- Around line 701-710: Update the “Merge macOS updater manifests” loop in the
release workflow so a missing $arm64_manifest causes the step to fail
immediately instead of silently skipping. Preserve the existing merge and
x64-manifest removal behavior when both manifests exist, and emit a clear error
before exiting for the missing-manifest case.
- Around line 672-678: Update the “Mint release app token” step using
actions/create-github-app-token@v2 to include a repositories input limited to
the current repository, using the repository name derived from
github.repository. Preserve the existing app ID, private key, and owner
configuration.
In `@apps/desktop/src/app/DesktopLifecycle.ts`:
- Around line 81-85: Update the shutdown flow in handleBeforeQuit so
DesktopWindow.flushMainWindowBounds is best-effort and bounded, ensuring a
stalled settings write cannot delay shutdown.request or shutdown.awaitComplete.
Add the established timeout/fallback behavior around the flush, then always
continue to the shutdown steps after success, failure, or timeout.
---
Outside diff comments:
In @.github/workflows/release.yml:
- Around line 570-580: Update the asset-copy loop in the release workflow to
replace the broad release/*.yml glob with a narrowly scoped manifest pattern
that matches only the intended release manifest files, excluding
electron-builder debug and effective-config outputs such as builder-debug.yml
and builder-effective-config.yaml.
In `@apps/server/src/provider/Layers/ClaudeAdapter.ts`:
- Around line 3737-3750: Update the model-switch branch in sendTurn to refresh
context.lastKnownContextWindow using selectedClaudeContextWindow(modelSelection)
after successfully switching models, matching startSession behavior. Keep the
existing currentApiModelId and session.model updates unchanged so token-usage
events immediately use the new model’s context window.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 610-616: Update the inline runner comment on the release job’s
runs-on setting to explicitly document that it must remain GitHub-hosted because
npm trusted publishing requires the GitHub-hosted OIDC issuer. Remove or replace
the ambiguous Blacksmith reference while preserving the existing runner and
id-token: write permission.
- Around line 363-375: The relay tracing configuration download and loading
logic is duplicated across three workflow jobs and writes values unsafely to
GITHUB_ENV. Extract the shared download, token masking, and environment-loading
steps into the local composite action .github/actions/load-relay-tracing, update
the affected jobs to invoke it, and use delimiter-based environment-file writes
to preserve multiline values.
- Around line 662-663: Update the release workflow around the “Publish CLI
package” step so the CLI is not permanently published under its final npm
dist-tag until the GitHub Release completes successfully. Either move final npm
publication after the release job or publish under a temporary tag and promote
it with npm dist-tag add only after release succeeds, while preserving the
existing version and package inputs.
- Around line 741-780: Collapse the duplicate “Publish release” and “Publish
first release” steps into one softprops/action-gh-release@v2 step, using the
shared payload and retaining the release condition. Include previous_tag as
needs.preflight.outputs.previous_tag; its empty value must preserve
first-release behavior.
In `@apps/desktop/src/settings/DesktopAppSettings.ts`:
- Around line 53-62: Export MIN_MAIN_WINDOW_SIZE from DesktopAppSettings.ts,
then update the DesktopWindow creation options to reuse its width and height
values for minWidth and minHeight instead of hardcoding 840 and 620. Preserve
the existing DesktopWindowBoundsSchema validation and ensure both window
constraints remain synchronized.
In `@apps/desktop/src/window/DesktopWindow.ts`:
- Around line 108-118: Update windowFitsWithinDisplay to accept bounds that
meaningfully overlap a display, rather than requiring complete containment
within one display. Preserve rejection for windows with only negligible
visibility by validating a substantial intersection, such as a visible title bar
or minimum overlap area, so valid windows spanning adjacent monitors are
retained.
In `@apps/server/src/provider/Layers/ClaudeProvider.ts`:
- Around line 142-145: Extract or extend a shared context-window descriptor
factory near CLAUDE_CONTEXT_WINDOW_OPTIONS, such as
buildClaudeContextWindowDescriptor, to support both "200k" and "1m" defaults.
Replace the duplicated option arrays in withClaudeFable5ContextWindowSelector,
withClaudeOpus46ContextWindowSelector, withClaudeSonnet5ContextWindowSelector,
and withClaudeSonnet46ContextWindowSelector, preserving each model’s current
default.
In `@apps/server/src/vcs/GitVcsDriverCore.test.ts`:
- Around line 599-617: The test case around GitVcsDriver.listRefs should verify
ordering as well as default flags. Call listRefs with limit: 1, assert the
returned first ref is origin/${initialBranch} and is marked remote/default,
using the existing feature/only-local branch as the competing ref; add or update
the focused test coverage for this behavior.
In `@apps/web/src/components/AppSidebarLayout.tsx`:
- Around line 161-169: Memoize the resizable configuration in AppSidebarLayout
using useMemo with dependencies on sidebarMaximumWidth, the referenced width
constants/storage key, and setSidebarWidth; also memoize sidebarProviderStyle if
needed for stable identity. In
apps/web/src/components/AppSidebarLayout.tsx:161-169, update the resizable
construction while preserving its existing behavior. In
apps/web/src/components/ui/sidebar.tsx:565-585, make no direct changes because
stabilizing the caller’s resizable reference fixes the resolvedResizable effect
reruns.
In `@apps/web/src/components/chat/ChangedFilesTree.tsx`:
- Around line 44-112: The duplicated changed-files header should be centralized
in a shared ChangedFilesCardHeader component. In
apps/web/src/components/chat/ChangedFilesTree.tsx lines 44-112, extract the
container, file-count/stat display, and tooltip-wrapped expand/collapse and
view-diff controls into that component with props for file count, summary stat,
allDirectoriesExpanded, onToggleAllDirectories, and onOpenTurnDiff. In
apps/web/src/components/chat/MessagesTimeline.tsx lines 1281-1341, replace the
existing header block with ChangedFilesCardHeader, passing the corresponding
turnSummary/checkpointFiles-derived values; do not change the surrounding tree
or section behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e9216c5f-ce6b-40d5-adff-8bfa208ade07
📒 Files selected for processing (59)
.github/workflows/release.ymlapps/desktop/src/app/DesktopLifecycle.tsapps/desktop/src/backend/DesktopBackendPool.test.tsapps/desktop/src/backend/DesktopServerExposure.test.tsapps/desktop/src/settings/DesktopAppSettings.test.tsapps/desktop/src/settings/DesktopAppSettings.tsapps/desktop/src/updates/DesktopUpdates.test.tsapps/desktop/src/window/DesktopApplicationMenu.test.tsapps/desktop/src/window/DesktopWindow.test.tsapps/desktop/src/window/DesktopWindow.tsapps/mobile/src/features/threads/new-task-flow-provider.tsxapps/mobile/src/lib/modelOptions.tsapps/server/src/orchestration/Layers/OrchestrationEngine.test.tsapps/server/src/orchestration/Layers/OrchestrationEngine.tsapps/server/src/orchestration/Services/OrchestrationEngine.tsapps/server/src/provider/Layers/ClaudeAdapter.test.tsapps/server/src/provider/Layers/ClaudeAdapter.tsapps/server/src/provider/Layers/ClaudeProvider.test.tsapps/server/src/provider/Layers/ClaudeProvider.tsapps/server/src/provider/Layers/CodexProvider.test.tsapps/server/src/provider/Layers/CodexProvider.tsapps/server/src/provider/Layers/ProviderRegistry.test.tsapps/server/src/relay/AgentAwarenessRelay.test.tsapps/server/src/server.test.tsapps/server/src/serverRuntimeStartup.test.tsapps/server/src/vcs/GitVcsDriverCore.test.tsapps/server/src/vcs/GitVcsDriverCore.tsapps/server/src/ws.tsapps/web/src/assets/assetUrls.tsapps/web/src/components/AppSidebarLayout.tsxapps/web/src/components/BranchToolbar.logic.test.tsapps/web/src/components/BranchToolbar.logic.tsapps/web/src/components/BranchToolbar.tsxapps/web/src/components/BranchToolbarBranchSelector.tsxapps/web/src/components/BranchToolbarEnvironmentSelector.tsxapps/web/src/components/chat/ChangedFilesTree.test.tsxapps/web/src/components/chat/ChangedFilesTree.tsxapps/web/src/components/chat/ChatComposer.tsxapps/web/src/components/chat/MessagesTimeline.test.tsxapps/web/src/components/chat/MessagesTimeline.tsxapps/web/src/components/chat/composerMentionDrag.test.tsapps/web/src/components/chat/composerMentionDrag.tsapps/web/src/components/files/FileBrowserPanel.tsxapps/web/src/components/files/FilePreviewPanel.tsxapps/web/src/components/files/fileTreeDragMention.test.tsapps/web/src/components/files/fileTreeDragMention.tsapps/web/src/components/files/projectFilesQueryState.tsapps/web/src/components/threadSidebarWidth.test.tsapps/web/src/components/threadSidebarWidth.tsapps/web/src/components/ui/sidebar.tsxapps/web/src/modelSelection.test.tsapps/web/src/modelSelection.tsapps/web/src/providerModels.tspackages/client-runtime/src/operations/projects.test.tspackages/contracts/src/model.tspackages/contracts/src/server.tspackages/contracts/src/settings.test.tspackages/contracts/src/settings.tsvite.config.ts
| - name: Upload relay client tracing config | ||
| uses: actions/upload-artifact@v7 | ||
| with: | ||
| name: relay-client-tracing-config | ||
| path: ${{ runner.temp }}/relay-client-tracing.env | ||
| if-no-files-found: error | ||
| retention-days: 1 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Tracing token is persisted to a downloadable workflow artifact.
relay-client-tracing.env contains T3CODE_RELAY_CLIENT_OTLP_TRACES_TOKEN. Artifacts are not encrypted and are downloadable by anyone with repo read access (and via the API) for the retention window, so this leaks the OTLP ingest credential outside the masked-secret boundary. Consider passing the non-secret tracing fields via job outputs and sourcing the token directly from secrets/the relay state in each consuming job instead of shipping it as a file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/release.yml around lines 215 - 221, Remove the “Upload
relay client tracing config” artifact step and stop transferring
relay-client-tracing.env between jobs. Update each consuming job to receive only
non-secret tracing fields through job outputs and source
T3CODE_RELAY_CLIENT_OTLP_TRACES_TOKEN directly from the configured secrets or
relay state, preserving the existing tracing configuration behavior without
exposing the token as an artifact.
| # Resolve node-pty from apps/server (where it's a dependency) and build | ||
| # its native binary from source for Linux against an older glibc. node- | ||
| # addon-api resolves from node-pty's own dependency tree inside the | ||
| # mounted workspace, so node-gyp has everything it needs. | ||
| # its native binary from source for Linux. node-addon-api resolves from | ||
| # node-pty's own dependency tree, so node-gyp has everything it needs. | ||
| pty_pkg="$(node -e "console.log(require.resolve('node-pty/package.json', { paths: ['$GITHUB_WORKSPACE/apps/server'] }))")" | ||
| pty_dir="$(dirname "$pty_pkg")" | ||
| docker run --rm \ | ||
| -v "$GITHUB_WORKSPACE:$GITHUB_WORKSPACE" \ | ||
| -w "$pty_dir" \ | ||
| node:24.13.1-bullseye \ | ||
| bash -lc 'apt-get update -qq && DEBIAN_FRONTEND=noninteractive apt-get install -y -qq build-essential python3 >/dev/null && npx --yes node-gyp rebuild' | ||
| ( cd "$pty_dir" && npx --yes node-gyp rebuild ) | ||
| mkdir -p wsl-prebuild | ||
| cp "$pty_dir/build/Release/pty.node" wsl-prebuild/pty.node | ||
| file wsl-prebuild/pty.node |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Dropping the bullseye container raises the glibc floor for the shipped WSL binary.
Building pty.node directly on the Ubuntu 24.04 runner links it against that runner's glibc (2.39). N-API makes it Node-version-agnostic, but not glibc-agnostic — users whose WSL distro is older (Ubuntu 22.04 → 2.35, Debian 12 → 2.36) will get GLIBC_2.3x not found at load time, which is precisely the first-launch failure this prebuild exists to avoid. Worth keeping the older-glibc build environment (container or zig cc/sysroot) unless the supported WSL distro floor is explicitly 24.04+.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/release.yml around lines 285 - 293, The WSL `pty.node`
build in the release workflow now links against the Ubuntu 24.04 runner’s newer
glibc, breaking older supported distributions. Update the build block around
`pty_pkg`, `pty_dir`, and `node-gyp rebuild` to compile in an older-glibc
environment such as the existing Bullseye container or an equivalent `zig
cc`/sysroot setup, while preserving the generated `wsl-prebuild/pty.node`
artifact flow.
| - id: app_token | ||
| name: Mint release app token | ||
| uses: actions/create-github-app-token@v2 | ||
| with: | ||
| app-id: ${{ secrets.RELEASE_APP_ID }} | ||
| private-key: ${{ secrets.RELEASE_APP_PRIVATE_KEY }} | ||
| owner: ${{ github.repository_owner }} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Scope the app token to this repository.
Without a repositories: input the minted token carries the installation's permissions for every repository the app is installed on, so any step in this job (or a compromised action in it) gets far more than release-publishing rights.
🔒 Restrict installation scope
app-id: ${{ secrets.RELEASE_APP_ID }}
private-key: ${{ secrets.RELEASE_APP_PRIVATE_KEY }}
owner: ${{ github.repository_owner }}
+ repositories: ${{ github.event.repository.name }}📝 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.
| - id: app_token | |
| name: Mint release app token | |
| uses: actions/create-github-app-token@v2 | |
| with: | |
| app-id: ${{ secrets.RELEASE_APP_ID }} | |
| private-key: ${{ secrets.RELEASE_APP_PRIVATE_KEY }} | |
| owner: ${{ github.repository_owner }} | |
| - id: app_token | |
| name: Mint release app token | |
| uses: actions/create-github-app-token@v2 | |
| with: | |
| app-id: ${{ secrets.RELEASE_APP_ID }} | |
| private-key: ${{ secrets.RELEASE_APP_PRIVATE_KEY }} | |
| owner: ${{ github.repository_owner }} | |
| repositories: ${{ github.event.repository.name }} |
🧰 Tools
🪛 zizmor (1.26.1)
[error] 678-678: dangerous use of GitHub App tokens (github-app): token granted access to all repositories for this owner's app installation
(github-app)
[error] 674-674: dangerous use of GitHub App tokens (github-app): app token inherits blanket installation permissions
(github-app)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/release.yml around lines 672 - 678, Update the “Mint
release app token” step using actions/create-github-app-token@v2 to include a
repositories input limited to the current repository, using the repository name
derived from github.repository. Preserve the existing app ID, private key, and
owner configuration.
Source: Linters/SAST tools
| - name: Merge macOS updater manifests | ||
| run: | | ||
| set -euo pipefail | ||
| shopt -s nullglob | ||
|
|
||
| found_mac_manifest=false | ||
| for x64_manifest in release-assets/*-mac-x64.yml; do | ||
| arm64_manifest="${x64_manifest/-x64.yml/.yml}" | ||
| if [[ ! -f "$arm64_manifest" ]]; then | ||
| echo "Missing matching arm64 macOS manifest for $x64_manifest" >&2 | ||
| exit 1 | ||
| arm64_manifest="${x64_manifest%-x64.yml}.yml" | ||
| if [[ -f "$arm64_manifest" ]]; then | ||
| node scripts/merge-update-manifests.ts --platform mac "$arm64_manifest" "$x64_manifest" | ||
| rm -f "$x64_manifest" | ||
| fi | ||
|
|
||
| found_mac_manifest=true | ||
| node scripts/merge-update-manifests.ts --platform mac \ | ||
| "$arm64_manifest" \ | ||
| "$x64_manifest" | ||
| rm -f "$x64_manifest" | ||
| done |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
A missing arm64 manifest silently ships a mis-named mac updater manifest.
If $arm64_manifest isn't present the loop just skips, leaving latest-mac-x64.yml in release-assets — which then gets published while latest-mac.yml is absent, so macOS auto-update silently 404s for the whole release. The (commented-out) Windows variant correctly hard-failed in this case; the mac path should too.
🐛 Fail loudly instead of skipping
for x64_manifest in release-assets/*-mac-x64.yml; do
arm64_manifest="${x64_manifest%-x64.yml}.yml"
- if [[ -f "$arm64_manifest" ]]; then
- node scripts/merge-update-manifests.ts --platform mac "$arm64_manifest" "$x64_manifest"
- rm -f "$x64_manifest"
- fi
+ if [[ ! -f "$arm64_manifest" ]]; then
+ echo "Missing matching arm64 mac manifest for $x64_manifest" >&2
+ exit 1
+ fi
+ node scripts/merge-update-manifests.ts --platform mac "$arm64_manifest" "$x64_manifest"
+ rm -f "$x64_manifest"
done📝 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.
| - name: Merge macOS updater manifests | |
| run: | | |
| set -euo pipefail | |
| shopt -s nullglob | |
| found_mac_manifest=false | |
| for x64_manifest in release-assets/*-mac-x64.yml; do | |
| arm64_manifest="${x64_manifest/-x64.yml/.yml}" | |
| if [[ ! -f "$arm64_manifest" ]]; then | |
| echo "Missing matching arm64 macOS manifest for $x64_manifest" >&2 | |
| exit 1 | |
| arm64_manifest="${x64_manifest%-x64.yml}.yml" | |
| if [[ -f "$arm64_manifest" ]]; then | |
| node scripts/merge-update-manifests.ts --platform mac "$arm64_manifest" "$x64_manifest" | |
| rm -f "$x64_manifest" | |
| fi | |
| found_mac_manifest=true | |
| node scripts/merge-update-manifests.ts --platform mac \ | |
| "$arm64_manifest" \ | |
| "$x64_manifest" | |
| rm -f "$x64_manifest" | |
| done | |
| - name: Merge macOS updater manifests | |
| run: | | |
| shopt -s nullglob | |
| for x64_manifest in release-assets/*-mac-x64.yml; do | |
| arm64_manifest="${x64_manifest%-x64.yml}.yml" | |
| if [[ ! -f "$arm64_manifest" ]]; then | |
| echo "Missing matching arm64 mac manifest for $x64_manifest" >&2 | |
| exit 1 | |
| fi | |
| node scripts/merge-update-manifests.ts --platform mac "$arm64_manifest" "$x64_manifest" | |
| rm -f "$x64_manifest" | |
| done |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/release.yml around lines 701 - 710, Update the “Merge
macOS updater manifests” loop in the release workflow so a missing
$arm64_manifest causes the step to fail immediately instead of silently
skipping. Preserve the existing merge and x64-manifest removal behavior when
both manifests exist, and emit a clear error before exiting for the
missing-manifest case.
| const shutdown = yield* DesktopShutdown.DesktopShutdown; | ||
| const desktopWindow = yield* DesktopWindow.DesktopWindow; | ||
| yield* desktopWindow.flushMainWindowBounds; | ||
| yield* shutdown.request; | ||
| yield* shutdown.awaitComplete; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Bound the bounds flush so a stalled settings write cannot block quit/relaunch.
flushMainWindowBounds joins an in-flight settings file write. handleBeforeQuit already called event.preventDefault(), so if that write stalls the app never reaches shutdown.request and never quits. Persisting window geometry is best-effort and should not gate shutdown.
As per coding guidelines, prioritize "reliability, predictable behavior under load and failures".
🛡️ Proposed fix
const shutdown = yield* DesktopShutdown.DesktopShutdown;
const desktopWindow = yield* DesktopWindow.DesktopWindow;
- yield* desktopWindow.flushMainWindowBounds;
+ yield* desktopWindow.flushMainWindowBounds.pipe(
+ Effect.timeout(MAIN_WINDOW_BOUNDS_FLUSH_TIMEOUT_MS),
+ Effect.ignore,
+ );
yield* shutdown.request;📝 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.
| const shutdown = yield* DesktopShutdown.DesktopShutdown; | |
| const desktopWindow = yield* DesktopWindow.DesktopWindow; | |
| yield* desktopWindow.flushMainWindowBounds; | |
| yield* shutdown.request; | |
| yield* shutdown.awaitComplete; | |
| const shutdown = yield* DesktopShutdown.DesktopShutdown; | |
| const desktopWindow = yield* DesktopWindow.DesktopWindow; | |
| yield* desktopWindow.flushMainWindowBounds.pipe( | |
| Effect.timeout(MAIN_WINDOW_BOUNDS_FLUSH_TIMEOUT_MS), | |
| Effect.ignore, | |
| ); | |
| yield* shutdown.request; | |
| yield* shutdown.awaitComplete; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/app/DesktopLifecycle.ts` around lines 81 - 85, Update the
shutdown flow in handleBeforeQuit so DesktopWindow.flushMainWindowBounds is
best-effort and bounded, ensuring a stalled settings write cannot delay
shutdown.request or shutdown.awaitComplete. Add the established timeout/fallback
behavior around the flush, then always continue to the shutdown steps after
success, failure, or timeout.
Source: Coding guidelines
9 participants
Summary
Ports upstream web/desktop/defaults work through pingdotgg#4244 onto fork
main(after #181).Includes fork adaptations required for the pingdotgg#4177 orchestration resume path:
subscribeDomainEvents(fork live-buffer race fix) andlatestSequence(upstream resume gap check)isDefaultresolution matches contractsisDefaultfallback pathopenPathon desktop ElectronShell fixturesMerge order
#181(merged) →#182→#184→#183→#185→#186→#187Test plan
vp run typecheckvp checkon touched filesSummary by CodeRabbit
New Features
Improvements
Bug Fixes