refactor: replace window with globalThis #2507
Conversation
Summary by CodeRabbit
WalkthroughReplaces environment-specific Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
frontend/src/components/MultiSearch.tsx (1)
103-103: Incomplete migration:window.open()should beglobalThis.open().Line 103 still uses
window.open()which is inconsistent with the PR objective to replace allwindowreferences withglobalThis.Apply this diff:
- window.open((suggestion as Event).url, '_blank') + globalThis.open((suggestion as Event).url, '_blank')frontend/src/utils/helpers/githubHeatmap.ts (1)
171-175: Incomplete migration causes logic error.Line 171 checks for
globalThisbut Line 174 still useswindow.devicePixelRatio. This is inconsistent and can cause reference errors in environments wherewindowis not defined.Apply this diff:
function getPixelRatio() { if (typeof globalThis === 'undefined') { return 1 } - return window.devicePixelRatio || 1 + return globalThis.devicePixelRatio || 1 }frontend/src/utils/scrollToAnchor.ts (1)
13-17: Incomplete migration:window.scrollYandwindow.scrollTo()should useglobalThis.Lines 13-14 still use
window.scrollYandwindow.scrollTo(), which is inconsistent with the PR objective to replace allwindowreferences withglobalThis.Apply this diff:
- const y = element.getBoundingClientRect().top + window.scrollY + yOffset - window.scrollTo({ + const y = element.getBoundingClientRect().top + globalThis.scrollY + yOffset + globalThis.scrollTo({ top: y, behavior: 'smooth', })frontend/src/components/Header.tsx (1)
27-55: Inconsistent mixing of window and globalThis in the same useEffect.This useEffect mixes
window(lines 29, 48, 52) andglobalThis(lines 49, 53) for similar operations within the same lifecycle hook. This creates an inconsistent pattern that diverges from the PR's stated objective to "replace all occurrences" of window with globalThis.For consistency with the PR objectives and to maintain a uniform pattern, consider completing the migration:
useEffect(() => { const handleResize = () => { - if (window.innerWidth >= desktopViewMinWidth) { + if (globalThis.innerWidth >= desktopViewMinWidth) { setMobileMenuOpen(false) } } const handleOutsideClick = (event: Event) => { const navbar = document.getElementById('navbar-sticky') const sidebar = document.querySelector('.fixed.inset-y-0') if ( mobileMenuOpen && navbar && !navbar.contains(event.target as Node) && sidebar && !sidebar.contains(event.target as Node) ) { setMobileMenuOpen(false) } } - window.addEventListener('resize', handleResize) + globalThis.addEventListener('resize', handleResize) globalThis.addEventListener('click', handleOutsideClick) return () => { - window.removeEventListener('resize', handleResize) + globalThis.removeEventListener('resize', handleResize) globalThis.removeEventListener('click', handleOutsideClick) } }, [mobileMenuOpen])frontend/src/components/SingleModuleCard.tsx (1)
48-51: Critical: Incomplete migration on line 50.Line 50 still uses
window.location.pathnameand should be updated toglobalThis.location.pathnamefor consistency with the rest of this file and the PR objectives.Apply this diff to complete the migration:
const handleCreate = () => { setDropdownOpen(false) - router.push(`${window.location.pathname}/modules/create`) + router.push(`${globalThis.location.pathname}/modules/create`) }frontend/src/app/my/mentorship/page.tsx (1)
119-122: Critical: Incomplete migration on line 121.Line 121 still uses
window.scrollToand should be updated toglobalThis.scrollTofor consistency with the rest of this PR.Apply this diff to complete the migration:
onPageChange={(p) => { setPage(p) - window.scrollTo({ top: 0, behavior: 'smooth' }) + globalThis.scrollTo({ top: 0, behavior: 'smooth' }) }}
🧹 Nitpick comments (1)
frontend/__tests__/unit/components/ScrollToTop.test.tsx (1)
6-6: Consider completing the globalThis migration in this file.Lines 6 and 48 still reference
window.scrollTo, which is inconsistent with the globalThis migration applied to other parts of this test file (scrollY, innerHeight, dispatchEvent).For consistency, consider applying this change:
beforeEach(() => { - window.scrollTo = jest.fn() + globalThis.scrollTo = jest.fn() Object.defineProperty(globalThis, 'scrollY', { value: 0, writable: true }) Object.defineProperty(globalThis, 'innerHeight', { value: 1000, writable: true }) })And update the assertion:
fireEvent.click(button) - expect(window.scrollTo).toHaveBeenCalledWith({ top: 0, behavior: 'smooth' }) + expect(globalThis.scrollTo).toHaveBeenCalledWith({ top: 0, behavior: 'smooth' }) })Also applies to: 48-48
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
frontend/__tests__/unit/components/AnchorTitle.test.tsx(18 hunks)frontend/__tests__/unit/components/AnimatedCounter.test.tsx(1 hunks)frontend/__tests__/unit/components/MultiSearch.test.tsx(1 hunks)frontend/__tests__/unit/components/ScrollToTop.test.tsx(3 hunks)frontend/__tests__/unit/components/Search.test.tsx(3 hunks)frontend/__tests__/unit/pages/Header.test.tsx(3 hunks)frontend/jest.setup.ts(5 hunks)frontend/src/app/mentorship/programs/[programKey]/page.tsx(1 hunks)frontend/src/app/my/mentorship/page.tsx(1 hunks)frontend/src/app/my/mentorship/programs/[programKey]/page.tsx(1 hunks)frontend/src/components/AnchorTitle.tsx(1 hunks)frontend/src/components/CardDetailsPage.tsx(1 hunks)frontend/src/components/ChapterMap.tsx(1 hunks)frontend/src/components/Header.tsx(1 hunks)frontend/src/components/ModuleCard.tsx(1 hunks)frontend/src/components/MultiSearch.tsx(1 hunks)frontend/src/components/ProgramActions.tsx(1 hunks)frontend/src/components/Search.tsx(1 hunks)frontend/src/components/SingleModuleCard.tsx(2 hunks)frontend/src/hooks/useIsMobile.ts(1 hunks)frontend/src/server/fetchMetricsPDF.ts(1 hunks)frontend/src/utils/helpers/githubHeatmap.ts(1 hunks)frontend/src/utils/scrollToAnchor.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
PR: OWASP/Nest#2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS is actively used in frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx for program editing functionality and cannot be removed. It serves a different purpose than GET_PROGRAM_ADMIN_DETAILS, providing comprehensive program information needed for editing.
Applied to files:
frontend/src/app/mentorship/programs/[programKey]/page.tsx
🧬 Code graph analysis (1)
frontend/src/hooks/useIsMobile.ts (1)
frontend/src/utils/constants.ts (1)
desktopViewMinWidth(119-119)
🔇 Additional comments (52)
frontend/src/components/MultiSearch.tsx (1)
51-51: LGTM!The change from
window.location.pathnametoglobalThis.location.pathnameimproves cross-environment compatibility.frontend/__tests__/unit/components/MultiSearch.test.tsx (1)
42-45: LGTM!The mock setup correctly uses
globalThis.openinstead ofwindow.open, aligning with the environment-agnostic refactoring objective.frontend/src/components/Search.tsx (1)
43-43: LGTM!The change to
globalThis.location.pathnameensures the GTM tracking works correctly in SSR and non-browser environments.frontend/__tests__/unit/components/Search.test.tsx (3)
188-189: LGTM!Test assertions correctly updated to expect
globalThis.location.pathnameinstead ofwindow.location.pathname.
206-207: LGTM!Consistent with the refactoring to use
globalThis.location.pathname.
217-218: LGTM!Test expectations properly aligned with the globalThis migration.
frontend/src/components/ModuleCard.tsx (1)
73-73: LGTM!The change to
globalThis.location.pathnameensures the navigation path construction works correctly in all environments.frontend/src/server/fetchMetricsPDF.ts (2)
24-24: LGTM!Using
globalThis.URL.createObjectURLensures compatibility across environments.
31-31: LGTM!Consistent use of
globalThis.URL.revokeObjectURLcompletes the migration for URL APIs.frontend/src/utils/scrollToAnchor.ts (1)
29-29: LGTM!The change to
globalThis.history.pushStateimproves environment compatibility.frontend/src/components/ChapterMap.tsx (1)
74-74: LGTM! Correct migration to globalThis.The change from
window.location.hreftoglobalThis.location.hrefaligns with ES2020 standards and improves environment compatibility.frontend/src/components/ProgramActions.tsx (2)
25-25: LGTM! Correct globalThis usage for pathname access.The migration from
window.location.pathnametoglobalThis.location.pathnameimproves compatibility across different JavaScript environments.
28-28: LGTM! Consistent pathname access pattern.Correctly uses
globalThis.location.pathnameconsistent with the migration objectives.frontend/__tests__/unit/components/ScrollToTop.test.tsx (3)
7-8: LGTM! Correct test environment setup.Properly uses
globalThisfor scrollY and innerHeight properties, ensuring environment-agnostic test setup.
27-28: LGTM! Consistent globalThis usage for scroll simulation.Correctly updates scrollY via globalThis and dispatches the scroll event on globalThis.
40-41: LGTM! Proper event dispatch pattern.Correctly uses globalThis for both property access and event dispatching.
frontend/__tests__/unit/pages/Header.test.tsx (3)
193-197: LGTM! Correct test environment setup.Properly uses
globalThis.innerWidthfor test setup, aligning with the migration objectives.
490-490: LGTM! Consistent event dispatching.Correctly dispatches the resize event on
globalThis.
688-692: LGTM! Proper responsive test setup.Uses
globalThis.innerWidthconsistently for simulating mobile viewport.frontend/src/components/CardDetailsPage.tsx (1)
94-94: LGTM! Correct pathname access pattern.The migration to
globalThis.location.pathnameis consistent with the PR objectives and maintains the same functionality for client-side navigation.frontend/src/app/my/mentorship/programs/[programKey]/page.tsx (1)
104-104: LGTM! Consistent URL manipulation pattern.Correctly uses
globalThis.location.pathnamefor obtaining the current path when cleaning query parameters. This aligns with the universal compatibility objectives.frontend/__tests__/unit/components/AnimatedCounter.test.tsx (1)
176-176: LGTM! Correct spy target.The change to spy on
globalThis.requestAnimationFrameinstead ofwindow.requestAnimationFrameis correct and aligns with environment-agnostic testing practices.frontend/src/components/Header.tsx (2)
49-49: LGTM! Correct event listener attachment.The change to
globalThis.addEventListenerfor click events is correct.
53-53: LGTM! Consistent cleanup pattern.Correctly removes the click event listener from
globalThis.frontend/src/hooks/useIsMobile.ts (1)
9-11: LGTM! Consistent migration to globalThis.The matchMedia API access has been correctly updated to use
globalThisfor environment-agnostic compatibility.frontend/src/components/SingleModuleCard.tsx (2)
40-46: LGTM! Consistent migration to globalThis.The location pathname access has been correctly updated to use
globalThisin both navigation handlers.
75-77: LGTM! Consistent migration to globalThis.The Link href construction correctly uses
globalThis.location.pathname.frontend/src/app/mentorship/programs/[programKey]/page.tsx (1)
46-46: LGTM! Consistent migration to globalThis.The fallback pathname for router.replace correctly uses
globalThis.location.pathnamewhen query parameters are cleared.frontend/src/components/AnchorTitle.tsx (2)
25-30: LGTM! Consistent migration to globalThis.The location hash access in the initial scroll effect correctly uses
globalThis.location.hash.
32-41: LGTM! Consistent migration to globalThis.The popstate event handler correctly uses
globalThisfor both location hash access and event listener management.frontend/__tests__/unit/components/AnchorTitle.test.tsx (15)
75-75: LGTM! Consistent test cleanup using globalThis.The afterEach hook correctly uses
globalThis.historyandglobalThis.locationfor state cleanup.
159-171: LGTM! Consistent test mocking using globalThis.The test setup correctly mocks
globalThis.scrollTo,globalThis.history.pushState, andglobalThis.requestAnimationFrame.
234-243: LGTM! Consistent test mocking using globalThis.The useEffect test setup correctly uses
globalThisfor scrollTo and requestAnimationFrame mocks.
255-267: LGTM! Consistent test assertions using globalThis.The hash-based scroll test correctly sets
globalThis.location.hashand verifies scroll behavior.
269-297: LGTM! Consistent event testing using globalThis.The popstate event tests correctly use
globalThis.location.hash, fire events on globalThis, and verify event listener cleanup withglobalThis.removeEventListener.
350-359: LGTM! Consistent error handling test using globalThis.The edge case test correctly mocks
globalThis.scrollTowhen testing missing anchor elements.
383-399: LGTM! Consistent accessibility test using globalThis.The focus management test correctly mocks both
globalThis.scrollToandglobalThis.history.pushState.
420-436: LGTM! Consistent rapid interaction test using globalThis.The rapid click test correctly uses globalThis-based mocks to verify multiple invocations.
452-483: LGTM! Consistent browser API interaction test using globalThis.The pageYOffset test correctly uses
globalThis.pageYOffsetandObject.definePropertyon globalThis.
485-504: LGTM! Consistent hash change test using globalThis.The URL hash change test correctly uses
globalThis.location.hash,globalThis.requestAnimationFrame, and fires events on globalThis.
508-523: LGTM! Consistent performance test using globalThis.The useCallback optimization test correctly mocks
globalThis.scrollTo.
525-539: LGTM! Consistent cleanup test using globalThis.The event listener cleanup test correctly spies on
globalThis.addEventListenerandglobalThis.removeEventListener.
543-570: LGTM! Consistent state change test using globalThis.The dynamic dimension test correctly mocks
globalThis.scrollTo.
572-586: LGTM! Consistent rerender test using globalThis.The component rerender test correctly spies on
globalThis.addEventListenerandglobalThis.removeEventListener.
649-681: LGTM! Consistent integration test using globalThis.The complete user journey test correctly mocks both
globalThis.scrollToandglobalThis.history.pushState.frontend/src/app/my/mentorship/page.tsx (1)
46-49: LGTM! Consistent migration to globalThis.The URL construction and comparison correctly use
globalThis.location.pathnameandglobalThis.location.search.frontend/jest.setup.ts (6)
5-5: LGTM! Consistent migration to globalThis.React is correctly assigned to
globalThis.Reactfor environment-agnostic access.
9-28: LGTM! Consistent fetch polyfill using globalThis.The fetch polyfill correctly checks for and assigns to
globalThis.fetchwith a comprehensive mock Response structure.
53-55: LGTM! Consistent structuredClone polyfill using globalThis.The structuredClone polyfill correctly checks for and assigns to
globalThis.structuredClone.
57-75: LGTM! Consistent test setup using globalThis.The beforeAll hook correctly sets up
globalThis.requestAnimationFrame,globalThis.runAnimationFrameCallbacks, andglobalThis.ResizeObserver.
82-89: LGTM! Consistent console warning mock using globalThis.The console.warn spy correctly uses
globalThis.console.warn.
91-107: LGTM! Consistent matchMedia and animation helper setup using globalThis.The matchMedia mock and animation frame helpers correctly use
globalThis.matchMedia,globalThis.runAnimationFrameCallbacks, andglobalThis.removeAnimationFrameCallbacks.
@Utkarsh-0304 could you also check these items CodeRabbit found? seems like there are a few more cases where we use |
|
Certainly, @kasya, I will make the necessary changes. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/__tests__/unit/components/ScrollToTop.test.tsx (1)
27-28: LGTM! Tests correctly use globalThis throughout.All test implementations properly simulate scrolling and verify
globalThis.scrollTobehavior. This addresses the previous review feedback about remainingwindowreferences.Minor: Update test description on line 36.
The comment mentions "window.scrollTo" but the code now uses
globalThis.scrollTo. Consider updating for consistency:- test('Clicking the button should call window.scrollTo with smooth behavior', async () => { + test('Clicking the button should call globalThis.scrollTo with smooth behavior', async () => {Also applies to: 36-49
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
frontend/__tests__/unit/components/ScrollToTop.test.tsx(3 hunks)frontend/src/app/my/mentorship/page.tsx(2 hunks)frontend/src/components/Header.tsx(2 hunks)frontend/src/components/MultiSearch.tsx(2 hunks)frontend/src/components/SingleModuleCard.tsx(2 hunks)frontend/src/utils/helpers/githubHeatmap.ts(1 hunks)frontend/src/utils/scrollToAnchor.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- frontend/src/components/SingleModuleCard.tsx
- frontend/src/app/my/mentorship/page.tsx
- frontend/src/components/Header.tsx
- frontend/src/utils/scrollToAnchor.ts
- frontend/src/utils/helpers/githubHeatmap.ts
- frontend/src/components/MultiSearch.tsx
🔇 Additional comments (1)
frontend/__tests__/unit/components/ScrollToTop.test.tsx (1)
6-8: LGTM! Test setup correctly migrated to globalThis.The beforeEach hook properly mocks
scrollToand defines required properties onglobalThis, ensuring the test environment matches the updated component implementation.
|
2 participants
Proposed change
Resolves #2485
I have replaced all environment specific global object window references with standardized globalThis, taking the reference of this list
Checklist
make check-testlocally; all checks and tests passed.