fix: Scrolling to timeslot issue on Safari with inline-embeds #22547
Conversation
WalkthroughThis update introduces new utilities and logic to improve scrolling behavior, especially for embedded content and Safari browsers within iframes. A cross-browser Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. All modifications relate to improving scrolling behavior, particularly addressing Safari iframe issues and embed scrolling, consistent with the linked issue objectives. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/embeds/embed-core/src/ModalBox/ModalBox.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-playwright". (The package "eslint-plugin-playwright" was not found when loaded as a Node module from the directory "/packages/embeds".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-playwright" was referenced from the config file in "packages/embeds/.eslintrc.js » ../../.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. packages/embeds/embed-core/src/embed-iframe/lib/embedStore.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-playwright". (The package "eslint-plugin-playwright" was not found when loaded as a Node module from the directory "/packages/embeds".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-playwright" was referenced from the config file in "packages/embeds/.eslintrc.js » ../../.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. packages/embeds/embed-core/src/Inline/inline.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-playwright". (The package "eslint-plugin-playwright" was not found when loaded as a Node module from the directory "/packages/embeds".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-playwright" was referenced from the config file in "packages/embeds/.eslintrc.js » ../../.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team.
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
|
✅ No security or compliance issues detected. Reviewed everything up to 13d764a. Security Overview
Detected Code Changes
Reply to this PR with |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/lib/browser/browser.utils.ts (1)
26-30: Consider more robust Safari detection approach.The current user agent string parsing approach works but has limitations:
- User agent strings can be spoofed or modified
- Some Safari-based browsers might not be detected correctly
- The logic assumes Chrome-based browsers always include "chrome" in UA
Consider a more robust detection method:
export const isSafariBrowser = (): boolean => { if (typeof window === "undefined") return false; - const ua = navigator.userAgent.toLowerCase(); - return ua.includes("safari") && !ua.includes("chrome"); + // More robust Safari detection using vendor and other properties + const ua = navigator.userAgent.toLowerCase(); + const vendor = navigator.vendor?.toLowerCase() || ""; + const isSafari = ua.includes("safari") && !ua.includes("chrome") && !ua.includes("chromium"); + const isAppleWebKit = vendor.includes("apple"); + return isSafari && isAppleWebKit; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/embeds/embed-core/index.html(1 hunks)packages/embeds/embed-core/playground/lib/playground.ts(2 hunks)packages/embeds/embed-core/src/Inline/inline.ts(1 hunks)packages/embeds/embed-core/src/ModalBox/ModalBox.ts(1 hunks)packages/embeds/embed-core/src/embed-iframe/lib/embedStore.ts(1 hunks)packages/embeds/embed-core/src/embed.ts(3 hunks)packages/embeds/embed-core/src/lib/domUtils.test.ts(1 hunks)packages/embeds/embed-core/src/lib/domUtils.ts(1 hunks)packages/embeds/embed-core/src/sdk-action-manager.ts(1 hunks)packages/eslint-plugin/src/rules/no-scroll-into-view-embed.ts(2 hunks)packages/features/bookings/Booker/Booker.tsx(2 hunks)packages/lib/browser/browser.utils.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/features/bookings/Booker/Booker.tsx (1)
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.341Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
🧬 Code Graph Analysis (3)
packages/embeds/embed-core/playground/lib/playground.ts (1)
packages/embeds/embed-core/src/embed.ts (1)
Cal(217-838)
packages/features/bookings/Booker/Booker.tsx (1)
packages/lib/browser/browser.utils.ts (1)
scrollIntoViewSmooth(60-83)
packages/embeds/embed-core/src/embed.ts (1)
packages/embeds/embed-core/src/lib/domUtils.ts (1)
getScrollableAncestor(1-29)
⏰ 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: Security Check
🔇 Additional comments (30)
packages/embeds/embed-core/src/ModalBox/ModalBox.ts (1)
2-2: LGTM - consistent import path refactoring.The import path update aligns with the broader refactoring to organize utilities under the
libdirectory, maintaining consistency across the embed-core package.packages/embeds/embed-core/src/Inline/inline.ts (1)
2-2: LGTM - consistent import path refactoring.The import path update maintains consistency with the utility organization changes across the embed-core package.
packages/embeds/embed-core/src/embed-iframe/lib/embedStore.ts (1)
1-1: LGTM - consistent import path refactoring.The import path update correctly adjusts the relative path for this file's location while maintaining consistency with the utility organization changes.
packages/embeds/embed-core/src/sdk-action-manager.ts (1)
114-119: LGTM - well-structured event type for scroll functionality.The new
__scrollByDistanceevent type follows existing patterns with proper documentation and appropriate typing for the distance property.packages/embeds/embed-core/playground/lib/playground.ts (2)
291-291: LGTM - consistent namespace prefix requirement.The condition change to require "ns:" prefix aligns with the naming convention used throughout the playground for namespaced embed instances.
696-728: LGTM - well-structured namespaces for scroll testing.The new namespaces
containerScrollToTimeslotandwindowScrollToTimeslotfollow the established pattern with proper configuration including debug mode, origin, iframe attributes, and COEP flags. These appear to be specifically designed for testing scroll functionality in different contexts.packages/eslint-plugin/src/rules/no-scroll-into-view-embed.ts (3)
10-10: LGTM - Rule description updated correctly.The description accurately reflects that the rule now covers both scrollIntoView and scrollIntoViewSmooth methods.
15-15: LGTM - Error message updated appropriately.The error message correctly mentions both methods and maintains clarity for developers.
27-39: LGTM - Logic correctly extended to handle both methods.The condition properly checks for both
scrollIntoViewandscrollIntoViewSmoothin both member expressions and direct identifier calls. The implementation is consistent and comprehensive.packages/features/bookings/Booker/Booker.tsx (2)
16-16: LGTM - Import updated to use cross-browser utility.The import change from native scrollIntoView to scrollIntoViewSmooth aligns with the Safari embed scroll fix improvements.
160-162: LGTM - Function updated with proper null check and Safari workaround.The changes improve the implementation by:
- Adding defensive null check for
timeslotsRef.current- Using
scrollIntoViewSmoothwithisEmbedparameter for Safari iframe scroll workarounds- Maintaining the same behavior for the scrolling logic
The ESLint disable comment correctly acknowledges this is intentional user-action triggered scrolling.
packages/embeds/embed-core/src/lib/domUtils.ts (1)
1-29: LGTM - Well-implemented scrollable ancestor utility.The function correctly implements the logic to find the nearest scrollable ancestor:
✅ Proper traversal: Starts from parent element and walks up the DOM tree
✅ Comprehensive checks: Validates bothoverflow-yandoverflowproperties
✅ Content verification: Ensures element actually has scrollable content
✅ Edge case handling: Stops at document.documentElement and handles null scrollingElement
✅ Appropriate fallback: Returns document.scrollingElement when no scrollable ancestor is foundThe implementation is efficient and handles the expected use cases for embed scroll management.
packages/embeds/embed-core/index.html (2)
402-404: LGTM - Updated container description and added missing element.The description update correctly mentions the window scroll testing functionality, and the missing
<div class="place"></div>element is properly added.
406-413: LGTM - New scroll test containers added appropriately.The new containers provide comprehensive test scenarios:
windowScrollToTimeslot: Tests window scrolling with fixed width and vertical scrollcontainerScrollToTimeslot: Tests container scrolling with fixed height/width and vertical scrollThe structure is consistent with existing containers and includes proper namespace prefixes and descriptive headings.
packages/embeds/embed-core/src/embed.ts (3)
13-26: LGTM - Import reorganization improves code structure.The imports are properly grouped and organized without changing functionality.
476-486: LGTM - Event listener properly handles scroll events for inline embeds.The implementation correctly:
- Checks for inline embed context (
this.inlineEl) before processing- Logs appropriate warning for non-inline embeds
- Extracts distance from event detail and delegates to scrollByDistance method
- Includes helpful comment explaining the iframe height behavior for inline embeds
516-528: LGTM - ScrollByDistance method implements smooth scrolling correctly.The method properly:
- Guards against missing iframe
- Uses
getScrollableAncestorto find the appropriate scroll container- Calculates new scroll position relative to current position
- Performs smooth scrolling with appropriate behavior
The comment explaining on-demand computation of scrollable ancestor is helpful and justified.
packages/lib/browser/browser.utils.ts (4)
1-1: LGTM! Clean import for embed integration.The import statement correctly pulls in the
sdkActionManagerneeded for iframe communication.
36-42: LGTM! Clean iframe communication implementation.The function correctly:
- Gets the element's position relative to the viewport
- Fires the appropriate event to the parent frame
- Uses the top position for scroll distance calculation
44-52: LGTM! Elegant paint cycle utility.The recursive
requestAnimationFrameimplementation is clean and handles the timing correctly for detecting scroll completion.
60-83: Well-designed Safari iframe scrolling workaround.The implementation correctly:
- Attempts native scrolling first
- Detects if scrolling occurred by comparing positions
- Falls back to parent frame communication for Safari iframes
- Uses appropriate timing with paint cycle detection
The logic flow and edge case handling look solid.
packages/embeds/embed-core/src/lib/domUtils.test.ts (9)
5-34: LGTM! Well-designed mock element factory.The
createMockElementfunction provides a clean abstraction for creating DOM elements with configurable properties. The use ofObject.definePropertycorrectly sets up non-enumerable properties that match DOM behavior.
36-47: LGTM! Flexible style mocking implementation.The
mockGetComputedStylefunction provides a clean way to mock CSS computed styles with element-specific overrides. The fallback to "visible" is appropriate for the test scenarios.
54-85: LGTM! Proper test lifecycle management.The setup and teardown logic correctly:
- Preserves original DOM and window properties
- Resets state between tests
- Restores original values after all tests
- Uses proper cleanup with
vi.restoreAllMocks()
88-152: LGTM! Comprehensive overflow property testing.The test cases correctly verify all scrollable overflow combinations:
overflow-y: autoandoverflow-y: scrolloverflow: autoandoverflow: scroll- Proper priority handling between specific and general overflow properties
The test setup and assertions are accurate.
176-207: LGTM! Excellent DOM traversal testing.The test correctly verifies that the function skips non-scrollable ancestors and finds the correct scrollable grandparent, testing the core traversal logic.
210-265: LGTM! Thorough edge case coverage.The tests correctly handle elements that have scrollable CSS properties but no actual scrollable content (
scrollHeight <= clientHeight). This is crucial for the function's correctness.
267-326: LGTM! Proper fallback behavior testing.The tests correctly verify:
- Fallback to
document.scrollingElementwhen no scrollable ancestors exist- Handling of
nulldocument.scrollingElementgracefully- Return value consistency with expected behavior
368-409: LGTM! Comprehensive overflow value matrix testing.The parameterized test approach efficiently verifies all combinations of
overflow-yandoverflowvalues, ensuring the function correctly interprets CSS overflow properties.
412-462: LGTM! Deep DOM tree traversal testing.The test correctly verifies that the function can traverse complex, deeply nested DOM structures to find the correct scrollable ancestor, ensuring robustness in real-world scenarios.
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (07/16/25)1 reviewer was added to this PR based on Keith Williams's automation. |
9ce2936 to
fbb2d5f
Compare
E2E results are ready! |
fbb2d5f to
1c76635
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/embeds/embed-core/src/lib/eventHandlers/scrollByDistanceEventHandler.ts (1)
14-14: Consider adding error handling for the scrollByDistance call.The
scrollByDistancefunction is called without any error handling. If this function throws an error, it could break the event handling flow.Consider wrapping the call in a try-catch block:
- cal.scrollByDistance(distanceRelativeToIframe); + try { + cal.scrollByDistance(distanceRelativeToIframe); + } catch (error) { + console.error('Error during scrollByDistance:', error); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/embeds/embed-core/index.html(1 hunks)packages/embeds/embed-core/src/Inline/inline.ts(1 hunks)packages/embeds/embed-core/src/ModalBox/ModalBox.ts(1 hunks)packages/embeds/embed-core/src/__tests__/utils.test.ts(1 hunks)packages/embeds/embed-core/src/embed-iframe/lib/embedStore.ts(1 hunks)packages/embeds/embed-core/src/embed.ts(3 hunks)packages/embeds/embed-core/src/lib/domUtils.test.ts(1 hunks)packages/embeds/embed-core/src/lib/domUtils.ts(1 hunks)packages/embeds/embed-core/src/lib/eventHandlers/scrollByDistanceEventHandler.ts(1 hunks)packages/embeds/embed-core/src/lib/utils.ts(1 hunks)packages/embeds/embed-core/src/sdk-action-manager.ts(1 hunks)packages/eslint-plugin/src/rules/no-scroll-into-view-embed.ts(2 hunks)packages/features/bookings/Booker/Booker.tsx(2 hunks)packages/lib/browser/browser.utils.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/embeds/embed-core/src/tests/utils.test.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/embeds/embed-core/src/ModalBox/ModalBox.ts
- packages/embeds/embed-core/src/Inline/inline.ts
- packages/embeds/embed-core/src/lib/utils.ts
- packages/embeds/embed-core/src/embed-iframe/lib/embedStore.ts
- packages/features/bookings/Booker/Booker.tsx
- packages/embeds/embed-core/src/sdk-action-manager.ts
- packages/eslint-plugin/src/rules/no-scroll-into-view-embed.ts
- packages/embeds/embed-core/src/embed.ts
- packages/embeds/embed-core/src/lib/domUtils.ts
- packages/embeds/embed-core/index.html
- packages/lib/browser/browser.utils.ts
- packages/embeds/embed-core/src/lib/domUtils.test.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/embeds/embed-core/src/lib/eventHandlers/scrollByDistanceEventHandler.ts (1)
packages/embeds/embed-core/src/sdk-action-manager.ts (1)
EmbedEvent(131-131)
⏰ 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: Security Check
🔇 Additional comments (1)
packages/embeds/embed-core/src/lib/eventHandlers/scrollByDistanceEventHandler.ts (1)
3-15: Well-structured event handler with clear separation of concerns.The factory function pattern is appropriate here, and the logic correctly differentiates between inline and non-inline embed scenarios. The comments provide excellent context for the implementation decisions.
packages/embeds/embed-core/src/lib/eventHandlers/scrollByDistanceEventHandler.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Refactors and core logic in PR look good to me. Thanks for the Loom as well @hariombalhara.
What does this PR do?
Fixes PRI-288
Root cause behind the issue was that the iframe has no scroll in inline embed and Safari refuses to let scrollIntoView scroll to an element when called from within the iframe. A fallback approach of asking parent to do the scroll has been added.
Tech Debt Cleanup
Video Demo
https://www.loom.com/share/ae36df9b88ac44ce8817f8269855ddd0
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
See loom