Conversation
WalkthroughAdds external URL handling with IPC-based validation and protocol checking, modifies notification attention behavior to apply across notification types, and introduces a crash handler for renderer process recovery with session state clearing. Changes
Sequence Diagram(s)sequenceDiagram
participant IPC as IPC Main
participant Handler as open-external Handler
participant Nav as navigation/main
participant Shell as shell/electron
participant Window as BrowserWindow
IPC->>Handler: 'open-external' event
Handler->>Handler: Validate URL format
Handler->>Nav: Import isProtocolAllowed
Nav-->>Handler: Protocol validation function
Handler->>Handler: Check if protocol allowed
alt Protocol Allowed
Handler->>Shell: openExternal() or shell.openExternal()
Shell-->>Handler: URL opened
else Protocol Blocked
Handler->>Handler: Block disallowed protocol
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 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. 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/notifications/main.ts`:
- Around line 88-92: The predicate using notificationTypes.get(id) is always
true because options.notificationType is constrained to 'text'|'voice', so
change the blink decision to check an actual chat indicator instead: when
creating a notification (where options.notificationType is set) also store a
boolean or explicit source (e.g., notificationIsChat.set(id, true) or
notificationSource.set(id, 'chat')) and then here replace the current check
(notificationType === 'text' || notificationType === 'voice') with that
chat-specific check (e.g., notificationIsChat.get(id) === true or
notificationSource.get(id) === 'chat') before calling
attentionDrawing.drawAttention(id); ensure the value is written at creation so
the lookup is meaningful.
In `@src/ui/main/rootWindow.ts`:
- Around line 524-544: The handler attached to
browserWindow.webContents.on('render-process-gone', async (_event, details) => {
... }) indiscriminately clears the app's shared session via session.clearCache()
and session.clearStorageData(...), which wipes global cookies and persisted
state; change this to first inspect details.reason and only perform
cache/storage clearing for real crash/oom/abnormal conditions (e.g., 'crashed',
'oom', 'abnormal-exit', 'launch-failed') while skipping or logging for benign
reasons ('clean-exit', 'killed', 'memory-eviction', 'integrity-failure');
additionally, avoid clearing auth-bearing storages (cookies, serviceworkers,
cachestorage) unless you detect a crash loop or the BrowserWindow uses a
non-default partition—modify the logic around the session.clearCache and
session.clearStorageData calls so they run conditionally and target only safe
storages when necessary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bce09a87-fee8-4657-bb6c-eafd8e86c52d
📒 Files selected for processing (3)
src/main.tssrc/notifications/main.tssrc/ui/main/rootWindow.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript strict mode enabled in TypeScript configuration
Use React functional components with hooks instead of class components
Follow FSA (Flux Standard Action) pattern for Redux actions
Use camelCase for file names and PascalCase for component file names
All code must pass ESLint and TypeScript checks
Write self-documenting code with clear naming; avoid unnecessary comments except for complex business logic or non-obvious decisions
Use Fuselage components from@rocket.chat/fuselagefor all UI work and only create custom components when Fuselage doesn't provide what's needed
CheckTheme.d.tsfor valid color tokens when using Fuselage components
Use defensive coding with optional chaining and fallbacks for Linux-only APIs (process.getuid(), process.getgid(), process.geteuid(), process.getegid()) to ensure cross-platform compatibility across Windows, macOS, and Linux
Files:
src/ui/main/rootWindow.tssrc/main.tssrc/notifications/main.ts
🔇 Additional comments (1)
src/notifications/main.ts (1)
104-105: Safe simplification.
src/notifications/attentionDrawing.ts:46-76already no-ops when an id was never tracked, so makingstopAttention()unconditional on close/dismiss removes duplicate branching without changing behavior.Also applies to: 250-251
| handle('open-external', async (_webContents, rawUrl) => { | ||
| let url: URL; | ||
|
|
||
| try { | ||
| url = new URL(rawUrl); | ||
| } catch { | ||
| console.warn('Blocked malformed external URL'); | ||
| return; | ||
| } | ||
|
|
||
| const { isProtocolAllowed } = await import('./navigation/main'); | ||
|
|
||
| if (!(await isProtocolAllowed(url.toString()))) { | ||
| console.warn('Blocked external URL with disallowed protocol'); | ||
| return; | ||
| } | ||
|
|
||
| if (url.protocol === 'http:' || url.protocol === 'https:') { | ||
| await openExternal(url.toString()); | ||
| return; | ||
| } | ||
|
|
||
| await shell.openExternal(url.toString()); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the handle function implementation in src/ipc/main.ts
fd "ipc.*main" --type f | head -20Repository: RocketChat/Rocket.Chat.Electron
Length of output: 57
🏁 Script executed:
# Read the main handler at lines 148-171
sed -n '140,180p' src/main.ts | cat -nRepository: RocketChat/Rocket.Chat.Electron
Length of output: 1370
🏁 Script executed:
# Read the handle function in src/ipc/main.ts around lines 29-43
fd "ipc" --type dRepository: RocketChat/Rocket.Chat.Electron
Length of output: 83
🏁 Script executed:
# Search for ipc/main.ts or similar
find . -path "*/ipc/main*" -type fRepository: RocketChat/Rocket.Chat.Electron
Length of output: 92
🏁 Script executed:
# Read the handle function in src/ipc/main.ts around lines 29-43
sed -n '20,50p' src/ipc/main.ts | cat -nRepository: RocketChat/Rocket.Chat.Electron
Length of output: 731
🏁 Script executed:
# Read the isProtocolAllowed function in src/navigation/main.ts around lines 213-241
sed -n '205,245p' src/navigation/main.ts | cat -nRepository: RocketChat/Rocket.Chat.Electron
Length of output: 1236
Lock this IPC handler to trusted frames and explicit schemes.
The _webContents parameter is unused, so this privileged endpoint cannot distinguish the app's main frame from an embedded or compromised frame. The handle() wrapper in src/ipc/main.ts passes event.sender directly without validating caller origin or frame source. isProtocolAllowed() in src/navigation/main.ts also allows any previously-persisted custom protocol via the Redux externalProtocols store, without restriction. This means any external protocol the user previously approved can be reopened by any frame. Before calling shell.openExternal(), validate the sender's origin against trusted app frames and explicitly allow only the schemes this app supports.
| const notificationType = notificationTypes.get(id); | ||
| if (notificationType === 'voice') { | ||
|
|
||
| // Only flash for chat-related notifications | ||
| if (notificationType === 'text' || notificationType === 'voice') { | ||
| attentionDrawing.drawAttention(id); |
There was a problem hiding this comment.
This predicate never filters anything.
Line 226 stores options.notificationType || 'text' for every new notification, and src/notifications/common.ts:1-26 constrains that value to 'text' | 'voice'. That makes the new notificationType === 'text' || notificationType === 'voice' check always true for created notifications, so this change does not actually limit blinking to message notifications.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/notifications/main.ts` around lines 88 - 92, The predicate using
notificationTypes.get(id) is always true because options.notificationType is
constrained to 'text'|'voice', so change the blink decision to check an actual
chat indicator instead: when creating a notification (where
options.notificationType is set) also store a boolean or explicit source (e.g.,
notificationIsChat.set(id, true) or notificationSource.set(id, 'chat')) and then
here replace the current check (notificationType === 'text' || notificationType
=== 'voice') with that chat-specific check (e.g., notificationIsChat.get(id) ===
true or notificationSource.get(id) === 'chat') before calling
attentionDrawing.drawAttention(id); ensure the value is written at creation so
the lookup is meaningful.
| browserWindow.webContents.on( | ||
| 'render-process-gone', | ||
| async (_event, details) => { | ||
| console.error('Renderer process crashed:', details.reason); | ||
| try { | ||
| const { session } = browserWindow.webContents; | ||
| await session.clearCache(); | ||
| await session.clearStorageData({ | ||
| storages: [ | ||
| 'cookies', | ||
| 'indexdb', | ||
| 'filesystem', | ||
| 'shadercache', | ||
| 'websql', | ||
| 'serviceworkers', | ||
| 'cachestorage', | ||
| ], | ||
| }); | ||
| console.log('Cache cleared. Reloading window...'); | ||
| browserWindow.reload(); | ||
| } catch (error) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file structure and the specific lines mentioned
find . -name "rootWindow.ts" -type fRepository: RocketChat/Rocket.Chat.Electron
Length of output: 122
🏁 Script executed:
# Once we find it, let's examine the code around lines 524-544
head -n 550 src/ui/main/rootWindow.ts | tail -n 50Repository: RocketChat/Rocket.Chat.Electron
Length of output: 1334
🏁 Script executed:
# Let's also check for the crashHandlerRegistered flag mentioned in the scratchpad
rg "crashHandlerRegistered" src/ui/main/rootWindow.ts -B 2 -A 2Repository: RocketChat/Rocket.Chat.Electron
Length of output: 377
🏁 Script executed:
# Check webPreferences to see if a custom partition is specified
rg "webPreferences|partition" src/ui/main/rootWindow.ts -B 3 -A 3Repository: RocketChat/Rocket.Chat.Electron
Length of output: 663
🌐 Web query:
Electron render-process-gone event reasons documentation
💡 Result:
Electron documents the 'render-process-gone' event’s details.reason values in the RenderProcessGoneDetails structure. The possible reason strings are: clean-exit, abnormal-exit, killed, crashed, oom, launch-failed, integrity-failure, and memory-eviction, with exitCode giving the process exit code (or a platform-specific launch failure code when reason is launch-failed). [1]
You’ll encounter this details object when handling 'render-process-gone' on webContents (and similarly on app / <webview>), which replaced older crash-related events. [2], [3]
Sources
[1] Electron docs — RenderProcessGoneDetails structure: https://www.electronjs.org/docs/latest/api/structures/render-process-gone-details
[2] Electron docs — webContents API (render-process-gone context): https://www.electronjs.org/docs/latest/api/web-contents
[3] Electron docs — Breaking changes (migration to render-process-gone): https://www.electronjs.org/docs/latest/breaking-changes
🌐 Web query:
Electron session.clearStorageData cookies indexdb impact shared session
💡 Result:
session.clearStorageData() clears Chromium storage inside a specific Electron Session (i.e., a storage partition)—optionally limited to a specific origin and/or specific storage types like cookies and indexdb. [1]
What it clears (relevant to your question)
- If you include
storages: ['cookies'], it deletes cookies for that session (and for the specifiedoriginif you pass one). [1] - If you include
storages: ['indexdb'], it deletes IndexedDB data for that session (and origin, if provided). [1] - If you omit
storages, it clears all listed storage types for that session. [1]
Impact when the session is “shared”
Electron sessions are shared by partition:
session.fromPartition(partition)returns the sameSessionobject for the samepartitionstring. [1]- If the partition starts with
persist:, it’s a persistent session available to all pages in the app using that same partition; withoutpersist:it’s in-memory. [1][2]
So, calling clearStorageData() on a shared/persistent partition will affect every BrowserWindow/webContents using that same partition (they’ll all see cookies/IndexedDB disappear), but it won’t affect other partitions (including other persist:* partitions) or other Electron apps. [1][2]
Sources:
[1] Electron session API docs (clearStorageData, fromPartition, persist partitions) (electronjs.org)
[2] Stack Overflow discussion of persist: partitions being shared/persistent (stackoverflow.com)
Citations:
- 1: https://www.electronjs.org/docs/latest/api/session
- 2: https://stackoverflow.com/questions/69943164/how-to-create-a-persistent-on-disk-session-in-electronjs?utm_source=openai
🏁 Script executed:
# Let's check if there's any filtering on details.reason in the handler
rg "details.reason" src/ui/main/rootWindow.ts -B 3 -A 3Repository: RocketChat/Rocket.Chat.Electron
Length of output: 349
🏁 Script executed:
# Check if there are any other handlers or guards for this event
rg "render-process-gone" src/ -B 2 -A 5Repository: RocketChat/Rocket.Chat.Electron
Length of output: 558
Don't erase the shared session on all renderer process terminations.
Lines 529–541 clear the main window's shared session without filtering on the termination reason. Electron's render-process-gone event reports diverse reasons—clean-exit, abnormal-exit, killed, crashed, oom, launch-failed, integrity-failure, and memory-eviction—but this handler treats all equally, immediately clearing storage including cookies, indexdb, serviceworkers, and cachestorage. Since no custom partition is specified in webPreferences, this clears the app's default shared session, which logs out users and erases persisted state app-wide on legitimate non-crash scenarios like clean-exit, killed, or memory-eviction. Gate recovery logic to actual crash reasons (crashed, oom, abnormal-exit, launch-failed) and avoid clearing auth-bearing storage unless you've detected a repeat crash loop.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/main/rootWindow.ts` around lines 524 - 544, The handler attached to
browserWindow.webContents.on('render-process-gone', async (_event, details) => {
... }) indiscriminately clears the app's shared session via session.clearCache()
and session.clearStorageData(...), which wipes global cookies and persisted
state; change this to first inspect details.reason and only perform
cache/storage clearing for real crash/oom/abnormal conditions (e.g., 'crashed',
'oom', 'abnormal-exit', 'launch-failed') while skipping or logging for benign
reasons ('clean-exit', 'killed', 'memory-eviction', 'integrity-failure');
additionally, avoid clearing auth-bearing storages (cookies, serviceworkers,
cachestorage) unless you detect a crash loop or the BrowserWindow uses a
non-default partition—modify the logic around the session.clearCache and
session.clearStorageData calls so they run conditionally and target only safe
storages when necessary.
1 participant
Adds taskbar icon blinking when a new message notification is received.
This improves visibility of unread messages when the application window is minimized or not focused.
Implementation:
Summary by CodeRabbit
New Features
Bug Fixes