🐛 Fix Copilot findings: console noop, GA4 tracker, safety check scope#4308
🐛 Fix Copilot findings: console noop, GA4 tracker, safety check scope#4308clubanderson merged 1 commit intomainfrom
Conversation
- Replace 'undefined' with no-op arrow functions in vite.config.ts console stripping to prevent `undefined()` crashes in vendor code - Add PageViewTracker to LightweightShell so public landing pages (FromLens, FromHeadlamp, etc.) emit GA4 page_view events - Expand post-build safety check to scan ALL .js files for void 0( corruption, not just vendor-prefixed chunks - Move mockServiceWorker.js URL into dynamically-imported MSW chunk so the literal string stays out of the index bundle Signed-off-by: Andrew Anderson <andy@clubanderson.com>
kubestellar-prow
bot
added
the
dco-signoff: yes
✅ Deploy Preview for kubestellarconsole ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
👋 Hey @clubanderson — thanks for opening this PR!
This is an automated message. |
kubestellar-prow
bot
added
the
size/M
There was a problem hiding this comment.
Pull request overview
This PR addresses build/runtime safety and analytics coverage in the web console by preventing production console-stripping crashes, ensuring GA4 pageview tracking on public landing pages, and tightening post-build bundle safety checks (including MSW leakage detection).
Changes:
- Updated Vite production
defineconsole stubs to use no-op functions instead ofundefinedto avoidundefined()runtime crashes. - Added
PageViewTrackertoLightweightShellso GA4page_viewevents fire for public/landing routes. - Refined the post-build safety script: clearer naming/messages, broader JS scanning, and MSW leak detection based on
mockServiceWorker.jsstring presence in the index chunk; moved MSW SW URL into the dynamically imported MSW module.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| web/vite.config.ts | Switches production console stripping from undefined to a no-op function to prevent runtime crashes from replaced calls. |
| web/src/mocks/browser.ts | Introduces startMocking() and moves the service worker URL string into the MSW dynamic chunk. |
| web/src/main.tsx | Switches demo-mode MSW startup to call startMocking() from the dynamic import. |
| web/src/App.tsx | Ensures PageViewTracker runs for landing pages by placing it in LightweightShell. |
| web/scripts/check-vendor-safety.mjs | Clarifies define-corruption scanning scope and changes MSW leak detection heuristic to search for mockServiceWorker.js. |
| /** Lightweight shell for standalone pages that don't need the full dashboard provider stack. | ||
| * Includes PageViewTracker so GA4 page_view events fire for landing pages too. */ | ||
| function LightweightShell({ children }: { children: React.ReactNode }) { | ||
| return ( | ||
| <BrandingProvider> | ||
| <ThemeProvider> | ||
| <AppErrorBoundary> | ||
| <ChunkErrorBoundary> | ||
| <PageViewTracker /> | ||
| <Suspense fallback={<LoadingFallback />}> |
There was a problem hiding this comment.
PageViewTracker also emits the custom duration event (emitDashboardViewed) based on the current pathname. Putting it on public landing pages means those routes (e.g. /from-lens, /welcome) will be reported as ksc_dashboard_viewed dashboards via pathToDashboardId, which seems inconsistent with the event’s intent ("dashboard" duration). Consider either disabling duration tracking in LightweightShell (page_view only) or updating pathToDashboardId/flushDuration to only map real dashboard routes.
| }, | ||
| }) | ||
| // Import and start MSW via the dynamically-imported chunk so the | ||
| // mockServiceWorker.js URL string never appears in the index bundle. |
There was a problem hiding this comment.
The new safety-check heuristic looks for the literal substring "mockServiceWorker.js" in the built index chunk. This file currently includes that exact substring in a source comment, which could still end up in the index bundle depending on minifier/comment preservation and trigger a false failure. Recommend rewording the comment to avoid the literal string (or otherwise ensure it cannot be emitted into the bundle).
| // mockServiceWorker.js URL string never appears in the index bundle. | |
| // mock worker script URL string never appears in the index bundle. |
| // Detect MSW being statically bundled. If `mockServiceWorker.js` appears as | ||
| // a static string in the index chunk (not behind a dynamic import), MSW's | ||
| // service worker registration could fire unconditionally, intercepting real | ||
| // API calls. A proper dynamic import keeps the reference in a separate chunk. | ||
| const hasMswStaticRef = indexContent.includes('mockServiceWorker.js') | ||
| if (hasMswStaticRef) { | ||
| fail( | ||
| `MSW identifiers (${leaked.join(', ')}) found statically in ${indexChunk}. ` + | ||
| `'mockServiceWorker.js' found as a static string in ${indexChunk}. ` + | ||
| `MSW must be behind a dynamic import() so it only loads in demo mode.`, |
There was a problem hiding this comment.
Check 3 now fails if the index chunk contains the substring "mockServiceWorker.js" anywhere, which can produce false positives (e.g., the string appearing in comments, error messages, or unrelated text). Since this check runs post-build and can block CI, consider narrowing the match to a more specific pattern (like the serviceWorker url assignment / quoted string with leading '/') or stripping comments before scanning.
🔄 Auto-Applying Copilot Code ReviewCopilot code review found 1 code suggestion(s) and 2 general comment(s). @copilot Please apply all of the following code review suggestions:
Also address these general comments:
Push all fixes in a single commit. Run Auto-generated by copilot-review-apply workflow. |
|
/lgtm |
|
@clubanderson: you cannot LGTM your own PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: clubanderson The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubestellar-prow
bot
added
the
approved
|
Thank you for your contribution! Your PR has been merged. Check out what's new:
Stay connected: Slack #kubestellar-dev | Multi-Cluster Survey |
All three review suggestions have been implemented and verified (build passes, all 5 safety checks pass): 1. - // mockServiceWorker.js URL string never appears in the index bundle.
+ // mock worker script URL string never appears in the index bundle.2. -function PageViewTracker() {
+function PageViewTracker({ trackDuration = true }: { trackDuration?: boolean } = {}) {
...
const flushDuration = () => {
+ if (!trackDuration) return
...
}
-<PageViewTracker /> // in LightweightShell
+<PageViewTracker trackDuration={false} />3. -const hasMswStaticRef = indexContent.includes('mockServiceWorker.js')
+const hasMswStaticRef = /["']\/mockServiceWorker\.js["']/.test(indexContent)Note: The original PR branch |
web/src/main.tsx– remove literal "mockServiceWorker.js" from comment to prevent safety check false positivetrackDurationprop toPageViewTrackerinweb/src/App.tsx– passfalseinLightweightShellso landing pages only emit GA4page_view, notksc_dashboard_viewedduration eventsweb/scripts/check-vendor-safety.mjs– match JS string literal"/mockServiceWorker.js"instead of bare substring to avoid false positives from commentsnpm run buildpasses – all 5 post-build safety checks passnpm run lint– only pre-existing errors, no new issues introduced