-
Notifications
You must be signed in to change notification settings - Fork 39
feat: better alerts around theme awareness #279
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
📝 WalkthroughWalkthroughThis update enhances theme synchronization between a parent page and an iframe. It introduces a status indicator for theme sync readiness, conditional message sending based on sync status, timeout handling, and refines the messaging logic in both the parent test page and the Changes
Sequence Diagram(s)sequenceDiagram
participant Parent as Parent Page (iframe-test.html)
participant Iframe as Iframe (ThemeSync.tsx)
Parent->>Iframe: Loads iframe
Note right of Parent: Sets status to "waiting", disables Apply Theme
Iframe-->>Parent: (after 20ms) Sends "theme-ready" message
Parent->>Parent: Clears timeout, sets status to "ready", enables Apply Theme
Parent->>Iframe: Sends "theme-changed" if Apply Theme is clicked and ready
Iframe->>Iframe: Updates theme, tracks last sent theme
Iframe-->>Parent: Sends "theme-changed" only if theme differs from last sent
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code Graph Analysis (2)src/theme/Layout/IframeNavigation.tsx (1)
src/theme/Layout/index.tsx (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (3)
🪧 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 (
|
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: 0
🧹 Nitpick comments (2)
src/theme/Layout/ThemeSync.tsx (1)
17-28
: Improved theme readiness notification with delayed initializationThis new effect properly ensures the Docusaurus theme system is fully initialized before sending the ready notification to the parent window. The timeout approach is a good way to handle this asynchronous initialization.
Consider making the 20ms delay configurable via a constant for easier adjustments if needed in the future.
+// Delay before sending theme-ready message to ensure initialization +const THEME_READY_DELAY_MS = 20; // Ensure theme system is ready and notify parent useEffect(() => { if (isInIframeState) { // Wait a short time to ensure Docusaurus theme system is fully initialized const readyTimer = setTimeout(() => { // Send ready message to parent window.parent.postMessage({ type: 'theme-ready' }, '*'); - }, 20); + }, THEME_READY_DELAY_MS); return () => clearTimeout(readyTimer); } }, [isInIframeState]);iframe-test.html (1)
186-202
: Good timeout implementation for sync readinessThe timeout implementation provides a robust fallback mechanism in case the iframe never sends a 'theme-ready' message, preventing the UI from being stuck in a waiting state indefinitely.
Consider storing the timeout ID in a separate variable rather than as a property of the iframe element for better code organization.
// Auto-apply theme when iframe loads iframe.addEventListener('load', function() { console.log('Iframe loaded, waiting for theme-ready message...'); // Reset the theme sync status when iframe loads updateThemeStatus(false); + // Track timeout ID for theme sync + let syncTimeoutId; // Set a timeout for theme sync readiness - const syncTimeout = setTimeout(() => { + syncTimeoutId = setTimeout(() => { if (!isThemeSyncReady) { console.warn('Theme sync ready message not received within timeout period'); // Enable theme controls anyway after timeout updateThemeStatus(true); } }, 5000); - // Store the timeout in a property so we can clear it if needed - iframe.syncTimeoutId = syncTimeout; + // Store the timeout ID in a variable accessible to the message event handler + window.currentSyncTimeoutId = syncTimeoutId; }); // Then update the message handler: if (event.data && event.data.type === 'theme-ready') { console.log('Docusaurus is ready for theme sync'); // Clear any existing timeout - if (iframe.syncTimeoutId) { - clearTimeout(iframe.syncTimeoutId); + if (window.currentSyncTimeoutId) { + clearTimeout(window.currentSyncTimeoutId); + window.currentSyncTimeoutId = null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
iframe-test.html
(5 hunks)src/theme/Layout/ThemeSync.tsx
(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/theme/Layout/ThemeSync.tsx (1)
src/theme/Layout/utils.ts (1)
isInIframe
(4-6)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (6)
src/theme/Layout/ThemeSync.tsx (2)
10-10
: Great addition of state tracking for theme messages!Adding
lastSentTheme
state is an excellent optimization that prevents redundant theme messages from being sent to the parent window.
58-70
: Improved theme change notification with conditional sendingThe conditional check
colorMode !== lastSentTheme
is an excellent optimization that prevents sending duplicate theme messages when the theme hasn't actually changed. This reduces unnecessary communication between frames and follows React best practices with proper dependency tracking.iframe-test.html (4)
65-79
: Great addition of visual theme sync status indicatorThe addition of theme status styles and UI element provides clear visual feedback to users about the synchronization state. The color-coding (yellow for waiting, green for ready) makes the status immediately recognizable.
Also applies to: 121-121
144-165
: Well-structured theme sync state managementThe implementation of theme sync readiness tracking and the
updateThemeStatus
function creates a clean and maintainable approach to managing the sync state. The function properly handles all aspects of status updates: variable state, UI text, CSS classes, and button state.
167-177
: Improved defensive coding for theme message sendingThe conditional check before sending theme messages prevents potential race conditions by ensuring messages are only sent when the iframe is ready to receive them. The console logging provides helpful debugging information as well.
209-220
: Complete message handling for theme synchronizationThe implementation properly handles the 'theme-ready' message from the iframe by clearing any existing timeout, updating the UI status, and immediately synchronizing the iframe's theme. This creates a robust communication protocol between the parent page and iframe.
This pull request is automatically being deployed by Amplify Hosting (learn more). |
Before Submitting This PR, Please Ensure You Have Completed The Following:
Summary by CodeRabbit
New Features
Bug Fixes
Style