-
Notifications
You must be signed in to change notification settings - Fork 1
fix: safely disconnect themeChangeObserver with optional chaining #22
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
WalkthroughReplaces a direct property access in unobserveThemeChange with optional chaining to safely check for themeChangeObserver on markDownEditor.$connector before disconnecting and clearing it. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Assessment against linked issues
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 (
|
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
🔭 Outside diff range comments (2)
src/main/resources/META-INF/resources/frontend/fcMarkdownEditorConnector.js (2)
24-31: Prevent re-initialization lockout after unobserveobserveThemeChange returns early if $connector exists, but unobserveThemeChange does not remove $connector, only nulls themeChangeObserver. On a re-attach for the same element instance, observeThemeChange will bail out and never recreate the observer. This silently disables theme observation after the first detach/attach cycle.
Refactor the guard to only skip when an active observer exists, and ensure $connector is initialized when missing.
Apply this diff:
- // Check whether the connector was already initialized for markDownEditor - if (markDownEditor.$connector) { - return; - } - - markDownEditor.$connector = {}; + // Initialize connector namespace if missing + if (!markDownEditor.$connector) { + markDownEditor.$connector = {}; + } + // Skip only if an observer is already active + if (markDownEditor.$connector.themeChangeObserver) { + return; + }
74-80: Tear down connector state to allow clean re-observeWhen unobserving, also clear the themeChangeObserver property (and $connector if empty) so a subsequent observeThemeChange can re-initialize correctly.
Apply this diff:
- if (markDownEditor?.$connector?.themeChangeObserver) { - markDownEditor.$connector.themeChangeObserver.disconnect(); - markDownEditor.$connector.themeChangeObserver = null; - } + if (markDownEditor?.$connector?.themeChangeObserver) { + markDownEditor.$connector.themeChangeObserver.disconnect(); + // Remove the observer reference to allow re-initialization later + delete markDownEditor.$connector.themeChangeObserver; + // If no other connector state is stored, clean up the namespace too + if (Object.keys(markDownEditor.$connector).length === 0) { + delete markDownEditor.$connector; + } + }
🧹 Nitpick comments (2)
src/main/resources/META-INF/resources/frontend/fcMarkdownEditorConnector.js (2)
51-53: Scope the MutationObserver to the ‘theme’ attribute onlyNarrowing the observer to attributeFilter ['theme'] reduces noise and avoids callbacks for irrelevant attribute changes.
Apply this diff:
- // options for the observer (which mutations to observe) - const config = { attributes: true }; + // options for the observer (only watch the 'theme' attribute) + const config = { attributes: true, attributeFilter: ['theme'] };Also applies to: 69-73
59-61: Remove debug loggingconsole.log in the hot path will spam the console on every theme change. Remove or downgrade to console.debug if needed.
Apply this diff:
- console.log("theme", themeName);
📜 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)
src/main/resources/META-INF/resources/frontend/fcMarkdownEditorConnector.js(1 hunks)
🔇 Additional comments (1)
src/main/resources/META-INF/resources/frontend/fcMarkdownEditorConnector.js (1)
76-79: Null-safe disconnect via optional chaining looks goodUsing optional chaining prevents the “Cannot read properties of null (reading '$connector')” error on detach. This directly addresses Issue #20 without changing behavior otherwise.
Close #20
Summary by CodeRabbit