feat: add support for dynamic theme in iframes#150
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a ThemeChangeEvent and updates observer API; TabbedDemo now emits event-based theme changes; DynamicTheme.apply resolves the correct document at runtime (iframe vs top) and injects a local Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.java (1)
142-145: Guard against null selection before applying dynamic theme.Line 143 dereferences
ev.getValue()directly. A defensive null guard avoids a potential NPE if the value is ever cleared/reset.💡 Suggested change
themeSelect.addValueChangeListener(ev -> { - ev.getValue().apply(this); + DynamicTheme selected = ev.getValue(); + if (selected == null) { + return; + } + selected.apply(this); observeThemeChange(this); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.java` around lines 142 - 145, The value-change listener on themeSelect dereferences ev.getValue() without a null check, risking an NPE if the selection is cleared; update the listener in TabbedDemo to first check that ev.getValue() != null before calling ev.getValue().apply(this) and observeThemeChange(this), and skip (or handle) the theme application when the new value is null so the code only applies themes for non-null selections.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/flowingcode/vaadin/addons/demo/DynamicTheme.java`:
- Around line 239-240: Remove the accidental debug logging in the theme
application code: locate the console.error(_document) call inside the
DynamicTheme implementation (the theme application method that also queries link
elements with querySelector(`link[href='${href}']`)) and delete that
console.error invocation so the document object is not logged on every theme
switch; leave the subsequent logic (link lookup and theme switching) untouched.
In `@src/main/java/com/flowingcode/vaadin/addons/demo/ThemeChangeEvent.java`:
- Around line 34-35: The ThemeChangeEvent constructor currently calls
super(source, true) marking the event as client-originated; change it to call
super(source, false) so the event is marked server-originated. Update the
ThemeChangeEvent(Component source, ColorScheme colorScheme, DynamicTheme
dynamicTheme) constructor to pass false to the ComponentEvent superclass (so
isFromClient() returns false) — this aligns with how
TabbedDemo.observeThemeChange() creates and dispatches the event from
server-side code.
---
Nitpick comments:
In `@src/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.java`:
- Around line 142-145: The value-change listener on themeSelect dereferences
ev.getValue() without a null check, risking an NPE if the selection is cleared;
update the listener in TabbedDemo to first check that ev.getValue() != null
before calling ev.getValue().apply(this) and observeThemeChange(this), and skip
(or handle) the theme application when the new value is null so the code only
applies themes for non-null selections.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 131e6910-66ff-47d7-ad78-9d3dbc6914ad
📒 Files selected for processing (4)
src/main/java/com/flowingcode/vaadin/addons/demo/DynamicTheme.javasrc/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.javasrc/main/java/com/flowingcode/vaadin/addons/demo/ThemeChangeEvent.javasrc/main/java/com/flowingcode/vaadin/addons/demo/ThemeChangeObserver.java
src/main/java/com/flowingcode/vaadin/addons/demo/DynamicTheme.java
Outdated
Show resolved
Hide resolved
src/main/java/com/flowingcode/vaadin/addons/demo/ThemeChangeEvent.java
Outdated
Show resolved
Hide resolved
javier-godoy
force-pushed
the
dynamic-theme-iframe
branch
from
34079ad to
afc7306
Compare
paodb
requested changes
Mar 6, 2026
github-project-automation
bot
moved this from To Do
to In Progress
in Flowing Code Addons
paodb
reviewed
Mar 6, 2026
javier-godoy
force-pushed
the
dynamic-theme-iframe
branch
from
afc7306 to
eab7f53
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.java (1)
483-484: Minor: Extra blank line.There are two consecutive blank lines here. Consider removing one for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.java` around lines 483 - 484, Remove the extra blank line in TabbedDemo.java so there aren't two consecutive empty lines; locate the consecutive blank lines inside the TabbedDemo class (near the existing methods/fields around the 480s) and delete one to keep a single blank line for consistent formatting.src/main/java/com/flowingcode/vaadin/addons/demo/ThemeChangeObserver.java (1)
34-36: Minor: Trailing semicolon after method body.The semicolon after the closing brace is syntactically valid but unconventional. Consider removing it for consistency.
`@Deprecated`(forRemoval = true, since = "5.3.0") default void onThemeChange(String themeName) { - }; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/flowingcode/vaadin/addons/demo/ThemeChangeObserver.java` around lines 34 - 36, Remove the stray trailing semicolon after the default method body in ThemeChangeObserver.onThemeChange; in the default method declaration (default void onThemeChange(String themeName) { }) delete the semicolon following the closing brace so the method ends with just the closing brace, keeping the `@Deprecated` annotation and method signature intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.java`:
- Around line 475-482: In observeThemeChange, replace the
DynamicTheme.isFeatureSupported() check with DynamicTheme.isFeatureInitialized()
before calling DynamicTheme.getCurrent() so you only call getCurrent() when the
feature is actually initialized; update the conditional in the
observeThemeChange method (which creates the ThemeChangeEvent and calls
collectThemeChangeObservers(...).forEach(...)) to use isFeatureInitialized() and
keep dynamicTheme null when not initialized.
- Around line 471-472: In setColorScheme, add a brief inline comment above the
two notification calls (collectThemeChangeObservers(...).forEach(observer ->
observer.onThemeChange(theme)); and observeThemeChange(component);) explaining
that observers are deliberately notified twice to support backward
compatibility: first via the deprecated onThemeChange(String theme) and then via
the new onThemeChange(ThemeChangeEvent) delivery; reference ThemeChangeObserver
and note both methods are default methods so implementers overriding both may
receive both notifications during migration and the first call can be removed
once the deprecated API is dropped.
---
Nitpick comments:
In `@src/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.java`:
- Around line 483-484: Remove the extra blank line in TabbedDemo.java so there
aren't two consecutive empty lines; locate the consecutive blank lines inside
the TabbedDemo class (near the existing methods/fields around the 480s) and
delete one to keep a single blank line for consistent formatting.
In `@src/main/java/com/flowingcode/vaadin/addons/demo/ThemeChangeObserver.java`:
- Around line 34-36: Remove the stray trailing semicolon after the default
method body in ThemeChangeObserver.onThemeChange; in the default method
declaration (default void onThemeChange(String themeName) { }) delete the
semicolon following the closing brace so the method ends with just the closing
brace, keeping the `@Deprecated` annotation and method signature intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fab3fd98-b705-4daf-b55e-c3c0b4bae1bc
📒 Files selected for processing (4)
src/main/java/com/flowingcode/vaadin/addons/demo/DynamicTheme.javasrc/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.javasrc/main/java/com/flowingcode/vaadin/addons/demo/ThemeChangeEvent.javasrc/main/java/com/flowingcode/vaadin/addons/demo/ThemeChangeObserver.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/flowingcode/vaadin/addons/demo/DynamicTheme.java
javier-godoy
force-pushed
the
dynamic-theme-iframe
branch
from
eab7f53 to
37f64d7
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/flowingcode/vaadin/addons/demo/DynamicTheme.java (1)
236-246:⚠️ Potential issue | 🟠 MajorThe iframe path can blank the target document on the first switch.
If
_documentresolves to an iframe document, Line 240 skips missing links, butinitialize(...)only injects the active stylesheet anddoPrepare(...)still populates the outerdocument. That lets Lines 244-245 disable the current iframe stylesheet before the new one exists, leaving the iframe unthemed after the first switch. Please create/preload the selected href in_documentbefore disabling the old theme, or makedoPrepare()resolve the same document.🔧 Minimal guard to avoid dropping all iframe theme styles
const applyTheme = () => { ["lumo/lumo.css", "aura/aura.css"].forEach(href=> { let link = _document.querySelector(`link[href='${href}']`); - if (!link) return; + if (!link) { + if (href !== $0) return; + link = _document.createElement("link"); + link.rel = "stylesheet"; + link.href = href; + _document.head.prepend(link); + } if (href === $0) { if (link.rel === 'preload') link.rel = 'stylesheet'; if (link.disabled) link.disabled = false; } else if (link.rel === 'stylesheet' && !link.disabled) { link.disabled = true; } }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/flowingcode/vaadin/addons/demo/DynamicTheme.java` around lines 236 - 246, applyTheme currently disables the old stylesheet in _document before ensuring the new one exists, which can leave an iframe unthemed; update the logic in applyTheme (and/or adjust doPrepare/initialize) so that the selected href is created or preloaded in the same _document before disabling other styles: specifically, when iterating in applyTheme for the target href ($0) ensure you create or insert a preload/link element into _document (or have doPrepare return the iframe document rather than the outer document) and wait for it to be available (or convert preload to stylesheet) before altering link.rel or link.disabled on existing links to avoid temporarily removing all iframe styles.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/main/java/com/flowingcode/vaadin/addons/demo/DynamicTheme.java`:
- Around line 236-246: applyTheme currently disables the old stylesheet in
_document before ensuring the new one exists, which can leave an iframe
unthemed; update the logic in applyTheme (and/or adjust doPrepare/initialize) so
that the selected href is created or preloaded in the same _document before
disabling other styles: specifically, when iterating in applyTheme for the
target href ($0) ensure you create or insert a preload/link element into
_document (or have doPrepare return the iframe document rather than the outer
document) and wait for it to be available (or convert preload to stylesheet)
before altering link.rel or link.disabled on existing links to avoid temporarily
removing all iframe styles.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1bf228f7-9dd1-40b3-99be-8399e782b649
📒 Files selected for processing (4)
src/main/java/com/flowingcode/vaadin/addons/demo/DynamicTheme.javasrc/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.javasrc/main/java/com/flowingcode/vaadin/addons/demo/ThemeChangeEvent.javasrc/main/java/com/flowingcode/vaadin/addons/demo/ThemeChangeObserver.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.java
Member
Author
Yes... I tested it quickly and assumed it was Aura theme, but now I realized that it was switching to the base theme instead. |
javier-godoy
force-pushed
the
dynamic-theme-iframe
branch
from
37f64d7 to
602bf21
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/flowingcode/vaadin/addons/demo/DynamicTheme.java`:
- Around line 228-258: The current apply() implementation (inside
component.getElement().executeJs where applyTheme is defined) can disable the
active theme before the new theme is created in an iframe; change applyTheme to
run in two passes against the iframe-aware _document: first ensure the selected
theme ($0) exists and is enabled in _document (create a <link> if missing or
convert preload→stylesheet and enable it), then in a second pass disable other
themes only after the chosen theme is present; also update doPrepare() to
preload into the same iframe-aware document (use the same iframe detection logic
that sets document/_document) so preloads happen inside the iframe rather than
only on the outer document.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cef1be75-1241-4a7c-9b52-828d422521a2
📒 Files selected for processing (1)
src/main/java/com/flowingcode/vaadin/addons/demo/DynamicTheme.java
paodb
approved these changes
Mar 16, 2026
github-project-automation
bot
moved this from In Progress
to Pending release
in Flowing Code Addons
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor