Skip to content

Package all js libs - #5481

Merged
ja8zyjits merged 14 commits into
mainfrom
packaje-ALL-js-libs
Jul 14, 2026
Merged

ja8zyjits merged 14 commits into
mainfrom
packaje-ALL-js-libs

Conversation

@gcgoncalves

@gcgoncalves gcgoncalves commented Jul 2, 2026 •

Copy link
Copy Markdown
Collaborator

Pull Request

🔗 Related Issue

Closes https://github.ibm.com/contextforge-org/internal_issues/issues/387


📝 Summary

Packages all JS packages, and then chunks the package to prevent a massive bundle.

📏 Reviewability

  • This PR has one clear purpose
  • The linked issue is not labeled triage
  • Unrelated bugs or improvements are tracked in separate issues/PRs
  • Tests are included with the code they validate
  • If AI-assisted, I understand and can explain the generated changes

🏷️ Type of Change

  • Bug fix
  • Feature / Enhancement
  • Documentation
  • Refactor
  • Chore (deps, CI, tooling)
  • Other (describe below)

🧪 Verification

Run make build-ui dev then test the app on localhost. Make sure there are no errors on the browser console.

Check Command Status
Lint suite make lint ✅
Unit tests make test ✅
Coverage ≥ 80% make coverage ✅

✅ Checklist

  • Code formatted (make black isort pre-commit)
  • Tests added/updated for changes
  • Documentation updated (if applicable)
  • No secrets or credentials committed

@gcgoncalves
gcgoncalves force-pushed the packaje-ALL-js-libs branch from 45c9400 to 8faa479 Compare July 2, 2026 14:53
@gcgoncalves gcgoncalves changed the title Packaje all js libs Package all js libs Jul 2, 2026
@gcgoncalves
gcgoncalves marked this pull request as ready for review July 2, 2026 15:45
@gcgoncalves
gcgoncalves requested review from marekdano and vishu-bh July 2, 2026 15:47
window.htmxConfig.inlineScriptNonce = "{{ csp_nonce(request) }}";
</script>
<script defer src="{{ root_path }}/static/{{ bundle_js }}"></script>
<script type="module" src="{{ root_path }}/static/{{ bundle_js }}"></script>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this PR removes the vendored asset download/build path, I think the ui_airgapped branch here may need to be updated too. It still references /static/vendor/..., so offline mode appears to depend on files that is no longer produced.

@cafalchio cafalchio Jul 2, 2026 •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also update the packages in this PR? Please?

npm update
npm audit
npm audit fix

@marekdano marekdano left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few findings:

Blocking

1. The bundled CSS (Font Awesome + CodeMirror) never gets linked into the page. admin.js now imports the vendor CSS (import 'codemirror/lib/codemirror.css', import '@fortawesome/.../all.min.css', etc.), and Vite does emit it — but only as separate files that the entry is expected to <link> from HTML. After npm run vite:build, the manifest looks like:

  "mcpgateway/admin_ui/index.js": {
      "isEntry": true,
      "file": "bundle-3LZTBtVw.js",
      "css": ["assets/index-RTp56Tkr.css"]   // contains Font Awesome
  }

get_bundle_js_filename() (admin.py) reads only ["file"], so assets/index-*.css and assets/vendor-editor-*.css (the latter holds the .CodeMirror rules) are emitted but never referenced by any template or auto-injected by the bundle. In practice that means CodeMirror editors render unstyled and Font Awesome doesn't load. Would it work to have the manifest reader also return the entry's css array and emit a <link> for each?

2. The vendor/fontawesome path the templates still point at is no longer produced.
admin.html, login.html, and change-password-required.html all keep <link ... href=".../static/vendor/fontawesome/css/all.min.css">, but download-cdn-assets.sh and its Containerfile step are removed and static/vendor/ isn't committed, so that URL 404s. This is the same thing @madhu-mohan-jaishankar flagged. It hits login.html / change-password-required.html hardest since they don't load the JS bundle at all — even once #1 is fixed, those two pages have no Font Awesome source. Might be worth linking the Vite-emitted CSS on those pages too, or otherwise giving them a real path.

Suggestions

3. The lazy-loading currently doesn't split anything. The build reports:

[INEFFECTIVE_DYNAMIC_IMPORT] tools.js/servers.js/gateways.js/teams.js/llmChat.js/ logging.js/metrics.js/plugins.js is dynamically imported by lazy-loader.js but also statically imported by admin.js — dynamic import will not move module into another chunk.

Because admin.js still statically imports these modules, Rollup keeps them in the eager graph, so the tab-click loader + loading indicators don't actually defer anything yet. Similarly, chunk-vendor-charts (~200 KB) is pulled in eagerly via the top-level import { Chart } in admin.js, so Chart.js now loads for non-admins too (it used to be gated behind {% if is_admin %}). If deferral is a goal here, the static import chains into these modules would need to be broken first.

4. CSP still allowlists CDNs that are no longer used. Now that nothing loads from cdnjs/jsdelivr/unpkg, security_headers.py (script-src-elem, style-src, font-src) could drop those origins as a nice follow-up hardening. Not blocking.

5. A couple of test gaps. The tabs.test.js update mocks loadFeature to resolve instantly, so lazy-loader.js itself (dedup, in-flight coalescing, the error path) and the loading-indicator show/hide aren't exercised. A small unit test for the loader would be a good add.

@gcgoncalves
gcgoncalves force-pushed the packaje-ALL-js-libs branch from ed51d5f to 5bb9b8f Compare July 9, 2026 15:53

@marekdano marekdano left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just two more findings

  1. Lazy-loading is a no-op — every "lazy" chunk still loads on initial page load mcpgateway/admin_ui/admin.js

admin.js (the eagerly-evaluated entry) statically imports gateways.js (L188), llmModels.js (L228), logging.js (L278), plugins.js (L342), servers.js (L427), teams.js (L459), and tools.js (L505). Because these are static imports of the entry, Vite emits them as static import statements in bundle-*.js, so the browser fetches and evaluates all of those chunks during initial load — loadFeature() at tab-click time just re-resolves already-loaded modules.

metrics.js and llmChat.js (the only two not imported by name) are grouped by manualChunks into the monitoring chunk (with statically-imported logging.js) and the llm chunk (with statically-imported llmModels.js), so those chunks load eagerly too.

Net effect: the initial payload isn't reduced, and lazy-loader.js, TAB_FEATURE_MAP, the loading-indicator UI, and the await loadFeature(...) path in showTab add complexity and startup cost while deferring nothing. To make lazy-loading real, remove the static feature imports (and their window.Admin.* assignments) from admin.js; otherwise drop the lazy machinery and reword the PR description.

  1. Air-gapped docs point at build paths that don't exist docs/docs/overview/ui.md, docs/docs/deployment/container.md

ui.md states all three container builds "include the Vite-built Admin UI assets via the frontend-builder stage," and container.md says to "Use Containerfile.lite which automatically downloads remaining vendor assets during build." At the PR head, only Containerfile has a frontend-builder stage and copies static/; Containerfile.lite and Containerfile.scratch reference neither frontend-builder, the built static dir, nor any vendor download (and download-cdn-assets.sh is deleted in this PR). A user following the air-gapped guidance with Containerfile.lite/.scratch gets a build with no bundled Admin UI assets. Correct the doc claims to match what those Containerfiles actually do.

window.DOMPurify = DOMPurify;

// Import Font Awesome CSS
import '@fortawesome/fontawesome-free/css/all.min.css';

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR imports CodeMirror CSS, CodeMirror theme CSS, and Font Awesome CSS from admin.js, which means Vite emits CSS files separately and records them in the manifest. However, the backend still only returns a single JS filename via get_bundle_js_filename(), and the template only injects the JS bundle. There is no corresponding generation for manifest[entry].css.

Suggested fix: Replace the single “bundle filename” helper with a manifest helper that returns both:

  1. entry JS file
    2.entry CSS files

Then render:
admin.html

{% for css_file in bundle_css %}
<link rel="stylesheet" href="{{ root_path }}/static/{{ css_file }}">
{% endfor %}
<script type="module" src="{{ root_path }}/static/{{ bundle_js }}"></script>

Also ensure the fallback path handles assets/*.css if the manifest is missing.

@gcgoncalves
gcgoncalves force-pushed the packaje-ALL-js-libs branch 2 times, most recently from c9438b0 to 936515f Compare July 13, 2026 09:46
@gcgoncalves
gcgoncalves requested a review from marekdano July 13, 2026 09:49

@madhu-mohan-jaishankar madhu-mohan-jaishankar left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

marekdano
marekdano previously approved these changes Jul 13, 2026

@marekdano marekdano left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR looks good!

LGTM 🚀

Signed-off-by: Gabriel Costa <gabrielcg@proton.me>
Signed-off-by: Gabriel Costa <gabrielcg@proton.me>
Signed-off-by: Gabriel Costa <gabrielcg@proton.me>
Signed-off-by: Gabriel Costa <gabrielcg@proton.me>
Signed-off-by: Gabriel Costa <gabrielcg@proton.me>
Signed-off-by: Gabriel Costa <gabrielcg@proton.me>
Signed-off-by: Gabriel Costa <gabrielcg@proton.me>
Signed-off-by: Gabriel Costa <gabrielcg@proton.me>
Signed-off-by: Gabriel Costa <gabrielcg@proton.me>
Signed-off-by: Gabriel Costa <gabrielcg@proton.me>
Signed-off-by: Gabriel Costa <gabrielcg@proton.me>
Signed-off-by: Gabriel Costa <gabrielcg@proton.me>
Signed-off-by: Gabriel Costa <gabrielcg@proton.me>
Signed-off-by: Gabriel Costa <gabrielcg@proton.me>

@ja8zyjits ja8zyjits left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ja8zyjits
ja8zyjits added this pull request to the merge queue Jul 14, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 14, 2026
@ja8zyjits
ja8zyjits added this pull request to the merge queue Jul 14, 2026
@ja8zyjits ja8zyjits self-assigned this Jul 14, 2026
Merged via the queue into main with commit 3f06618 Jul 14, 2026
62 checks passed
@ja8zyjits
ja8zyjits deleted the packaje-ALL-js-libs branch July 14, 2026 11:16
@cafalchio cafalchio mentioned this pull request Jul 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-time-findings issues discovered during release testing.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG]: Production (gunicorn) E2E run - Edit Gateway OAuth toggle, server delete, RBAC invite cascade

5 participants