feat: Consolidate Third-Party CDN Fallbacks#1960
feat: Consolidate Third-Party CDN Fallbacks#1960desireddymohithreddy0925 wants to merge 6 commits into
Conversation
Signed-off-by: DESIREDDY MOHITH REDDY <desireddymohithreddy@MRD-2.local>
Signed-off-by: DESIREDDY MOHITH REDDY <desireddymohithreddy@MRD-2.local>
Signed-off-by: DESIREDDY MOHITH REDDY <desireddymohithreddy@MRD-2.local>
|
Someone is attempting to deploy a commit to the s3dfx-cyber's projects Team on Vercel. A member of the Team first needs to authorize it. |
💬 Faster Reviews & AssignmentsHi @desireddymohithreddy0925, for faster coordination and smoother communication, consider joining our Discord community: Useful Channels
|
|
👋 Thanks for opening a PR, @desireddymohithreddy0925!Your PR has entered the 🚦 PR Review Pipeline.
🔄 Review Flow
A pipeline status comment may appear automatically as your PR progresses. ✅ Contributor Checklist
|
github-actions
Bot
added
dco-missing
enhancement
✅ DCO Sign-off VerifiedHi @desireddymohithreddy0925 👋 All commits in this PR contain valid Thank you for following the DCO requirements 🚀 |
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR broadens workflow program detection, centralizes third-party CDN fallback loading, and updates theme initialization to honor saved preferences or system theme settings, including system-theme change handling. ChangesWorkflow PR-body parsing
Frontend loading and theme sync
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant index_html
participant jsDelivr
participant unpkg
Browser->>index_html: request page
index_html->>jsDelivr: load dependency
jsDelivr-->>index_html: load response
index_html->>index_html: validate global
alt validation or loading fails
index_html->>unpkg: load fallback dependency
unpkg-->>index_html: fallback response
end
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
🚦 PR Review Pipeline
Last updated: Thu, 25 Jun 2026 19:56:44 GMT |
🤖 TENET Agent Review📋 SummaryThis pull request significantly refactors the loading mechanism for third-party JavaScript libraries ( 🔐 Security FindingsNo security issues found. The PR correctly implements Subresource Integrity (SRI) using 🧹 Code Quality
✅ What's Done Well
📝 Overall Verdict[APPROVE] - The PR significantly improves code quality and robustness without introducing security vulnerabilities, and enhances user experience. Review powered by TENET Agent 🛡️ | Triggered automatically on PR #1960 |
Fixes SonarCloud warnings and DCO Signed-off-by: DESIREDDY MOHITH REDDY <desireddymohithreddy@MRD-2.local>
b3fa500 to
d21bd42
Compare
github-actions
Bot
added
dco-verified
stage-1-approved
and removed
dco-missing
needs-stage-1-fixes
labels
🤖 TENET Agent Review📋 SummaryThe pull request refactors the loading of third-party JavaScript libraries ( 🔐 Security FindingsNo security issues found. The PR correctly implements Subresource Integrity (SRI) for all CDN-loaded scripts, mitigating risks associated with CDN compromise. The changes are primarily focused on frontend dependency management and workflow improvements, not directly impacting the core LLM security middleware logic. 🧹 Code Quality
✅ What's Done Well
📝 Overall Verdict[APPROVE] - Improves code quality, maintainability, and user experience without introducing security vulnerabilities. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
index.html (1)
110-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
globalThis.matchMedia?.(...)here.This clears the Sonar warning and avoids calling
matchMediatwice.- } else if (window.matchMedia && window.matchMedia('(prefers-color-scheme: dark)').matches) { + } else if (globalThis.matchMedia?.('(prefers-color-scheme: dark)')?.matches) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@index.html` at line 110, The dark-mode check currently calls window.matchMedia twice and triggers the Sonar warning. Update the conditional in the color-scheme detection logic to use globalThis.matchMedia?.(...) instead of window.matchMedia, and reuse that single result in the existing branch so the matchMedia lookup is only performed once.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@index.html`:
- Around line 80-95: The dependency loader in loadDependencies() returns before
the scripts are ready, so code that uses marked, DOMPurify, or html2pdf can run
too early. Update the loading flow to expose and await a shared readiness signal
such as globalThis.thirdPartyDependenciesReady before any app code depends on
these globals. Also make the fallback path run the same dep.check() validation
as the primary load, so both primary and fallback behave consistently inside the
dependencies.forEach/injectScript flow.
In `@src/js/app.js`:
- Around line 112-131: The theme initialization in app.js should treat
non-explicit values consistently with system mode so the media-query listener
keeps syncing after first load. Update the saved-theme handling around
localStorage.getItem('theme') and the matchMedia change listener to normalize
any stale value like system/auto to “no explicit preference,” and use globalThis
instead of window for the matchMedia checks and listener setup. Keep the logic
centered around the existing theme bootstrap and updateThemeIcon flow.
---
Nitpick comments:
In `@index.html`:
- Line 110: The dark-mode check currently calls window.matchMedia twice and
triggers the Sonar warning. Update the conditional in the color-scheme detection
logic to use globalThis.matchMedia?.(...) instead of window.matchMedia, and
reuse that single result in the existing branch so the matchMedia lookup is only
performed once.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1fb43438-462f-4c2f-a31b-63c844939494
📒 Files selected for processing (3)
.github/workflows/program-classification-validator.ymlindex.htmlsrc/js/app.js
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-06-15T18:15:28.688Z
Learnt from: arghya29
Repo: S3DFX-CYBER/GSoC-Org-Finder- PR: 1882
File: src/js/footer.js:33-33
Timestamp: 2026-06-15T18:15:28.688Z
Learning: In this repo’s JavaScript (e.g., footer.js), `globalThis` is intentionally preferred over `window` to keep code environment-agnostic and to satisfy SonarCloud static analysis. The project targets modern browsers (ES2021) with no transpilation, so `globalThis` is fully supported—do not flag `globalThis` usage as a browser compatibility concern or suggest replacing it with `window` during review.
Applied to files:
src/js/app.js
🪛 GitHub Check: SonarCloud Code Analysis
src/js/app.js
[warning] 125-125: Prefer globalThis over window.
[warning] 117-117: Prefer using an optional chain expression instead, as it's more concise and easier to read.
[warning] 124-124: Prefer globalThis over window.
[warning] 117-117: Prefer globalThis over window.
[warning] 117-117: Prefer globalThis over window.
index.html
[warning] 110-110: Prefer globalThis over window.
[warning] 110-110: Prefer globalThis over window.
🔇 Additional comments (1)
.github/workflows/program-classification-validator.yml (1)
90-112: LGTM!
KumarNirupam1
left a comment
KumarNirupam1
left a comment
There was a problem hiding this comment.
thanks for the refactor!
consolidating the third-party dependency loading makes index.html much cleaner and easier to maintain. i also like that the implementation adds sri support and improves theme handling. i only have one minor suggestion: consider validating dep.check() after the fallback script loads as well for extra robustness, but i don't see it as a blocker.
lgtm 👍
|
@desireddymohithreddy0925 merge conflicts pls resolve |
…n-fallbacks Signed-off-by: DESIREDDY MOHITH REDDY <desireddymohithreddy@MRD-4.local> # Conflicts: # .github/workflows/program-classification-validator.yml
5c95ba0
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
🤖 TENET Agent Review📋 SummaryThis pull request refactors the loading mechanism for third-party scripts ( 🔐 Security FindingsNo security issues found. The use of 🧹 Code Quality
✅ What's Done Well
📝 Overall Verdict[APPROVE] - This PR significantly improves code quality, reliability, and user experience without introducing security concerns. Review powered by TENET Agent 🛡️ | Triggered automatically on PR #1960 |
|
|
@S3DFX-CYBER done sir |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
.github/workflows/program-classification-validator.yml (4)
168-174: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winFix undefined variable reference.
The variable
detectedProgramsis undefined. The array initialized on line 161 is nameddetectedActualPrograms. Attempting to access.includes()on an undefined variable will throw aReferenceErrorand crash the workflow when a PR has multiple program markers.🐛 Proposed fix for the variable reference
- if (detectedPrograms.includes('gssoc') && detectedPrograms.includes('nsoc')) { - const nsocIndex = detectedPrograms.indexOf('nsoc'); + if (detectedActualPrograms.includes('gssoc') && detectedActualPrograms.includes('nsoc')) { + const nsocIndex = detectedActualPrograms.indexOf('nsoc'); if (nsocIndex > -1) { - detectedPrograms.splice(nsocIndex, 1); + detectedActualPrograms.splice(nsocIndex, 1); } core.warning('Dual-label program classification detected (gssoc + nsoc) — resolved to GSSoC priority'); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/program-classification-validator.yml around lines 168 - 174, Update the dual-label resolution block to use the initialized detectedActualPrograms array instead of the undefined detectedPrograms variable for the includes, indexOf, and splice operations, preserving the existing GSSoC-priority warning behavior.
277-277: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winDo not fail valid general contributions.
If a PR is a "General Contribution", the
detectedActualProgramsarray is empty, buthasGeneralContributionistrue. The current logic only checks if the array is empty, which incorrectly flags valid general contributions as missing a classification.🐛 Proposed fix to bypass the check for general contributions
- if (detectedActualPrograms.length === 0) { + if (detectedActualPrograms.length === 0 && !hasGeneralContribution) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/program-classification-validator.yml at line 277, Update the validation condition around detectedActualPrograms to require both an empty detectedActualPrograms array and a false hasGeneralContribution value before reporting a missing classification. Preserve the existing validation behavior for non-general contributions while allowing valid General Contribution PRs to pass.
30-31: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRemove the invalid step definition.
The step
Checkout repositorylacks ausesorrunkey, which makes it invalid YAML for a GitHub Actions workflow. This will causeactionlintto fail parsing and prevent the workflow from starting. Since no checkout is needed, the incomplete step should be removed entirely.🐛 Proposed fix to remove the invalid step
steps: - - name: Checkout repository - # No checkout/setup-node needed for actions/github-script-only workflow - name: Validate classification🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/program-classification-validator.yml around lines 30 - 31, Remove the incomplete “Checkout repository” step and its accompanying comment from the workflow, leaving the actions/github-script-only job without a checkout step or any step lacking a uses or run key.Source: Linters/SAST tools
411-412: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winPrevent
TypeErrorfor general contributions.For general contributions,
detectedActualProgramsis empty, meaningdetectedActualPrograms[0]evaluates toundefined. When execution later attempts to generate the success comment usingprogram.toUpperCase(), it will throw aTypeErrorand crash the script. Provide a default value to represent the general program.🐛 Proposed fix to handle the fallback program
- const program = - detectedActualPrograms[0]; + const program = detectedActualPrograms.length > 0 + ? detectedActualPrograms[0] + : 'general contribution';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/program-classification-validator.yml around lines 411 - 412, Update the program assignment in the classification validation flow so an empty detectedActualPrograms array uses a default value representing a general program instead of undefined. Ensure the value passed to the later success-comment generation remains a string safe for program.toUpperCase(), while preserving the detected program when one exists.
🧹 Nitpick comments (2)
.github/workflows/program-classification-validator.yml (1)
41-41: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse event payload for bot detection.
Relying on
context.actorto identify bots can be problematic because the actor context can be spoofed or differ during re-runs. It's safer to use the immutable event payload to check the sender.🛡️ Proposed fix to verify the event sender
- if (context.actor === 'github-actions[bot]') { + if (context.payload.sender.login === 'github-actions[bot]') {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/program-classification-validator.yml at line 41, Update the bot-detection condition in the workflow’s conditional block to use the immutable event payload sender identity rather than context.actor. Preserve the existing github-actions bot check while reading the sender from the event payload.index.html (1)
2787-2789: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove redundant inline style mutations.
The JavaScript applies hardcoded inline colors to the mobile language pills when their state changes. This is entirely redundant because the CSS already handles these active states using
.mobile-lang-pill.active(including necessary overrides for dark mode). Rely purely on toggling theactiveclass to maintain separation of concerns and prevent potential UI bugs.
index.html#L2787-L2789: Remove thebtn.style.borderColor,color, andbackgroundassignments.index.html#L2825-L2827: Remove thebtn.style.borderColor,color, andbackgroundassignments.index.html#L4578-L4580: Remove thebtn.style.borderColor,color, andbackgroundclear assignments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@index.html` around lines 2787 - 2789, Remove the redundant inline borderColor, color, and background mutations from the mobile language pill state updates at index.html lines 2787-2789 and 2825-2827, and remove the corresponding clearing assignments at lines 4578-4580. Keep the existing active-class toggling in the affected language-pill handlers so styling is provided solely by .mobile-lang-pill.active.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@index.html`:
- Line 110: Update loadDependencies() to return or expose a readiness promise
that resolves only after all injected CDN scripts finish loading, then await
that promise before downstream application code executes. Ensure consumers use
the readiness signal so no third-party dependency is accessed before loading
completes.
---
Outside diff comments:
In @.github/workflows/program-classification-validator.yml:
- Around line 168-174: Update the dual-label resolution block to use the
initialized detectedActualPrograms array instead of the undefined
detectedPrograms variable for the includes, indexOf, and splice operations,
preserving the existing GSSoC-priority warning behavior.
- Line 277: Update the validation condition around detectedActualPrograms to
require both an empty detectedActualPrograms array and a false
hasGeneralContribution value before reporting a missing classification. Preserve
the existing validation behavior for non-general contributions while allowing
valid General Contribution PRs to pass.
- Around line 30-31: Remove the incomplete “Checkout repository” step and its
accompanying comment from the workflow, leaving the actions/github-script-only
job without a checkout step or any step lacking a uses or run key.
- Around line 411-412: Update the program assignment in the classification
validation flow so an empty detectedActualPrograms array uses a default value
representing a general program instead of undefined. Ensure the value passed to
the later success-comment generation remains a string safe for
program.toUpperCase(), while preserving the detected program when one exists.
---
Nitpick comments:
In @.github/workflows/program-classification-validator.yml:
- Line 41: Update the bot-detection condition in the workflow’s conditional
block to use the immutable event payload sender identity rather than
context.actor. Preserve the existing github-actions bot check while reading the
sender from the event payload.
In `@index.html`:
- Around line 2787-2789: Remove the redundant inline borderColor, color, and
background mutations from the mobile language pill state updates at index.html
lines 2787-2789 and 2825-2827, and remove the corresponding clearing assignments
at lines 4578-4580. Keep the existing active-class toggling in the affected
language-pill handlers so styling is provided solely by
.mobile-lang-pill.active.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1a4a55ef-2027-4459-9c5b-5e751b6ab903
📒 Files selected for processing (2)
.github/workflows/program-classification-validator.ymlindex.html
📜 Review details
⚠️ CI failures not shown inline (1)
Commit Status: Vercel: Vercel
Conclusion: failure
Authorization required to deploy.
🧰 Additional context used
🪛 actionlint (1.7.12)
.github/workflows/program-classification-validator.yml
[error] 30-30: step must run script with "run" section or run action with "uses" section
(syntax-check)
🪛 ast-grep (0.44.1)
index.html
[warning] 2816-2816: Avoid assigning untrusted data to innerHTML/outerHTML or document.write
Context: mobileStrip.innerHTML = mainStrip ? mainStrip.innerHTML : ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(inner-outer-html)
[error] 3603-3603: React's useState should not be directly called
Context: setTimeout(() => { t.style.opacity = '0'; }, 2200)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(usestate-direct-usage)
🔇 Additional comments (1)
index.html (1)
2707-2747: 🎯 Functional Correctness | ⚡ Quick winImprove accessibility for the mobile dropdowns.
The mobile filter and language dropdown buttons lack
aria-expandedandaria-controlsattributes. Adding and dynamically updating these attributes ensures that screen reader users are informed of the panels' visibility states.♿ Proposed fixes for HTML and JS
First, update the trigger buttons in the HTML (around lines 1504 and 1535):
<!-- Filter button --> - <button id="mobileFilterBtn" onclick="toggleMobileFilterDropdown()" class="w-full flex... + <button id="mobileFilterBtn" aria-expanded="false" aria-controls="mobileFilterPanel" onclick="toggleMobileFilterDropdown()" class="w-full flex... <!-- Language button --> - <button id="mobileLangBtn" onclick="toggleMobileLangDropdown()" class="w-full flex... + <button id="mobileLangBtn" aria-expanded="false" aria-controls="mobileLangPanel" onclick="toggleMobileLangDropdown()" class="w-full flex...Next, update the toggle and click-outside functions to sync the
aria-expandedstate:globalThis.toggleMobileFilterDropdown = function() { const panel = document.getElementById('mobileFilterPanel'); const chevron = document.getElementById('mobileFilterChevron'); + const btn = document.getElementById('mobileFilterBtn'); if (!panel) return; const isOpen = !panel.classList.contains('hidden'); panel.classList.toggle('hidden', isOpen); if (chevron) chevron.style.transform = isOpen ? '' : 'rotate(180deg)'; + if (btn) btn.setAttribute('aria-expanded', !isOpen); // close lang panel if open document.getElementById('mobileLangPanel')?.classList.add('hidden'); const lc = document.getElementById('mobileLangChevron'); if (lc) lc.style.transform = ''; + document.getElementById('mobileLangBtn')?.setAttribute('aria-expanded', 'false'); }; globalThis.toggleMobileLangDropdown = function() { const panel = document.getElementById('mobileLangPanel'); const chevron = document.getElementById('mobileLangChevron'); + const btn = document.getElementById('mobileLangBtn'); if (!panel) return; const isOpen = !panel.classList.contains('hidden'); panel.classList.toggle('hidden', isOpen); if (chevron) chevron.style.transform = isOpen ? '' : 'rotate(180deg)'; + if (btn) btn.setAttribute('aria-expanded', !isOpen); // close filter panel if open document.getElementById('mobileFilterPanel')?.classList.add('hidden'); const fc = document.getElementById('mobileFilterChevron'); if (fc) fc.style.transform = ''; + document.getElementById('mobileFilterBtn')?.setAttribute('aria-expanded', 'false'); }; // Close mobile dropdowns when clicking outside document.addEventListener('click', function(e) { const filterWrap = document.getElementById('mobileFilterDropdownWrap'); const langWrap = document.getElementById('mobileLangDropdownWrap'); if (filterWrap && !filterWrap.contains(e.target)) { document.getElementById('mobileFilterPanel')?.classList.add('hidden'); const fc = document.getElementById('mobileFilterChevron'); if (fc) fc.style.transform = ''; + document.getElementById('mobileFilterBtn')?.setAttribute('aria-expanded', 'false'); } if (langWrap && !langWrap.contains(e.target)) { document.getElementById('mobileLangPanel')?.classList.add('hidden'); const lc = document.getElementById('mobileLangChevron'); if (lc) lc.style.transform = ''; + document.getElementById('mobileLangBtn')?.setAttribute('aria-expanded', 'false'); } });
3 participants
Resolves #1950
✨ Feature Description
Refactored and streamlined how third-party scripts (
marked,DOMPurify, andhtml2pdf) load and handle fallbacks inindex.html.🤔 Problem It Solves
Previously,
index.htmlloaded scripts via a primary CDN and immediately followed up with inline script blocks to inject fallbacks if undefined. This cluttered the<head>, caused potential race conditions, and made dependency management difficult.💡 Proposed Solution
Created a robust, asynchronous script-loading utility function to handle CDN fallbacks cleanly. Replaced the repeated inline fallback pattern with an array of dependencies and their primary/fallback URLs, loading them safely and efficiently.
✅ Acceptance Criteria Met
<head>tag is significantly cleaner and free of redundant inline fallback scripts.markedorDOMPurifyusage in the application.