Skip to content

chore: add check about legacy theme annotation#145

Merged
paodb merged 1 commit intomasterfrom
legacy-theme-check
Feb 10, 2026
Merged

chore: add check about legacy theme annotation#145
paodb merged 1 commit intomasterfrom
legacy-theme-check

Conversation

@javier-godoy
Copy link
Member

@javier-godoy javier-godoy commented Feb 10, 2026

Summary by CodeRabbit

  • Bug Fixes
    • Initialization now performs legacy-theme pre-checks and fails fast if an incompatible legacy theme is detected (covers startup and HTML response paths).
  • Documentation
    • Initialization docs updated to note that legacy theme configuration can cause an initialization error.

@coderabbitai
Copy link

coderabbitai bot commented Feb 10, 2026

Walkthrough

Added a private validation method assertNotLegacyTheme() to DynamicTheme.java that detects legacy @Theme usage via AppShellRegistry/AppShellConfigurator and throws IllegalStateException; the check is invoked at the start of initialize(AppShellSettings) and initialize(IndexHtmlResponse) and referenced in Javadoc for relevant initialization paths.

Changes

Cohort / File(s) Summary
DynamicTheme validation
src/main/java/com/flowingcode/vaadin/addons/demo/DynamicTheme.java
Added private assertNotLegacyTheme() that reads VaadinContext, resolves AppShellConfigurator via AppShellRegistry, and throws IllegalStateException when an @Theme annotation is present. Added imports (AppShellConfigurator, AppShellRegistry, VaadinContext, VaadinService, Theme). Invoked the check at the start of initialize(AppShellSettings) and initialize(IndexHtmlResponse) and updated related Javadoc to document the exception path.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • mlopezFC
  • paodb
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ 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 check for legacy theme annotation. It is specific, concise, and directly reflects the primary purpose of the changeset.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ 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 legacy-theme-check

No actionable comments were generated in the recent review. 🎉

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.


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

🤖 Fix all issues with AI agents
In `@src/main/java/com/flowingcode/vaadin/addons/demo/DynamicTheme.java`:
- Around line 116-118: In DynamicTheme, the initialize(IndexHtmlResponse)
overload currently calls assertFeatureSupported() but misses the legacy-theme
check; update the initialize(IndexHtmlResponse) method to call
assertNotLegacyTheme() immediately after assertFeatureSupported() so both
initialization paths (initialize(AppShellSettings) and
initialize(IndexHtmlResponse)) enforce the same legacy-theme validation via
assertNotLegacyTheme().
- Around line 82-89: The method assertNotLegacyTheme() currently calls
AppShellRegistry.getInstance(context).getShell() and immediately dereferences
the result; add a null-check for the returned Class (e.g., store in
appShellClass and if (appShellClass == null) return) before calling
getAnnotation to avoid a NullPointerException, and keep the existing
IllegalStateException only when a Theme annotation is present; also verify that
initialize(IndexHtmlResponse) invokes assertNotLegacyTheme() (or perform the
same null-safe check there) so both initialization paths enforce the
legacy-theme check consistently.

@sonarqubecloud
Copy link

@javier-godoy javier-godoy requested a review from paodb February 10, 2026 17:18
@javier-godoy javier-godoy marked this pull request as ready for review February 10, 2026 17:18
@paodb paodb merged commit ab41a98 into master Feb 10, 2026
3 checks passed
@paodb paodb deleted the legacy-theme-check branch February 10, 2026 18:14
@github-project-automation github-project-automation bot moved this to Pending release in Flowing Code Addons Feb 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Pending release

Development

Successfully merging this pull request may close these issues.

2 participants