fix: fix column index calculation when cells are joined #135
Conversation
WalkthroughUpdates HeaderFooterStylesHelper's column-index computation to treat consecutive columns mapping to the same header/footer cell as a single index step. Adds integration test support: RPC interfaces, test view, integration test, and DOM helpers to verify header cell classes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Pre-merge checks (4 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 0
🧹 Nitpick comments (14)
src/main/java/com/flowingcode/vaadin/addons/gridhelpers/HeaderFooterStylesHelper.java (3)
133-151: Joined-cells index calc: correct approach; remove unused param and harden lookupLogic to increment only on cell transitions fixes #134. Minor refinements:
- Drop the unused
rowIndexparam.- Cache
targetand return-1if not found (defensive).- private int getColumnIndex(int rowIndex) { + private int getColumnIndex() { ROW row = getRowSelector().getRow(); int j = -1; - - CELL last = null; + CELL last = null; + final CELL target = getCell(); for (Column<?> c : helper.getGrid().getColumns()) { if (c.isVisible()) { - CELL curr = getCell(row, c); - if (curr != last) { - ++j; - last = curr; - if (curr == getCell()) { - break; - } - } + CELL curr = getCell(row, c); + if (curr == last) continue; + ++j; + last = curr; + if (curr == target) return j; } } - return j; + return -1; // not found; selector will be null }Also update the caller:
// outside the selected range (in getSelector) int j = getColumnIndex();
141-146: Intentional reference equalityUsing
==/!=forCELLcomparison is correct here because joined header/footer cells are the same instance across columns. Consider a short comment to preempt “use equals()” static-analysis noise.
5-5: License yearYear range updated here. Ensure other modified files carry the same range for consistency.
src/test/java/com/flowingcode/vaadin/addons/gridhelpers/it/GridHelperElement.java (3)
131-135: Guard against OOB and improve error messagesAdd bounds checks so failures are clearer during debugging.
- public WebElement getHeaderRow(int rowIndex) { - WebElement thead = $("*").id("header"); - List<WebElement> headerRows = thead.findElements(By.tagName("tr")); - return headerRows.get(rowIndex); - } + public WebElement getHeaderRow(int rowIndex) { + WebElement thead = $("*").id("header"); + List<WebElement> headerRows = thead.findElements(By.tagName("tr")); + if (rowIndex < 0 || rowIndex >= headerRows.size()) { + throw new IndexOutOfBoundsException("rowIndex=" + rowIndex + ", headerRows=" + headerRows.size()); + } + return headerRows.get(rowIndex); + }
137-140: Cell index bounds checksSame here for columns.
- public WebElement getHeaderCell(int rowIndex, int columnIndex) { - List<WebElement> headerCells = getHeaderRow(rowIndex).findElements(By.tagName("th")); - return headerCells.get(columnIndex); - } + public WebElement getHeaderCell(int rowIndex, int columnIndex) { + List<WebElement> headerCells = getHeaderRow(rowIndex).findElements(By.tagName("th")); + if (columnIndex < 0 || columnIndex >= headerCells.size()) { + throw new IndexOutOfBoundsException( + "rowIndex=" + rowIndex + ", columnIndex=" + columnIndex + ", headerCells=" + headerCells.size()); + } + return headerCells.get(columnIndex); + }
131-140: Prefer TestBenchElement for chainingReturning TestBenchElement improves fluency with TB APIs (optional).
src/test/java/com/flowingcode/vaadin/addons/gridhelpers/it/HeaderFooterStylesIT.java (2)
56-61: Stabilize assertions with an explicit waitClass mutations are scheduled via beforeClientResponse; add a small wait to avoid flakiness.
- assertEquals("row0-cell0", grid.getHeaderCell(0, 0).getAttribute("class")); - assertEquals("row0-cell1", grid.getHeaderCell(0, 1).getAttribute("class")); + waitUntil(driver -> "row0-cell0".equals(grid.getHeaderCell(0, 0).getAttribute("class"))); + waitUntil(driver -> "row0-cell1".equals(grid.getHeaderCell(0, 1).getAttribute("class")));
45-63: Name the test after the behavior under testOptional: rename to clarify the intent.
- public void testHeaderVisible() { + public void testHeaderClassesAppliedToJoinedCells() {src/test/java/com/flowingcode/vaadin/addons/gridhelpers/it/HeaderFooterStylesCallables.java (2)
28-31: Clarify index semantics (visible vs. all columns) and preconditions.Explicitly documenting that indexes are 0-based and refer to visible columns will keep tests aligned with HeaderFooterStylesHelper’s visible-column logic and prevent ambiguity when columns are hidden.
public interface HeaderRowWrapper extends GridStylesHelper, RmiRemote { - HeaderCellWrapper getCell(int columnIndex); - HeaderCellWrapper join(int... columnIndexes); + /** Returns the header cell by visible column index (0-based). */ + HeaderCellWrapper getCell(int columnIndex); + /** + * Joins consecutive visible columns by index (0-based). + * Requires at least two indexes. + */ + HeaderCellWrapper join(int... columnIndexes); }
35-35: Document rowIndex meaning.Small doc helps: rowIndex is 0-based in grid.getHeaderRows() order (top to bottom).
- HeaderRowWrapper getRow(int rowIndex); + /** Returns the header row by index (0-based, topmost header row is 0). */ + HeaderRowWrapper getRow(int rowIndex);src/test/java/com/flowingcode/vaadin/addons/gridhelpers/it/HeaderFooterStylesView.java (4)
98-113: Map by visible-column index; add preconditions; use extension methods consistently.Avoid addressing columns by absolute index from getColumns() (includes hidden ones). This keeps the test API consistent with the helper’s visible-column semantics and prevents brittle failures when columns are toggled. Also enforce minimal args for join and prefer the extension method consistently.
private final class HeaderRowWrapperImpl extends StylesWrapper implements HeaderRowWrapper { private final HeaderRow row; public HeaderRowWrapperImpl(HeaderRow row) { - super(GridHelper.getHeaderStyles(grid, row)); + super(grid.getHeaderStyles(row)); this.row = row; } + private Column<?> visibleColumnAt(int visibleIndex) { + int j = -1; + for (Column<?> c : grid.getColumns()) { + if (c.isVisible() && ++j == visibleIndex) { + return c; + } + } + throw new IndexOutOfBoundsException("Visible columnIndex out of range: " + visibleIndex); + } + @Override public HeaderCellWrapper getCell(int columnIndex) { - HeaderCell cell = row.getCell(grid.getColumns().get(columnIndex)); - return new HeaderCellWrapperImpl(grid.getHeaderStyles(cell)); + HeaderCell cell = row.getCell(visibleColumnAt(columnIndex)); + return new HeaderCellWrapperImpl(grid.getHeaderStyles(cell)); } @Override public HeaderCellWrapper join(int... columnIndexes) { - HeaderCell cell = row.join(IntStream.of(columnIndexes) - .mapToObj(grid.getColumns()::get) - .toArray(Column[]::new)); + if (columnIndexes == null || columnIndexes.length < 2) { + throw new IllegalArgumentException("join requires at least 2 visible column indexes"); + } + Column<?>[] cols = IntStream.of(columnIndexes) + .mapToObj(this::visibleColumnAt) + .toArray(Column[]::new); + HeaderCell cell = row.join(cols); cell.setText("join " + IntStream.of(columnIndexes) .mapToObj(Integer::toString) .collect(Collectors.joining())); - return new HeaderCellWrapperImpl(GridHelper.getHeaderStyles(grid, cell)); + return new HeaderCellWrapperImpl(grid.getHeaderStyles(cell)); }
118-121: Bounds-check rowIndex for clearer failures.Helpful in IT logs and prevents NPEs from deeper layers.
@Override public HeaderRowWrapper getRow(int rowIndex) { - HeaderRow row = grid.getHeaderRows().get(rowIndex); + if (rowIndex < 0 || rowIndex >= grid.getHeaderRows().size()) { + throw new IndexOutOfBoundsException("rowIndex out of range: " + rowIndex); + } + HeaderRow row = grid.getHeaderRows().get(rowIndex); return new HeaderRowWrapperImpl(row); }
46-51: Make grid final and assign via this for clarity/immutability.Small cleanup; reduces accidental reassignment.
- private Grid<Integer> grid; + private final Grid<Integer> grid; @@ - grid = new Grid<>(); + this.grid = new Grid<>();
58-68: Remove commented-out placeholder code.Keeps the IT view minimal and focused.
- // HeaderRow row0 = ; - // HeaderRow row1 = grid.prependHeaderRow(); - // HeaderCell cell01 = row1.join(col0, col1); - // HeaderCell cell23 = row1.join(col2, col3); - // grid.getHeaderStyles(row0.getCell(col0)).setClassName("cell0"); - // grid.getHeaderStyles(row0.getCell(col1)).setClassName("cell1"); - // grid.getHeaderStyles(row0).setClassName("row0"); - // grid.getHeaderStyles(cell01).setClassName("cell01"); - // grid.getHeaderStyles(cell23).setClassName("cell23"); - // grid.getHeaderStyles(row1).setClassName("row1");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/com/flowingcode/vaadin/addons/gridhelpers/HeaderFooterStylesHelper.java(2 hunks)src/test/java/com/flowingcode/vaadin/addons/gridhelpers/it/GridHelperElement.java(2 hunks)src/test/java/com/flowingcode/vaadin/addons/gridhelpers/it/HeaderFooterStylesCallables.java(1 hunks)src/test/java/com/flowingcode/vaadin/addons/gridhelpers/it/HeaderFooterStylesIT.java(1 hunks)src/test/java/com/flowingcode/vaadin/addons/gridhelpers/it/HeaderFooterStylesView.java(1 hunks)
🔇 Additional comments (4)
src/main/java/com/flowingcode/vaadin/addons/gridhelpers/HeaderFooterStylesHelper.java (1)
153-158: nth-child offset remains correct
j + 1aligns the zero-based computed index with CSS 1-basednth-child. Looks good.src/test/java/com/flowingcode/vaadin/addons/gridhelpers/it/HeaderFooterStylesIT.java (2)
45-55: Good coverage of joined vs. non-joined rowsTest exercises both a joined header row and a non-joined row. Nicely isolates the regression.
51-55: Confirm intended number of cells in row 0Row 0 joins (0–1) and (2–3). If a 5th column exists, there might be a third header cell (col 4). If intentional to have only two cells, please confirm the view setup; otherwise, consider asserting the third cell too.
src/test/java/com/flowingcode/vaadin/addons/gridhelpers/it/HeaderFooterStylesCallables.java (1)
26-37: LGTM overall — clean RPC contract for tests.Interfaces extend RmiRemote correctly and the surface matches the intended test usage.
javier-godoy
force-pushed
the
fix-134
branch
from
e00bb22 to
431432f
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/test/java/com/flowingcode/vaadin/addons/gridhelpers/it/GridHelperElement.java (1)
131-140: Harden header accessors (bounds checks; return TestBenchElement for consistency).Current methods can throw IndexOutOfBoundsException with little context and return WebElement while most helpers return TestBenchElement. Add explicit bounds validation and use TestBenchElement throughout.
- public WebElement getHeaderRow(int rowIndex) { - WebElement thead = $("*").id("header"); - List<WebElement> headerRows = thead.findElements(By.tagName("tr")); - return headerRows.get(rowIndex); - } + public TestBenchElement getHeaderRow(int rowIndex) { + TestBenchElement thead = $(TestBenchElement.class).id("header"); + List<TestBenchElement> headerRows = thead.$(TestBenchElement.class).tagName("tr").all(); + if (rowIndex < 0 || rowIndex >= headerRows.size()) { + throw new IndexOutOfBoundsException("Header row index out of bounds: " + rowIndex); + } + return headerRows.get(rowIndex); + } - public WebElement getHeaderCell(int rowIndex, int columnIndex) { - List<WebElement> headerCells = getHeaderRow(rowIndex).findElements(By.tagName("th")); - return headerCells.get(columnIndex); - } + public TestBenchElement getHeaderCell(int rowIndex, int columnIndex) { + List<TestBenchElement> headerCells = + getHeaderRow(rowIndex).$(TestBenchElement.class).tagName("th").all(); + if (columnIndex < 0 || columnIndex >= headerCells.size()) { + throw new IndexOutOfBoundsException("Header column index out of bounds: " + columnIndex); + } + return headerCells.get(columnIndex); + }Optional: if Vaadin ever renames the internal “header” id, consider scoping via executeScript on this element’s shadowRoot rather than relying on a fixed id.
src/main/java/com/flowingcode/vaadin/addons/gridhelpers/HeaderFooterStylesHelper.java (1)
133-151: Remove unused parameter and micro-cleanup.rowIndex is unused; simplifying the signature clarifies intent. Also cache getCell() to avoid repeated calls inside the loop.
- private int getColumnIndex(int rowIndex) { + private int getColumnIndex() { ROW row = getRowSelector().getRow(); int j = -1; - CELL last = null; + CELL last = null; + CELL target = getCell(); for (Column<?> c : helper.getGrid().getColumns()) { if (c.isVisible()) { - CELL curr = getCell(row, c); + CELL curr = getCell(row, c); if (curr != last) { ++j; last = curr; - if (curr == getCell()) { + if (curr == target) { break; } } } } return j; } @@ public final Serializable[] getSelector() { int i = getRowSelector().getRowIndex(); - int j = getColumnIndex(i); + int j = getColumnIndex(); return j < 0 ? null : new Serializable[] {getSelectorTemplate(), i, j + 1}; }Also applies to: 155-158
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/com/flowingcode/vaadin/addons/gridhelpers/HeaderFooterStylesHelper.java(2 hunks)src/test/java/com/flowingcode/vaadin/addons/gridhelpers/it/GridHelperElement.java(2 hunks)src/test/java/com/flowingcode/vaadin/addons/gridhelpers/it/HeaderFooterStylesCallables.java(1 hunks)src/test/java/com/flowingcode/vaadin/addons/gridhelpers/it/HeaderFooterStylesIT.java(1 hunks)src/test/java/com/flowingcode/vaadin/addons/gridhelpers/it/HeaderFooterStylesView.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/test/java/com/flowingcode/vaadin/addons/gridhelpers/it/HeaderFooterStylesView.java
- src/test/java/com/flowingcode/vaadin/addons/gridhelpers/it/HeaderFooterStylesIT.java
- src/test/java/com/flowingcode/vaadin/addons/gridhelpers/it/HeaderFooterStylesCallables.java
🔇 Additional comments (3)
src/test/java/com/flowingcode/vaadin/addons/gridhelpers/it/GridHelperElement.java (1)
5-5: License year bump — OK.src/main/java/com/flowingcode/vaadin/addons/gridhelpers/HeaderFooterStylesHelper.java (2)
5-5: License year bump — OK.
137-147: Correctly collapse joined cells — fix looks right.Incrementing the index only when the cell instance changes eliminates overcounting across colspans and aligns nth-child with the rendered TH sequence. Nice.
github-project-automation
bot
moved this from To Do
to Pending release
in Flowing Code Addons
2 participants
Close #134
Summary by CodeRabbit
Bug Fixes
Tests