feat: redesign notification modal - #8140
Merged
Merged
Conversation
Co-authored-by: Prateek Sunal <41370460+prateekmedia@users.noreply.github.com>
Co-authored-by: Prateek Sunal <41370460+prateekmedia@users.noreply.github.com>
Co-authored-by: Prateek Sunal <41370460+prateekmedia@users.noreply.github.com>
Co-authored-by: Prateek Sunal <41370460+prateekmedia@users.noreply.github.com>
Co-authored-by: Prateek Sunal <41370460+prateekmedia@users.noreply.github.com>
Co-authored-by: Prateek Sunal <41370460+prateekmedia@users.noreply.github.com>
Co-authored-by: Prateek Sunal <41370460+prateekmedia@users.noreply.github.com>
Co-authored-by: Prateek Sunal <41370460+prateekmedia@users.noreply.github.com>
Co-authored-by: Prateek Sunal <41370460+prateekmedia@users.noreply.github.com>
prateek-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno and
sid-bruno
as code owners
May 28, 2026 21:40
Contributor
|
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:
WalkthroughRefactors notifications into a hook-driven modal UI with a resizable sidebar (Tabs/List/Detail), adds NotificationList/NotificationTabs/NotificationDetail components, persists cleared IDs in Redux, switches Electron IPC to push, updates modal padding/icon, theme/schema entries, and a Playwright test. ChangesNotifications Feature Refactor
Estimated code review effortðŊ 4 (Complex) | âąïļ ~45 minutes Possibly related PRs
Suggested reviewers
ðĨ Pre-merge checks | â 5â Passed checks (5 passed)
âïļ Tip: You can configure your own custom pre-merge checks in the settings. âĻ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
ð§đ Nitpick comments (2)
packages/bruno-app/src/components/Notifications/NotificationsModal/StyledWrapper.js (1)
218-224: ⥠Quick winMove the default badge color into the theme.
#8d43b1(and the matching rgba) are hardcoded here while everydata-variantbelow pulls fromtheme.*. Hardcoded hex won't adapt across themes and bypasses the theme system.The same applies to the literal
white/rgba(...)values at lines 76, 80-82 and 93 â prefer routing them throughprops.theme.*.As per coding guidelines: "Use styled component's theme prop to manage CSS colors and not CSS variables when in the context of a styled component".
ðĪ 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 `@packages/bruno-app/src/components/Notifications/NotificationsModal/StyledWrapper.js` around lines 218 - 224, The .notif-type-badge styles in StyledWrapper.js use hardcoded colors (`#8d43b1` and rgba(141,67,177,0.10)) and several other literal values (white/rgba at the other notification style blocks) which bypass the theme; replace these literals with theme properties (e.g. props.theme.notifications.list.badgeColor and props.theme.notifications.list.badgeBackground or similar keys) and update the other places that use literal "white"/rgba to use appropriate props.theme.* values so all colors come from the styled-components theme; ensure the new theme keys exist or are added to the theme object so the components continue to render correctly.packages/bruno-app/src/components/Notifications/hooks/useNotifications.js (1)
33-45: âïļ Poor tradeoffSelection-sync effect: confirm the auto-mark-read-on-open is intended.
This effect marks the most-recent item as read merely by opening the panel (no explicit user read). That matches the "auto-mark on visibility" intent in the summary, so flagging only to confirm it's deliberate.
Also, per repo guidance to avoid
useEffectwhere feasible, the open/tab-entry selection could move intoopen()/onTabChange, leaving the effect to only react to external Redux-driven list changes (clear/markAll). Not blocking.As per coding guidelines: "MUST: Avoid
useEffectunless absolutely needed. Prefer derived state and event handlers in React".ðĪ 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 `@packages/bruno-app/src/components/Notifications/hooks/useNotifications.js` around lines 33 - 45, The current useEffect both selects the first listed notification on open and auto-marks it read; refactor so selection-and-auto-read on panel open or tab change is handled inside the open()/onTabChange handlers (call setSelectedNotification(first) and dispatch(markNotificationAsRead({ notificationId: first.id })) there when appropriate), and reduce the useEffect to only respond to external list changes (clears/markAll) to keep selection in sync (use listed and selectedNotification checks but do not perform open-time selection/marking inside useEffect); also make the auto-mark-read behavior explicit/configurable (e.g., comment or flag) so itâs clear this is intentional.
ðĪ 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 `@packages/bruno-app/src/components/Notifications/index.js`:
- Around line 13-18: The trigger element currently uses StyledWrapper (a
styled.a) which is not keyboard-focusable without an href; update StyledWrapper
in StyledWrapper.js to be a styled.button (preserving existing styles) and
ensure any instances (like the one rendering IconBell with onClick={open}) pass
type="button" and retain aria-label; also remove any href usage and ensure the
component forwards onClick and onKeyDown so the open handler is
keyboard-activatable via Enter/Space.
In
`@packages/bruno-app/src/components/Notifications/NotificationsModal/NotificationList.js`:
- Around line 11-18: The list item is only mouse-clickable; make rows
keyboard-accessible by adding role="button" and tabIndex={0} to the <li> with
className 'notif-list-item' (keyed by notification.id), and implement an
onKeyDown that calls the existing onSelect(notification) when Enter or Space is
pressed (preventDefault for Space to avoid scrolling); keep the existing onClick
handler and ensure you reference the same onSelect prop and notification object
so keyboard and mouse activation behave identically.
In `@packages/bruno-app/src/components/Notifications/StyledWrapper.js`:
- Around line 3-5: The notifications trigger is currently a non-focusable
styled.a used with onClick={open} in index.js; change StyledWrapper from
styled.a to styled.button so it becomes keyboard-focusable/activatable and
ensure it has type="button" to avoid form submit behavior; reset browser button
chrome inside StyledWrapper (e.g., background: none; border: none; padding: 0;
line-height: inherit; display: inline-flex; cursor: pointer;) so visual/layout
stays the same and retain any ARIA/props and the existing onClick={open} usage.
In `@packages/bruno-electron/src/ipc/notifications.js`:
- Around line 20-23: The did-finish-load handler is causing repeated
pushNotifications on reloads; update registerNotificationsIpc to guard so
pushNotifications(mainWindow) runs only once per window (e.g., replace
mainWindow.webContents.on('did-finish-load', ...) with
mainWindow.webContents.once('did-finish-load', ...) or set a boolean flag like
mainWindow.__notificationsPushed and check it before calling pushNotifications),
and keep ipcMain.on('renderer:notifications-opened', ...) as-is unless you later
observe duplicate listenersâensure you reference the registerNotificationsIpc
function and pushNotifications symbol when making the change.
---
Nitpick comments:
In `@packages/bruno-app/src/components/Notifications/hooks/useNotifications.js`:
- Around line 33-45: The current useEffect both selects the first listed
notification on open and auto-marks it read; refactor so selection-and-auto-read
on panel open or tab change is handled inside the open()/onTabChange handlers
(call setSelectedNotification(first) and dispatch(markNotificationAsRead({
notificationId: first.id })) there when appropriate), and reduce the useEffect
to only respond to external list changes (clears/markAll) to keep selection in
sync (use listed and selectedNotification checks but do not perform open-time
selection/marking inside useEffect); also make the auto-mark-read behavior
explicit/configurable (e.g., comment or flag) so itâs clear this is intentional.
In
`@packages/bruno-app/src/components/Notifications/NotificationsModal/StyledWrapper.js`:
- Around line 218-224: The .notif-type-badge styles in StyledWrapper.js use
hardcoded colors (`#8d43b1` and rgba(141,67,177,0.10)) and several other literal
values (white/rgba at the other notification style blocks) which bypass the
theme; replace these literals with theme properties (e.g.
props.theme.notifications.list.badgeColor and
props.theme.notifications.list.badgeBackground or similar keys) and update the
other places that use literal "white"/rgba to use appropriate props.theme.*
values so all colors come from the styled-components theme; ensure the new theme
keys exist or are added to the theme object so the components continue to render
correctly.
ðŠ 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d90a6b52-35c6-457a-9dbd-4d9a0d08e4d3
ð Files selected for processing (14)
packages/bruno-app/src/components/Modal/index.jspackages/bruno-app/src/components/Notifications/NotificationsModal/NotificationDetail.jspackages/bruno-app/src/components/Notifications/NotificationsModal/NotificationList.jspackages/bruno-app/src/components/Notifications/NotificationsModal/NotificationTabs.jspackages/bruno-app/src/components/Notifications/NotificationsModal/StyledWrapper.jspackages/bruno-app/src/components/Notifications/NotificationsModal/index.jspackages/bruno-app/src/components/Notifications/StyleWrapper.jspackages/bruno-app/src/components/Notifications/StyledWrapper.jspackages/bruno-app/src/components/Notifications/hooks/useNotifications.jspackages/bruno-app/src/components/Notifications/index.jspackages/bruno-app/src/providers/App/useIpcEvents.jspackages/bruno-app/src/providers/ReduxStore/slices/notifications.jspackages/bruno-electron/src/ipc/notifications.jstests/footer/notifications/notifications.spec.js
ðĪ Files with no reviewable changes (1)
- packages/bruno-app/src/components/Notifications/StyleWrapper.js
Co-authored-by: Prateek Sunal <41370460+prateekmedia@users.noreply.github.com>
Contributor
|
Actionable comments posted: 0 |
Co-authored-by: Prateek Sunal <41370460+prateekmedia@users.noreply.github.com>
Contributor
|
Actionable comments posted: 0 |
Co-authored-by: Prateek Sunal <41370460+prateekmedia@users.noreply.github.com>
Co-authored-by: Prateek Sunal <41370460+prateekmedia@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
âŧïļ Duplicate comments (1)
packages/bruno-app/src/components/Notifications/StyledWrapper.js (1)
3-3:â ïļ Potential issue | ðĄ Minor | ⥠Quick winAdd explicit button type to prevent accidental form submit
StyledWrapperis now a<button>, but without an explicit type it defaults to submit in forms. Please settype='button'(e.g., viaattrs) at Line 3.Suggested patch
-const StyledWrapper = styled.button` +const StyledWrapper = styled.button.attrs({ type: 'button' })`ðĪ 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 `@packages/bruno-app/src/components/Notifications/StyledWrapper.js` at line 3, StyledWrapper is defined as a styled.button and lacks an explicit button type, which can make it default to type="submit" inside forms; update the StyledWrapper definition (the styled.button) to set type='button' (for example by using styled.button.attrs({ type: 'button' }) or equivalent) so the component will not accidentally submit forms.
ðĪ 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 `@packages/bruno-app/src/components/Notifications/StyledWrapper.js`:
- Line 3: StyledWrapper is defined as a styled.button and lacks an explicit
button type, which can make it default to type="submit" inside forms; update the
StyledWrapper definition (the styled.button) to set type='button' (for example
by using styled.button.attrs({ type: 'button' }) or equivalent) so the component
will not accidentally submit forms.
âđïļ Review info
âïļ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3f50abdb-fbfa-47f4-8d94-9b422d488f80
ð Files selected for processing (2)
packages/bruno-app/src/components/Notifications/NotificationsModal/NotificationList.jspackages/bruno-app/src/components/Notifications/StyledWrapper.js
ð§ Files skipped from review as they are similar to previous changes (1)
- packages/bruno-app/src/components/Notifications/NotificationsModal/NotificationList.js
Co-authored-by: Prateek Sunal <41370460+prateekmedia@users.noreply.github.com>
Co-authored-by: Prateek Sunal <41370460+prateekmedia@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ðĪ 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
`@packages/bruno-app/src/components/Notifications/NotificationsModal/NotificationDetail.js`:
- Around line 5-8: The getBadgeStyle helper should guard against unknown remote
colors to avoid throwing when accessing theme.status[color]; update
getBadgeStyle (and any callers that pass notification.color) to first check that
theme.status has the requested color (e.g., via
Object.prototype.hasOwnProperty.call(theme.status, color) or optional chaining)
and fall back to a safe default key like "info" (or a default background/text
pair) when missing; ensure you return an object with backgroundColor and color
using the validated key (or default values) so
theme.status[color].background/.text never accesses undefined.
ðŠ 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 97964532-70f0-4389-b835-b2545761a2d1
ð Files selected for processing (5)
packages/bruno-app/src/components/Notifications/NotificationsModal/NotificationDetail.jspackages/bruno-app/src/components/Notifications/NotificationsModal/NotificationTabs.jspackages/bruno-app/src/components/Notifications/NotificationsModal/StyledWrapper.jspackages/bruno-app/src/components/Notifications/NotificationsModal/index.jspackages/bruno-app/src/components/Notifications/StyledWrapper.js
ð§ Files skipped from review as they are similar to previous changes (2)
- packages/bruno-app/src/components/Notifications/NotificationsModal/NotificationTabs.js
- packages/bruno-app/src/components/Notifications/NotificationsModal/index.js
Co-authored-by: Prateek Sunal <41370460+prateekmedia@users.noreply.github.com>
naman-bruno
previously approved these changes
Jun 18, 2026
sid-bruno
requested changes
Jun 18, 2026
Co-authored-by: Sid <siddharth@usebruno.com>
Co-authored-by: Prateek Sunal <41370460+prateekmedia@users.noreply.github.com>
Co-authored-by: Prateek Sunal <41370460+prateekmedia@users.noreply.github.com>
sid-bruno
approved these changes
Jun 18, 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
4 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.
Description
Jira
Redesign notification modal.
Screen.Recording.2026-06-18.at.9.26.32.PM.mov
Contribution Checklist:
Summary by CodeRabbit
New Features
Improvements
Refactor
Tests