Auto filter end tags, add 1 click offset #75
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes made in the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/app/(core)/v/[slug]/player.tsx (4 hunks)
🧰 Additional context used
🪛 Biome
src/app/(core)/v/[slug]/player.tsx
[error] 24-24: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
| className={`-m-2 rounded-full px-2 py-1 text-sm font-bold ${ | ||
| marker.type === "start" | ||
| ? "bg-green-800 text-white" | ||
| : marker.type === "end" | ||
| ? "bg-red-800 text-white" | ||
| : marker.type === "offset" | ||
| ? "bg-blue-800 text-white" | ||
| : "bg-gray-800 text-white" | ||
| }`} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor nested ternary operator for clarity
The nested ternary operator used for className determination reduces readability. Consider using a mapping object or a helper function to enhance clarity.
Example using a mapping object:
const typeClassMap = {
start: "bg-green-800 text-white",
end: "bg-red-800 text-white",
offset: "bg-blue-800 text-white",
default: "bg-gray-800 text-white",
};
...
className={`-m-2 rounded-full px-2 py-1 text-sm font-bold ${
typeClassMap[marker.type] || typeClassMap.default
}`}|
|
||
| return value | ||
| .split(":") | ||
| .reduce((acc, cur) => (acc = acc * 60 + parseInt(cur, 10)), 0); |
There was a problem hiding this comment.
Avoid assignment within expression in reduce callback
The assignment within the reduce function can be confusing and is unnecessary. Assignments in expressions may lead to unintended side effects and reduce code clarity.
Apply this diff to remove the assignment:
- .reduce((acc, cur) => (acc = acc * 60 + parseInt(cur, 10)), 0);
+ .reduce((acc, cur) => acc * 60 + parseInt(cur, 10), 0);📝 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.
| .reduce((acc, cur) => (acc = acc * 60 + parseInt(cur, 10)), 0); | |
| .reduce((acc, cur) => acc * 60 + parseInt(cur, 10), 0); |
🧰 Tools
🪛 Biome
[error] 24-24: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
src/app/(core)/v/[slug]/player.tsx
Outdated
| const parsedMarkers = useMemo(() => { | ||
| if (!props.vod) return; | ||
|
|
||
| if (endTime < 0) endTime = 1; | ||
|
|
||
| const startTime = Math.max( | ||
| marker.position_seconds - offset.totalSeconds - EXPORT_BUFFER, | ||
| 0 | ||
| ); | ||
|
|
||
| if (marker.description.toLowerCase().startsWith("end of")) return []; | ||
|
|
||
| return [`${startTime},${endTime},${marker.description.replace(",", "")}`]; | ||
| }); | ||
|
|
||
| const ytChapters = mockedMarkers.reduce((acc, marker) => { | ||
| const startTime = new Date( | ||
| (marker.position_seconds - offset.totalSeconds) * 1000 | ||
| ); | ||
| const timeStr = startTime.toISOString().substr(11, 8); | ||
| return `${acc}${marker.description} - ${timeStr}\n`; | ||
| }, ""); | ||
| return parseMarkers({ | ||
| vod: props.vod, | ||
| offset: offset, | ||
| }); | ||
| }, [props, offset]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize dependencies in useMemo hook
Including props in the dependency array can cause unnecessary recomputations. It's better to specify the exact properties that affect the memoization.
Apply this diff to adjust the dependencies:
- }, [props, offset]);
+ }, [props.vod, offset]);📝 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 parsedMarkers = useMemo(() => { | |
| if (!props.vod) return; | |
| if (endTime < 0) endTime = 1; | |
| const startTime = Math.max( | |
| marker.position_seconds - offset.totalSeconds - EXPORT_BUFFER, | |
| 0 | |
| ); | |
| if (marker.description.toLowerCase().startsWith("end of")) return []; | |
| return [`${startTime},${endTime},${marker.description.replace(",", "")}`]; | |
| }); | |
| const ytChapters = mockedMarkers.reduce((acc, marker) => { | |
| const startTime = new Date( | |
| (marker.position_seconds - offset.totalSeconds) * 1000 | |
| ); | |
| const timeStr = startTime.toISOString().substr(11, 8); | |
| return `${acc}${marker.description} - ${timeStr}\n`; | |
| }, ""); | |
| return parseMarkers({ | |
| vod: props.vod, | |
| offset: offset, | |
| }); | |
| }, [props, offset]); | |
| const parsedMarkers = useMemo(() => { | |
| if (!props.vod) return; | |
| return parseMarkers({ | |
| vod: props.vod, | |
| offset: offset, | |
| }); | |
| }, [props.vod, offset]); |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/app/(core)/v/[slug]/player.tsx (2)
39-71: Effective marker categorization with room for optimizationThe
parseMetadataFromMarkerfunction successfully categorizes markers into types (start, end, offset) based on their labels. The implementation is clear and easy to understand.Consider optimizing the function by using a single loop instead of multiple loops. Here's a suggested refactor:
function parseMetadataFromMarker(marker: string) { const lowerMarker = marker.toLowerCase(); for (const [type, labels] of Object.entries({ start: START_LABELS, end: END_LABELS, offset: OFFSET_LABELS, })) { for (const label of labels) { if (lowerMarker.startsWith(label.toLowerCase())) { return { type, label: type === 'offset' ? marker.slice(label.length).trim().replaceAll("-", ":") : marker.slice(label.length).trim(), }; } } } return { type: "start", label: marker }; }This refactored version reduces code duplication and improves maintainability.
Line range hint
216-345: Effective integration of new parsing logic with UI improvementsThe VodPlayer component has been successfully updated to use the new
parseMarkersfunction and render more detailed marker information. The CSV download functionality has been appropriately updated.Consider refactoring the nested ternary operator for
classNamedetermination to improve readability. Here's a suggested approach:const typeClassMap = { start: "bg-green-800 text-white", end: "bg-red-800 text-white", offset: "bg-blue-800 text-white", default: "bg-gray-800 text-white", }; // In the JSX className={`-m-2 rounded-full px-2 py-1 text-sm font-bold ${ typeClassMap[marker.type] || typeClassMap.default }`}This change will make the code more maintainable and easier to read.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/app/(core)/v/[slug]/player.tsx (4 hunks)
🧰 Additional context used
🪛 Biome
src/app/(core)/v/[slug]/player.tsx
[error] 24-24: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (1)
src/app/(core)/v/[slug]/player.tsx (1)
Line range hint
1-359: Overall assessment: Significant improvements with minor refinements neededThe changes to the VodPlayer component and its supporting functions greatly enhance the functionality of marker parsing and display. The new functions
parseOffsetValue,parseMetadataFromMarker, andparseMarkersprovide a robust foundation for handling various marker formats and generating useful data.Key improvements:
- Enhanced time format parsing
- Structured marker categorization
- Comprehensive marker processing with YouTube chapter and CSV generation
- Improved UI rendering of marker details
To further refine the implementation, consider addressing the following:
- Optimize the
parseMetadataFromMarkerfunction- Fix the YouTube chapters format
- Improve type safety by avoiding
anytypes- Refactor the nested ternary operator for better readability
These changes will result in a more maintainable, efficient, and type-safe implementation.
🧰 Tools
🪛 Biome
[error] 24-24: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
| function parseMarkers(props: { vod: VOD; offset?: { totalSeconds: number } }) { | ||
| const videoDuration = getDurationFromTwitchFormat( | ||
| (props.vod as any)?.duration ?? "0h0m0s" | ||
| ); | ||
|
|
||
| const mockedMarkers = [ | ||
| { position_seconds: 0, id: "start", description: "Intro" }, | ||
| ...props.vod.markers, | ||
| ]; | ||
|
|
||
| const OFFSET = props.offset?.totalSeconds ?? 0; | ||
|
|
||
| const taggedMarkers = mockedMarkers.map((marker, id) => { | ||
| let endTime = | ||
| (mockedMarkers[id + 1]?.position_seconds ?? | ||
| (videoDuration as duration.Duration)?.asSeconds?.()) - OFFSET; | ||
|
|
||
| endTime += EXPORT_BUFFER; | ||
|
|
||
| if (endTime < 0) endTime = 1; | ||
|
|
||
| const startTime = Math.max( | ||
| marker.position_seconds - OFFSET - EXPORT_BUFFER, | ||
| 0 | ||
| ); | ||
|
|
||
| const duration = dayjs | ||
| .duration(endTime * 1000 - startTime * 1000) | ||
| .format("HH:mm:ss"); | ||
|
|
||
| const taggedDescription = parseMetadataFromMarker(marker.description); | ||
|
|
||
| return { | ||
| startTime, | ||
| endTime, | ||
| duration, | ||
| ...taggedDescription, | ||
| }; | ||
| }); | ||
|
|
||
| const filteredMarkers = taggedMarkers.filter( | ||
| (m) => m.type === "start" || m.type === "offset" | ||
| ); | ||
|
|
||
| const ytChapters = filteredMarkers.reduce((acc, marker) => { | ||
| const timeStr = dayjs | ||
| .duration(marker.startTime * 1000) | ||
| .format("HH:mm:ss"); | ||
| return `${acc}${timeStr} ${marker.label}\n`; | ||
| }, ""); | ||
|
|
||
| const csv = filteredMarkers | ||
| .map((marker) => { | ||
| return `${marker.startTime},${marker.endTime},${marker.label}`; | ||
| }) | ||
| .join("\n"); | ||
|
|
||
| return { | ||
| taggedMarkers, | ||
| filteredMarkers, | ||
| ytChapters, | ||
| csv, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Comprehensive marker processing with minor improvements needed
The parseMarkers function effectively processes video markers, calculates start and end times, and generates YouTube chapter strings and CSV data.
- The YouTube chapters format is incorrect. Apply this diff to fix it:
- return `${acc}${timeStr} ${marker.label}\n`;
+ return `${acc}${marker.startTime} ${marker.label}\n`;- Avoid using
anytype forprops.vod. Consider defining a proper type for the VOD object.
Replace the any type with a proper type definition:
interface VODWithDuration extends VOD {
duration?: string;
}
function parseMarkers(props: { vod: VODWithDuration; offset?: { totalSeconds: number } }) {
// ...
}These changes will improve type safety and ensure correct YouTube chapter formatting.
| function parseOffsetValue(value: string): number | undefined { | ||
| // if there are no colons, assume its seconds | ||
| if (/^\d+$/.test(value)) return parseInt(value, 10); | ||
|
|
||
| // Supports HH:MM:SS, MM:SS, SS | ||
| // If it's not in the format, return undefined | ||
| if (!/^([0-5]?[0-9]:){0,2}[0-5][0-9]$/.test(value)) return undefined; | ||
|
|
||
| return value | ||
| .split(":") | ||
| .reduce((acc, cur) => (acc = acc * 60 + parseInt(cur, 10)), 0); | ||
| } |
There was a problem hiding this comment.
Good implementation of time parsing, with a minor improvement needed
The parseOffsetValue function effectively handles various time formats (seconds, MM:SS, HH:MM:SS) and converts them to total seconds. The use of regex for input validation is appropriate.
However, there's a minor issue in the reduce function. Apply this diff to remove the unnecessary assignment:
- .reduce((acc, cur) => (acc = acc * 60 + parseInt(cur, 10)), 0);
+ .reduce((acc, cur) => acc * 60 + parseInt(cur, 10), 0);This change improves clarity and avoids potential side effects from assignments in expressions.
📝 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.
| function parseOffsetValue(value: string): number | undefined { | |
| // if there are no colons, assume its seconds | |
| if (/^\d+$/.test(value)) return parseInt(value, 10); | |
| // Supports HH:MM:SS, MM:SS, SS | |
| // If it's not in the format, return undefined | |
| if (!/^([0-5]?[0-9]:){0,2}[0-5][0-9]$/.test(value)) return undefined; | |
| return value | |
| .split(":") | |
| .reduce((acc, cur) => (acc = acc * 60 + parseInt(cur, 10)), 0); | |
| } | |
| function parseOffsetValue(value: string): number | undefined { | |
| // if there are no colons, assume its seconds | |
| if (/^\d+$/.test(value)) return parseInt(value, 10); | |
| // Supports HH:MM:SS, MM:SS, SS | |
| // If it's not in the format, return undefined | |
| if (!/^([0-5]?[0-9]:){0,2}[0-5][0-9]$/.test(value)) return undefined; | |
| return value | |
| .split(":") | |
| .reduce((acc, cur) => acc * 60 + parseInt(cur, 10), 0); | |
| } |
🧰 Tools
🪛 Biome
[error] 24-24: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
Summary by CodeRabbit
New Features
Bug Fixes