feat: implement PendingJavaScriptResult methods

[javier-godoy]

Summary by CodeRabbit

• New Features
  • Enhanced type-safe handling for asynchronous JavaScript results with automatic JSON→Java decoding for primitives, wrappers and JSON values.
  • New APIs to consume typed callbacks and to obtain typed futures from JavaScript results, including null/error handling for safer async flows.

[coderabbitai]

Walkthrough

Adds typed result handling for JavaScript execution: new generic overloads for then(...) and toCompletableFuture(...) on ElementalPendingJavaScriptResult, a JsonCodec.decodeAs utility, and corresponding decoding support in JsonMigrationHelper25's PendingJavaScriptResultImpl. All changes focus on decoding JsonValue to requested target types.

Changes

Cohort / File(s)	Summary
Interface: Elemental pending result src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalPendingJavaScriptResult.java	Added generic overloads: default <T> void then(Class<T>, SerializableConsumer<T>, SerializableConsumer<String>), default <T> void then(Class<T>, SerializableConsumer<T>), <T> CompletableFuture<T> toCompletableFuture(Class<T>), and default CompletableFuture<JsonValue> toCompletableFuture(). Added imports for JsonCodec and CompletableFuture.
JSON codec utility src/main/java/com/flowingcode/vaadin/jsonmigration/JsonCodec.java	Added JsonCodec with public static <T> T decodeAs(JsonValue json, Class<T> type) to convert JsonValue into supported Java types (String, Boolean, Integer, Double, primitives, JsonValue) with null/primitive handling and IllegalArgumentException for unsupported types.
Implementation: PendingJavaScriptResultImpl src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java	Added typed then(Class<T>, SerializableConsumer<T>, SerializableConsumer<String>) and <T> CompletableFuture<T> toCompletableFuture(Class<T>) in PendingJavaScriptResultImpl. Introduced a private decodeAs(JsonNode, Class<T>) helper and conditional decoding paths to convert JsonNode/JsonValue to target types before invoking handlers or completing futures. Added CompletableFuture import.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Review JsonCodec.decodeAs for correctness across edge cases (null, numeric coercion, primitive wrappers).
• Verify null-checks and error forwarding in the new ElementalPendingJavaScriptResult.then(...) overloads.
• Confirm consistency between interface defaults and JsonMigrationHelper25 implementation, especially JsonValue-to-type decoding and CompletableFuture chaining.

Possibly related PRs

• feat: add executeJs helper #8 — Introduced ElementalPendingJavaScriptResult and untyped pending-result implementations that these typed overloads and decoding paths extend.

Suggested reviewers

• paodb
• mlopezFC

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 54.55% 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 directly describes the main change: implementing typed methods for PendingJavaScriptResult to handle results with specific target types.

✨ 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 pending-js

---

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: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between eb84a1e and bd5c2e9.

📒 Files selected for processing (3)

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

🧰 Additional context used

🧬 Code graph analysis (3)

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

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

• SuppressWarnings (149-194)

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

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

• JsonCodec (61-104)

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

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

• JsonCodec (61-104)

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (1)

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

171-182: Restore null‑argument validation in typed then(...) override.

The override of then(Class<T> targetType, SerializableConsumer<T> resultHandler, ...) still omits the non‑null precondition checks that the default method provides. As a result:

• resultHandler == null with a JsonValue target will now cause an asynchronous NullPointerException inside the lambda.
• targetType == null is only validated (if at all) by the delegate, and the behavior depends on its implementation.

To keep the contract consistent and fail fast, please validate arguments before branching:

   @Override
   @SuppressWarnings("unchecked")
   public <T> void then(Class<T> targetType, SerializableConsumer<T> resultHandler,
       SerializableConsumer<String> errorHandler) {
+    if (targetType == null) {
+      throw new IllegalArgumentException("Target type cannot be null");
+    }
+    if (resultHandler == null) {
+      throw new IllegalArgumentException("Result handler cannot be null");
+    }
-    if (targetType != null && JsonValue.class.isAssignableFrom(targetType)) {
+    if (JsonValue.class.isAssignableFrom(targetType)) {
       delegate.then(JsonNode.class, wrap(value -> {
         resultHandler.accept(JsonCodec.decodeAs(value, targetType));
       }), errorHandler);
     } else {
       delegate.then(targetType, resultHandler, errorHandler);
     }
   }

This preserves the non‑null contract and avoids deferred NPEs.

🧹 Nitpick comments (2)

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

43-61: Clarify class‑level Javadoc vs current implementation surface.

The class Javadoc advertises a more general “encode to and from JSON” utility (including Element/Component), but the class currently only exposes decodeAs for a smaller set of types. Consider narrowing the Javadoc to match what this helper actually implements, or adding the missing encode/use cases if you intend to support the full contract.

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

160-162: Avoid reusing the same generic type name for helper and outer methods.

decodeAs(JsonNode node, Class<T> type) uses T as its type parameter, which is also the type parameter name of the enclosing then/toCompletableFuture methods. This is legal but slightly confusing when reading call sites like decodeAs(node, targetType). Renaming the helper’s parameter to <U> (or similar) would make the type flow clearer.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bd5c2e9 and 8861333.

📒 Files selected for processing (3)

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

🚧 Files skipped from review as they are similar to previous changes (1)

• src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalPendingJavaScriptResult.java

🧰 Additional context used

🧬 Code graph analysis (2)

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

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

• SuppressWarnings (149-194)

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

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

• JsonCodec (61-104)

🔇 Additional comments (1)

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

184-192: Add null check for targetType to match then() method pattern in toCompletableFuture(Class<T>).

The issue is confirmed: Class.isAssignableFrom(null) throws NullPointerException, and line 186 calls this without a null guard. However, the inconsistency is even more evident in the codebase itself—the then(Class<T> targetType, ...) method at line 175 already protects this exact check with if (targetType != null && JsonValue.class.isAssignableFrom(targetType)), then delegates null through to the underlying implementation (line 180).

The toCompletableFuture() method should follow the same pattern for consistency:

  @Override
  public <T> CompletableFuture<T> toCompletableFuture(Class<T> targetType) {
-   if (JsonValue.class.isAssignableFrom(targetType)) {
+   if (targetType != null && JsonValue.class.isAssignableFrom(targetType)) {
      return delegate.toCompletableFuture(JsonNode.class)
          .thenApply(node -> decodeAs(node, targetType));
    } else {
      return delegate.toCompletableFuture(targetType);
    }
  }

This prevents NullPointerException and aligns with the existing contract already demonstrated in the then() overload.

Likely an incorrect or invalid review comment.