Chore: notifications url params and visibility#32
Conversation
…niesList and SideBar - Introduced NotificationsSideSection to display weekly email digest and manage notifications. - Integrated NotificationsSideSection into CompaniesList and SideBar for enhanced user experience. - Updated settings management to utilize new SettingsTab enum for better type safety.
- Deleted the useSettingsTab hook as it is no longer needed in the project, streamlining the codebase.
…on layout - Deleted SponsorSideSection from CompaniesList and SideBar components to streamline the UI. - Enhanced NotificationsSideSection layout for better user engagement, updating text and structure for clarity.
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on December 16. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
WalkthroughSponsorSideSection was removed and replaced with a new NotificationsSideSection used in CompaniesList and SideBar. Settings tab handling was converted to controlled state using a new SettingsTab enum and query-state utilities; types were adjusted and Settings page wrapped in Suspense. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as UI Components
participant Router as Next.js Router
participant Settings as Settings Page
participant QueryState as Query State
User->>UI: Click "Subscribe now" (NotificationsSideSection)
UI->>Router: Navigate to /settings?tab=NOTIFICATIONS
Router->>Settings: Load Settings page
Settings->>QueryState: read `tab` via settingsQueryStateKeys
QueryState-->>Settings: provide controlled tab value (NOTIFICATIONS)
Settings->>Settings: set Tabs `value` to NOTIFICATIONS
Note right of Settings: User interacts with controlled Tabs\non change -> update query state
Settings->>QueryState: update `tab` param on tab change
QueryState-->>Router: sync URL with selected tab
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
alexmarqs
changed the title
![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
🧹 Nitpick comments (1)
apps/web/src/components/settings/index.tsx (1)
34-38: Type assertion is safe but consider alternatives.The type assertion
value as SettingsTabon line 36 is safe becauseparseAsStringEnumvalidates the incoming value. However, consider whether the assertion is necessary—ifsettingsTab.tabis already typed asSettingsTab, the assertion might be redundant.If the types align correctly, you could simplify to:
- onValueChange={(value) => setSettingsTab({ tab: value as SettingsTab })} + onValueChange={(value) => setSettingsTab({ tab: value })}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/web/src/components/CompaniesList.tsx(2 hunks)apps/web/src/components/NotificationsSideSection.tsx(1 hunks)apps/web/src/components/SideBar.tsx(2 hunks)apps/web/src/components/SponsorSideSection.tsx(0 hunks)apps/web/src/components/settings/index.tsx(2 hunks)apps/web/src/lib/search-params.ts(3 hunks)apps/web/src/lib/types.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/web/src/components/SponsorSideSection.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
apps/web/src/components/CompaniesList.tsx (2)
apps/web/src/components/NotificationsSideSection.tsx (1)
NotificationsSideSection(6-27)apps/web/src/components/SponsorSideSection.tsx (1)
SponsorSideSection(3-27)
apps/web/src/components/settings/index.tsx (2)
apps/web/src/lib/types.ts (1)
SettingsTabs(32-37)apps/web/src/lib/search-params.ts (1)
settingsQueryStateKeys(42-46)
apps/web/src/components/SideBar.tsx (1)
apps/web/src/components/NotificationsSideSection.tsx (1)
NotificationsSideSection(6-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Web E2E Tests
🔇 Additional comments (8)
apps/web/src/components/CompaniesList.tsx (1)
12-12: LGTM—straightforward component replacement.The replacement of
SponsorSideSectionwithNotificationsSideSectionis clean and aligns with the PR's objective to introduce notification subscription UI.Also applies to: 92-92
apps/web/src/components/NotificationsSideSection.tsx (1)
6-27: LGTM—clean implementation with good enum usage.The component properly uses the
SettingsTab.NOTIFICATIONSenum for type-safe URL construction, and theprefetchprop on the Link component is appropriate for this navigation pattern.apps/web/src/components/SideBar.tsx (2)
1-1: LGTM—consistent component replacement.The replacement of
SponsorSideSectionwithNotificationsSideSectionis consistent with changes in other files.Also applies to: 12-12
6-6: Confirm whether categoryOptions addition aligns with PR scope.The
categoryOptionsprop is correctly implemented: it's properly defined in SideBar.tsx's type definition, passed through to SearchSideBar, and actively used to populate the category filter dropdown. The implementation mirrorslocationOptionsexactly. However, the core concern remains valid—verify that adding category filtering support to the search sidebar aligns with this PR's stated objective of "notifications url params and visibility," or whether this should be separated into a distinct PR.apps/web/src/lib/search-params.ts (2)
16-23: LGTM—well-structured enum and defaults.The
SettingsTabenum with lowercase values is appropriate for URL parameters, and thedefaultSettingsobject provides a sensible starting point.
41-52: LGTM—consistent with existing search params pattern.The settings query state infrastructure (keys, loader, cache) follows the same pattern as the existing
searchParamsQueryStateKeys, ensuring consistency across the codebase. The use ofparseAsStringEnumwithObject.values(SettingsTab)provides type-safe validation.apps/web/src/components/settings/index.tsx (1)
24-26: LGTM—good use of query state for controlled tabs.The
useQueryStateshook properly syncs tab state with URL parameters, and thescroll: trueoption provides good UX by scrolling to the top when tabs change.apps/web/src/lib/types.ts (1)
1-1: Review comment is incorrect.The changes do not represent a "renaming from
SettingsTabtoSettingsTabs." These are two distinct, complementary entities that coexist:
SettingsTab(enum in search-params.ts) provides the enum values (ACCOUNT, NOTIFICATIONS)SettingsTabs(type in types.ts) defines the tab object structure with anidfield typed asSettingsTabThe consuming code correctly uses both—the array is typed as
SettingsTabs[], tab values referenceSettingsTab.ACCOUNTandSettingsTab.NOTIFICATIONS, and type casts use the enum. This is not a breaking change requiring widespread refactoring.Likely an incorrect or invalid review comment.
- Wrapped the Settings component in Suspense for improved loading behavior. - Updated notification label for clarity in NotificationSettings component.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/web/src/app/settings/page.tsx (1)
8-10: Consider adding a loading fallback for better UX.The empty string fallback in Suspense will result in a blank screen while the Settings component is loading. Consider providing a loading skeleton or spinner to improve the user experience.
For example:
- <Suspense fallback=""> + <Suspense fallback={<div className="container mx-auto max-w-2xl p-6"><Skeleton className="h-40 w-full" /></div>}> <Settings /> </Suspense>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/src/app/settings/page.tsx(1 hunks)apps/web/src/components/settings/notification/NotificationSettings.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/app/settings/page.tsx (1)
apps/web/src/components/settings/index.tsx (1)
Settings(23-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Web E2E Tests
🔇 Additional comments (1)
apps/web/src/components/settings/notification/NotificationSettings.tsx (1)
15-15: Verify the label change aligns with notification behavior.The removal of "weekly" from the notification label suggests a change in email frequency or a shift to a more generic description. Please confirm that this updated text accurately reflects the actual notification behavior.
1 participant
Summary by CodeRabbit
New Features
Refactor
Bug Fixes / UX