feat: initial implementation#10
Merged
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a new EasyGrid API and implementation with multi-level ColumnConfiguration, renderer factories, bean-property support, multiple demos and models, extensive unit tests and parameterized delegation tests, documentation updates, and Maven/build adjustments. ChangesEasyGrid implementation, configuration, renderers and tests
Docs, build and miscellaneous
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (5)
src/test/java/com/flowingcode/vaadin/addons/easygrid/model/Person.java (1)
49-52: ⚡ Quick winReorganize field declarations before methods.
The
activefield (line 52) is declared after thegetAppointmentTime()method. Java convention is to group all field declarations together before methods. This makes the class structure clearer and easier to navigate.♻️ Suggested reorganization
private String phoneNumber; private LocalDate birthDate; private LocalDateTime appointmentDateTime; private boolean subscriber; + private boolean active; public LocalTime getAppointmentTime() { return appointmentDateTime != null ? appointmentDateTime.toLocalTime() : null; } - private boolean active;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/flowingcode/vaadin/addons/easygrid/model/Person.java` around lines 49 - 52, The field declaration for active is placed after the method getAppointmentTime() in class Person; move the private boolean active; declaration up into the block with the other field declarations (i.e. group all fields together at the top of the class) so that all fields are declared before any methods, preserving existing modifiers and order of other fields.src/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/RendererFactory.java (1)
33-33: ⚡ Quick winConsider adding
@FunctionalInterfaceannotation.While the interface correctly extends
SerializableFunctionand will be treated as a functional interface by the compiler, explicitly annotating it with@FunctionalInterfaceis a Java best practice that makes the intent clear and enables compile-time verification.📝 Suggested addition
/** * A named functional interface that creates a {`@link` Renderer} for a grid column given a * {`@link` com.vaadin.flow.function.ValueProvider}. * * `@param` <T> the grid bean type * `@param` <V> the column value type */ +@FunctionalInterface public interface RendererFactory<T, V> extends SerializableFunction<ValueProvider<T, V>, Renderer<T>> {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/RendererFactory.java` at line 33, Add the `@FunctionalInterface` annotation to the RendererFactory interface declaration (RendererFactory<T, V> extends SerializableFunction<ValueProvider<T, V>, Renderer<T>>) so the compiler enforces the functional-interface contract and the intent is explicit; simply annotate the interface line with `@FunctionalInterface` and keep the existing generic signature unchanged.src/main/java/com/flowingcode/vaadin/addons/easygrid/BeanPropertyDefinition.java (1)
74-80: ⚡ Quick winAvoid relying on Vaadin internal API for descriptor lookup.
Line 77 calls
com.vaadin.flow.internal.BeanUtil, which is explicitly marked "For internal use only. May be renamed or removed in a future release." Use the public JavaBeans API instead:new PropertyDescriptor(propertyName, beanType)directly instantiates the descriptor without requiring Vaadin internals.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/flowingcode/vaadin/addons/easygrid/BeanPropertyDefinition.java` around lines 74 - 80, The method getPropertyDescriptor in BeanPropertyDefinition currently uses the Vaadin internal BeanUtil (BeanUtil.getPropertyDescriptor) which is unsupported; replace that call with the public JavaBeans API by creating a new PropertyDescriptor(propertyName, beanType) and keep existing exception handling (catch IntrospectionException and rethrow as RuntimeReflectiveOperationException) so behavior remains the same; update imports to use java.beans.PropertyDescriptor and remove dependency on com.vaadin.flow.internal.BeanUtil.src/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalRendererFactoryTest.java (1)
36-41: 💤 Low valueConsider type-safe renderer factory invocation.
The explicit
<Object>type parameter combined with the cast toLocalDatebypasses compile-time type safety. While this works in the test context, a more type-safe approach would preserve the resolved type.♻️ Alternative approach
- var config = new InstanceEasyGridConfiguration().resolve(LocalDate.class); - var renderer = config.<Object>getRendererFactory().apply(v -> (LocalDate) v); + var config = new InstanceEasyGridConfiguration().resolve(LocalDate.class); + var renderer = config.getRendererFactory().apply(v -> v);The same pattern applies to the other test methods below.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalRendererFactoryTest.java` around lines 36 - 41, The test globalLocalDateRendererFactoryIsApplied uses an unchecked cast via config.<Object>getRendererFactory().apply(v -> (LocalDate) v), which bypasses compile-time type safety; change the generic invocation to the resolved type (use config.<LocalDate>getRendererFactory()) and provide a type-safe lambda (e.g., v -> v) so no cast is needed; update the same pattern in the other test methods that call InstanceEasyGridConfiguration.getRendererFactory to preserve type safety and avoid raw/unchecked casts, and verify the result is still instanceOf(LocalDateRenderer.class).src/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGridWrapper.java (1)
248-256: 💤 Low valueConsider excluding void.class from the primitive sortability check.
Line 252 uses
type.isPrimitive()to determine sortability, butvoid.class.isPrimitive()returnstrueeven though void values cannot be compared. While this is unlikely to occur in practice (since column value providers wouldn't return void), adding an explicit check would make the code more defensive.🛡️ Optional defensive check
- if (Comparable.class.isAssignableFrom(type) || type.isPrimitive()) { + if (Comparable.class.isAssignableFrom(type) || (type.isPrimitive() && type != void.class)) { column.setComparator((ValueProvider) getter); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGridWrapper.java` around lines 248 - 256, In createEasyColumn(Class<V> type, ValueProvider<T, V> getter) the sortability check currently treats all primitives as sortable; make it defensive by excluding void.class from the primitive branch so void values aren't considered comparable — change the condition that sets column.setComparator(...) (referencing createEasyColumn, type.isPrimitive(), and Comparable.class.isAssignableFrom) to only treat primitives as sortable when type != void.class.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CONFIGURATION_RESOLUTION.md`:
- Around line 13-20: Several fenced code blocks in CONFIGURATION_RESOLUTION.md
are unlabeled and trigger MD040; update each triple-backtick fence that contains
sequences like "I·Foo → I·Entity → G·Foo → G·Entity → Default", "I·Foo → G·Foo →
I·Entity → G·Entity → Default", "per-column → I·Foo → I·Entity → I·Object →
G·Foo → G·Entity → G·Object → Default", "I·Foo(impl) → I·Entity(impl) →
I·Object(impl)", "G·Foo(impl) → G·Entity(impl) → G·Object(impl)", and
"ColumnConfigurationLink(primary = I·Foo chain, fallback = G·Foo chain)" to use
a language identifier (e.g., text) after the opening ``` so the fences read
```text and satisfy markdown linting.
In `@FEATURE_ROW_ACTIONS.md`:
- Around line 51-54: The example uses easyGrid.addRowAction but refreshes using
grid.getDataProvider().refreshAll(), causing a variable-name mismatch; update
the refresh call to use easyGrid.getDataProvider().refreshAll() (or consistently
rename the grid variable so addRowAction, the deletion call
personService.delete(person), and the subsequent refresh all reference the same
grid instance) so the snippet is internally consistent.
In `@SPECIFICATIONS.md`:
- Around line 184-189: The fenced code block containing the snippet that starts
with "Column·Foo" is missing a language tag; update the opening fence from ```
to ```text so the block is explicitly marked (e.g., change the fence before
"Column·Foo" to "```text") to satisfy MD040; keep the block contents unchanged
and leave the closing fence as ``` to complete the fix.
In
`@src/main/java/com/flowingcode/vaadin/addons/easygrid/BeanPropertyDefinition.java`:
- Around line 66-69: The catch block in BeanPropertyDefinition that currently
catches NoSuchElementException | IllegalArgumentException and rethrows a new
IllegalArgumentException for "Can't resolve property name '" + propertyName + "'
from '" + propertySet + "'" must preserve the original exception as the cause;
modify the throw in that catch to pass the caught exception (e) as the cause
when constructing the new IllegalArgumentException so the root cause is retained
in stack traces and logs, keeping the same message but using the constructor
IllegalArgumentException(String, Throwable) (or equivalent) referencing
propertyName and propertySet.
In
`@src/main/java/com/flowingcode/vaadin/addons/easygrid/config/ColumnConfigurationLink.java`:
- Around line 38-41: The class ColumnConfigurationLink currently allows a null
primary which causes setters that call primary.* to NPE at runtime; ensure
primary is non-null by adding an explicit null check (e.g.
Objects.requireNonNull(primary, "primary must not be null")) in the
ColumnConfigurationLink constructor(s) and update any factory methods to enforce
the same, and keep fallback nullable as before; this guarantees setters that
call primary.* (and any methods in ColumnConfigurationLink that assume primary
!= null) are safe.
In
`@src/main/java/com/flowingcode/vaadin/addons/easygrid/config/GlobalEasyGridConfiguration.java`:
- Around line 70-87: There is a race between freeze() and forType(Class<V>)
because the check of the static boolean frozen and the call to
map.getOrCreate(type) are not atomic; fix it by synchronizing access: make
freeze() and forType(...) coordinate on the same lock (e.g., synchronize on
GlobalEasyGridConfiguration.class or add a private static final Object LOCK) and
perform the frozen check and the call to map.getOrCreate(type) inside the same
synchronized block (or use a ReentrantReadWriteLock with write lock in freeze()
and read lock in forType()) so no concurrent thread can create a registration
after freeze() completes.
In
`@src/test/java/com/flowingcode/vaadin/addons/easygrid/ColumnConfigurationDemo.java`:
- Around line 77-79: The Boolean formatter on
grid.getColumn("subscriber").as(Boolean.class).setFormatter(...) is not
null-safe; update the formatter to guard against null (e.g., treat null as false
or return an explicit placeholder like "Unknown") before dereferencing so it
never throws NPE. Modify the lambda passed to setFormatter on the subscriber
column to first check v for null and then return "Subscribed" or "Not
Subscribed" (or your chosen null placeholder) accordingly.
In
`@src/test/java/com/flowingcode/vaadin/addons/easygrid/model/NumberSample.java`:
- Around line 36-43: The constructor NumberSample(BigDecimal) currently converts
bigDecimal to smaller integer types without overflow checks (fields: bigDecimal,
bigInteger, integer, intValue, longValue, doubleValue), causing silent overflow
for values like 1234567890123.456; either: 1) update the conversions to use
exact checks (use bigInteger = value.toBigInteger(); integer =
bigInteger.intValueExact(); intValue = integer; longValue =
bigInteger.longValueExact();) so an ArithmeticException is thrown on overflow,
or 2) change the demo input in NumberRenderingDemo to a smaller BigDecimal that
fits int/long ranges, or add a comment documenting that overflow is intentional
— pick one approach and apply it consistently to NumberSample's constructor and
the demo.
In
`@src/test/java/com/flowingcode/vaadin/addons/easygrid/service/PersonService.java`:
- Around line 30-37: The fetch method should validate and clamp pagination
bounds before calling personData.getPersons().subList: ensure offset and limit
are non-negative (treat negative as 0), if limit == 0 return empty list
immediately, clamp offset to be at most size (if offset >= size return empty
list), compute end = min(offset + limit, size) and ensure end >= offset before
calling subList; update the fetch(...) implementation to perform these guards
using the existing fetch method and personData.getPersons() references.
In
`@src/test/java/com/flowingcode/vaadin/addons/easygrid/test/DelegationTest.java`:
- Around line 126-133: The buildArg method in DelegationTest only handles
boolean and int primitives and will fail for other primitive parameter types;
update buildArg to cover all primitive types (long, short, byte, char, float,
double) by adding branches like long.class -> Long.valueOf(0L), double.class ->
Double.valueOf(0.0), float.class -> Float.valueOf(0.0f), short.class ->
Short.valueOf((short)0), byte.class -> Byte.valueOf((byte)0), char.class ->
Character.valueOf('\0') while keeping the existing boolean and int branches, and
preserve the existing array and Mockito.mock(type) fallback so arrays and
reference types still behave as before.
- Around line 188-190: The isChainable method is using the wrong variable and a
brittle name-based heuristic; change it to inspect the method's return type
using the targetMethod parameter and treat methods returning void as
non-chainable (e.g., return !targetMethod.getReturnType().equals(Void.TYPE));
update references from method to targetMethod in isChainable to fix the variable
mismatch and derive chainability from the return type instead.
In `@src/test/java/com/flowingcode/vaadin/addons/easygrid/TypedColumnDemo.java`:
- Around line 68-73: The computed-days-until-birthday code in
TypedColumnDemo.java can throw for Feb 29 births when calling
p.getBirthDate().withYear(today.getYear()); fix by computing the birthday via
MonthDay (e.g., MonthDay birthMd = MonthDay.from(p.getBirthDate())), then create
birthday = birthMd.isValidYear(today.getYear()) ?
birthMd.atYear(today.getYear()) : MonthDay.of(2,28).atYear(today.getYear()) (or
another chosen fallback) so Feb 29 is handled safely, then keep the existing
logic that pushes the birthday to next year and returns today.until(birthday,
ChronoUnit.DAYS).
---
Nitpick comments:
In
`@src/main/java/com/flowingcode/vaadin/addons/easygrid/BeanPropertyDefinition.java`:
- Around line 74-80: The method getPropertyDescriptor in BeanPropertyDefinition
currently uses the Vaadin internal BeanUtil (BeanUtil.getPropertyDescriptor)
which is unsupported; replace that call with the public JavaBeans API by
creating a new PropertyDescriptor(propertyName, beanType) and keep existing
exception handling (catch IntrospectionException and rethrow as
RuntimeReflectiveOperationException) so behavior remains the same; update
imports to use java.beans.PropertyDescriptor and remove dependency on
com.vaadin.flow.internal.BeanUtil.
In `@src/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGridWrapper.java`:
- Around line 248-256: In createEasyColumn(Class<V> type, ValueProvider<T, V>
getter) the sortability check currently treats all primitives as sortable; make
it defensive by excluding void.class from the primitive branch so void values
aren't considered comparable — change the condition that sets
column.setComparator(...) (referencing createEasyColumn, type.isPrimitive(), and
Comparable.class.isAssignableFrom) to only treat primitives as sortable when
type != void.class.
In
`@src/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/RendererFactory.java`:
- Line 33: Add the `@FunctionalInterface` annotation to the RendererFactory
interface declaration (RendererFactory<T, V> extends
SerializableFunction<ValueProvider<T, V>, Renderer<T>>) so the compiler enforces
the functional-interface contract and the intent is explicit; simply annotate
the interface line with `@FunctionalInterface` and keep the existing generic
signature unchanged.
In `@src/test/java/com/flowingcode/vaadin/addons/easygrid/model/Person.java`:
- Around line 49-52: The field declaration for active is placed after the method
getAppointmentTime() in class Person; move the private boolean active;
declaration up into the block with the other field declarations (i.e. group all
fields together at the top of the class) so that all fields are declared before
any methods, preserving existing modifiers and order of other fields.
In
`@src/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalRendererFactoryTest.java`:
- Around line 36-41: The test globalLocalDateRendererFactoryIsApplied uses an
unchecked cast via config.<Object>getRendererFactory().apply(v -> (LocalDate)
v), which bypasses compile-time type safety; change the generic invocation to
the resolved type (use config.<LocalDate>getRendererFactory()) and provide a
type-safe lambda (e.g., v -> v) so no cast is needed; update the same pattern in
the other test methods that call
InstanceEasyGridConfiguration.getRendererFactory to preserve type safety and
avoid raw/unchecked casts, and verify the result is still
instanceOf(LocalDateRenderer.class).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a325efc3-43a7-4ca9-b6f0-4641a95438d9
📒 Files selected for processing (56)
.gitignoreCONFIGURATION_RESOLUTION.mdFEATURE_ROW_ACTIONS.mdREADME.mdSPECIFICATIONS.mdpom.xmlsrc/main/java/com/flowingcode/vaadin/addons/easygrid/BeanPropertyDefinition.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/EasyColumn.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGrid.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGridComposite.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGridWrapper.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/IEasyGridColumn.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/IEasyGridComposite.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/RuntimeReflectiveOperationException.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/ColumnConfiguration.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/ColumnConfigurationImpl.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/ColumnConfigurationLink.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/ColumnConfigurationTextRenderer.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/EasyGridConfigurationClassMap.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/GlobalEasyGridConfiguration.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/InstanceEasyGridConfiguration.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/LocalDateRenderers.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/LocalDateTimeRenderers.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/LocalTimeRenderers.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/NumberRenderers.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/RendererFactory.javasrc/test/java/com/flowingcode/vaadin/addons/DemoLayout.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/AutoColumnsDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/ColumnConfigurationDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/ConfigurationHierarchyDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/DemoView.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/EasyGridDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/EasyGridDemoView.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/GridColumnMethodLister.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/NumberRenderingDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/SelectiveColumnsDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/TypeRenderingDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/TypedColumnDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/data/PersonData.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/AbstractViewTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/ViewIT.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/model/Address.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/model/NumberSample.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/model/Person.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/service/PersonService.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/ColumnConfigurationImplParentDelegationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/ColumnConfigurationLinkDelegationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/DelegationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyColumnTestHelper.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyColumnToConfigurationDelegationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyColumnToGridColumnDelegationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalEasyGridConfigurationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalRendererFactoryTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/SerializationTest.javasrc/test/resources/META-INF/frontend/styles/easy-grid-demo-styles.csssrc/test/resources/META-INF/frontend/styles/shared-styles.css
💤 Files with no reviewable changes (2)
- src/test/java/com/flowingcode/vaadin/addons/easygrid/EasyGridDemo.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/it/ViewIT.java
Comment on lines
+13
to
+20
| ``` | ||
| I·Foo → I·Entity → G·Foo → G·Entity → Default | ||
| ``` | ||
|
|
||
| **Type-first** exhausts all Foo-specific settings before looking at Entity: | ||
| ``` | ||
| I·Foo → G·Foo → I·Entity → G·Entity → Default | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks.
Line 13, Line 18, Line 78, Line 86, Line 92, and Line 98 use unlabeled fences, which triggers MD040 and can fail markdown lint gates.
Suggested fix
-```
+```text
I·Foo → I·Entity → G·Foo → G·Entity → Default- +text
I·Foo → G·Foo → I·Entity → G·Entity → Default
-```
+```text
per-column → I·Foo → I·Entity → I·Object → G·Foo → G·Entity → G·Object → Default
- +text
I·Foo(impl) → I·Entity(impl) → I·Object(impl)
-```
+```text
G·Foo(impl) → G·Entity(impl) → G·Object(impl)
- +text
ColumnConfigurationLink(primary = I·Foo chain, fallback = G·Foo chain)
Also applies to: 78-100
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 13-13: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 18-18: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CONFIGURATION_RESOLUTION.md` around lines 13 - 20, Several fenced code blocks
in CONFIGURATION_RESOLUTION.md are unlabeled and trigger MD040; update each
triple-backtick fence that contains sequences like "I·Foo → I·Entity → G·Foo →
G·Entity → Default", "I·Foo → G·Foo → I·Entity → G·Entity → Default",
"per-column → I·Foo → I·Entity → I·Object → G·Foo → G·Entity → G·Object →
Default", "I·Foo(impl) → I·Entity(impl) → I·Object(impl)", "G·Foo(impl) →
G·Entity(impl) → G·Object(impl)", and "ColumnConfigurationLink(primary = I·Foo
chain, fallback = G·Foo chain)" to use a language identifier (e.g., text) after
the opening ``` so the fences read ```text and satisfy markdown linting.
Member
Author
There was a problem hiding this comment.
Defer to reviewers' judgment. This file is intended for AI consumption.
There was a problem hiding this comment.
@javier-godoy, understood — I'll dismiss this concern. Since CONFIGURATION_RESOLUTION.md is intended for AI consumption rather than human readers or standard documentation pipelines, markdown lint compliance is not a meaningful concern here.
✏️ Learnings added
Learnt from: javier-godoy
Repo: FlowingCode/EasyGridAddon PR: 10
File: CONFIGURATION_RESOLUTION.md:13-20
Timestamp: 2026-05-14T12:05:24.375Z
Learning: The file `CONFIGURATION_RESOLUTION.md` in the `FlowingCode/EasyGridAddon` repository is intended for AI consumption, not for human readers or standard documentation pipelines. Markdown style/lint issues (e.g., MD040 unlabeled fenced code blocks) should not be flagged in this file during code reviews.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Member
There was a problem hiding this comment.
It is okay for me to dismiss this.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/test/java/com/flowingcode/vaadin/addons/easygrid/TypedColumnDemo.java (1)
67-73:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle leap-day birthdays safely in computed column logic.
Line 69/Line 72 can throw at runtime for Feb 29 birthdays in non-leap years due to
withYear(...), which can break rendering for those rows.Suggested fix
import java.time.LocalDate; +import java.time.MonthDay; import java.time.temporal.ChronoUnit; @@ grid.addColumn(Integer.class, p -> { LocalDate today = LocalDate.now(); - LocalDate birthday = p.getBirthDate().withYear(today.getYear()); + MonthDay birthMd = MonthDay.from(p.getBirthDate()); + LocalDate birthday = birthMd.atYear(today.getYear()); if (birthday.isBefore(today)) { - birthday = p.getBirthDate().withYear(today.getYear() + 1); + birthday = birthMd.atYear(today.getYear() + 1); } return (int) ChronoUnit.DAYS.between(today, birthday); }).setHeader("Days Until Birthday");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/flowingcode/vaadin/addons/easygrid/TypedColumnDemo.java` around lines 67 - 73, The computed-birthday logic in grid.addColumn (the lambda using p.getBirthDate() and withYear(...)) can throw for Feb 29 birthdays in non-leap years; update the code that computes birthday to choose a valid day for the target year instead of blindly calling withYear. Concretely, compute targetYear = today.getYear() (and targetYear+1 where used), then build birthday using LocalDate.of(targetYear, birthDate.getMonthValue(), Math.min(birthDate.getDayOfMonth(), Year.of(targetYear).length())) (or wrap withYear in a try/catch and fallback to the last valid day of the month) so Feb 29s are mapped safely to Feb 28 in non-leap years before the before/after comparisons.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/test/java/com/flowingcode/vaadin/addons/easygrid/TypedColumnDemo.java`:
- Around line 67-73: The computed-birthday logic in grid.addColumn (the lambda
using p.getBirthDate() and withYear(...)) can throw for Feb 29 birthdays in
non-leap years; update the code that computes birthday to choose a valid day for
the target year instead of blindly calling withYear. Concretely, compute
targetYear = today.getYear() (and targetYear+1 where used), then build birthday
using LocalDate.of(targetYear, birthDate.getMonthValue(),
Math.min(birthDate.getDayOfMonth(), Year.of(targetYear).length())) (or wrap
withYear in a try/catch and fallback to the last valid day of the month) so Feb
29s are mapped safely to Feb 28 in non-leap years before the before/after
comparisons.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1b032e9f-8d3a-46cd-983c-6557e27a2096
📒 Files selected for processing (11)
pom.xmlsrc/main/java/com/flowingcode/vaadin/addons/easygrid/EasyColumn.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGridComposite.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGridWrapper.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/IEasyGridColumn.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/IEasyGridComposite.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/GlobalEasyGridConfiguration.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/TypedColumnDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/model/NumberSample.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/service/PersonService.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/DelegationTest.java
🚧 Files skipped from review as they are similar to previous changes (10)
- src/test/java/com/flowingcode/vaadin/addons/easygrid/model/NumberSample.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/service/PersonService.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGridComposite.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/config/GlobalEasyGridConfiguration.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/IEasyGridColumn.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGridWrapper.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/IEasyGridComposite.java
- pom.xml
- src/main/java/com/flowingcode/vaadin/addons/easygrid/EasyColumn.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/test/DelegationTest.java
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalEasyGridConfigurationFreezeTest.java (1)
51-54: 💤 Low valueConsider clarifying the test name.
The test name suggests it verifies that an entry is created, but the implementation only checks that
resolve()returns non-null without verifying whether a new entry was actually created. Since LocalDate is pre-registered (per the comment on line 71), this test may be verifying retrieval rather than creation. Consider renaming to something likeresolveBeforeFreezeReturnsNonNullor adding assertions to explicitly verify creation behavior if that's the intent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalEasyGridConfigurationFreezeTest.java` around lines 51 - 54, The test name resolveBeforeFreezeCreatesEntry is misleading because it only asserts GlobalEasyGridConfiguration.resolve(LocalDate.class) != null; either rename the test to resolveBeforeFreezeReturnsNonNull (or similar) to reflect its behavior, or modify the test to explicitly verify creation (e.g., assert absence in the registry before calling resolve and presence after) by using the registry accessors used elsewhere in GlobalEasyGridConfiguration; update the method name and any references accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyGridConstructionTest.java`:
- Around line 55-59: The test currently uses hasItems(...) which allows extra
unexpected columns; update the assertion in EasyGridConstructionTest so the
collected keys (from grid.getWrappedGrid().getColumns() -> keys) are asserted
for exact content/size — e.g., replace the hasItems assertion with an
exact-match assertion (such as assertThat(keys, contains(...)) or assertEquals
on a List of expected keys) to ensure no extra columns are present.
---
Nitpick comments:
In
`@src/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalEasyGridConfigurationFreezeTest.java`:
- Around line 51-54: The test name resolveBeforeFreezeCreatesEntry is misleading
because it only asserts GlobalEasyGridConfiguration.resolve(LocalDate.class) !=
null; either rename the test to resolveBeforeFreezeReturnsNonNull (or similar)
to reflect its behavior, or modify the test to explicitly verify creation (e.g.,
assert absence in the registry before calling resolve and presence after) by
using the registry accessors used elsewhere in GlobalEasyGridConfiguration;
update the method name and any references accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 17eb997e-af69-427a-8309-02c50e552807
📒 Files selected for processing (3)
src/test/java/com/flowingcode/vaadin/addons/easygrid/TypedColumnDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyGridConstructionTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalEasyGridConfigurationFreezeTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/com/flowingcode/vaadin/addons/easygrid/TypedColumnDemo.java
javier-godoy
force-pushed
the
first-iteration
branch
from
8002919 to
23b26a3
Compare
Member
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/test/java/com/flowingcode/vaadin/addons/easygrid/service/PersonService.java (1)
30-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate pagination inputs before
skip/limit.
offset < 0orlimit < 0will fail at runtime. Guard and clamp bounds before slicing.Suggested fix
public List<Person> fetch(int offset, int limit) { - return personData.getPersons().stream().skip(offset).limit(limit).toList(); + if (offset < 0 || limit < 0) { + throw new IllegalArgumentException("offset and limit must be non-negative"); + } + List<Person> persons = personData.getPersons(); + int size = persons.size(); + if (offset >= size || limit == 0) { + return List.of(); + } + int end = (int) Math.min((long) size, (long) offset + limit); + return persons.subList(offset, end); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/flowingcode/vaadin/addons/easygrid/service/PersonService.java` around lines 30 - 31, The fetch method may throw for negative pagination values; in PersonService.fetch validate and clamp offset/limit before slicing the list from personData.getPersons(): compute safeOffset = max(0, offset) and safeLimit = max(0, limit), get the list size, return empty list if safeOffset >= size, otherwise call stream().skip(safeOffset).limit(min(safeLimit, size - safeOffset)).toList() so skip/limit never receive negative or out-of-range values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/com/flowingcode/vaadin/addons/easygrid/config/GlobalEasyGridConfiguration.java`:
- Around line 104-106: resolve(...) returns the actual mutable
ColumnConfiguration instance when frozen, so callers can still mutate global
state; change resolve to return a defensive copy or an immutable/read-only view
when frozen (e.g., if frozen return a copy or an unmodifiable wrapper of
map.get(type) instead of the live instance). Locate the resolve method and
replace the frozen branch to return a safe copy (or wrap via a new
ReadOnlyColumnConfiguration(map.get(type)) or map.get(type).copy() / clone()),
ensuring nulls are handled the same as forType(type).
In
`@src/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyColumnTestHelper.java`:
- Around line 45-46: The mocked renderer factory ignores its input and always
renders item.toString(); update the Mockito stub for mock.getRendererFactory()
in EasyColumnTestHelper to use the provided getter parameter: return a
TextRenderer that calls getter.apply(item) (or getter.apply(item)::toString as
appropriate) so the renderer uses the actual value-provider passed into the
factory instead of Object::toString; keep using
com.vaadin.flow.data.renderer.TextRenderer and ensure the lambda references the
factory parameter named getter.
In `@src/test/resources/META-INF/frontend/styles/shared-styles.css`:
- Around line 1-20: The CSS comment blocks in shared-styles.css violate the
comment-whitespace-inside rule; fix by ensuring all block comments use a single
space after the opening /* and a single space before the closing */, e.g. change
/*- and /*Demo styles*/ to /* - ... */ and /* Demo styles */ (apply consistently
across the header license block and any short comments) so stylelint passes.
---
Duplicate comments:
In
`@src/test/java/com/flowingcode/vaadin/addons/easygrid/service/PersonService.java`:
- Around line 30-31: The fetch method may throw for negative pagination values;
in PersonService.fetch validate and clamp offset/limit before slicing the list
from personData.getPersons(): compute safeOffset = max(0, offset) and safeLimit
= max(0, limit), get the list size, return empty list if safeOffset >= size,
otherwise call stream().skip(safeOffset).limit(min(safeLimit, size -
safeOffset)).toList() so skip/limit never receive negative or out-of-range
values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 25cf5000-9af9-424b-8b60-6f55ca439bbe
📒 Files selected for processing (58)
.gitignoreCONFIGURATION_RESOLUTION.mdFEATURE_ROW_ACTIONS.mdREADME.mdSPECIFICATIONS.mdpom.xmlsrc/main/java/com/flowingcode/vaadin/addons/easygrid/BeanPropertyDefinition.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/EasyColumn.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGrid.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGridComposite.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGridWrapper.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/IEasyGridColumn.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/IEasyGridComposite.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/RuntimeReflectiveOperationException.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/ColumnConfiguration.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/ColumnConfigurationImpl.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/ColumnConfigurationLink.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/ColumnConfigurationTextRenderer.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/EasyGridConfigurationClassMap.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/GlobalEasyGridConfiguration.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/InstanceEasyGridConfiguration.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/LocalDateRenderers.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/LocalDateTimeRenderers.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/LocalTimeRenderers.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/NumberRenderers.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/RendererFactory.javasrc/test/java/com/flowingcode/vaadin/addons/DemoLayout.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/AutoColumnsDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/ColumnConfigurationDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/ConfigurationHierarchyDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/DemoView.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/EasyGridDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/EasyGridDemoView.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/GridColumnMethodLister.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/NumberRenderingDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/SelectiveColumnsDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/TypeRenderingDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/TypedColumnDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/data/PersonData.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/AbstractViewTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/ViewIT.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/model/Address.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/model/NumberSample.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/model/Person.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/service/PersonService.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/ColumnConfigurationImplParentDelegationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/ColumnConfigurationLinkDelegationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/DelegationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyColumnTestHelper.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyColumnToConfigurationDelegationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyColumnToGridColumnDelegationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyGridConstructionTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalEasyGridConfigurationFreezeTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalEasyGridConfigurationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalRendererFactoryTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/SerializationTest.javasrc/test/resources/META-INF/frontend/styles/easy-grid-demo-styles.csssrc/test/resources/META-INF/frontend/styles/shared-styles.css
💤 Files with no reviewable changes (2)
- src/test/java/com/flowingcode/vaadin/addons/easygrid/it/ViewIT.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/EasyGridDemo.java
Comment on lines
+104
to
+106
| public <V> ColumnConfiguration<V> resolve(Class<V> type) { | ||
| return frozen ? map.get(type) : forType(type); | ||
| } |
There was a problem hiding this comment.
resolve() currently allows post-freeze global mutation.
After freeze(), callers can still do GlobalEasyGridConfiguration.resolve(type).set..., which mutates global config and bypasses the “locked after startup” contract.
🔧 Minimal fix
- public <V> ColumnConfiguration<V> resolve(Class<V> type) {
+ static <V> ColumnConfiguration<V> resolve(Class<V> type) {
return frozen ? map.get(type) : forType(type);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public <V> ColumnConfiguration<V> resolve(Class<V> type) { | |
| return frozen ? map.get(type) : forType(type); | |
| } | |
| static <V> ColumnConfiguration<V> resolve(Class<V> type) { | |
| return frozen ? map.get(type) : forType(type); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/java/com/flowingcode/vaadin/addons/easygrid/config/GlobalEasyGridConfiguration.java`
around lines 104 - 106, resolve(...) returns the actual mutable
ColumnConfiguration instance when frozen, so callers can still mutate global
state; change resolve to return a defensive copy or an immutable/read-only view
when frozen (e.g., if frozen return a copy or an unmodifiable wrapper of
map.get(type) instead of the live instance). Locate the resolve method and
replace the frozen branch to return a safe copy (or wrap via a new
ReadOnlyColumnConfiguration(map.get(type)) or map.get(type).copy() / clone()),
ensuring nulls are handled the same as forType(type).
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Member
Author
There was a problem hiding this comment.
Yes. resolve() is package-private, so external callers cannot invoke it. The public API entry point is forType(), which throws IllegalStateException when frozen. The only remaining theoretical gap is a caller who retained a ColumnConfiguration reference obtained from forType() before freezing could still mutate it afterwards, but that is a general Java object-mutability concern rather than something this method exposes.
javier-godoy
force-pushed
the
first-iteration
branch
from
23b26a3 to
0691f32
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CONFIGURATION_RESOLUTION.md`:
- Around line 31-33: Update the docs to use the canonical API name: replace
examples calling easyGrid.configuration.forType(...) with the spec-defined
easyGrid.typeConfiguration(Class) usage; ensure all occurrences (including the
other example block currently showing easyGrid.configuration.forType) are
changed so the docs consistently reference easyGrid.typeConfiguration and its
Class-based signature.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cd8a906a-8d1b-4d18-b379-b191c49380a3
📒 Files selected for processing (58)
.gitignoreCONFIGURATION_RESOLUTION.mdFEATURE_ROW_ACTIONS.mdREADME.mdSPECIFICATIONS.mdpom.xmlsrc/main/java/com/flowingcode/vaadin/addons/easygrid/BeanPropertyDefinition.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/EasyColumn.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGrid.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGridComposite.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGridWrapper.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/IEasyGridColumn.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/IEasyGridComposite.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/RuntimeReflectiveOperationException.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/ColumnConfiguration.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/ColumnConfigurationImpl.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/ColumnConfigurationLink.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/ColumnConfigurationTextRenderer.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/EasyGridConfigurationClassMap.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/GlobalEasyGridConfiguration.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/InstanceEasyGridConfiguration.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/LocalDateRenderers.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/LocalDateTimeRenderers.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/LocalTimeRenderers.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/NumberRenderers.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/RendererFactory.javasrc/test/java/com/flowingcode/vaadin/addons/DemoLayout.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/AutoColumnsDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/ColumnConfigurationDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/ConfigurationHierarchyDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/DemoView.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/EasyGridDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/EasyGridDemoView.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/GridColumnMethodLister.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/NumberRenderingDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/SelectiveColumnsDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/TypeRenderingDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/TypedColumnDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/data/PersonData.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/AbstractViewTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/ViewIT.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/model/Address.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/model/NumberSample.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/model/Person.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/service/PersonService.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/ColumnConfigurationImplParentDelegationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/ColumnConfigurationLinkDelegationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/DelegationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyColumnTestHelper.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyColumnToConfigurationDelegationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyColumnToGridColumnDelegationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyGridConstructionTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalEasyGridConfigurationFreezeTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalEasyGridConfigurationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalRendererFactoryTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/SerializationTest.javasrc/test/resources/META-INF/frontend/styles/easy-grid-demo-styles.csssrc/test/resources/META-INF/frontend/styles/shared-styles.css
💤 Files with no reviewable changes (3)
- src/test/java/com/flowingcode/vaadin/addons/easygrid/EasyGridDemo.java
- src/test/resources/META-INF/frontend/styles/shared-styles.css
- src/test/java/com/flowingcode/vaadin/addons/easygrid/it/ViewIT.java
✅ Files skipped from review due to trivial changes (8)
- .gitignore
- src/main/java/com/flowingcode/vaadin/addons/easygrid/RuntimeReflectiveOperationException.java
- src/test/resources/META-INF/frontend/styles/easy-grid-demo-styles.css
- README.md
- src/main/java/com/flowingcode/vaadin/addons/easygrid/config/InstanceEasyGridConfiguration.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/it/AbstractViewTest.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/DemoView.java
- src/test/java/com/flowingcode/vaadin/addons/DemoLayout.java
🚧 Files skipped from review as they are similar to previous changes (44)
- src/test/java/com/flowingcode/vaadin/addons/easygrid/EasyGridDemoView.java
- FEATURE_ROW_ACTIONS.md
- src/test/java/com/flowingcode/vaadin/addons/easygrid/model/Address.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/model/NumberSample.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/GridColumnMethodLister.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalEasyGridConfigurationTest.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/model/Person.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/config/EasyGridConfigurationClassMap.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/LocalDateTimeRenderers.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/config/ColumnConfigurationTextRenderer.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/RendererFactory.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyColumnToGridColumnDelegationTest.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/NumberRenderingDemo.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalEasyGridConfigurationFreezeTest.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGrid.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyGridConstructionTest.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/TypedColumnDemo.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/test/SerializationTest.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/SelectiveColumnsDemo.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/config/ColumnConfigurationLink.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/config/ColumnConfiguration.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/NumberRenderers.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/ConfigurationHierarchyDemo.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/IEasyGridColumn.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyColumnTestHelper.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/service/PersonService.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/LocalDateRenderers.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/TypeRenderingDemo.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalRendererFactoryTest.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGridComposite.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/data/PersonData.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/config/ColumnConfigurationImpl.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGridWrapper.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/BeanPropertyDefinition.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/EasyColumn.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyColumnToConfigurationDelegationTest.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/LocalTimeRenderers.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/test/ColumnConfigurationImplParentDelegationTest.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/test/ColumnConfigurationLinkDelegationTest.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/config/GlobalEasyGridConfiguration.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/ColumnConfigurationDemo.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/IEasyGridComposite.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/test/DelegationTest.java
- pom.xml
javier-godoy
force-pushed
the
first-iteration
branch
from
0691f32 to
de3c379
Compare
paodb
requested changes
May 15, 2026
github-project-automation
Bot
moved this from To Do
to In Progress
in Flowing Code Addons
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 86-95: The snippet declares the same variable name twice (grid)
causing a compile error; rename the second EasyGrid instance and its usages to a
unique identifier such as explicitGrid: change the second declaration
EasyGrid<Person> grid to EasyGrid<Person> explicitGrid and update subsequent
calls grid.setItems(...) and add(grid) to explicitGrid.setItems(...) and
add(explicitGrid) to keep both examples in the same scope.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cd55f30e-3865-428f-9d5e-7279f1aad07a
📒 Files selected for processing (2)
FEATURE_ROW_ACTIONS.mdREADME.md
✅ Files skipped from review due to trivial changes (1)
- FEATURE_ROW_ACTIONS.md
Comment on lines
+86
to
+95
| ```java | ||
| // Auto-discover columns from bean properties | ||
| EasyGrid<Person> grid = new EasyGrid<>(Person.class); | ||
| grid.setItems(personService.findAll()); | ||
| add(grid); | ||
|
|
||
| // Or specify columns explicitly | ||
| EasyGrid<Person> grid = new EasyGrid<>(Person.class, "firstName", "lastName", "email"); | ||
| grid.setItems(personService.findAll()); | ||
| add(grid); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Java, can a local variable be redeclared in the same scope (e.g., EasyGrid grid twice in one method/block)?
💡 Result:
No, in Java, a local variable cannot be redeclared within the same scope [1][2][3]. If you attempt to declare a variable with the same name as an existing local variable in the same block or method, the Java compiler will report a compilation error [1][2][3]. Java uses block scoping, meaning a variable's scope is defined by the nearest enclosing curly braces {} [1][4]. Within that specific scope, each identifier must be unique [1][2]. For example, the following code is invalid and will fail to compile: void myMethod { EasyGrid grid = new EasyGrid<>; //... EasyGrid grid = new EasyGrid<>; // Compilation error: variable grid is already defined } However, you can declare a variable with the same name if it is in a different, nested scope (such as inside an inner block or a loop) [1][3]. In this case, the inner declaration shadows the outer one, meaning the outer variable becomes inaccessible by its simple name within that inner block [1][5][3]. void myMethod { EasyGrid grid = new EasyGrid<>; { EasyGrid grid = new EasyGrid<>; // This is allowed because it is in a new, nested scope } }
Citations:
- 1: https://stackoverflow.com/questions/30857753/re-declaring-variables-inside-loops-in-java
- 2: https://stackoverflow.com/questions/19531167/redeclaring-variables-and-scope
- 3: https://www.cs.emory.edu/~cheung/Courses/255/Syllabus/1-C-intro/scope-local.html
- 4: https://www.geeksforgeeks.org/java/variable-scope-in-java/
- 5: https://docs.oracle.com/javase/specs/jls/se21/html/jls-6.html
Split the two Java examples or rename the second variable.
grid is declared twice in the same scope, preventing the code snippet from compiling. Rename the second instance to explicitGrid:
Proposed doc fix
```java
// Auto-discover columns from bean properties
EasyGrid<Person> grid = new EasyGrid<>(Person.class);
grid.setItems(personService.findAll());
add(grid);
// Or specify columns explicitly
-EasyGrid<Person> grid = new EasyGrid<>(Person.class, "firstName", "lastName", "email");
-grid.setItems(personService.findAll());
-add(grid);
+EasyGrid<Person> explicitGrid = new EasyGrid<>(Person.class, "firstName", "lastName", "email");
+explicitGrid.setItems(personService.findAll());
+add(explicitGrid);</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 86 - 95, The snippet declares the same variable name
twice (grid) causing a compile error; rename the second EasyGrid instance and
its usages to a unique identifier such as explicitGrid: change the second
declaration EasyGrid<Person> grid to EasyGrid<Person> explicitGrid and update
subsequent calls grid.setItems(...) and add(grid) to explicitGrid.setItems(...)
and add(explicitGrid) to keep both examples in the same scope.
Member
There was a problem hiding this comment.
It's just a readme example, no need to fix anything.
There was a problem hiding this comment.
🧩 Analysis chain
Understood, @paodb — README snippets are illustrative and don't need to compile. I'll dismiss this comment.
(ˆ ᴗ ˆ)っ
🏁 Script executed:
#!/bin/bash
# Resolve the review comment
gh pr view 10 --repo FlowingCode/EasyGridAddon --json reviews,comments
# List bot comments to find the one to resolve
gh api graphql -f query='
{
repository(owner: "FlowingCode", name: "EasyGridAddon") {
pullRequest(number: 10) {
reviewThreads(first: 50) {
nodes {
id
isResolved
comments(first: 5) {
nodes {
id
body
author { login }
url
}
}
}
}
}
}
}'
Length of output: 233729
@paodb Understood. I’ll treat this README snippet as illustrative and won’t push this change. Marking it non-blocking on my side.
✏️ Learnings added
Learnt from: paodb
Repo: FlowingCode/EasyGridAddon PR: 10
File: README.md:86-95
Timestamp: 2026-05-18T14:34:28.752Z
Learning: Repo: FlowingCode/EasyGridAddon
Context: README.md code snippets
Learning: README examples are illustrative and need not be strictly compilable; avoid nitpicks like duplicate local variable declarations within a single snippet unless the team explicitly requests such edits.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
paodb
requested changes
May 18, 2026
Member
paodb
left a comment
paodb
left a comment
There was a problem hiding this comment.
On a Claude review, this was mention. I think it's worth taking a look:
null-representation falls off the chain when setFormatter is called on an instance-type config.
ColumnConfigurationImpl.setFormatter (ColumnConfigurationImpl.java:87) captures this — the impl — in the renderer:
setRendererFactory(getter -> new ColumnConfigurationTextRenderer<>(getter, this, formatter));
When the call comes through EasyColumn.setFormatter (per-column layer above the Link), this is the column-level impl whose parent chain does include the Link, so it works. But when the call comes through EasyGrid.typeConfiguration(X).setFormatter(...), Link.setFormatter forwards to primary.setFormatter, so this ends up being I·X_impl. Its parent chain is purely the instance side (I·X → I·Object) and never reaches the global chain — so G·Object.nullRepresentation = "" (the built-in default) is not honored, and null values fall through to null/empty in the renderer. Fix would be to have Link.setFormatter build the renderer using the link itself, or have setFormatter accept the lookup config explicitly.
paodb
approved these changes
May 18, 2026
github-project-automation
Bot
moved this from In Progress
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
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.
Summary by CodeRabbit