Added in "clip" feature and updated deps#76
Open
bmdavis419 wants to merge 3 commits intopingdotgg:mainfrom
Open
Added in "clip" feature and updated deps#76bmdavis419 wants to merge 3 commits intopingdotgg:mainfrom
bmdavis419 wants to merge 3 commits intopingdotgg:mainfrom
Conversation
- Add `.env.example` and ignore local env files - Update Next.js, Clerk, TypeScript, Prettier, and related packages - Tweak app layout/player files and VS Code color settings
|
@bmdavis419 is attempting to deploy a commit to the Ping Labs Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a repository env template and ignores real env files, upgrades packages and tooling, tweaks VS Code settings, removes several TypeScript suppression comments, and implements CLIP marker support with clip-specific parsing, timing, UI badge styling, and seek behavior in the VOD player. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
rect rgba(220,240,255,0.5)
participant User
end
rect rgba(200,230,200,0.5)
participant UI as VodPlayer UI
participant Parser as Marker Parser
participant Video as Video Element
end
User->>UI: Click timeline marker (type=start or clip)
UI->>Parser: Resolve marker -> type, startTime, endTime, highlightTime
Parser-->>UI: Return marker metadata
UI->>Video: Seek to highlightTime (or startTime as applicable) and play
Video-->>UI: Playback updates
UI->>User: Render marker badge (type-specific styling)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR introduces a new Key observations:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Marker description string] --> B{parseMetadataFromMarker}
B -->|starts with START:/START OF/START| C[type: start]
B -->|starts with END:/END OF/END| D[type: end]
B -->|starts with CLIP:/CLIP| E[type: clip]
B -->|starts with OFFSET/OFFSET:| F[type: offset]
B -->|no match| C
C --> G[startTime = markerPos − EXPORT_BUFFER]
C --> H[endTime = nextMarkerPos + EXPORT_BUFFER]
E --> I[startTime = markerPos − CLIP_LOOKBACK 600s − EXPORT_BUFFER 10s]
E --> J[endTime = markerPos + EXPORT_BUFFER 10s]
G --> K{filteredMarkers}
I --> K
F --> K
D -->|filtered out| L[Excluded from output]
K --> M[CSV export]
K --> N[YT Chapters ⚠️ uses startTime for all types]
K --> O[Sidebar list]
O --> P{User clicks marker}
P -->|start| Q[seek to startTime ≈ markerPos]
P -->|clip ⚠️| R[seek to startTime = 10 min before markerPos]
P -->|offset| S[update offset state]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+317
to
319
| if (marker.type === "start" || marker.type === "clip") { | ||
| player?.seek(marker.startTime); | ||
| } |
There was a problem hiding this comment.
When a clip marker is clicked, player?.seek(marker.startTime) seeks to markerPosition - 600 - 10 (10 min + 10 s before the actual clip event). This means clicking a clip marker drops the user ~10 minutes before the interesting moment, rather than at the moment itself.
Consider whether seeking to the end of the clip window (the actual highlighted moment) would be more intuitive:
Suggested change
| if (marker.type === "start" || marker.type === "clip") { | |
| player?.seek(marker.startTime); | |
| } | |
| if (marker.type === "start") { | |
| player?.seek(marker.startTime); | |
| } | |
| if (marker.type === "clip") { | |
| // seek to the actual clip moment (endTime - buffer) | |
| player?.seek(marker.endTime - EXPORT_BUFFER); | |
| } |
- Increase the VOD clip lookback from 10 to 15 minutes - Preserves a longer history for clip creation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
.env.example (1)
2-3: Optional: reorder Clerk keys to keep dotenv-linter clean.Line 2 and Line 3 trigger the
UnorderedKeywarning. If lint cleanliness is enforced in CI, swap these two lines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.example around lines 2 - 3, The .env.example triggers dotenv-linter's UnorderedKey warning because the Clerk keys are out of alphabetical order; fix it by swapping the entries so that CLERK_SECRET_KEY comes before NEXT_PUBLIC_CLERK_PUBLISHABLE_KEY (i.e., reorder the two variables CLERK_SECRET_KEY and NEXT_PUBLIC_CLERK_PUBLISHABLE_KEY) to satisfy the linter.package.json (1)
20-34: Move build/type/lint packages todevDependencies.The following packages are currently in
dependenciesbut are only needed at build time and should be moved todevDependencies:
@types/node,@types/react,@types/react-dom(type definitions)typescript,eslint,eslint-config-next(build and lint tooling)tailwindcss,postcss,autoprefixer(CSS processing)None are imported in runtime source code. Keeping them in production dependencies inflates the install size and attack surface unnecessarily.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 20 - 34, The listed build/time-only packages in package.json (specifically "@types/node", "@types/react", "@types/react-dom", "typescript", "eslint", "eslint-config-next", "tailwindcss", "postcss", "autoprefixer") should be moved from "dependencies" into "devDependencies": update package.json by removing these entries from the dependencies object and adding them with the same versions under devDependencies so they are installed only in development/build environments and not shipped at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Line 3: The .gitignore currently only ignores ".env" which misses variants
like ".env.production" or ".env.development"; update the ignore rules to broadly
ignore environment files by adding a pattern such as ".env*" and then explicitly
allow the example file by negating ".env.example" so that real env files are
never committed while ".env.example" remains tracked for documentation; modify
the .gitignore entry that currently contains ".env" accordingly.
In `@src/app/`(core)/v/[slug]/player.tsx:
- Around line 100-107: The clip branch is double-applying EXPORT_BUFFER to the
start window: when taggedDescription.type === "clip" startTime subtracts both
CLIP_LOOKBACK_SECONDS and EXPORT_BUFFER, producing a window larger than the
intended CLIP_LOOKBACK_SECONDS. Change the clip start calculation to use
markerPosition - CLIP_LOOKBACK_SECONDS (clamped to 0) and keep the existing
endTime (markerPosition + EXPORT_BUFFER) so the clip exports the intended
lookback; also update the other occurrence referenced (the similar calculation
around the second occurrence) to the same logic.
- Around line 105-110: The endTime calculation currently stops at the next
mockedMarkers[id+1] even if that marker is a CLIP, which lets clip markers
truncate the previous range; change the logic used when taggedDescription.type
!== "clip" to scan forward from id+1 to find the next non-clip marker (e.g., use
mockedMarkers.slice(id+1).find(...) or a simple loop to locate the next marker
whose taggedDescription.type !== "clip") and use that marker's position_seconds
as the boundary (falling back to (videoDuration as
duration.Duration)?.asSeconds?.() when none found), then apply - OFFSET +
EXPORT_BUFFER as before; update the endTime assignment that references
markerPosition, mockedMarkers, id, EXPORT_BUFFER and OFFSET accordingly.
---
Nitpick comments:
In @.env.example:
- Around line 2-3: The .env.example triggers dotenv-linter's UnorderedKey
warning because the Clerk keys are out of alphabetical order; fix it by swapping
the entries so that CLERK_SECRET_KEY comes before
NEXT_PUBLIC_CLERK_PUBLISHABLE_KEY (i.e., reorder the two variables
CLERK_SECRET_KEY and NEXT_PUBLIC_CLERK_PUBLISHABLE_KEY) to satisfy the linter.
In `@package.json`:
- Around line 20-34: The listed build/time-only packages in package.json
(specifically "@types/node", "@types/react", "@types/react-dom", "typescript",
"eslint", "eslint-config-next", "tailwindcss", "postcss", "autoprefixer") should
be moved from "dependencies" into "devDependencies": update package.json by
removing these entries from the dependencies object and adding them with the
same versions under devDependencies so they are installed only in
development/build environments and not shipped at runtime.
🪄 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: c62c7c52-9ca9-4c23-9de4-989dc021e9c5
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
.env.example.gitignore.vscode/settings.jsonREADME.mdpackage.jsonprettier.config.jssrc/app/(core)/layout.tsxsrc/app/(core)/v/[slug]/player.tsxsrc/app/[slug]/layout.tsxsrc/app/[slug]/page.tsxsrc/app/_components/layout-helper.tsx
💤 Files with no reviewable changes (4)
- src/app/(core)/layout.tsx
- src/app/[slug]/page.tsx
- src/app/[slug]/layout.tsx
- src/app/_components/layout-helper.tsx
src/app/(core)/v/[slug]/player.tsx
Outdated
Comment on lines
+100
to
+107
| let startTime = | ||
| taggedDescription.type === "clip" | ||
| ? Math.max(markerPosition - CLIP_LOOKBACK_SECONDS - EXPORT_BUFFER, 0) | ||
| : Math.max(markerPosition - EXPORT_BUFFER, 0); | ||
|
|
||
| if (endTime < 0) endTime = 1; | ||
| let endTime = | ||
| taggedDescription.type === "clip" | ||
| ? markerPosition + EXPORT_BUFFER |
Contributor
There was a problem hiding this comment.
The clip branch exports 10m20s, not 10m.
CLIP_LOOKBACK_SECONDS already encodes the requested window, but this branch still applies EXPORT_BUFFER on both sides. A CLIP: at 01:00:00 becomes 00:49:50–01:00:10, which overshoots the “previous 10 minutes” behavior in the PR objective.
🛠️ Suggested fix
let startTime =
taggedDescription.type === "clip"
- ? Math.max(markerPosition - CLIP_LOOKBACK_SECONDS - EXPORT_BUFFER, 0)
+ ? Math.max(markerPosition - CLIP_LOOKBACK_SECONDS, 0)
: Math.max(markerPosition - EXPORT_BUFFER, 0);
let endTime =
taggedDescription.type === "clip"
- ? markerPosition + EXPORT_BUFFER
+ ? markerPosition
: (mockedMarkers[id + 1]?.position_seconds ??
(videoDuration as duration.Duration)?.asSeconds?.()) - OFFSET +
EXPORT_BUFFER;Also applies to: 212-212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/`(core)/v/[slug]/player.tsx around lines 100 - 107, The clip branch
is double-applying EXPORT_BUFFER to the start window: when
taggedDescription.type === "clip" startTime subtracts both CLIP_LOOKBACK_SECONDS
and EXPORT_BUFFER, producing a window larger than the intended
CLIP_LOOKBACK_SECONDS. Change the clip start calculation to use markerPosition -
CLIP_LOOKBACK_SECONDS (clamped to 0) and keep the existing endTime
(markerPosition + EXPORT_BUFFER) so the clip exports the intended lookback; also
update the other occurrence referenced (the similar calculation around the
second occurrence) to the same logic.
- Adjust VOD marker boundaries and chapter timestamps to use highlight times - Update player seeks to jump to the visible marker point - Ignore local env files while keeping `.env.example` tracked
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.
.env.exampleand ignore local env filesSummary by CodeRabbit
New Features
Chores