fix(workspace-update): prompt for option drift and immutable params#967
fix(workspace-update): prompt for option drift and immutable params#967EhabY wants to merge 2 commits into
Conversation
Aligns parameter collection with the dashboard's getMissingParameters
in coder/site/src/api/api.ts. Previously the filter only checked
`required && !default_value` against `name in storedValues`, so:
- Required params whose stored option value was no longer in the new
version's option list (option-value drift) were carried forward
and the build was rejected server-side with no useful UI.
- Immutable params without a stored value were never prompted, even
though they can never be set after the first build.
Add `needsPrompt(param, storedValue)` covering both cases. Beyond the
dashboard, also handle multi-select drift by parsing the JSON-encoded
value (dashboard explicitly skips multi-select). The `default_value`
short-circuit is preserved for the unset case so the server can fall
back to the template default during auto-update without a prompt.
|
/coder-agents-review |
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, well-scoped fix. The needsPrompt extraction is solid, the behavior table in the PR description is exactly the kind of documentation that saves future debugging time, and 6 of 7 new tests fail on the base branch (Netero verified). The deviation comment at line 55 is a model for documenting intentional divergence.
One design concern (P2), two P3s, one P3 test gap, one P4, and three nits.
The P2 is about the default_value short-circuit applying to immutable parameters. When a new immutable param has a default, the user is never prompted, and because the param is immutable, they can never override it afterward. The dashboard prompts in this case. The PR documents this as intentional ("avoid auto-update friction"), but the friction argument holds for mutable params (temporary inconvenience), not immutable ones (permanent lock-in).
Melody walked all 11 ParameterFormType variants and confirmed the dispatch/filter pairing is clean. Chopper verified edge cases ("", "[]", invalid JSON). Mafu-san traced the alignment claim against the actual dashboard code and confirmed the logic matches where claimed.
"The user chose correctly; the tool failed to preserve what was still good." (Hisoka, on multi-select drift discarding valid picks)
🤖 This review was automatically generated with Coder Agents.
EhabY
force-pushed
the
fix/workspace-update-param-drift
branch
2 times, most recently
from
fa6a8a5 to
2e176c5
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
All 9 round-1 findings addressed. The P2 (immutable default lock-in) is fixed at the root: if (!param.mutable) return true now runs before the default_value check. Kite, Pariston, Mafuuu, and Chopper independently confirmed the fix is complete. Melody walked 8 pairing modes across the updated code and found zero mismatches. Pariston's adversarial analysis couldn't build a case against the change.
One P3 test gap and two nits remain. The P3 is a missing test for the required-multi-select empty-submission guard; the behavior is correct but unprotected against regression.
"I tried to build a case against this change and couldn't." (Pariston)
🤖 This review was automatically generated with Coder Agents.
- needsPrompt: prompt immutable + default (no user override possible later
— permanent lock-in to server default otherwise). Per DEREM-3.
- promptForParameter: accept empty selection on non-required multi-select
re-prompts; the previous logic kept the QuickPick open with no way to
express "none." Per DEREM-5.
- promptForParameter: pre-select still-valid picks on multi-select drift
re-prompts so the user doesn't lose what was already correct. Per DEREM-7.
- Inline notes on both deviations from the dashboard's getMissingParameters
(default short-circuit + multi-select drift) so future syncs catch them.
- Drop manual `(v): v is string =>` predicate (TS 5.5+ inference).
- Tests: multi-select happy path, '[]' empty array, non-required empty
submission, preselect verification, immutable + default prompt.
- CHANGELOG: user-facing wording ("closing a gap with the web dashboard").
EhabY
force-pushed
the
fix/workspace-update-param-drift
branch
from
2e176c5 to
bd42d05
Compare
1 participant
Aligns workspace-update parameter collection with the dashboard's
getMissingParameters(coder/site/src/api/api.ts:39). Two cases previously slipped through and produced server-side build rejections with no UI:needsPrompt(param, storedValue)centralizes the decision; multi-select drift is also detected (parsing the JSON-encoded stored value) because the dashboard explicitly skips multi-select.On a multi-select drift re-prompt the QuickPick now pre-selects the picks that are still valid, so the user does not have to re-select what was already correct. Non-required multi-selects can now also submit zero picks (previously the prompt stayed open with no way to express "none").
Behavior matrix
"[]"Files
src/api/updateParameters.ts:needsPrompt,parseMultiSelectValue;promptForParameteracceptsstoredValueand pre-selects still-valid multi-select picks.test/unit/api/updateParameters.test.ts: 12 new cases covering single/multi drift, immutable + default, multi-select happy path, empty-array stored value, empty submission, and preselect verification.CHANGELOG.md: user-facing entry under Unreleased > Fixed.