[772] fix: prevent content flash when using case studies filtering#213
[772] fix: prevent content flash when using case studies filtering#213
Conversation
WalkthroughAdds a loading/reveal system across templates, styles, and frontend logic. HTML: add "loading" class and aria-busy to breadcrumb, sf-container, and the case-studies main content, make Barba container namespace dynamic, and add noscript CSS fallbacks for visibility when JS is disabled. SCSS: introduce .loading/.loaded rules with specialized breadcrumb and container variants and remove gap from .cs_content. JS: add revealLoaded(), call it on DOMContentLoaded and in Barba's after hook, consolidate initFilterModal() into that flow, and simplify case-studies detection. Also updates a .cursor submodule pointer. No public API/signature changes. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
🔍 Vulnerabilities of
|
| digest | sha256:a56c04b7a020726791ca23314a98f742eff043614f411164d6c3365aa6a42b64 |
| vulnerabilities |     |
| platform | linux/amd64 |
| size | 291 MB |
| packages | 984 |
📦 Base Image node:23-alpine
| also known as |
|
| digest | sha256:b9d38d589853406ff0d4364f21969840c3e0397087643aef8eede40edbb6c7cd |
| vulnerabilities |     |
Description
| ||||||||||||
Description
| ||||||||||||
Description
| ||||||||||||
Description
| ||||||||||||
Description
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
website/modules/asset/ui/src/scss/_base.scss(1 hunks)website/modules/case-studies-page/views/index.html(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: lint
- GitHub Check: e2e-tests
- GitHub Check: unit-tests
- GitHub Check: security-scan
🔇 Additional comments (1)
website/modules/case-studies-page/views/index.html (1)
300-310: ID Uniqueness Verified – No Duplicates FoundA repository-wide ripgrep search for
page-main_contentreturned only the instance in
website/modules/case-studies-page/views/index.html, confirming the ID is unique.
No further changes are needed.
- Add breadcrumbs functionality with hidden/visible classes - Implement DOM content loading script for breadcrumbs - Update layout.html to handle breadcrumbs visibility - Modify case-studies-page to control breadcrumbs display - Update CSS for better content visibility control
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
website/views/layout.html (1)
47-47: Remove empty script tagThis empty tag is noise and may trigger CSP linters. Remove it.
- <script type="text/javascript"></script>website/views/fragments/fragments.html (1)
36-61: Add a no-JS fallback to prevent permanently hidden breadcrumbs when scripts don’t runWith CSS-based hiding and inline scripts, users with JS disabled or strict CSP will never see breadcrumbs. Provide a graceful fallback.
Option A (preferred): Use a no-js/js class on and CSS like:
- HTML: and JS switches it to js on DOMContentLoaded.
- CSS: .js .breadcrumb.loading { visibility: hidden; opacity: 0; } so in no-js, breadcrumbs remain visible.
Option B (local fallback): Add a noscript override near the breadcrumbs:
<noscript><style>.breadcrumb{visibility: visible !important; opacity: 1 !important;}</style></noscript>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (5)
website/modules/asset/ui/src/scss/_base.scss(1 hunks)website/modules/asset/ui/src/scss/_cases.scss(0 hunks)website/modules/case-studies-page/views/index.html(2 hunks)website/views/fragments/fragments.html(1 hunks)website/views/layout.html(2 hunks)
💤 Files with no reviewable changes (1)
- website/modules/asset/ui/src/scss/_cases.scss
🚧 Files skipped from review as they are similar to previous changes (2)
- website/modules/asset/ui/src/scss/_base.scss
- website/modules/case-studies-page/views/index.html
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e-tests
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: security-scan
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
website/views/layout.html (1)
36-36: Align with PR objective: use loading/loaded semantics (not hidden/visible) and set aria-busyPer PR goals, this should start as a loading state and later switch to loaded. Also add aria-busy to signal assistive tech while content is being prepared.
- <div class="sf-container hidden"> + <div class="sf-container loading" aria-busy="true">Follow-up:
- Ensure corresponding CSS for .loading/.loaded exists and matches the intended visibility behavior.
- If this layout applies to all pages, confirm that globally hiding the container until JS runs won’t cause a blank page flash site-wide; you may want to scope this to the case studies pages only.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
website/views/layout.html(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e-tests
- GitHub Check: lint
- GitHub Check: security-scan
- GitHub Check: unit-tests
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
website/views/layout.html (1)
50-65: Refactor reveal logic: target all matches and avoid duplication; clear aria-busy conditionallyUse
querySelectorAllto handle multiple breadcrumbs/containers, and centralize this logic in a shared asset (and call it after Barba transitions). This was flagged previously and remains applicable.Apply this minimal diff here:
- document.addEventListener('DOMContentLoaded', function () { - const breadcrumbs = document.querySelector('.breadcrumb'); - const container = document.querySelector('.sf-container'); - if (breadcrumbs) { - breadcrumbs.classList.remove('loading'); - breadcrumbs.classList.add('loaded'); - breadcrumbs.setAttribute('aria-busy', 'false'); - } - if (container) { - container.classList.remove('loading'); - container.classList.add('loaded'); - container.setAttribute('aria-busy', 'false'); - } - }); + document.addEventListener('DOMContentLoaded', function () { + document.querySelectorAll('.breadcrumb.loading, .sf-container.loading') + .forEach(function (el) { + el.classList.remove('loading'); + el.classList.add('loaded'); + if (el.hasAttribute('aria-busy')) { + el.setAttribute('aria-busy', 'false'); + } + }); + });Follow-up (recommended):
- Extract into a shared JS module and invoke on DOMContentLoaded and Barba hooks (e.g.,
barba.hooks.after()), so transitions don’t leave elements in the loading state.
🧹 Nitpick comments (2)
website/views/layout.html (2)
40-45: Add a no-JS fallback so content isn’t stuck in “loading” if JS is disabledIf
.loadingstyles hide content, users without JS will never see it. Consider the commonno-js/jspattern (defaultno-json , then flip tojsearly), and scope the hiding rule to.js .loadingso non-JS environments still show content.Happy to add:
- a tiny inline head script to set
class="js"on<html>; and- a CSS tweak:
.js .loading { visibility: hidden; }while leaving.loadingvisible by default.
48-48: Remove empty script tagThis empty
<script type="text/javascript"></script>is unnecessary and can confuse linters or readers.Apply this diff:
- <script type="text/javascript"></script>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (5)
.cursor/rules/common(1 hunks)website/modules/asset/ui/src/scss/_base.scss(1 hunks)website/modules/case-studies-page/views/index.html(2 hunks)website/views/fragments/fragments.html(1 hunks)website/views/layout.html(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .cursor/rules/common
🚧 Files skipped from review as they are similar to previous changes (3)
- website/modules/asset/ui/src/scss/_base.scss
- website/views/fragments/fragments.html
- website/modules/case-studies-page/views/index.html
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: unit-tests
- GitHub Check: e2e-tests
- GitHub Check: lint
- GitHub Check: security-scan
🔇 Additional comments (2)
website/views/layout.html (2)
29-37: Confirm intent: disable Barba container when user is authenticated
data-barba="container"is conditionally applied only whennot data.user. If authenticated pages should also participate in transitions, this condition will disable them and could lead to inconsistent behavior. If disabling Barba for logged-in users is deliberate (e.g., for editing UI), ignore; otherwise, consider always rendering the container.Would you like me to propose a guarded pattern (e.g., a site-level flag) instead of coupling it to
data.user?
40-45: Good adoption of loading state and aria-busy on the main content containerThis aligns with the PR objective and improves a11y during async fetches. Looks solid.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
website/views/layout.html (2)
8-8: Remove literal and tags from this block (invalid HTML structure)This template extends an outer layout; adding a wrapper inside a block injects invalid markup (body content within head) and breaks the document structure. This was flagged earlier and still applies.
Apply this diff:
-<head> ... -</head>Also applies to: 73-73
48-60: Switch reveal logic to loading→loaded (current hidden→visible is inconsistent and breaks the UX)The template initializes the container with class="loading" (Line 40), but this script targets ".hidden" and toggles to ".visible". As a result, elements remain in the loading state and never become visible as "loaded". Toggle loading→loaded and keep aria-busy in sync.
Apply this diff:
- document.addEventListener('DOMContentLoaded', function () { - document - .querySelectorAll('.breadcrumb.hidden, .sf-container.hidden') - .forEach(function (el) { - el.classList.remove('hidden'); - el.classList.add('visible'); - if (el.hasAttribute('aria-busy')) { - el.setAttribute('aria-busy', 'false'); - } - }); - }); + document.addEventListener('DOMContentLoaded', function () { + document + .querySelectorAll('.breadcrumb.loading, .sf-container.loading') + .forEach(function (el) { + el.classList.remove('loading'); + el.classList.add('loaded'); + if (el.hasAttribute('aria-busy')) { + el.setAttribute('aria-busy', 'false'); + } + }); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (3)
website/modules/asset/ui/src/scss/_base.scss(1 hunks)website/modules/case-studies-page/views/index.html(2 hunks)website/views/layout.html(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- website/modules/asset/ui/src/scss/_base.scss
- website/modules/case-studies-page/views/index.html
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: lint
- GitHub Check: e2e-tests
- GitHub Check: unit-tests
- GitHub Check: security-scan
🔇 Additional comments (2)
website/views/layout.html (2)
20-23: Barba attribute placement: move namespace to the container and avoid hardcodingBarba expects data-barba="wrapper" on the wrapper and data-barba="container" + data-barba-namespace on the page container. Here, namespace is on the wrapper and hardcoded as "home". Suggest moving namespace to the main element when it acts as the container, and avoid a hardcoded value.
Apply this minimal, consistent change:
- <div - class="bp-wrapper relative" - data-barba="wrapper" - data-barba-namespace="home" - > + <div + class="bp-wrapper relative" + data-barba="wrapper" + >And add the namespace alongside the conditional container attribute:
id="main" {% if not data.user %} - data-barba="container" + data-barba="container" + data-barba-namespace="home" {% endif %}Follow-up:
- If you have a per-page namespace (preferred), bind it dynamically (e.g., based on data.page or similar) instead of "home".
- Verify Barba transitions still work as expected after the move.
Also applies to: 29-37
40-45: Confirm global CSS for .loading/.loaded existsThe container starts with class="loading" and relies on .loaded for reveal. Ensure the global stylesheet defines .loading/.loaded behavior (and that legacy .hidden/.visible isn’t relied upon here).
If needed, I can scan the repo and generate a minimal SCSS snippet to define .loading/.loaded to prevent flashes or stuck states. Would you like me to proceed?
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
website/views/layout.html (1)
50-62: Centralize and reuse reveal logic; also run after Barba transitionsInline duplication makes it easy to miss routes or transitions. Move this to a shared UI script and call it on DOMContentLoaded and Barba hooks (e.g., barba.hooks.after).
Example (outside this file, in your UI bundle):
export function revealLoaded() { document.querySelectorAll('.breadcrumb.loading, [data-reveal="dom"].loading') .forEach(el => { el.classList.remove('loading'); el.classList.add('loaded'); if (el.hasAttribute('aria-busy')) el.setAttribute('aria-busy', 'false'); }); } document.addEventListener('DOMContentLoaded', revealLoaded); if (window.barba && window.barba.hooks) { window.barba.hooks.after(() => revealLoaded()); }
🧹 Nitpick comments (2)
website/views/layout.html (2)
50-50: Nit: drop redundant type="text/javascript" in HTML5HTML5 defaults to JavaScript. Removing this reduces noise.
Apply this diff:
- <script type="text/javascript"> + <script>
64-71: Noscript fallback should force display in case .loading uses display:noneIf
.loadingsetsdisplay: none, the current rules won’t make content visible. Add display: block to guarantee visibility without JS.Apply this diff:
.breadcrumb.loading, .sf-container.loading { + display: block !important; opacity: 1 !important; overflow: visible !important; visibility: visible !important; }Optional: If
.loading/.loadedare defined differently (e.g., opacity/visibility only), confirm whetherdisplayis needed. I can scan SCSS and adjust accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
website/views/layout.html(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: security-scan
- GitHub Check: lint
- GitHub Check: e2e-tests
- GitHub Check: unit-tests
🔇 Additional comments (1)
website/views/layout.html (1)
37-39: Breadcrumbs initial "loading" + aria-busy confirmed — no change requiredConfirmed:
- website/views/fragments/fragments.html — renders:
- website/views/layout.html — renders a wrapper
and includes the DOMContentLoaded script that queries '.breadcrumb.loading, .sf-container.loading', swaps loading → loaded and sets aria-busy="false".- website/modules/asset/ui/src/scss/_base.scss — contains .loading and .loaded styles.
No fixes needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
website/modules/asset/ui/src/scss/_animation.scss (2)
59-76: Good pattern for reveal states; consider reduced-motion supportThe base
.loading/.loadedpattern looks solid and will prevent FOUC. To respect users who prefer reduced motion, disable the transform/transition whenprefers-reduced-motion: reduceis set.Add this (outside the changed block) to honor OS a11y settings:
@media (prefers-reduced-motion: reduce) { .loading, .breadcrumb.loading, .sf-container.loading { transform: none; transition: none; } .loaded, .breadcrumb.loaded, .sf-container.loaded { transition: none; } }
77-87: Visibility doesn’t transition; remove from transition list
visibilityis a discrete property and doesn’t animate. Keeping it in the transition list is misleading and can confuse future maintainers. Rely on opacity + transform only..breadcrumb.loading, .sf-container.loading { opacity: 0 !important; visibility: hidden !important; transform: translateY(15px); transition: - opacity 0.5s ease-out, - visibility 0.5s ease-out, - transform 0.5s ease-out; + opacity 0.5s ease-out, + transform 0.5s ease-out; }website/modules/asset/ui/src/index.js (3)
100-103: Avoid multiple initializations of initCaseStudiesFilterHandlerThis handler is invoked in three places: inside initializeAllComponents(), inside initBarbaPageTransitions’ onReady, and again in export default’s onReady. Unless it’s strictly idempotent, this risks duplicate event listeners/state.
Suggested consolidation:
- Keep the call within initializeAllComponents() (already run on initial load and in Barba enter).
- Remove the two extra calls below.
apos.util.onReady(() => { - initCaseStudiesFilterHandler(); … });-apos.util.onReady(() => { - initCaseStudiesFilterHandler(); -});Please confirm the handler is idempotent or guarded internally; otherwise duplicate listeners may accumulate.
Also applies to: 109-110, 276-278
251-265: FilterModal re-instantiation across navigations — verify teardownYou instantiate a new FilterModal on DOMContentLoaded and again in Barba after. If FilterModal binds any document/window-level listeners, these can stack unless the previous instance is destroyed.
If the class exposes a destroy/teardown, clear it before re-instantiating:
window.caseStudiesFilterModal = new FilterModal({Becomes:
+ if (window.caseStudiesFilterModal?.destroy) { + window.caseStudiesFilterModal.destroy(); + } window.caseStudiesFilterModal = new FilterModal({If there’s no destroy, consider adding one to the class or using delegated event listeners to avoid per-instance globals.
Also applies to: 267-267, 199-202
287-293: Condition reads clearer; consider URLSearchParams for robustnessThe simplified condition is readable. For robustness (e.g., encoded params, reordering), using URLSearchParams is a bit safer and intent-revealing.
Example:
const params = new URLSearchParams(search); const hasFilter = params.has('industry') || params.has('stack') || params.has('caseStudyType') || hash.includes('filter'); if (pathname.includes('/cases') && hasFilter) { … }website/modules/case-studies-page/views/index.html (1)
305-313: noscript fallback should also reset transform
.loadingappliestransform: translateY(20px), so without JS the content will remain offset. Add a reset for transform to ensure proper layout for no-JS users.<noscript> <style> #page-main_content { opacity: 1 !important; overflow: visible !important; visibility: visible !important; + transform: none !important; } </style> </noscript>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
website/modules/asset/ui/src/index.js(4 hunks)website/modules/asset/ui/src/scss/_animation.scss(1 hunks)website/modules/case-studies-page/views/index.html(2 hunks)website/views/layout.html(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- website/views/layout.html
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-14T09:44:56.398Z
Learnt from: yuramax
PR: speedandfunction/website#84
File: website/modules/case-studies-page/views/index.html:87-90
Timestamp: 2025-05-14T09:44:56.398Z
Learning: For the Case Study cards in website/modules/case-studies-page/views/index.html, the "type" and "industry" fields are temporarily hard-coded with values "Mobile Development" and "Manufacturing" as placeholders. These will be replaced with dynamic data ({{ article.type }} and {{ article.industry }}) in a future task.
Applied to files:
website/modules/case-studies-page/views/index.html
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e-tests
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: security-scan
🔇 Additional comments (4)
website/modules/asset/ui/src/scss/_animation.scss (1)
89-94: LGTM: loaded state overrides are clear and scopedUsing specific selectors with
!importanthere makes sense given existing breadcrumb/container styles. No issues.website/modules/asset/ui/src/index.js (2)
15-27: Simple, idempotent reveal — good implementationThe selector targets are precise, and flipping aria-busy to false is a good a11y touch. Idempotent by design since it only selects
.loading.
198-202: Hooking revealLoaded into Barba after is the right placeThis ensures the newly replaced containers become visible promptly after navigation. Nicely done.
website/modules/case-studies-page/views/index.html (1)
9-15: A11y-friendly loading wrapper — good alignment with revealLoadedAdding
loading+aria-busy="true"on#page-main_contentaligns with the new reveal flow and communicates state to AT. Looks good.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
website/modules/asset/ui/src/index.js (2)
274-279: Addresses DOMContentLoaded race for revealLoadedThis readyState guard ensures revealLoaded runs even if the script loads after DOMContentLoaded. Nice follow-through.
267-267: Ensure initFilterModal runs even if DOMContentLoaded already firedIf this script loads after DOMContentLoaded, the listener won’t fire and the modal won’t initialize on first load (before any Barba transitions). Guard with readyState.
-document.addEventListener('DOMContentLoaded', initFilterModal); +if (document.readyState === 'loading') { + document.addEventListener('DOMContentLoaded', initFilterModal); +} else { + initFilterModal(); +}
🧹 Nitpick comments (1)
website/modules/asset/ui/src/index.js (1)
291-297: Use URLSearchParams and stricter path check to avoid false positivesincludes('/cases') can match “/showcases”, and scanning raw search can mis-detect. Prefer URLSearchParams and a stricter pathname check.
- const { pathname, search, hash } = window.location; - if ( - pathname.includes('/cases') && - (search.includes('industry') || - search.includes('stack') || - search.includes('caseStudyType') || - hash.includes('filter')) - ) { + const { pathname, search, hash } = window.location; + const params = new URLSearchParams(search); + if ( + (pathname === '/cases' || pathname.startsWith('/cases/')) && + (params.has('industry') || + params.has('stack') || + params.has('caseStudyType') || + hash === '#filter' || + hash.startsWith('#filter=')) + ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
website/modules/asset/ui/src/index.js(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: unit-tests
- GitHub Check: e2e-tests
- GitHub Check: lint
- GitHub Check: security-scan
🔇 Additional comments (2)
website/modules/asset/ui/src/index.js (2)
15-27: Solid, idempotent revealLoaded implementationToggles loading→loaded states cleanly and resets aria-busy to false. This should prevent flashes while remaining a11y-friendly.
198-201: Good placement: reveal before initializing the modalCalling revealLoaded() in Barba’s after hook (before initFilterModal) ensures the UI is visible when the modal is bound, minimizing flash.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
website/modules/asset/ui/src/index.js (1)
270-275: Good fix: revealLoaded now runs even if the script loads after DOMContentLoadedThis addresses the race noted earlier so content doesn’t remain hidden.
🧹 Nitpick comments (2)
website/modules/asset/ui/src/index.js (2)
15-27: Nit: use classList.replace for clarity and fewer DOM mutationsSlightly cleaner and avoids two separate classList ops.
- el.classList.remove('loading'); - el.classList.add('loaded'); + el.classList.replace('loading', 'loaded');
286-292: Prefer URLSearchParams over substring checks to avoid false positivesSubstring checks (e.g., search.includes('industry')) can misfire on similarly named params. Using URLSearchParams keeps it precise.
- if ( - pathname.includes('/cases') && - (search.includes('industry') || - search.includes('stack') || - search.includes('caseStudyType') || - hash.includes('filter')) - ) { + if ( + pathname.includes('/cases') && + (new URLSearchParams(search).has('industry') || + new URLSearchParams(search).has('stack') || + new URLSearchParams(search).has('caseStudyType') || + hash.includes('filter')) + ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
website/modules/asset/ui/src/index.js(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e-tests
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: security-scan
|
3 participants
Uh oh!
There was an error while loading. Please reload this page.