feat: implement row actions for EasyGrid#16
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughImplements EasyGrid row actions with new APIs, renderer strategies, frontend assets, demos, tests, and Maven updates. The row-actions feature document is rewritten to match the implemented API. ChangesEasyGrid Row Actions
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@pom.xml`:
- Around line 73-77: The json-migration-helper dependency is currently part of
the main build graph even though it is only used by test classes like
EasyRowActionITView and ViewInitializerImpl. Update the dependency entry in
pom.xml to add test scope so it is only available during tests and does not leak
into the published add-on or its transitive dependencies.
In
`@src/main/java/com/flowingcode/vaadin/addons/easygrid/actions/DropdownMenuRowActionsRenderer.java`:
- Around line 54-75: The DropdownMenuRowActionsRenderer is calling Vaadin’s
private context-menu connector through
grid.$contextMenuTargetConnector.openOnHandler and removeListener(), which is
version-sensitive. Update the handling in createContextMenu() and the row-action
handler setup to use the public Grid/GridContextMenu APIs for opening and
managing the context menu, and remove any direct executeJs access to the
internal connector.
In
`@src/main/java/com/flowingcode/vaadin/addons/easygrid/actions/EasyRowAction.java`:
- Around line 301-304: The EasyRowAction title handling is stripping manually
set title attributes during refresh. Update the attribute forwarding in
EasyRowAction so refreshRowActions() preserves a caller-provided
getElement().setAttribute("title", ...) value unless an explicit tooltip
provider is being used, and keep the per-row bind("title", tooltipProvider)
behavior in sync with that logic. Use the copyAllAttributesAndPropertiesExcept
and bind("title", tooltipProvider) paths to ensure the title field supports both
manual attributes/properties and dynamic tooltips.
- Around line 258-265: The confirm flow in EasyRowAction.execute() bypasses the
server-side row guard by calling actionHandler.accept(item) directly from the
ConfirmDialog confirm listener. Update the confirm listener to re-check the same
visibleWhen/enabledWhen condition used before opening the dialog, and only
invoke the action when the row still passes the guard. Use the existing
execute(), confirmDialogSupplier, and actionHandler logic as the place to apply
this check.
In
`@src/test/java/com/flowingcode/vaadin/addons/easygrid/it/AbstractViewTest.java`:
- Around line 66-82: The semaphore in AbstractViewTest.setup() is being released
too early, so the browser limit is not enforced during the actual test
execution. Move the permit release out of setup and hold it for the full
lifecycle by pairing semaphore.acquire() with a release in a teardown method or
equivalent final cleanup path, ensuring the ChromeDriver/TestBench driver
created in setup remains counted until the test finishes.
In
`@src/test/java/com/flowingcode/vaadin/addons/easygrid/it/EasyRowActionIT.java`:
- Around line 243-250: The test in EasyRowActionIT opens a second context menu
immediately after sending ESCAPE without waiting for the first overlay to
disappear, which makes the interaction flaky. Update the sequence around the
first grid.getCell(...).contextClick() so the menu is closed through the
existing closeMenu() helper before opening the next row’s context menu. Keep the
fix localized to the context-menu assertions in EasyRowActionIT and reuse the
helper rather than relying on a raw ESCAPE keypress alone.
In
`@src/test/java/com/flowingcode/vaadin/addons/easygrid/RowActionsDynamicDemo.java`:
- Around line 54-58: The demo comment and behavior around
RowActionsDynamicDemo#visibleWhen are inconsistent with the tested contract:
fluent mutators should update the row action state without requiring a separate
refresh. Update the demo to match the API behavior enforced by EasyRowActionIT
by either removing the note about needing refreshRowActions() or adding the
explicit refresh call after changing visibleWhen. Keep the example centered on
editAction and the Checkbox listener so the demo reflects the same
self-refreshing behavior as the tests.
🪄 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: 7a18e1ed-ad7a-4f1e-accd-a4f326f91b59
📒 Files selected for processing (31)
FEATURE_ROW_ACTIONS.mdpom.xmlsrc/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGridWrapper.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/AbstractContextMenuRowActionsRenderer.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/Constant.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/ContextMenuRowActionsRenderer.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/DropdownMenuRowActionsRenderer.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/EasyRowAction.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/HasRowActions.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/LitRendererBuilder.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/LitRowActionsRenderer.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/RowActionsManager.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/RowActionsRenderer.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/RowActionsStyle.javasrc/main/resources/META-INF/frontend/fc-dynamic-buttons.csssrc/main/resources/META-INF/frontend/fc-icon.tssrc/main/resources/META-INF/frontend/styles/static_addon_stylessrc/test/java/com/flowingcode/vaadin/addons/easygrid/EasyGridDemoView.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/RowActionsDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/RowActionsDynamicDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/RowActionsMenuDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/actions/EasyRowActionTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/actions/LitRendererBuilderTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/AbstractViewTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/EasyRowActionIT.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/EasyRowActionITCallables.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/EasyRowActionITView.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/RmiEasyRowAction.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/ViewInitializerImpl.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyRowActionUnitTest.javasrc/test/resources/META-INF/services/com.vaadin.flow.server.VaadinServiceInitListener
💤 Files with no reviewable changes (1)
- src/main/resources/META-INF/frontend/styles/static_addon_styles
| String handler = """ | ||
| ev=>{const grid = ev.composedPath() | ||
| .find(el => el.matches?.('vaadin-grid-cell-content')).parentElement | ||
| grid.preventContextMenu=false; | ||
| grid.$contextMenuTargetConnector.openOnHandler.call(grid,ev); | ||
| } | ||
| """; | ||
| var button = new EasyRowAction<T>(null, null, dots, handler); | ||
| button.updateRenderer(builder); | ||
| column = grid.addColumn(builder.build()); | ||
| column.setAutoWidth(true); | ||
| } | ||
|
|
||
| super.update(actions); | ||
| } | ||
|
|
||
| @Override | ||
| protected GridContextMenu<T> createContextMenu() { | ||
| var menu = super.createContextMenu(); | ||
| grid.getElement().executeJs(""" | ||
| setTimeout(()=>this.$contextMenuTargetConnector.removeListener()); | ||
| """); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant files and show compact structure first.
git ls-files | rg '^src/main/java/com/flowingcode/vaadin/addons/easygrid/actions/(DropdownMenuRowActionsRenderer|GridContextMenu|EasyGridWrapper|.*ContextMenu.*|.*RowActions.*)\.java$' || true
printf '\n== outline: DropdownMenuRowActionsRenderer ==\n'
ast-grep outline src/main/java/com/flowingcode/vaadin/addons/easygrid/actions/DropdownMenuRowActionsRenderer.java --view expanded || true
printf '\n== outline: related context menu files ==\n'
for f in $(git ls-files 'src/main/java/com/flowingcode/vaadin/addons/easygrid/actions/*.java' | rg 'ContextMenu|RowActions|EasyGridWrapper|DropdownMenu'); do
echo "--- $f"
ast-grep outline "$f" --view expanded || true
doneRepository: FlowingCode/EasyGridAddon
Length of output: 5256
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the target file around the relevant lines, plus related definitions/usages.
file='src/main/java/com/flowingcode/vaadin/addons/easygrid/actions/DropdownMenuRowActionsRenderer.java'
nl -ba "$file" | sed -n '1,220p'
printf '\n== grep for $contextMenuTargetConnector usage ==\n'
rg -n '\$contextMenuTargetConnector|removeListener\(\)|openOnHandler' src/main/java -S
printf '\n== show related GridContextMenu / context menu classes ==\n'
for f in $(git ls-files 'src/main/java/com/flowingcode/vaadin/addons/easygrid/actions/*.java' | rg 'ContextMenu|DropdownMenu|RowActions|EasyGridWrapper'); do
echo "--- $f"
nl -ba "$f" | sed -n '1,260p' | rg -n '\$contextMenuTargetConnector|removeListener\(\)|openOnHandler|context menu|createContextMenu|update\(' -n -C 3 || true
doneRepository: FlowingCode/EasyGridAddon
Length of output: 203
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If the code references Vaadin private APIs, inspect whether the implementation
# already isolates per-grid state or shares a listener across instances.
rg -n 'class .*GridContextMenu|extends GridContextMenu|createContextMenu\(|removeListener\(|openOnHandler|contextMenuTargetConnector' src/main/java src/test/java -SRepository: FlowingCode/EasyGridAddon
Length of output: 1332
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='src/main/java/com/flowingcode/vaadin/addons/easygrid/actions/AbstractContextMenuRowActionsRenderer.java'
echo "== $file =="
sed -n '1,220p' "$file"
file='src/main/java/com/flowingcode/vaadin/addons/easygrid/actions/DropdownMenuRowActionsRenderer.java'
echo
echo "== $file =="
sed -n '1,220p' "$file"
printf '\n== any tests/usages for DropdownMenuRowActionsRenderer ==\n'
rg -n 'DropdownMenuRowActionsRenderer|createContextMenu\(|removeListener\(|openOnHandler|contextMenuTargetConnector' src/test src/main -SRepository: FlowingCode/EasyGridAddon
Length of output: 9112
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='src/main/java/com/flowingcode/vaadin/addons/easygrid/actions/AbstractContextMenuRowActionsRenderer.java'
echo "== $file =="
sed -n '1,220p' "$file"
file='src/main/java/com/flowingcode/vaadin/addons/easygrid/actions/DropdownMenuRowActionsRenderer.java'
echo
echo "== $file =="
sed -n '1,220p' "$file"
printf '\n== any tests/usages for DropdownMenuRowActionsRenderer ==\n'
rg -n 'DropdownMenuRowActionsRenderer|createContextMenu\(|removeListener\(|openOnHandler|contextMenuTargetConnector' src/test src/main -SRepository: FlowingCode/EasyGridAddon
Length of output: 9112
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='src/main/java/com/flowingcode/vaadin/addons/easygrid/actions/AbstractContextMenuRowActionsRenderer.java'
echo "== $file =="
sed -n '1,220p' "$file"
file='src/main/java/com/flowingcode/vaadin/addons/easygrid/actions/DropdownMenuRowActionsRenderer.java'
echo
echo "== $file =="
sed -n '1,220p' "$file"
printf '\n== any tests/usages for DropdownMenuRowActionsRenderer ==\n'
rg -n 'DropdownMenuRowActionsRenderer|createContextMenu\(|removeListener\(\)|openOnHandler|contextMenuTargetConnector' src/test src/main -SRepository: FlowingCode/EasyGridAddon
Length of output: 9112
🌐 Web query:
Vaadin Grid addContextMenu removeListener $contextMenuTargetConnector openOnHandler multiple GridContextMenu same grid
💡 Result:
In Vaadin, the GridContextMenu component is designed to be associated with a Grid to provide row-specific context menus [1][2]. Below are the technical insights regarding your specific concerns: Multiple GridContextMenu instances on the same grid: It is possible to attach multiple GridContextMenu instances to a single Grid, though it is often considered a workaround [3]. A common pattern to enable both left and right-click to open a context menu involves using one dummy GridContextMenu and a second functional instance [3]. In this setup, you can programmatically close the dummy menu in a listener to handle the desired interaction flow [3]. Removing Listeners and Managing State: There is no explicit removeContextMenu method on the Grid component [4]. To effectively manage dynamic content or listeners within a GridContextMenu, you should use the setDynamicContentHandler method [5][6]. This allows you to update the menu items immediately before it opens, based on the specific row or column clicked [5][7]. If you are adding listeners to individual menu items dynamically, ensure they are registered and handled within the dynamic content update cycle to avoid issues where click listeners fail to trigger [8]. Handling $contextMenuTargetConnector and openOnHandler: These are internal client-side implementation details of the Vaadin Flow component architecture [9]. Developers generally do not interact with $contextMenuTargetConnector directly. If you encounter issues where the context menu fails to re-attach or function after the Grid is re-added to a layout, a common workaround is to manually re-initialize the connector using JavaScript via the Page object: e.getUI.getPage.executeJavaScript("window.Vaadin.Flow.contextMenuConnector.init($0)", targetElement); [9] For advanced scenarios—such as needing to identify which column was clicked—the standard approach involves adding a listener to the Grid's element to intercept the native contextmenu event, capturing the target column data, and then using that information when the menu is subsequently populated [5][6]. Top results: [5][6][9][3][8]
Citations:
- 1: https://vaadin.com/docs/latest/components/context-menu
- 2: https://vaadin.com/api/platform/current/com/vaadin/flow/component/grid/contextmenu/GridContextMenu.html
- 3: https://vaadin.com/forum/t/opening-gridcontextmenu-with-both-left-and-right-click/166785
- 4: Add support to remove GridContextMenu vaadin/vaadin-grid-flow#458
- 5: https://vaadin.com/forum/t/dynamic-popup-context-menu-for-example-in-a-grid/167841
- 6: https://vaadin.com/forum/t/use-grid-column-information-to-generate-context-menu/166711
- 7: https://vaadin.com/forum/t/dynamic-contextmenu/167912
- 8: https://vaadin.com/forum/t/14-1-17-gridcontextmenu-clicklisteners-cannot-work-when-add-menu-item-in/159887
- 9: GridContextMenu is not available after remove/add grid element vaadin/vaadin-grid-flow#459
🌐 Web query:
Vaadin GridContextMenu multiple context menus same grid $contextMenuTargetConnector removeListener single connector
💡 Result:
In Vaadin, a Grid component is designed to be associated with one GridContextMenu instance at a time using the standard grid.addContextMenu() API [1][2]. Using multiple GridContextMenu instances attached to the same grid is not a supported use case and can lead to unpredictable behavior, such as conflicting event listeners or UI issues [1]. The contextMenuTargetConnector is an internal Vaadin flow mechanism used to bind the ContextMenu component to its target element (the Grid) [3][1]. It is not intended for direct manual manipulation, and attempting to use removeListener on this connector is not a standard or stable way to swap menus [1][4]. If you need to display different context menu options, the recommended approach is to use a single GridContextMenu instance and update its content dynamically based on the target row or other criteria [5][6][7]: 1. Use setDynamicContentHandler(handler): This method allows you to execute logic immediately before the context menu is opened. You can use it to determine which items should be shown or enabled based on the selected row or column [6]. 2. Use addGridContextMenuOpenedListener(event -> ...): This listener is triggered after the menu is opened. It provides access to the clicked item and column, allowing you to re-populate the menu if necessary [6][7]. If you must switch between entirely different menu structures, you should clear the existing items using menu.removeAll() within the dynamic handler or opened listener and add the required new items programmatically [1][6][7]. Attempting to attach and remove multiple GridContextMenu instances is not supported and there is no public API to "remove" a context menu from a grid once added [4]. Top results: [5], [1], [6], [7]
Citations:
- 1: Dynamic grid context menu creation on click time vaadin/vaadin-context-menu-flow#76
- 2: https://vaadin.com/api/platform/current/com/vaadin/flow/component/grid/contextmenu/GridContextMenu.html
- 3: https://vaadin.com/docs/latest/components/context-menu
- 4: Add support to remove GridContextMenu vaadin/vaadin-grid-flow#458
- 5: https://stackoverflow.com/questions/74806891/conditional-context-menu-with-vaadin-grid
- 6: https://vaadin.com/forum/t/dynamic-popup-context-menu-for-example-in-a-grid/167841
- 7: https://vaadin.com/forum/t/14-1-17-gridcontextmenu-clicklisteners-cannot-work-when-add-menu-item-in/159887
Avoid relying on Vaadin’s private context-menu connector here. grid.$contextMenuTargetConnector.openOnHandler / removeListener() are internal Flow details, so this right-click suppression is version-sensitive and may break on Vaadin upgrades.
🤖 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/actions/DropdownMenuRowActionsRenderer.java`
around lines 54 - 75, The DropdownMenuRowActionsRenderer is calling Vaadin’s
private context-menu connector through
grid.$contextMenuTargetConnector.openOnHandler and removeListener(), which is
version-sensitive. Update the handling in createContextMenu() and the row-action
handler setup to use the public Grid/GridContextMenu APIs for opening and
managing the context menu, and remove any direct executeJs access to the
internal connector.
✅ Addressed in commit b2ba085
There was a problem hiding this comment.
For the dropdown mode I need a GridContextMenu that only opens programatically. API does not provide a mechanism for disabling the event. I cannot use a popover or context menu because I need to know which item was clicked.
There was a problem hiding this comment.
@javier-godoy, understood — that's a real Vaadin API gap. GridContextMenu doesn't expose a public way to suppress its default right-click trigger while still receiving the per-row item context, so accessing $contextMenuTargetConnector.openOnHandler / removeListener() is the only viable path here. Withdrawing the comment.
✏️ Learnings added
Learnt from: javier-godoy
Repo: FlowingCode/EasyGridAddon PR: 16
File: src/main/java/com/flowingcode/vaadin/addons/easygrid/actions/DropdownMenuRowActionsRenderer.java:54-75
Timestamp: 2026-06-24T17:00:02.996Z
Learning: In `DropdownMenuRowActionsRenderer.java` (EasyGridAddon, Vaadin Flow), the renderer intentionally uses Vaadin's internal JS connector (`grid.$contextMenuTargetConnector.openOnHandler` / `removeListener()`) to suppress the default right-click trigger on `GridContextMenu` while opening it programmatically from an overflow button. This is a known workaround for a Vaadin public-API gap: there is no supported mechanism to disable the right-click event on `GridContextMenu` while retaining per-row item context. Do not flag these private-connector calls as a bug or suggest switching to a popover/plain context menu — they are the only viable implementation.
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.
🧠 Learnings used
Learnt from: javier-godoy
Repo: FlowingCode/EasyGridAddon PR: 10
File: src/test/java/com/flowingcode/vaadin/addons/easygrid/ColumnConfigurationDemo.java:77-79
Timestamp: 2026-05-14T11:59:45.173Z
Learning: When reviewing EasyGridAddon code that registers a formatter via `EasyColumn.setFormatter(...)` (i.e., the lambda parameter `val`), assume `val` is non-null. `ColumnConfigurationTextRenderer` guards `formatter.apply(val)` with a null check and returns `config.getNullRepresentation()` for `val == null`, so formatter lambdas registered through `setFormatter` should not be flagged as “null-unsafe” for dereferencing `val` without an explicit null check. Only flag null-safety issues if the formatter is called from a path that does not follow this guard.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/com/flowingcode/vaadin/addons/easygrid/it/EasyRowActionIT.java (1)
96-99: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the debug
System.out.printlnartifact.The message
"WAS STALE... I guess I should fix this?"is a leftover debug print that also signals an unresolved doubt about the stale-element handling. Either drop the print (thereturn falsefallback is the intended behavior and is already documented in the comment above) or replace it with proper logging.Suggested change
} catch (StaleElementReferenceException e) { - System.out.println("WAS STALE... I guess I should fix this?"); return false; }If the stale path needs more than a silent fallback, I can help draft a retry/wait around the menu-open probe.
🤖 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/it/EasyRowActionIT.java` around lines 96 - 99, Remove the leftover debug print in EasyRowActionIT’s stale-element handling and keep the existing fallback behavior. In the helper that catches StaleElementReferenceException, drop the System.out.println artifact (or replace it with proper test logging if needed) and leave the return false path intact, since the surrounding logic already documents that this is the intended outcome.
♻️ Duplicate comments (1)
src/test/java/com/flowingcode/vaadin/addons/easygrid/it/AbstractViewTest.java (1)
69-84: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard the release so a failed/interrupted
acquire()cannot over-release the permit.Moving the release into
@Aftercorrectly holds the permit for the whole test lifecycle. However,tearDown()now releases unconditionally. JUnit still runs@Afterifsemaphore.acquire()is interrupted (throws) before a permit is obtained, sorelease()would add a permit that was never acquired, pushing capacity above 4 and weakening the intended browser cap. Track acquisition with a flag and only release when held.Suggested guard
`@Before` public void setup() throws Exception { semaphore.acquire(); + setupPermitAcquired = true; if (isUsingHub()) { super.setup(); } else { setDriver(TestBench.createDriver(new ChromeDriver())); } getDriver().get(getURL(route)); getCommandExecutor().waitForVaadin(); } `@After` public void tearDown() throws Exception { - semaphore.release(); + if (setupPermitAcquired) { + setupPermitAcquired = false; + semaphore.release(); + } }Add the field alongside the semaphore:
private final static Semaphore semaphore = new Semaphore(4); + private boolean setupPermitAcquired;🤖 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/it/AbstractViewTest.java` around lines 69 - 84, The semaphore handling in AbstractViewTest should only release a permit if setup() actually acquired one, since tearDown() can still run after an interrupted or failed semaphore.acquire(). Add a simple acquisition flag alongside the semaphore in AbstractViewTest, set it only after acquire succeeds, and have tearDown() conditionally call semaphore.release() based on that flag so the browser cap cannot be over-released.
🧹 Nitpick comments (1)
src/test/java/com/flowingcode/vaadin/addons/easygrid/it/EasyRowActionIT.java (1)
488-489: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
closeMenu()instead of the raw ESCAPE + wait.This duplicates the
closeMenu()helper (lines 114-117), which already sends ESCAPE and waits for!isContextMenuOpen(). Using the helper keeps the close behavior consistent with the rest of the suite (e.g., the recently updated line 246).Suggested change
grid.getCell(0, 0).contextClick(); waitUntil(d -> isContextMenuOpen()); - new org.openqa.selenium.interactions.Actions(getDriver()).sendKeys(Keys.ESCAPE).perform(); - waitUntil(d -> !isContextMenuOpen()); + closeMenu();🤖 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/it/EasyRowActionIT.java` around lines 488 - 489, Replace the duplicated ESCAPE key sequence in EasyRowActionIT with the existing closeMenu() helper so the test uses one consistent menu-closing path. Locate the direct Actions(getDriver()).sendKeys(Keys.ESCAPE).perform() and waitUntil(d -> !isContextMenuOpen()) calls and replace them by invoking closeMenu(), matching the helper already defined in the class and used elsewhere in the suite.
🤖 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.
Outside diff comments:
In
`@src/test/java/com/flowingcode/vaadin/addons/easygrid/it/EasyRowActionIT.java`:
- Around line 96-99: Remove the leftover debug print in EasyRowActionIT’s
stale-element handling and keep the existing fallback behavior. In the helper
that catches StaleElementReferenceException, drop the System.out.println
artifact (or replace it with proper test logging if needed) and leave the return
false path intact, since the surrounding logic already documents that this is
the intended outcome.
---
Duplicate comments:
In
`@src/test/java/com/flowingcode/vaadin/addons/easygrid/it/AbstractViewTest.java`:
- Around line 69-84: The semaphore handling in AbstractViewTest should only
release a permit if setup() actually acquired one, since tearDown() can still
run after an interrupted or failed semaphore.acquire(). Add a simple acquisition
flag alongside the semaphore in AbstractViewTest, set it only after acquire
succeeds, and have tearDown() conditionally call semaphore.release() based on
that flag so the browser cap cannot be over-released.
---
Nitpick comments:
In
`@src/test/java/com/flowingcode/vaadin/addons/easygrid/it/EasyRowActionIT.java`:
- Around line 488-489: Replace the duplicated ESCAPE key sequence in
EasyRowActionIT with the existing closeMenu() helper so the test uses one
consistent menu-closing path. Locate the direct
Actions(getDriver()).sendKeys(Keys.ESCAPE).perform() and waitUntil(d ->
!isContextMenuOpen()) calls and replace them by invoking closeMenu(), matching
the helper already defined in the class and used elsewhere in the suite.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0b8abbc2-e3d5-41e1-83b8-4aff8cac683b
📒 Files selected for processing (5)
src/main/java/com/flowingcode/vaadin/addons/easygrid/actions/EasyRowAction.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/RowActionsDynamicDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/actions/EasyRowActionTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/AbstractViewTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/EasyRowActionIT.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/test/java/com/flowingcode/vaadin/addons/easygrid/RowActionsDynamicDemo.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/actions/EasyRowActionTest.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/actions/EasyRowAction.java
javier-godoy
force-pushed
the
impl/row-actions
branch
from
18b461c to
b2ba085
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
src/test/java/com/flowingcode/vaadin/addons/easygrid/it/AbstractViewTest.java (1)
67-83: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winThe permit is still released before the browser is actually cleaned up.
Because this release happens in the subclass
@After, JUnit runs it before inherited teardown logic closes the driver, so the 4-browser cap is lifted too early again. It also releases unconditionally even if Line 71 never finished acquiring a permit.Suggested fix
- private final static Semaphore semaphore = new Semaphore(4); + private static final Semaphore semaphore = new Semaphore(4); + private boolean permitAcquired; `@Before` public void setup() throws Exception { semaphore.acquire(); + permitAcquired = true; if (isUsingHub()) { super.setup(); } else { setDriver(TestBench.createDriver(new ChromeDriver())); } getDriver().get(getURL(route)); getCommandExecutor().waitForVaadin(); } `@After` public void tearDown() throws Exception { - semaphore.release(); + try { + super.tearDown(); + } finally { + if (permitAcquired) { + permitAcquired = false; + semaphore.release(); + } + } }🤖 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/it/AbstractViewTest.java` around lines 67 - 83, The semaphore permit is being released too early in AbstractViewTest. Move the release so it happens only after the browser cleanup from the inherited teardown has completed, and make sure the same setup path that acquires the permit in setup() is the only path that can release it in tearDown(). Use the AbstractViewTest setup/tearDown methods and the semaphore field to preserve the 4-browser cap until the driver is fully closed.
🤖 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 `@FEATURE_ROW_ACTIONS.md`:
- Around line 120-130: Update the Row Actions documentation to make it clear
that tooltip and styling APIs are not yet renderer-agnostic. In the sections for
EasyRowAction.tooltip(...), addClassName(...), getStyle(), and
addThemeVariants(...), scope the behavior to INLINE_BUTTONS or add an explicit
caveat that dropdown/context-menu renderers do not currently forward these
settings. Use the EasyRowAction and RowActionsStyle references to keep the
wording aligned across the affected sections.
- Around line 136-140: The withConfirmation API docs in EasyRowAction currently
read like fully generic confirmation support, but the implementation still
hardcodes the dialog buttons as “Ok” and “Cancel”. Update the documentation near
withConfirmation(String message) and withConfirmation(String title, String
message) to explicitly say the confirmation button labels are fixed English for
now and that localization/customization is deferred, so the public API
description matches the current behavior.
In
`@src/main/java/com/flowingcode/vaadin/addons/easygrid/actions/DropdownMenuRowActionsRenderer.java`:
- Around line 63-65: The action column in DropdownMenuRowActionsRenderer is
still allowed to expand because only setAutoWidth(true) is applied. Update the
column configuration right after grid.addColumn(builder.build()) to also call
setFlexGrow(0) on the same column so the ⋮ trigger stays compact and does not
stretch with leftover grid space.
In
`@src/main/java/com/flowingcode/vaadin/addons/easygrid/actions/LitRendererBuilder.java`:
- Around line 475-479: The withFunction(SerializableBiConsumer<T, JsonArray>)
method in LitRendererBuilder currently allows handlers to be registered even
when staticOnly() was used, which defers the unsupported-state failure until
later. Add an immediate guard in withFunction() to reject static-only builders
before adding the handler, using the same state tracked by
staticOnly()/requireNotClosed(). Keep the exception at the withFunction() call
site so misuse is caught early and the builder behavior stays consistent with
its documented contract.
In
`@src/main/java/com/flowingcode/vaadin/addons/easygrid/actions/LitRowActionsRenderer.java`:
- Line 25: The renderer property key generation in LitRowActionsRenderer is
still collision-prone because ThreadLocalRandom can eventually repeat a previous
value. Replace the random-based key generation with a monotonic counter so each
property name is guaranteed unique across refreshes, and update the affected
property-name creation logic in the renderer setup to use that counter
consistently instead of ThreadLocalRandom.
In
`@src/test/java/com/flowingcode/vaadin/addons/easygrid/it/EasyRowActionIT.java`:
- Around line 191-207: The confirmation-dialog tests are reusing the same flow
immediately after a cancel without waiting for the dialog to fully close. Update
the relevant assertions in testConfirmation() and testDropdownConfirmation() to
wait for the ConfirmDialogElement to disappear after clicking the cancel button
before reopening it, using the existing dialog lookup/wait pattern in
EasyRowActionIT; leave the final confirm-click paths unchanged since they do not
need the extra close wait.
- Around line 91-94: The isContextMenuOpen helper can still throw during overlay
teardown when a ContextMenuElement becomes stale. Update isContextMenuOpen() in
EasyRowActionIT to catch StaleElementReferenceException around the
stream/element checks and treat that case as closed by returning false, so
closeMenu() can safely continue during detach timing windows.
In `@src/test/java/com/flowingcode/vaadin/addons/easygrid/RowActionsDemo.java`:
- Around line 72-76: The last action in RowActionsDemo is currently icon-only,
which showcases an inaccessible pattern. Update the example to use the row
action API in a way that provides both a text label and an icon, ideally via the
addRowAction call that accepts label and icon providers for the dynamic
subscriber action. Keep the existing star icon behavior, but ensure the rendered
control always has a usable label alongside the icon.
---
Duplicate comments:
In
`@src/test/java/com/flowingcode/vaadin/addons/easygrid/it/AbstractViewTest.java`:
- Around line 67-83: The semaphore permit is being released too early in
AbstractViewTest. Move the release so it happens only after the browser cleanup
from the inherited teardown has completed, and make sure the same setup path
that acquires the permit in setup() is the only path that can release it in
tearDown(). Use the AbstractViewTest setup/tearDown methods and the semaphore
field to preserve the 4-browser cap until the driver is fully closed.
🪄 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: d5ad8503-3b62-4bb0-8186-dcd38dbe3640
📒 Files selected for processing (31)
FEATURE_ROW_ACTIONS.mdpom.xmlsrc/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGridWrapper.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/AbstractContextMenuRowActionsRenderer.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/Constant.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/ContextMenuRowActionsRenderer.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/DropdownMenuRowActionsRenderer.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/EasyRowAction.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/HasRowActions.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/LitRendererBuilder.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/LitRowActionsRenderer.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/RowActionsManager.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/RowActionsRenderer.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/RowActionsStyle.javasrc/main/resources/META-INF/frontend/fc-dynamic-buttons.csssrc/main/resources/META-INF/frontend/fc-icon.tssrc/main/resources/META-INF/frontend/styles/static_addon_stylessrc/test/java/com/flowingcode/vaadin/addons/easygrid/EasyGridDemoView.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/RowActionsDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/RowActionsDynamicDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/RowActionsMenuDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/actions/EasyRowActionTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/actions/LitRendererBuilderTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/AbstractViewTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/EasyRowActionIT.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/EasyRowActionITCallables.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/EasyRowActionITView.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/RmiEasyRowAction.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/ViewInitializerImpl.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyRowActionUnitTest.javasrc/test/resources/META-INF/services/com.vaadin.flow.server.VaadinServiceInitListener
💤 Files with no reviewable changes (1)
- src/main/resources/META-INF/frontend/styles/static_addon_styles
javier-godoy
force-pushed
the
impl/row-actions
branch
from
c3585e6 to
257a3bf
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/com/flowingcode/vaadin/addons/easygrid/actions/LitRendererBuilder.java (1)
359-371: 📐 Maintainability & Code Quality | 🔵 TrivialDelete the unused
BindingType.read(...)methods
copyAttributes(...)already inlines the prefix switch, and nothing else calls the enum’sread(...)overloads. Removing them keeps the dispatch logic in one place.🤖 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/actions/LitRendererBuilder.java` around lines 359 - 371, Delete the unused BindingType.read(...) overloads since copyAttributes(...) in LitRendererBuilder now performs the prefix-based read logic inline and no other code references those methods. Remove the redundant enum methods and keep the attribute/property dispatch centralized in copyAttributes(...) using BindingType.of(name), preserving the existing behavior for ATTRIBUTE versus property bindings.
🤖 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.
Nitpick comments:
In
`@src/main/java/com/flowingcode/vaadin/addons/easygrid/actions/LitRendererBuilder.java`:
- Around line 359-371: Delete the unused BindingType.read(...) overloads since
copyAttributes(...) in LitRendererBuilder now performs the prefix-based read
logic inline and no other code references those methods. Remove the redundant
enum methods and keep the attribute/property dispatch centralized in
copyAttributes(...) using BindingType.of(name), preserving the existing behavior
for ATTRIBUTE versus property bindings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 06f82414-a370-4a48-a0f3-c57682650d45
📒 Files selected for processing (31)
FEATURE_ROW_ACTIONS.mdpom.xmlsrc/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGridWrapper.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/AbstractContextMenuRowActionsRenderer.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/Constant.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/ContextMenuRowActionsRenderer.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/DropdownMenuRowActionsRenderer.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/EasyRowAction.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/HasRowActions.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/LitRendererBuilder.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/LitRowActionsRenderer.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/RowActionsManager.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/RowActionsRenderer.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/RowActionsStyle.javasrc/main/resources/META-INF/frontend/fc-dynamic-buttons.csssrc/main/resources/META-INF/frontend/fc-icon.tssrc/main/resources/META-INF/frontend/styles/static_addon_stylessrc/test/java/com/flowingcode/vaadin/addons/easygrid/EasyGridDemoView.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/RowActionsDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/RowActionsDynamicDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/RowActionsMenuDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/actions/EasyRowActionTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/actions/LitRendererBuilderTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/AbstractViewTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/EasyRowActionIT.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/EasyRowActionITCallables.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/EasyRowActionITView.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/RmiEasyRowAction.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/ViewInitializerImpl.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyRowActionUnitTest.javasrc/test/resources/META-INF/services/com.vaadin.flow.server.VaadinServiceInitListener
💤 Files with no reviewable changes (1)
- src/main/resources/META-INF/frontend/styles/static_addon_styles
javier-godoy
force-pushed
the
impl/row-actions
branch
from
257a3bf to
d0de22c
Compare
javier-godoy
force-pushed
the
impl/row-actions
branch
from
d0de22c to
b9fd620
Compare
✅ Action performedFull review finished. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/actions/LitRowActionsRenderer.java`:
- Around line 66-68: The inline actions column in LitRowActionsRenderer is still
allowed to expand because only setAutoWidth(true) is applied. Update the column
setup in the branch that creates the column with grid.addColumn(litRenderer) so
it also sets flex growth to zero, matching the behavior used in
DropdownMenuRowActionsRenderer for its trigger column. Keep the existing
auto-width sizing, but explicitly disable flex growth on that column so it stays
compact.
In
`@src/test/java/com/flowingcode/vaadin/addons/easygrid/it/AbstractViewTest.java`:
- Around line 67-83: Guard the Semaphore lifecycle in AbstractViewTest so
tearDown() only releases a permit if setup() successfully acquired one. Add a
per-test boolean or similar state around semaphore.acquire() in setup(), set it
only after acquisition succeeds, and check that flag in tearDown() before
calling semaphore.release(). Keep the fix local to AbstractViewTest and its
setup()/tearDown() methods.
🪄 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: d9acd02b-187d-41bf-a21c-f84de53c2567
📒 Files selected for processing (31)
FEATURE_ROW_ACTIONS.mdpom.xmlsrc/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGridWrapper.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/AbstractContextMenuRowActionsRenderer.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/Constant.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/ContextMenuRowActionsRenderer.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/DropdownMenuRowActionsRenderer.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/EasyRowAction.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/HasRowActions.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/LitRendererBuilder.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/LitRowActionsRenderer.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/RowActionsManager.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/RowActionsRenderer.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/actions/RowActionsStyle.javasrc/main/resources/META-INF/frontend/fc-dynamic-buttons.csssrc/main/resources/META-INF/frontend/fc-icon.tssrc/main/resources/META-INF/frontend/styles/static_addon_stylessrc/test/java/com/flowingcode/vaadin/addons/easygrid/EasyGridDemoView.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/RowActionsDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/RowActionsDynamicDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/RowActionsMenuDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/actions/EasyRowActionTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/actions/LitRendererBuilderTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/AbstractViewTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/EasyRowActionIT.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/EasyRowActionITCallables.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/EasyRowActionITView.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/RmiEasyRowAction.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/ViewInitializerImpl.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyRowActionUnitTest.javasrc/test/resources/META-INF/services/com.vaadin.flow.server.VaadinServiceInitListener
💤 Files with no reviewable changes (1)
- src/main/resources/META-INF/frontend/styles/static_addon_styles
javier-godoy
force-pushed
the
impl/row-actions
branch
from
b9fd620 to
6c4ed3c
Compare
javier-godoy
force-pushed
the
impl/row-actions
branch
from
6c4ed3c to
750de74
Compare
ReviewSolid implementation — the public API matches Spec conformance
Code
Minor
Nothing here is a hard blocker; the spec/behavior mismatch on tooltips/styling for menu styles is the one I'd address before merge. |
paodb
left a comment
paodb
left a comment
There was a problem hiding this comment.
See #16 (comment) and .gitignore needs update to add /src/main/frontend/generated and /src/main/frontend/index.html
github-project-automation
Bot
moved this from To Do
to In Progress
in Flowing Code Addons
|
.gitignore needs update to add /src/main/frontend/generated and /src/main/frontend/index.html
A clarification first: this is already documented in the PR under "Pending issues." The entry calls out exactly this gap — "Menu renderers (dropdown/context menu) don't support tooltips and styling" — and lists "forward tooltip to each MenuItem," "copy CSS classes," "copy inline styles" as the planned fixes, plus theme variants as explicitly "Unresolved: ... pending decision on whether to treat them as inline-only with a spec caveat, or design a menu-item mapping strategy." So the mismatch isn't undiscovered — it's known, tracked, and deliberately referred to follow-up. The request to scope the spec down reads as if that referral isn't there. On the substance: FEATURE_ROW_ACTIONS.md is normative, not descriptive. It defines the intended contract for EasyRowAction, not a snapshot of what the renderers currently do. tooltip(...) and the HasStyle setters are declared on EasyRowAction itself (it implements HasStyle/HasThemeVariant), so they belong to the action's contract regardless of how a given style renders it today. When the spec and the implementation disagree, the spec is the source of truth and the implementation is what's behind — not the other way around. The fix is to make the menu renderers honor the contract, not to rewrite the contract to match an incomplete renderer. Scoping these sections to INLINE_BUTTONS would bake a temporary implementation gap into the spec as if it were the intended design. Concretely, that splits into two cases, both already captured in Pending issues:
So my position is to keep the spec unchanged in this PR and resolve the gap through the tracked follow-up. Question back to you: do you consider closing these gaps a blocker — i.e. you want the menu renderers fixed (and the theme-variant decision made) before this PR merges — or do you agree with treating them as the referred Pending issues and merging on that basis? If it's the former, let's scope what specifically must land here; if the latter, the spec stays normative and the work proceeds as tracked.
The internal-API dependency is real and worth documenting, so I've added comments on the trigger handler and createContextMenu() (commit 2ec9a91) explaining why we reach into $contextMenuTargetConnector, why the removeListener() call is deferred, and the Vaadin versions it's validated against (24.10.x / 25.1.x). On the two specific risks, though:
I'd push back on the "guard if the connector is absent" suggestion specifically: a guard that no-ops when the connector is missing would convert a loud IT failure into silent wrong behavior (the button quietly stops opening the menu, or right-click suppression silently stops applying). That's the silent-breakage outcome we want to avoid, just built in deliberately. The ITs are the right tripwire here.
I'd like to push back on this one. I think the framing makes the control flow sound more hazardous than it is, and the suggested refactor trades away a property without fixing a defect. Proposing we resolve as working-as-intended. It isn't open-ended recursion — the depth is structurally bounded at 2. remove() nulls this.manager before it delegates: So the return trip into remove() (via removeRowAction → action.remove()) always hits manager == null and is a guaranteed no-op. There is no input that makes this recurse deeper, and therefore no stack-growth or non-termination failure mode to defend against. "Mutually recursive whose termination depends on guards" invites the reader to picture a hazard the code doesn't actually admit — it's a fixed two-step handshake, not a loop. Termination is locally provable, contrary to the finding. I don't need to read removeRowAction to convince myself remove() is safe. Capture-local-then-null is the standard re-entrancy idiom — the same shape as flipping a guard flag before firing listeners. Reading remove() in isolation tells you it can fire its outbound call at most once per live manager. So the claim that termination "depends entirely on the actions.remove(...) / manager != null guards spread across two classes" isn't accurate: the real call path relies only on guard A (manager != null, nulled first), and that guard is self-contained in remove(). Worth noting removeRowAction is package-private with exactly one caller — EasyRowAction.remove() — so actions.remove(...) returning false never actually fires on the live path; it's a backstop, not a load-bearing termination guard. The proposed "single owner" relocates the coupling rather than removing it. A remove() + private detach() split still has the action reach across and mutate the manager's actions list — same two-object dependency, just asymmetric instead of symmetric. What it gives up is a real invariant: today either side can initiate removal and the object graph ends consistent (actions membership ⟺ the action's manager back-reference). A one-directional detach() only stays correct while the action handle remains the sole entry point. So the refactor buys a cosmetic "no recursion" at the cost of making the association fragile to a second caller — which is the opposite of the robustness the change is reaching for. What the finding gets right, and why it still doesn't compel a refactor. Fair point that, with today's single caller, the action.remove() callback at the end of removeRowAction never does work — by the time it runs, the field is already null. But that's two lines preserving the invariant that removeRowAction leaves an action fully detached (out of the list and back-reference cleared) regardless of who calls it. That's cheap, idempotent self-consistency, not a smell. If we want to apply YAGNI here, the honest right-sized response is to delete that one callback line and document that the action handle is the only removal path — not restructure into an owner/detach arrangement, which is churn that reshapes correct code without resolving any actual defect. Net: no bug, bounded depth, locally verifiable termination, and a consistent-from-either-side invariant. I'd resolve as working-as-intended, and at most drop the inert callback if we want to formally assert a single removal path.
✅ Done
✅ Done
✅ Done (hoisted)
✅ Done
✅ Done |
|
Thanks for the detailed responses. Spec conformance: I agree with treating them as the referred Pending issues and merging on that basis. The spec stays normative. Dropdown internals & mutual recursion: Fair points on both, you're right, thanks for pointing it out. Resolving as working-as-intended; the doc comments on the dropdown were the part that mattered. The rest are already addressed. 👍 |
javier-godoy
force-pushed
the
impl/row-actions
branch
from
12e5b2e to
3e57090
Compare
2 participants
Pending issues
Dynamic and localizable confirmation dialogs (Deferred — future feature)
Menu renderers (dropdown/context menu) don't support tooltips and styling
Summary by CodeRabbit
Summary
refreshRowActionshook.