fix: track columns instead of cells #137
Conversation
WalkthroughAdds reflection-based mapping from a header/footer grid cell to its owning column and uses that mapping to correctly compute column indexes (handling joined cells). Adds test API to reorder columns and an integration test verifying header cell reuse and class mutations after column reordering. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
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. 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: 1
🧹 Nitpick comments (6)
src/test/java/com/flowingcode/vaadin/addons/gridhelpers/it/HeaderFooterStylesCallables.java (1)
37-38: Document index base and expected invariants for setColumnOrderClarify that indices are zero-based and must be a permutation of current grid.getColumns() to avoid misuse in tests.
src/main/java/com/flowingcode/vaadin/addons/gridhelpers/HeaderFooterStylesHelper.java (4)
155-160: Guard reflection and provide clear failure semanticsWhen reflection isn’t available, return the cell itself as identity; otherwise, wrap reflective errors into IllegalStateException for debuggability.
- @SneakyThrows - protected final /* AbstractColumn */ Object getColumn(CELL cell) { - AbstractCell.cast(cell); - Object result = AbstractCell_getColumn.invoke(cell); - return AbstractColumn.cast(result); - } + @SneakyThrows + protected final /* AbstractColumn or CELL */ Object getColumn(CELL cell) { + if (AbstractCell_getColumn == null) { + return cell; // fallback: use cell identity + } + try { + AbstractCell.cast(cell); + Object result = AbstractCell_getColumn.invoke(cell); + return AbstractColumn.cast(result); + } catch (Throwable t) { + throw new IllegalStateException("Unable to resolve owning column via reflection", t); + } + }
170-185: Null‑safety and tiny readability nitAdd a null-check when deriving curr to avoid surprising NPEs if Vaadin internals change. Also, rename variables for clarity.
- Object curr = getColumn(getCell(row, c)); + Object curr = Objects.requireNonNull(getColumn(getCell(row, c)), "cell/column identity must not be null");
206-215: Enrich exception message for faster diagnosticsThrow a descriptive message when the HeaderCell doesn’t belong to the grid.
- throw new IllegalArgumentException(); + throw new IllegalArgumentException("HeaderCell does not belong to any HeaderRow of this Grid");
230-239: Same as above for FooterCellImprove the exception message to ease debugging.
- throw new IllegalArgumentException(); + throw new IllegalArgumentException("FooterCell does not belong to any FooterRow of this Grid");src/test/java/com/flowingcode/vaadin/addons/gridhelpers/it/HeaderFooterStylesView.java (1)
113-119: Validate indices before reordering to prevent obscure failuresCheck bounds and uniqueness to fail fast in tests.
@Override public void setColumnOrder(int... columnIndexes) { - List<Column<Integer>> columns = grid.getColumns(); - grid.setColumnOrder( - IntStream.of(columnIndexes).mapToObj(columns::get).collect(Collectors.toList())); + List<Column<Integer>> columns = grid.getColumns(); + int size = columns.size(); + // bounds + IntStream.of(columnIndexes).forEach(idx -> { + if (idx < 0 || idx >= size) { + throw new IllegalArgumentException("Index out of range: " + idx + " (size=" + size + ")"); + } + }); + // uniqueness and completeness + long distinct = IntStream.of(columnIndexes).distinct().count(); + if (distinct != size) { + throw new IllegalArgumentException("Indices must be a permutation of 0.." + (size - 1)); + } + grid.setColumnOrder(IntStream.of(columnIndexes).mapToObj(columns::get).collect(Collectors.toList())); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/flowingcode/vaadin/addons/gridhelpers/HeaderFooterStylesHelper.java(4 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(2 hunks)src/test/java/com/flowingcode/vaadin/addons/gridhelpers/it/HeaderFooterStylesView.java(3 hunks)
🔇 Additional comments (4)
src/main/java/com/flowingcode/vaadin/addons/gridhelpers/HeaderFooterStylesHelper.java (2)
148-153: Capturing the owning column once is correctCaching the identity to anchor the cell across joins/reorders is the right approach.
127-160: Confirm Vaadin Flow versions targeted (keep 24+ fallback)Root pom.xml declares <vaadin.version> 23.3.15; profile "v23" has 23.3.5 and another profile sets 24.2.6 — repo targets both 23.x and 24.x. Verify that com.vaadin.flow.component.grid.AbstractRow$AbstractCell, com.vaadin.flow.component.grid.AbstractColumn and AbstractCell.getColumn() exist on every supported version; retain the fallback path for Vaadin 24+ if needed.
Applies to: src/main/java/com/flowingcode/vaadin/addons/gridhelpers/HeaderFooterStylesHelper.java lines 127–160 and 170–185.
src/test/java/com/flowingcode/vaadin/addons/gridhelpers/it/HeaderFooterStylesIT.java (1)
65-77: Good regression coverage for reordering + joined headersThe test captures the bug and validates identity tracking after setColumnOrder. Looks solid.
src/test/java/com/flowingcode/vaadin/addons/gridhelpers/it/HeaderFooterStylesView.java (1)
54-56: Setting both header and key is a nice touchThis helps with clarity and potential lookups. LGTM.
src/main/java/com/flowingcode/vaadin/addons/gridhelpers/HeaderFooterStylesHelper.java
Outdated
Show resolved
Hide resolved
javier-godoy
force-pushed
the
fix-134-2
branch
from
f71798d to
8d4c9ee
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/com/flowingcode/vaadin/addons/gridhelpers/HeaderFooterStylesHelper.java (1)
127-141: Make reflection init fully non-fatal (catch SecurityException/others and keep working).A SecurityException (setAccessible) or other linkage errors will currently bubble and fail class init. Fall back cleanly to the cell-identity path on any reflection failure.
Apply this diff:
private static final Method AbstractCell_getColumn; static { - Method method = null; - try { - Class<?> AbstractCell = - Class.forName("com.vaadin.flow.component.grid.AbstractRow$AbstractCell"); - method = AbstractCell.getDeclaredMethod("getColumn"); - method.setAccessible(true); - } catch (ClassNotFoundException | NoSuchMethodException e) { - // Will use cell identity; keep field null. - } - - AbstractCell_getColumn = method; + Method method = null; + try { + Class<?> abstractCell = + Class.forName("com.vaadin.flow.component.grid.AbstractRow$AbstractCell"); + method = abstractCell.getDeclaredMethod("getColumn"); + try { + method.setAccessible(true); + } catch (SecurityException ignore) { + // Best-effort; invocation may still work if public. + } + } catch (Throwable ignore) { + // Fallback: use cell identity; keep method null. + } + AbstractCell_getColumn = method; }
🧹 Nitpick comments (2)
src/main/java/com/flowingcode/vaadin/addons/gridhelpers/HeaderFooterStylesHelper.java (2)
39-39: Clean up imports after fallback refactor.Remove unused imports once resolveColumnOrFallback is in place.
-import java.util.Objects; -import lombok.SneakyThrows;Also applies to: 43-43
170-175: Document intentional identity semantics.Clarify that identity (==) is by design so joined cells/columns are counted once.
Object last = null; Object target = getColumn(); for (Column<?> c : helper.getGrid().getColumns()) { if (c.isVisible()) { Object curr = getColumn(getCell(row, c)); + // Intentional identity check: joined cells/columns reuse the same instance. if (curr != last) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/flowingcode/vaadin/addons/gridhelpers/HeaderFooterStylesHelper.java(4 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(2 hunks)src/test/java/com/flowingcode/vaadin/addons/gridhelpers/it/HeaderFooterStylesView.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/test/java/com/flowingcode/vaadin/addons/gridhelpers/it/HeaderFooterStylesCallables.java
- src/test/java/com/flowingcode/vaadin/addons/gridhelpers/it/HeaderFooterStylesView.java
- src/test/java/com/flowingcode/vaadin/addons/gridhelpers/it/HeaderFooterStylesIT.java
🔇 Additional comments (2)
src/main/java/com/flowingcode/vaadin/addons/gridhelpers/HeaderFooterStylesHelper.java (2)
207-207: Constructor order is fine once fallback is safe.Calling super(cell) before membership check is OK with the proposed safe fallback; nothing to change here.
If you don’t adopt the fallback change, consider moving super(cell) after the membership check to avoid possible NPE from Objects.requireNonNull when a foreign cell is passed.
231-231: Same note as header path.Footer path mirrors header; fine with the safe fallback in place.
src/main/java/com/flowingcode/vaadin/addons/gridhelpers/HeaderFooterStylesHelper.java
Show resolved
Hide resolved
|
@brunoagretti Can you please give it a try in your project? |
|
@javier-godoy I tried these changes in my project, and they successfully fixed the reported problem in #134 (comment) |
Close #134
Summary by CodeRabbit
New Features
Bug Fixes
Tests