feat: add 'Require Watching' option to YouTube video settings and enh… - #5218
feat: add 'Require Watching' option to YouTube video settings and enh…#5218WillemBarendKruger wants to merge 4 commits into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
WalkthroughThe YouTube component now tracks playback start and completion separately, exposes both states through its public API, always enables the YouTube JS API, and applies generalized watch gating. Settings add a start-watching requirement, clarify completion behavior, and update title-level labels. ChangesYouTube watch-state API and gating
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Form
participant YoutubeVideoComponent
participant YouTubePlayer
Form->>YoutubeVideoComponent: configure watch requirements
YoutubeVideoComponent->>YouTubePlayer: initialize embed with JS API
YouTubePlayer->>YoutubeVideoComponent: report PLAYING
YoutubeVideoComponent->>Form: expose started watch state
YouTubePlayer->>YoutubeVideoComponent: report ENDED
YoutubeVideoComponent->>Form: expose completed watch state
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shesha-reactjs/src/designer-components/youtubeVideo/index.tsx`:
- Around line 354-356: Update the satisfaction logic near satisfied so thumbnail
activation does not count as watching: introduce or reuse a state/value that is
set only when the player reports PLAYING, and use that playback-only state for
the non-completion branch instead of hasWatched. Preserve value === true and the
stricter watchCompletionRequired/isCompleted behavior.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: caafee2a-3809-4f77-9ea5-daa17d010b8e
📒 Files selected for processing (2)
shesha-reactjs/src/designer-components/youtubeVideo/index.tsxshesha-reactjs/src/designer-components/youtubeVideo/settingsForm.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
shesha-reactjs/src/designer-components/youtubeVideo/index.tsx (2)
78-81: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle YouTube API rejection before enabling required watch gates.
When
loadYouTubeIframeApi()rejects withenablejsapi=1, the embed still plays buthasStarted/completion state are never updated, soisRequiredorwatchCompletionRequiredcan leave the submission gate permanently unsatisfied. Surface a tracking-unavailable error or disable the gate before allowing submit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shesha-reactjs/src/designer-components/youtubeVideo/index.tsx` around lines 78 - 81, Update the YouTube API initialization flow around loadYouTubeIframeApi and the needsJsApi/watch-gate logic so a rejected enablejsapi=1 request is handled before enabling isRequired or watchCompletionRequired. Surface a tracking-unavailable error or disable the affected gate, ensuring submission cannot remain permanently blocked when player state cannot be tracked.
113-130: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not strip CSS units or convert pixels to percentages.
toPercentage()turns560into560%,"500px"into"500%", and"10rem"into"10%", while"calc(...)"values becomeundefinedand fall back to100%. Keep the original CSS value as-is for responsive width, or convert pixel dimensions only on a real parent-width basis rather than dropping the unit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shesha-reactjs/src/designer-components/youtubeVideo/index.tsx` around lines 113 - 130, Update toPercentage so responsive width preserves the original CSS value and units instead of converting numeric prefixes to percentages; retain valid values such as numbers, px, rem, %, and calc(...) without stripping units, and avoid falling back to 100% for supported CSS expressions. If pixel-to-percentage conversion is required, base it on the actual parent width rather than the raw numeric value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shesha-reactjs/src/designer-components/youtubeVideo/index.tsx`:
- Around line 64-68: Reset hasStarted, isCompleted, and hasWatched whenever the
watched video identity (videoId or thumbnail) changes, and invalidate the bound
value so satisfied/value cannot remain true for the newly selected video. Update
the relevant effect or video-change handling around these state variables and
the value binding, while preserving normal state behavior when the video
identity is unchanged.
---
Outside diff comments:
In `@shesha-reactjs/src/designer-components/youtubeVideo/index.tsx`:
- Around line 78-81: Update the YouTube API initialization flow around
loadYouTubeIframeApi and the needsJsApi/watch-gate logic so a rejected
enablejsapi=1 request is handled before enabling isRequired or
watchCompletionRequired. Surface a tracking-unavailable error or disable the
affected gate, ensuring submission cannot remain permanently blocked when player
state cannot be tracked.
- Around line 113-130: Update toPercentage so responsive width preserves the
original CSS value and units instead of converting numeric prefixes to
percentages; retain valid values such as numbers, px, rem, %, and calc(...)
without stripping units, and avoid falling back to 100% for supported CSS
expressions. If pixel-to-percentage conversion is required, base it on the
actual parent width rather than the raw numeric value.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: fa4afd2c-abde-4776-89eb-103d128f11e3
📒 Files selected for processing (1)
shesha-reactjs/src/designer-components/youtubeVideo/index.tsx
IvanIlyichev
left a comment
IvanIlyichev
left a comment
There was a problem hiding this comment.
Hi @WillemBarendKruger. Addressing the isRequired/watchCompletionRequired requirement through direct binding to a form field is architecturally incorrect.
There are 3 correct ways to cover this requirement:
- Expose isWatched/isWatchedEntirely on the component API. This allows it to be handled within the final form's configuration via form events (e.g., OnBeforeSubmit).
- Create custom events (OnWatch/OnWatchedEntirely). This allows it to be handled within the final form's configuration with explicit binding to a form field or context.
- Implement custom component validation without bindings (though this may require changes to the base form functionality).
…to track watch state
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shesha-reactjs/src/designer-components/youtubeVideo/index.tsx (1)
127-164: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not relabel pixel dimensions as percentages.
500,500px, and similar widths become500%, causing severe overflow. Preserve the configured CSS width and use100%only when it is absent.Proposed fix
- const toPercentage = (value: string | number | undefined): string | undefined => { - ... - }; - const finalDimensionStyles = responsive ? { ...dimensionStyles, - width: toPercentage(dimensionStyles.width) ?? '100%', + width: dimensionStyles.width ?? '100%', height: undefined,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shesha-reactjs/src/designer-components/youtubeVideo/index.tsx` around lines 127 - 164, Update the responsive branch of the dimension resolution around finalDimensionStyles so configured widths retain their original CSS value, including numeric values and pixel strings, instead of passing through toPercentage. Use 100% only when dimensionStyles.width is absent, while preserving the existing height reset and fixed-dimension behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shesha-reactjs/src/designer-components/youtubeVideo/index.tsx`:
- Line 287: Update the effect containing the formMode, videoId, and iframeShown
guard so it reruns when a previously hidden iframe becomes visible,
reinitializing the player and restoring playback tracking and configured events.
Include iframeShown in the effect’s dependency list and preserve the existing
designer-mode and empty-video behavior.
- Line 27: The YouTube video component currently ignores the configured “Require
Watching” start and completion settings at runtime. Update the component’s
runtime/rendering flow around its non-input configuration and the logic at the
referenced settings sections to evaluate these requirements, display the
appropriate warning, and gate submission when enabled, preserving normal
behavior when both options are disabled.
---
Outside diff comments:
In `@shesha-reactjs/src/designer-components/youtubeVideo/index.tsx`:
- Around line 127-164: Update the responsive branch of the dimension resolution
around finalDimensionStyles so configured widths retain their original CSS
value, including numeric values and pixel strings, instead of passing through
toPercentage. Use 100% only when dimensionStyles.width is absent, while
preserving the existing height reset and fixed-dimension behavior.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 071a36a8-50af-43b7-acb9-cc315eacc537
📒 Files selected for processing (4)
shesha-reactjs/src/componentsApi/componentApi.tsshesha-reactjs/src/designer-components/youtubeVideo/index.tsxshesha-reactjs/src/designer-components/youtubeVideo/interfaces.tsshesha-reactjs/src/designer-components/youtubeVideo/settingsForm.ts
💤 Files with no reviewable changes (2)
- shesha-reactjs/src/designer-components/youtubeVideo/interfaces.ts
- shesha-reactjs/src/designer-components/youtubeVideo/settingsForm.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shesha-reactjs/src/designer-components/youtubeVideo/index.tsx (1)
127-145: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not reinterpret CSS dimensions as percentages.
toPercentage('500px')returns500%, so fixed responsive widths can stretch beyond the container. Values likecalc(100% - 20px)are discarded and fall back to100%, ignoring configured responsive widths. Keep valid CSS sizes separate from percentage support, or derive explicit percentage input from the settings schema.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shesha-reactjs/src/designer-components/youtubeVideo/index.tsx` around lines 127 - 145, Update toPercentage so it does not reinterpret pixel or other CSS dimensions as percentages: preserve only valid percentage values, and handle configured CSS sizes such as 500px or calc(100% - 20px) through the existing width logic rather than converting or discarding them. Use the responsive width settings and schema to distinguish explicit percentage input from other valid CSS values, retaining the intended fallback only for unsupported values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@shesha-reactjs/src/designer-components/youtubeVideo/index.tsx`:
- Around line 127-145: Update toPercentage so it does not reinterpret pixel or
other CSS dimensions as percentages: preserve only valid percentage values, and
handle configured CSS sizes such as 500px or calc(100% - 20px) through the
existing width logic rather than converting or discarding them. Use the
responsive width settings and schema to distinguish explicit percentage input
from other valid CSS values, retaining the intended fallback only for
unsupported values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f5189aa3-7d2a-4f92-919a-e6acfce743cc
📒 Files selected for processing (2)
shesha-reactjs/src/designer-components/youtubeVideo/index.tsxshesha-reactjs/src/designer-components/youtubeVideo/styles.ts
💤 Files with no reviewable changes (1)
- shesha-reactjs/src/designer-components/youtubeVideo/styles.ts
2 participants
…ance tooltips
Summary by CodeRabbit