feat(external-sources): add externalSourcesEmbedCss config option to set embed css on external sources iframe
#769
Conversation
…o set embed css on external sources iframe
WalkthroughThe pull request introduces a new configuration option Changes
Sequence DiagramsequenceDiagram
participant User
participant Config
participant ExternalSource
participant MessageBridge
User->>Config: Set externalSourcesEmbedCss
Config->>ExternalSource: Trigger initCallback
ExternalSource->>MessageBridge: Send applyEmbedCss
MessageBridge->>ExternalSource: Apply CSS to external source
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
blocks/ExternalSource/ExternalSource.js (1)
163-164: Recheck load sequence for consistencyCalling
applyEmbedCssbeforeapplyThemeis fine, but confirm there's no conflict between the two operations. For instance, if the embedded CSS overrides theme variables, verify that the outcome is intentional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
blocks/Config/initialConfig.js(1 hunks)blocks/Config/normalizeConfigValue.js(1 hunks)blocks/ExternalSource/ExternalSource.js(2 hunks)blocks/ExternalSource/types.js(1 hunks)types/exported.d.ts(1 hunks)
🔇 Additional comments (6)
blocks/Config/initialConfig.js (1)
21-21: Great addition to the initial configuration
By introducing externalSourcesEmbedCss, we now provide a straightforward way to embed arbitrary CSS in external iframes. This makes styling more flexible.
blocks/ExternalSource/types.js (1)
139-139: Confirm switch from object-based to string-based CSS
Switching from Partial<ThemeDefinition> to a simple string type offers broad styling flexibility, but it removes structured or type-checked constraints. Ensure that any validations or sanitizations are performed upstream if needed.
blocks/Config/normalizeConfigValue.js (1)
116-116: String normalization for externalSourcesEmbedCss
Mapping externalSourcesEmbedCss to asString correctly aligns with its usage in the external source's logic. This is a straightforward approach, and no issues are apparent.
blocks/ExternalSource/ExternalSource.js (2)
109-112: Watch for untrusted CSS
Subscribing to externalSourcesEmbedCss and applying it through applyEmbedCss is a valuable feature. However, ensure no untrusted input is passed to avoid unexpected styling or potential CSS injection.
169-186: New applyEmbedCss method is well-designed
The applyEmbedCss(css) method accurately sends a set-embed-css message. This keeps the logic consistent with how the theme definition is sent, fostering a clear separation of concerns.
types/exported.d.ts (1)
35-35: Consider adding a doc comment or making this property optional
If externalSourcesEmbedCss is not strictly required in all usage scenarios, it can be marked as optional. Alternatively, consider adding a short doc comment to clarify its purpose and usage, for instance:
+ /**
+ * Custom CSS applied on embedded external sources (e.g., iframes).
+ * If not set, defaults to an empty string.
+ */
externalSourcesEmbedCss: string;Check whether any usage in the codebase expects this property to always be defined. If not, making it optional might be more flexible. To verify:
✅ Verification successful
Property has a default value and is used safely - can be documented but optional flag not needed
The verification shows that externalSourcesEmbedCss is well-handled in the codebase:
- It has a default empty string value in
blocks/Config/initialConfig.js - It's normalized using
asStringinblocks/Config/normalizeConfigValue.js - It's safely used in
ExternalSource.jsthroughsubConfigValueandapplyEmbedCss
Given this implementation, the property doesn't need to be optional since it always has a value. However, documenting the default value would be helpful:
+ /**
+ * Custom CSS applied on embedded external sources (e.g., iframes).
+ * Defaults to an empty string.
+ */
externalSourcesEmbedCss: string;🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for references to `externalSourcesEmbedCss` in the entire repository
rg -A 4 "externalSourcesEmbedCss"
Length of output: 1554
Description
Checklist
Summary by CodeRabbit
New Features
ExternalSourcefunctionality to dynamically apply embedded CSS and manage themes.Bug Fixes
Documentation