fix: add cache busting to web component extractor #1731
Conversation
WalkthroughSkip processing of the manifest entry with key Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Extractor
participant Manifest
Caller->>Extractor: extract(manifest)
Extractor->>Manifest: iterate entries
alt entry.key == "ts"
Note right of Extractor #DDDDDD: Skip processing this entry
Extractor-->>Manifest: continue
else other entries
Extractor->>Extractor: build asset paths & tags (unchanged)
end
Extractor-->>Caller: return built assets/tags
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1731 +/- ##
==========================================
+ Coverage 53.31% 53.34% +0.03%
==========================================
Files 860 860
Lines 48271 48271
Branches 4918 4926 +8
==========================================
+ Hits 25734 25752 +18
+ Misses 22468 22450 -18
Partials 69 69 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/include/web-components-extractor.php (3)
38-46: Harden version appender: handle anchors and avoid duplicate v=Current logic appends after a fragment (e.g., url#frag?v=…) and can create multiple v= params if called twice. Insert before any '#' and replace existing v= to be idempotent.
- private function appendVersionQuery(string $path, string $version): string - { - if ($version === '') { - return $path; - } - - $separator = strpos($path, '?') !== false ? '&' : '?'; - return $path . $separator . 'v=' . rawurlencode($version); - } + private function appendVersionQuery(string $path, string $version): string + { + if ($version === '') { + return $path; + } + + // Split off any fragment so query params are added before '#' + $fragment = null; + $base = $path; + $hashPos = strpos($path, '#'); + if ($hashPos !== false) { + $fragment = substr($path, $hashPos + 1); + $base = substr($path, 0, $hashPos); + } + + $encoded = rawurlencode($version); + + // If v already present, replace it (idempotent); otherwise append + if (preg_match('/([?&])v=/', $base)) { + $base = preg_replace('/([?&])v=[^&]*/', '$1v=' . $encoded, $base, 1); + } else { + $base .= (strpos($base, '?') !== false ? '&' : '?') . 'v=' . $encoded; + } + + return $fragment !== null ? $base . '#' . $fragment : $base; + }
84-86: Skip any alternate version keys if fallback is adoptedIf you add the optional fallback, also skip those keys during iteration.
- if ($key === 'ts') { + if (in_array($key, ['ts','version','build','timestamp'], true)) { continue; }
75-81: Add optional fallback keys for version extraction
Quick check confirms every manifest has a valid ‘ts’ field, so this change isn’t required now; still, iterating over ['ts','version','build','timestamp'] can future-proof the code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/include/web-components-extractor.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/include/web-components-extractor.php (1)
web/scripts/add-timestamp-webcomponent-manifest.js (1)
filePath(4-4)
🔇 Additional comments (2)
plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/include/web-components-extractor.php (2)
94-94: LGTM: applying version when building the asset srcUsing the helper at construction time is correct and ensures a single, consistent cache buster per request.
115-115: LGTM: per‑entry CSS gets the same versionConsistent versioning for CSS referenced from JS entries avoids stale styles after deploys.
…and update tests for standalone assets - Removed the `appendVersionQuery` method from `WebComponentsExtractor` to simplify asset path generation. - Updated test cases to reflect changes in asset naming, ensuring correct handling of standalone JS and CSS files with hashed names. - Adjusted Vite configuration to generate hashed filenames for standalone assets, improving cache busting.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/vite.config.ts (1)
118-146: Replace static standalone-apps asset URLs in test loader
Inweb/public/test-pages/load-manifest.js(around lines 114–128), update the hard-coded/dist/standalone-apps.jsand/dist/standalone-apps.cssreferences to use the hashed filenames (for example, by pulling them from the Vite manifest or using a dynamic lookup) instead of static URLs.
🧹 Nitpick comments (4)
web/vite.config.ts (2)
11-13: Use .js extensions on local TS ESM imports.Per the repo guidelines for TS/ESM, add .js extensions to local relative imports to avoid Node16/Bundler resolution pitfalls.
As per coding guidelines.
-import scopeTailwindToUnapi from './postcss/scopeTailwindToUnapi'; -import { serveStaticHtml } from './vite-plugin-serve-static'; +import scopeTailwindToUnapi from './postcss/scopeTailwindToUnapi.js'; +import { serveStaticHtml } from './vite-plugin-serve-static.js';
163-186: Tighten types on proxy error handler; remove assertion.Avoid casting; model proper types.
http-proxycan pass an HTTP response or a socket (for WS). Use union types and a type guard to remove theas { writeHead?... }assertion.As per coding guidelines.
+import type { IncomingMessage, ServerResponse } from 'node:http'; +import type { Socket } from 'node:net'; @@ - proxy.on('error', (err: Error, _req: unknown, res: unknown) => { + proxy.on('error', (err: Error, _req: IncomingMessage | undefined, res?: ServerResponse | Socket) => { console.warn('[Vite] GraphQL proxy error (API server may not be running):', err.message); - // Check if res has writeHead method (it's an HTTP response, not a socket) - const httpRes = res as { - writeHead?: (statusCode: number, headers: Record<string, string>) => void; - end?: (data: string) => void; - }; - if ( - httpRes && - typeof httpRes.writeHead === 'function' && - typeof httpRes.end === 'function' - ) { - httpRes.writeHead(503, { - 'Content-Type': 'application/json', - }); - httpRes.end( - JSON.stringify({ - error: 'GraphQL API server not available', - message: 'Please start the API server on port 3001', - }) - ); - } + // If this is an HTTP response (not a socket), return a JSON error + if (res && typeof (res as ServerResponse).writeHead === 'function' && typeof (res as ServerResponse).end === 'function') { + const httpRes = res as ServerResponse; + httpRes.writeHead(503, { 'Content-Type': 'application/json' }); + httpRes.end(JSON.stringify({ + error: 'GraphQL API server not available', + message: 'Please start the API server on port 3001', + })); + } });plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/include/web-components-extractor.php (1)
81-110: Optional: consider reserving a small set of non-asset keys.If other metadata keys are added (e.g.,
timestamp,version), extend the guard to a reserved set to keep logic future-proof.- foreach ($manifest as $key => $entry) { - if ($key === 'ts') { + foreach ($manifest as $key => $entry) { + if (in_array($key, ['ts', 'timestamp', 'version'], true)) { continue; }plugin/tests/test-extractor.php (1)
60-60: Add an explicit test that 'ts' entries are ignored.You already include a
tskey in the fixture; add a quick assertion to ensure it never renders.@@ $output = $this->getExtractorOutput(); @@ echo "\nTest: Invalid Entries Handling\n"; echo "-------------------------------\n"; $this->test( "Skips entries without 'file' key", strpos($output, 'notAFile') === false ); + $this->test( + "Skips manifest metadata key 'ts'", + strpos($output, 'id="unraid-standalone-apps-ts"') === false + );Also applies to: 131-208
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/include/web-components-extractor.php(1 hunks)plugin/tests/test-extractor.php(8 hunks)web/vite.config.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript import specifiers with .js extensions for ESM compatibility
Never use the any type; prefer precise typing
Avoid type casting; model proper types from the start
Files:
web/vite.config.ts
🧬 Code graph analysis (1)
plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/include/web-components-extractor.php (1)
web/scripts/add-timestamp-standalone-manifest.js (1)
key(20-20)
🔇 Additional comments (6)
web/vite.config.ts (1)
129-136: Cache-busted entry and CSS naming look good.Hashing the entry JS and consolidating CSS to a single hashed asset should resolve stale asset issues and plays nicely with the extractor/tests. No functional concerns from this change.
plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/include/web-components-extractor.php (1)
65-71: Explicitly skipping the 'ts' manifest key is fine (and clearer).This is functionally redundant with the subsequent validation but makes intent obvious and avoids accidental processing if the shape changes later. Low risk; approved.
plugin/tests/test-extractor.php (4)
17-18: Good switch to dynamic hashed filenames in fixtures.Using
$standaloneJsFile/$standaloneCssFilemirrors production and decouples tests from fixed names. Looks solid.Also applies to: 51-58
149-151: Assertions correctly updated for hashed IDs/paths.Expectations for script/link IDs and paths now account for hashing and sanitization; this aligns with the extractor’s ID generation.
Also applies to: 165-167, 214-215, 279-280, 283-284
350-353: Helper matches production sanitizer; nice.
sanitizeForExpectedIdmirrors the extractor’s regex, reducing brittle tests.
376-376: Minor: exit status propagation is correct.
exit($test->run());properly propagates test status to the shell. LGTM.
🤖 I have created a release *beep* *boop* --- ## [4.25.1](v4.25.0...v4.25.1) (2025-09-30) ### Bug Fixes * add cache busting to web component extractor ([#1731](#1731)) ([0d165a6](0d165a6)) * Connect won't appear within Apps - Previous Apps ([#1727](#1727)) ([d73953f](d73953f)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary by CodeRabbit