Conversation
|
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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds AI review config and pretty-bytes; introduces SidenavWrapper and two navigation hooks; implements OS detection and downloadDesktopApp with tests and types; augments global ambient types; extends Redux to track userSubscription and updates tests and store setup. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Sidenav as SidenavWrapper
participant Redux as Redux Store
participant NavHook as useSidenavNavigation
participant SuiteHook as useSuiteLauncher
participant NavigationService
User->>Sidenav: mount / interact
activate Sidenav
Sidenav->>Redux: read user, storage, features
Redux-->>Sidenav: userState, storage metrics, feature flags
Sidenav->>NavHook: build items
NavHook-->>Sidenav: itemsNavigation
Sidenav->>SuiteHook: build suiteArray
SuiteHook-->>Sidenav: suiteArray
Sidenav-->>User: render UI
User->>NavHook: click item
NavHook->>NavigationService: navigate(id)
NavigationService-->>User: route change
deactivate Sidenav
sequenceDiagram
participant User
participant Config as ConfigService
participant OSDet as OS Detection
participant API as Download API
participant Notif as NotificationService
participant Browser
User->>Config: downloadDesktopApp(translate)
activate Config
Config->>OSDet: getOperatingSystem()
OSDet->>OSDet: evaluate userAgentData / userAgent / appVersion / opera
OSDet-->>Config: OperatingSystem
Config->>API: getDownloadAppUrl()
API-->>Config: platform-specific URL or null
alt URL found
Config->>Browser: window.open(url)
Browser-->>User: download initiated
else No URL / Unknown OS
Config->>Notif: show(translated error, ToastType.Error)
Notif-->>User: error toast
end
deactivate Config
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
src/hooks/navigation/useSidenavNavigation.tsx (2)
32-46: Consider swapping icons for Drafts and Sent.
PaperPlaneTiltIcon(paper airplane) semantically represents "sent" mail, whileFileIconis more neutral. Currently, Drafts uses the paper airplane and Sent uses the file icon, which seems inverted.♻️ Suggested icon swap
{ isActive: isActiveButton('/drafts'), label: translate('sidebar.drafts'), - icon: PaperPlaneTiltIcon, + icon: FileIcon, iconDataCy: 'sideNavDraftsIcon', isVisible: true, onClick: () => onSidenavItemClick(AppView.Drafts), }, { isActive: isActiveButton('/sent'), label: translate('sidebar.sent'), - icon: FileIcon, + icon: PaperPlaneTiltIcon, iconDataCy: 'sideNavSentIcon', isVisible: true, onClick: () => onSidenavItemClick(AppView.Sent), },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/navigation/useSidenavNavigation.tsx` around lines 32 - 46, The Drafts and Sent items use inverted icons; swap PaperPlaneTiltIcon and FileIcon so Drafts uses FileIcon and Sent uses PaperPlaneTiltIcon: update the objects where isActiveButton('/drafts') and onSidenavItemClick(AppView.Drafts) to reference FileIcon, and where isActiveButton('/sent') and onSidenavItemClick(AppView.Sent) to reference PaperPlaneTiltIcon (leave label, iconDataCy, isVisible, isActive and onClick unchanged).
15-20: Unnecessary singleton in dependency array.
NavigationService.instanceis a singleton that doesn't change between renders. Including it in the dependency array is unnecessary and may trigger ESLint warnings.♻️ Proposed fix
const onSidenavItemClick = useCallback( (path: AppView) => { NavigationService.instance.navigate({ id: path }); }, - [NavigationService.instance], + [], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/navigation/useSidenavNavigation.tsx` around lines 15 - 20, The dependency array for the useCallback on onSidenavItemClick incorrectly includes the singleton NavigationService.instance; remove it and replace with an empty array (or capture the stable navigate function outside the callback) so the callback does not re-create on each render—e.g., reference NavigationService.instance.navigate inside onSidenavItemClick and change the dependency array from [NavigationService.instance] to [] (or to [navigate] if you first assign const navigate = NavigationService.instance.navigate).src/services/config/index.ts (1)
63-68: Deprecated API usage is acceptable for fallback detection.The static analysis flags
navigator.vendor,navigator.appVersion, andwindow.operaas deprecated. However, these are used intentionally as fallbacks whenuserAgentDataisn't available (older browsers). This is a reasonable trade-off for broader compatibility.Consider adding a brief comment explaining the fallback chain for future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/config/index.ts` around lines 63 - 68, Add a short explanatory comment above the UA fallback logic describing the intentional use of deprecated properties as legacy fallbacks: note that navigator.userAgent, navigator.vendor, window.opera and navigator.appVersion are only used when userAgentData is unavailable to maximize compatibility; reference the matchPlatform function and UA_MATCHERS/APP_VERSION_MATCHERS so future maintainers understand the fallback chain and rationale for keeping these deprecated API accesses.
🤖 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/components/sidenav/SidenavWrapper.tsx`:
- Around line 80-83: The SidenavWrapper component already reads Redux via
useAppSelector, so remove the redundant connect HOC and its mapState mapping
(the export default connect((state: RootState) => ({ user: state.user.user,
userTier: state.user.userTier }))(SidenavWrapper);). Replace with a plain export
default SidenavWrapper and remove the unused mapped props and any now-unused
imports (e.g., connect and RootState) to avoid dead code; keep the component
body and useAppSelector calls as-is.
- Line 70: In SidenavWrapper.tsx, the percentage calculation "percentage:
Math.min((planUsage / planLimit) * 100, 100)" can produce Infinity when
planLimit is 0; replace it with a guarded calculation that uses a safe
denominator (e.g., const safeLimit = planLimit > 0 ? planLimit : 1) or an inline
ternary and then compute percentage as Math.min((planUsage / safeLimit) * 100,
100); ensure the guard uses planLimit > 0 so negative/zero limits don't divide
and the UI shows a bounded percentage.
In `@src/globals.d.ts`:
- Line 6: The execute function type in globals.d.ts uses an invalid anonymous
destructured parameter "{ action: string }"; update the signature for the
execute symbol to use a named second parameter with an object type (e.g., params
or options) so it reads something like execute: (siteKey: string, params: {
action: string }) => Promise<string>, ensuring the parameter name replaces the
invalid inline brace syntax.
In `@src/hooks/navigation/useSidenavNavigation.tsx`:
- Line 13: The isActiveButton callback is passing arguments to matchPath in the
wrong order; swap them so the pattern comes first and the pathname second.
Update the useCallback line in isActiveButton to call matchPath(path, pathname)
(keeping the !! cast and the [pathname] dependency) so route matching uses the
correct signature of matchPath(pattern, pathname).
In `@src/hooks/navigation/useSuiteLauncher.tsx`:
- Around line 30-33: The two alert() calls in useSuiteLauncher (using
suite.upgradeTitle and suite.upgradeDescription) should be replaced with your
app's upgrade modal/dialog flow: remove the alert(...) lines and instead invoke
the existing modal API or upgrade action (e.g., call openUpgradeModal /
showUpgradeDialog / dispatch openModal with a payload containing
suite.upgradeTitle and suite.upgradeDescription) so the UI shows a styled modal
with title, description and action buttons; ensure you import the modal helper
or use the existing context/hook used elsewhere for opening modals and pass the
suite data and any callbacks (confirm/cancel) to match the current upgrade UX.
In `@src/services/config/index.ts`:
- Around line 71-90: getDownloadAppUrl currently performs fetch and JSON.parse
without guards; wrap the network + parse steps in a try/catch, check
fetchDownloadResponse.ok before calling .json(), and throw a descriptive Error
(including status and response text when non-ok or when JSON parsing fails) so
callers like downloadDesktopApp can surface a notification; keep existing
fallback URLs and the getOperatingSystem switch, but return null only for
unknown OS and rethrow or propagate errors for network/parse failures so they
are handled upstream.
In `@src/utils/bytesToString.test.ts`:
- Around line 9-12: The test for bytesToString incorrectly passes only {
nonBreakingSpace: true } so no visible change occurs; update the test to enable
space as well and assert the actual non-breaking space character: call
bytesToString(1024 * 1024 * 1024, { space: true, nonBreakingSpace: true }) and
expect the result to include the U+00A0 character (e.g. '1.07\u00A0GB') to
verify the nonBreakingSpace behavior.
---
Nitpick comments:
In `@src/hooks/navigation/useSidenavNavigation.tsx`:
- Around line 32-46: The Drafts and Sent items use inverted icons; swap
PaperPlaneTiltIcon and FileIcon so Drafts uses FileIcon and Sent uses
PaperPlaneTiltIcon: update the objects where isActiveButton('/drafts') and
onSidenavItemClick(AppView.Drafts) to reference FileIcon, and where
isActiveButton('/sent') and onSidenavItemClick(AppView.Sent) to reference
PaperPlaneTiltIcon (leave label, iconDataCy, isVisible, isActive and onClick
unchanged).
- Around line 15-20: The dependency array for the useCallback on
onSidenavItemClick incorrectly includes the singleton
NavigationService.instance; remove it and replace with an empty array (or
capture the stable navigate function outside the callback) so the callback does
not re-create on each render—e.g., reference NavigationService.instance.navigate
inside onSidenavItemClick and change the dependency array from
[NavigationService.instance] to [] (or to [navigate] if you first assign const
navigate = NavigationService.instance.navigate).
In `@src/services/config/index.ts`:
- Around line 63-68: Add a short explanatory comment above the UA fallback logic
describing the intentional use of deprecated properties as legacy fallbacks:
note that navigator.userAgent, navigator.vendor, window.opera and
navigator.appVersion are only used when userAgentData is unavailable to maximize
compatibility; reference the matchPlatform function and
UA_MATCHERS/APP_VERSION_MATCHERS so future maintainers understand the fallback
chain and rationale for keeping these deprecated API accesses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a7739db7-e09f-430e-95c1-5429265c61f3
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (17)
.coderabbit.yamlpackage.jsonsrc/components/notifications/NotificationToast.tsxsrc/components/sidenav/SidenavWrapper.tsxsrc/constants.tssrc/globals.d.tssrc/hooks/auth/useAuth.tssrc/hooks/navigation/useSidenavNavigation.tsxsrc/hooks/navigation/useSuiteLauncher.tsxsrc/routes/layouts/SidebarAndHeaderLayout.tsxsrc/services/config/config.service.test.tssrc/services/config/index.tssrc/store/slices/user/index.tssrc/test-utils/createTestStore.tssrc/types/config/index.tssrc/utils/bytesToString.test.tssrc/utils/bytesToString.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/hooks/navigation/useSuiteLauncher.tsx (1)
22-31:⚠️ Potential issue | 🟠 MajorLocked suite clicks are silent; disabled path should trigger the upgrade flow.
openSuiteis a no-op whenenabledis false, so locked suites currently provide no feedback even thoughupgradeTitle/upgradeDescriptionare passed everywhere.💡 Suggested direction
const openSuite = (suite: { enabled: boolean; onOpenSuite: () => void; upgradeTitle: string; upgradeDescription: string; + onUpgrade: (payload: { title: string; description: string }) => void; }) => { if (suite.enabled) { suite.onOpenSuite(); + } else { + suite.onUpgrade({ + title: suite.upgradeTitle, + description: suite.upgradeDescription, + }); } };Wire
onUpgradeto your existing modal/dialog action used elsewhere in the app.Also applies to: 46-51, 72-82, 89-96, 103-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/navigation/useSuiteLauncher.tsx` around lines 22 - 31, The openSuite handler currently does nothing for disabled suites; update openSuite to call suite.onOpenSuite() when suite.enabled is true, and when false call a provided upgrade action (e.g., suite.onUpgrade(...) or a common showUpgradeModal) passing suite.upgradeTitle and suite.upgradeDescription so locked suites trigger the upgrade flow; apply the same change to the other similar handlers in this file (the other open* functions around the file) so all paths wire disabled suites to the upgrade modal/action instead of being silent.
🤖 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/hooks/navigation/useSuiteLauncher.tsx`:
- Around line 47-53: The code reads nested feature flags like
userFeatures?.[Service.Meet].enabled which can throw if the service entry is
missing; update all such accesses (e.g., in useSuiteLauncher.tsx where
userFeatures, Service.Meet and isLocked are used) to guard the property access
with optional chaining before .enabled (e.g.,
userFeatures?.[Service.Meet]?.enabled) and apply the same change to the other
occurrences called out (lines around the blocks using Service.* and .enabled) so
that enabled checks and isLocked assignments use ?.enabled rather than .enabled.
---
Duplicate comments:
In `@src/hooks/navigation/useSuiteLauncher.tsx`:
- Around line 22-31: The openSuite handler currently does nothing for disabled
suites; update openSuite to call suite.onOpenSuite() when suite.enabled is true,
and when false call a provided upgrade action (e.g., suite.onUpgrade(...) or a
common showUpgradeModal) passing suite.upgradeTitle and suite.upgradeDescription
so locked suites trigger the upgrade flow; apply the same change to the other
similar handlers in this file (the other open* functions around the file) so all
paths wire disabled suites to the upgrade modal/action instead of being silent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c2b1fc6-bed4-4eb5-b766-553f5526d578
📒 Files selected for processing (6)
src/components/sidenav/SidenavWrapper.tsxsrc/globals.d.tssrc/hooks/navigation/useSidenavNavigation.tsxsrc/hooks/navigation/useSuiteLauncher.tsxsrc/services/config/index.tssrc/utils/bytesToString.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/hooks/navigation/useSidenavNavigation.tsx
- src/services/config/index.ts
- src/components/sidenav/SidenavWrapper.tsx
- src/globals.d.ts
- src/utils/bytesToString.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/hooks/navigation/useSuiteLauncher.tsx (2)
104-104:⚠️ Potential issue | 🟠 MajorUnsafe nested access can throw for missing Cleaner feature key.
Line 104 uses
userFeatures?.[Service.Cleaner].enabledwithout guarding the service entry itself. IfCleaneris absent, this can throw at runtime.Safe access fix
- enabled: userFeatures?.[Service.Cleaner].enabled ?? false, + enabled: userFeatures?.[Service.Cleaner]?.enabled ?? false,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/navigation/useSuiteLauncher.tsx` at line 104, The current expression reads the Cleaner service entry then .enabled directly, which can throw if userFeatures lacks the Service.Cleaner key; update the initializer for the enabled flag in useSuiteLauncher to safely guard the service object (e.g., check that userFeatures?.[Service.Cleaner] exists before reading .enabled) and fall back to false — locate the enabled: assignment referencing Service.Cleaner in useSuiteLauncher.tsx and replace it with a guarded access that uses optional chaining or an existence check on the service entry before reading .enabled, with a final nullish/default fallback of false.
22-31:⚠️ Potential issue | 🟠 MajorLocked suites currently fail silently instead of triggering upgrade UX.
openSuiteignoresupgradeTitleandupgradeDescriptionwhen disabled, so clicking locked suites does nothing (Line 22–31). This leaves the upgrade path unimplemented.Suggested fix
const openSuite = (suite: { enabled: boolean; onOpenSuite: () => void; upgradeTitle: string; upgradeDescription: string; }) => { if (suite.enabled) { suite.onOpenSuite(); + return; } + + // Wire this into the existing app modal/dialog flow. + // Example: + // openUpgradeModal({ + // title: suite.upgradeTitle, + // description: suite.upgradeDescription, + // }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/navigation/useSuiteLauncher.tsx` around lines 22 - 31, openSuite currently no-ops for locked suites; change it so when suite.enabled is false it triggers the upgrade UX using the suite.upgradeTitle and suite.upgradeDescription instead of doing nothing. Specifically, in the openSuite function check if (!suite.enabled) and call your app's upgrade modal/flow (e.g., showUpgradeModal or openUpgradePrompt) passing suite.upgradeTitle and suite.upgradeDescription; keep the existing behavior of calling suite.onOpenSuite() when enabled. Ensure you reference openSuite, suite.enabled, suite.onOpenSuite, suite.upgradeTitle and suite.upgradeDescription when implementing the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/hooks/navigation/useSuiteLauncher.tsx`:
- Line 104: The current expression reads the Cleaner service entry then .enabled
directly, which can throw if userFeatures lacks the Service.Cleaner key; update
the initializer for the enabled flag in useSuiteLauncher to safely guard the
service object (e.g., check that userFeatures?.[Service.Cleaner] exists before
reading .enabled) and fall back to false — locate the enabled: assignment
referencing Service.Cleaner in useSuiteLauncher.tsx and replace it with a
guarded access that uses optional chaining or an existence check on the service
entry before reading .enabled, with a final nullish/default fallback of false.
- Around line 22-31: openSuite currently no-ops for locked suites; change it so
when suite.enabled is false it triggers the upgrade UX using the
suite.upgradeTitle and suite.upgradeDescription instead of doing nothing.
Specifically, in the openSuite function check if (!suite.enabled) and call your
app's upgrade modal/flow (e.g., showUpgradeModal or openUpgradePrompt) passing
suite.upgradeTitle and suite.upgradeDescription; keep the existing behavior of
calling suite.onOpenSuite() when enabled. Ensure you reference openSuite,
suite.enabled, suite.onOpenSuite, suite.upgradeTitle and
suite.upgradeDescription when implementing the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e92ce06b-25b7-463c-87bf-8f71671e4397
📒 Files selected for processing (1)
src/hooks/navigation/useSuiteLauncher.tsx
|
larryrider
approved these changes
Mar 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Sidebar component has been added.
Summary by CodeRabbit
New Features
Bug Fixes / UX
Tests
Chores