-
Notifications
You must be signed in to change notification settings - Fork 1
fix: ensure that Grid columns are configured before setting a footer toolbar #130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe changes introduce a precondition check in Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were found. ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/test/java/com/flowingcode/vaadin/addons/gridhelpers/test/FooterToolbarTest.java (2)
31-37
: Test asserts nothing beyond “no exception”
addToolbarFooter()
currently passes if no exception is thrown. To verify the feature, consider asserting that the toolbar was actually placed:var footerRows = grid.getFooterRows(); assertEquals(1, footerRows.size()); assertTrue(footerRows.get(0).getCells().stream() .anyMatch(c -> toolbarFooter.equals(c.getComponent().orElse(null))));This makes the test robust against regressions where the call silently becomes a no-op.
39-44
: PreferassertThrows
overexpected
attributeUsing
assertThrows
(JUnit 4.13+) gives more control and allows you to assert the exception message too:assertThrows(IllegalStateException.class, () -> GridHelper.addToolbarFooter(grid, toolbarFooter));It also isolates the failing statement, preventing false positives if another line unexpectedly throws.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/flowingcode/vaadin/addons/gridhelpers/FooterToolbarGridHelper.java
(2 hunks)src/main/java/com/flowingcode/vaadin/addons/gridhelpers/GridHelper.java
(2 hunks)src/test/java/com/flowingcode/vaadin/addons/gridhelpers/test/FooterToolbarTest.java
(2 hunks)
🔇 Additional comments (1)
src/main/java/com/flowingcode/vaadin/addons/gridhelpers/GridHelper.java (1)
351-360
: Clear, helpful Javadoc – thanks for adding itThe new comment precisely describes the pre-condition and the resulting exception, making the API contract explicit and discoverable via IDEs/Javadoc.
No further action needed.
src/main/java/com/flowingcode/vaadin/addons/gridhelpers/FooterToolbarGridHelper.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approved this, but then I realized there's a Sonar issue that might need to be addressed first.
re sonar, it was already there (5bd2788), I just refactored it. Any comments about that test are independent from this issue (Sonar is right, though, and FooterToolbarIT already covers it). re code rabbit, it was reported under #131 since it's not related to this PR |
Add a explicit check to ensure the grid has columns before the toolbar is initialized.
Close #129
Summary by CodeRabbit
Bug Fixes
Documentation
Tests