Skip to content

fix(desktop): one unreadable value in desktop-settings.json no longer resets every desktop setting - #28682

Open
only21mil wants to merge 1 commit into
pingdotgg:mainfrom
only21mil:fix/audit-m060
Open

only21mil wants to merge 1 commit into
pingdotgg:mainfrom
only21mil:fix/audit-m060

Conversation

@only21mil

Copy link
Copy Markdown
Contributor

Problem

If desktop-settings.json holds one value this build can't read, the desktop app ignores the whole file and starts from defaults. For example, "updateChannel": "beta" or "wslOnly": "yes", which can come from a hand edit or from another build. Local environment, network access, Tailscale, WSL and window settings all go back to defaults, and the next settings save writes those defaults over the file, so the user's choices are gone for good.

normalizeDesktopSettingsDocument already falls back field by field. The strict document decode in readSettings fails first, though, and Effect.orElseSucceed swaps in the defaults for every field.

Change

The document decode now drops a key whose value it can't read, so that field falls back to its default and every other field still loads. It maps the existing document schema's fields through Schema.catchDecoding. Field types, defaults, the normalizer and the write path are unchanged. Malformed JSON, or a file that isn't an object, still falls back to defaults as before.

Mapping the fields, instead of rewriting each one, also covers any field added later.

A value the build can't read is still not kept. The next save rewrites the file from the loaded settings, the same as for values the normalizer already replaces, such as an out-of-range Tailscale port.

Scope and approval

This is a small, focused fix for an obvious bug, so it needs no prior issue under CONTRIBUTING.md's prior-approval rules: one bad field should not throw away the other valid settings, and the normalizer already treats fields independently. It doesn't redesign the settings schema or defaults.

Possibly related: #26144, where localEnvironmentEnabled: false disappeared from desktop-settings.json. The run below shows the same symptom from a file with one unreadable value, but that report doesn't say what the file held, so this PR doesn't claim to fix it.

Verification

  • vp test run apps/desktop/src/settings/DesktopAppSettings.test.ts: I added "updateChannel": "beta" and "wslOnly": "yes" to the existing lenient-JSON load case. On main it fails, because every field comes back as its default (serverExposureMode: "local-only", tailscaleServePort: 443, mainWindowBounds: null). With the fix, 12/12 pass and the valid fields load.

  • vp run check:changed 8760d063e9: format, lint, knip, typecheck (2 packages) and tests pass.

  • Real desktop app, base vs this branch, unpackaged Electron 44 with an isolated home, on Linux, macOS 27 and Windows 11. Before each launch I wrote the same desktop-settings.json:

    { "localEnvironmentEnabled": false, "updateChannel": "beta" }

    The result was the same on all three. On main the local backend started and Settings → Environments showed Local environment on, and after a normal close the saved file held only mainWindowBounds, so localEnvironmentEnabled was gone. With this PR the app showed "This computer is off", the toggle was off, and the saved file kept "localEnvironmentEnabled": false (updateChannel was dropped).

Linux, before (main): Local environment is on after loading the file.

Linux: Local environment on after loading on main

Linux, after (this PR): Local environment stays off.

Linux: Local environment off after loading with this PR

macOS, before (main): Local environment is on after loading the file.

macOS: Local environment on after loading on main

macOS, after (this PR): Local environment stays off.

macOS: Local environment off after loading with this PR

Windows, before (main): Local environment is on after loading the file.

Windows: Local environment on after loading on main

Windows, after (this PR): Local environment stays off.

Windows: Local environment off after loading with this PR

Not checked: packaged installers, and settings files from real older or newer builds. The runs used a hand-written unreadable value.

Prior PRs

None make this fix. #3265, #3379 and #3432 by juliusmarminge reworked how desktop settings errors are structured and diagnosed. #26757 by t3dotgg adds a field to the same document schema and benefits from this without changes.

Generated with Claude Opus 5.5 and GPT-6.1 Terra and GPT-6.1 Sol in T3 Code Nightly

… resets every desktop setting

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Nov 1, 2026
@macroscopeapp

macroscopeapp Bot commented Nov 1, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at a3ca0bd

Macroscope's review found this PR approvable — Unreadable desktop settings now fall back individually, preserving other saved preferences. Existing defaults and validation remain unchanged, with no new operational behavior introduced.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Nov 1, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 82b10d5 and a3ca0bd.

Files selected for processing: 2
  • apps/desktop/src/settings/DesktopAppSettings.test.ts
  • apps/desktop/src/settings/DesktopAppSettings.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.

Walkthrough

Desktop settings JSON decoding now omits fields that cannot be decoded instead of failing the entire document. A test adds unsupported values for updateChannel and wslOnly and checks that their defaults remain.

Changes: Desktop settings

Layer / File(s) Summary
Per-field settings decoding
apps/desktop/src/settings/DesktopAppSettings.ts, apps/desktop/src/settings/DesktopAppSettings.test.ts
The decoder applies dropUnreadableKey to each settings field. The test checks that unsupported updateChannel and wslOnly values leave their defaults intact.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Possibly related issues

  • pingdotgg/t3code#26144 — This issue concerns persistence of localEnvironmentEnabled; the change allows other settings fields to continue decoding when one field is unreadable.

Suggested reviewers: juliusmarminge, t3dotgg

Merge Risk: Minimal · up to a3ca0

The settings change has no remaining actionable merge risk.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly and concisely describes the primary fix: preserving valid desktop settings when one value is unreadable.
Description check Passed The description covers the problem, change, scope and approval rationale, verification results, cross-platform checks, limitations, and agent attribution. It is specific and complete.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Finishing Touches
Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

@maria-rcks

maria-rcks commented Nov 2, 2026 •

Copy link
Copy Markdown
Collaborator

Note

Written by gpt-6.1-sol on behalf of Maria

Verified in a real dev environment

verified head a3ca0bde317205c476237d86d4ea673cf21358dc as-is on linux, using the real electron 44.10.0 desktop client, dev renderer/server, and playwright from the repository. no mocks or seeded app data.

reproduced on github/main (e925dd681904) first. both launches received this same settings document in separate task homes:

{"localEnvironmentEnabled":false,"updateChannel":"beta","wslOnly":"yes","mainWindowBounds":{"x":120,"y":80,"width":1280,"height":900}}
  • main started the local backend, showed the local environment switch on, and used default 1100×780 bounds. its next save dropped the local-off choice.
  • this head skipped the local backend, showed "This computer is off", kept the switch off, and restored 1280×900 bounds at (120,80).
  • restarted with the same file: still off. cancelling enable left it off. confirming enable started the local environment after restart.
  • created a project through the client pointing at a real git repository. codex gpt-6.1-sol, through the local proxy, completed two client-submitted turns: cat README.md returned the repository marker; a follow-up git status --short returned a clean tree. both commands exited 0 and both turns completed without errors.
  • disabled the local environment through settings and restarted again: still off, no desktop backend listener, bounds preserved. the saved document retained localEnvironmentEnabled:false and removed the unreadable fields.

the development harness requests relaunch by exiting with code 75; i relaunched the identical electron command after each restart request. session d-bus was isolated to avoid configuring the vm's keyring.

vp run check:changed 8760d063e903 passed through blacksmith-check: format, lint, knip, typechecks for both affected packages, and all 12 desktop settings tests. existing ci reports 31 successful checks. no source changes were needed.

before, main:

main incorrectly enables the local environment

after, this head with the same input:

pr preserves the disabled local environment and saved window size

local environment enable confirmation in the real desktop client

real codex follow-up turn executes git status and completes

runtime transcript

Not verified

macos, windows/wsl, native ios/android, packaged installers, tailscale exposure, remote/relay connections, and update downloads were not exercised on this linux vm. linux keyring integration was not exercised. invalid update-channel and wsl values were hand-written to reproduce the reported file corruption; project, thread, and provider data were created through the real app.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants