Proj/setting auto update#696
Conversation
feat: [ENG-2894] WebUI updates panel with Switch toggle
… when setting off
bao-byterover
requested review from
RyanNg1403,
cuongdo-byterover,
danhdoan,
leehpham,
ncnthien,
ngduyanhece and
wzlng
as code owners
|
Claude finished @bao-byterover's task in 4m 25s —— View job Claude PR ReviewOverall score: 4.5 / 5 A focused, well-scoped feature with strong TDD discipline, clean dependency-injection boundaries, and well-articulated scope guarantees (manual Strengths
Issues raised inline
Risks already mitigated
Todos
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| export function checkForUpdatesSetting(): boolean { | ||
| try { | ||
| const path = join(getGlobalDataDir(), SETTINGS_FILENAME) | ||
| if (!existsSync(path)) return true | ||
|
|
||
| const parsed: unknown = JSON.parse(readFileSync(path, 'utf8')) | ||
| if (!isRecord(parsed)) return true | ||
| if (!isRecord(parsed.values)) return true | ||
|
|
||
| const value = parsed.values[SETTINGS_KEY] | ||
| if (typeof value !== 'boolean') return true | ||
| return value | ||
| } catch { | ||
| return true | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion (non-blocking): The fallback values (return true on lines 25, 28, 29, 32, 35) hardcode true, duplicating the default that the registry sources from UPDATE_CHECK_FOR_UPDATES_DEFAULT in src/server/constants.ts:137.
Per CLAUDE.md: "Descriptors in server/core/domain/entities/settings.ts reference src/constants.ts so a constant change updates the setting's default automatically." This helper bypasses that wiring, so if the team ever flips the default to false (or makes it env-conditional) the constant will change but this helper will silently keep returning true — the very drift the registry pattern was designed to prevent.
| export function checkForUpdatesSetting(): boolean { | |
| try { | |
| const path = join(getGlobalDataDir(), SETTINGS_FILENAME) | |
| if (!existsSync(path)) return true | |
| const parsed: unknown = JSON.parse(readFileSync(path, 'utf8')) | |
| if (!isRecord(parsed)) return true | |
| if (!isRecord(parsed.values)) return true | |
| const value = parsed.values[SETTINGS_KEY] | |
| if (typeof value !== 'boolean') return true | |
| return value | |
| } catch { | |
| return true | |
| } | |
| } | |
| import {UPDATE_CHECK_FOR_UPDATES_DEFAULT} from '../../server/constants.js' | |
| export function checkForUpdatesSetting(): boolean { | |
| try { | |
| const path = join(getGlobalDataDir(), SETTINGS_FILENAME) | |
| if (!existsSync(path)) return UPDATE_CHECK_FOR_UPDATES_DEFAULT | |
| const parsed: unknown = JSON.parse(readFileSync(path, 'utf8')) | |
| if (!isRecord(parsed)) return UPDATE_CHECK_FOR_UPDATES_DEFAULT | |
| if (!isRecord(parsed.values)) return UPDATE_CHECK_FOR_UPDATES_DEFAULT | |
| const value = parsed.values[SETTINGS_KEY] | |
| if (typeof value !== 'boolean') return UPDATE_CHECK_FOR_UPDATES_DEFAULT | |
| return value | |
| } catch { | |
| return UPDATE_CHECK_FOR_UPDATES_DEFAULT | |
| } | |
| } | |
| function isRecord(value: unknown): value is Record<string, unknown> { | |
| return typeof value === 'object' && value !== null && !Array.isArray(value) | |
| } |
Note: the suggestion above adds an import that this file doesn't currently have — apply the import line separately if you accept it.
| export function handleBlockAutoupdateWhenOff(deps: BlockAutoupdateWhenOffDeps): void { | ||
| if (deps.commandId !== 'update') return | ||
| if (!deps.argv.includes('--autoupdate')) return | ||
| if (checkForUpdatesSetting()) return | ||
| deps.exitFn(0) | ||
| } |
There was a problem hiding this comment.
question (non-blocking): Hook ordering — the PR description (and package.json:159-164) registers block-autoupdate-when-off before block-command-update-npm, which is critical: if reversed, the npm-install + setting-off + autoupdate path would log a noisy exit-1 instead of the intended silent exit.
There's no test guard that asserts this ordering. Given the risk you've already documented in the PR body ("If a future refactor reorders the hooks..."), would it be worth adding an integration-style test that reads package.json and asserts block-autoupdate-when-off precedes block-command-update-npm in oclif.hooks.init? Cheap to write, gives you a CI tripwire if anyone alphabetizes or reorders the array.
| const path = join(getGlobalDataDir(), SETTINGS_FILENAME) | ||
| if (!existsSync(path)) return true | ||
|
|
||
| const parsed: unknown = JSON.parse(readFileSync(path, 'utf8')) |
There was a problem hiding this comment.
nitpick (non-blocking): readFileSync is used here without a size guard. If a user (or a corrupted writer) ever produces a multi-megabyte settings.json, this synchronous read runs on every invocation of brv. The PR body notes "single read of a small file; measured well below 10 ms" — which is true for the happy path — but the test suite doesn't pin that assumption.
Strictly optional: add an fs.statSync size check and bail to default if the file is unexpectedly large (e.g. > 1 MB). The current try/catch already handles parse failures, so this is purely defense-in-depth against a pathologically slow startup. Skip if you think it's overkill.
| export function shouldRunUpdateCheck(args: {commandId: string | undefined}): boolean { | ||
| if (args.commandId === 'update') return false | ||
| if (process.env.BRV_ENV === 'development') return false | ||
| return checkForUpdatesSetting() | ||
| } |
There was a problem hiding this comment.
praise: Clean three-condition gate with explicit short-circuit ordering — commandId === 'update' is checked first to avoid an unnecessary filesystem read on the recursive path, then BRV_ENV === 'development', then the disk read. Good order. The test coverage in update-notifier.test.ts:212-258 covers each condition independently, and the new shouldRunUpdateCheck boundary keeps handleUpdateNotification itself pure (no env / disk side effects). Nice extraction.
2 participants
Summary
update-notifierat every init (polls npm registry, shows interactive y/n prompt when outdated). Separately,@oclif/plugin-updateschedulesbrv update --autoupdatein the background on a 1ms debounce. Users have no way to opt out of either path.block-autoupdate-when-offexits silently (process.exit(0)) whencommandId === 'update'AND argv contains--autoupdateAND the setting is off. Stops plugin-update's background autoupdate without noisy errors.update-notifierinit hook now wraps its existing logic inshouldRunUpdateCheck(), which combines: the new setting gate, the anti-recursion guard (commandId !== 'update'), and the dev-env guard (BRV_ENV !== 'development').checkForUpdatesSetting()reads the user'ssettings.json. Default istrue(back-compat). Returnsfalseonly when the file explicitly containsupdate.checkForUpdates: false.brv updateflow: tarball still works, npm still errors with existing "use npm update -g" message.(latest)/(outdated)tags): still polls and shows tags regardless of the setting. Deferred per team decision.block-command-update-npmhook: untouched. Existing aggressive block onbrv updatefor npm installs is preserved.update-notifierpackage's internal background HTTP cache refresh: unchanged.Type of change
Scope (select all touched areas)
Linked issues
Root cause (bug fixes only, otherwise write
N/A)N/A (new feature)
Test plan
test/unit/oclif/lib/check-for-updates-setting.test.ts(10 specs)test/hooks/init/block-autoupdate-when-off.test.ts(7 specs)test/hooks/init/update-notifier.test.ts(+5 newshouldRunUpdateCheckspecs, 9 existing preserved)truebrv update --autoupdateexits silentlyBRV_ENV=development)User-visible changes
update.checkForUpdates(boolean, defaulttrue) is now visible inbrv settings list,brv settings get,brv settings set, andbrv settings reset.update.checkForUpdates false:brv update --autoupdate(spawned by plugin-update) silently exits without performing an update.brv updatemanually (the manual escape hatch is unaffected).true): no behavior change from today.Evidence
End-to-end verification on the dev build (
./bin/run.js) with two temp data dirs:brv update --autoupdatebrv update --autoupdateblock-command-update-npmfires with the existing npm-install errorbrv --versionbrv --helpUnit-test RED then GREEN per TDD; final unit-test count: 8197 passing, 0 failing.
Checklist
npm test: 8197 passing, 0 failing, 16 pending)npm run lint: 0 errors, 234 pre-existing warnings)npm run typecheck: 0 errors)npm run build: tsc + vite both clean)true, preserves today's behavior)main(branch parent isproj/setting-auto-update; rebase on merge to main)Risks and mitigations
block-autoupdate-when-offis registered first inpackage.json. If a future refactor reorders the hooks so this one fires afterblock-command-update-npm, the npm-install + setting OFF + autoupdate path would log an exit-1 error fromblock-command-update-npminstead of exiting silently. The actual update is still blocked, only the cosmetics regress.block-command-update-npmalso blocks the actual update on npm installs, so no real autoupdate runs regardless of order.fs.readFileSyncofsettings.jsonat init time adds latency to everybrvinvocation.npm list -gexecSync, so net startup cost is comparable.settings.jsonto an invalid JSON shape.true(current behavior preserved). 10 unit specs cover the edge cases (missing file, malformed JSON, wrong top-level type, wrong values type, wrong value type per key).