Repository navigation
fix(speed): accept a trailing × or x in the custom speed field - #1444
Conversation
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Sanitize and validate speed drafts src/components/video-editor/customPlaybackSpeed.ts, src/components/video-editor/customPlaybackSpeed.test.ts, src/components/ai-edition/v4/SpeedControl.test.tsx |
The parser trims whitespace, converts decimal commas, removes one trailing x or ×, and returns empty when no input remains. Tests cover accepted and rejected formats, bounds, and committing 1,25× as 1.25. |
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix · Severity of issue fixed: Medium
Suggested reviewers: etiennelescot
Merge Risk: 
60014
-
The change adds multiplier-suffix support to custom speed input. No concrete merge-blocking risk is established; it is mergeable subject to normal checks.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title names the fix and the affected field: it makes the custom speed field accept a trailing × or x. The changeset does this in parseCustomPlaybackSpeedInput, and the tests cover the accepted and… Description check The description follows the repository template. It includes Summary, a Related issue that uses 'Fixes #1441' (the issue is linked and the fix is complete), Type of change (Bug fix checked), Release i…Linked Issues check [ #1441] Requirement: Custom speed must accept a trailingxor×suffix, as the Custom zoom field does. At the reviewed head,parseCustomPlaybackSpeedInputin `src/components/video-editor/customP…Out of Scope Changes check Whole-PR scope, base f7894518to head600141b4. The change touches the parser and its tests, and one component test. Each change serves the#1441fix. The stripping step adds the suffix support. T…Full details: Title check
-
Explanation
The title names the fix and the affected field: it makes the custom speed field accept a trailing × or x. The changeset does this in parseCustomPlaybackSpeedInput, and the tests cover the accepted and rejected suffix forms. The title does not mention the empty/suffix-only return change, which is a secondary part of the diff.
Full details: Description check
-
Explanation
The description follows the repository template. It includes Summary, a Related issue that uses 'Fixes
#1441' (the issue is linked and the fix is complete), Type of change (Bug fix checked), Release impact (Patch checked), Desktop impact (Not platform-specific checked), Screenshots/video (explained, with a link to the recording in#1441), and Testing with commands and results. It also explains the one related behavior change (empty vs too-slow) and why it is needed.
Full details: Linked Issues check
-
Explanation
[
#1441] Requirement: Custom speed must accept a trailingxor×suffix, as the Custom zoom field does. At the reviewed head,parseCustomPlaybackSpeedInputinsrc/components/video-editor/customPlaybackSpeed.tstrims the draft, converts commas to dots, and removes one trailing/\s*[×x]$/i. It then callsNumber()on the remainder.1.25x,1.25×,1.25X, and1,25×therefore parse to1.25. Speed bounds are checked after stripping, so suffixed values are bounded like plain values. Inputs that must stay rejected also fail:1.25xxleaves1.25x, which gives NaN;x1.25and1x2give NaN. Plain decimal input is unchanged. The summary reports new parser tests and a component test inSpeedControl.test.tsxthat types1,25×and expects a commit of1.25. I did not read those test files, so this verdict rests on the source code and the reported test results. No coding requirement of#1441is unmet.
Full details: Out of Scope Changes check
-
Explanation
Whole-PR scope, base
f7894518to head600141b4. The change touches the parser and its tests, and one component test. Each change serves the#1441fix. The stripping step adds the suffix support. The newemptyreturn for blank or bare-suffix drafts follows directly from that step: stripping a barexleaves"", andNumber("")is0, which would otherwise classify the draft astoo-slow. The author states this in the PR description. The change is related to the fix and does not add unrelated behavior. Unverified point: the claim thatSpeedControlhandlesemptyandtoo-slowthe same way. I could not read theSpeedControlsource. The pathsrc/components/ai-edition/v4/SpeedControl.tsxdid not resolve at head. This affects UI behavior, not scope, so it does not change this verdict.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 3 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Summary
Typing
1.25xor1.25×into the Custom speed field was rejected and the field reverted. The placeholder is2×and the zoom field already accepts the suffix, so the two disagreed. The parser now trims the input, converts a comma decimal to a dot, strips one trailingxor×, and passes the rest toNumber().This regressed in
59df3544(v1.8.0), which replaced the old character-stripping parser withNumber(). Details and the recording are in #1441.One related change: an empty or suffix-only draft now returns
emptyinstead oftoo-slow.SpeedControlhandles both the same way, so users see no difference. It's needed because stripping a barexleaves"", andNumber("")is0. Happy to split it out if you prefer.Related issue
Fixes #1441
Type of change
Release impact
Desktop impact
Screenshots / video
None. The field looks the same before and after. The recording of the bug is in #1441.
Testing
1.25x,1.25×,1.25X,1,25×,1.25 x, and a whitespace-padded one), rejected ones (1.25xx,x1.25,1x2), blank and bare-suffix input, and the 0.25 to 16 bounds after stripping.1,25×commits1.25and clears the field.npx vitest --run src/components/video-editor/customPlaybackSpeed.test.ts src/components/ai-edition/v4/SpeedControl.test.tsx: 29 passednpx tsc --noEmitandnpx tsc -p tsconfig.test.json --noEmit: passnpx biome checkon the three touched files: cleanSummary by CodeRabbit