performance(preload): eliminate indefinite setInterval module polling loop#3376
performance(preload): eliminate indefinite setInterval module polling loop#3376Shevilll wants to merge 3 commits into
Conversation
isInsideSomeScreen required the window to be fully contained within a single display, so a window left at a screen edge or spanning two monitors failed the check and applyRootWindowState re-centered it on the primary display on the next launch. Treat a window as on-screen when it overlaps any display (the standard window-within-bounds heuristic) instead of requiring full containment, so the saved bounds are preserved. This also fixes the same re-centering for the video call window, which reuses this helper. Closes RocketChat#2714
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughInjected startup now tracks module load promises, runs reactive setup after successful loads, and performs a final setup after all loads settle. Root window restoration now treats any overlap with a display as on-screen, with tests covering several display geometries. ChangesInjected startup sequencing
Root window screen overlap
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant initialization
participant loadModule
participant setupReactiveFeatures
participant setInterval
initialization->>loadModule: load modules
loadModule->>setupReactiveFeatures: run after each successful module load
initialization->>setInterval: start backup setup interval
initialization->>setupReactiveFeatures: final run after Promise.allSettled(modulesToLoad)
initialization->>setInterval: clear backup interval
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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
🤖 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 `@src/injected.ts`:
- Around line 590-595: The final setup path in Promise.allSettled(modulesToLoad)
leaves the backup polling interval active if setupReactiveFeatures() throws.
Update the completion handler so clearInterval(setupInterval) is guaranteed to
run even when the final setup fails, and keep the setupReactiveFeatures() call
in a guarded/finalized flow within the same Promise.allSettled callback.
🪄 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: efce3c76-12d4-44be-9220-01962d7592ba
📒 Files selected for processing (3)
src/injected.tssrc/ui/main/rootWindow.spec.tssrc/ui/main/rootWindow.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use TypeScript for all new code unless explicitly told otherwise
Use optional chaining with fallbacks for platform-specific APIs instead of mocking when possible. Example:const uid = process.getuid?.() ?? 1000;
Files:
src/ui/main/rootWindow.tssrc/ui/main/rootWindow.spec.tssrc/injected.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{tsx,ts}: MANDATORY: Use Fuselage components for all UI work. Only create custom components when Fuselage doesn't provide what's needed
Import UI components from@rocket.chat/fuselageand checkTheme.d.tsfor valid color tokens
Use React functional components with hooks
Use PascalCase for component file names
Files:
src/ui/main/rootWindow.tssrc/ui/main/rootWindow.spec.tssrc/injected.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Redux actions must follow FSA (Flux Standard Action) pattern
Avoid unnecessary comments — write self-documenting code through clear naming
Always verify libraries by checking official docs and.d.tsfiles innode_modules/. Never assume props, tokens, or APIs work without verification
Avoid subjective descriptors ('smart', 'excellent', 'dumb') in documentation and comments
Use measurable descriptions in code documentation: 'reduced memory usage', 'improved by X%' instead of subjective claims
NEVER invent metrics — don't include estimated time spent or speculated user counts. Only include numbers from actual logs, error messages, or documented sources
Files:
src/ui/main/rootWindow.tssrc/ui/main/rootWindow.spec.tssrc/injected.ts
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.spec.tsfile naming for Renderer process tests
Files:
src/ui/main/rootWindow.spec.ts
**/*.{spec.ts,main.spec.ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Only mock platform-specific APIs when defensive coding isn't possible. Linux-only APIs requiring mocks:
process.getuid(),process.getgid(),process.geteuid(),process.getegid()
Files:
src/ui/main/rootWindow.spec.ts
🔇 Additional comments (4)
src/ui/main/rootWindow.ts (1)
140-156: LGTM!src/ui/main/rootWindow.spec.ts (2)
2-2: LGTM!Also applies to: 22-22
479-518: LGTM!src/injected.ts (1)
148-190: LGTM!
1 participant
Summary
This PR addresses Issue #3251 by eliminating the indefinite
setIntervalmodule polling loop insrc/injected.ts.Problem
Previously, a continuous 1-second interval loop (
setInterval(setupReactiveFeatures, 1000)) ran indefinitely. Since this interval continuously fired, it prevented the system from entering low-power CPU states even when completely idle, resulting in continuous steady-state CPU overhead and battery drain.Solution
Instead of continuous polling:
modulesToLoad).setupReactiveFeaturesimmediately, ensuring event-driven initialization.Promise.allSettled(modulesToLoad). Once all attempts settle (either succeeding or failing), we execute a final run ofsetupReactiveFeaturesand immediately clear the backup interval.This event-driven initialization design completely eliminates the steady-state polling overhead and allows the CPU to correctly enter low-power sleep states when idle.
Summary by CodeRabbit