refactor(export): add lightning route policy plan#554
Conversation
📝 WalkthroughWalkthroughAdds platform-aware export-route planning types and functions and extensive tests verifying route selection, fallback ordering, and propagation of rejection/fallback reason codes for native-static, breeze-stream, and webcodecs. ChangesBackend Route Planning
Sequence DiagramsequenceDiagram
participant Caller
participant planLightningExportRoutes
participant shouldPreferNativeStaticLayoutBeforeBreeze
Caller->>planLightningExportRoutes: options { backendPreference, platform, nativeStaticLayoutAvailable, nativeStaticLayoutSkipReasons? }
planLightningExportRoutes->>shouldPreferNativeStaticLayoutBeforeBreeze: (platform, backendPreference)
shouldPreferNativeStaticLayoutBeforeBreeze-->>planLightningExportRoutes: boolean
planLightningExportRoutes->>planLightningExportRoutes: build ordered decisions (native-static-layout → breeze-stream → webcodecs)
planLightningExportRoutes-->>Caller: LightningExportRoutePlan { selectedRoute, decisions[] }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/exporter/backendPolicy.ts (1)
109-121: ⚡ Quick winConsider documenting native static layout decision for consistency.
The
preferStaticFirstpath (lines 87-107) documents all three routes in thedecisionsarray, but this path only documents breeze-stream and webcodecs. This creates an inconsistent decision structure—consumers of the plan might expect a consistent three-route breakdown across all scenarios.If native static layout is never considered for darwin auto mode, consider adding a
rejecteddecision with a reason like "platform-does-not-use-native-static-layout" to maintain structural consistency.📋 Proposed addition for consistent decision documentation
if (options.backendPreference === "auto" && shouldPreferNativeAutoBackend(options.platform)) { + decisions.push({ + route: "native-static-layout", + status: "rejected", + reasons: ["platform-does-not-use-native-static-layout"], + }); decisions.push({ route: "breeze-stream",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/exporter/backendPolicy.ts` around lines 109 - 121, The decisions array for the auto/darwin path (inside the block that checks options.backendPreference === "auto" && shouldPreferNativeAutoBackend(options.platform)) only pushes breeze-stream and webcodecs entries, creating an inconsistent decision structure vs. the preferStaticFirst path; add a third decision for the native static route (use the same route identifier used elsewhere, e.g., "native-static" or "static-layout") with status "rejected" and reason "platform-does-not-use-native-static-layout" so the decisions array always contains a three-route breakdown while leaving selectedRoute as "breeze-stream".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/exporter/backendPolicy.ts`:
- Around line 83-107: Add unit tests in src/lib/exporter/backendPolicy.test.ts
covering the branch where options.backendPreference === "breeze" for
planLightningExportRoutes: create tests for (1) nativeStaticLayoutAvailable:
true (expect selectedRoute "native-static-layout", breeze-stream status
"fallback" with reason "user-selected-breeze"), (2) nativeStaticLayoutAvailable:
false (expect selectedRoute "breeze-stream" selected and webcodecs fallback),
and (3) a case with nativeStaticLayoutSkipReasons non-empty to ensure
addNativeStaticLayoutDecision records a rejection and decisions include the
appropriate skip reason; use the same helper/setup used by existing "auto" tests
and assert decisions array and selectedRoute match the semantics in
backendPolicy.ts.
---
Nitpick comments:
In `@src/lib/exporter/backendPolicy.ts`:
- Around line 109-121: The decisions array for the auto/darwin path (inside the
block that checks options.backendPreference === "auto" &&
shouldPreferNativeAutoBackend(options.platform)) only pushes breeze-stream and
webcodecs entries, creating an inconsistent decision structure vs. the
preferStaticFirst path; add a third decision for the native static route (use
the same route identifier used elsewhere, e.g., "native-static" or
"static-layout") with status "rejected" and reason
"platform-does-not-use-native-static-layout" so the decisions array always
contains a three-route breakdown while leaving selectedRoute as "breeze-stream".
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4b9c841a-ca7d-4c0d-916c-8807947dab1c
📒 Files selected for processing (2)
src/lib/exporter/backendPolicy.test.tssrc/lib/exporter/backendPolicy.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/exporter/backendPolicy.ts (1)
83-107:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClarify the semantics of
backendPreference: "breeze"selectingnative-static-layout.When a user sets
backendPreference: "breeze", the current implementation triesnative-static-layoutfirst (line 88) and only falls back tobreeze-streamif native-static is unavailable. This means explicitly selecting "breeze" can result inselectedRoute: "native-static-layout", withbreeze-streammarked as"fallback"carrying reason"user-selected-breeze".This behavior is counterintuitive—users selecting "breeze" likely expect
breeze-streamto be used. While the tests confirm this is intentional, the code lacks documentation explaining why "breeze" preference prioritizesnative-static-layoutoverbreeze-stream.Consider adding a comment explaining the design intent (e.g., if "breeze" means "native-like pipeline" encompassing both routes), or reconsider whether the preference should be renamed to better reflect its behavior.
📝 Suggested documentation addition
const preferStaticFirst = + // Note: "breeze" preference prioritizes native-static-layout over breeze-stream + // when visually compatible, as both are part of the native/Breeze pipeline. options.backendPreference === "breeze" || shouldPreferNativeStaticLayoutBeforeBreeze(options.platform, options.backendPreference);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/exporter/backendPolicy.ts` around lines 83 - 107, The code treats backendPreference === "breeze" as preferring native-static-first which can yield selectedRoute "native-static-layout" and mark "breeze-stream" as fallback; add a clear explanatory comment immediately above the preferStaticFirst block (near the preferStaticFirst variable, addNativeStaticLayoutDecision call, and the selectedRoute/decisions logic) describing the design intent: that "breeze" is interpreted as a native-like pipeline which prefers native-static-layout when available and only falls back to breeze-stream (and why the fallback reason uses "user-selected-breeze"), so future readers/tests understand this intentional behavior rather than assuming a bug.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/lib/exporter/backendPolicy.ts`:
- Around line 83-107: The code treats backendPreference === "breeze" as
preferring native-static-first which can yield selectedRoute
"native-static-layout" and mark "breeze-stream" as fallback; add a clear
explanatory comment immediately above the preferStaticFirst block (near the
preferStaticFirst variable, addNativeStaticLayoutDecision call, and the
selectedRoute/decisions logic) describing the design intent: that "breeze" is
interpreted as a native-like pipeline which prefers native-static-layout when
available and only falls back to breeze-stream (and why the fallback reason uses
"user-selected-breeze"), so future readers/tests understand this intentional
behavior rather than assuming a bug.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b5da11c7-2501-40ff-9f45-76b76fa44abb
📒 Files selected for processing (2)
src/lib/exporter/backendPolicy.test.tssrc/lib/exporter/backendPolicy.ts
1 participant
Summary
backendPolicy.Why
This is a small architecture slice from the broader #504 work. It establishes the route policy as pure TypeScript before wiring it into
ModernVideoExporter, so the next PR can change execution flow against a tested policy contract instead of mixing policy, runtime behavior, and native helper changes in one broad diff.Validation
npm test -- src/lib/exporter/backendPolicy.test.ts(5 passed)npx tsc --noEmitnpx biome check --formatter-enabled=false src/lib/exporter/backendPolicy.ts src/lib/exporter/backendPolicy.test.tsgit diff --checkNote: full formatter-enabled Biome on Windows wants to normalize existing CRLF line endings in these files, so I used formatter-disabled scoped Biome to avoid unrelated line-ending churn.
Summary by CodeRabbit
New Features
Tests