Add time range import feature for quick video segment trimming #2599
Conversation
This feature allows users to quickly import multiple time ranges using a simple text format like "03:05-03:10|40:05-40:10|1:03:05-1:04:05" to cut and concatenate video segments. Key changes: - Created new TimeRangeImport dialog component with live preview - Supports flexible time format: HH:MM:SS, MM:SS, or SS - Shows total duration and segment count before import - Added scissors button in TopMenu to trigger the feature - Integrated with existing segment management system The dialog provides: - Real-time validation and preview of parsed segments - Example format shown to guide users - Total duration calculation - Clear error messages for invalid input This makes it much faster to extract and concatenate specific portions of long videos without manually creating each segment.
WalkthroughAdds a TimeRangeImport modal component, integrates it into App by wiring state and handlers, exposes a new "Import time ranges" button in TopMenu, and threads a toggle prop through StreamsSelector to trigger the import flow. Also exports a TimeRange type. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant TopMenu
participant App
participant TimeRangeImport
participant StreamsSelector
participant ImportHandler as onImport
User->>TopMenu: Click "Import time ranges"
TopMenu->>App: toggleTimeRangeImport()
App->>TimeRangeImport: show modal (isShown=true)
Note right of TimeRangeImport: Modal displayed (input, preview)
User->>TimeRangeImport: Enter ranges / preview
TimeRangeImport->>TimeRangeImport: parseTimeRanges() / formatTime()
TimeRangeImport->>User: Show preview or validation error
User->>TimeRangeImport: Click "Import Segments"
TimeRangeImport->>TimeRangeImport: Validate parsed ranges
alt success
TimeRangeImport->>ImportHandler: onImport(parsedRanges)
ImportHandler->>App: apply ranges (update state)
App->>TimeRangeImport: hide modal (isShown=false)
else failure
TimeRangeImport->>User: Show error
end
%% StreamsSelector may also trigger modal
User->>StreamsSelector: Request import
StreamsSelector->>App: toggleTimeRangeImport()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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 (3)
src/renderer/src/App.tsx (1)
1656-1665: Consider adding a confirmation or append option.The current implementation replaces all existing segments (
append: false). Users who already have segments might lose their work unexpectedly. Consider either:
- Adding a confirmation dialog when segments exist
- Providing an option to append vs. replace
- At minimum, documenting this behavior clearly in the UI
src/renderer/src/components/TimeRangeImport.tsx (2)
29-29: Consider ESLint-compliant arrow function syntax.ESLint flags the arrow function parameters without parentheses and suggests using
Booleandirectly instead ofs => s.Apply this diff for cleaner code:
-const segments = input.split('|').map(s => s.trim()).filter(s => s); +const segments = input.split('|').map((s) => s.trim()).filter(Boolean);
176-184: Consider using a stable key for list items.Using array index as key can cause issues with React's reconciliation, especially if the list order changes.
Use a more stable key:
-{previewRanges.map((range, index) => ( - <div key={index} style={{ marginBottom: '.3em', fontSize: '.9em' }}> +{previewRanges.map((range, index) => ( + <div key={`${range.start}-${range.end}`} style={{ marginBottom: '.3em', fontSize: '.9em' }}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/renderer/src/App.tsx(5 hunks)src/renderer/src/TopMenu.tsx(4 hunks)src/renderer/src/components/TimeRangeImport.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/renderer/src/App.tsx (1)
src/renderer/src/components/TimeRangeImport.tsx (1)
TimeRange(210-210)
src/renderer/src/components/TimeRangeImport.tsx (3)
src/renderer/src/util/duration.ts (1)
parseDuration(53-80)src/renderer/src/colors.ts (1)
dangerColor(2-2)src/renderer/src/components/Button.tsx (1)
DialogButton(17-20)
🪛 ESLint
src/renderer/src/components/TimeRangeImport.tsx
[error] 29-29: Expected parentheses around arrow function argument.
(arrow-parens)
[error] 29-29: Expected parentheses around arrow function argument.
(arrow-parens)
[error] 29-29: arrow function is equivalent to Boolean. Use Boolean directly.
(unicorn/prefer-native-coercion-functions)
[error] 32-32: Expected parentheses around arrow function argument.
(arrow-parens)
[error] 177-177: Do not use Array index in keys
(react/no-array-index-key)
🔇 Additional comments (4)
src/renderer/src/TopMenu.tsx (1)
157-159: LGTM! Clean integration.The new Import time ranges button follows existing patterns and is well-placed in the toolbar.
src/renderer/src/App.tsx (1)
2785-2785: LGTM! Proper component integration.The TimeRangeImport component is correctly wired with appropriate props.
src/renderer/src/components/TimeRangeImport.tsx (2)
10-13: LGTM! Clean interface definition.The TimeRange interface is simple and appropriate for representing time segments.
56-207: Well-designed component with good UX!The TimeRangeImport component provides:
- Live preview of parsed segments
- Clear validation feedback
- Auto-focus for better UX
- Total duration calculation
- Helpful examples and placeholders
The implementation is clean and follows React best practices.
| const start = parseDuration(startStr); | ||
| const end = parseDuration(endStr); | ||
|
|
||
| if (start == null || end == null) { | ||
| return null; // Invalid time format | ||
| } | ||
|
|
||
| if (start >= end) { | ||
| return null; // Start must be before end | ||
| } |
There was a problem hiding this comment.
Add validation for negative time values.
The function doesn't validate that start and end are non-negative. Since parseDuration can return negative values (as seen in the codebase context), segments with negative times could be created, which doesn't make sense for video trimming.
Add validation after parsing:
const start = parseDuration(startStr);
const end = parseDuration(endStr);
if (start == null || end == null) {
return null; // Invalid time format
}
+if (start < 0 || end < 0) {
+ return null; // Times must be non-negative
+}
+
if (start >= end) {
return null; // Start must be before end
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const start = parseDuration(startStr); | |
| const end = parseDuration(endStr); | |
| if (start == null || end == null) { | |
| return null; // Invalid time format | |
| } | |
| if (start >= end) { | |
| return null; // Start must be before end | |
| } | |
| const start = parseDuration(startStr); | |
| const end = parseDuration(endStr); | |
| if (start == null || end == null) { | |
| return null; // Invalid time format | |
| } | |
| if (start < 0 || end < 0) { | |
| return null; // Times must be non-negative | |
| } | |
| if (start >= end) { | |
| return null; // Start must be before end | |
| } |
🤖 Prompt for AI Agents
In src/renderer/src/components/TimeRangeImport.tsx around lines 39 to 48, the
parsed start/end values are not checked for negativity; after calling
parseDuration for startStr and endStr, validate that both start and end are >= 0
and return null (invalid) if either is negative (in addition to the existing
null and ordering checks), ensuring segments with negative times are rejected
before further processing.
|
Thanks for your PR. How is it different from the Import YoutTube chapters feature? just the format? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
UI_LAYOUT.txt (2)
15-45: Mixed language content: Consider UI localization strategy.The dialog description mixes Vietnamese and English text. If this is meant to reflect actual UI strings, clarify whether:
- The UI will be fully localized based on user preferences, or
- Vietnamese guidance is temporary for development purposes
If documentation, consider standardizing on a single language or clearly marking which parts represent translatable strings vs. developer notes.
24-27: Format specification is clear but consider documenting edge cases.The time range format example is well-presented. For completeness, the documentation could clarify:
- Are leading zeros required (e.g.,
03:05vs3:05)?- What happens with invalid ranges (e.g., start time > end time)?
- Are duplicate ranges allowed?
These details are likely implemented in TimeRangeImport.tsx but documenting them here would help users understand constraints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
FEATURE_GUIDE.md(1 hunks)UI_LAYOUT.txt(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
FEATURE_GUIDE.md
7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
78-78: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
84-84: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
90-90: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
122-122: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (3)
FEATURE_GUIDE.md (2)
7-11: Add language identifiers to fenced code blocks and fix heading formatting to pass markdown lint.The static analysis detected markdown lint violations:
- Lines 7, 44, 78, 84, 90: Fenced code blocks need language specification (MD040)
- Line 122: Emphasis should not be used as heading (MD036)
Apply these fixes:
-``` +```bash ┌─────────────────────────────────────────────────────────────┐ │ [Tracks] [Filter] [Working Dir] [Format] [Mode] 🌙 ✂️ ⚙️ │ <- TopMenu └─────────────────────────────────────────────────────────────┘ -``` +```- ``` + ```bash - ``` + ```-``` +```bash -``` +```-``` +```bash -``` +```-``` +```bash -``` +```---- +## Additional Notes - -**Chúc bạn sử dụng tính năng hiệu quả! 🎬✂️**Also applies to: 44-46, 78-80, 84-86, 90-92, 122-122
1-122: Comprehensive guide with clear structure and practical examples.The feature guide is well-organized with step-by-step instructions, practical examples, and troubleshooting guidance. The Vietnamese documentation is clear and aligns well with the PR objectives. The guide effectively explains:
- UI entry point (Scissors button in TopMenu)
- Setup and execution flow
- Flexible time format support with examples
- Real-world use cases (intro/outro trimming, multi-segment extraction)
- Common validation issues and troubleshooting
UI_LAYOUT.txt (1)
1-45: Documentation file: Clarify the purpose and location of this design document.This file appears to be a UI specification/design document rather than executable code. While it clearly describes the intended feature layout with helpful ASCII diagrams and examples, consider whether this belongs in the main repository or should be kept in design/documentation folders or pull request discussions instead.
3 participants
This feature allows users to quickly import multiple time ranges using a simple text format like "03:05-03:10|40:05-40:10|1:03:05-1:04:05" to cut and concatenate video segments.
Key changes:
The dialog provides:
This makes it much faster to extract and concatenate specific portions of long videos without manually creating each segment.
Summary by CodeRabbit
New Features
Documentation