Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPropagates explicit shorts context and isShort flags through UI and player layers, adds a queued portal-driven Shorts transition system, introduces floating-Shorts actions/UI, updates selectors/actions to carry short metadata, and adds extensive Shorts-floating and transition SCSS. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ShortsPage
participant TransitionQueue
participant PortalPreview
participant Redux
participant VideoRender
User->>ShortsPage: wheel / next / prev input
ShortsPage->>TransitionQueue: enqueueTransition(direction, thumbnail)
TransitionQueue->>PortalPreview: renderPreview(thumbnail, direction)
PortalPreview-->>User: animate preview
TransitionQueue->>ShortsPage: finalizeTransition -> compute next URI
ShortsPage->>Redux: doSetPlayingUri({ uri, isShort: true })
Redux-->>VideoRender: playingUri updated
VideoRender-->>User: play updated short
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/view.jsx (1)
110-123:⚠️ Potential issue | 🟠 Major
urlParamsin the dependency array triggers this effect on every render.
urlParamsis anew URLSearchParams(search)created on line 85, so it's a new object reference every render. React's referential equality check will always see it as changed, causing this effect to fire continuously. Sincesearch(a string) is already in the dependency array,urlParamsshould be removed.🐛 Proposed fix
- }, [isShortVideo, shortsView, urlParams, search, history]); + }, [isShortVideo, shortsView, search, history]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/view.jsx` around lines 110 - 123, The effect is re-running every render because the URLSearchParams object (urlParams) has a new reference each time; remove urlParams from the useEffect dependency array and instead either (A) create const urlParams = new URLSearchParams(search) inside the useEffect body so the effect reads the current search without depending on the external object, or (B) memoize urlParams with useMemo(() => new URLSearchParams(search), [search]) and keep only stable primitives (isShortVideo, shortsView, search, history) in the dependency list; update the useEffect for the branches that set/delete 'view' (which reference urlParams, isShortVideo, shortsView, search, history) accordingly.
🧹 Nitpick comments (8)
ui/redux/selectors/content.js (1)
83-83: Pre-existing bug: JS comma operator discardscanonical_urlcheck.Not introduced in this PR, but worth noting:
(claim.canonical_url, claim.permanent_url).includes(primaryUri)uses the comma operator, so onlyclaim.permanent_urlis checked. The same pattern appears on line 99. The intent was likely[claim.canonical_url, claim.permanent_url].includes(...).♻️ Proposed fix (for both lines 83 and 99)
- return (claim.canonical_url, claim.permanent_url).includes(primaryUri); + return [claim.canonical_url, claim.permanent_url].includes(primaryUri);- return (claim.canonical_url, claim.permanent_url).includes(playingUri); + return [claim.canonical_url, claim.permanent_url].includes(playingUri);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/redux/selectors/content.js` at line 83, The return uses the comma operator so only claim.permanent_url is checked: replace the comma expression (claim.canonical_url, claim.permanent_url).includes(primaryUri) with an array check [claim.canonical_url, claim.permanent_url].includes(primaryUri) (and apply the same fix to the second occurrence that uses the same pattern) so both canonical_url and permanent_url are tested; locate occurrences around the selector that references primaryUri/claim and update them accordingly.ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx (3)
609-632: Wheel handler:e.preventDefault()may block legitimate page scrolling.The handler calls
e.preventDefault()for all scroll events with|deltaY| >= 8inside the shorts container, even if the user is scrolling content in the side panel (the side panel check on line 614 only catchesclientX/clientYdetection, which can miss cases). This could interfere with scrolling comments or info panels on desktop if the mouse hovers over the shorts container boundary.The 120ms wheel lock (line 628-629) is fairly short and may allow rapid sequential transitions if the user scrolls aggressively. The queue system handles this gracefully, but it could result in skipping through many shorts unintentionally.
💡 Consider increasing the wheel lock duration
setTimeout(() => { wheelLockRef.current = false; - }, 120); + }, REEL_TRANSITION_MS + 100);Aligning the lock with the transition duration prevents queuing additional transitions before the current one's navigation fires.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx` around lines 609 - 632, The wheel handler handleScroll currently calls e.preventDefault() for any large wheel event and uses a short 120ms lock; modify it so preventDefault is only invoked when the wheel event is not occurring over a scrollable/side-panel region (extend the side-panel detection by checking e.target and ancestors via isSwipeInsideSidePanel or by testing for a scrollable ancestor where scrollHeight > clientHeight and overflow allows scrolling), and only block default when that check fails; keep the existing goToNext/goToPrevious calls but ensure wheelLockRef is held for a longer duration (increase from 120ms to match the shorts transition time, e.g., ~300ms) so rapid scrolls don’t trigger multiple transitions.
519-524: Directdocument.body.stylemutation — ensure cleanup is robust.Setting
document.body.style.overflow = 'hidden'(line 523) is cleaned up infinishTransition(line 485) and the unmount effect (line 604). However, ifprocessNextQueuedTransitionis called butfinishTransitionnever fires (e.g., component unmounts between the two timers), the fallback timer (line 538-541) clears the queue and callsfinishTransitionRef.current(false), which does restore overflow. The unmount cleanup on lines 597-607 also restores it. This looks covered, but consider using a CSS class toggle on body instead, which is easier to reason about and debug.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx` around lines 519 - 524, The code directly mutates document.body.style.overflow; replace that with a body CSS-class toggle (e.g., "shorts-modal-open") to control overflow so mutations are centralized and easier to reason about: wherever you currently set document.body.style.overflow = 'hidden' (in the transition start path inside processNextQueuedTransition / the block using window.player) call document.body.classList.add('shorts-modal-open') instead, and ensure finishTransition (and finishTransitionRef.current(false) used by the fallback timer) removes the class (document.body.classList.remove('shorts-modal-open')), and also remove the class in the unmount cleanup effect so all three places (start, finishTransition, unmount/fallback) consistently add/remove the same class; add the CSS rule for .shorts-modal-open { overflow: hidden !important; } in the component stylesheet.
139-148: Consider consolidating mirrored ref + state pairs.Several state values are duplicated as both
useStateanduseRef(e.g.,isTransitioning/isTransitioningRef,finishTransition/finishTransitionRef,processNextQueuedTransition/processNextTransitionRef). The refs are needed for accessing the latest value inside callbacks/effects without stale closures, which is valid, but the sheer number (6 refs + 3 states for transition tracking alone) increases cognitive load. AuseReduceror a single transition state object could simplify this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx` around lines 139 - 148, You have multiple duplicated state+ref pairs (isTransitioning/isTransitioningRef, transitionDirection, transitionThumbnailUrl, activeTransitionRef, finishTransitionRef, processNextTransitionRef) which increases complexity; replace them with a single reducer or consolidated state object (e.g., transitionState with {isTransitioning, direction, thumbnailUrl, active}) managed via useReducer, keep only truly imperative refs (transitionTimerRef, transitionFallbackTimerRef) for timers, and convert finishTransitionRef and processNextTransitionRef into stable callbacks (useCallback) that read/update the consolidated state or dispatch actions so you no longer maintain mirrored useState/useRef pairs—update all places that set/read isTransitioning, transitionDirection, transitionThumbnailUrl, activeTransitionRef, finishTransitionRef, and processNextTransitionRef to use the new reducer state and dispatched actions (or stable callbacks) instead.ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/view.jsx (1)
98-108: Full page reload (window.location.reload()) is heavy-handed.When a non-short claim is visited with
?view=shorts, this triggers a full browser reload. Consider usinghistory.replace(...)and letting React re-render instead — similar to lines 118-121 which handle the reverse case without a reload.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/view.jsx` around lines 98 - 108, The effect handling the transition away from shorts currently forces a full page reload (window.location.reload()), which is unnecessary; in the React effect in view.jsx that checks isClaimShort, shortsView and search, remove the window.location.reload() call and rely on the already-called window.history.replaceState({}, '', newUrl) (or call history.replace/new navigate helper if using react-router) so the URL is updated and React can re-render based on the updated search param; keep the effect dependencies (isClaimShort, shortsView, search) unchanged so components reading search will update.ui/hocs/withStreamClaimRender/view.jsx (1)
345-351:{...clickProps}spread could shadow explicit props.Since the spread comes after the named props, any key in
clickProps(e.g.,uri,isShortsContext) would silently override them. Consider placing the spread before named props, or destructuring only the expected keys.🔧 Safer ordering
<ClaimCoverRender + {...clickProps} uri={uri} transparent isShortsContext={isShortsContext} isFloatingContext={isFloatingContext} - {...clickProps} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/hocs/withStreamClaimRender/view.jsx` around lines 345 - 351, The spread {...clickProps} placed after explicit props on the ClaimCoverRender component can silently override named props like uri, isShortsContext, and isFloatingContext; fix this by either moving {...clickProps} before the named props on ClaimCoverRender or by destructuring clickProps to pass only the expected keys (e.g., const { onClick, onMouseEnter } = clickProps) and then spreading those safe props, ensuring uri, isShortsContext, and isFloatingContext cannot be shadowed.ui/scss/component/_shorts.scss (2)
1-58: Transition container and CSS-variable-driven positioning are clean.The
--shorts-preview-translate-xvariable approach is a good pattern for keeping the keyframe animations responsive to panel state and breakpoint. Thenext/previousmodifiers and their animations read well.Stylelint flags missing empty lines before declarations on Lines 3 and 51 (
declaration-empty-line-before). These are minor formatting nits — consider adding blank lines after the CSS variable declarations to satisfy the linter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/scss/component/_shorts.scss` around lines 1 - 58, Add missing blank lines after the CSS variable declarations to satisfy stylelint's declaration-empty-line-before rule: in .shorts-transition-preview add one empty line after the initial "--shorts-preview-translate-x: calc(-50% - 6px);" declaration, and in the "@media (max-width: $breakpoint-small)" block inside .shorts-transition-preview add one empty line after the "--shorts-preview-translate-x: 0;" declaration (references: .shorts-transition-preview and the --shorts-preview-translate-x variable).
737-783: Duplicate styles:--panel-openand--side-panel-openare identical.
.content__cover--panel-open(Lines 737-747) and.content__cover--side-panel-open(Lines 773-783) contain the exact same transform values and media queries. If they're intended to behave identically, combine them into a single rule to reduce duplication:♻️ Suggested consolidation
- &.content__cover--panel-open { - transform: translate(-130%, 0%) !important; - - `@media` (max-width: $breakpoint-small) { - transform: translate(-50%, 0%) !important; - } - - `@media` (max-width: 1180px) and (min-width: $breakpoint-small) { - transform: translate(-105%, 0%) !important; - } - } ... - &.content__cover--side-panel-open { - transform: translate(-130%, 0%) !important; - - `@media` (max-width: $breakpoint-small) { - transform: translate(-50%, 0%) !important; - } - - `@media` (max-width: 1180px) and (min-width: $breakpoint-small) { - transform: translate(-105%, 0%) !important; - } - } + &.content__cover--panel-open, + &.content__cover--side-panel-open { + transform: translate(-130%, 0%) !important; + + `@media` (max-width: $breakpoint-small) { + transform: translate(-50%, 0%) !important; + } + + `@media` (max-width: 1180px) and (min-width: $breakpoint-small) { + transform: translate(-105%, 0%) !important; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/scss/component/_shorts.scss` around lines 737 - 783, The two modifier classes .content__cover--panel-open and .content__cover--side-panel-open are identical; replace the duplicate block by consolidating them into a single rule (e.g., a grouped selector for .content__cover--panel-open, .content__cover--side-panel-open) containing the shared transform and media-query overrides, then remove the redundant block; ensure the grouped selector preserves the same inner media query structure and !important declarations so behavior remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/component/shortsActions/swipeNavigation/view.jsx`:
- Around line 158-160: The current wheel lock timeout (setTimeout that sets
wheelLockRef.current = false) is too short (120ms) and allows trackpad inertia
to trigger duplicate onNext/onPrevious calls; update the lock to a 300–400 ms
window (e.g., 300ms) or replace the simple timeout with an accumulated-delta
approach that extends wheelLockRef until no wheel delta has arrived for ~300ms
so momentum events are swallowed; change the timeout call in the swipe
navigation code (the setTimeout that clears wheelLockRef.current) or implement a
debounced reset that references wheelLockRef and the wheel event handler to
avoid double-navigation.
In `@ui/component/videoRenderFloating/view.jsx`:
- Around line 837-856: The FloatingRender component's Flow props are missing the
forceRenderStream prop: update the destructured prop type for the HOC component
(the anonymous function passed to withStreamClaimRender that returns
<VideoRender ... />) to include forceRenderStream: boolean (or the appropriate
type), and pass it through to VideoRender via the existing spread/prop so the
Flow signature matches the runtime usage; adjust the type alongside the existing
uri, draggable, isShortsContext, and isFloatingContext declarations in the same
function.
In
`@ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx`:
- Around line 533-536: The current extraction of nextClaimId uses
activeTransition.targetUri.split('#')[1] which fails for channel-prefixed LBRY
URIs (e.g. lbry://@channel#abc123/video#def456); update the logic in the block
that sets nextClaimId (inside the activeTransition.direction === 'next' branch)
to take the last fragment after the final '#' when present (e.g.
split('#').pop()) and only fall back to splitting on '/' (pop) if no '#' exists,
then pass that resulting claim id to onRecommendationClicked(claimId,
nextClaimId).
In `@ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/view.jsx`:
- Around line 149-155: The ShortsPage instance is reused across different uri
values causing stale local UI state (mobileModalOpen, panelMode, localViewMode,
firstShortPlayedRef) to persist; fix by adding a unique key prop tied to the uri
(e.g., key={uri}) on the <ShortsPage /> element so React will unmount/remount
ShortsPage when uri changes, ensuring its internal state resets.
In `@ui/scss/component/_content.scss`:
- Line 263: Replace the chained negation selectors with the level-4 combined
:not() notation: change the selector that currently uses
&:not(.content-info__playlist):not(.content__info--shorts-floating) to a single
:not() containing both classes (i.e., :not(.content-info__playlist,
.content__info--shorts-floating)) so it satisfies Stylelint's
selector-not-notation rule.
---
Outside diff comments:
In `@ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/view.jsx`:
- Around line 110-123: The effect is re-running every render because the
URLSearchParams object (urlParams) has a new reference each time; remove
urlParams from the useEffect dependency array and instead either (A) create
const urlParams = new URLSearchParams(search) inside the useEffect body so the
effect reads the current search without depending on the external object, or (B)
memoize urlParams with useMemo(() => new URLSearchParams(search), [search]) and
keep only stable primitives (isShortVideo, shortsView, search, history) in the
dependency list; update the useEffect for the branches that set/delete 'view'
(which reference urlParams, isShortVideo, shortsView, search, history)
accordingly.
---
Nitpick comments:
In `@ui/hocs/withStreamClaimRender/view.jsx`:
- Around line 345-351: The spread {...clickProps} placed after explicit props on
the ClaimCoverRender component can silently override named props like uri,
isShortsContext, and isFloatingContext; fix this by either moving
{...clickProps} before the named props on ClaimCoverRender or by destructuring
clickProps to pass only the expected keys (e.g., const { onClick, onMouseEnter }
= clickProps) and then spreading those safe props, ensuring uri,
isShortsContext, and isFloatingContext cannot be shadowed.
In
`@ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx`:
- Around line 609-632: The wheel handler handleScroll currently calls
e.preventDefault() for any large wheel event and uses a short 120ms lock; modify
it so preventDefault is only invoked when the wheel event is not occurring over
a scrollable/side-panel region (extend the side-panel detection by checking
e.target and ancestors via isSwipeInsideSidePanel or by testing for a scrollable
ancestor where scrollHeight > clientHeight and overflow allows scrolling), and
only block default when that check fails; keep the existing
goToNext/goToPrevious calls but ensure wheelLockRef is held for a longer
duration (increase from 120ms to match the shorts transition time, e.g., ~300ms)
so rapid scrolls don’t trigger multiple transitions.
- Around line 519-524: The code directly mutates document.body.style.overflow;
replace that with a body CSS-class toggle (e.g., "shorts-modal-open") to control
overflow so mutations are centralized and easier to reason about: wherever you
currently set document.body.style.overflow = 'hidden' (in the transition start
path inside processNextQueuedTransition / the block using window.player) call
document.body.classList.add('shorts-modal-open') instead, and ensure
finishTransition (and finishTransitionRef.current(false) used by the fallback
timer) removes the class (document.body.classList.remove('shorts-modal-open')),
and also remove the class in the unmount cleanup effect so all three places
(start, finishTransition, unmount/fallback) consistently add/remove the same
class; add the CSS rule for .shorts-modal-open { overflow: hidden !important; }
in the component stylesheet.
- Around line 139-148: You have multiple duplicated state+ref pairs
(isTransitioning/isTransitioningRef, transitionDirection,
transitionThumbnailUrl, activeTransitionRef, finishTransitionRef,
processNextTransitionRef) which increases complexity; replace them with a single
reducer or consolidated state object (e.g., transitionState with
{isTransitioning, direction, thumbnailUrl, active}) managed via useReducer, keep
only truly imperative refs (transitionTimerRef, transitionFallbackTimerRef) for
timers, and convert finishTransitionRef and processNextTransitionRef into stable
callbacks (useCallback) that read/update the consolidated state or dispatch
actions so you no longer maintain mirrored useState/useRef pairs—update all
places that set/read isTransitioning, transitionDirection,
transitionThumbnailUrl, activeTransitionRef, finishTransitionRef, and
processNextTransitionRef to use the new reducer state and dispatched actions (or
stable callbacks) instead.
In `@ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/view.jsx`:
- Around line 98-108: The effect handling the transition away from shorts
currently forces a full page reload (window.location.reload()), which is
unnecessary; in the React effect in view.jsx that checks isClaimShort,
shortsView and search, remove the window.location.reload() call and rely on the
already-called window.history.replaceState({}, '', newUrl) (or call
history.replace/new navigate helper if using react-router) so the URL is updated
and React can re-render based on the updated search param; keep the effect
dependencies (isClaimShort, shortsView, search) unchanged so components reading
search will update.
In `@ui/redux/selectors/content.js`:
- Line 83: The return uses the comma operator so only claim.permanent_url is
checked: replace the comma expression (claim.canonical_url,
claim.permanent_url).includes(primaryUri) with an array check
[claim.canonical_url, claim.permanent_url].includes(primaryUri) (and apply the
same fix to the second occurrence that uses the same pattern) so both
canonical_url and permanent_url are tested; locate occurrences around the
selector that references primaryUri/claim and update them accordingly.
In `@ui/scss/component/_shorts.scss`:
- Around line 1-58: Add missing blank lines after the CSS variable declarations
to satisfy stylelint's declaration-empty-line-before rule: in
.shorts-transition-preview add one empty line after the initial
"--shorts-preview-translate-x: calc(-50% - 6px);" declaration, and in the
"@media (max-width: $breakpoint-small)" block inside .shorts-transition-preview
add one empty line after the "--shorts-preview-translate-x: 0;" declaration
(references: .shorts-transition-preview and the --shorts-preview-translate-x
variable).
- Around line 737-783: The two modifier classes .content__cover--panel-open and
.content__cover--side-panel-open are identical; replace the duplicate block by
consolidating them into a single rule (e.g., a grouped selector for
.content__cover--panel-open, .content__cover--side-panel-open) containing the
shared transform and media-query overrides, then remove the redundant block;
ensure the grouped selector preserves the same inner media query structure and
!important declarations so behavior remains unchanged.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
flow-typed/content.jsui/component/claimCoverRender/view.jsxui/component/shortsActions/swipeNavigation/view.jsxui/component/videoClaimRender/view.jsxui/component/videoRenderFloating/index.jsui/component/videoRenderFloating/view.jsxui/hocs/withStreamClaimRender/view.jsxui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsxui/page/claim/internal/claimPageComponent/internal/streamClaimPage/view.jsxui/redux/actions/content.jsui/redux/selectors/content.jsui/scss/component/_content.scssui/scss/component/_shorts.scss
Comment on lines
158
to
+160
| setTimeout(() => { | ||
| scrollLockRef.current = false; | ||
| }, 500); | ||
| wheelLockRef.current = false; | ||
| }, 120); |
There was a problem hiding this comment.
120 ms may be too short to absorb trackpad inertia, causing double-navigation.
Trackpad and high-resolution wheel devices emit momentum events for 300–500 ms after the physical gesture ends. With a 120 ms lock window, a single swipe can release the lock while inertia events are still arriving, firing a second onNext/onPrevious call unintentionally. Discrete scroll-wheel mice are unaffected.
A threshold in the 300–400 ms range (or an accumulated-delta approach) is safer:
🐛 Proposed fix — raise the guard to 300 ms
- }, 120);
+ }, 300);📝 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.
Suggested change
| setTimeout(() => { | |
| scrollLockRef.current = false; | |
| }, 500); | |
| wheelLockRef.current = false; | |
| }, 120); | |
| setTimeout(() => { | |
| wheelLockRef.current = false; | |
| }, 300); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/component/shortsActions/swipeNavigation/view.jsx` around lines 158 - 160,
The current wheel lock timeout (setTimeout that sets wheelLockRef.current =
false) is too short (120ms) and allows trackpad inertia to trigger duplicate
onNext/onPrevious calls; update the lock to a 300–400 ms window (e.g., 300ms) or
replace the simple timeout with an accumulated-delta approach that extends
wheelLockRef until no wheel delta has arrived for ~300ms so momentum events are
swallowed; change the timeout call in the swipe navigation code (the setTimeout
that clears wheelLockRef.current) or implement a debounced reset that references
wheelLockRef and the wheel event handler to avoid double-navigation.
Comment on lines
+837
to
+856
| const FloatingRender = withStreamClaimRender( | ||
| ({ | ||
| uri, | ||
| draggable, | ||
| isShortsContext, | ||
| isFloatingContext, | ||
| }: { | ||
| uri: string, | ||
| draggable: boolean, | ||
| isShortsContext?: boolean, | ||
| isFloatingContext?: boolean, | ||
| }) => ( | ||
| <VideoRender | ||
| className={classnames({ draggable })} | ||
| uri={uri} | ||
| isShortsContext={isShortsContext} | ||
| isFloatingContext={isFloatingContext} | ||
| /> | ||
| ) | ||
| ); |
There was a problem hiding this comment.
forceRenderStream is passed to FloatingRender but not declared in its type.
At Line 539, forceRenderStream={isFloating} is passed, but the destructured props and Flow type on Lines 838-848 don't include forceRenderStream. The HOC receives it via ...props so it works at runtime, but the type annotation is incomplete.
🔧 Add missing prop to type
const FloatingRender = withStreamClaimRender(
({
uri,
draggable,
isShortsContext,
isFloatingContext,
}: {
uri: string,
draggable: boolean,
isShortsContext?: boolean,
isFloatingContext?: boolean,
+ forceRenderStream?: boolean,
}) => (📝 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.
Suggested change
| const FloatingRender = withStreamClaimRender( | |
| ({ | |
| uri, | |
| draggable, | |
| isShortsContext, | |
| isFloatingContext, | |
| }: { | |
| uri: string, | |
| draggable: boolean, | |
| isShortsContext?: boolean, | |
| isFloatingContext?: boolean, | |
| }) => ( | |
| <VideoRender | |
| className={classnames({ draggable })} | |
| uri={uri} | |
| isShortsContext={isShortsContext} | |
| isFloatingContext={isFloatingContext} | |
| /> | |
| ) | |
| ); | |
| const FloatingRender = withStreamClaimRender( | |
| ({ | |
| uri, | |
| draggable, | |
| isShortsContext, | |
| isFloatingContext, | |
| }: { | |
| uri: string, | |
| draggable: boolean, | |
| isShortsContext?: boolean, | |
| isFloatingContext?: boolean, | |
| forceRenderStream?: boolean, | |
| }) => ( | |
| <VideoRender | |
| className={classnames({ draggable })} | |
| uri={uri} | |
| isShortsContext={isShortsContext} | |
| isFloatingContext={isFloatingContext} | |
| /> | |
| ) | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/component/videoRenderFloating/view.jsx` around lines 837 - 856, The
FloatingRender component's Flow props are missing the forceRenderStream prop:
update the destructured prop type for the HOC component (the anonymous function
passed to withStreamClaimRender that returns <VideoRender ... />) to include
forceRenderStream: boolean (or the appropriate type), and pass it through to
VideoRender via the existing spread/prop so the Flow signature matches the
runtime usage; adjust the type alongside the existing uri, draggable,
isShortsContext, and isFloatingContext declarations in the same function.
ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx
Show resolved
Hide resolved
Comment on lines
149
to
155
| if (isShortVideo) { | ||
| return ( | ||
| <React.Suspense fallback={null}> | ||
| <ShortsPage uri={uri} accessStatus={accessStatus} key={uri} /> | ||
| <ShortsPage uri={uri} accessStatus={accessStatus} /> | ||
| </React.Suspense> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check what state is reset on uri change in ShortsPage
rg -n 'useEffect.*\[.*uri' ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx
echo "---"
# Check for useState calls that might go stale
rg -n 'useState' ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsxRepository: OdyseeTeam/odysee-frontend
Length of output: 696
🏁 Script executed:
# Get file size and check the structure
wc -l ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx
echo "---"
# Look for all useEffect calls
rg -n 'useEffect' ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx | head -20
echo "---"
# Search for hasInitializedRef mentioned in review
rg -n 'hasInitializedRef' ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx
echo "---"
# Look at dependency arrays more broadly
rg -n '\[\s*(uri|.*uri.*)\s*\]' ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsxRepository: OdyseeTeam/odysee-frontend
Length of output: 1406
🏁 Script executed:
# Read the key useEffect hooks with uri dependency
sed -n '396,412p' ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx
echo "---"
sed -n '285,295p' ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx
echo "---"
sed -n '288,305p' ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx
echo "---"
sed -n '430,440p' ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx
echo "---"
# Check if videoStarted, mobileModalOpen, panelMode, localViewMode state is reset anywhere on uri change
rg -n -B2 'setVideoStarted\|setMobileModalOpen\|setPanelMode\|setLocalViewMode' ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx | head -40Repository: OdyseeTeam/odysee-frontend
Length of output: 1829
🏁 Script executed:
# Check all uses of setVideoStarted, setMobileModalOpen, setPanelMode, setLocalViewMode
rg -n 'setVideoStarted|setMobileModalOpen|setPanelMode|setLocalViewMode' ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx
echo "---"
# Check which useEffect hooks contain resets to these states
rg -B5 'setVideoStarted|setMobileModalOpen|setPanelMode|setLocalViewMode' ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx | grep -A5 -B5 'useEffect\|setVideoStarted\|setMobileModalOpen\|setPanelMode\|setLocalViewMode'Repository: OdyseeTeam/odysee-frontend
Length of output: 3729
🏁 Script executed:
# Get the exact useEffect at line 414-420 with setLocalViewMode
sed -n '414,422p' ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx
echo "---"
# Get the exact useEffect around line 250-260 with setMobileModalOpen and setPanelMode
sed -n '248,262p' ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx
echo "---"
# Get the exact useEffect around line 318 with setVideoStarted
sed -n '314,325p' ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsxRepository: OdyseeTeam/odysee-frontend
Length of output: 1220
🏁 Script executed:
rg -n 'firstShortPlayedRef' ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsxRepository: OdyseeTeam/odysee-frontend
Length of output: 312
🏁 Script executed:
# Check context around lines 350 and 372
sed -n '345,355p' ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx
echo "---"
sed -n '368,378p' ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx
echo "---"
# Check what useEffect these are in by looking at broader context
sed -n '340,380p' ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsxRepository: OdyseeTeam/odysee-frontend
Length of output: 2528
Removing the key prop from ShortsPage does create a stale state risk for UI state variables.
Without the key prop, React reuses the same ShortsPage component instance across different uri values. While some state (e.g., videoStarted, hasInitializedRef) is reset on URI changes via useEffect dependencies, critical UI state is not:
mobileModalOpen– NOT reset on URI change (dependency:[linkedCommentId, isMobile])panelMode– NOT reset on URI change (dependency:[linkedCommentId, isMobile])localViewMode– NOT reset on URI change (dependency:[reduxViewMode, localViewMode])firstShortPlayedRef– NOT reset on URI change (only resets on page navigation away from shorts)
When navigating between different shorts, users will see the previous short's modal open/closed state, panel mode, and view mode carry over to the new short. Adding a key={uri} to ShortsPage would unmount and remount the component on URI changes, properly resetting all local state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/view.jsx`
around lines 149 - 155, The ShortsPage instance is reused across different uri
values causing stale local UI state (mobileModalOpen, panelMode, localViewMode,
firstShortPlayedRef) to persist; fix by adding a unique key prop tied to the uri
(e.g., key={uri}) on the <ShortsPage /> element so React will unmount/remount
ShortsPage when uri changes, ensuring its internal state resets.
| } | ||
| .content__info { | ||
| &:not(.content-info__playlist) { | ||
| &:not(.content-info__playlist):not(.content__info--shorts-floating) { |
There was a problem hiding this comment.
Stylelint: use complex :not() notation (Level 4 selector).
Stylelint's selector-not-notation rule expects the combined form :not(.a, .b) instead of chaining :not(.a):not(.b).
🔧 Proposed fix
- &:not(.content-info__playlist):not(.content__info--shorts-floating) {
+ &:not(.content-info__playlist, .content__info--shorts-floating) {📝 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.
Suggested change
| &:not(.content-info__playlist):not(.content__info--shorts-floating) { | |
| &:not(.content-info__playlist, .content__info--shorts-floating) { |
🧰 Tools
🪛 Stylelint (17.3.0)
[error] 263-263: Expected complex :not() pseudo-class notation (selector-not-notation)
(selector-not-notation)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/scss/component/_content.scss` at line 263, Replace the chained negation
selectors with the level-4 combined :not() notation: change the selector that
currently uses
&:not(.content-info__playlist):not(.content__info--shorts-floating) to a single
:not() containing both classes (i.e., :not(.content-info__playlist,
.content__info--shorts-floating)) so it satisfies Stylelint's
selector-not-notation rule.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx (1)
533-536:⚠️ Potential issue | 🟠 MajorFix claim ID extraction for channel-prefixed LBRY URIs.
At Line 534,
split('#')[1]can extract the channel fragment instead of the stream claim ID (for example,lbry://@channel#abc123/video#def456). This misattributesonRecommendationClicked.Proposed fix
- if (activeTransition.direction === 'next' && claimId) { - const nextClaimId = activeTransition.targetUri.split('#')[1] || activeTransition.targetUri.split('/').pop(); - onRecommendationClicked(claimId, nextClaimId); - } + if (activeTransition.direction === 'next' && claimId) { + const hashParts = activeTransition.targetUri.split('#'); + const nextClaimId = + (hashParts.length > 1 ? hashParts[hashParts.length - 1] : null) || + activeTransition.targetUri.split('/').pop(); + + if (nextClaimId) { + onRecommendationClicked(claimId, nextClaimId); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx` around lines 533 - 536, The current extraction of nextClaimId uses split('#')[1] which can pick up a channel fragment (e.g. lbry://@channel#abc123/video#def456); update the logic in the block that handles activeTransition.targetUri (used to call onRecommendationClicked) to extract the final stream claim id by taking the last path segment then the last hash fragment, e.g. compute nextClaimId from activeTransition.targetUri by doing something like take activeTransition.targetUri.split('/').pop() and then .split('#').pop(), and pass that into onRecommendationClicked(claimId, nextClaimId).ui/component/videoRenderFloating/view.jsx (1)
966-977:⚠️ Potential issue | 🟡 Minor
forceRenderStreamis still missing fromFloatingRenderprop type.Line 658 passes
forceRenderStream, but Line 972-Line 977 type definition still omits it.🔧 Proposed fix
}: { uri: string, draggable: boolean, isShortsContext?: boolean, isFloatingContext?: boolean, + forceRenderStream?: boolean, }) => (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/component/videoRenderFloating/view.jsx` around lines 966 - 977, The FloatingRender component's prop type declaration (the object typed in the arrow function passed to withStreamClaimRender) is missing the forceRenderStream prop that is passed in elsewhere; update the destructured prop type for FloatingRender to include forceRenderStream (e.g., forceRenderStream?: boolean or the actual expected type) so it matches how it is used when invoking FloatingRender and flows through withStreamClaimRender.ui/scss/component/_content.scss (1)
264-264:⚠️ Potential issue | 🟡 MinorUse combined
:not()notation to satisfy Stylelint.Line 264 still uses chained
:not()selectors. Convert to level-4 combined form.🔧 Proposed fix
- &:not(.content-info__playlist):not(.content__info--shorts-floating) { + &:not(.content-info__playlist, .content__info--shorts-floating) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/scss/component/_content.scss` at line 264, Replace the chained :not() selectors used on the rule with a single level-4 combined :not() form: locate the nested selector starting with "&:not(.content-info__playlist):not(.content__info--shorts-floating)" and change it to use one :not(...) that lists both classes (i.e., :not(.content-info__playlist, .content__info--shorts-floating)) so Stylelint accepts the selector; ensure spacing and nesting follow the surrounding SCSS style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/component/videoRenderFloating/internal/floatingShortsActions/view.jsx`:
- Around line 42-44: The effect is calling doFetchReactions(claimId) but
reactions are keyed by URI; update the React.useEffect to call
doFetchReactions(uri) and change its dependency from claimId to uri (keep
doFetchReactions) so the URI-based reaction selectors/actions refresh correctly;
reference the useEffect, doFetchReactions, claimId and uri identifiers when
making the change.
In `@ui/component/videoRenderFloating/view.jsx`:
- Around line 413-443: The handleEnded callback schedules inner setTimeouts that
can fire after unmount/URI change; modify handleEnded to store those timer IDs
(e.g., push to an array or assign to a variable like endTimers) when calling
setTimeout for goToNextShort() and for resetting/playing the video, expose/close
over that endTimers collection in the surrounding scope, and update cleanupFn
and the effect return to clear all stored endTimers (clearTimeout) in addition
to removing the 'ended' listener and clearing interval/timeout so no delayed
replay or navigation runs after teardown; reference handleEnded, attachListener,
cleanupFn, interval and timeout when making the changes.
In
`@ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx`:
- Around line 484-486: When you lock scrolling by setting
document.body.style.overflow = 'hidden' (seen in view.jsx), capture and save the
prior value (e.g., const prevOverflow = document.body?.style.overflow) before
overwriting it, and on cleanup restore document.body.style.overflow =
prevOverflow (or undefined/null-check before assigning) rather than assigning an
empty string; apply this pattern wherever you set overflow to 'hidden' (the
occurrences around the current snippet and the ones mentioned at the other
spots) and ensure the saved prevOverflow is scoped so the effect/handler cleanup
can access it.
In `@ui/scss/component/_content.scss`:
- Around line 430-456: The close button (.content__floating-close) is forced
hidden by opacity:0 and only shown on &:hover or .shorts-floating--paused, which
fails on touch devices; update the reveal selectors to also show the close
button during touch playback by adding the playing state selector (e.g.
.shorts-floating--playing) to the same rule set so the block that currently
targets &:hover, &.shorts-floating--paused also includes
&.shorts-floating--playing (or equivalently add a rule .shorts-floating--playing
.content__floating-close { opacity: 1; }), ensuring .content__floating-close
becomes visible on touch while shorts are playing.
In `@ui/scss/component/_shorts.scss`:
- Around line 1-117: Stylelint is flagging missing blank lines before
declarations in the .shorts-transition-preview and .shorts-transition-current
blocks; open those selectors (identify by .shorts-transition-preview and
.shorts-transition-current and their nested modifiers &--panel-open, &--next,
&--previous) and insert the required empty line(s) before the offending
declarations so they comply with declaration-empty-line-before (i.e., add a
blank line before the first CSS declaration after the selector and before the
nested modifier blocks where Stylelint expects it), then re-run linting to
confirm the failures at those selector regions are gone.
---
Duplicate comments:
In `@ui/component/videoRenderFloating/view.jsx`:
- Around line 966-977: The FloatingRender component's prop type declaration (the
object typed in the arrow function passed to withStreamClaimRender) is missing
the forceRenderStream prop that is passed in elsewhere; update the destructured
prop type for FloatingRender to include forceRenderStream (e.g.,
forceRenderStream?: boolean or the actual expected type) so it matches how it is
used when invoking FloatingRender and flows through withStreamClaimRender.
In
`@ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx`:
- Around line 533-536: The current extraction of nextClaimId uses split('#')[1]
which can pick up a channel fragment (e.g. lbry://@channel#abc123/video#def456);
update the logic in the block that handles activeTransition.targetUri (used to
call onRecommendationClicked) to extract the final stream claim id by taking the
last path segment then the last hash fragment, e.g. compute nextClaimId from
activeTransition.targetUri by doing something like take
activeTransition.targetUri.split('/').pop() and then .split('#').pop(), and pass
that into onRecommendationClicked(claimId, nextClaimId).
In `@ui/scss/component/_content.scss`:
- Line 264: Replace the chained :not() selectors used on the rule with a single
level-4 combined :not() form: locate the nested selector starting with
"&:not(.content-info__playlist):not(.content__info--shorts-floating)" and change
it to use one :not(...) that lists both classes (i.e.,
:not(.content-info__playlist, .content__info--shorts-floating)) so Stylelint
accepts the selector; ensure spacing and nesting follow the surrounding SCSS
style.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
ui/component/videoRenderFloating/index.jsui/component/videoRenderFloating/internal/floatingShortsActions/index.jsui/component/videoRenderFloating/internal/floatingShortsActions/view.jsxui/component/videoRenderFloating/view.jsxui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsxui/scss/component/_content.scssui/scss/component/_shorts.scss
Comment on lines
+42
to
+44
| React.useEffect(() => { | ||
| if (claimId) doFetchReactions(claimId); | ||
| }, [claimId, doFetchReactions]); |
There was a problem hiding this comment.
Fetch reactions with uri, not claimId.
At Line 43, doFetchReactions is called with claimId, but this flow is URI-keyed (selectors and reaction actions are URI-based). This can cause reaction counts/state not to refresh for the displayed short.
🔧 Proposed fix
type Props = {
uri: string,
claimId: string,
@@
- doFetchReactions: (claimId: string) => void,
+ doFetchReactions: (uri: string) => void,
@@
React.useEffect(() => {
- if (claimId) doFetchReactions(claimId);
- }, [claimId, doFetchReactions]);
+ if (uri) doFetchReactions(uri);
+ }, [uri, doFetchReactions]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/component/videoRenderFloating/internal/floatingShortsActions/view.jsx`
around lines 42 - 44, The effect is calling doFetchReactions(claimId) but
reactions are keyed by URI; update the React.useEffect to call
doFetchReactions(uri) and change its dependency from claimId to uri (keep
doFetchReactions) so the URI-based reaction selectors/actions refresh correctly;
reference the useEffect, doFetchReactions, claimId and uri identifiers when
making the change.
Comment on lines
+413
to
+443
| const handleEnded = () => { | ||
| if (autoPlayNextShort && hasNextShort) { | ||
| setTimeout(() => goToNextShort(), 500); | ||
| } else if (videoEl) { | ||
| const v = videoEl; | ||
| setTimeout(() => { | ||
| v.currentTime = 0; | ||
| // $FlowFixMe | ||
| v.play().catch(() => {}); | ||
| }, 100); | ||
| } | ||
| }; | ||
|
|
||
| videoEl.addEventListener('ended', handleEnded); | ||
| const currentEl = videoEl; | ||
| cleanupFn = () => { | ||
| currentEl.removeEventListener('ended', handleEnded); | ||
| videoEl = null; | ||
| }; | ||
| return true; | ||
| }; | ||
|
|
||
| attachListener(); | ||
| const interval = setInterval(attachListener, 200); | ||
| const timeout = setTimeout(() => clearInterval(interval), 10000); | ||
|
|
||
| return () => { | ||
| clearInterval(interval); | ||
| clearTimeout(timeout); | ||
| if (cleanupFn) cleanupFn(); | ||
| }; |
There was a problem hiding this comment.
Clear delayed end-of-video timers during cleanup.
At Line 415 and Line 418, delayed actions are scheduled but never tracked/cleared in effect cleanup. This can fire stale goToNextShort() or replay logic after URI changes/unmount.
🔧 Proposed fix
React.useEffect(() => {
if (!isShortsFloating) return;
let videoEl = null;
let cleanupFn = null;
+ let nextTimeoutId = null;
+ let replayTimeoutId = null;
@@
const handleEnded = () => {
if (autoPlayNextShort && hasNextShort) {
- setTimeout(() => goToNextShort(), 500);
+ if (nextTimeoutId) clearTimeout(nextTimeoutId);
+ nextTimeoutId = setTimeout(() => goToNextShort(), 500);
} else if (videoEl) {
const v = videoEl;
- setTimeout(() => {
+ if (replayTimeoutId) clearTimeout(replayTimeoutId);
+ replayTimeoutId = setTimeout(() => {
v.currentTime = 0;
// $FlowFixMe
v.play().catch(() => {});
}, 100);
}
};
@@
return () => {
clearInterval(interval);
clearTimeout(timeout);
+ if (nextTimeoutId) clearTimeout(nextTimeoutId);
+ if (replayTimeoutId) clearTimeout(replayTimeoutId);
if (cleanupFn) cleanupFn();
};
}, [isShortsFloating, uri, autoPlayNextShort, hasNextShort, goToNextShort]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/component/videoRenderFloating/view.jsx` around lines 413 - 443, The
handleEnded callback schedules inner setTimeouts that can fire after unmount/URI
change; modify handleEnded to store those timer IDs (e.g., push to an array or
assign to a variable like endTimers) when calling setTimeout for goToNextShort()
and for resetting/playing the video, expose/close over that endTimers collection
in the surrounding scope, and update cleanupFn and the effect return to clear
all stored endTimers (clearTimeout) in addition to removing the 'ended' listener
and clearing interval/timeout so no delayed replay or navigation runs after
teardown; reference handleEnded, attachListener, cleanupFn, interval and timeout
when making the changes.
Comment on lines
+484
to
486
| if (document.body) { | ||
| document.body.style.overflow = ''; | ||
| } |
There was a problem hiding this comment.
Preserve previous body overflow state when locking scroll.
This code sets document.body.style.overflow = 'hidden' during transition, but restores with ''. If another surface had already locked scroll, this can incorrectly unlock it.
Proposed fix
const wheelLockRef = React.useRef(false);
+ const previousBodyOverflowRef = React.useRef<?string>(null);
@@
- if (document.body) {
- document.body.style.overflow = '';
- }
+ if (document.body) {
+ document.body.style.overflow = previousBodyOverflowRef.current || '';
+ previousBodyOverflowRef.current = null;
+ }
@@
- if (document.body) {
- document.body.style.overflow = 'hidden';
- }
+ if (document.body) {
+ if (previousBodyOverflowRef.current === null) {
+ previousBodyOverflowRef.current = document.body.style.overflow || '';
+ }
+ document.body.style.overflow = 'hidden';
+ }
@@
- if (document.body) {
- document.body.style.overflow = '';
- }
+ if (document.body) {
+ document.body.style.overflow = previousBodyOverflowRef.current || '';
+ previousBodyOverflowRef.current = null;
+ }Also applies to: 522-524, 603-605
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx`
around lines 484 - 486, When you lock scrolling by setting
document.body.style.overflow = 'hidden' (seen in view.jsx), capture and save the
prior value (e.g., const prevOverflow = document.body?.style.overflow) before
overwriting it, and on cleanup restore document.body.style.overflow =
prevOverflow (or undefined/null-check before assigning) rather than assigning an
empty string; apply this pattern wherever you set overflow to 'hidden' (the
occurrences around the current snippet and the ones mentioned at the other
spots) and ensure the saved prevOverflow is scoped so the effect/handler cleanup
can access it.
Comment on lines
+430
to
+456
| .content__floating-close { | ||
| opacity: 0; | ||
| transition: opacity 200ms ease; | ||
| } | ||
|
|
||
| &:hover, | ||
| &.shorts-floating--paused { | ||
| .content__info--shorts-floating { | ||
| opacity: 1; | ||
| } | ||
|
|
||
| .vjs-control-bar { | ||
| opacity: 1 !important; | ||
| } | ||
|
|
||
| .content__shorts-floating-actions { | ||
| opacity: 1; | ||
| } | ||
|
|
||
| .content__shorts-floating-nav { | ||
| opacity: 1; | ||
| } | ||
|
|
||
| .content__floating-close { | ||
| opacity: 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
Close button remains hidden on touch while shorts are playing.
Line 431 forces opacity: 0, and reveal is tied to hover/paused state (Line 435-Line 437). On touch, hover is absent, so close can remain invisible during playback.
🔧 Proposed fix
.content__floating-close {
opacity: 0;
transition: opacity 200ms ease;
}
+
+ `@media` (hover: none) and (pointer: coarse) {
+ .content__floating-close {
+ opacity: 1;
+ }
+ }📝 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.
Suggested change
| .content__floating-close { | |
| opacity: 0; | |
| transition: opacity 200ms ease; | |
| } | |
| &:hover, | |
| &.shorts-floating--paused { | |
| .content__info--shorts-floating { | |
| opacity: 1; | |
| } | |
| .vjs-control-bar { | |
| opacity: 1 !important; | |
| } | |
| .content__shorts-floating-actions { | |
| opacity: 1; | |
| } | |
| .content__shorts-floating-nav { | |
| opacity: 1; | |
| } | |
| .content__floating-close { | |
| opacity: 1; | |
| } | |
| } | |
| .content__floating-close { | |
| opacity: 0; | |
| transition: opacity 200ms ease; | |
| } | |
| `@media` (hover: none) and (pointer: coarse) { | |
| .content__floating-close { | |
| opacity: 1; | |
| } | |
| } | |
| &:hover, | |
| &.shorts-floating--paused { | |
| .content__info--shorts-floating { | |
| opacity: 1; | |
| } | |
| .vjs-control-bar { | |
| opacity: 1 !important; | |
| } | |
| .content__shorts-floating-actions { | |
| opacity: 1; | |
| } | |
| .content__shorts-floating-nav { | |
| opacity: 1; | |
| } | |
| .content__floating-close { | |
| opacity: 1; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/scss/component/_content.scss` around lines 430 - 456, The close button
(.content__floating-close) is forced hidden by opacity:0 and only shown on
&:hover or .shorts-floating--paused, which fails on touch devices; update the
reveal selectors to also show the close button during touch playback by adding
the playing state selector (e.g. .shorts-floating--playing) to the same rule set
so the block that currently targets &:hover, &.shorts-floating--paused also
includes &.shorts-floating--playing (or equivalently add a rule
.shorts-floating--playing .content__floating-close { opacity: 1; }), ensuring
.content__floating-close becomes visible on touch while shorts are playing.
Comment on lines
+1
to
+117
| .shorts-transition-preview { | ||
| --shorts-preview-translate-x: calc(-50% - 6px); | ||
| position: fixed; | ||
| top: -9999px; | ||
| left: -9999px; | ||
| top: 0; | ||
| left: 50%; | ||
| width: 380px; | ||
| height: 90vh; | ||
| margin-top: 60px; | ||
| z-index: 2; | ||
| opacity: 0; | ||
| pointer-events: none; | ||
| background: #000; | ||
| background-position: center; | ||
| background-repeat: no-repeat; | ||
| background-size: cover; | ||
| overflow: hidden; | ||
| border-radius: 10px; | ||
|
|
||
| &--panel-open { | ||
| --shorts-preview-translate-x: -130%; | ||
|
|
||
| @media (max-width: $breakpoint-small) { | ||
| --shorts-preview-translate-x: 0; | ||
| } | ||
|
|
||
| @media (max-width: 1180px) and (min-width: $breakpoint-small) { | ||
| --shorts-preview-translate-x: -105%; | ||
| } | ||
| } | ||
|
|
||
| &--next { | ||
| opacity: 1; | ||
| animation: shorts-preview-next 0.32s cubic-bezier(0.4, 0, 0.2, 1) forwards; | ||
| } | ||
|
|
||
| &--previous { | ||
| opacity: 1; | ||
| animation: shorts-preview-previous 0.32s cubic-bezier(0.4, 0, 0.2, 1) forwards; | ||
| } | ||
|
|
||
| @media (min-width: $breakpoint-large) { | ||
| width: var(--shorts-viewer-width, var(--shorts-viewer-width-default)); | ||
| } | ||
|
|
||
| @media (max-width: 1380px) and (min-width: $breakpoint-small) and (max-height: 860px) { | ||
| width: 350px; | ||
| } | ||
|
|
||
| @media (max-width: $breakpoint-small) { | ||
| --shorts-preview-translate-x: 0; | ||
| left: 0; | ||
| width: 100vw; | ||
| height: 100vh; | ||
| height: 100svh; | ||
| margin-top: 0; | ||
| border-radius: 0; | ||
| } | ||
| } | ||
|
|
||
| .shorts-transition-current { | ||
| --shorts-preview-translate-x: calc(-50% - 6px); | ||
| position: fixed; | ||
| top: 0; | ||
| left: 50%; | ||
| width: 380px; | ||
| height: 90vh; | ||
| margin-top: 60px; | ||
| z-index: 2; | ||
| opacity: 0; | ||
| pointer-events: none; | ||
| background: #000; | ||
| background-position: center; | ||
| background-repeat: no-repeat; | ||
| background-size: cover; | ||
| overflow: hidden; | ||
| border-radius: 10px; | ||
| transform: translate(var(--shorts-preview-translate-x), 0); | ||
|
|
||
| &--panel-open { | ||
| --shorts-preview-translate-x: -130%; | ||
|
|
||
| @media (max-width: $breakpoint-small) { | ||
| --shorts-preview-translate-x: 0; | ||
| } | ||
|
|
||
| @media (max-width: 1180px) and (min-width: $breakpoint-small) { | ||
| --shorts-preview-translate-x: -105%; | ||
| } | ||
| } | ||
|
|
||
| &--next { | ||
| opacity: 1; | ||
| animation: shorts-current-next 0.32s cubic-bezier(0.4, 0, 0.2, 1) forwards; | ||
| } | ||
|
|
||
| &--previous { | ||
| opacity: 1; | ||
| animation: shorts-current-previous 0.32s cubic-bezier(0.4, 0, 0.2, 1) forwards; | ||
| } | ||
|
|
||
| @media (min-width: $breakpoint-large) { | ||
| width: var(--shorts-viewer-width, var(--shorts-viewer-width-default)); | ||
| } | ||
|
|
||
| @media (max-width: 1380px) and (min-width: $breakpoint-small) and (max-height: 860px) { | ||
| width: 350px; | ||
| } | ||
|
|
||
| @media (max-width: $breakpoint-small) { | ||
| --shorts-preview-translate-x: 0; | ||
| left: 0; | ||
| width: 100vw; | ||
| height: 100vh; | ||
| height: 100svh; | ||
| margin-top: 0; | ||
| border-radius: 0; | ||
| } |
There was a problem hiding this comment.
Resolve Stylelint declaration-empty-line-before failures in this block.
This section still has lint failures at Line 3, Line 51, Line 62, and Line 111. Please add the expected empty lines so Stylelint passes.
🧰 Tools
🪛 Stylelint (17.3.0)
[error] 3-3: Expected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
[error] 51-51: Expected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
[error] 62-62: Expected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
[error] 111-111: Expected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/scss/component/_shorts.scss` around lines 1 - 117, Stylelint is flagging
missing blank lines before declarations in the .shorts-transition-preview and
.shorts-transition-current blocks; open those selectors (identify by
.shorts-transition-preview and .shorts-transition-current and their nested
modifiers &--panel-open, &--next, &--previous) and insert the required empty
line(s) before the offending declarations so they comply with
declaration-empty-line-before (i.e., add a blank line before the first CSS
declaration after the selector and before the nested modifier blocks where
Stylelint expects it), then re-run linting to confirm the failures at those
selector regions are gone.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx (2)
486-488:⚠️ Potential issue | 🟠 MajorPreserve pre-existing
document.body.style.overflowwhen locking scroll.At Line 487 / Line 608, restoring to
''can incorrectly unlock scrolling that was already locked by another surface before this component.Proposed fix
const wheelLockRef = React.useRef(false); + const previousBodyOverflowRef = React.useRef<?string>(null); @@ if (document.body) { - document.body.style.overflow = ''; + document.body.style.overflow = previousBodyOverflowRef.current || ''; + previousBodyOverflowRef.current = null; } @@ if (document.body) { + if (previousBodyOverflowRef.current === null) { + previousBodyOverflowRef.current = document.body.style.overflow || ''; + } document.body.style.overflow = 'hidden'; } @@ if (document.body) { - document.body.style.overflow = ''; + document.body.style.overflow = previousBodyOverflowRef.current || ''; + previousBodyOverflowRef.current = null; }Also applies to: 524-526, 607-609
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx` around lines 486 - 488, The component currently sets document.body.style.overflow = '' when unlocking which can erroneously remove a pre-existing lock; capture and store the previous overflow value (e.g., in a module-scoped or component ref like previousBodyOverflow) at the time you set document.body.style.overflow = 'hidden' and only restore that saved value when unlocking (instead of always setting ''), ensuring you set previousBodyOverflow once when locking and clear it after restoring to avoid clobbering other consumers; update the places that set/unset document.body.style.overflow (the lock/unlock logic in the shorts view component) to use this saved value.
536-538:⚠️ Potential issue | 🟠 MajorFix claim-id extraction for channel-prefixed LBRY URIs.
At Line 537,
split('#')[1]returns the wrong fragment for URIs likelbry://@channel#abc/video#def. This can send incorrect IDs toonRecommendationClicked.Proposed fix
- const nextClaimId = activeTransition.targetUri.split('#')[1] || activeTransition.targetUri.split('/').pop(); + const nextClaimId = activeTransition.targetUri.split('#').pop() || activeTransition.targetUri.split('/').pop();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx` around lines 536 - 538, The claim-id extraction for activeTransition.targetUri is wrong for channel-prefixed LBRY URIs (e.g., "lbry://@channel#abc/video#def"); instead of using split('#')[1] which can return the wrong fragment, derive nextClaimId by taking the last fragment after the final '#' (e.g., activeTransition.targetUri.split('#').pop()) and then, if that is empty, fall back to the last '/' segment (e.g., .split('/').pop()); update the logic that sets nextClaimId (the variable referenced before calling onRecommendationClicked) to use this last-fragment-first approach so onRecommendationClicked(claimId, nextClaimId) receives the correct id.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx`:
- Around line 486-488: The component currently sets document.body.style.overflow
= '' when unlocking which can erroneously remove a pre-existing lock; capture
and store the previous overflow value (e.g., in a module-scoped or component ref
like previousBodyOverflow) at the time you set document.body.style.overflow =
'hidden' and only restore that saved value when unlocking (instead of always
setting ''), ensuring you set previousBodyOverflow once when locking and clear
it after restoring to avoid clobbering other consumers; update the places that
set/unset document.body.style.overflow (the lock/unlock logic in the shorts view
component) to use this saved value.
- Around line 536-538: The claim-id extraction for activeTransition.targetUri is
wrong for channel-prefixed LBRY URIs (e.g., "lbry://@channel#abc/video#def");
instead of using split('#')[1] which can return the wrong fragment, derive
nextClaimId by taking the last fragment after the final '#' (e.g.,
activeTransition.targetUri.split('#').pop()) and then, if that is empty, fall
back to the last '/' segment (e.g., .split('/').pop()); update the logic that
sets nextClaimId (the variable referenced before calling
onRecommendationClicked) to use this last-fragment-first approach so
onRecommendationClicked(claimId, nextClaimId) receives the correct id.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/index.jsui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
ui/scss/component/_content.scss (2)
264-264:⚠️ Potential issue | 🟡 MinorStylelint: use complex
:not()notation.This was flagged in a previous review. Update the chained
:not()selectors to use the combined Level 4 notation.- &:not(.content-info__playlist):not(.content__info--shorts-floating) { + &:not(.content-info__playlist, .content__info--shorts-floating) {,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/scss/component/_content.scss` at line 264, Replace the chained negation selectors in the rule using the Level 4 complex :not() notation: change the selector that currently uses &:not(.content-info__playlist):not(.content__info--shorts-floating) to a single :not() that lists both classes, so update the selector in _content.scss (the rule containing &:not(.content-info__playlist):not(.content__info--shorts-floating)) to use the combined :not(.content-info__playlist, .content__info--shorts-floating) form.
434-460:⚠️ Potential issue | 🟠 MajorClose button inaccessible on touch devices during playback.
This was flagged in a previous review. The close button has
opacity: 0and only becomes visible on:hoveror.shorts-floating--paused. On touch devices without hover capability, users cannot see or tap the close button while the video is playing.🔧 Proposed fix: add media query for touch devices
&:hover, &.shorts-floating--paused { .content__info--shorts-floating { opacity: 1; } // ... other rules .content__floating-close { opacity: 1; } } + + `@media` (hover: none) and (pointer: coarse) { + .content__floating-close { + opacity: 1; + } + },
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/scss/component/_content.scss` around lines 434 - 460, The close button (.content__floating-close) is hidden by default and only made visible on :hover or when .shorts-floating--paused, which breaks touch devices; add a touch-device media query (e.g. `@media` (hover: none) and/or (pointer: coarse)) and inside it set the floating-close to visible while the shorts are playing (e.g. target the floating shorts container like .shorts-floating or .shorts-floating:not(.shorts-floating--paused) to set .content__floating-close { opacity: 1 }), keeping the existing transition and not affecting desktop hover behavior.
🧹 Nitpick comments (3)
ui/scss/component/_content.scss (3)
406-437: Consolidate duplicate.content__floating-closedeclarations.There are two separate
.content__floating-closeblocks within the same parent selector (lines 406-423 and 434-437). Merging them improves maintainability.♻️ Proposed consolidation
.content__floating-close { visibility: visible; width: 2.25rem; height: 2.25rem; padding: 0; border-radius: 50%; top: var(--spacing-xxs); right: var(--spacing-xxs); display: flex; align-items: center; justify-content: center; + opacity: 0; + transition: opacity 200ms ease; .icon { width: 16px; height: 16px; stroke: white; } } - .content__floating-close { - opacity: 0; - transition: opacity 200ms ease; - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/scss/component/_content.scss` around lines 406 - 437, Merge the two duplicate .content__floating-close rules into a single block: combine the base properties (visibility, width, height, padding, border-radius, top, right, display, align-items, justify-content), the nested .icon settings (width, height, stroke), the opacity and transition (opacity: 0; transition: opacity 200ms ease), and the `@media` (max-width: $breakpoint-small) adjustments (width, height, top, left, transform, border-radius) under the same .content__floating-close selector; remove the second standalone .content__floating-close rule and ensure the media query is placed either inside that consolidated block or targets .content__floating-close so behavior is preserved.
610-612: Consider avoiding!importantif possible.The
!importanton.button-bubble--activesuggests a specificity conflict. If feasible, increase selector specificity or refactor the competing rule to avoid!importantusage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/scss/component/_content.scss` around lines 610 - 612, The .button-bubble--active rule currently uses !important to override a competing style; remove the !important and instead raise selector specificity or refactor the competing rule: for example, replace `.button-bubble--active` with a more specific selector (e.g., a parent + class combo or `.button-bubble.button-bubble--active`) or locate and weaken the conflicting rule so the active state can be applied without !important; update the SCSS accordingly and verify the active background uses var(--color-primary) as intended.
63-64: Consider browser support for:has()pseudo-class.The
:has()selector requires Chrome 105+, Firefox 121+, and Safari 15.4+. If the project needs to support older browsers, consider adding a fallback or a JavaScript-based alternative for detecting the paused state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/scss/component/_content.scss` around lines 63 - 64, The selector using :has(.vjs-paused) may break in older browsers; replace this dependency by either adding a fallback CSS rule and a feature-query guard or by using a JS-driven class toggle. Concretely, remove reliance on &:has(.vjs-paused) in _content.scss and instead wrap the :has rule in an `@supports`(selector(:has(*))) block or alternatively style the same hover/paused state via a class like .is-paused on the parent (toggle .is-paused in your player code), updating selectors referenced in this file accordingly (e.g., replace &:has(.vjs-paused) with .is-paused or the `@supports` guarded rule and ensure the player toggles the .is-paused class when paused).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/scss/component/_file-render.scss`:
- Around line 917-919: The rule .vjs-button--play-previous:first-child currently
forces margin-left with !important which overrides the small-screen reset;
remove the !important (or move this rule inside the same desktop-only media
query) so the existing mobile reset for the first previous button can take
effect and avoid squeezed controls — update the selector
.vjs-button--play-previous:first-child accordingly and ensure its desktop margin
is applied only at larger breakpoints.
---
Duplicate comments:
In `@ui/scss/component/_content.scss`:
- Line 264: Replace the chained negation selectors in the rule using the Level 4
complex :not() notation: change the selector that currently uses
&:not(.content-info__playlist):not(.content__info--shorts-floating) to a single
:not() that lists both classes, so update the selector in _content.scss (the
rule containing
&:not(.content-info__playlist):not(.content__info--shorts-floating)) to use the
combined :not(.content-info__playlist, .content__info--shorts-floating) form.
- Around line 434-460: The close button (.content__floating-close) is hidden by
default and only made visible on :hover or when .shorts-floating--paused, which
breaks touch devices; add a touch-device media query (e.g. `@media` (hover: none)
and/or (pointer: coarse)) and inside it set the floating-close to visible while
the shorts are playing (e.g. target the floating shorts container like
.shorts-floating or .shorts-floating:not(.shorts-floating--paused) to set
.content__floating-close { opacity: 1 }), keeping the existing transition and
not affecting desktop hover behavior.
---
Nitpick comments:
In `@ui/scss/component/_content.scss`:
- Around line 406-437: Merge the two duplicate .content__floating-close rules
into a single block: combine the base properties (visibility, width, height,
padding, border-radius, top, right, display, align-items, justify-content), the
nested .icon settings (width, height, stroke), the opacity and transition
(opacity: 0; transition: opacity 200ms ease), and the `@media` (max-width:
$breakpoint-small) adjustments (width, height, top, left, transform,
border-radius) under the same .content__floating-close selector; remove the
second standalone .content__floating-close rule and ensure the media query is
placed either inside that consolidated block or targets .content__floating-close
so behavior is preserved.
- Around line 610-612: The .button-bubble--active rule currently uses !important
to override a competing style; remove the !important and instead raise selector
specificity or refactor the competing rule: for example, replace
`.button-bubble--active` with a more specific selector (e.g., a parent + class
combo or `.button-bubble.button-bubble--active`) or locate and weaken the
conflicting rule so the active state can be applied without !important; update
the SCSS accordingly and verify the active background uses var(--color-primary)
as intended.
- Around line 63-64: The selector using :has(.vjs-paused) may break in older
browsers; replace this dependency by either adding a fallback CSS rule and a
feature-query guard or by using a JS-driven class toggle. Concretely, remove
reliance on &:has(.vjs-paused) in _content.scss and instead wrap the :has rule
in an `@supports`(selector(:has(*))) block or alternatively style the same
hover/paused state via a class like .is-paused on the parent (toggle .is-paused
in your player code), updating selectors referenced in this file accordingly
(e.g., replace &:has(.vjs-paused) with .is-paused or the `@supports` guarded rule
and ensure the player toggles the .is-paused class when paused).
Comment on lines
+917
to
+919
| .vjs-button--play-previous:first-child { | ||
| margin-left: var(--spacing-xs) !important; | ||
| } |
There was a problem hiding this comment.
Preserve the mobile margin reset for the first previous button.
Line 918 uses !important, which overrides the existing small-screen reset (Line 882-Line 884) and can squeeze controls on narrow widths.
Suggested fix
.vjs-button--play-previous:first-child {
- margin-left: var(--spacing-xs) !important;
+ margin-left: var(--spacing-xs);
+ `@media` (max-width: $breakpoint-small) {
+ margin-left: 0 !important;
+ }
}📝 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.
Suggested change
| .vjs-button--play-previous:first-child { | |
| margin-left: var(--spacing-xs) !important; | |
| } | |
| .vjs-button--play-previous:first-child { | |
| margin-left: var(--spacing-xs); | |
| `@media` (max-width: $breakpoint-small) { | |
| margin-left: 0 !important; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/scss/component/_file-render.scss` around lines 917 - 919, The rule
.vjs-button--play-previous:first-child currently forces margin-left with
!important which overrides the small-screen reset; remove the !important (or
move this rule inside the same desktop-only media query) so the existing mobile
reset for the first previous button can take effect and avoid squeezed controls
— update the selector .vjs-button--play-previous:first-child accordingly and
ensure its desktop margin is applied only at larger breakpoints.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx (1)
486-487:⚠️ Potential issue | 🟠 MajorPreserve previous
body.style.overflowwhen locking scroll.Line 487, Line 525, and Line 609 still restore overflow with
'', which can incorrectly unlock scroll if another surface already locked it.💡 Proposed fix
@@ - const wheelLockRef = React.useRef(false); + const wheelLockRef = React.useRef(false); + const previousBodyOverflowRef = React.useRef<?string>(null); @@ if (document.body) { - document.body.style.overflow = ''; + document.body.style.overflow = previousBodyOverflowRef.current || ''; + previousBodyOverflowRef.current = null; } @@ if (document.body) { + if (previousBodyOverflowRef.current === null) { + previousBodyOverflowRef.current = document.body.style.overflow || ''; + } document.body.style.overflow = 'hidden'; } @@ if (document.body) { - document.body.style.overflow = ''; + document.body.style.overflow = previousBodyOverflowRef.current || ''; + previousBodyOverflowRef.current = null; }Also applies to: 524-526, 608-610
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx` around lines 486 - 487, When you lock the page scroll you must save the previous document.body.style.overflow and restore that exact value instead of setting it to an empty string; modify the places that set document.body.style.overflow = '' (the three occurrences that restore overflow) to restore a previously-captured variable (e.g., prevBodyOverflow) saved when you set document.body.style.overflow = 'hidden'. Add a module-/component-scoped variable (like prevBodyOverflow) and update the code paths that enable/disable scroll (where overflow is set to 'hidden') to set prevBodyOverflow = document.body?.style.overflow before changing it and to use document.body.style.overflow = prevBodyOverflow when unlocking in the three locations currently using ''. Ensure null/undefined handling so restoring when prevBodyOverflow is undefined leaves the style unset.
🧹 Nitpick comments (1)
ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx (1)
566-569: Consider bounding the transition queue to avoid stale backlog navigation.Line 567 always appends; rapid wheel input can outpace transition processing and replay stale direction events after user input stops.
💡 Proposed refactor
@@ const REEL_TRANSITION_MS = 320; const REEL_NAVIGATION_FALLBACK_MS = 1200; +const MAX_REEL_QUEUE_SIZE = 2; @@ const queueTransition = React.useCallback((direction: ReelDirection) => { - transitionQueueRef.current.push(direction); + if (transitionQueueRef.current.length >= MAX_REEL_QUEUE_SIZE) { + transitionQueueRef.current[transitionQueueRef.current.length - 1] = direction; + } else { + transitionQueueRef.current.push(direction); + } processNextTransitionRef.current(); }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx` around lines 566 - 569, queueTransition is unbounded and can accumulate stale directions when rapid input outpaces processing; to fix, add a small fixed cap (e.g. MAX_TRANSITION_QUEUE = 3) and enforce it inside queueTransition before pushing to transitionQueueRef.current: if the queue is already at the cap, either drop the oldest entry (shift) or replace the last pending direction so only recent inputs remain, then call processNextTransitionRef.current(); update any related assumptions in processNextTransitionRef and transitionQueueRef usage to handle the capped queue.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx`:
- Around line 486-487: When you lock the page scroll you must save the previous
document.body.style.overflow and restore that exact value instead of setting it
to an empty string; modify the places that set document.body.style.overflow = ''
(the three occurrences that restore overflow) to restore a previously-captured
variable (e.g., prevBodyOverflow) saved when you set
document.body.style.overflow = 'hidden'. Add a module-/component-scoped variable
(like prevBodyOverflow) and update the code paths that enable/disable scroll
(where overflow is set to 'hidden') to set prevBodyOverflow =
document.body?.style.overflow before changing it and to use
document.body.style.overflow = prevBodyOverflow when unlocking in the three
locations currently using ''. Ensure null/undefined handling so restoring when
prevBodyOverflow is undefined leaves the style unset.
---
Nitpick comments:
In
`@ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx`:
- Around line 566-569: queueTransition is unbounded and can accumulate stale
directions when rapid input outpaces processing; to fix, add a small fixed cap
(e.g. MAX_TRANSITION_QUEUE = 3) and enforce it inside queueTransition before
pushing to transitionQueueRef.current: if the queue is already at the cap,
either drop the oldest entry (shift) or replace the last pending direction so
only recent inputs remain, then call processNextTransitionRef.current(); update
any related assumptions in processNextTransitionRef and transitionQueueRef usage
to handle the capped queue.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsxui/page/claim/internal/claimPageComponent/internal/streamClaimPage/view.jsx
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.
Improve shorts player
Summary by CodeRabbit
New Features
Bug Fixes
Style