Skip to content

Conversation

@javier-godoy
Copy link
Member

@javier-godoy javier-godoy commented Nov 14, 2025

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.

@javier-godoy javier-godoy requested a review from paodb November 14, 2025 13:23
@coderabbitai
Copy link

coderabbitai bot commented Nov 14, 2025

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

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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@paodb paodb merged commit d6ea20f into master Nov 14, 2025
3 checks passed
@paodb paodb deleted the pending-js branch November 14, 2025 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants