-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(maxun-core): add multiple scroll approach #760
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughReplaced bottom-scroll and height-detection logic in maxun-core/src/interpret.ts to compute page height using Math.max(document.body.scrollHeight, document.documentElement.scrollHeight) across main and pagination paths. Updated currentHeight helpers accordingly. No exported signatures changed; control flow remains the same. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
@coderabbitai
coderabbitai
bot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
maxun-core/src/interpret.ts (1)
781-801: scrollUp path also needs container-aware fallback and measurementwindow.scrollTo(0, 0) + documentElement.scrollTop will fail on inner scrollers (e.g., YouTube). Mirror the multi-approach logic and read the active scroller’s scrollTop.
- await page.evaluate(() => window.scrollTo(0, 0)); + await page.evaluate(() => { + if (typeof window.scrollUp === 'function') { + window.scrollUp(1); + return; + } + const main = document.scrollingElement || document.documentElement || document.body; + if (typeof main.scrollTo === 'function') main.scrollTo(0, 0); + else (main as any).scrollTop = 0; + const scrollables = Array.from(document.querySelectorAll('*')) as HTMLElement[]; + const best = scrollables + .filter(el => { + const s = getComputedStyle(el); + return (s.overflowY === 'auto' || s.overflowY === 'scroll') && el.scrollHeight > el.clientHeight; + }) + .sort((a, b) => b.scrollHeight - a.scrollHeight)[0]; + if (best) { + if (typeof (best as any).scrollTo === 'function') (best as any).scrollTo(0, 0); + else (best as any).scrollTop = 0; + } + }); - const currentTopHeight = await page.evaluate(() => document.documentElement.scrollTop); + const currentTopHeight = await page.evaluate(() => { + const main = document.scrollingElement || document.documentElement || document.body; + const scrollables = Array.from(document.querySelectorAll('*')) as HTMLElement[]; + const best = scrollables + .filter(el => { + const s = getComputedStyle(el); + return (s.overflowY === 'auto' || s.overflowY === 'scroll') && el.scrollHeight > el.clientHeight; + }) + .sort((a, b) => b.scrollHeight - a.scrollHeight)[0]; + const el = (best ?? main) as HTMLElement; + return el.scrollTop; + });
🧹 Nitpick comments (2)
maxun-core/src/interpret.ts (2)
514-519: Unify "scroll" action with the new multi-approachKeep the action consistent with pagination paths so user-invoked scroll benefits from the fallback too.
- await page.evaluate(async (pagesInternal) => { - for (let i = 1; i <= (pagesInternal ?? 1); i += 1) { - // @ts-ignore - window.scrollTo(0, window.scrollY + window.innerHeight); - } - }, pages ?? 1); + await page.evaluate(async (pagesInternal) => { + const times = pagesInternal ?? 1; + for (let i = 0; i < times; i += 1) { + if (typeof window.scrollDown === 'function') { + window.scrollDown(1); + continue; + } + const main = document.scrollingElement || document.documentElement || document.body; + if (typeof main.scrollBy === 'function') main.scrollBy(0, main.clientHeight); + else (main as any).scrollTop += (main as any).clientHeight || window.innerHeight; + } + }, pages ?? 1);
737-769: Replace fixed 2s sleeps after scrolls withwaitForFunctionDetected three occurrences of
await page.waitForTimeout(2000)immediately afterwindow.scrollToin maxun-core/src/interpret.ts:
- lines 743–745
- lines 781–783
- lines 1045–1047
Consider replacing each with something like:
- await page.waitForTimeout(2000); + await page.waitForFunction(() => { + const el = (document.scrollingElement || document.documentElement || document.body) as HTMLElement; + return el.scrollTop + el.clientHeight >= el.scrollHeight - 2; + }, { timeout: 3000 }).catch(() => {});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
maxun-core/src/interpret.ts(2 hunks)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Still relying on window.scrollTo; add real fallback or delegate to window.scrollDown to meet PR objective
On sites like youtube.com the scrollable container isn’t window; this will no-op. Either call the injected window.scrollDown (which can encapsulate multi-approach logic) or implement a container-aware fallback here.
Apply one of the following diffs (Option A preferred):
Option A — delegate to injected helper:
- await page.evaluate(() => { - const scrollHeight = Math.max( - document.body.scrollHeight, - document.documentElement.scrollHeight - ); - - window.scrollTo(0, scrollHeight); - }); + await page.evaluate(() => { + if (typeof window.scrollDown === 'function') { + window.scrollDown(1); + return; + } + const main = document.scrollingElement || document.documentElement || document.body; + if (typeof main.scrollTo === 'function') main.scrollTo(0, main.scrollHeight); + else (main as any).scrollTop = main.scrollHeight; + const scrollables = Array.from(document.querySelectorAll('*')) as HTMLElement[]; + const best = scrollables + .filter(el => { + const s = getComputedStyle(el); + return (s.overflowY === 'auto' || s.overflowY === 'scroll') && el.scrollHeight > el.clientHeight; + }) + .sort((a, b) => b.scrollHeight - a.scrollHeight)[0]; + if (best) { + if (typeof (best as any).scrollTo === 'function') (best as any).scrollTo(0, best.scrollHeight); + else (best as any).scrollTop = best.scrollHeight; + } + });
📝 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.
🤖 Prompt for AI Agents
In maxun-core/src/interpret.ts around lines 737 to 745, the code uses
window.scrollTo which no-ops on sites where the scrollable container is not
window (e.g., YouTube); replace the direct window.scrollTo call with a call to
the injected helper window.scrollDown (preferred) so the page-level script can
choose the correct container and perform multi-step scrolling, or implement a
container-aware fallback here by locating the nearest scrollable element
(checking document.scrollingElement, document.body, and overflowed descendants)
and scrolling that element to its scrollHeight instead; ensure you await the
evaluated helper and keep the waitForTimeout(2000) after the delegated/fallback
scroll.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Height check ignores inner scroll containers; track the active scroller’s height
Comparing only body/documentElement heights will prematurely stop on pages with inner scrolling containers.
Apply:
- const currentHeight = await page.evaluate(() => { - return Math.max( - document.body.scrollHeight, - document.documentElement.scrollHeight - ); - }); + const currentHeight = await page.evaluate(() => { + const main = document.scrollingElement || document.documentElement || document.body; + const scrollables = Array.from(document.querySelectorAll('*')) as HTMLElement[]; + const best = scrollables + .filter(el => { + const s = getComputedStyle(el); + return (s.overflowY === 'auto' || s.overflowY === 'scroll') && el.scrollHeight > el.clientHeight; + }) + .sort((a, b) => b.scrollHeight - a.scrollHeight)[0]; + const el = (best ?? main) as HTMLElement; + return el.scrollHeight; + });
📝 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Load-more flow: same window.scrollTo limitation — use helper/fallback scrolling
Mirror the scrollDown fix here; otherwise "Load more" pages with inner scrollers won’t advance.
- await page.evaluate(() => { - const scrollHeight = Math.max( - document.body.scrollHeight, - document.documentElement.scrollHeight - ); - - window.scrollTo(0, scrollHeight); - }); + await page.evaluate(() => { + if (typeof window.scrollDown === 'function') { + window.scrollDown(1); + return; + } + const main = document.scrollingElement || document.documentElement || document.body; + if (typeof main.scrollTo === 'function') main.scrollTo(0, main.scrollHeight); + else (main as any).scrollTop = main.scrollHeight; + const scrollables = Array.from(document.querySelectorAll('*')) as HTMLElement[]; + const best = scrollables + .filter(el => { + const s = getComputedStyle(el); + return (s.overflowY === 'auto' || s.overflowY === 'scroll') && el.scrollHeight > el.clientHeight; + }) + .sort((a, b) => b.scrollHeight - a.scrollHeight)[0]; + if (best) { + if (typeof (best as any).scrollTo === 'function') (best as any).scrollTo(0, best.scrollHeight); + else (best as any).scrollTop = best.scrollHeight; + } + });
🤖 Prompt for AI Agents
In maxun-core/src/interpret.ts around lines 1040 to 1046, the code calls
window.scrollTo(0, scrollHeight) which fails for pages that use inner scrollable
containers; replace this direct window.scrollTo with the same scrollDown
helper/fallback used elsewhere (or implement the equivalent: detect the deepest
scrollable element, scroll that element to its max scrollTop, and fall back to
window.scrollTo if none found). Ensure you call or await the helper correctly
and remove the direct window.scrollTo call so "Load more" flows advance inner
scrollers as well as the window.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Load-more height measurement should use the active scroller
Align measurement with the scrolling target to avoid false "no more items" detections.
- const currentHeight = await page.evaluate(() => { - return Math.max( - document.body.scrollHeight, - document.documentElement.scrollHeight - ); - }); + const currentHeight = await page.evaluate(() => { + const main = document.scrollingElement || document.documentElement || document.body; + const scrollables = Array.from(document.querySelectorAll('*')) as HTMLElement[]; + const best = scrollables + .filter(el => { + const s = getComputedStyle(el); + return (s.overflowY === 'auto' || s.overflowY === 'scroll') && el.scrollHeight > el.clientHeight; + }) + .sort((a, b) => b.scrollHeight - a.scrollHeight)[0]; + const el = (best ?? main) as HTMLElement; + return el.scrollHeight; + });
📝 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.
🤖 Prompt for AI Agents
In maxun-core/src/interpret.ts around lines 1049 to 1054, the height measurement
uses document.body/documentElement which can mismatch the actual scrolling
container; change the evaluation to measure the active scroller (use
document.scrollingElement if available or the specific scroller element used for
scroll actions). Update the page.evaluate to query the active scroller (falling
back to document.scrollingElement or a known scroller selector), and return its
scrollHeight (or bounding height) instead of
document.body/document.documentElement so load-more detection aligns with the
element being scrolled.
Uh oh!
There was an error while loading. Please reload this page.
What this PR does?
Brings in support for one more scroll approach just in case the original one fails as it seems to be not supported for certain sites like www.youtube.com
Fixes: #759
Summary by CodeRabbit