-
Notifications
You must be signed in to change notification settings - Fork 5
refactor: load client scripts with @JsModule instead of executeJs #412
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
totally-not-ai
wants to merge
9
commits into
main
Choose a base branch
from
refactor/load-client-scripts-as-files
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+294
−425
Open
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
f8060df
test: load client scripts as files instead of executeJs
totally-not-ai[bot] 0bd357b
refactor: bundle client scripts with @JsModule instead of executeJs
totally-not-ai[bot] 9b0148f
test: check that loading the collector module does not install it
totally-not-ai[bot] 49741ec
test: drop ClientScriptLoadingTest
totally-not-ai[bot] c0625e8
test: check the details flag the collector element is given
totally-not-ai[bot] 9141014
fix: keep the Copilot panel hidden while the kit is not active
totally-not-ai[bot] 5d21eba
feat: show the Copilot panel with a license notice when unlicensed
totally-not-ai[bot] 5e466ba
test: check that a valid license clears the missing-license flag
totally-not-ai[bot] 15b2fd0
Merge branch 'main' into refactor/load-client-scripts-as-files
Artur- File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
95 changes: 0 additions & 95 deletions
95
...it-micrometer/src/main/java/com/vaadin/observability/micrometer/ClientResourceLoader.java
This file was deleted.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
33 changes: 0 additions & 33 deletions
33
...ometer/src/main/java/com/vaadin/observability/micrometer/ObservabilityDevToolsClient.java
This file was deleted.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before this change the panel script only reached the browser after
serviceInithad passed the license and configuration checks. Now the module is bundled and registers itself with Copilot on every dev-mode page load, and the handler is always registered through the service loader. So an unlicensed or unconfigured app still gets the Observability panel in Copilot, polling an empty registry and showing "instrumentation: inactive".If we want to keep the old behaviour (no license, no panel), the panel could ask the handler whether the kit is active before adding itself. Or the handler could answer with nothing when there is no active registry, and the panel could stay hidden in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@heruan Good catch, this now keeps the old "no license, no panel" behaviour. I combined your two suggestions:
ObservabilityDevToolsHandlerstays silent until the kit has an active registry, i.e. it is licensed andserviceInithas run. It still claims its commands, so nothing gets queued for other handlers.addPanelininit. It adds itself to Copilot, once, when the first metrics or insights answer arrives. It keeps polling, so if the app restarts in the same tab with a valid license, the panel shows up.An unlicensed or unconfigured app now gets no Observability entry in Copilot, and never sees
instrumentation: inactivefrom an unbound kit. Tests:kitNotActive_answersNothingSoThePanelStaysHiddeninObservabilityDevToolsHandlerTest, and a case inVaadinObservabilityDevTools.test.jschecking there is no panel before an answer and exactly one after.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better if the panel always showed up and even had the correct content but all numbers would be missing + there would be a text about needing a license for it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Artur- Agreed, done. The panel is back to always registering with Copilot, and the "wait for the first answer" gating is gone. What changed:
MetricsServiceInitListenerrecords a failed license check inObservabilityKit.isLicenseMissing(), a new public getter next togetActiveMeterRegistry().licensed.licensedisfalse, the panel shows a notice at the top: "Observability Kit needs a license. Without one, no metrics or insights are collected", with a link to the license terms. The Insights and Metrics sections keep their usual layout but read "Not collected without a license." That replaces the hints about the insights setting and about interacting with the app, which would be misleading here.Tests:
licenseMissing_stillAnswersAndSaysSoinObservabilityDevToolsHandlerTest, an assertion in the license test that the flag is recorded, and a JS case checking the notice appears when unlicensed and not otherwise.