Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughThis PR updates 3Speak video embed handling in the render-helper package by switching embed endpoints from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
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. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/render-helper/src/methods/iframe.method.spec.ts (1)
421-432: Consider asserting/watch?in the "no double-prefix" test after this changeThe test at line 424 feeds
https://play.3speak.tv/embed?v=user/video123— after this PR's change, the URL will be converted to/watch?. The current assertions (toContain('play.3speak.tv'),not.toContain('play.play.3speak.tv')) are still satisfied, but the test no longer distinguishes the old and new behavior for already-correct domains. Addingexpect(src).toContain('/watch?v=')would tighten coverage here.♻️ Suggested assertion addition
const src = el.getAttribute('src')! expect(src).toContain('play.3speak.tv') expect(src).not.toContain('play.play.3speak.tv') + expect(src).toContain('/watch?v=') + expect(src).not.toContain('/embed?')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/render-helper/src/methods/iframe.method.spec.ts` around lines 421 - 432, The test for iframe() that ensures we don't double-prefix play.3speak.tv should also assert the new converted path; update the spec in the test that calls iframe(el) (the case that sets src to 'https://play.3speak.tv/embed?v=user/video123') to include an assertion like expect(src).toContain('/watch?v=') so the test verifies the URL was normalized to the new /watch? form in addition to still containing 'play.3speak.tv' and not containing 'play.play.3speak.tv'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/render-helper/src/methods/a.method.ts`:
- Line 776: The runtime string change in a.method.ts (const videoHref) was
updated to use `watch?v=` but the generated distribution bundles were not
rebuilt; rebuild the package (run the project build script, e.g., npm run build)
so the published dist artifacts are regenerated, verify the built node/browser
bundles contain the new `watch?v=` string (replace any remaining `embed?v=`),
and commit the updated dist files to the PR so consumers installing the package
get the fix; reference the updated symbol videoHref in a.method.ts when
verifying the output.
---
Nitpick comments:
In `@packages/render-helper/src/methods/iframe.method.spec.ts`:
- Around line 421-432: The test for iframe() that ensures we don't double-prefix
play.3speak.tv should also assert the new converted path; update the spec in the
test that calls iframe(el) (the case that sets src to
'https://play.3speak.tv/embed?v=user/video123') to include an assertion like
expect(src).toContain('/watch?v=') so the test verifies the URL was normalized
to the new /watch? form in addition to still containing 'play.3speak.tv' and not
containing 'play.play.3speak.tv'.
| // match[1] / match[2] = tld , match[3] = embed address | ||
| if ((match[1] || match[2]) && match[3]) { | ||
| const videoHref = `https://play.3speak.tv/embed?v=${match[3]}&mode=iframe` | ||
| const videoHref = `https://play.3speak.tv/watch?v=${match[3]}&mode=iframe` |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm dist files reference old embed path and check package.json main/module fields
echo "=== Checking if dist directory exists ==="
ls -la packages/render-helper/dist/ 2>/dev/null || echo "dist directory not found"
echo ""
echo "=== Checking dist files for old embed path ==="
rg -n "embed\?v=" packages/render-helper/dist/ 2>/dev/null || echo "No matches found or dist directory doesn't exist"
echo ""
echo "=== Checking package.json main/module/exports fields ==="
fd "package.json" packages/render-helper --max-depth 1 --exec cat {}
echo ""
echo "=== Checking .gitignore for dist patterns ==="
if [ -f "packages/render-helper/.gitignore" ]; then
cat packages/render-helper/.gitignore | grep -E "dist|build" || echo "No dist/build ignore patterns found"
else
echo "No .gitignore in packages/render-helper"
fi
echo ""
echo "=== Checking root .gitignore for dist patterns ==="
cat .gitignore 2>/dev/null | grep -E "dist|build" || echo "No dist/build ignore patterns in root"Repository: ecency/vision-next
Length of output: 50375
🏁 Script executed:
#!/bin/bash
echo "=== Checking source file line 776 for 'watch?v=' ==="
sed -n '770,780p' packages/render-helper/src/methods/a.method.ts
echo ""
echo "=== Checking package.json for main/module/exports ==="
cat packages/render-helper/package.json | grep -A 10 '"main"\|"module"\|"exports"'Repository: ecency/vision-next
Length of output: 1230
Rebuild and commit dist artifacts before merging
The source code change to use watch?v= is correct, but all three pre-built dist files still contain the old embed?v= endpoint:
dist/node/index.mjsline 905dist/node/index.cjsline 934dist/browser/index.jsline 902
Since package.json explicitly declares these dist files as the published entry points ("main", "module", and "exports"), consumers installing this package from npm will receive the old code and the fix will have no effect. The dist files must be rebuilt and committed as part of this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/render-helper/src/methods/a.method.ts` at line 776, The runtime
string change in a.method.ts (const videoHref) was updated to use `watch?v=` but
the generated distribution bundles were not rebuilt; rebuild the package (run
the project build script, e.g., npm run build) so the published dist artifacts
are regenerated, verify the built node/browser bundles contain the new
`watch?v=` string (replace any remaining `embed?v=`), and commit the updated
dist files to the PR so consumers installing the package get the fix; reference
the updated symbol videoHref in a.method.ts when verifying the output.
feruzm
added
the
patch
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.
Summary by CodeRabbit
Bug Fixes
Tests