feat: sys tray offline icon#3342
Conversation
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughTray icon asset selection now reflects login status across all platforms. Platform-specific helpers return offline icons when logged out; otherwise they return badge-based icons. A new watcher detects logged-out transitions, restores/focuses the root window, displays a critical notification, and updates the tray icon accordingly. ChangesLogged-out tray icon state and user notification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
🧹 Nitpick comments (1)
src/ui/main/trayIcon.ts (1)
151-154: 💤 Low valueMinor: Simplify the regex for stripping HTML tags.
The escape
\/is unnecessary inside a character class. A cleaner pattern:- body: t('error.authNeeded', { auth: '' }).replace( - /<[\/]?strong>/g, - '' - ), + body: t('error.authNeeded', { auth: '' }).replace(/<\/?strong>/g, ''),🤖 Prompt for 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. In `@src/ui/main/trayIcon.ts` around lines 151 - 154, In the trayIcon.ts assignment where body is built using t('error.authNeeded', { auth: '' }).replace(...), simplify the regex used to strip <strong> tags by removing the unnecessary escape and character class; update the argument to replace to use the cleaner pattern (e.g., replace the current /<[\/]?strong>/g with a simpler /<\/?strong>/g) so the call in that .replace(...) is clearer and equivalent.
🤖 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/ui/main/icons.ts`:
- Line 31: Add the missing semicolon after the function declaration that ends
with the final closing brace shown in the diff (the function in icons.ts that
closes with “}”); update the file by appending a semicolon immediately after
that closing brace so the function declaration ends with “};” to satisfy ESLint.
In `@src/ui/main/trayIcon.ts`:
- Around line 66-72: The current isLoggedIn uses servers.every(...) which
returns true for an empty array, causing a false "logged in" state; change the
computation to consider empty servers as not logged in and align with the
watcher logic — e.g. set isLoggedIn to servers.length > 0 &&
servers.every((server) => server.userLoggedIn) (or, if you prefer the "offline
if any logged out" behavior, compute offline = servers.length === 0 ||
servers.some((s) => !s.userLoggedIn) and pass isLoggedIn = !offline) and update
the code paths that use servers and getTrayIconPath so both the tray
initialization and the watcher use the same predicate.
- Line 159: The call to updateTrayIconImage(trayIcon, undefined) is executed
unconditionally on watcher triggers and sends badge: undefined, which can
erroneously clear the badge; move this call inside the isLoggedOut conditional
so it only runs when isLoggedOut is true (or remove it entirely and rely on
unwatchGlobalBadge to update the tray), ensuring updateTrayIconImage is invoked
with the real badge value when logging in/out and that unwatchGlobalBadge
continues to handle badge-driven updates.
---
Nitpick comments:
In `@src/ui/main/trayIcon.ts`:
- Around line 151-154: In the trayIcon.ts assignment where body is built using
t('error.authNeeded', { auth: '' }).replace(...), simplify the regex used to
strip <strong> tags by removing the unnecessary escape and character class;
update the argument to replace to use the cleaner pattern (e.g., replace the
current /<[\/]?strong>/g with a simpler /<\/?strong>/g) so the call in that
.replace(...) is clearer and equivalent.
🪄 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: c644bd8f-3ad5-40db-a10a-bb147513bc44
⛔ Files ignored due to path filters (5)
src/public/images/tray/darwin/offline.pngis excluded by!**/*.pngsrc/public/images/tray/darwin/offline@2x.pngis excluded by!**/*.pngsrc/public/images/tray/linux/offline.pngis excluded by!**/*.pngsrc/public/images/tray/linux/offline@2x.pngis excluded by!**/*.pngsrc/public/images/tray/win32/offline.icois excluded by!**/*.ico
📒 Files selected for processing (3)
src/ui/main/icons.tssrc/ui/main/rootWindow.tssrc/ui/main/trayIcon.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript for all new code in this codebase unless explicitly told otherwise
Use Fuselage components from@rocket.chat/fuselagefor all UI work — only create custom components when Fuselage doesn't provide the needed functionality
Check Theme.d.ts for valid color tokens when working with Fuselage components
Use optional chaining with fallbacks for platform-specific APIs instead of mocks (e.g., process.getuid?.() ?? 1000) to ensure code works across all platforms without requiring mocks
TypeScript code must use strict mode
Use React functional components with hooks instead of class components
Redux actions must follow the FSA (Flux Standard Action) pattern
Use camelCase for file naming
Use PascalCase for component file names (React components)
Write self-documenting code through clear naming — avoid unnecessary comments
Files:
src/ui/main/rootWindow.tssrc/ui/main/icons.tssrc/ui/main/trayIcon.ts
🪛 ESLint
src/ui/main/icons.ts
[error] 31-31: Insert ;
(prettier/prettier)
🔇 Additional comments (3)
src/ui/main/icons.ts (1)
19-30: LGTM!Also applies to: 33-47, 49-63, 65-87
src/ui/main/trayIcon.ts (1)
1-1: LGTM!Also applies to: 134-161, 208-213
src/ui/main/rootWindow.ts (1)
436-445: LGTM!
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/main/trayIcon.ts (1)
141-159:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRefresh tray icon on both login-state transitions, not only logged-out.
Line 158updates the image only whenisLoggedOutistrue. After re-authentication, if the badge value does not change, the icon may remain offline until another badge update occurs.Proposed fix
const unwatchUserLoggedIn = watch( @@ - async (isLoggedOut) => { + async (isLoggedOut) => { + const globalBadge = select(selectGlobalBadge); + updateTrayIconImage(trayIcon, globalBadge); + if (isLoggedOut) { const rootWindow = await getRootWindow(); if (rootWindow) { @@ new Notification({ @@ }).show(); - updateTrayIconImage(trayIcon, undefined); } } );🤖 Prompt for 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. In `@src/ui/main/trayIcon.ts` around lines 141 - 159, The tray icon refresh is only triggered when isLoggedOut is true, so move the call to updateTrayIconImage(trayIcon, undefined) out of the if block (or add an else that calls it) so it runs for both logout and login transitions; update the async (isLoggedOut) => handler that calls getRootWindow() and creates the Notification to ensure updateTrayIconImage is invoked regardless of isLoggedOut to prevent the icon staying stale after re-authentication.
🤖 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.
Outside diff comments:
In `@src/ui/main/trayIcon.ts`:
- Around line 141-159: The tray icon refresh is only triggered when isLoggedOut
is true, so move the call to updateTrayIconImage(trayIcon, undefined) out of the
if block (or add an else that calls it) so it runs for both logout and login
transitions; update the async (isLoggedOut) => handler that calls
getRootWindow() and creates the Notification to ensure updateTrayIconImage is
invoked regardless of isLoggedOut to prevent the icon staying stale after
re-authentication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d2e9b0d8-76bc-49e2-a939-a4eb4b3e7745
📒 Files selected for processing (2)
src/ui/main/icons.tssrc/ui/main/trayIcon.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript for all new code in this codebase unless explicitly told otherwise
Use Fuselage components from@rocket.chat/fuselagefor all UI work — only create custom components when Fuselage doesn't provide the needed functionality
Check Theme.d.ts for valid color tokens when working with Fuselage components
Use optional chaining with fallbacks for platform-specific APIs instead of mocks (e.g., process.getuid?.() ?? 1000) to ensure code works across all platforms without requiring mocks
TypeScript code must use strict mode
Use React functional components with hooks instead of class components
Redux actions must follow the FSA (Flux Standard Action) pattern
Use camelCase for file naming
Use PascalCase for component file names (React components)
Write self-documenting code through clear naming — avoid unnecessary comments
Files:
src/ui/main/icons.tssrc/ui/main/trayIcon.ts
🔇 Additional comments (1)
src/ui/main/icons.ts (1)
31-31: LGTM!
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/ui/main/trayIcon.ts (1)
141-160:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve badge state when refreshing tray icon on auth transitions.
At Line 159, refreshing with
undefinedcan clear badge-based tray variants even when unread state exists. Use the current global badge when forcing this refresh.Suggested fix
- updateTrayIconImage(trayIcon, undefined); + const globalBadge = select(selectGlobalBadge); + updateTrayIconImage(trayIcon, globalBadge);🤖 Prompt for 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. In `@src/ui/main/trayIcon.ts` around lines 141 - 160, When handling the isLoggedOut flow in the async callback, calling updateTrayIconImage(trayIcon, undefined) clears any badge-based tray variant; change the call to pass the current global badge state instead of undefined (e.g., replace undefined with the module/global badge variable used to track unread/badge state) so updateTrayIconImage(trayIcon, <currentBadgeVariable>) preserves badge visuals during auth transitions.
🤖 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.
Duplicate comments:
In `@src/ui/main/trayIcon.ts`:
- Around line 141-160: When handling the isLoggedOut flow in the async callback,
calling updateTrayIconImage(trayIcon, undefined) clears any badge-based tray
variant; change the call to pass the current global badge state instead of
undefined (e.g., replace undefined with the module/global badge variable used to
track unread/badge state) so updateTrayIconImage(trayIcon,
<currentBadgeVariable>) preserves badge visuals during auth transitions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 556473b7-5c21-4532-82b8-d02774afb309
📒 Files selected for processing (1)
src/ui/main/trayIcon.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript for all new code in this codebase unless explicitly told otherwise
Use Fuselage components from@rocket.chat/fuselagefor all UI work — only create custom components when Fuselage doesn't provide the needed functionality
Check Theme.d.ts for valid color tokens when working with Fuselage components
Use optional chaining with fallbacks for platform-specific APIs instead of mocks (e.g., process.getuid?.() ?? 1000) to ensure code works across all platforms without requiring mocks
TypeScript code must use strict mode
Use React functional components with hooks instead of class components
Redux actions must follow the FSA (Flux Standard Action) pattern
Use camelCase for file naming
Use PascalCase for component file names (React components)
Write self-documenting code through clear naming — avoid unnecessary comments
Files:
src/ui/main/trayIcon.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/main/trayIcon.ts (1)
142-148:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
getRootWindow()rejection in the logged-out watcher.At Line 143, a rejected
getRootWindow()can break this watcher callback and prevent the notification/icon refresh path from completing. Wrap it intry/catch(or.catch) and continue safely.Proposed fix
async (isLoggedOut) => { if (isLoggedOut) { - const rootWindow = await getRootWindow(); - if (rootWindow) { - if (rootWindow.isMinimized()) rootWindow.restore(); - rootWindow.show(); - rootWindow.focus(); - } + try { + const rootWindow = await getRootWindow(); + if (rootWindow.isMinimized()) rootWindow.restore(); + rootWindow.show(); + rootWindow.focus(); + } catch { + // Root window may not be ready/destroyed; continue with notification/icon update. + } new Notification({🤖 Prompt for 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. In `@src/ui/main/trayIcon.ts` around lines 142 - 148, The watcher callback that runs when isLoggedOut can throw if getRootWindow() rejects, so wrap the await getRootWindow() call in a try/catch (or attach .catch) inside the logged-out branch to swallow/log the error and continue execution so the rest of the notification/icon refresh path still runs; update the code around the isLoggedOut block that calls getRootWindow() to handle rejection safely (use getRootWindow, rootWindow.restore(), rootWindow.show(), rootWindow.focus() references) and ensure the catch logs the error and does not rethrow.
🤖 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.
Outside diff comments:
In `@src/ui/main/trayIcon.ts`:
- Around line 142-148: The watcher callback that runs when isLoggedOut can throw
if getRootWindow() rejects, so wrap the await getRootWindow() call in a
try/catch (or attach .catch) inside the logged-out branch to swallow/log the
error and continue execution so the rest of the notification/icon refresh path
still runs; update the code around the isLoggedOut block that calls
getRootWindow() to handle rejection safely (use getRootWindow,
rootWindow.restore(), rootWindow.show(), rootWindow.focus() references) and
ensure the catch logs the error and does not rethrow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c3be9576-e66d-452e-bb76-2a7160c86abe
📒 Files selected for processing (1)
src/ui/main/trayIcon.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript for all new code in this codebase unless explicitly told otherwise
Use Fuselage components from@rocket.chat/fuselagefor all UI work — only create custom components when Fuselage doesn't provide the needed functionality
Check Theme.d.ts for valid color tokens when working with Fuselage components
Use optional chaining with fallbacks for platform-specific APIs instead of mocks (e.g., process.getuid?.() ?? 1000) to ensure code works across all platforms without requiring mocks
TypeScript code must use strict mode
Use React functional components with hooks instead of class components
Redux actions must follow the FSA (Flux Standard Action) pattern
Use camelCase for file naming
Use PascalCase for component file names (React components)
Write self-documenting code through clear naming — avoid unnecessary comments
Files:
src/ui/main/trayIcon.ts
🔇 Additional comments (1)
src/ui/main/trayIcon.ts (1)
151-151: LGTM!
2 participants
Closes #2457
Summary by CodeRabbit