Repository navigation
feat: swagger-ui from node_modules - #3141
Merged
Merged
Conversation
✅ Deploy Preview for netdata-docusaurus ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk | ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 7 files. (5 skipped: 5… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly identifies the main change: serving Swagger UI from the installed node_modules package instead of committed vendor assets. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 7 files. (5 skipped: 5 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
-
📝 Generate docstrings 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 36 files
Confidence score: 5/5
- In
tests/swagger_ui_assets.test.js, the pattern skips local URLs with directory paths like/js/...and/img/..., so broken Swagger UI assets can slip through this check. Update it to capture and resolve the full local URL path.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/swagger_ui_assets.test.js">
<violation number="1" location="tests/swagger_ui_assets.test.js:16">
P3: This pattern skips local references containing directories, including `/js/...` and `/img/...`, so the test never verifies those API assets resolve. Capture and resolve the full local URL path before checking whether it is Learn-owned or staged.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Dev as Developer
participant Yarn as Yarn (package manager)
participant NM as node_modules/swagger-ui-dist
participant Plug as swagger-ui-assets Plugin
participant Docus as Docusaurus
participant Static as static/ (api.html, swagger-initializer.js)
participant Gen as generated-static/swagger-ui/
participant Build as build/ (site output)
participant Browser as Browser
participant CF as Cloudflare Analytics
Note over Dev,Build: Build/Start Time Flow
Dev->>Yarn: yarn install / npm ci
Yarn->>NM: Install locked swagger-ui-dist version
NM-->>Dev: Package installed
Dev->>Docus: docusaurus build / start
Docus->>Plug: Load plugin (plugin factory)
Plug->>NM: Resolve swagger-ui-dist package.json
NM-->>Plug: Installed version + file paths
Plug->>Plug: Read SWAGGER_UI_ASSETS list (11 files)
Plug->>Gen: Create/clear dir, copy each asset
alt Missing asset in package
Plug-->>Docus: Throw error (build fails)
else All assets present
Plug-->>Docus: Return plugin config
end
Docus->>Docus: Read staticDirectories from config
Docus->>Static: Include static/ dir
Docus->>Gen: Include generated-static/swagger-ui/ dir
Docus->>Build: Generate static site (api.html + swagger assets)
Note over Browser,CF: Runtime / Serving Flow
Browser->>Build: GET /api
Build-->>Browser: Serve api.html
Browser->>Build: GET swagger-ui-bundle.js, swagger-ui-standalone-preset.js, swagger-ui.css, index.css, favicons
Build-->>Browser: Serve staged assets from generated-static/swagger-ui/
Browser->>Build: GET swagger-initializer.js
Build-->>Browser: Serve from static/
Browser->>CF: Load Cloudflare analytics beacon
Note over Browser,CF: api.html is the only sensitive route for analytics (oauth2-redirect removed)
Browser->>Browser: Render Swagger UI with API spec
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
|
||
| test('the API page loads only Learn-owned files and staged Swagger UI assets', () => { | ||
| const api = fs.readFileSync(path.join(root, 'static', 'api.html'), 'utf8'); | ||
| const localReferences = [...api.matchAll(/(?:src|href)="\.?\/?([^"/:]+\.(?:css|js|png))"/g)] |
There was a problem hiding this comment.
P3: This pattern skips local references containing directories, including /js/... and /img/..., so the test never verifies those API assets resolve. Capture and resolve the full local URL path before checking whether it is Learn-owned or staged.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tests/swagger_ui_assets.test.js, line 16:
<comment>This pattern skips local references containing directories, including `/js/...` and `/img/...`, so the test never verifies those API assets resolve. Capture and resolve the full local URL path before checking whether it is Learn-owned or staged.</comment>
<file context>
@@ -0,0 +1,46 @@
+
+test('the API page loads only Learn-owned files and staged Swagger UI assets', () => {
+ const api = fs.readFileSync(path.join(root, 'static', 'api.html'), 'utf8');
+ const localReferences = [...api.matchAll(/(?:src|href)="\.?\/?([^"/:]+\.(?:css|js|png))"/g)]
+ .map((match) => match[1])
+ .filter((filename) => !fs.existsSync(path.join(root, 'static', filename)));
</file context>
Contributor
Author
There was a problem hiding this comment.
out of the scope from this test, it checks only the files related to the swagger-ui
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Confidence score: 4/5
- In
.github/workflows/dependency-impact.yml, removing theifgate makes the advisory job run on every PR that changespackage.jsonoryarn.lock, not just Dependabot PRs. Restore the gate after testing.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/dependency-impact.yml">
<violation number="1" location=".github/workflows/dependency-impact.yml:23">
P2: Commenting out this `if` gate makes the advisory job run on every PR that touches package.json/yarn.lock, not just Dependabot PRs. It looks like a temporary "test dep-impact" change for this non-Dependabot PR, which runs two full builds plus a headless-browser comparison (up to 45 minutes). Restore the condition before merge, or the expensive job silently runs on every dependency PR.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| name: Advisory dependency impact | ||
| # The PR author, not github.actor: a maintainer re-running the job must not skip it. | ||
| if: ${{ github.event.pull_request.user.login == 'dependabot[bot]' }} | ||
| # if: ${{ github.event.pull_request.user.login == 'dependabot[bot]' }} |
There was a problem hiding this comment.
P2: Commenting out this if gate makes the advisory job run on every PR that touches package.json/yarn.lock, not just Dependabot PRs. It looks like a temporary "test dep-impact" change for this non-Dependabot PR, which runs two full builds plus a headless-browser comparison (up to 45 minutes). Restore the condition before merge, or the expensive job silently runs on every dependency PR.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At .github/workflows/dependency-impact.yml, line 23:
<comment>Commenting out this `if` gate makes the advisory job run on every PR that touches package.json/yarn.lock, not just Dependabot PRs. It looks like a temporary "test dep-impact" change for this non-Dependabot PR, which runs two full builds plus a headless-browser comparison (up to 45 minutes). Restore the condition before merge, or the expensive job silently runs on every dependency PR.</comment>
<file context>
@@ -20,7 +20,7 @@ jobs:
name: Advisory dependency impact
# The PR author, not github.actor: a maintainer re-running the job must not skip it.
- if: ${{ github.event.pull_request.user.login == 'dependabot[bot]' }}
+ # if: ${{ github.event.pull_request.user.login == 'dependabot[bot]' }}
runs-on: ubuntu-latest
timeout-minutes: 45
</file context>
Suggested change
| # if: ${{ github.event.pull_request.user.login == 'dependabot[bot]' }} | |
| if: ${{ github.event.pull_request.user.login == 'dependabot[bot]' }} |
witalisoft
force-pushed
the
deps/swagger-ui-from-node-modules
branch
from
October 12, 2026 08:36
43b4884 to
3a6399d
Compare
witalisoft
marked this pull request as ready for review
October 12, 2026 09:16
ktsaou
approved these changes
Oct 12, 2026
mtblz
approved these changes
Oct 12, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Serve Swagger UI from node_modules instead of a committed vendor copy
Summary
/apiloads Swagger UI from files copied out of the lockedswagger-ui-distpackage at build time. Nothing from the distribution is committed, so a Dependabot bump no longer needs a manual re-vendor step and no longer turns the Netlify build red.flowchart LR lock[yarn.lock<br/>version + integrity] --> nm[node_modules/swagger-ui-dist] nm -->|swagger-ui-assets plugin<br/>on start/build| gen[generated-static/swagger-ui/<br/>gitignored, 11 files] static[static/<br/>api.html, swagger-initializer.js] --> build[build/] gen -->|staticDirectories| buildAlso in this PR:
swagger-uidependency. Nothing imports it; it removes about 80 packages fromyarn.lockand no remaining package changes version.swagger-ui-distinto thebrowser-runtimeDependabot group.tests/dependency_authority.test.jsis updated to match, and its "Swagger must stay independent" assertion is removed.oauth2-redirect.htmlfrom the Cloudflare analytics gate's sensitive routes and from two tests.api.htmlstays protected.Evidence
Before: a Dependabot-style bump (only
package.jsonand theyarn.lockentry changed) fails before Docusaurus even starts:Bump swagger-ui-dist from 5.32.14 to 5.33.1 #3082 (5.32.14 → 5.33.1) shows the same thing in practice: every GitHub check was green,
netlify/netdata-docusaurus/deploy-previewwas red, and the PR was closed until Consolidate Learn dependencies and runtime updates #3136 re-vendored by hand.After: the plugin stages whatever version is installed, with no manual step:
Full Netlify build (
npm run build:netlify, Node 22.23.2): passes. Site build gate: 0 findings across 2,030 pages. Cloudflare analytics gate: "1 credential-handling route" (api.html). Broken-anchor warnings: 297, the same asmaster.Tests:
yarn test:runpasses: Vitest 494 passed and 1 skipped, plus the first-touch (26), IndexNow (64), swagger-ui-assets (3), dependency-authority (3) and site-build-gate (4) suites.Browser (headless Chromium, local
docusaurus serve):/apirenders 72 API operations.swagger-ui.css,index.css,swagger-ui-bundle.js,swagger-ui-standalone-preset.jsandswagger-initializer.jsall return 200. No console errors or warnings.Build output,
mastervs this branch (both built from directories at the same depth, compared file by file):/apiloads (bundle, preset, CSS,index.css, favicons, bundle source map)es-bundle*,swagger-ui.js,oauth2-redirect.*,swagger-ui-vendor.json/api)styles.cssmain.jsstaticDirectoriesvaluemain/styleshash names and one extra blank line; no content changeMerge Danger
Door: two-way
Reverting this PR restores the vendored files and the check exactly. The one thing that doesn't come back on its own is the
/oauth2-redirect.htmlURL: it returns 404 from the first deploy after merge. It is an OAuth callback page, not content, but backlinks for that path haven't been checked yet.Blast Radius:
/api/apicheck (.swagger-ui .opblockcount) shows the page as broken.Summary by cubic
/apinow serves Swagger UI copied from the lockedswagger-ui-distpackage at build/start time instead of a committed vendor copy, so Dependabot bumps no longer fail the Netlify build or require a manual re-vendor step.Notable changes
swagger-ui-assetsplugin stages the 11 filesapi.htmlloads into the gitignoredgenerated-static/swagger-ui/directory; the committed vendor copy, its contract, and the build-time verification script are removed.swagger-uidependency is dropped (~80 fewer packages inyarn.lock), andswagger-ui-distis now grouped withbrowser-runtimein Dependabot./oauth2-redirect.htmland its script are removed; the URL now 404s, but it was dead because the API spec has no OAuth2 scheme.dependency-impactworkflow's author filter is commented out, so its advisory job also runs for non-Dependabot PR authors.Merge notes
/oauth2-redirect.html; backlinks to that path haven't been checked yet.Written for commit 3a6399d. Summary will update on new commits.
Summary by CodeRabbit