fix: ensure dense theme CSS is included in production bundle#175
Merged
javier-godoy merged 3 commits intoFlowingCode:masterfrom Apr 8, 2026
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughReplaced direct theme-name usage with explicit APIs for dense styling in GridHelper (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 reverts commit 9ade7ea.
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. DENSE_THEME is deprecated in favor of the new methods. Closes FlowingCode#171 Co-authored-by: Javier Godoy <11554739+javier-godoy@users.noreply.github.com>
javier-godoy
force-pushed
the
fix/issue-171-dense-theme-production-bundle
branch
from
f44363c to
eccbdf0
Compare
|
github-project-automation
bot
moved this from To Do
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
3 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