Add Arabic language and Increase video decode tolerance for metadata tail handling#400
Open
selmamehdi48 wants to merge 9 commits intosiddharthvaddem:mainfrom
Open
Add Arabic language and Increase video decode tolerance for metadata tail handling#400selmamehdi48 wants to merge 9 commits intosiddharthvaddem:mainfrom
selmamehdi48 wants to merge 9 commits intosiddharthvaddem:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Arabic ("ar") locale support (seven new locale JSON files), updates i18n config/loader/context and validation to include "ar", introduces a local CLI permissions file, and replaces fixed streaming-decoder early-decode thresholds with a computed dynamic threshold and expanded error messaging. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d39e7fcff
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.claude/settings.local.json (1)
3-3: Scope CLI permission more narrowly (lowkey risky as-is).
Bash(npm run:*)grants execution for every npm script. That’s broader than needed and can enable unexpected script execution paths. nit: cleaner to whitelist only the scripts Claude actually needs for this repo.Proposed tightening
{ "permissions": { - "allow": ["Bash(npm run:*)"] + "allow": [ + "Bash(npm run:i18n-check)", + "Bash(npm run:lint)" + ] } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/settings.local.json at line 3, The current settings entry "allow": ["Bash(npm run:*)"] is too broad—limit CLI permissions by replacing the wildcard with an explicit whitelist of only the npm scripts this agent actually needs. Inspect package.json for the specific script names used by the repo (e.g., build, test, lint, start) and update the ".claude/settings.local.json" allow array to list only "Bash(npm run:<scriptName>)" entries for those scripts, removing the wildcard pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/i18n-check.mjs`:
- Line 14: The COMPARE_LOCALES array in scripts/i18n-check.mjs currently omits
French, leaving fr unvalidated; update the COMPARE_LOCALES constant (the array
assigned to COMPARE_LOCALES) to include "fr" alongside "zh-CN", "es", "tr", and
"ar" so French translations are included in the i18n validation run.
In `@src/contexts/I18nContext.tsx`:
- Line 67: When updating document.documentElement.lang in I18nContext (the spots
that set newLocale), also set document.documentElement.dir based on the locale:
if the locale is Arabic (e.g., equals 'ar' or startsWith('ar')), set dir =
'rtl', otherwise set dir = 'ltr'; apply this change in both places where lang is
assigned so switching locales correctly updates text direction.
In `@src/i18n/locales/ar/shortcuts.json`:
- Line 30: Update the shortcut label value for the "deleteSelectedAlt" key to
use capitalized "Alt" instead of "alt" so the string becomes "حذف المحدد (Alt)";
locate the "deleteSelectedAlt" entry in the ar locale shortcuts JSON and adjust
the casing only, leaving the key name unchanged.
---
Nitpick comments:
In @.claude/settings.local.json:
- Line 3: The current settings entry "allow": ["Bash(npm run:*)"] is too
broad—limit CLI permissions by replacing the wildcard with an explicit whitelist
of only the npm scripts this agent actually needs. Inspect package.json for the
specific script names used by the repo (e.g., build, test, lint, start) and
update the ".claude/settings.local.json" allow array to list only "Bash(npm
run:<scriptName>)" entries for those scripts, removing the wildcard pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c3a5f34f-e901-4dfe-9cf2-0663293c5b61
📒 Files selected for processing (12)
.claude/settings.local.jsonscripts/i18n-check.mjssrc/contexts/I18nContext.tsxsrc/i18n/config.tssrc/i18n/loader.tssrc/i18n/locales/ar/common.jsonsrc/i18n/locales/ar/dialogs.jsonsrc/i18n/locales/ar/editor.jsonsrc/i18n/locales/ar/launch.jsonsrc/i18n/locales/ar/settings.jsonsrc/i18n/locales/ar/shortcuts.jsonsrc/i18n/locales/ar/timeline.json
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/exporter/streamingDecoder.ts`:
- Around line 505-508: The error message incorrectly shows "gap: 0.000s" when no
frames decoded because lastDecodedFrameSec === null; update the throw in
streamingDecoder.ts to compute a gap representation conditionally (referencing
lastDecodedFrameSec, requiredEndSec, decodedAtLabel) — e.g. if
lastDecodedFrameSec is null set gapStr to "unknown" (or
requiredEndSec.toFixed(3) to indicate full duration) else compute gapSec =
requiredEndSec - lastDecodedFrameSec and format it; then include gapStr (not a
numeric 0) in the process of building the Error message so the log accurately
reflects the unknown gap when no frame decoded.
- Around line 70-72: EARLY_DECODE_END_THRESHOLD_SEC being a fixed 2.5s can let
large relative truncations slip for short exports; change the logic that checks
for "early decode end" to compute a dynamic threshold = min(ABS_CAP_SEC,
duration * RELATIVE_FRACTION) (keep an absolute cap e.g. 2.5s and a relative
fraction like 10–20%), and use that computed threshold instead of the constant
EARLY_DECODE_END_THRESHOLD_SEC when comparing decodedLength vs expected
duration; update any related tolerance checks (e.g., METADATA_TAIL_TOLERANCE_SEC
/ STREAM_DURATION_MATCH_TOLERANCE_SEC usage) to reference the new dynamic
threshold or remain distinct as intended so short clips fail loudly rather than
silently passing truncated tails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3a81d2fc-875c-44ce-b420-e2f0c6d23137
📒 Files selected for processing (1)
src/lib/exporter/streamingDecoder.ts
- Add French (fr) to i18n validation in i18n-check.mjs - Improve error message when no frames decoded (show 'unknown gap' instead of 0.000s) - Add RTL direction support for Arabic locale in I18nContext - Capitalize 'Alt' key in Arabic shortcuts translation
selmamehdi48
changed the title
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/lib/exporter/streamingDecoder.ts (1)
70-72:⚠️ Potential issue | 🟠 MajorFixed 2.5s grace is lowkey risky for short videos
Line 70 keeps an absolute pass window, so short exports can still pass with a big relative tail cut. A duration-relative threshold with an absolute cap would fail these cases more reliably.
nit: cleaner dynamic thresholding
const EARLY_DECODE_END_THRESHOLD_SEC = 2.5; +const EARLY_DECODE_END_RATIO = 0.1; +const EARLY_DECODE_END_MIN_SEC = 0.25; const METADATA_TAIL_TOLERANCE_SEC = 2.5; const STREAM_DURATION_MATCH_TOLERANCE_SEC = 0.5; @@ const decodeGapSec = requiredEndSec - lastDecodedFrameSec; - if (decodeGapSec <= EARLY_DECODE_END_THRESHOLD_SEC) { + const allowedDecodeGapSec = Math.min( + EARLY_DECODE_END_THRESHOLD_SEC, + Math.max(EARLY_DECODE_END_MIN_SEC, requiredEndSec * EARLY_DECODE_END_RATIO), + ); + if (decodeGapSec <= allowedDecodeGapSec) { return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/streamingDecoder.ts` around lines 70 - 72, The fixed 2.5s absolute early-decode threshold (EARLY_DECODE_END_THRESHOLD_SEC) allows short exports to pass despite large relative tail cuts; replace the hard constant with a computed threshold (e.g., function computeEarlyDecodeThreshold(durationSec)) that uses a duration-relative factor (like a small percentage of duration) with an absolute upper cap (e.g., 2.5s) so the threshold = min(ABS_CAP, durationSec * RELATIVE_FACTOR), update all uses of EARLY_DECODE_END_THRESHOLD_SEC in streamingDecoder.ts to call this function (keep METADATA_TAIL_TOLERANCE_SEC and STREAM_DURATION_MATCH_TOLERANCE_SEC unchanged unless needed) and add unit tests or checks where decode/end time comparisons occur to pass the stream duration into the new computation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/lib/exporter/streamingDecoder.ts`:
- Around line 70-72: The fixed 2.5s absolute early-decode threshold
(EARLY_DECODE_END_THRESHOLD_SEC) allows short exports to pass despite large
relative tail cuts; replace the hard constant with a computed threshold (e.g.,
function computeEarlyDecodeThreshold(durationSec)) that uses a duration-relative
factor (like a small percentage of duration) with an absolute upper cap (e.g.,
2.5s) so the threshold = min(ABS_CAP, durationSec * RELATIVE_FACTOR), update all
uses of EARLY_DECODE_END_THRESHOLD_SEC in streamingDecoder.ts to call this
function (keep METADATA_TAIL_TOLERANCE_SEC and
STREAM_DURATION_MATCH_TOLERANCE_SEC unchanged unless needed) and add unit tests
or checks where decode/end time comparisons occur to pass the stream duration
into the new computation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9bc1316b-a38b-48d4-a165-1023fe39039b
📒 Files selected for processing (4)
scripts/i18n-check.mjssrc/contexts/I18nContext.tsxsrc/i18n/locales/ar/shortcuts.jsonsrc/lib/exporter/streamingDecoder.ts
✅ Files skipped from review due to trivial changes (1)
- src/i18n/locales/ar/shortcuts.json
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/i18n-check.mjs
- src/contexts/I18nContext.tsx
- Replace fixed 2.5s threshold with dynamic computation based on duration - Short videos now use relative threshold (15% of duration) - Long videos capped at 2.5s absolute threshold - Prevents silent failures for truncated short clips (e.g., 40% loss) - Add computeEarlyDecodeThreshold() function with examples - Include computed threshold in error diagnostics
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/lib/exporter/streamingDecoder.ts (1)
74-75:⚠️ Potential issue | 🟠 MajorBound the metadata-tail fallback with the same dynamic threshold.
lowkey risky: the new relative threshold only protects the direct
decodeGapSecpath. The metadata-tail escape hatch below still uses fixed2.5s/0.5swindows, so short clips can still silently pass big truncations. For example, a 5s export withstreamDurationSec = 3.0andlastDecodedFrameSec = 2.6still returnsfalsehere, even though ~48% of the clip is missing. Please cap that fallback withcomputeEarlyDecodeThreshold(requiredEndSec)too, otherwise the short-clip hole is still open.Also applies to: 83-85, 103-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/streamingDecoder.ts` around lines 74 - 75, The metadata-tail fallback uses fixed constants METADATA_TAIL_TOLERANCE_SEC and STREAM_DURATION_MATCH_TOLERANCE_SEC which can allow large relative truncations for short clips; replace or clamp those fixed windows by computing the dynamic threshold via computeEarlyDecodeThreshold(requiredEndSec) and using Math.min(METADATA_TAIL_TOLERANCE_SEC, computeEarlyDecodeThreshold(requiredEndSec)) (and similarly for STREAM_DURATION_MATCH_TOLERANCE_SEC) wherever the fallback checks (the branches that inspect decodeGapSec, lastDecodedFrameSec, and streamDurationSec) are performed (including the other occurrences noted around the checks at lines ~83-85 and ~103-104). Ensure the fallback comparisons use this bounded value so short exports get the same relative protection as the direct decodeGapSec path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/lib/exporter/streamingDecoder.ts`:
- Around line 74-75: The metadata-tail fallback uses fixed constants
METADATA_TAIL_TOLERANCE_SEC and STREAM_DURATION_MATCH_TOLERANCE_SEC which can
allow large relative truncations for short clips; replace or clamp those fixed
windows by computing the dynamic threshold via
computeEarlyDecodeThreshold(requiredEndSec) and using
Math.min(METADATA_TAIL_TOLERANCE_SEC,
computeEarlyDecodeThreshold(requiredEndSec)) (and similarly for
STREAM_DURATION_MATCH_TOLERANCE_SEC) wherever the fallback checks (the branches
that inspect decodeGapSec, lastDecodedFrameSec, and streamDurationSec) are
performed (including the other occurrences noted around the checks at lines
~83-85 and ~103-104). Ensure the fallback comparisons use this bounded value so
short exports get the same relative protection as the direct decodeGapSec path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 65e440f5-3783-4490-9004-90cf0324bec4
📒 Files selected for processing (1)
src/lib/exporter/streamingDecoder.ts
- Use min(fixed_tolerance, dynamic_threshold) for metadata tail checks - Ensures consistent handling of truncation across short and long videos - Prevents fallback path from bypassing dynamic threshold protections - Short 5s video now uses clamped 0.75s tolerance instead of fixed 2.5s - Maintains metadata padding support for standard long videos
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/exporter/streamingDecoder.ts`:
- Around line 115-123: The existing logic clamps streamDurationMatchTolerance
and metadataTailTolerance separately but still allows their sum to exceed
earlyDecodeThreshold, so update the check around decodedNearStreamEnd and
metadataTailSec to also enforce the combined gap cap: compute the totalDecodeGap
= Math.abs(lastDecodedFrameSec - streamDurationSec) + metadataTailSec (using
lastDecodedFrameSec, streamDurationSec, and metadataTailSec) and require
totalDecodeGap <= earlyDecodeThreshold in addition to the current per-part
tolerance checks (streamDurationMatchTolerance and metadataTailTolerance) before
allowing the metadata fallback path.
- Around line 83-85: computeEarlyDecodeThreshold currently returns
Math.min(EARLY_DECODE_END_THRESHOLD_ABS_CAP_SEC, relativeThreshold) which can be
too small for ultra-short clips; add a small floor constant (e.g.
EARLY_DECODE_END_THRESHOLD_MIN_SEC) and clamp the relative threshold against
this floor before applying the absolute cap. Update computeEarlyDecodeThreshold
to compute relativeThreshold = durationSec *
EARLY_DECODE_END_THRESHOLD_RELATIVE_FACTOR, then clamped =
Math.max(EARLY_DECODE_END_THRESHOLD_MIN_SEC, relativeThreshold), and finally
return Math.min(EARLY_DECODE_END_THRESHOLD_ABS_CAP_SEC, clamped) so thresholds
never fall below the new minimum.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1d71caf6-f886-4e63-9301-cc18a196678e
📒 Files selected for processing (1)
src/lib/exporter/streamingDecoder.ts
Per-part tolerance clamping allowed the sum of streamEndGap and metadataTailSec to exceed earlyDecodeThreshold when each part was individually within its limit. Add a totalDecodeGap check so the metadata fallback only passes when the combined gap stays within the dynamic threshold.
A 0.2s clip produced a 0.03s threshold — below one frame at 30fps — causing any natural decoder timing jitter to trigger a false failure. Clamp the relative threshold to EARLY_DECODE_END_THRESHOLD_MIN_SEC (0.25s) before applying the absolute cap so all clips get at least ~8 frames worth of tolerance.
When a user quickly changes settings (wallpaper, borderRadius, shadow, etc.) and immediately clicks export, React's state batching could cause the export to use closure-captured stale values instead of the new ones. Add commitState() before export to flush all pending state updates and guarantee the export uses the current editor state.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#395
Title
Improve video decode robustness with dynamic threshold handling
Add Arabic language
Description
Problem
Video export fails with "Video decode ended early" errors for valid videos, especially short clips and files with metadata padding. The original implementation used fixed tolerance thresholds that didn't account for:
Root Causes
Solution
Implemented a multi-level dynamic threshold system:
Dynamic primary threshold (15% of duration, capped 0.25s-2.5s)
Clamped fallback tolerances
Combined gap enforcement
Floor threshold (0.25s minimum)
Changes
src/lib/exporter/streamingDecoder.tssrc/contexts/I18nContext.tsxscripts/i18n-check.mjssrc/i18n/locales/ar/shortcuts.jsonExamples
5-second video:
0.2-second ultra-short clip:
30-second video with metadata:
Testing
npm run i18n:checkto verify French is now validatedCommits
6b0cd78- Initial tolerance increase (1s → 2.5s)3474396- Fix i18n validation and diagnostics293013a- Implement dynamic threshold based on duration4e3fe81- Clamp metadata fallback tolerances9a03ece- Enforce combined gap cap79a349c- Add floor threshold for ultra-short clipsRelated Issues
Fixes video export truncation detection for short clips and metadata-padded files.
Checklist