Skip to content

Conversation

@javier-godoy
Copy link
Member

@javier-godoy javier-godoy commented Dec 3, 2025

Summary by CodeRabbit

  • Documentation

    • Added information about the required ASM dependency for Vaadin 14-23, including Maven configuration instructions.
  • Improvements

    • Added runtime validation to detect and report when the ASM dependency is missing, with clear error messaging to guide users toward resolution.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 3, 2025

Walkthrough

Added ASM library optional dependency detection and runtime validation. Introduced an IS_ASM_PRESENT static flag in ClassInstrumentationUtil to detect ASM availability at class load time, with a runtime check that throws IllegalStateException if absent. Updated README with ASM dependency documentation for Vaadin 14-23 support.

Changes

Cohort / File(s) Summary
Documentation
README.md
Added informational text documenting the required ASM dependency (org.ow2.asm:asm:9.8) for Vaadin 14-23 in two locations: after the ViewInitializerImpl code block and within the Direct Usage section.
Runtime Dependency Detection
src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java
Introduced static IS_ASM_PRESENT flag initialized via a static block to detect ASM API availability at class load time. Added runtime dependency check in instrumentClass that throws IllegalStateException with descriptive message if ASM is not present.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Verify the static initializer correctly detects the ASM ClassWriter presence
  • Ensure the runtime exception is thrown at the appropriate point before instrumentation is attempted
  • Confirm the exception message content and clarity for users

Possibly related PRs

  • PR #16: Adds ASM presence flag and runtime guard with exception handling to ClassInstrumentationUtil, directly complementing the instrumentation code
  • PR #19: Modifies ClassInstrumentationUtil.java instrumentation logic and bytecode generation, sharing common code area with ASM dependency detection changes

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 50.00% 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 describes the main change: adding a runtime check for the missing ASM dependency with an appropriate error message and documentation.
✨ 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 add-asm-check

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

🧹 Nitpick comments (1)
README.md (1)

155-162: Specify the language for the fenced code block.

The code block should specify xml as the language for proper syntax highlighting and to satisfy linting rules.

 This feature requires a dependency with ASM (which is not provided out-of-the-box in Vaadin 14-23):
-```
+```xml
 <dependency>
     <groupId>org.ow2.asm</groupId>
     <artifactId>asm</artifactId>
     <version>9.8</version>
 </dependency>

</blockquote></details>

</blockquote></details>

<details>
<summary>📜 Review details</summary>

**Configuration used**: CodeRabbit UI

**Review profile**: CHILL

**Plan**: Pro

<details>
<summary>📥 Commits</summary>

Reviewing files that changed from the base of the PR and between 5086a28a848338c0fd13aa894ebbebfeceb1b381 and fb0c568162fd6e03aa4855c0371e574d596f0cdf.

</details>

<details>
<summary>📒 Files selected for processing (2)</summary>

* `README.md` (1 hunks)
* `src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java` (2 hunks)

</details>

<details>
<summary>🧰 Additional context used</summary>

<details>
<summary>🧠 Learnings (1)</summary>

<details>
<summary>📚 Learning: 2025-11-25T16:35:42.544Z</summary>

Learnt from: javier-godoy
Repo: FlowingCode/JsonMigrationHelper PR: 11
File: src/main/java/com/flowingcode/vaadin/jsonmigration/JsonSerializer.java:281-301
Timestamp: 2025-11-25T16:35:42.544Z
Learning: In the JsonMigrationHelper project, code copied from the Vaadin codebase should be kept consistent with the original source unless there's a specific reason to deviate.


**Applied to files:**
- `README.md`

</details>

</details><details>
<summary>🪛 markdownlint-cli2 (0.18.1)</summary>

<details>
<summary>README.md</summary>

156-156: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

</details>

</details>

</details>

<details>
<summary>🔇 Additional comments (2)</summary><blockquote>

<details>
<summary>src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java (2)</summary><blockquote>

`68-80`: **LGTM! Clean ASM presence detection.**

The static initializer correctly uses `Class.forName` with `initialize=false` to detect ASM availability without triggering class initialization. This approach allows the class to load even when ASM is absent.

---

`140-142`: **Good placement of the ASM check.**

The check is correctly positioned after `needsInstrumentation` - this way, classes that don't require instrumentation will work fine even without ASM. The error message with exact Maven coordinates is helpful for developers.

</blockquote></details>

</blockquote></details>

</details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

@paodb paodb merged commit 2202d52 into master Dec 3, 2025
3 checks passed
@paodb paodb deleted the add-asm-check branch December 3, 2025 15:02
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