Short player: Fit to content on desktop + some minor stuff#3394
Short player: Fit to content on desktop + some minor stuff#3394tzarebczan merged 6 commits intomasterfrom
Conversation
Updated shorts stuff to use the global constants (hopefully) everywhere
📝 WalkthroughWalkthroughReplaces hardcoded Shorts thresholds with settings-driven constants; consolidates some imports; refactors several resize handlers to stable callbacks; adds responsive Shorts viewer width calculation (polls for video metadata) that sets a CSS variable; shifts styling breakpoints and replaces fixed 28vw with the CSS variable. Changes
Sequence Diagram(s)mermaid Note over ShortsView,Video: On uri change or window resize Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/component/portals/view.jsx (1)
58-69: Add null safety check for portals.mainPortal.Line 62 accesses
portals.mainPortal.portals.lengthwithout verifying thatportals.mainPortalexists. Ifportalsis defined butmainPortalis undefined, this will throw a runtime error.Note that line 33 uses optional chaining (
portals?.mainPortal?.portals?.length), indicating thatmainPortalcan be undefined.🛡️ Proposed fix with defensive null check
const handleResize = React.useCallback(() => { if (wrapper.current) { let wrapperWidth = wrapper.current.offsetWidth + 12; let tileNum = wrapperWidth > 954 ? 6 : wrapperWidth > 870 ? 5 : wrapperWidth > 470 ? 3 : 2; - if (tileNum === 6 && portals.mainPortal.portals.length < 9) { + if (tileNum === 6 && portals?.mainPortal?.portals && portals.mainPortal.portals.length < 9) { tileNum = portals.mainPortal.portals.length; } setWidth(wrapperWidth); setTileNum(tileNum); setTileWidth(wrapperWidth / tileNum); } }, [portals]);
🤖 Fix all issues with AI agents
In @ui/component/shortsActions/swipeNavigation/style.scss:
- Line 28: The fallback for the CSS variable is missing a unit: update the
declaration `width: var(--shorts-viewer-width, 28) !important;` to use a united
fallback such as `28vw` so it reads `width: var(--shorts-viewer-width, 28vw)
!important;`, referencing the `--shorts-viewer-width` variable and that specific
width declaration in style.scss.
In @ui/constants/settings.js:
- Line 57: The SHORTS_ASPECT_RATIO_LTE constant was changed from '0.95' to
'0.7', which will exclude videos with aspect ratios in (0.7,0.95) from all
shorts logic; confirm whether this is intentional with product—if not, revert
SHORTS_ASPECT_RATIO_LTE back to '0.95'; if intentional, update all dependent
code and documentation (uses in ui/util/claim.js, ui/redux/selectors/claims.js
and search/discovery components), adjust any tests or fixtures that assume the
old threshold, and add a short comment near SHORTS_ASPECT_RATIO_LTE explaining
the product rationale for the new threshold.
In
@ui/page/claim/internal/claimPageComponent/internal/channelPage/tabs/contentTab/view.jsx:
- Around line 204-206: The maxDuration prop passed to SearchResults is receiving
SETTINGS.SHORTS_DURATION_LTE (a string) but SearchResults expects ?number;
locate where maxDuration is set (prop name maxDuration on the SearchResults
component) and either convert the value to a number before passing (e.g.,
parseInt of SETTINGS.SHORTS_DURATION_LTE) or change the SearchResults prop type
to accept ?string | ?number for consistency with maxAspectRatio; ensure you
update all usages of maxDuration or the prop type definition in the
SearchResults component accordingly.
In @ui/scss/component/_shorts.scss:
- Line 253: The CSS variable --shorts-viewer-width is set dynamically in
view.jsx but not removed on navigation; update the navigation cleanup inside the
history.listen effect where doClearShortsPlaylist() is called (the
history.listen cleanup handler in view.jsx) to also call
document.documentElement?.style?.removeProperty('--shorts-viewer-width') so the
CSS variable is cleared when leaving the shorts player.
In @ui/util/claim.js:
- Around line 233-240: The comparisons in isShortDuration and isVertical use
string constants (SETTINGS.SHORTS_DURATION_LTE and
SETTINGS.SHORTS_ASPECT_RATIO_LTE) against numeric values (media.duration and
aspectRatio), which is error-prone; fix by converting the SETTINGS values to
numbers before comparison (e.g., wrap with Number(...) at the use site) or
change the constants to numeric types, and ensure you perform
Number(SETTINGS.SHORTS_DURATION_LTE) when computing isShortDuration and
Number(SETTINGS.SHORTS_ASPECT_RATIO_LTE) when computing isVertical so
media.duration and aspectRatio are compared as numbers.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
ui/component/claimPreview/view.jsxui/component/claimTilesDiscover/index.jsui/component/common/expandable.jsxui/component/common/tabs.jsxui/component/portals/view.jsxui/component/shortsActions/swipeNavigation/style.scssui/component/videoRenderFloating/index.jsui/constants/settings.jsui/effects/use-on-resize.jsui/page/claim/internal/claimPageComponent/internal/channelPage/tabs/contentTab/view.jsxui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/index.jsui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsxui/redux/selectors/claims.jsui/scss/component/_content.scssui/scss/component/_shorts.scssui/util/claim.jsui/util/search.js
🧰 Additional context used
🧬 Code graph analysis (7)
ui/effects/use-on-resize.js (3)
ui/component/common/expandable.jsx (1)
handleResize(52-54)ui/component/common/tabs.jsx (2)
handleResize(52-56)handleResize(106-110)ui/component/portals/view.jsx (1)
handleResize(58-69)
ui/component/common/expandable.jsx (3)
ui/component/common/tabs.jsx (3)
handleResize(52-56)handleResize(106-110)ref(114-114)ui/component/portals/view.jsx (1)
handleResize(58-69)ui/effects/use-on-resize.js (1)
useOnResize(4-23)
ui/util/claim.js (3)
ui/redux/selectors/claims.js (2)
isShortDuration(377-377)isVertical(378-378)ui/component/claimPreviewTile/index.js (1)
media(24-24)ui/component/claimPreview/index.js (1)
media(32-32)
ui/redux/selectors/claims.js (1)
ui/util/claim.js (2)
isShortDuration(233-233)isVertical(240-240)
ui/component/portals/view.jsx (3)
ui/component/common/expandable.jsx (1)
handleResize(52-54)ui/component/common/tabs.jsx (2)
handleResize(52-56)handleResize(106-110)ui/effects/use-on-resize.js (1)
useOnResize(4-23)
ui/component/common/tabs.jsx (3)
ui/component/common/expandable.jsx (2)
handleResize(52-54)ref(21-21)ui/component/portals/view.jsx (1)
handleResize(58-69)ui/effects/use-on-resize.js (1)
useOnResize(4-23)
ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx (1)
ui/effects/use-on-resize.js (1)
useOnResize(4-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint
🔇 Additional comments (20)
ui/component/claimTilesDiscover/index.js (1)
193-193: LGTM: Documentation update aligns with settings refactor.The comment now correctly references
SETTINGS.SHORTS_DURATION_LTEinstead of the deprecatedSHORTS_DURATION_LIMIT, maintaining consistency with the broader PR changes.ui/component/videoRenderFloating/index.js (1)
22-22: LGTM: Import consolidation improves maintainability.The change to an absolute import path aligns with the PR's goal of consolidating imports and improves code maintainability.
ui/component/claimPreview/view.jsx (1)
12-12: LGTM: Consistent import path refactoring.The change to an absolute import path from
util/claimaligns with the codebase-wide consolidation effort and improves maintainability.ui/component/shortsActions/swipeNavigation/style.scss (1)
27-27: Verify responsive breakpoint change doesn't affect UX.The breakpoint was reduced from
1600pxto1150px, which means the dynamic width behavior will now apply to smaller screens. Please verify that this breakpoint change provides the intended user experience across different screen sizes and doesn't cause layout issues.ui/util/search.js (1)
152-152: The aspect ratio threshold of0.7is properly centralized inSETTINGS.SHORTS_ASPECT_RATIO_LTEand used consistently across the recommendation, filtering, and detection systems throughout the codebase (13+ usages across query-params, claim detection, search, and channel content tabs). An aspect ratio of0.7correctly identifies portrait-oriented content (height at least 1.43x the width), which is appropriate for Shorts. The change from a hardcoded value to a configurable setting follows best practices and enables consistent behavior across the application.ui/scss/component/_content.scss (1)
849-854: LGTM! Dynamic width control via CSS variable.The shift from
$breakpoint-largeto$breakpoint-mediumand the introduction ofvar(--shorts-viewer-width, 28vw)enables JavaScript-driven responsive width adjustments while maintaining a sensible fallback.ui/effects/use-on-resize.js (2)
7-12: LGTM! Initial resize calculation on mount.The immediate invocation ensures sizing calculations run on mount, complementing the window resize listener.
22-22: Good addition ofcbto dependencies.Including
cbin the dependency array ensures the resize listener updates when the callback changes. This works correctly because all callers (tabs.jsx, expandable.jsx, portals/view.jsx) now pass memoized callbacks viaReact.useCallback.ui/component/common/tabs.jsx (2)
52-57: LGTM! Memoized resize handler.The stable callback reference prevents unnecessary re-registration of the resize listener.
106-111: LGTM! Consistent memoization pattern.Matches the pattern established in the parent
Tabscomponent.ui/component/common/expandable.jsx (1)
52-55: LGTM! Proper callback memoization.The memoized
handleResizewithexpandableRefdependency ensures a stable reference foruseOnResize, preventing unnecessary listener re-registration.ui/redux/selectors/claims.js (2)
30-30: LGTM! Settings import for configurable thresholds.Importing SETTINGS constants enables centralized configuration of shorts detection criteria.
377-379: The aspect ratio analysis is incorrect—the change actually restricts the shorts definition, not expands it.The current code with
SHORTS_ASPECT_RATIO_LTE = '0.7'does not allow square (1:1) or near-square (4:5) videos; it requires videos narrower than ~1:1.43. The recent change from 0.95 to 0.7 threshold is more restrictive, not more permissive. The code correctly consolidates shorts detection into centralized constants and is consistently used throughout the codebase.Likely an incorrect or invalid review comment.
ui/scss/component/_shorts.scss (1)
251-263: Breakpoint change aligns with intended responsive design.The change from
$breakpoint-large(1600px) to$breakpoint-medium(1150px) is intentional and part of a coordinated responsive design update. The CSS variable--shorts-viewer-widthis dynamically computed in JavaScript based on video dimensions, allowing flexible layout adjustment. This approach extends the positioning logic to a broader range of screen sizes starting at 1150px, which is the intended behavior.ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/index.js (4)
29-37: LGTM! Import paths refactored to absolute paths.The consolidation from relative to absolute import paths improves maintainability and aligns with the PR's objective of refactoring shorts-related functionality.
56-59: LGTM! Query key construction now uses settings constants.The aspect ratio filter now correctly references
SETTINGS.SHORTS_ASPECT_RATIO_LTE, ensuring consistency with the centralized settings approach.
74-75: LGTM! Search parameters now driven by settings.The duration and aspect ratio filters correctly use
SETTINGS.SHORTS_DURATION_LTEandSETTINGS.SHORTS_ASPECT_RATIO_LTE, replacing hardcoded values and centralizing configuration.
170-171: LGTM! Channel shorts fetching aligned with settings.The
doFetchChannelShortsfunction now correctly applies the same settings-driven filters, ensuring consistent behavior across all shorts-related queries.ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx (2)
17-17: LGTM! Import added for resize handling.The
useOnResizehook import is appropriate for implementing responsive width calculations.
173-173: LGTM! Resize handler integration looks correct.The
useOnResizehook is properly used with the memoized callback. The hook will update the resize listener whensetShortViewerWidthFromVideochanges (whensidePanelOpenchanges).
ui/constants/settings.js
Outdated
| export const AUTOPLAY_NEXT_SHORTS = 'autoplay_next_shorts'; | ||
| export const SHORTS_ASPECT_RATIO_LIMIT = '1'; | ||
| export const SHORTS_ASPECT_RATIO_LTE = '0.95'; | ||
| export const SHORTS_ASPECT_RATIO_LTE = '0.7'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the current state of the constants file
cat -n ui/constants/settings.js | grep -A 2 -B 2 "SHORTS_ASPECT_RATIO"Repository: OdyseeTeam/odysee-frontend
Length of output: 425
🏁 Script executed:
# Find all usages of SHORTS_ASPECT_RATIO_LTE
rg -n "SHORTS_ASPECT_RATIO_LTE" --type=js --type=jsx -C 3Repository: OdyseeTeam/odysee-frontend
Length of output: 97
🏁 Script executed:
# Search for aspect ratio logic in the codebase
rg -n "aspectRatio" --type=js --type=jsx -C 2 | head -100Repository: OdyseeTeam/odysee-frontend
Length of output: 97
🏁 Script executed:
# Check git history for this file to see if value changed from 0.95
cd ui/constants && git log -p --follow settings.js | grep -A 5 -B 5 "SHORTS_ASPECT_RATIO_LTE" | head -50Repository: OdyseeTeam/odysee-frontend
Length of output: 52
🏁 Script executed:
# Find all usages of SHORTS_ASPECT_RATIO_LTE
rg -n "SHORTS_ASPECT_RATIO_LTE" -C 3Repository: OdyseeTeam/odysee-frontend
Length of output: 9451
🏁 Script executed:
# Search for aspect ratio comparisons
rg -n "aspectRatio.*<=" -C 2Repository: OdyseeTeam/odysee-frontend
Length of output: 317
🏁 Script executed:
# Check if there are any short-related files that use aspect ratio logic
fd -e js -e ts | xargs rg -l "aspectRatio|SHORTS" | head -20Repository: OdyseeTeam/odysee-frontend
Length of output: 920
🏁 Script executed:
# Search for any mention of 0.95 in relation to aspect ratio
rg -n "0\.95" -C 3Repository: OdyseeTeam/odysee-frontend
Length of output: 7774
🏁 Script executed:
# Check git blame for this specific line
git blame ui/constants/settings.js | grep "SHORTS_ASPECT_RATIO_LTE"Repository: OdyseeTeam/odysee-frontend
Length of output: 161
🏁 Script executed:
# Look at recent commits affecting this file
git log --oneline -20 -- ui/constants/settings.jsRepository: OdyseeTeam/odysee-frontend
Length of output: 188
Confirm this significant aspect ratio threshold change is intentional.
The aspect ratio limit was changed from '0.95' to '0.7', making the shorts filter substantially more restrictive. This change affects video classification across the codebase (ui/util/claim.js, ui/redux/selectors/claims.js, and multiple search/discovery components) and will exclude videos with aspect ratios between 0.7 and 0.95 from shorts feeds.
Verify this aligns with current product requirements.
🤖 Prompt for AI Agents
In @ui/constants/settings.js at line 57, The SHORTS_ASPECT_RATIO_LTE constant
was changed from '0.95' to '0.7', which will exclude videos with aspect ratios
in (0.7,0.95) from all shorts logic; confirm whether this is intentional with
product—if not, revert SHORTS_ASPECT_RATIO_LTE back to '0.95'; if intentional,
update all dependent code and documentation (uses in ui/util/claim.js,
ui/redux/selectors/claims.js and search/discovery components), adjust any tests
or fixtures that assume the old threshold, and add a short comment near
SHORTS_ASPECT_RATIO_LTE explaining the product rationale for the new threshold.
| duration: `<=${SETTINGS.SHORTS_DURATION_LTE}`, | ||
| contentType: CS.FILE_VIDEO, | ||
| contentAspectRatio: `<${SETTINGS.SHORTS_ASPECT_RATIO_LTE}`, | ||
| contentAspectRatio: `<=${SETTINGS.SHORTS_ASPECT_RATIO_LTE}`, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the exact file location
find . -name "view.jsx" -path "*contentTab*" 2>/dev/nullRepository: OdyseeTeam/odysee-frontend
Length of output: 159
🏁 Script executed:
#!/bin/bash
# Search for SETTINGS definitions related to shorts
rg -n "SHORTS_DURATION_LTE|SHORTS_ASPECT_RATIO_LTE" --type=js --type=jsx -B 2 -A 2Repository: OdyseeTeam/odysee-frontend
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Find ClaimListDiscover component definition
rg -n "ClaimListDiscover" --type=js --type=jsx | head -20Repository: OdyseeTeam/odysee-frontend
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Find SearchResults component definition
rg -n "SearchResults" --type=js --type=jsx | head -20Repository: OdyseeTeam/odysee-frontend
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Read the view.jsx file to see lines 204-206 and 247-248
wc -l ./ui/page/claim/internal/claimPageComponent/internal/channelPage/tabs/contentTab/view.jsxRepository: OdyseeTeam/odysee-frontend
Length of output: 163
🏁 Script executed:
#!/bin/bash
# Read relevant lines from view.jsx
sed -n '200,210p' ./ui/page/claim/internal/claimPageComponent/internal/channelPage/tabs/contentTab/view.jsxRepository: OdyseeTeam/odysee-frontend
Length of output: 582
🏁 Script executed:
#!/bin/bash
# Read lines around 247-248
sed -n '240,255p' ./ui/page/claim/internal/claimPageComponent/internal/channelPage/tabs/contentTab/view.jsxRepository: OdyseeTeam/odysee-frontend
Length of output: 708
🏁 Script executed:
#!/bin/bash
# Search for SETTINGS constants (without type flag)
rg -n "SHORTS_DURATION_LTE|SHORTS_ASPECT_RATIO_LTE" -B 2 -A 2Repository: OdyseeTeam/odysee-frontend
Length of output: 9030
🏁 Script executed:
#!/bin/bash
# Search for ClaimListDiscover or SearchResults usage without type flag
rg -n "ClaimListDiscover|SearchResults" -A 5 | head -40Repository: OdyseeTeam/odysee-frontend
Length of output: 2584
🏁 Script executed:
#!/bin/bash
# Find ClaimListDiscover component and check prop handling
rg -n "maxDuration|maxAspectRatio" ui/component/claimListDiscover/ -A 3 -B 1Repository: OdyseeTeam/odysee-frontend
Length of output: 1053
🏁 Script executed:
#!/bin/bash
# Read ClaimListDiscover view.jsx to understand prop handling
head -100 ui/component/claimListDiscover/view.jsxRepository: OdyseeTeam/odysee-frontend
Length of output: 3565
🏁 Script executed:
#!/bin/bash
# Find the SearchResults component definition in contentTab
rg -n "export.*SearchResults|const SearchResults" ui/page/claim/internal/claimPageComponent/internal/channelPage/tabs/contentTab/Repository: OdyseeTeam/odysee-frontend
Length of output: 224
🏁 Script executed:
#!/bin/bash
# Get more context on how contentAspectRatio and duration are processed
rg -n "contentAspectRatio|duration" ui/component/claimListDiscover/view.jsx -B 2 -A 2 | head -60Repository: OdyseeTeam/odysee-frontend
Length of output: 1752
🏁 Script executed:
#!/bin/bash
# Check the SearchResults component props definition
sed -n '1,50p' ui/page/claim/internal/claimPageComponent/internal/channelPage/tabs/contentTab/internal/searchResults.jsxRepository: OdyseeTeam/odysee-frontend
Length of output: 1477
🏁 Script executed:
#!/bin/bash
# Search for maxDuration and maxAspectRatio as props in components
rg -n "maxDuration|maxAspectRatio" ui/component/ -B 2 -A 2Repository: OdyseeTeam/odysee-frontend
Length of output: 5024
🏁 Script executed:
#!/bin/bash
# Check if ClaimListDiscover has any other prop handling we might have missed
sed -n '100,200p' ui/component/claimListDiscover/view.jsxRepository: OdyseeTeam/odysee-frontend
Length of output: 2466
🏁 Script executed:
#!/bin/bash
# Search for SearchResults component implementation
cat ui/page/claim/internal/claimPageComponent/internal/channelPage/tabs/contentTab/internal/searchResults.jsxRepository: OdyseeTeam/odysee-frontend
Length of output: 4504
Fix type mismatch for maxDuration prop passed to SearchResults.
Lines 247-248 pass SETTINGS.SHORTS_DURATION_LTE (string '180') to the maxDuration prop, which expects ?number. The maxAspectRatio prop correctly accepts ?string | ?number, so that assignment is compatible.
To fix the type error, either convert the string to a number when passing it:
maxDuration: parseInt(SETTINGS.SHORTS_DURATION_LTE, 10)Or update the SearchResults prop type definition to accept ?string | ?number for consistency with maxAspectRatio.
Lines 204-206 are correct—the string templates for API query parameters are appropriate.
🤖 Prompt for AI Agents
In
@ui/page/claim/internal/claimPageComponent/internal/channelPage/tabs/contentTab/view.jsx
around lines 204 - 206, The maxDuration prop passed to SearchResults is
receiving SETTINGS.SHORTS_DURATION_LTE (a string) but SearchResults expects
?number; locate where maxDuration is set (prop name maxDuration on the
SearchResults component) and either convert the value to a number before passing
(e.g., parseInt of SETTINGS.SHORTS_DURATION_LTE) or change the SearchResults
prop type to accept ?string | ?number for consistency with maxAspectRatio;
ensure you update all usages of maxDuration or the prop type definition in the
SearchResults component accordingly.
| const setShortViewerWidthFromVideo = React.useCallback(() => { | ||
| const videoElement = document.querySelector('video'); | ||
| const videoW = videoElement?.videoWidth; | ||
| const videoH = videoElement?.videoHeight; | ||
| if (!videoW || !videoH) return; | ||
|
|
||
| const maxHeight = window.innerHeight * 0.9; | ||
|
|
||
| const scale = maxHeight / videoH; | ||
| const computedWidthPx = videoW * scale; | ||
|
|
||
| // Convert to vw (viewport width %) | ||
| const maxWidth = sidePanelOpen ? 30 : 80; // in percentages | ||
| const widthVW = (computedWidthPx / window.innerWidth) * 100; | ||
| const clampedVW = Math.min(widthVW, maxWidth); // Avoid overflow | ||
|
|
||
| requestAnimationFrame(() => { | ||
| // $FlowFixMe | ||
| document.documentElement?.style?.setProperty('--shorts-viewer-width', `${clampedVW}vw`); | ||
| }); | ||
| }, [sidePanelOpen]); |
There was a problem hiding this comment.
Scope the video selector to prevent selecting wrong elements.
Line 152 uses document.querySelector('video'), which could select any video element on the page, not just the shorts video. If multiple videos exist (e.g., ads, other embedded content), this will select the first one in DOM order, leading to incorrect width calculations.
🔍 Recommended fix: scope the selector to the shorts container
const setShortViewerWidthFromVideo = React.useCallback(() => {
- const videoElement = document.querySelector('video');
+ const videoElement = shortsContainerRef.current?.querySelector('video');
const videoW = videoElement?.videoWidth;
const videoH = videoElement?.videoHeight;
if (!videoW || !videoH) return;Alternatively, use a more specific selector:
- const videoElement = document.querySelector('video');
+ const videoElement = document.querySelector('.shorts__viewer video, .vjs-tech');Extract magic numbers as named constants.
Lines 157, 163 contain magic numbers (0.9, 30, 80) that should be extracted as named constants for clarity and maintainability.
♻️ Suggested refactor
+const MAX_HEIGHT_VIEWPORT_RATIO = 0.9;
+const MAX_WIDTH_WITH_PANEL = 30;
+const MAX_WIDTH_WITHOUT_PANEL = 80;
+
const setShortViewerWidthFromVideo = React.useCallback(() => {
const videoElement = shortsContainerRef.current?.querySelector('video');
const videoW = videoElement?.videoWidth;
const videoH = videoElement?.videoHeight;
if (!videoW || !videoH) return;
- const maxHeight = window.innerHeight * 0.9;
+ const maxHeight = window.innerHeight * MAX_HEIGHT_VIEWPORT_RATIO;
const scale = maxHeight / videoH;
const computedWidthPx = videoW * scale;
// Convert to vw (viewport width %)
- const maxWidth = sidePanelOpen ? 30 : 80; // in percentages
+ const maxWidth = sidePanelOpen ? MAX_WIDTH_WITH_PANEL : MAX_WIDTH_WITHOUT_PANEL;
const widthVW = (computedWidthPx / window.innerWidth) * 100;
const clampedVW = Math.min(widthVW, maxWidth); // Avoid overflow
requestAnimationFrame(() => {
- // $FlowFixMe
document.documentElement?.style?.setProperty('--shorts-viewer-width', `${clampedVW}vw`);
});
}, [sidePanelOpen]);Committable suggestion skipped: line range outside the PR's diff.
| React.useEffect(() => { | ||
| let timeoutId; | ||
| function loop() { | ||
| const video = document.querySelector('video'); | ||
| if (!video || !video.videoWidth || !video.videoHeight) { | ||
| timeoutId = setTimeout(loop, 300); | ||
| return; | ||
| } | ||
| setShortViewerWidthFromVideo(); | ||
| } | ||
|
|
||
| timeoutId = setTimeout(loop, 300); | ||
|
|
||
| return () => clearTimeout(timeoutId); | ||
| }, [uri, setShortViewerWidthFromVideo]); |
There was a problem hiding this comment.
Apply the same selector scoping fix here.
Line 351 has the same unscoped document.querySelector('video') issue as line 152. This should be fixed to scope the selector to the shorts container or use a more specific selector.
Consider using video events instead of polling.
The current polling approach (lines 348-362) works but is not optimal. Using the video element's loadedmetadata event would be more efficient and precise.
♻️ Suggested event-based approach
React.useEffect(() => {
- let timeoutId;
- function loop() {
- const video = document.querySelector('video');
- if (!video || !video.videoWidth || !video.videoHeight) {
- timeoutId = setTimeout(loop, 300);
- return;
- }
- setShortViewerWidthFromVideo();
- }
-
- timeoutId = setTimeout(loop, 300);
-
- return () => clearTimeout(timeoutId);
+ // Wait for video element to appear
+ const checkInterval = setInterval(() => {
+ const video = shortsContainerRef.current?.querySelector('video');
+ if (video) {
+ clearInterval(checkInterval);
+
+ const handleLoadedMetadata = () => {
+ setShortViewerWidthFromVideo();
+ };
+
+ // If metadata already loaded
+ if (video.videoWidth && video.videoHeight) {
+ setShortViewerWidthFromVideo();
+ } else {
+ video.addEventListener('loadedmetadata', handleLoadedMetadata, { once: true });
+ }
+ }
+ }, 100);
+
+ return () => clearInterval(checkInterval);
}, [uri, setShortViewerWidthFromVideo]);This approach:
- Still polls for video element existence (unavoidable since React may not have rendered it yet)
- Uses the standard
loadedmetadataevent instead of polling for dimensions - Handles the case where metadata is already loaded
- Uses a shorter polling interval (100ms) since we're only checking element existence
Committable suggestion skipped: line range outside the PR's diff.
| @media (min-width: $breakpoint-medium) { | ||
| .shorts-page__info-button.shorts-page-menu__button { | ||
| right: calc(49.8% - 28vw / 2 - 60px + 7.5px); | ||
| right: calc(49.8% - var(--shorts-viewer-width, 28vw) / 2 - 60px + 7.5px); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for where the CSS variable --shorts-viewer-width is being set in JavaScript/TypeScript files
# Search for direct references to the variable name
rg -n "shorts-viewer-width" --type=js --type=jsx --type=ts --type=tsx -C 3
# Also search for setProperty calls that might set CSS variables
rg -n "setProperty.*shorts|shorts.*setProperty" --type=js --type=jsx --type=ts --type=tsx -C 3Repository: OdyseeTeam/odysee-frontend
Length of output: 125
🏁 Script executed:
#!/bin/bash
# Search for the CSS variable in any file type
rg -n "shorts-viewer-width" -C 3
# Search for setProperty calls related to CSS variables
rg -n "setProperty.*--" --type=js --type=ts -C 2 | head -100
# Also check if there's any reference to setting CSS custom properties
rg -n "style\.setProperty" --type=js --type=ts -C 2 | head -100Repository: OdyseeTeam/odysee-frontend
Length of output: 18375
🏁 Script executed:
#!/bin/bash
# Get the full context of the shorts view.jsx file where the variable is set
sed -n '160,180p' ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx
# Also check if there's any cleanup or removal of the variable
rg -n "removeProperty.*shorts|shorts.*removeProperty" --type=ts --type=js
# Search for variable cleanup in the shorts component
rg -n "setProperty.*--shorts-viewer-width|removeProperty" ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx -A 5Repository: OdyseeTeam/odysee-frontend
Length of output: 2026
🏁 Script executed:
#!/bin/bash
# Look at the full useEffect that sets the variable
sed -n '155,175p' ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx
# Search for any cleanup functions or unmount handlers in the component
grep -n "useEffect\|cleanup\|return\s*().*=>" ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx | head -30
# Check if there's any unmount handling that clears CSS variables
rg -n "removeProperty\|removeAttribute" ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsxRepository: OdyseeTeam/odysee-frontend
Length of output: 1398
🏁 Script executed:
#!/bin/bash
# Get more context around the setProperty call to see the full useEffect
sed -n '150,180p' ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx
# Look for the function name that contains setProperty
rg -B 10 "setProperty.*--shorts-viewer-width" ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx | head -20Repository: OdyseeTeam/odysee-frontend
Length of output: 1648
🏁 Script executed:
#!/bin/bash
# Look for where setShortViewerWidthFromVideo is called or if there's any cleanup
sed -n '1,250p' ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx | grep -n "setShortViewerWidthFromVideo\|useEffect\|return ()"
# Check the end of the component for any cleanup or unmount handlers
tail -100 ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx | grep -n "useEffect\|return ()\|removeProperty"
# Look for the full component export to see if there's any cleanup
rg -A 5 "export.*ShortsPage|export default" ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsxRepository: OdyseeTeam/odysee-frontend
Length of output: 358
🏁 Script executed:
#!/bin/bash
# Search for any cleanup that might remove the CSS variable when leaving shorts
rg -n "componentWillUnmount|useEffect.*return|useEffect.*\[\]" ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx | head -20
# Check if there's a cleanup function that removes document styles
rg -B 5 -A 5 "return ()" ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx | grep -A 5 -B 5 "removeProperty\|--shorts"
# Look for the file size and check the actual end of the component
wc -l ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx
# Check the last 50 lines to see if there's any cleanup before export
tail -50 ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsxRepository: OdyseeTeam/odysee-frontend
Length of output: 1892
🏁 Script executed:
#!/bin/bash
# Look for all useEffect hooks in the file to find any with cleanup functions
rg -B 2 "React.useEffect.*\[\]" ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx
# Check if setShortViewerWidthFromVideo is called in any useEffect with cleanup
sed -n '145,230p' ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx
# Look for any useEffect that might clean up the CSS variable
rg -B 3 -A 8 "React.useEffect\(\(\)" ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx | head -80Repository: OdyseeTeam/odysee-frontend
Length of output: 5481
🏁 Script executed:
#!/bin/bash
# Look for the history.listen cleanup function to see if it clears CSS variables
sed -n '295,330p' ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx
# Check doClearShortsPlaylist to see if it clears the CSS variable
rg -n "doClearShortsPlaylist" ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx -A 2
# Search for the definition of doClearShortsPlaylist in the codebase
rg -n "doClearShortsPlaylist" -A 10 --type=js --type=tsRepository: OdyseeTeam/odysee-frontend
Length of output: 10882
🏁 Script executed:
#!/bin/bash
# Check if doClearShortsPlaylist removes the CSS variable
sed -n '28,32p' ui/redux/actions/shorts.js
# Search for where doClearShortsPlaylist is handled in reducers
rg -n "CLEAR_SHORTS_PLAYLIST" --type=js --type=ts -A 5
# Check if the CSS variable is explicitly removed anywhere in the shorts reducer or component
rg -n "removeProperty.*--shorts-viewer-width|documentElement.*removeProperty" --type=js --type=tsRepository: OdyseeTeam/odysee-frontend
Length of output: 1182
Add cleanup to remove the CSS variable when navigating away from shorts.
The CSS variable is correctly set dynamically based on video dimensions in view.jsx line 169 with proper fallback handling. However, it is never removed when navigating away from the shorts player. While doClearShortsPlaylist() clears the Redux state in the navigation cleanup handler (lines 295–346), it does not remove the --shorts-viewer-width property from document.documentElement.style.
Add document.documentElement?.style?.removeProperty('--shorts-viewer-width') to the cleanup function in the history.listen effect to prevent the CSS variable from persisting in the document after the user leaves the shorts view.
🤖 Prompt for AI Agents
In @ui/scss/component/_shorts.scss at line 253, The CSS variable
--shorts-viewer-width is set dynamically in view.jsx but not removed on
navigation; update the navigation cleanup inside the history.listen effect where
doClearShortsPlaylist() is called (the history.listen cleanup handler in
view.jsx) to also call
document.documentElement?.style?.removeProperty('--shorts-viewer-width') so the
CSS variable is cleared when leaving the shorts player.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx (1)
151-173: Good improvement on selector scoping.The selector has been improved to use
.shorts__vieweras a scope, which addresses the major concern from the previous review about selecting the wrong video element. The instanceof check on line 154 adds additional type safety.The magic numbers (0.9, 30, 80) on lines 159 and 165 remain in the code. Extracting these as named constants would improve maintainability.
♻️ Optional: Extract magic numbers as constants
+const MAX_HEIGHT_VIEWPORT_RATIO = 0.9; +const MAX_WIDTH_WITH_PANEL = 30; +const MAX_WIDTH_WITHOUT_PANEL = 80; + const setShortViewerWidthFromVideo = React.useCallback(() => { // $FlowFixMe const video = document.querySelector('.shorts__viewer')?.querySelector('video'); if (!(video instanceof HTMLVideoElement)) return; const videoW = video?.videoWidth; const videoH = video?.videoHeight; if (!videoW || !videoH) return; - const maxHeight = window.innerHeight * 0.9; + const maxHeight = window.innerHeight * MAX_HEIGHT_VIEWPORT_RATIO; const scale = maxHeight / videoH; const computedWidthPx = videoW * scale; // Convert to vw (viewport width %) - const maxWidth = sidePanelOpen ? 30 : 80; // in percentages + const maxWidth = sidePanelOpen ? MAX_WIDTH_WITH_PANEL : MAX_WIDTH_WITHOUT_PANEL; const widthVW = (computedWidthPx / window.innerWidth) * 100; const clampedVW = Math.min(widthVW, maxWidth); // Avoid overflow requestAnimationFrame(() => { // $FlowFixMe document.documentElement?.style?.setProperty('--shorts-viewer-width', `${clampedVW}vw`); }); }, [sidePanelOpen]);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx
🧰 Additional context used
🧬 Code graph analysis (1)
ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx (3)
ui/component/videoRenderFloating/view.jsx (1)
maxHeight(700-702)ui/effects/use-on-resize.js (1)
useOnResize(4-23)ui/component/videoRenderFloating/index.js (1)
uri(44-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint
🔇 Additional comments (3)
ui/page/claim/internal/claimPageComponent/internal/streamClaimPage/internal/shorts/view.jsx (3)
17-17: LGTM: Clean import addition.The
useOnResizehook import is correctly added and necessary for the new responsive width calculation feature.
175-175: LGTM: Correct hook usage.The
useOnResizehook is correctly used with the memoized callback, ensuring the shorts viewer width is recalculated on mount and resize events.
367-369: LGTM: Correct reactive behavior.This effect ensures the shorts viewer width is recalculated immediately when the side panel state changes, maintaining the correct max width constraint (30% when open, 80% when closed).
2 participants
Summary by CodeRabbit
New Features
Improvements
Refactor
Style
✏️ Tip: You can customize this high-level summary in your review settings.