feat: Add prop version to plugins manifest to avoid browser caching#24153
Conversation
WalkthroughAdded an optional field Sequence Diagram(s)sequenceDiagram
participant Client
participant GraphQL
participant PluginService
participant Helper as createFinalJavascriptEntrypointUrl
note right of Helper #E6F7FF: computes absolute URL if needed, validates manifest.version, appends version query param when valid
Client->>GraphQL: request plugin data
GraphQL->>PluginService: fetch PluginManifestContent
PluginService->>Helper: createFinalJavascriptEntrypointUrl(plugin, jsEntrypointAbsoluteUrl)
Helper->>Helper: if relative -> makeAbsoluteUrl(relative, plugin.baseUrl)
alt manifest.version exists
Helper->>Helper: Version.isValid(manifest.version)?
alt valid
Helper-->>PluginService: absoluteUrl + "?<param>=<version>"
else invalid
Helper-->>PluginService: absoluteUrl (and log warning)
end
else no version
Helper-->>PluginService: absoluteUrl
end
PluginService-->>GraphQL: return plugin data (final entrypoint URL)
GraphQL-->>Client: respond with plugin data
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
akka-bbb-apps/src/main/scala/org/bigbluebutton/core/models/Plugins.scala(3 hunks)docs/docs/plugins.md(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-package (bbb-apps-akka)
🔇 Additional comments (3)
docs/docs/plugins.md (1)
181-231: LGTM! Clear and accurate documentation.The documentation changes effectively describe the new
pluginVersionfeature:
- Grammar correction improves readability
- Manifest example properly demonstrates the field usage
- The explanation clearly communicates the cache-busting purpose
- Example URL format is helpful for understanding the behavior
akka-bbb-apps/src/main/scala/org/bigbluebutton/core/models/Plugins.scala (2)
42-42: LGTM! Proper field addition.The
pluginVersionfield is correctly defined asOption[String]with a default ofNone, maintaining backward compatibility with existing plugin manifests.
85-86: LGTM! Good refactoring.Extracting the URL construction logic into a dedicated helper function improves code organization and makes the version parameter logic easier to maintain.
akka-bbb-apps/src/main/scala/org/bigbluebutton/core/models/Plugins.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
akka-bbb-apps/src/main/scala/org/bigbluebutton/core/models/Plugins.scala (1)
126-128: Minor: Fix indentation inconsistency.Lines 126-128 have 2 extra spaces of indentation compared to the surrounding code.
Apply this diff to normalize the indentation:
plugin.manifest.content.foreach { manifest => manifest.pluginVersion.foreach { version => - if (!Version.isValid(version)) { - logger.warn("pluginVersion for [{}] is not valid, ignoring...", manifest.name) - } + if (!Version.isValid(version)) { + logger.warn("pluginVersion for [{}] is not valid, ignoring...", manifest.name) + } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
akka-bbb-apps/src/main/scala/org/bigbluebutton/core/models/Plugins.scala(3 hunks)
🔇 Additional comments (3)
akka-bbb-apps/src/main/scala/org/bigbluebutton/core/models/Plugins.scala (3)
42-42: LGTM!The optional
pluginVersionfield is properly added with backward-compatible defaults.
85-86: LGTM!The refactoring correctly delegates URL construction to the new helper method and properly updates the manifest content.
116-136: Excellent fix for the critical bug!The for-comprehension now correctly uses a guard clause to filter invalid versions, ensuring
maybeVersionis typed asOption[String]instead ofOption[Any]. The validation warning has been properly moved to a separateforeachblock. This implementation resolves the type-safety issue from the previous review.Based on learnings
akka-bbb-apps/src/main/scala/org/bigbluebutton/core/models/Plugins.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
akka-bbb-apps/src/main/scala/org/bigbluebutton/core/models/Plugins.scala (1)
118-138: Version validation logic is correct; consider simplifying the warning code.The validation logic correctly uses a guard clause in the for-comprehension to ensure
maybeVersionisOption[String], andURIBuilder.addParameterproperly handles existing query parameters. However, the warning logic can be simplified.Apply this diff to simplify the warning code:
plugin.manifest.content.foreach { manifest => manifest.pluginVersion.foreach { version => - if (!Version.isValid(version)) { - logger.warn("pluginVersion for [{}] is not valid, ignoring...", manifest.name) - } - } + if (!Version.isValid(version)) { + logger.warn("pluginVersion for [{}] is not valid, ignoring...", manifest.name) + } + } }This removes unnecessary indentation while maintaining the same functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
akka-bbb-apps/src/main/scala/org/bigbluebutton/core/models/Plugins.scala(4 hunks)docs/docs/plugins.md(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-package (bbb-apps-akka)
🔇 Additional comments (4)
docs/docs/plugins.md (2)
187-187: LGTM! Clear optional field indicator.The
pluginVersionfield is properly documented as optional in the manifest example, consistent with the pattern used for other optional fields.
225-232: LGTM! Concise and clear documentation.The
pluginVersiondocumentation effectively explains the cache-busting behavior with a clear example. The brevity aligns with previous review feedback.akka-bbb-apps/src/main/scala/org/bigbluebutton/core/models/Plugins.scala (2)
43-43: LGTM! Optional field properly defined.The
pluginVersionfield is correctly defined asOption[String]with aNonedefault, making it truly optional for plugin manifests.
86-87: Integration looks correct, pending makeAbsoluteUrl fix.The integration correctly replaces relative JavaScript entrypoint URLs with absolute URLs including the version parameter when applicable. However, correctness depends on the
makeAbsoluteUrlfix noted in the earlier comment.
akka-bbb-apps/src/main/scala/org/bigbluebutton/core/models/Plugins.scala
Show resolved
Hide resolved
GuiLeme
force-pushed
the
avoid-plugin-cache
branch
from
7586649 to
80a2db8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
akka-bbb-apps/src/main/scala/org/bigbluebutton/core/models/Plugins.scala (1)
113-116: Prefer robust URL resolution for relative paths.String concat won’t normalize "../" and may produce "//" when relativeUrl starts with "/". Use URI.resolve for correctness.
- private def makeAbsoluteUrl(plugin: Plugin, relativeUrl: String): String = { - val baseUrl = plugin.manifest.url.substring(0, plugin.manifest.url.lastIndexOf('/') + 1) - baseUrl + relativeUrl - } + private def makeAbsoluteUrl(plugin: Plugin, relativeUrl: String): String = { + val manifestUri = java.net.URI.create(plugin.manifest.url) + manifestUri.resolve(relativeUrl).toString + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
akka-bbb-apps/src/main/scala/org/bigbluebutton/core/models/Plugins.scala(4 hunks)docs/docs/plugins.md(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/docs/plugins.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-package (bbb-apps-akka)
🔇 Additional comments (2)
akka-bbb-apps/src/main/scala/org/bigbluebutton/core/models/Plugins.scala (2)
11-11: Good choice: use URIBuilder for query handling.This enables safe param encoding and preserves existing query parts.
43-43: Schema addition looks correct and backward‑compatible.Optional pluginVersion defaults to None; safe for existing manifests.
akka-bbb-apps/src/main/scala/org/bigbluebutton/core/models/Plugins.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
akka-bbb-apps/src/main/scala/org/bigbluebutton/core/models/Plugins.scala (1)
115-118: Consider handling edge cases in relative URL resolution.The current string-concatenation approach works for typical relative paths (
MyPlugin.js,./MyPlugin.js) but has edge cases:
Leading slash: If
relativeUrlstarts with/, it should resolve from the server root, not the manifest directory. CurrentlybaseUrl="https://example.com/plugins/foo/"+relativeUrl="/scripts/plugin.js"produces"https://example.com/plugins/foo//scripts/plugin.js"instead of"https://example.com/scripts/plugin.js".Protocol-relative URLs: URLs like
//cdn.example.com/file.jswould be concatenated incorrectly.Since most plugins use simple relative paths, this is unlikely to cause issues in practice. If edge-case robustness is desired, consider adding a check:
private def makeAbsoluteUrl(plugin: Plugin, relativeUrl: String): String = { + if (relativeUrl.startsWith("/") || relativeUrl.startsWith("//")) { + // Absolute path from root or protocol-relative + val manifestUrlBuilder = new URIBuilder(plugin.manifest.url) + val baseUrl = manifestUrlBuilder.getScheme + "://" + manifestUrlBuilder.getHost + + (if (manifestUrlBuilder.getPort != -1) s":${manifestUrlBuilder.getPort}" else "") + return if (relativeUrl.startsWith("//")) manifestUrlBuilder.getScheme + ":" + relativeUrl else baseUrl + relativeUrl + } val baseUrl = plugin.manifest.url.substring(0, plugin.manifest.url.lastIndexOf('/') + 1) baseUrl + relativeUrl }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
akka-bbb-apps/src/main/scala/org/bigbluebutton/core/models/Plugins.scala(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-package (bbb-web)
- GitHub Check: build-package (bbb-apps-akka)
🔇 Additional comments (4)
akka-bbb-apps/src/main/scala/org/bigbluebutton/core/models/Plugins.scala (4)
11-11: LGTM!The URIBuilder import is necessary for the query parameter handling in
createFinalJavascriptEntrypointUrland ensures proper URL encoding.
43-43: LGTM!The optional
pluginVersionfield is properly declared with aNonedefault, ensuring backward compatibility with existing plugin manifests.
85-92: LGTM! Past concerns about absolute URLs have been addressed.The refactored logic now correctly handles both relative and absolute entrypoint URLs:
- Relative URLs are converted to absolute via
makeAbsoluteUrl- Absolute URLs (e.g., CDN links) are passed through unchanged
- Both types then flow through
createFinalJavascriptEntrypointUrlto receive the version query parameterThis ensures cache-busting works for all plugin entrypoints, addressing the major concern from previous reviews.
119-138: LGTM! Past critical validation bug and major parameter handling concerns have been resolved.The implementation correctly addresses previous review feedback:
Validation logic fix (lines 120-124): The guard clause
if Version.isValid(version)ensuresmaybeVersionisOption[String]rather thanOption[Any]. Invalid versions produceNone, preventing malformed query parameters like?version=().Separate warning (lines 126-132): The warning for invalid versions is logged independently, keeping the for-comprehension clean and type-safe.
setParameter usage (line 135): Replaces any existing
versionparameter instead of creating duplicates, ensuring a single canonical version parameter in the URL.The function safely handles missing manifests, missing versions, and invalid versions through proper Option chaining and validation.
akka-bbb-apps/src/main/scala/org/bigbluebutton/core/models/Plugins.scala
Outdated
Show resolved
Hide resolved
akka-bbb-apps/src/main/scala/org/bigbluebutton/core/models/Plugins.scala
Show resolved
Hide resolved
|
Tested here it it's creating the new URL properly:
It should solve the issue! Thanks |
…nd changes in review
|
Automated tests Summary✅ All the CI tests have passed! |
gustavotrott
changed the title
pluginVersion support to avoid browser cachingversion to plugins manifest to avoid browser caching
|
@GuiLeme when you have a chance please include this prop to the manifest of the oficial plugins! |
3 participants
What does this PR do?
This PR introduces support for the optional
versiondirective in plugin manifests.When defined, the plugin version is automatically appended as a query parameter to the
javascriptEntrypointUrlduring plugin loading (e.g.,MyPlugin.js?version=0.0.8).This change helps prevent browsers from caching outdated plugin scripts between deployments, ensuring that the latest version is always loaded without affecting plugin behavior.
Closes
Closes bigbluebutton/bigbluebutton-html-plugin-sdk#224
Motivation
Previously, when redeploying a plugin without changing its filename, browsers could continue serving a cached version of the JavaScript entry point. This often led to unexpected behavior or outdated logic being executed.
By adding the
versiondirective, we ensure that each deployment produces a unique URL, prompting browsers to fetch the updated file instead of using the cached one.How to test
versionfield in your plugin’s manifest, for example:{ "version": "0.0.8" }No additional configuration is required.
More
versiondirective section in the plugin manifest docs)