fix: convert JsonValue in varargs methods and fix: invoke executeJs using reflection

[javier-godoy]

Summary by CodeRabbit

• Refactor
  • Improved runtime compatibility across framework versions by adapting how internal JavaScript calls are dispatched, ensuring consistent behavior across upgrades.
  • Enhanced handling of variable-argument parameters so methods receive correctly converted argument arrays, reducing invocation errors and improving stability.

[coderabbitai]

Walkthrough

Adds version-aware reflective dispatch for Element.executeJs and updates JsonMigrationHelper25 to convert trailing varargs Object[] arguments to the method's expected component type before reflective invocation. No public API signatures were changed.

Changes

Cohort / File(s)	Summary
Element.executeJs reflection dispatch src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigration.java	Introduces a private reflective Method lookup (lookup_executeJs) that selects the correct Element.executeJs signature based on Vaadin major version (uses Object[] for >24, Serializable[] otherwise). Replaces direct calls with reflective invoke(...) and casts result to PendingJavaScriptResult.
Varargs array conversion support src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java	Enhances invoke to handle varargs methods: if the target is varArgs and the last arg is an Object[], converts its elements to the method's component type via a new generic convertArray(Object[], Class<? extends T>) (uses java.lang.reflect.Array) and substitutes the converted array before invocation. Also gates existing argument-conversion loop behind a length check.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• Review reflective method selection for correct signature across Vaadin versions and accessibility handling.
• Verify cast and nullability handling when invoking and returning PendingJavaScriptResult.
• Test varargs conversion edge cases (already-matching types, conversion returning null, and interaction with convertToJsonNode).

Possibly related PRs

• feat: add executeJs helper #8 — Modifies executeJs invocation paths and conversion between PendingJavaScriptResult/ElementalPendingJavaScriptResult, closely related to reflective dispatch changes here.

Suggested reviewers

• mlopezFC
• paodb

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the two main changes: converting JsonValue in varargs methods and invoking executeJs using reflection.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix-executeJs

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fb2ae68 and 796ad3c.

📒 Files selected for processing (1)

• src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java (2 hunks)

🔇 Additional comments (3)

src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java (3)

30-30: LGTM!

The import is necessary for the Array.newInstance call in the new convertArray method.

---

62-84: Past review issues resolved; varargs handling looks solid.

The length-equality guard at line 65 properly addresses both concerns from the previous review:

1. Empty args: For varargs methods, parameterTypes.length ≥ 1, so empty args (args.length == 0) fail the check. For non-varargs methods with 0 params, method.isVarArgs() is false.
2. Expanded varargs: When args.length > parameterTypes.length, the check fails, preventing ArrayIndexOutOfBoundsException on parameterTypes[j].

The logic correctly handles packed varargs (last argument is Object[]) and skips conversion when convertArray returns null. Individual argument conversion at lines 75-83 efficiently handles JsonValue → BaseJsonNode on a per-argument basis.

---

93-117: Off-by-one error fixed; conversion logic is correct.

The System.arraycopy at line 104 now uses i as the length parameter, correctly copying elements [0..i-1]. This addresses the past review's concern about the off-by-one error.

The conversion logic properly handles:

• Lazy array creation: only allocates when the first JsonValue is encountered
• Prefix preservation: copies all non-JsonValue elements before the first JsonValue
• Type safety: uses newType.cast() for explicit casting at line 112, which throws ClassCastException if elements are incompatible (appropriate for a migration layer)

The assumption is that non-JsonValue elements are already compatible with newType. If not, runtime exceptions will surface, which is the correct behavior for this reflective conversion helper.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d6ea20f and da0d305.

📒 Files selected for processing (2)

• src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigration.java (2 hunks)
• src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java (3 hunks)

🔇 Additional comments (1)

src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigration.java (1)

119-128: Reflection-based executeJs dispatch looks consistent; just double-check Vaadin signatures

The lookup for Element.executeJs and the wrapper that delegates via helper.invoke are coherent with the stated goal of handling (String, Object...) for Vaadin > 24 and (String, Serializable...) otherwise. No obvious issues in the reflection wiring itself.

Please just verify against the exact Flow 24/25 Element.executeJs signatures you support (parameter and return types) to ensure the reflective lookup remains stable across versions.

Also applies to: 142-143

[javier-godoy]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java (1)

63-72: Consider adding defensive bounds check for parameterTypes array access.

While all current call sites are safe, line 67 accesses parameterTypes[j] without verifying j < parameterTypes.length. Adding this check would make the code more robust against potential future misuse:

-    if (method.isVarArgs() && j >= 0 && args[j] instanceof Object[]) {
+    if (method.isVarArgs() && j >= 0 && j < parameterTypes.length && args[j] instanceof Object[]) {

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d6ea20f and fb2ae68.

📒 Files selected for processing (2)

• src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigration.java (2 hunks)
• src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java (3 hunks)

🔇 Additional comments (3)

src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigration.java (2)

119-128: LGTM! Version-aware method lookup follows established patterns.

The reflective dispatch logic correctly selects the appropriate Element.executeJs signature based on Vaadin version, mirroring the pattern used for other version-sensitive methods in this class.

---

142-143: The original review concern is based on incorrect API assumptions.

Both Vaadin 24 and Vaadin 25 use the same method signature: public PendingJavaScriptResult executeJs(String expression, Serializable... parameters). The original review expected different signatures (Object[] vs Serializable[]), but both versions actually use varargs with identical parameter types. The reflective invocation at lines 142-143 correctly passes the parameters array to this signature.

Likely an incorrect or invalid review comment.

src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java (1)

90-114: LGTM! Array conversion logic is correct.

The convertArray method properly handles conversion of JsonValue elements to BaseJsonNode within varargs arrays. The prefix copy on line 101 correctly uses i as the length, preserving all elements before the first converted one. The lazy initialization and mixed-type handling are both appropriate.