Fix chart indicators disappearing in fullscreen#67
Fix chart indicators disappearing in fullscreen#67softsideof wants to merge 2 commits intoOpen-Dev-Society:mainfrom
Conversation
|
@keshav-005 is attempting to deploy a commit to the ravixalgorithm's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR removes height management from the TradingView hook and component: the hook signature drops the height parameter and stops producing height-based markup; the component moves height styling to its outer container, removes an inner wrapper, and calls the hook with (scriptUrl, widgetConfig) only. Config is serialized once in the hook. Changes
Sequence Diagram(s)(Skipped — changes are refactor/logic simplification, not a new multi-component control flow.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/TradingViewWidget.tsx (1)
18-31:⚠️ Potential issue | 🟡 MinorInitial render has
currentHeight = 0until post-mount effect fires.
windowHeightis initialized to0, so on the first client render the chart container getsstyle={{ height: 0 }}before theuseEffectupdates it. Since the TradingView embed runs inside this same container withheight: 100%, users will see a zero-height flash, and in StrictMode the widget may also initialize against a zero-height parent.Two simple options:
♻️ Fallback to `height` prop until window size is known
- const currentHeight = isExpanded ? windowHeight : height; + const currentHeight = isExpanded ? (windowHeight || height) : height;Alternatively, lazy-initialize state from
window:- const [windowHeight, setWindowHeight] = useState(0); - - useEffect(() => { - if (typeof window !== 'undefined') { - setWindowHeight(window.innerHeight); - const handleResize = () => setWindowHeight(window.innerHeight); - window.addEventListener('resize', handleResize); - return () => window.removeEventListener('resize', handleResize); - } - }, []); + const [windowHeight, setWindowHeight] = useState(() => + typeof window !== 'undefined' ? window.innerHeight : height + ); + + useEffect(() => { + const handleResize = () => setWindowHeight(window.innerHeight); + window.addEventListener('resize', handleResize); + return () => window.removeEventListener('resize', handleResize); + }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/TradingViewWidget.tsx` around lines 18 - 31, TradingViewWidget currently initializes windowHeight to 0 causing currentHeight to be 0 on first render; fix by ensuring windowHeight is seeded or currentHeight falls back to the height prop until the effect runs — e.g. lazy-initialize windowHeight via useState(() => typeof window !== 'undefined' ? window.innerHeight : height) or compute currentHeight as isExpanded ? (windowHeight || height) : height; update references to windowHeight/currentHeight and keep the existing useEffect and setWindowHeight logic.
🧹 Nitpick comments (1)
components/TradingViewWidget.tsx (1)
65-69: Consider100vhin expanded mode rather than a px value.When expanded,
currentHeight = windowHeight(px) is applied to afixed inset-0subtree that already stretches to the viewport. Using100vh(or lettingh-fullwin by not setting inlineheight) is more robust against mobile URL-bar resize, avoids one extra render after theresizelistener fires, and removes the need to trackwindowHeightat all for the fullscreen case.- <div - className={cn('tradingview-widget-container', className, isExpanded && "h-full")} - ref={containerRef} - style={{ height: currentHeight, width: "100%" }} - /> + <div + className={cn('tradingview-widget-container', className, isExpanded && "h-full")} + ref={containerRef} + style={{ height: isExpanded ? '100vh' : height, width: "100%" }} + />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/TradingViewWidget.tsx` around lines 65 - 69, The inline style always sets height: currentHeight (px) even when isExpanded is true; change the TradingViewWidget render so the containerRef div does not set a pixel height when isExpanded — either set style.height to '100vh' when isExpanded or omit the height key entirely so the 'h-full' class can control sizing. Update the JSX around the div that uses className, isExpanded, currentHeight and containerRef to conditionally apply the inline height only for non-expanded mode and remove reliance on windowHeight for the fullscreen case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@components/TradingViewWidget.tsx`:
- Around line 18-31: TradingViewWidget currently initializes windowHeight to 0
causing currentHeight to be 0 on first render; fix by ensuring windowHeight is
seeded or currentHeight falls back to the height prop until the effect runs —
e.g. lazy-initialize windowHeight via useState(() => typeof window !==
'undefined' ? window.innerHeight : height) or compute currentHeight as
isExpanded ? (windowHeight || height) : height; update references to
windowHeight/currentHeight and keep the existing useEffect and setWindowHeight
logic.
---
Nitpick comments:
In `@components/TradingViewWidget.tsx`:
- Around line 65-69: The inline style always sets height: currentHeight (px)
even when isExpanded is true; change the TradingViewWidget render so the
containerRef div does not set a pixel height when isExpanded — either set
style.height to '100vh' when isExpanded or omit the height key entirely so the
'h-full' class can control sizing. Update the JSX around the div that uses
className, isExpanded, currentHeight and containerRef to conditionally apply the
inline height only for non-expanded mode and remove reliance on windowHeight for
the fullscreen case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ce9fb56-8af2-48e0-8e04-7cecf3b6aa8f
📒 Files selected for processing (2)
components/TradingViewWidget.tsxhooks/useTradingViewWidget.tsx
|
Thanks, updated. Fullscreen now uses viewport height, so the widget no longer initializes against a zero-height container. |
|
Closing this in favor of #68 so the contribution is tracked under my main account. |
|
Superseded by #68, which is the same fix reopened from my main account. Please use that PR for review. |
2 participants
Summary
Test plan
Fixes #49
Summary by CodeRabbit