fix: use polyfilled requestIdleCallback in render-if-visible HOC#8937
Conversation
The polyfill from makeplane#5689 is only applied as a side-effect when app/provider.tsx is evaluated. Lazy-loaded chunks like gantt-layout-loader can execute before the provider tree finishes hydrating, causing window.requestIdleCallback to be undefined on Safari (the only major browser without native support) and crashing the entire UI with a TypeError. Add safeRequestIdleCallback / safeCancelIdleCallback wrappers to apps/web/core/lib/polyfills and use them at the call sites in render-if-visible-HOC.tsx, making the call defensive at use rather than relying on global side-effect ordering. The existing side-effect is preserved for backward compatibility. Also cleans up a typo in the original guard at line 54 (typeof window !== undefined → effectively always truthy). Closes makeplane#8904 Closes makeplane#8871
jamartineztelecoengineer84-dotcom
requested a review
from sriramveeraghanta
as a code owner
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaces direct usage of window.requestIdleCallback/cancelIdleCallback with exported safeRequestIdleCallback/safeCancelIdleCallback fallbacks, and updates the render-if-visible HOC to use these safe wrappers and adjust intersection observer idle scheduling and cleanup. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
apps/web/core/components/core/render-if-visible-HOC.tsx (1)
77-83:⚠️ Potential issue | 🟡 MinorSame handle-leak concern on the height-measurement effect.
The deferred height measurement scheduled via
safeRequestIdleCallbackis never cancelled when the effect re-runs or the component unmounts. The body is guarded byintersectionRef.currentso it won't NPE, but on Safari this now goes through thesetTimeoutfallback (per the PR's behavior note), which means a stale callback can still fire on a detached element and write toplaceholderHeight.currentafter unmount. Capturing the returned handle and callingsafeCancelIdleCallbackin the cleanup would make this bulletproof.Also,
intersectionRefis a stable ref object; including it in the dep array has no effect — you can drop it for clarity.🛠️ Suggested fix
useEffect(() => { if (intersectionRef.current && isVisible && shouldRecordHeights) { - safeRequestIdleCallback(() => { + const handle = safeRequestIdleCallback(() => { if (intersectionRef.current) placeholderHeight.current = `${intersectionRef.current.offsetHeight}px`; }); + return () => safeCancelIdleCallback(handle); } - }, [isVisible, intersectionRef, shouldRecordHeights]); + }, [isVisible, shouldRecordHeights]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/core/components/core/render-if-visible-HOC.tsx` around lines 77 - 83, The useEffect that measures height (the effect referencing intersectionRef, isVisible, shouldRecordHeights, placeholderHeight and calling safeRequestIdleCallback) can schedule a deferred callback that is never cancelled; capture the handle returned by safeRequestIdleCallback, return a cleanup that calls safeCancelIdleCallback(handle) to cancel it on re-run/unmount, and remove the stable intersectionRef from the dependency array (leave isVisible and shouldRecordHeights) for clarity.
🧹 Nitpick comments (1)
apps/web/core/lib/polyfills/index.ts (1)
30-57: Optional: deduplicate the setTimeout fallback shared with the side-effect polyfill.The fallback body in
safeRequestIdleCallback(lines 41–48) is essentially identical to the one assigned towindow.requestIdleCallbackat lines 9–21, and the same applies tosafeCancelIdleCallbackvs. lines 23–27. Extracting a single internal helper (e.g.idleCallbackFallback/cancelIdleCallbackFallback) and reusing it in both the side-effect assignment and the safe wrappers would avoid drift if one branch is ever tweaked (e.g., timeout value,timeRemaininglogic).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/core/lib/polyfills/index.ts` around lines 30 - 57, The fallback implementations for requestIdleCallback and cancelIdleCallback are duplicated between the side-effect polyfill (the window.requestIdleCallback/window.cancelIdleCallback assignments) and the safe wrappers safeRequestIdleCallback and safeCancelIdleCallback; extract the shared logic into a single internal helper pair (e.g., idleCallbackFallback and cancelIdleCallbackFallback) and have both the side-effect polyfill and the safe* wrappers call those helpers so the timeout value and timeRemaining logic are defined once and reused across requestIdleCallback and cancelIdleCallback code paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/core/components/core/render-if-visible-HOC.tsx`:
- Around line 56-59: The effect schedules idle callbacks with
safeRequestIdleCallback but never stores or cancels the returned handle; update
the IntersectionObserver callback (inside render-if-visible-HOC) to capture the
handle returned by safeRequestIdleCallback into a local variable (e.g.,
idleHandle) and before scheduling a new idle callback call
safeCancelIdleCallback(idleHandle), and also cancel any pending idleHandle in
the effect cleanup; ensure you reference safeRequestIdleCallback,
safeCancelIdleCallback, setShouldVisible, and the observer callback when making
these changes so pending callbacks are always cleared on re-run/unmount.
---
Outside diff comments:
In `@apps/web/core/components/core/render-if-visible-HOC.tsx`:
- Around line 77-83: The useEffect that measures height (the effect referencing
intersectionRef, isVisible, shouldRecordHeights, placeholderHeight and calling
safeRequestIdleCallback) can schedule a deferred callback that is never
cancelled; capture the handle returned by safeRequestIdleCallback, return a
cleanup that calls safeCancelIdleCallback(handle) to cancel it on
re-run/unmount, and remove the stable intersectionRef from the dependency array
(leave isVisible and shouldRecordHeights) for clarity.
---
Nitpick comments:
In `@apps/web/core/lib/polyfills/index.ts`:
- Around line 30-57: The fallback implementations for requestIdleCallback and
cancelIdleCallback are duplicated between the side-effect polyfill (the
window.requestIdleCallback/window.cancelIdleCallback assignments) and the safe
wrappers safeRequestIdleCallback and safeCancelIdleCallback; extract the shared
logic into a single internal helper pair (e.g., idleCallbackFallback and
cancelIdleCallbackFallback) and have both the side-effect polyfill and the safe*
wrappers call those helpers so the timeout value and timeRemaining logic are
defined once and reused across requestIdleCallback and cancelIdleCallback code
paths.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 636ea034-2382-4a20-845d-812c4e76ac0a
📒 Files selected for processing (2)
apps/web/core/components/core/render-if-visible-HOC.tsxapps/web/core/lib/polyfills/index.ts
Address review feedback from coderabbitai: the handle returned by safeRequestIdleCallback was discarded, allowing setShouldVisible to fire on unmounted components or after the effect's deps changed. Capture the handle, cancel any pending callback before scheduling a new one, and cancel on cleanup.
2 participants
Description
When opening any project view in Safari, the entire UI crashes with:
The polyfill added in #5689 only takes effect when
apps/web/app/provider.tsxfinishes evaluating. However, lazy-loaded chunks (like
gantt-layout-loader,which
RenderIfVisibleis part of) can execute before the provider treefinishes hydrating. Their direct calls to
window.requestIdleCallbackthencrash on Safari, which is the only major browser without native support
for this API (caniuse).
This PR fixes the issue by adding two defensive wrappers
(
safeRequestIdleCallback,safeCancelIdleCallback) to the polyfills moduleand using them at the only remaining unguarded call site:
render-if-visible-HOC.tsx.Changes
apps/web/core/lib/polyfills/index.ts— adds wrapper exports whilepreserving the existing side-effect for backward compatibility with any
code that still depends on
window.requestIdleCallbackbeingmonkey-patched.
apps/web/core/components/core/render-if-visible-HOC.tsx— replacesboth
window.requestIdleCallbackcall sites with the new wrapper. Alsocleans up a small typo at line 54 where the original guard
typeof window !== undefinedwas missing the quotes around"undefined"(effectively a no-op check, always truthy).
Note on behavior change
At the line-54 call site, removing the
&& window.requestIdleCallbackshort-circuit changes Safari behavior from "fall through to synchronous"
to "async via setTimeout fallback". This matches the intent of
useIdletime: true(defer the work) and is consistent with the side-effectpolyfill already applied in
app/provider.tsxfor the rest of the tree.Incidental hardening
While editing the HOC, two preexisting react-hooks warnings became
blocking under
--deny-warnings. Both are legitimate fixes that alignwith making
RenderIfVisiblemore robust:intersectionRef.currentin a local variable inside theuseEffectso the cleanup function uses the captured node ratherthan re-reading the (possibly mutated) ref.
useIdletimeto theuseEffectdependency arrayso the observer is reconfigured when the prop changes (it was
silently sticking to the first render's value).
The previous
// eslint-disable-next-line react-hooks/exhaustive-depsdirective is removed since the underlying warning is now resolved.
Type of Change
Screenshots and Media (if applicable)
Before — Safari opening any project view:
After: Safari renders all views (List, Kanban, Spreadsheet, Gantt) without crashing.
Test Scenarios
/{workspace}/projects/{project_id}/issues/→ kanban renders, all views switchable, no console errors related torequestIdleCallback.window.requestIdleCallbackis used natively (the wrapper short-circuits towindow.requestIdleCallback(cb, options)).gantt-layout-loader-*.jsno longer contains unguardedwindow.requestIdleCallback(...)calls.pnpm check:types --filter=web...— passes (21/21 tasks).pnpm check:lint --filter=web— 0 errors. The fix reduces the preexisting warnings on the modified file from 4 to 2.pnpm check:format --filter=web— passes.pnpm build --filter=web...— passes (11/11 tasks).No new tests added; the affected HOC has no existing test coverage in
this repo, and PR #5689 (which originally added the polyfill) followed the
same precedent. Adding test infrastructure for this HOC would be a useful
follow-up.
References
Closes #8904
Closes #8871
Related to #5689 (the original polyfill PR — this completes its intent for lazy-loaded chunks)
Summary by CodeRabbit