feat: add support for dynamic theme in iframes#150
feat: add support for dynamic theme in iframes#150javier-godoy wants to merge 3 commits intomasterfrom
Conversation
WalkthroughRestructures theme-change propagation to publish a new ThemeChangeEvent and notify observers; updates TabbedDemo to emit that event after theme changes; modifies DynamicTheme.apply to resolve the correct document at runtime (iframe host vs top document) and use a local _document in the injected JS. 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)
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
| public ThemeChangeEvent(Component source, ColorScheme colorScheme, DynamicTheme dynamicTheme) { | ||
| super(source, true); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd 'ThemeChangeEvent.java$' --exec sed -n '31,38p' {}
fd 'TabbedDemo.java$' --exec sed -n '475,482p' {}Repository: FlowingCode/CommonsDemo
Length of output: 697
Set fromClient to false for this event.
Line 35 marks the event as client-originated, but ThemeChangeEvent is instantiated from server-side code in TabbedDemo.observeThemeChange() and dispatched to observers without any client interaction. This breaks the semantic contract of ComponentEvent and can cause listener logic relying on isFromClient() to behave incorrectly.
🔧 Proposed fix
- super(source, true);
+ super(source, false);🤖 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/ThemeChangeEvent.java`
around lines 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.
There was a problem hiding this comment.
@coderabbitai The event change is usually initiated from the client, but by the time the event is dispatched there is no clear indication of the actual source.
There was a problem hiding this comment.
@javier-godoy, that's a fair point — since theme changes are typically user-driven (client-initiated), defaulting to true is a reasonable approximation. The limitation is that if the theme is ever changed programmatically from server-side code, isFromClient() would return true incorrectly.
If you anticipate mixing both client- and server-initiated changes in the future, an alternative would be to thread fromClient as a constructor parameter:
public ThemeChangeEvent(Component source, ColorScheme colorScheme, DynamicTheme dynamicTheme, boolean fromClient) {
super(source, fromClient);
...
}That way, callers like observeThemeChange() can pass the appropriate value when the origin is known. That said, if theme changes are exclusively user-driven in practice, the current approach is acceptable. I'll leave it to your judgment.
🧠 Learnings used
Learnt from: javier-godoy
Repo: FlowingCode/CommonsDemo PR: 138
File: src/main/java/com/flowingcode/vaadin/addons/demo/ColorScheme.java:22-36
Timestamp: 2026-01-29T18:39:49.374Z
Learning: The file `src/main/java/com/flowingcode/vaadin/addons/demo/ColorScheme.java` contains both Flowing Code's license header and Vaadin's copyright notice because the ColorScheme code is copied from Vaadin's open source framework. Both headers are intentional and required for proper attribution.
Learnt from: javier-godoy
Repo: FlowingCode/CommonsDemo PR: 143
File: src/main/java/com/flowingcode/vaadin/addons/demo/DynamicThemeInitializer.java:1-38
Timestamp: 2026-02-04T19:18:59.956Z
Learning: For DynamicThemeInitializer.java (src/main/java/com/flowingcode/vaadin/addons/demo/DynamicThemeInitializer.java), when loading the theme property from dynamic-theme.properties, use the value exactly as written (case-sensitive) after trimming whitespace. Do not apply any automatic case normalization (e.g., toUpperCase/toLowerCase). The value should be used to match the corresponding enum constant name precisely.
Learnt from: javier-godoy
Repo: FlowingCode/CommonsDemo PR: 143
File: src/main/java/com/flowingcode/vaadin/addons/demo/DynamicThemeInitializer.java:1-38
Timestamp: 2026-02-04T18:39:52.796Z
Learning: In `src/main/java/com/flowingcode/vaadin/addons/demo/DynamicThemeServiceInitListener.java`, configuration validation for the theme property should use a fail-fast approach. Invalid theme values should propagate IllegalArgumentException to cause service initialization to fail immediately, allowing developers to catch configuration errors during the build/test phase rather than masking them with defaults or logging.
Learnt from: javier-godoy
Repo: FlowingCode/CommonsDemo PR: 140
File: src/main/java/com/flowingcode/vaadin/addons/demo/DynamicTheme.java:70-73
Timestamp: 2026-02-02T19:45:32.988Z
Learning: In `src/main/java/com/flowingcode/vaadin/addons/demo/DynamicTheme.java`, the methods `isFeatureInitialized()`, `getCurrent()`, and `initialize()` that call `VaadinSession.getCurrent()` are designed to be invoked only from within a valid Vaadin request context, where the session is guaranteed to be non-null. Null checks for the session are not required.
Learnt from: javier-godoy
Repo: FlowingCode/CommonsDemo PR: 149
File: src/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.java:205-214
Timestamp: 2026-03-02T18:40:21.008Z
Learning: Routing convention guideline: In FlowingCode/CommonsDemo demos (e.g., TabbedDemo.java), the first segment of routes must be a static, alphanumeric component or module name (e.g., /twincolgrid). Routes starting with pure symbols or dynamic numbers violate the convention. Apply this rule to all demo Java files under src/main/java/com/flowingcode/vaadin/addons/demo/. During reviews, verify that the first path segment in route mappings is ASCII alphanumeric and does not start with symbols or digits. If a route violates this, propose refactoring to a valid static segment (e.g., /moduleName) and update any related navigation or permissions logic accordingly.
There was a problem hiding this comment.
I'll leave it to the reviewer's judgment.
There was a problem hiding this comment.
@javier-godoy, understood — I'll leave the note for the human reviewer to weigh in. The current true value is a reasonable approximation given that theme changes are typically user-driven; the alternative of threading fromClient through the constructor would offer more precision if mixed origins ever become a concern.
🧠 Learnings used
Learnt from: javier-godoy
Repo: FlowingCode/CommonsDemo PR: 138
File: src/main/java/com/flowingcode/vaadin/addons/demo/ColorScheme.java:22-36
Timestamp: 2026-01-29T18:39:49.374Z
Learning: The file `src/main/java/com/flowingcode/vaadin/addons/demo/ColorScheme.java` contains both Flowing Code's license header and Vaadin's copyright notice because the ColorScheme code is copied from Vaadin's open source framework. Both headers are intentional and required for proper attribution.
Learnt from: javier-godoy
Repo: FlowingCode/CommonsDemo PR: 143
File: src/main/java/com/flowingcode/vaadin/addons/demo/DynamicThemeInitializer.java:1-38
Timestamp: 2026-02-04T19:18:59.956Z
Learning: For DynamicThemeInitializer.java (src/main/java/com/flowingcode/vaadin/addons/demo/DynamicThemeInitializer.java), when loading the theme property from dynamic-theme.properties, use the value exactly as written (case-sensitive) after trimming whitespace. Do not apply any automatic case normalization (e.g., toUpperCase/toLowerCase). The value should be used to match the corresponding enum constant name precisely.
Learnt from: javier-godoy
Repo: FlowingCode/CommonsDemo PR: 143
File: src/main/java/com/flowingcode/vaadin/addons/demo/DynamicThemeInitializer.java:1-38
Timestamp: 2026-02-04T18:39:52.796Z
Learning: In `src/main/java/com/flowingcode/vaadin/addons/demo/DynamicThemeServiceInitListener.java`, configuration validation for the theme property should use a fail-fast approach. Invalid theme values should propagate IllegalArgumentException to cause service initialization to fail immediately, allowing developers to catch configuration errors during the build/test phase rather than masking them with defaults or logging.
Learnt from: javier-godoy
Repo: FlowingCode/CommonsDemo PR: 140
File: src/main/java/com/flowingcode/vaadin/addons/demo/DynamicTheme.java:70-73
Timestamp: 2026-02-02T19:45:32.988Z
Learning: In `src/main/java/com/flowingcode/vaadin/addons/demo/DynamicTheme.java`, the methods `isFeatureInitialized()`, `getCurrent()`, and `initialize()` that call `VaadinSession.getCurrent()` are designed to be invoked only from within a valid Vaadin request context, where the session is guaranteed to be non-null. Null checks for the session are not required.
Learnt from: javier-godoy
Repo: FlowingCode/CommonsDemo PR: 149
File: src/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.java:205-214
Timestamp: 2026-03-02T18:40:21.008Z
Learning: Routing convention guideline: In FlowingCode/CommonsDemo demos (e.g., TabbedDemo.java), the first segment of routes must be a static, alphanumeric component or module name (e.g., /twincolgrid). Routes starting with pure symbols or dynamic numbers violate the convention. Apply this rule to all demo Java files under src/main/java/com/flowingcode/vaadin/addons/demo/. During reviews, verify that the first path segment in route mappings is ASCII alphanumeric and does not start with symbols or digits. If a route violates this, propose refactoring to a valid static segment (e.g., /moduleName) and update any related navigation or permissions logic accordingly.
There was a problem hiding this comment.
I think CodeRabbit raises a fair point here. The justification given in it's first message should be taken in consideration. Hardcoding the value to true might not accurately reflect the origin and could affect listeners relying on isFromClient(). But this is my opinion. On the other hand, we could leave the current implementation and go back and fix it if some of the concerns come true.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@javier-godoy as discussed in meeting, the parameter should be set as false when the event is initialized in observeThemeChange method.
javier-godoy
force-pushed
the
dynamic-theme-iframe
branch
from
34079ad to
afc7306
Compare
github-project-automation
bot
moved this from To Do
to In Progress
in Flowing Code Addons
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
| private static void observeThemeChange(Component source) { | ||
| DynamicTheme dynamicTheme = null; | ||
| if (DynamicTheme.isFeatureSupported()) { | ||
| dynamicTheme = DynamicTheme.getCurrent(); | ||
| } | ||
| ThemeChangeEvent event = new ThemeChangeEvent(source, false, getColorScheme(), dynamicTheme); | ||
| collectThemeChangeObservers(source).forEach(observer -> observer.onThemeChange(event)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd 'DynamicTheme.java$' --exec cat -n {}Repository: FlowingCode/CommonsDemo
Length of output: 10202
🏁 Script executed:
fd 'TabbedDemo.java$' --exec cat -n {} | sed -n '130,140p'Repository: FlowingCode/CommonsDemo
Length of output: 558
Use isFeatureInitialized() instead of isFeatureSupported() for consistency.
Line 477 checks DynamicTheme.isFeatureSupported() before calling getCurrent(), while line 134 correctly uses DynamicTheme.isFeatureInitialized(). These are distinct methods:
isFeatureSupported()- checks Vaadin version >= 25isFeatureInitialized()- checks both version support and session initialization
Since getCurrent() returns null if the feature has not been initialized (per its documentation), the check at line 477 should use isFeatureInitialized() to match line 134 and ensure the theme is actually available before use.
🤖 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 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.
There was a problem hiding this comment.
@coderabbitai I disagree. Since getCurrent() returns null if the feature has not been initialized, I only have to check if the feature is supported (in order to avoid the exception). Note that isFeatureInitialized already checks getCurrent()!=null, thus modifying this block to call isFeatureInitialized would be equivalent to the following redundant code:
DynamicTheme dynamicTheme = null;
if (DynamicTheme.isFeatureSupported() && DynamicTheme.getCurrent()!=null) {
dynamicTheme = DynamicTheme.getCurrent();
}
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd 'DynamicTheme.java$' --exec grep -n 'isFeatureSupported\|isFeatureInitialized\|getCurrent' {}
Length of output: 804
@javier-godoy, you're right. Looking at the implementation, getCurrent() (line 97–99) does not guard on isFeatureSupported() — it directly returns VaadinSession.getCurrent().getAttribute(DynamicTheme.class), which safely returns null when the feature hasn't been initialized. The isFeatureSupported() check in observeThemeChange is only needed to guard methods that do throw when unsupported (like apply()).
As you noted, substituting isFeatureInitialized() — which is defined as isFeatureSupported() && getCurrent() != null — would introduce a redundant implicit call to getCurrent(), making the block equivalent to calling it twice. The current code is correct and non-redundant. I withdraw my suggestion.
✏️ Learnings added
Learnt from: javier-godoy
Repo: FlowingCode/CommonsDemo PR: 150
File: src/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.java:475-482
Timestamp: 2026-03-06T19:42:42.656Z
Learning: In `src/main/java/com/flowingcode/vaadin/addons/demo/DynamicTheme.java`, `getCurrent()` (line 97-99) does NOT check `isFeatureSupported()` internally; it directly returns the session attribute and safely returns null when the feature is not initialized. Only methods like `apply()` throw when `isFeatureSupported()` is false. Therefore, checking `isFeatureSupported()` before calling `getCurrent()` is sufficient and correct — using `isFeatureInitialized()` instead would be redundant, as it internally calls `getCurrent() != null`, causing an implicit double invocation.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: javier-godoy
Repo: FlowingCode/CommonsDemo PR: 140
File: src/main/java/com/flowingcode/vaadin/addons/demo/DynamicTheme.java:70-73
Timestamp: 2026-02-02T19:45:32.988Z
Learning: In `src/main/java/com/flowingcode/vaadin/addons/demo/DynamicTheme.java`, the methods `isFeatureInitialized()`, `getCurrent()`, and `initialize()` that call `VaadinSession.getCurrent()` are designed to be invoked only from within a valid Vaadin request context, where the session is guaranteed to be non-null. Null checks for the session are not required.
Learnt from: javier-godoy
Repo: FlowingCode/CommonsDemo PR: 138
File: src/main/java/com/flowingcode/vaadin/addons/demo/ColorScheme.java:22-36
Timestamp: 2026-01-29T18:39:49.374Z
Learning: The file `src/main/java/com/flowingcode/vaadin/addons/demo/ColorScheme.java` contains both Flowing Code's license header and Vaadin's copyright notice because the ColorScheme code is copied from Vaadin's open source framework. Both headers are intentional and required for proper attribution.
Learnt from: javier-godoy
Repo: FlowingCode/CommonsDemo PR: 143
File: src/main/java/com/flowingcode/vaadin/addons/demo/DynamicThemeInitializer.java:1-38
Timestamp: 2026-02-04T18:39:52.796Z
Learning: In `src/main/java/com/flowingcode/vaadin/addons/demo/DynamicThemeServiceInitListener.java`, configuration validation for the theme property should use a fail-fast approach. Invalid theme values should propagate IllegalArgumentException to cause service initialization to fail immediately, allowing developers to catch configuration errors during the build/test phase rather than masking them with defaults or logging.
Learnt from: javier-godoy
Repo: FlowingCode/CommonsDemo PR: 143
File: src/main/java/com/flowingcode/vaadin/addons/demo/DynamicThemeInitializer.java:1-38
Timestamp: 2026-02-04T19:18:59.956Z
Learning: For DynamicThemeInitializer.java (src/main/java/com/flowingcode/vaadin/addons/demo/DynamicThemeInitializer.java), when loading the theme property from dynamic-theme.properties, use the value exactly as written (case-sensitive) after trimming whitespace. Do not apply any automatic case normalization (e.g., toUpperCase/toLowerCase). The value should be used to match the corresponding enum constant name precisely.
Learnt from: javier-godoy
Repo: FlowingCode/CommonsDemo PR: 149
File: src/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.java:205-214
Timestamp: 2026-03-02T18:40:21.008Z
Learning: Routing convention guideline: In FlowingCode/CommonsDemo demos (e.g., TabbedDemo.java), the first segment of routes must be a static, alphanumeric component or module name (e.g., /twincolgrid). Routes starting with pure symbols or dynamic numbers violate the convention. Apply this rule to all demo Java files under src/main/java/com/flowingcode/vaadin/addons/demo/. During reviews, verify that the first path segment in route mappings is ASCII alphanumeric and does not start with symbols or digits. If a route violates this, propose refactoring to a valid static segment (e.g., /moduleName) and update any related navigation or permissions logic accordingly.
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
Yes... I tested it quickly and assumed it was Aura theme, but now I realized that it was switching to the base theme instead. |
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor