Repository navigation
Conversation
There was a problem hiding this comment.
ℹ️ No critical issues — one minor inconsistency in the
disable-ask-aihandling.
Reviewed changes
- Boolean flag list: seven renderer flags (
throw-if-site-exists,cross-site-isolation,prefer-lossless,enable-multiprocess-on-linux,enable-folder-expiry,enable-lambda-insights,disable-ask-ai) are added toBooleanFlags, so minimist now parses an explicit--flag falseinto booleanfalseinstead of the string"false". nulldefaults: the five flags whosegetValue()distinguishes "provided" from "absent" get anullentry in the minimistdefaultmap, so an absent flag no longer reads asfalseand the config file can still win.getValue()guards: those five switch to!== undefined && !== null(four also wrap the value inBoolean()) — the same shapedisableWebSecurityOptionalready uses.
I verified the parsing against the repo's own minimist: all seven now parse --flag false → false, a bare --flag → true, and absent → null for the five with a default entry. I also checked that the shared CliInternals.BooleanFlags is spread into the Lambda and CloudRun minimist configs (which lack the null defaults), but every getValue() call site passes CliInternals.parsedCli, so those commands are unaffected.
deepseek-v4.1-flash | 𝕏
|
Inline note for the
if (commandLine[cliFlag] !== undefined && commandLine[cliFlag] !== null) {
askAIEnabled = !Boolean(commandLine[cliFlag]);If presence-only is intentional for this flag, it may be worth noting in the PR description, since the current wording reads as if
|

Summary
packages/cli/src/parsed-cli.tshands minimist a list of boolean flags (boolean: BooleanFlags), so that--overwrite,--sequenceand friends parse into real booleans. Seven boolean-typed renderer options never made it into that list:--throw-if-site-exists,--cross-site-isolation,--prefer-lossless,--enable-multiprocess-on-linux,--enable-folder-expiry,--enable-lambda-insightsand--disable-ask-ai. Because those flags are unknown to minimist's boolean parser, passing them with an explicit value parsesfalseinto the five-character string "false", and a truthy string in JavaScript is not a request to turn something off.That string then flows into each option's
getValue().throwIfSiteExistsOptionand the prefer-lossless option use a truthy check on the CLI value, so--throw-if-site-exists falseand--prefer-lossless falsereturn{source: 'cli', value: true}: the user asks to switch the option off and the option comes back on. The other five (cross-site-isolation.tsx,enable-multiprocess-on-linux.tsx,folder-expiry.tsx,enable-lambda-insights.tsxandask-ai.tsx) distinguish "flag provided" from "flag absent" with!== undefined, so the literal string "false" is passed down where a boolean is expected. The worst case is--enable-folder-expiry false: the request to switch the S3 lifecycle policy off arrives as a truthy string, and the deploy keeps the auto-delete behavior the user just asked to disable.This PR adds the seven flags to
BooleanFlags. That alone is not enough for five of them: minimist parses an absent boolean-listed flag asfalserather thanundefined, so agetValue()written as!== undefinedwould read "flag not provided" as "flag provided, false" and the config file could no longer win. The repo already solved this fordisableWebSecurityOption, and this PR copies that exact shape: the five flags get anullentry in the minimistdefaultmap, and theirgetValue()guards with!== undefined && !== nulland wraps the CLI value inBoolean().throw-if-site-existsandprefer-losslessuse truthy checks, so they need no default entry, exactly likeenforce-audioandskip-skills.After this change,
--flag falseparses as a booleanfalsefor all seven. For the five guarded options, an explicit--flag falsenow genuinely disables the option and wins over the config file, which it never managed before. For the two truthy-check options,--flag falsefalls through to the config value, the same precedence every other flag in the list already has. A bare--flagstill parses astrue, and leaving the flag out still reads the config file and then the default, unchanged.Validation
Following the repository rule about not adding new tests, I verified this with a throwaway script that imports the real
BooleanFlagsarray and runs the repo's own minimist install with the same configurationparsed-cli.tsuses, including thedefaultmap. The script was removed before committing, so the PR is source-only. Happy to add a permanent regression test if you would like one.--prefer-lossless falseand--enable-folder-expiry falseparsing to the string "false" with truthinesstrue, and--cross-site-isolation falseto the literal string. After the change, all seven parse--flag falseinto booleanfalse, a bare--flagintotrue, and an absent flag intonullfor the five with a default entry. Reverting theBooleanFlagsedit brings the string behavior back.packages/rendererat all: with the flags in the boolean list, an absent flag parses asfalse, which would make the five!== undefinedchecks treat "not provided" as "provided, false" and freeze the config file out of those options. Thenulldefault plus the!== undefined && !== nullguard is the patterndisable-web-security.tsxalready uses; I copied it rather than inventing a new one.bun test src/test/boolean-option-precedence.test.tsinpackages/rendererpasses (1 test, 28 expectations) with the new guards in place, so the config-over-CLI precedence the suite pins down is preserved.bun test srcinpackages/renderer: 377 passing, 3 skipped, 5 failing, and the five failures are identical before and after the change, which I checked by stashing the diff and re-running. They are "Need to pass valid metadata" twice, "Returns buffer in promise result", "Catches invalid image format" and "Fast Start is selected from the output container", all of which need the built compositor binary that this checkout does not have.bun test src/testinpackages/cli: 194 passing, 3 failing, same three failures before and after ("all source Remotion skills match the current Remotion version", "Get GitHub repo", "forwards the correct arguments to the skills CLI"). They need network access and a GitHub API round trip.bunx tsc -d --noEmitreports 0 errors in bothpackages/cliandpackages/renderer.bunx eslint srcreports 0 errors inpackages/renderer. Inpackages/clieslint cannot run in this checkout at all (eslint-plugin-react-hooksis missing and even an untouched file fails to load the config), so that check is left to CI.Tradeoffs
The two groups of flags behave slightly differently on an explicit
--flag false, and that is inherited from the repo's own conventions rather than decided here: for the five guarded options, an explicitfalseon the command line wins over the config file, while for the two truthy-check options an explicitfalsefalls through to the config value, because theirgetValue()can only return the CLI value when it is truthy. Unifying the two groups would mean rewriting the two truthygetValue()implementations, which is a behavior change well beyond this parsing fix. Anyone who wantsprefer-losslessoff via the config file can setConfig.setPreferLosslessAudio(false)inremotion.config.ts, and that works the same before and after this PR.