Skip to content

feat: [ENG-2894] WebUI updates panel with Switch toggle#695

Merged
ncnthien merged 2 commits into
proj/setting-auto-updatefrom
feat/ENG-2894
May 23, 2026
Merged

feat: [ENG-2894] WebUI updates panel with Switch toggle#695
ncnthien merged 2 commits into
proj/setting-auto-updatefrom
feat/ENG-2894

Conversation

@ncnthien
Copy link
Copy Markdown
Collaborator

Summary

  • Adds an Updates panel to the WebUI Configuration page with a <Switch> bound to update.checkForUpdates via the existing settings:* transport.
  • Splits SettingsRow into an outer dispatcher + IntegerSettingsRow; boolean rows now delegate to a new BooleanSettingsRow.
  • Filters RestartBanner so it no longer fires on boolean toggles whose descriptor has restartRequired: false (mirrors the TUI fix from ENG-2893).

Linear: ENG-2894

Changes

  • restart-banner-store.tsmarkDirty(key, restartRequired); no-ops (and preserves Set identity) when false.
  • settings-row.tsx — branches boolean rows to BooleanSettingsRow; integer flow now threads row.restartRequired into both markDirty calls.
  • boolean-settings-row.tsx (new) — <Switch> with toast on success/error via formatError.
  • updates-panel.tsx (new) — mirrors task-history-panel.tsx; filters rows by category === 'updates'.
  • general.tsx — mounts <UpdatesPanel /> after <TaskHistoryPanel /> (matches CATEGORY_ORDER).
  • labels.ts — adds 'update.checkForUpdates': 'Check for updates at startup'.
  • restart-banner-store.test.ts — updates 6 existing cases for the new signature; adds 4 new cases for filter behavior and Set-identity preservation.

Test plan

  • npm run typecheck
  • npm run lint (clean on all 7 changed files)
  • npm test — 8177 passing
  • npm run build — server + WebUI bundles build clean
  • Manual: open WebUI Configuration → confirm Updates panel renders one row labeled "Check for updates at startup" as a Switch
  • Manual: toggle Switch → settings:set dispatched, cache invalidates, Switch reflects new state, no RestartBanner appears
  • Manual regression: change agentPool.maxSize (integer, restartRequired: true) → RestartBanner still appears

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 22, 2026

Claude finished @ncnthien's task in 6m 12s —— View job


Claude PR Review

  • Read changed files
  • Review restart-banner-store.ts changes + tests
  • Review settings-row.tsx dispatcher split
  • Review new boolean-settings-row.tsx
  • Review new updates-panel.tsx
  • Review general.tsx + labels.ts
  • Post inline comments for issues
  • Post summary

Summary

Overall: 4.5 / 5 — Clean, well-scoped follow-up to ENG-2893. The WebUI side now mirrors the TUI fix correctly: boolean toggles with restartRequired: false don't trigger the restart banner, and the change is locked in with both store-level unit tests and a clear dispatcher split between BooleanSettingsRow and IntegerSettingsRow. No correctness or security issues.

Correctness

  • markDirty signature change is fully propagated. All three call sites in boolean-settings-row.tsx:29 and settings-row.tsx:67,80 pass row.restartRequired — no stragglers. The TUI parallel (src/tui/features/settings/components/settings-page.tsx:38-46) filters in the consumer; the WebUI now filters in the store. Both are valid; the store-level approach is slightly tighter because it also short-circuits Set re-allocation.
  • Reset path threads restartRequired. settings-row.tsx:80 — the integer reset flow correctly carries row.restartRequired into markDirty (today all integer rows are restartRequired: true, so this is forward-looking but correct).
  • Set identity preserved on no-op. restart-banner-store.ts:13-14 returns the existing state (and existing dirtyKeys Set), so useRestartBannerStore((s) => s.dirtyKeys.size > 0) in restart-banner.tsx:10 won't re-fire. Test at restart-banner-store.test.ts:50-55 enforces this.
  • Test coverage is solid. 10 cases, including the subtle "previously dirty key + later non-restart change" ordering case (restart-banner-store.test.ts:66-70). Strict TDD pattern satisfied.

Security

No issues. No new transport surface, no new user input sinks, no auth changes. The Switch dispatches through the existing settings:set event, which is already validated by settings-validator.ts on the daemon side.

Code quality

  • Dispatcher split is appropriately Outside-InUpdatesPanel (the consumer) drove the need for a boolean row, and BooleanSettingsRow was extracted to satisfy it without overgeneralizing.
  • Style is consistent with the integer row (toast-on-success, noop-caught promises, aria-describedby, disabled while pending).
  • Two minor follow-ups flagged inline (panel description copy, optional SettingsRow discriminator tightening). Neither blocks this PR.

Inline comments

5 inline comments posted: 1 nitpick on panel description copy, 2 non-blocking suggestions (discriminator typing, file split), 2 praise notes (store identity preservation + ordering test).
· branch feat/ENG-2894

Comment thread src/webui/features/settings/components/updates-panel.tsx Outdated
Comment thread src/webui/features/settings/components/boolean-settings-row.tsx
Comment thread src/webui/features/settings/components/boolean-settings-row.tsx
Comment thread src/webui/features/settings/components/settings-row.tsx
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 22, 2026

Claude finished @ncnthien's task in 1m 4s —— View job


Claude PR Re-Review (after sync)

  • Inspect what changed since last review
  • Verify the updates-panel description fix
  • Re-check correctness of full diff vs proj/setting-auto-update
  • Post inline comments if any new issues
  • Post final summary

Summary

Overall: 5 / 5 — The only change in this push is c93d67d, which applies my prior suggestion verbatim:

- description="Background update checks for the brv CLI."
+ description="Update checks performed when brv starts."

This now matches the setting descriptor at src/server/core/domain/entities/settings.ts:144 ("Check for brv updates at startup and notify when one is available") and the row label "Check for updates at startup" in labels.ts:6. The "Background" → "at startup" mismatch is resolved.

Correctness

  • Single-line copy change. No semantic risk — text-only update to the panel header.
  • Rest of the PR is unchanged vs the prior review (verified via git diff origin/proj/setting-auto-update...HEAD --stat: same 7 files, same 152/-14 totals). All prior correctness conclusions still hold:
    • markDirty(key, restartRequired) propagation across all three call sites ✓
    • Set identity preserved on no-op ✓
    • Test coverage at 10 cases including the subtle ordering case ✓

Security

No changes to transport, validation, or auth surface. Unchanged from prior review.

Code quality

  • The three prior non-blocking nits (SettingsRow discriminator tightening, integer-row file split, optimistic-update UX) were all defended by the author with reasonable scope/consistency arguments — agreed they belong in follow-ups, not this PR.
  • No new comments to file. Ship it.

· branch feat/ENG-2894 ·

@ncnthien ncnthien merged commit 2967eff into proj/setting-auto-update May 23, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants