fix: ensure dense theme CSS is included in production bundle#175
Open
timomeinen wants to merge 1 commit intoFlowingCode:masterfrom
Open
fix: ensure dense theme CSS is included in production bundle#175timomeinen wants to merge 1 commit intoFlowingCode:masterfrom
timomeinen wants to merge 1 commit intoFlowingCode:masterfrom
Conversation
Add setDenseTheme(Grid, boolean) and isDenseTheme(Grid) methods that create a bytecode reference to GridHelper, ensuring the Vaadin production bundle scanner discovers its @CssImport annotations. The DENSE_THEME constant is a compile-time constant (static final String), so javac inlines it at the call site. When a consumer only uses the constant (e.g. grid.addThemeName(GridHelper.DENSE_THEME)), no bytecode reference to GridHelper is emitted and the scanner never reaches it. The previous VaadinServiceInitListener approach is removed in favor of the simpler method-based solution. DENSE_THEME is deprecated in favor of the new methods. Closes FlowingCode#171
WalkthroughRefactored the dense theme configuration in GridHelper to use explicit setter methods instead of direct theme-name manipulation. Removed the VaadinServiceInitListener class and its service registration. Added Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
🧹 Nitpick comments (2)
src/main/java/com/flowingcode/vaadin/addons/gridhelpers/GridHelper.java (2)
117-119: Same null-safety consideration applies toisDenseTheme.For consistency, consider applying the same null check here.
Proposed null check
public static boolean isDenseTheme(Grid<?> grid) { + java.util.Objects.requireNonNull(grid, "grid cannot be null"); return grid.hasThemeName(DENSE_THEME_NAME); }🤖 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/gridhelpers/GridHelper.java` around lines 117 - 119, The isDenseTheme method currently calls grid.hasThemeName(DENSE_THEME_NAME) without guarding against a null grid; add the same null-safety check used elsewhere (e.g., return false if grid is null) before calling Grid.hasThemeName to avoid NPEs—update the isDenseTheme(Grid<?> grid) implementation to check grid for null and only call grid.hasThemeName(DENSE_THEME_NAME) when non-null.
103-109: Consider adding null-safety for thegridparameter.The method will throw a
NullPointerExceptionifgridisnull. While callers are expected to pass a valid grid, adding a null check would provide a clearer error message and align with defensive coding practices seen elsewhere in Vaadin components.Proposed null check
public static void setDenseTheme(Grid<?> grid, boolean dense) { + java.util.Objects.requireNonNull(grid, "grid cannot be null"); if (dense) { grid.addThemeName(DENSE_THEME_NAME); } else { grid.removeThemeName(DENSE_THEME_NAME); } }🤖 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/gridhelpers/GridHelper.java` around lines 103 - 109, The setDenseTheme(Grid<?> grid, boolean dense) method lacks null-safety and will NPE if grid is null; add a defensive check at the top of setDenseTheme to validate the grid parameter (e.g., if grid == null) and throw a clear IllegalArgumentException or NullPointerException with a concise message like "grid must not be null" before referencing DENSE_THEME_NAME or calling grid.addThemeName/removeThemeName; this keeps behavior explicit and consistent with other Vaadin defensive checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/com/flowingcode/vaadin/addons/gridhelpers/GridHelper.java`:
- Around line 117-119: The isDenseTheme method currently calls
grid.hasThemeName(DENSE_THEME_NAME) without guarding against a null grid; add
the same null-safety check used elsewhere (e.g., return false if grid is null)
before calling Grid.hasThemeName to avoid NPEs—update the isDenseTheme(Grid<?>
grid) implementation to check grid for null and only call
grid.hasThemeName(DENSE_THEME_NAME) when non-null.
- Around line 103-109: The setDenseTheme(Grid<?> grid, boolean dense) method
lacks null-safety and will NPE if grid is null; add a defensive check at the top
of setDenseTheme to validate the grid parameter (e.g., if grid == null) and
throw a clear IllegalArgumentException or NullPointerException with a concise
message like "grid must not be null" before referencing DENSE_THEME_NAME or
calling grid.addThemeName/removeThemeName; this keeps behavior explicit and
consistent with other Vaadin defensive checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1a469487-95d9-4d53-8fe4-a3b433ae41d2
📒 Files selected for processing (6)
README.mdsrc/main/java/com/flowingcode/vaadin/addons/gridhelpers/GridHelper.javasrc/main/java/com/flowingcode/vaadin/addons/gridhelpers/GridHelperServiceInitListener.javasrc/main/resources/META-INF/services/com.vaadin.flow.server.VaadinServiceInitListenersrc/test/java/com/flowingcode/vaadin/addons/gridhelpers/AllFeaturesDemo.javasrc/test/java/com/flowingcode/vaadin/addons/gridhelpers/DenseThemeDemo.java
💤 Files with no reviewable changes (2)
- src/main/resources/META-INF/services/com.vaadin.flow.server.VaadinServiceInitListener
- src/main/java/com/flowingcode/vaadin/addons/gridhelpers/GridHelperServiceInitListener.java
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.
Problem
The
DENSE_THEMEconstant is astatic final String, so javac inlines it at the call site. When a consumer only uses the constant (e.g.grid.addThemeName(GridHelper.DENSE_THEME)), no bytecode reference toGridHelperis emitted. The Vaadin production bundle scanner is reachability-based, so it never visitsGridHelperand its@CssImportannotations are not processed. The dense theme CSS is missing from the production bundle.Solution
Add
setDenseTheme(Grid<?>, boolean)andisDenseTheme(Grid<?>)static methods toGridHelper. Calling these methods emits aninvokestaticinstruction referencingGridHelper, making the class reachable to the scanner.Changes
setDenseTheme/isDenseThememethods with a privateDENSE_THEME_NAMEconstant for internal use. Deprecated the publicDENSE_THEMEconstant with guidance to use the new methods.GridHelper.setDenseTheme(grid, true).GridHelper.setDenseTheme/isDenseTheme.Closes #171
Summary by CodeRabbit
New Features
Documentation
Deprecated