[ENGG-4757] fix: Not able to quit desktop app#4171
Conversation
WalkthroughThe AppModeInitializer useEffect that registers IPC handlers for desktop deep linking and app-close initiation was changed to include a cleanup function that unregisters the "deeplink-handler" and "initiate-app-close" events on unmount or dependency change. The effect's dependency array was updated from Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-12-11T06:43:01.328ZApplied to files:
🔇 Additional comments (1)
✏️ Tip: You can disable this entire section by setting 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 (1)
app/src/hooks/AppModeInitializer.js (1)
214-224: Add cleanup function to prevent duplicate IPC event handlers.The dependency array change from
[]to[appMode]correctly ensures handlers register onceappModebecomesDESKTOP. However, this creates duplicate handler registrations: each timeappModechanges after initial registration, the effect re-runs and registers the handlers again without removing the previous ones.The codebase has
window.RQ.DESKTOP.SERVICES.IPC.unregisterEvent()available (used in DesktopBackgroundService.ts), so cleanup is required:🔧 Required cleanup pattern
useEffect(() => { if (appMode === GLOBAL_CONSTANTS.APP_MODES.DESKTOP) { window.RQ.DESKTOP.SERVICES.IPC.registerEvent("deeplink-handler", (payload) => { navigate(payload); }); window.RQ.DESKTOP.SERVICES.IPC.registerEvent("initiate-app-close", () => { navigate(PATHS.DESKTOP.QUIT.ABSOLUTE); }); + + return () => { + window.RQ.DESKTOP.SERVICES.IPC.unregisterEvent("deeplink-handler"); + window.RQ.DESKTOP.SERVICES.IPC.unregisterEvent("initiate-app-close"); + }; } // eslint-disable-next-line react-hooks/exhaustive-deps }, [appMode]);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/hooks/AppModeInitializer.js
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rohanmathur91
Repo: requestly/requestly PR: 4000
File: app/src/features/apiClient/slices/multiWorkspaceView/helpers/ApiClientContextRegistry/hooks.ts:6-14
Timestamp: 2025-12-11T06:43:01.328Z
Learning: In app/src/features/apiClient/slices/multiWorkspaceView/helpers/ApiClientContextRegistry/hooks.ts, the useApiClientFeatureContext hook uses useMemo without reactivity intentionally. Contexts are expected to be registered before component mount, and reactivity to registry updates is not needed.
Learnt from: nafees87n
Repo: requestly/requestly PR: 3417
File: app/src/hooks/workspaceFetcher/useWorkspaceFetcher.ts:4-7
Timestamp: 2025-08-20T11:23:01.896Z
Learning: In useFetchTeamWorkspaces hook, activeWorkspaceId should not be included in the effect dependency array to avoid re-subscription on every workspace change, as per user's preference.
📚 Learning: 2025-12-11T06:43:01.328Z
Learnt from: rohanmathur91
Repo: requestly/requestly PR: 4000
File: app/src/features/apiClient/slices/multiWorkspaceView/helpers/ApiClientContextRegistry/hooks.ts:6-14
Timestamp: 2025-12-11T06:43:01.328Z
Learning: In app/src/features/apiClient/slices/multiWorkspaceView/helpers/ApiClientContextRegistry/hooks.ts, the useApiClientFeatureContext hook uses useMemo without reactivity intentionally. Contexts are expected to be registered before component mount, and reactivity to registry updates is not needed.
Applied to files:
app/src/hooks/AppModeInitializer.js
🧬 Code graph analysis (1)
app/src/hooks/AppModeInitializer.js (16)
app/src/features/sessionBook/components/PermissionError/index.js (1)
appMode(18-18)app/src/layouts/DashboardLayout/DashboardContent.js (1)
appMode(44-44)app/src/hooks/DbListenerInit/DBListeners.js (1)
appMode(21-21)app/src/components/user/Teams/CreateWorkspace/CreateWorkspace.js (1)
appMode(27-27)app/src/features/settings/components/Profile/ManageTeams/TeamViewer/SwitchWorkspaceButton/index.jsx (1)
appMode(15-15)app/src/components/features/rules/ChangeRuleGroupModal/index.js (1)
appMode(36-36)app/src/components/features/rules/RenameGroupModal/index.js (1)
appMode(27-27)app/src/components/features/rules/RuleBuilder/RuleBuilder.js (1)
appMode(58-58)app/src/components/features/rules/UngroupOrDeleteRulesModal/index.js (1)
appMode(26-26)app/src/hooks/FeatureUsageEvent.js (1)
appMode(15-15)app/src/views/features/sessions/ConfigurationPage/ConfigurationPage.js (1)
appMode(50-50)app/src/layouts/DashboardLayout/MenuHeader/HeaderUser.jsx (1)
appMode(41-41)app/src/features/rules/modals/ImportRulesModal/index.jsx (1)
appMode(45-45)app/src/views/features/rules/RuleEditor/components/Header/ActionButtons/CreateRuleButton/index.jsx (1)
appMode(113-113)app/src/components/sections/Navbars/NavbarRightContent/DesktopAppProxyInfo/index.jsx (1)
appMode(16-16)app/src/components/misc/SignInViaEmailLink/SignInViaEmailLink.js (1)
appMode(40-40)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
2 participants
Root Cause Analysis: First Startup Quit Bug
Issue
On first startup, clicking the close button (X) causes the app to hang instead of quitting. Works normally on subsequent startups.
Root Cause
The
initiate-app-closeIPC handler registration had a missing dependency in its useEffect, causing it to never register whenappModechanged from the default"EXTENSION"to"DESKTOP".Technical Details
Flow on first startup:
"initiate-app-close"messageAppModeInitializer.js(line 215) should catch this and navigate to quit pageappMode = "EXTENSION"(default value from Redux)if (appMode === DESKTOP)condition fails → handler not registeredappModeto"DESKTOP"[])Why it worked on 2nd+ startups:
appModepersisted from previous session as"DESKTOP"Fix
Added
appModeto the useEffect dependency array:}, [appMode]); // Changed from []