fix(site): keep activity bump editable when allow_user_autostop is on - #27083
Conversation
The UI guard added in #22112 disabled the Activity bump field and cleared its saved value whenever the template's Default autostop was 0. It did not check the "Allow users to customize autostop duration for workspaces" (allow_user_autostop) setting, so templates that relied on user-defined autostop timers had their activity_bump_ms silently cleared on save. Enable the field, preserve the value on submit, and update the helper text when either default_ttl_ms > 0 or allow_user_autostop is true. Fixes DEVEX-438.
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.9.0 | Round 3 | Last posted: Round 3, 6 findings (1 P3, 2 P4, 2 Nit, 1 Note), APPROVE. Review Finding inventoryFindings
Round logRound 1Panel: Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Gon, Leorio, Nami, Ging-ts, Ging-react, Komugi, Chopper. 1 P3, 1 P4, 1 Nit new. 1 dropped. Reviewed against 195dffc..a3a7c12. Round 2Panel: Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Nami, Leorio, Komugi, Chopper, Gon, Ging-ts, Ging-react, Razor. CRF-1 accepted, CRF-2 and CRF-3 fixed. 1 P4, 1 Nit new. 3 dropped. Reviewed against 195dffc..146650d. Round 3Panel: Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Leorio, Nami, Komugi, Chopper, Gon, Ging-ts, Ging-react, Kite. CRF-5 accepted, CRF-6 and CRF-7 fixed. No new findings posted. 1 dropped. Reviewed against 195dffc..7069c86. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3a7c12e11
ℹ️ 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".
There was a problem hiding this comment.
Well-scoped fix with strong test coverage (86% test density, 114 test lines for 19 production lines). The guard broadening is consistent across all three touchpoints (disabled state, submit path, helper text), and the backend confirms activity bump is meaningful when allow_user_autostop is true. Tests cover the full condition matrix including toggle transitions.
"I tried to build a case against this change and couldn't. The premises hold." (Pariston)
1 P3, 1 P4, 1 Nit. The P3 is about a sibling component (AutostopReminderHelperText) in the same file having the same class of allow_user_autostop gap. Outside the diff but same pattern.
site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TTLHelperText.tsx:78
P3 [CRF-2] AutostopReminderHelperText has the same class of bug this PR fixes: its guard checks !defaultTTL && !hasAutostopRequirement but does not consider allow_user_autostop. When default_ttl=0, autostop_requirement=off, and allow_user_autostop=true, the helper text reads "Autostop reminders only apply when an autostop deadline is configured," telling the admin the feature is inert when users DO have autostop deadlines.
The field itself is never disabled (only on isSubmitting), so no data is lost. But the misleading text may cause admins to skip configuring reminders, leaving users with no advance warning before workspace shutdown.
Fix: pass allowUserAutostop to AutostopReminderHelperText and broaden the guard to !defaultTTL && !hasAutostopRequirement && !allowUserAutostop. (Chopper P3, Nami P4)
🤖
🤖 This review was automatically generated with Coder Agents.
- Fix the same allow_user_autostop gap in AutostopReminderHelperText so the "no autostop deadline" hint does not appear when users can configure their own autostop timers (CRF-2). - Reword activity bump hint to reference the exact "Allow users to customize autostop duration for workspaces" checkbox label instead of the vague "enable user autostop" phrasing (CRF-3). - Move UI interaction coverage for the activity bump guard from RTL page tests into Storybook stories per site/AGENTS.md.
|
/coder-agents-review |
There was a problem hiding this comment.
All R1 findings addressed. CRF-2 fix (AutostopReminderHelperText guard broadened to check allowUserAutostop) verified by 9 reviewers. CRF-3 fix (label mismatch) matches the exact checkbox label in both helper texts. CRF-1 defense (autostop_requirement gap is a no-op) confirmed by 5 reviewers tracing the max_deadline cap in the bump SQL. Test migration from RTL to Storybook is clean and covers all prior scenarios plus new allow_user_autostop paths.
"Boring code. Consistent layering. Nothing hiding." (Hisoka)
1 P4, 1 Nit new. Both minor.
🤖 This review was automatically generated with Coder Agents.
- Refer to the "Default autostop" field by its actual UI label instead of "default TTL" in both helper texts (CRF-7). - Add a Storybook story asserting AutostopReminderHelperText suppresses the "no autostop deadline" hint when allow_user_autostop is on and every other deadline source is off (CRF-6).
|
/coder-agents-review |
There was a problem hiding this comment.
All findings resolved across 3 rounds. CRF-6 fix verified: AutostopReminderHelperTextHidesWhenAllowUserAutostopIsEnabled story covers the missing test path. CRF-7 fix verified: both helper texts now use "Default autostop" matching the field label. Guard logic is consistent across all three touchpoints, test coverage is complete (truth table fully covered per Bisky's verification), and the RTL-to-Storybook migration is clean.
"Oh, this test suite. Five new stories, full Chromium browser runs, submit-path assertions, toggle transitions. It is gorgeous. And every single stone is real." (Bisky)
No new findings. Ship it.
🤖 This review was automatically generated with Coder Agents.
The UI guard added in #22112 disabled the
Activity bumpfield and cleared its saved value whenever the template'sDefault autostopwas 0. It did not check the "Allow users to customize autostop duration for workspaces" (allow_user_autostop) setting, so templates that relied on user-defined autostop timers had theiractivity_bump_mssilently cleared when saving in the Coder UI.Enable the field, preserve the value on submit, and update the helper text when either
default_ttl_ms > 0orallow_user_autostopis true.Closes DEVEX-438.
Implementation notes
Problem
#22112 introduced a UI guard that:
Activity bump (hours)field whendefault_ttl_ms === 0.activity_bump_ms: undefinedon submit under the same condition, which the backend treats as "do not update", but combined with the disabled state users cannot re-enter a value once cleared and the previously stored value effectively becomes orphaned.The guard ignored
allow_user_autostop. When that setting is enabled, workspaces still have a scheduled stop (whatever the user configures on their workspace), soactivity_bump_msis still meaningful.Fix
Broaden the guard to consider both signals. The field is only disabled and the value only discarded when both
default_ttl_ms === 0andallow_user_autostop === false.Changes:
TemplateScheduleForm.tsxdisabledprop now checks!default_ttl_ms && !allow_user_autostop.activity_bump_mswhen either signal is truthy.allowUserAutostopthrough to the helper text.TTLHelperText.tsxActivityBumpHelperTextacceptsallowUserAutostopand only shows the "no scheduled stop" hint when neither signal is set. Updated copy mentions both signals.allow_user_autostopbefore asserting the guard fires (sinceMockTemplate.allow_user_autostopdefaults totrue).allow_user_autostopis enabled; togglingallow_user_autostopre-enables the field without touchingdefault_ttl_ms.activity_bump_msis preserved on submit whenallow_user_autostopis enabled anddefault_ttl_msis 0.