Repository navigation
fix(web): keep disabled-model editing equally available in both editor surfaces - #718
Merged
Merged
Conversation
…r surfaces The provider-owned-catalog kinds (Copilot, Codex, Claude Code) could not toggle a listed model from the model-list switch: the switch folded in `readOnly`, which for those kinds is always true because the upstream owns the catalog and the rows cannot be added, removed or restructured. The disabled-models combobox in the sidebar moved the same field for the same editor, so the two disagreed. The mirror-image case was broken too: the combobox was gated on Full connection editability, so a models-level editor could flip the switch but not the combobox. Both controls now read one predicate, `modelsEditable`, which expresses the actual requirement -- an editor who may change the catalog may set which listed models are enabled. Structural controls keep the shape-based restriction, so catalog-owned rows stay unaddable and uneditable. The regression test mounts both surfaces in one form and asserts they agree for Full, models-level and audit-only editors. It replaces the two workspace-interactions cases, whose assertions only covered `disabled` and missed the `readOnly` state this defect lived in.
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
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.
Problem
In the upstream detail editor, an administrator could not enable or disable a model from the model list on the right, while the disabled-models combobox in the left sidebar edited the same field fine. The two surfaces of one setting had drifted apart.
Cause
Both controls were added their read-only flags by #626, each against a different restriction:
permissionReadOnly || readOnly. HerereadOnlyis!manualModelsSupported(record), i.e. "the provider owns this catalog" — always true for Copilot, Codex and Claude Code. So the switch was read-only for exactly the kinds whose catalog can only be toggled, never restructured.immutable(editLevel !== 'full'), which is about editing the connection. That made the opposite case wrong too: a models-level editor could flip the switch but not the combobox.Fix
modelsEditable(editLevel)indata.tsis now the single statement of "may this caller set which listed models are enabled": anyone who may edit the catalog, i.e.editLevel !== 'off'. Both surfaces read it.The structural controls (add / edit-as-YAML / delete / model detail) still take the shape-based
readOnlyalongside it, so catalog-owned rows remain unaddable and uneditable.permissionReadOnlyis gone — it was the second name for the same capability, which is what let the two derivations diverge.Tests
New
disabled-model-parity_test.tsxmounts the sidebar and the workspace in one form and asserts both surfaces agree — samearia-readonly, switch actually toggles when editable — for Full (provider-owned catalog), models-level and audit-only editors.Reverting the fix makes two of its cases fail, so it observes the property the defect lived in: the old suite only asserted
disabled, neverreadOnly, which is why this shipped. The two supersededworkspace-interactionscases are removed rather than kept as duplicates.Verification
pnpm exec vitest runover all 19apps/web/__tests__/components/upstream-editor/*files — 117 passedpnpm --filter @floway-dev/web run typecheckpnpm exec eslintover the changed sources and tests