Use standard Window interface for idle callbacks#227
Conversation
…questIdleCallback
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ 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 |
ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan Review Summary by QodoAdd proper TypeScript types for idle callback APIs
WalkthroughsDescription• Added TypeScript definitions for requestIdleCallback and cancelIdleCallback to Window interface • Removed unsafe any type casts in idle callback usage • Improved type safety for browser idle scheduling APIs Diagramflowchart LR
A["vite-env.d.ts<br/>Window interface"] -- "defines types" --> B["requestIdleCallback<br/>cancelIdleCallback"]
B -- "used by" --> C["App.tsx<br/>Idle callback handlers"]
C -- "removes" --> D["any type casts"]
File Changes1. src/vite-env.d.ts
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the application's type safety and code clarity by introducing explicit TypeScript definitions for the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Code Review by Qodo
1. Idle callbacks typed required
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Since you already guard usage with
'requestIdleCallback' in window, consider markingrequestIdleCallbackandcancelIdleCallbackas optional on theWindowinterface to better reflect that they may not exist in all environments. - The
optionsparameter forrequestIdleCallbackshould likely usetimeout?: numberrather thantimeout: numberto match the browser API, which treatstimeoutas optional.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since you already guard usage with `'requestIdleCallback' in window`, consider marking `requestIdleCallback` and `cancelIdleCallback` as optional on the `Window` interface to better reflect that they may not exist in all environments.
- The `options` parameter for `requestIdleCallback` should likely use `timeout?: number` rather than `timeout: number` to match the browser API, which treats `timeout` as optional.
## Individual Comments
### Comment 1
<location path="src/vite-env.d.ts" line_range="9-15" />
<code_context>
+ readonly didTimeout: boolean;
+ timeRemaining(): number;
+ }) => void,
+ options?: { timeout: number }
+ ): number;
+ cancelIdleCallback(handle: number): void;
</code_context>
<issue_to_address>
**suggestion:** Align the `timeout` option shape with the standard `IdleRequestOptions` to avoid unnecessary type friction.
The DOM `requestIdleCallback` uses `IdleRequestOptions` with an optional `timeout?: number`, but this overload makes `timeout` required whenever `options` is passed, which is stricter than the platform API and rejects valid calls like `requestIdleCallback(cb, {})`. Please make `timeout` optional (e.g., `options?: { timeout?: number }` or reuse `IdleRequestOptions`) to keep the signature compatible with the standard.
```suggestion
requestIdleCallback(
callback: (deadline: {
readonly didTimeout: boolean;
timeRemaining(): number;
}) => void,
options?: IdleRequestOptions
): number;
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| requestIdleCallback( | ||
| callback: (deadline: { | ||
| readonly didTimeout: boolean; | ||
| timeRemaining(): number; | ||
| }) => void, | ||
| options?: { timeout: number } | ||
| ): number; |
There was a problem hiding this comment.
suggestion: Align the timeout option shape with the standard IdleRequestOptions to avoid unnecessary type friction.
The DOM requestIdleCallback uses IdleRequestOptions with an optional timeout?: number, but this overload makes timeout required whenever options is passed, which is stricter than the platform API and rejects valid calls like requestIdleCallback(cb, {}). Please make timeout optional (e.g., options?: { timeout?: number } or reuse IdleRequestOptions) to keep the signature compatible with the standard.
| requestIdleCallback( | |
| callback: (deadline: { | |
| readonly didTimeout: boolean; | |
| timeRemaining(): number; | |
| }) => void, | |
| options?: { timeout: number } | |
| ): number; | |
| requestIdleCallback( | |
| callback: (deadline: { | |
| readonly didTimeout: boolean; | |
| timeRemaining(): number; | |
| }) => void, | |
| options?: IdleRequestOptions | |
| ): number; |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly extends the global Window interface to include TypeScript definitions for requestIdleCallback and cancelIdleCallback, which successfully removes the need for any type assertions. The implementation is sound. I have added one suggestion in src/vite-env.d.ts to further improve the type definitions by using a declare global block and creating a separate IdleDeadline interface, which enhances clarity and aligns with best practices for global type augmentation.
| interface Window { | ||
| requestIdleCallback( | ||
| callback: (deadline: { | ||
| readonly didTimeout: boolean; | ||
| timeRemaining(): number; | ||
| }) => void, | ||
| options?: { timeout: number } | ||
| ): number; | ||
| cancelIdleCallback(handle: number): void; | ||
| } |
There was a problem hiding this comment.
To improve type safety and adhere to best practices for global type augmentation, it's better to wrap this in declare global. Additionally, defining IdleDeadline as a separate interface improves readability and reusability.
| interface Window { | |
| requestIdleCallback( | |
| callback: (deadline: { | |
| readonly didTimeout: boolean; | |
| timeRemaining(): number; | |
| }) => void, | |
| options?: { timeout: number } | |
| ): number; | |
| cancelIdleCallback(handle: number): void; | |
| } | |
| declare global { | |
| interface IdleDeadline { | |
| readonly didTimeout: boolean; | |
| timeRemaining(): number; | |
| } | |
| interface Window { | |
| requestIdleCallback( | |
| callback: (deadline: IdleDeadline) => void, | |
| options?: { timeout: number } | |
| ): number; | |
| cancelIdleCallback(handle: number): void; | |
| } | |
| } |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| interface Window { | ||
| requestIdleCallback( | ||
| callback: (deadline: { | ||
| readonly didTimeout: boolean; | ||
| timeRemaining(): number; | ||
| }) => void, | ||
| options?: { timeout: number } | ||
| ): number; | ||
| cancelIdleCallback(handle: number): void; | ||
| } |
There was a problem hiding this comment.
1. Idle callbacks typed required 🐞 Bug ✓ Correctness
src/vite-env.d.ts makes requestIdleCallback/cancelIdleCallback mandatory on Window, even though the app treats them as optional at runtime (feature-detects before calling). This weakens type safety by allowing other call sites to call these APIs without checks and still compile, potentially crashing at runtime in environments that don’t implement them.
Agent Prompt
## Issue description
`Window.requestIdleCallback` and `Window.cancelIdleCallback` are declared as required in `src/vite-env.d.ts`, but the runtime code still feature-detects them. This makes the type system claim the APIs always exist and can let other call sites compile without guards, risking runtime `TypeError` in non-supporting environments.
## Issue Context
The current warmup logic already uses feature detection before calling idle callbacks.
## Fix Focus Areas
- Make the augmented properties optional (e.g., `requestIdleCallback?: ...`, `cancelIdleCallback?: ...`) to reflect runtime variability.
- Update call sites to narrow with `typeof window.requestIdleCallback === 'function'` / `typeof window.cancelIdleCallback === 'function'` (or keep existing guards and use non-null assertion after narrowing).
### Target locations
- src/vite-env.d.ts[8-17]
- src/App.tsx[486-526]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
ysdede
changed the title
1 participant
🎯 What: Extended the global
Windowinterface to include proper TypeScript definitions forrequestIdleCallbackandcancelIdleCallbackinstead of castingwindowtoany.💡 Why: Reduces unsafe
anyusages and correctly utilizes native browser APIs, improving type safety and readability.✅ Verification: Ran test suite and production build step.
✨ Result: Safe typing applied to idle callbacks.
PR created automatically by Jules for task 5515221070947143723 started by @ysdede
Summary by Sourcery
Strengthen typing around browser idle callbacks and remove unsafe window casts.
Enhancements: