feat: initial implementation#10
Draft
javier-godoy wants to merge 2 commits into
Draft
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 (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughAdds a new EasyGrid API and implementation: grid wrapper/composite, typed EasyColumn, multi-level column configuration (global/instance/column) with renderer factories, renderer utility classes, demos and model/test fixtures, documentation updates, and pom/.gitignore adjustments. ChangesConfiguration core
Core grid API & column wrapper
Renderer utilities
Demos, models and service fixtures
Tests and test utilities
Documentation, build, and metadata
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.
Comment on lines
+184
to
+189
| ``` | ||
| Column·Foo | ||
| → Instance·Foo → Instance·Entity → Instance·Object | ||
| → Global·Foo → Global·Entity → Global·Object | ||
| → Built-in default | ||
| ``` |
There was a problem hiding this comment.
Add a language to the fenced code block.
The block starting at Line 184 violates MD040 (missing fenced-code language).
Suggested fix
-```
+```text
Column·Foo
→ Instance·Foo → Instance·Entity → Instance·Object
→ Global·Foo → Global·Entity → Global·Object
→ Built-in default</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
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 184-184: 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 `@SPECIFICATIONS.md` around lines 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.
Comment on lines
+36
to
+43
| public NumberSample(BigDecimal value) { | ||
| this.bigDecimal = value; | ||
| this.bigInteger = value.toBigInteger(); | ||
| this.integer = bigInteger.intValue(); | ||
| this.intValue = integer; | ||
| this.longValue = bigInteger.longValue(); | ||
| this.doubleValue = value.doubleValue(); | ||
| } |
There was a problem hiding this comment.
Integer overflow in numeric conversions.
The constructor converts BigDecimal values to smaller integer types without overflow protection. The demo in NumberRenderingDemo.java (line 56) uses new BigDecimal("1234567890123.456"), which exceeds Integer.MAX_VALUE (2,147,483,647). When converted via toBigInteger().intValue(), this will silently overflow and produce incorrect values in the integer and intValue fields.
🛡️ Suggested approach
Either:
- Use smaller test values that fit within
int/longranges, or - Document that overflow is expected and intentional for the demo
If overflow is intentional, consider adding a comment explaining this behavior.
🤖 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/NumberSample.java`
around lines 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.
Comment on lines
+30
to
+37
| public List<Person> fetch(int offset, int limit) { | ||
| int end = offset + limit; | ||
| int size = personData.getPersons().size(); | ||
| if (size <= end) { | ||
| end = size; | ||
| } | ||
| return personData.getPersons().subList(offset, end); | ||
| } |
There was a problem hiding this comment.
Validate pagination bounds before subList call.
Line 36 can throw IndexOutOfBoundsException when offset < 0, limit < 0, or offset > size. Add explicit guards and clamp end safely.
Suggested fix
public List<Person> fetch(int offset, int limit) {
- int end = offset + limit;
- int size = personData.getPersons().size();
- if (size <= end) {
- end = size;
- }
- return personData.getPersons().subList(offset, end);
+ 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) {
+ 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 - 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.
Comment on lines
+126
to
+133
| } else if (type == boolean.class) { | ||
| return Boolean.TRUE; | ||
| } else if (type == int.class) { | ||
| return Integer.valueOf(0); | ||
| } else if (type.isArray()) { | ||
| return Array.newInstance(type.getComponentType(), 0); | ||
| } else { | ||
| return Mockito.mock(type); |
There was a problem hiding this comment.
Argument factory misses most primitive parameter types.
buildArg handles only boolean and int. If a delegated API adds long, double, etc., this base test will fail before delegation assertions run.
Suggested fix
} else if (type == boolean.class) {
return Boolean.TRUE;
} else if (type == int.class) {
return Integer.valueOf(0);
+ } else if (type == long.class) {
+ return Long.valueOf(0L);
+ } else if (type == double.class) {
+ return Double.valueOf(0d);
+ } else if (type == float.class) {
+ return Float.valueOf(0f);
+ } else if (type == short.class) {
+ return Short.valueOf((short) 0);
+ } else if (type == byte.class) {
+ return Byte.valueOf((byte) 0);
+ } else if (type == char.class) {
+ return Character.valueOf('a');
} else if (type.isArray()) {
return Array.newInstance(type.getComponentType(), 0);📝 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
| } else if (type == boolean.class) { | |
| return Boolean.TRUE; | |
| } else if (type == int.class) { | |
| return Integer.valueOf(0); | |
| } else if (type.isArray()) { | |
| return Array.newInstance(type.getComponentType(), 0); | |
| } else { | |
| return Mockito.mock(type); | |
| } else if (type == boolean.class) { | |
| return Boolean.TRUE; | |
| } else if (type == int.class) { | |
| return Integer.valueOf(0); | |
| } else if (type == long.class) { | |
| return Long.valueOf(0L); | |
| } else if (type == double.class) { | |
| return Double.valueOf(0d); | |
| } else if (type == float.class) { | |
| return Float.valueOf(0f); | |
| } else if (type == short.class) { | |
| return Short.valueOf((short) 0); | |
| } else if (type == byte.class) { | |
| return Byte.valueOf((byte) 0); | |
| } else if (type == char.class) { | |
| return Character.valueOf('a'); | |
| } else if (type.isArray()) { | |
| return Array.newInstance(type.getComponentType(), 0); | |
| } else { | |
| return Mockito.mock(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/test/java/com/flowingcode/vaadin/addons/easygrid/test/DelegationTest.java`
around lines 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.
Comment on lines
+188
to
+190
| protected boolean isChainable(Method targetMethod) { | ||
| return !method.getName().startsWith("get"); | ||
| } |
There was a problem hiding this comment.
Chainability detection is too brittle.
Using !method.getName().startsWith("get") misclassifies methods (e.g., non-get methods that return void/other types). Base behavior should derive chainability from return type instead.
Suggested fix
protected boolean isChainable(Method targetMethod) {
- return !method.getName().startsWith("get");
+ return targetMethod.getReturnType().isAssignableFrom(getTargetClass());
}📝 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
| protected boolean isChainable(Method targetMethod) { | |
| return !method.getName().startsWith("get"); | |
| } | |
| protected boolean isChainable(Method targetMethod) { | |
| return targetMethod.getReturnType().isAssignableFrom(getTargetClass()); | |
| } |
🤖 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/DelegationTest.java`
around lines 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.
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
1 participant
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
New Features
Documentation
Chores