Add autoplay support #441
Open
Add autoplay support #441
Conversation
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
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Reviewer's GuideImplements YouTube-tag–based autoplay functionality with playlist management and persistence, wires it into the existing player and UI, and refactors the Puppeteer/Mocha test suite to use top-level hooks, explicit resource cleanup, and new functional tests for autoplay/repeat, along with small style and npm script tweaks. Sequence diagram for autoplay flow on video endsequenceDiagram
actor User
participant PlyrPlayer
participant ZenPlayer
participant AutoplayLogic as Autoplay
participant YouTubeAPI
participant SessionStorage
participant Form as SearchForm
User->>PlyrPlayer: Play current video
PlyrPlayer-->>ZenPlayer: playing event
ZenPlayer->>SessionStorage: loadAutoPlayDetails
Note over User,PlyrPlayer: After video finishes
PlyrPlayer-->>ZenPlayer: ended event
ZenPlayer->>Autoplay: autoplayState check
alt autoplayState is true
Autoplay->>Autoplay: check playList size
alt playList empty
Autoplay->>Autoplay: fetchSuggestedVideoIds
Autoplay->>YouTubeAPI: search by tags (multiple requests)
YouTubeAPI-->>Autoplay: related video items
Autoplay->>Autoplay: onRelatedVideoFetchSuccess
Autoplay->>Autoplay: add videoId to playList (Set)
Autoplay->>SessionStorage: set playList
end
Autoplay->>Autoplay: getNewVideoID
Autoplay->>SessionStorage: update playList
Autoplay-->>ZenPlayer: next videoID
ZenPlayer->>ZenPlayer: playNext(videoID)
ZenPlayer->>SearchForm: set #v value
ZenPlayer->>SearchForm: submit()
SearchForm->>Browser: navigate to new listen URL
else autoplayState is false
ZenPlayer-->>User: Stop after current video
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- The autoplay playlist logic mixes
Setand array semantics (e.g., usingplayList = new Set(), checkingplayList.length, and later doingplayList = Array.from(playList)plusplayList.pop()), which is error‑prone; pick one data structure and update length/size checks, mutation, and serialization consistently. getNewVideoID()can enter a tight loop or returnundefinedif the playlist is empty or all entries matchcurrentVideoID; add explicit guards for an empty playlist and a maximum number of retries/fallback behavior when no suitable next ID exists.- When restoring
autoplayStatefromsessionStorage,window.sessionStorage.getItem("autoPlay")returns a string, so consider normalizing it back to a boolean (e.g.,autoplayState = window.sessionStorage.getItem("autoPlay") === "true") to avoid type inconsistencies in later checks.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The autoplay playlist logic mixes `Set` and array semantics (e.g., using `playList = new Set()`, checking `playList.length`, and later doing `playList = Array.from(playList)` plus `playList.pop()`), which is error‑prone; pick one data structure and update length/size checks, mutation, and serialization consistently.
- `getNewVideoID()` can enter a tight loop or return `undefined` if the playlist is empty or all entries match `currentVideoID`; add explicit guards for an empty playlist and a maximum number of retries/fallback behavior when no suitable next ID exists.
- When restoring `autoplayState` from `sessionStorage`, `window.sessionStorage.getItem("autoPlay")` returns a string, so consider normalizing it back to a boolean (e.g., `autoplayState = window.sessionStorage.getItem("autoPlay") === "true"`) to avoid type inconsistencies in later checks.
## Individual Comments
### Comment 1
<location> `js/everything.js:21-27` </location>
<code_context>
+// global playlist, this is populated with an ajax call
+var tags = [];
+var playList = new Set();
+var autoplayState = false;
+const MAX_TAGS = 10;
</code_context>
<issue_to_address>
**issue (bug_risk):** playList switches between Set and Array semantics, which will break methods like .add() and .pop().
playList starts as a Set and is used with .add(), but getNewVideoID calls .pop() (Array-only), and fetchSuggestedVideoIds does `playList = Array.from(playList)`, changing it to an Array. After that, any .add() call will throw. Use a single, consistent representation (e.g., always a Set and iterate to get the next video, or always an Array and use push/pop) and update the callers accordingly.
</issue_to_address>
### Comment 2
<location> `js/everything.js:586-589` </location>
<code_context>
+ }
+}
+
+function getNewVideoID() {
+ var nextID = null;
+ nextID = playList.pop();
+ while (currentVideoID === nextID) {
+ nextID = playList.pop();
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** getNewVideoID can loop indefinitely if the playlist only contains the current video ID.
Since `playList.pop()` returns `undefined` when the array is empty and `while (currentVideoID === nextID)` has no empty-guard, this can spin until the playlist is exhausted and then loop on `undefined` if `currentVideoID` is also `undefined`. Add a check to break when the playlist is empty (and return null/undefined), and have the caller handle the “no next video” case before calling `playNext`.
</issue_to_address>
### Comment 3
<location> `js/everything.js:179` </location>
<code_context>
+ // when player has finished playing
+ plyrPlayer.addEventListener("ended", function() {
+ if (autoplayState) {
+ if (playList.length === 0 || playList.size === 0) {
+ fetchSuggestedVideoIds();
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Checking both playList.length and playList.size suggests uncertainty about the data structure and may mask bugs.
Supporting both Array (.length) and Set (.size) in the same condition can conceal type mismatches and make future bugs harder to spot. Once playList has a single, well-defined type, this should be reduced to one emptiness check on the appropriate property to keep the logic clear and reliable.
Suggested implementation:
```javascript
// when player has finished playing
plyrPlayer.addEventListener("ended", function() {
if (autoplayState) {
// playList is expected to be an Array
if (playList.length === 0) {
fetchSuggestedVideoIds();
}
var newId = getNewVideoID();
that.playNext(newId);
}
});
```
To fully support this change, ensure that:
1. `playList` is consistently treated as an `Array` throughout `js/everything.js` (and the rest of the codebase), including its initialization (e.g. `var playList = [];`) and all mutations (push/splice/etc.).
2. Any existing uses of `playList.size` elsewhere in the code are removed or updated to use `playList.length`, so there is a single, well-defined type and emptiness check for `playList`.
</issue_to_address>
### Comment 4
<location> `js/everything.js:602-605` </location>
<code_context>
+ // get similar videos, populate playList
+ if (!isFileProtocol()) {
+ for (let index = 0; index < tags.length && index < MAX_TAGS; index++) {
+ $.ajax({
+ url: "https://www.googleapis.com/youtube/v3/search",
+ dataType: "json",
+ async: false,
+ data: {
+ key: youTubeDataApiKey,
</code_context>
<issue_to_address>
**suggestion (performance):** Synchronous AJAX in a loop can block the UI and significantly hurt UX.
`fetchSuggestedVideoIds` performs multiple synchronous (`async: false`) XHR calls in a loop, which will freeze the tab until all requests complete, especially noticeable on slow networks. Please switch to async requests and either fire them in parallel and update `playList` as each response arrives, or use Promises/Deferreds to preserve sequential behavior if required.
</issue_to_address>
### Comment 5
<location> `js/everything.js:641-642` </location>
<code_context>
+
+ // fetch autoPlay from session storage on reload
+ if (window.sessionStorage.getItem("autoPlay")) {
+ autoplayState = window.sessionStorage.getItem("autoPlay");
+ updateAutoplayToggle(autoplayState);
+ }
+}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** autoPlay state is persisted as a string, which may lead to subtle type inconsistencies.
Because `sessionStorage.getItem` always returns a string, `autoplayState` changes from boolean to string depending on the path. This is currently truthy-safe but will break with strict comparisons or JSON usage. Coerce explicitly, e.g. `autoplayState = window.sessionStorage.getItem("autoPlay") === "true";` to keep the type consistently boolean.
Suggested implementation:
```javascript
// fetch autoPlay from session storage on reload
if (window.sessionStorage.getItem("autoPlay")) {
autoplayState = window.sessionStorage.getItem("autoPlay") === "true";
updateAutoplayToggle(autoplayState);
}
```
This assumes `autoplayState` and `updateAutoplayToggle` already exist in the surrounding scope. If they don't, you should:
1. Ensure `autoplayState` is declared (e.g. `let autoplayState = false;`) in the appropriate scope where other player state is managed.
2. Confirm `updateAutoplayToggle` accepts a boolean parameter; if it currently expects a string, adjust its implementation to treat the argument as a boolean.
</issue_to_address>
### Comment 6
<location> `test/autoplay-repeat.js:397` </location>
<code_context>
+ // For autoplay test, we just verify that toggle functionality works
+ // The actual autoplay functionality may not work due to YouTube API limitations
+ // but we can verify the video setup and toggle functionality works
+ assert.ok(true, "Autoplay test completed - video setup and toggle functionality verified");
+
+ // TODO: add a real assertion here, maybe video title change, etc.
</code_context>
<issue_to_address>
**issue (testing):** Autoplay test does not assert any actual autoplay behavior
Right now this only confirms the toggles switch; it never checks that a new video actually loads after the first one ends, so autoplay regressions would slip through. Please add at least one concrete assertion of autoplay behavior (e.g., wait for the first video to finish and assert a change in `#zen-video-title`, the `v` query param, or `currentVideoID`). If that’s too flaky, asserting that a playlist is populated and `playNext` is invoked would still provide stronger coverage than `assert.ok(true)`.
</issue_to_address>
### Comment 7
<location> `test/autoplay-repeat.js:228` </location>
<code_context>
+ assert.equal(finalState.displayedTitle, initialTitle,
+ "Video title should remain unchanged");
+
+ // Current time should be 0 (or very close to it) after video completes
+ assert.ok(finalState.currentTime < 1,
+ `Current time should be 0 after video completes. Current time: ${finalState.currentTime}s`);
</code_context>
<issue_to_address>
**issue (testing):** Test comment contradicts the earlier test description for end-of-track behavior
The JSDoc for this test says the current time should be at (or near) the video duration, while this assertion and inline comment expect it to be ~0. This contradiction makes the test’s intent unclear. Please update either the assertion to keep the player near the end, or revise the JSDoc to state that the player should reset to 0 after completion, so both description and expectation match.
</issue_to_address>
### Comment 8
<location> `js/everything.js:3-6` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 9
<location> `js/everything.js:20` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 10
<location> `js/everything.js:21` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 11
<location> `js/everything.js:22` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 12
<location> `js/everything.js:144-150` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 13
<location> `js/everything.js:182` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 14
<location> `js/everything.js:288` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 15
<location> `js/everything.js:290` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 16
<location> `js/everything.js:575` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 17
<location> `js/everything.js:587` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 18
<location> `js/everything.js:597-623` </location>
<code_context>
if (playList.length === 0 || playList.size === 0) {
if (tags.length) {
// get similar videos, populate playList
if (!isFileProtocol()) {
for (let index = 0; index < tags.length && index < MAX_TAGS; index++) {
$.ajax({
url: "https://www.googleapis.com/youtube/v3/search",
dataType: "json",
async: false,
data: {
key: youTubeDataApiKey,
part: "snippet",
type: "video",
order: "relevance",
q: tags[index],
maxResults: 2
},
success: onRelatedVideoFetchSuccess
}).fail(function(jqXHR, textStatus, errorThrown) {
logError(jqXHR, textStatus, errorThrown, "Related video lookup error");
});
}
playList = Array.from(playList);
window.sessionStorage.setItem("playList", JSON.stringify(playList));
}
}
}
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge nested if conditions ([`merge-nested-ifs`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/merge-nested-ifs))
```suggestion
if ((playList.length === 0 || playList.size === 0) && tags.length) {
if (!isFileProtocol()) {
for (let index = 0; index < tags.length && index < MAX_TAGS; index++) {
$.ajax({
url: "https://www.googleapis.com/youtube/v3/search",
dataType: "json",
async: false,
data: {
key: youTubeDataApiKey,
part: "snippet",
type: "video",
order: "relevance",
q: tags[index],
maxResults: 2
},
success: onRelatedVideoFetchSuccess
}).fail(function(jqXHR, textStatus, errorThrown) {
logError(jqXHR, textStatus, errorThrown, "Related video lookup error");
});
}
playList = Array.from(playList);
window.sessionStorage.setItem("playList", JSON.stringify(playList));
}
}
```
<br/><details><summary>Explanation</summary>Reading deeply nested conditional code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two `if` conditions can be combined using
`and` is an easy win.
</details>
</issue_to_address>
### Comment 19
<location> `js/everything.js:598-622` </location>
<code_context>
if (tags.length) {
// get similar videos, populate playList
if (!isFileProtocol()) {
for (let index = 0; index < tags.length && index < MAX_TAGS; index++) {
$.ajax({
url: "https://www.googleapis.com/youtube/v3/search",
dataType: "json",
async: false,
data: {
key: youTubeDataApiKey,
part: "snippet",
type: "video",
order: "relevance",
q: tags[index],
maxResults: 2
},
success: onRelatedVideoFetchSuccess
}).fail(function(jqXHR, textStatus, errorThrown) {
logError(jqXHR, textStatus, errorThrown, "Related video lookup error");
});
}
playList = Array.from(playList);
window.sessionStorage.setItem("playList", JSON.stringify(playList));
}
}
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge nested if conditions ([`merge-nested-ifs`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/merge-nested-ifs))
```suggestion
if (tags.length && !isFileProtocol()) {
for (let index = 0; index < tags.length && index < MAX_TAGS; index++) {
$.ajax({
url: "https://www.googleapis.com/youtube/v3/search",
dataType: "json",
async: false,
data: {
key: youTubeDataApiKey,
part: "snippet",
type: "video",
order: "relevance",
q: tags[index],
maxResults: 2
},
success: onRelatedVideoFetchSuccess
}).fail(function(jqXHR, textStatus, errorThrown) {
logError(jqXHR, textStatus, errorThrown, "Related video lookup error");
});
}
playList = Array.from(playList);
window.sessionStorage.setItem("playList", JSON.stringify(playList));
}
```
<br/><details><summary>Explanation</summary>Reading deeply nested conditional code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two `if` conditions can be combined using
`and` is an easy win.
</details>
</issue_to_address>
### Comment 20
<location> `js/everything.js:628` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 21
<location> `test/autoplay-repeat.js:54` </location>
<code_context>
const embed = player.plyr.embed;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
const {embed} = player.plyr;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 22
<location> `test/autoplay-repeat.js:192` </location>
<code_context>
const duration = initialState.duration;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
const {duration} = initialState;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 23
<location> `test/autoplay-repeat.js:287` </location>
<code_context>
const duration = initialState.duration;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
const {duration} = initialState;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 24
<location> `test/autoplay-repeat.js:343` </location>
<code_context>
const currentTime = finalState.currentTime;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
const {currentTime} = finalState;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+21
to
27
| var playList = new Set(); | ||
| var autoplayState = false; | ||
| const MAX_TAGS = 10; | ||
|
|
||
| var errorMessage = { | ||
| init: function() { | ||
| // nothing for now |
Contributor
There was a problem hiding this comment.
issue (bug_risk): playList switches between Set and Array semantics, which will break methods like .add() and .pop().
playList starts as a Set and is used with .add(), but getNewVideoID calls .pop() (Array-only), and fetchSuggestedVideoIds does playList = Array.from(playList), changing it to an Array. After that, any .add() call will throw. Use a single, consistent representation (e.g., always a Set and iterate to get the next video, or always an Array and use push/pop) and update the callers accordingly.
Comment on lines
+586
to
+589
| function getNewVideoID() { | ||
| var nextID = null; | ||
| nextID = playList.pop(); | ||
| while (currentVideoID === nextID) { |
Contributor
There was a problem hiding this comment.
issue (bug_risk): getNewVideoID can loop indefinitely if the playlist only contains the current video ID.
Since playList.pop() returns undefined when the array is empty and while (currentVideoID === nextID) has no empty-guard, this can spin until the playlist is exhausted and then loop on undefined if currentVideoID is also undefined. Add a check to break when the playlist is empty (and return null/undefined), and have the caller handle the “no next video” case before calling playNext.
| // when player has finished playing | ||
| plyrPlayer.addEventListener("ended", function() { | ||
| if (autoplayState) { | ||
| if (playList.length === 0 || playList.size === 0) { |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Checking both playList.length and playList.size suggests uncertainty about the data structure and may mask bugs.
Supporting both Array (.length) and Set (.size) in the same condition can conceal type mismatches and make future bugs harder to spot. Once playList has a single, well-defined type, this should be reduced to one emptiness check on the appropriate property to keep the logic clear and reliable.
Suggested implementation:
// when player has finished playing
plyrPlayer.addEventListener("ended", function() {
if (autoplayState) {
// playList is expected to be an Array
if (playList.length === 0) {
fetchSuggestedVideoIds();
}
var newId = getNewVideoID();
that.playNext(newId);
}
});To fully support this change, ensure that:
playListis consistently treated as anArraythroughoutjs/everything.js(and the rest of the codebase), including its initialization (e.g.var playList = [];) and all mutations (push/splice/etc.).- Any existing uses of
playList.sizeelsewhere in the code are removed or updated to useplayList.length, so there is a single, well-defined type and emptiness check forplayList.
Comment on lines
+602
to
+605
| $.ajax({ | ||
| url: "https://www.googleapis.com/youtube/v3/search", | ||
| dataType: "json", | ||
| async: false, |
Contributor
There was a problem hiding this comment.
suggestion (performance): Synchronous AJAX in a loop can block the UI and significantly hurt UX.
fetchSuggestedVideoIds performs multiple synchronous (async: false) XHR calls in a loop, which will freeze the tab until all requests complete, especially noticeable on slow networks. Please switch to async requests and either fire them in parallel and update playList as each response arrives, or use Promises/Deferreds to preserve sequential behavior if required.
Comment on lines
+641
to
+642
| autoplayState = window.sessionStorage.getItem("autoPlay"); | ||
| updateAutoplayToggle(autoplayState); |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): autoPlay state is persisted as a string, which may lead to subtle type inconsistencies.
Because sessionStorage.getItem always returns a string, autoplayState changes from boolean to string depending on the path. This is currently truthy-safe but will break with strict comparisons or JSON usage. Coerce explicitly, e.g. autoplayState = window.sessionStorage.getItem("autoPlay") === "true"; to keep the type consistently boolean.
Suggested implementation:
// fetch autoPlay from session storage on reload
if (window.sessionStorage.getItem("autoPlay")) {
autoplayState = window.sessionStorage.getItem("autoPlay") === "true";
updateAutoplayToggle(autoplayState);
}This assumes autoplayState and updateAutoplayToggle already exist in the surrounding scope. If they don't, you should:
- Ensure
autoplayStateis declared (e.g.let autoplayState = false;) in the appropriate scope where other player state is managed. - Confirm
updateAutoplayToggleaccepts a boolean parameter; if it currently expects a string, adjust its implementation to treat the argument as a boolean.
|
|
||
| function onRelatedVideoFetchSuccess(data) { | ||
| // push items into playlist | ||
| for (var i = 0; i < data.items.length; i++) { |
Contributor
There was a problem hiding this comment.
issue (code-quality): Use const or let instead of var. (avoid-using-var)
Explanation
`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code). `let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than function-scoped.From the Airbnb JavaScript Style Guide
Copilot finished reviewing on behalf of
shakeelmohamed
December 5, 2025 10:46
There was a problem hiding this comment.
Pull request overview
This PR adds autoplay functionality to the Zen Audio Player, allowing videos to automatically play next suggested videos when the current one ends. The implementation includes UI controls, session storage persistence, and comprehensive test coverage. The PR also refactors the test suite structure from using immediately-invoked function expressions (IIFE) to standard Mocha before/after hooks.
Key changes:
- Implements autoplay feature with toggle button and YouTube API integration for fetching suggested videos based on video tags
- Adds comprehensive functional tests for autoplay and repeat features using Puppeteer
- Refactors all existing test files to use top-level before/after hooks and adds proper cleanup with page.close() calls
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| test/autoplay-repeat.js | New test file with three test cases for autoplay/repeat functionality, including helper functions for video state management and toggle button interaction |
| test/page_structure.js | Refactored from IIFE to standard Mocha hooks, added page.close() calls and timeout configuration for better resource management |
| test/javascript_components.js | Refactored test structure and added page cleanup to prevent memory leaks |
| test/form.js | Refactored test structure and improved cleanup logic |
| test/demo.js | Refactored test structure and improved server/browser cleanup with existence checks |
| js/everything.js | Core autoplay implementation including playlist management, YouTube API integration, session storage persistence, and event handlers |
| js/zap-common.js | Removed fields parameter from YouTube API call to fetch complete snippet data including tags |
| index.html | Added autoplay toggle button to the player controls |
| css/styles.css | Added styling for autoplay toggle button (active/hover states) and increased container width |
| package.json | Added "fest" convenience script for running fix and test together |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $.ajax({ | ||
| url: "https://www.googleapis.com/youtube/v3/search", | ||
| dataType: "json", | ||
| async: false, |
There was a problem hiding this comment.
Using async: false makes synchronous AJAX requests, which is deprecated and blocks the browser's main thread. This can cause the entire page to freeze while waiting for API responses. Consider refactoring to use async requests with proper promise handling or async/await.
|
|
||
| // Current time should be reset to near 0 (between 0:00 and some small value) | ||
| const currentTime = finalState.currentTime; | ||
| assert.ok(currentTime >= 0 && currentTime < duration / 2, |
There was a problem hiding this comment.
The assertion message states "Current time (${currentTime}) should be near 0 (between 0 and 5 seconds)", but the actual assertion checks currentTime < duration / 2, which could be much larger than 5 seconds for long videos. The assertion and message don't match. Either update the assertion to currentTime < 5 or update the message to accurately reflect what's being checked.
Suggested change
| assert.ok(currentTime >= 0 && currentTime < duration / 2, | |
| assert.ok(currentTime >= 0 && currentTime < 5, |
| } | ||
| else { | ||
| description = data.items[0].snippet.description; | ||
| tags = data.items[0].snippet.tags; |
There was a problem hiding this comment.
The tags array is retrieved from data.items[0].snippet.tags without checking if it exists. If a video doesn't have tags (which is common), this will set tags to undefined, causing a runtime error at line 598 when checking tags.length. Add a null/undefined check: tags = data.items[0].snippet.tags || [].
Suggested change
| tags = data.items[0].snippet.tags; | |
| tags = data.items[0].snippet.tags || []; |
| while (currentVideoID === nextID) { | ||
| nextID = playList.pop(); | ||
| } | ||
| window.sessionStorage.setItem("playList", JSON.stringify(playList)); |
There was a problem hiding this comment.
After calling pop() on the playlist (which has issues as discussed elsewhere), attempting to serialize it to sessionStorage will fail if playList is a Set, because JSON.stringify() on a Set produces {}. This should serialize the Array version or use Array.from(playList) before stringifying. However, this entire data structure needs to be consistently either a Set or an Array throughout the codebase.
Suggested change
| window.sessionStorage.setItem("playList", JSON.stringify(playList)); | |
| window.sessionStorage.setItem("playList", JSON.stringify(Array.from(playList))); |
| // when player has finished playing | ||
| plyrPlayer.addEventListener("ended", function() { | ||
| if (autoplayState) { | ||
| if (playList.length === 0 || playList.size === 0) { |
There was a problem hiding this comment.
The condition checks playList.length === 0 but playList is a Set (line 21), which doesn't have a length property. Use playList.size === 0 instead. This same issue appears in the main code at line 597.
| if (tags.length) { | ||
| // get similar videos, populate playList | ||
| if (!isFileProtocol()) { | ||
| for (let index = 0; index < tags.length && index < MAX_TAGS; index++) { |
There was a problem hiding this comment.
The condition iterates up to MAX_TAGS (10) or tags.length, but the YouTube API is called with maxResults: 2 for each tag. This could result in up to 20 API calls in the loop, which may hit rate limits and cause performance issues. Consider reducing MAX_TAGS, increasing maxResults per call, or implementing a more efficient batching strategy.
Comment on lines
+82
to
+85
| await page.click(selector); | ||
| await page.waitForTimeout(waitTime); | ||
| // Wait for any navigation to complete | ||
| await page.waitForNavigation({ waitUntil: "networkidle0", timeout: 5000 }).catch(() => {}); |
There was a problem hiding this comment.
The waitForNavigation is called with catch(() => {}) which silently swallows navigation completion. However, this is called AFTER the waitForTimeout, meaning the navigation has likely already completed. The waitForNavigation should be awaited in parallel with the click action using Promise.all(), not sequentially after a timeout. Consider refactoring to: await Promise.all([page.click(selector), page.waitForNavigation({ waitUntil: "networkidle0", timeout: waitTime }).catch(() => {})]) for more reliable navigation handling.
Comment on lines
95
to
96
| <button class="btn" id="toggleRepeat"> | ||
| Repeat Track |
There was a problem hiding this comment.
[nitpick] The button text "Repeat Track" is inconsistent with "Autoplay" which doesn't have additional descriptive text. Consider shortening to just "Repeat" for consistency and to save horizontal space, especially on mobile devices.
|
|
||
| // How do we know if the value is truly invalid? | ||
| // Preload the form from the URL | ||
| var currentVideoID = getCurrentVideoID(); |
There was a problem hiding this comment.
This creates a local currentVideoID variable that shadows the global currentVideoID declared at line 17. The global variable is used in other functions like getNewVideoID() at line 589, but it won't be updated by assignments to this local variable. Remove the var keyword to assign to the global variable instead: currentVideoID = getCurrentVideoID();
Comment on lines
+589
to
+591
| while (currentVideoID === nextID) { | ||
| nextID = playList.pop(); | ||
| } |
There was a problem hiding this comment.
The while loop could become infinite if playList only contains one item that matches currentVideoID, or if playList becomes empty during iteration. After calling pop() in line 588, the loop should check if nextID is undefined (which happens when the Set/Array is empty) before continuing. Add a check: while (currentVideoID === nextID && nextID !== undefined) or break if playList is empty.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR bring in autoplay functionality so kindly contributed by @PraveenRaj1833 in #428 over 2 years ago!
I finally had some time to try it out, clean up some inconsistent code patterns and the start of some functional tests!
There are some odd UI issues right now with our very old version of the plyr library. I’ll make another PR to address that upgrade.
Summary by Sourcery
Add autoplay controls and playlist handling to the audio player and expand the end-to-end test suite.
New Features:
Enhancements:
Build:
Tests: