feat: add speech bubble notifications#7066
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
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:
WalkthroughAdds typing and delay hooks, splits the speech-bubble UI into separate hiring and notification banners, introduces notification location typing, new notification filters/hooks/atoms, updates sidebar notification handling and header integration, and adjusts bubble styling and CowSpeechBubble public props. Changes
Sequence DiagramsequenceDiagram
participant AppContainer as AppContainer
participant Hook as useSpeechBubbleNotification
participant Store as NotificationsStore
participant BannerH as HiringBanner
participant BannerN as NotificationBanner
participant UI as User
AppContainer->>Hook: request currentNotification & dismiss()
Hook->>Store: read notifications + unread set
Store-->>Hook: notifications list
Hook->>Hook: filter by location='speechBubble'\n sort by createdAt desc
Hook-->>AppContainer: currentNotification or null
alt currentNotification exists
AppContainer->>BannerN: render NotificationBanner(currentNotification)
BannerN->>UI: display title, description, optional link
else no currentNotification
AppContainer->>BannerH: render HiringBanner()
BannerH->>BannerH: useDelay(delayMs) -> hasDelayElapsed
BannerH->>BannerH: useTypingProgress(message, hasDelayElapsed) -> charIndex
BannerH->>UI: animated typing, reveal "View jobs" link
end
UI->>BannerN: user closes or clicks link
UI->>BannerH: user closes or clicks link
BannerN->>Hook: dismiss() (mark as read)
Hook->>Store: markNotificationsAsRead(...)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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. ✨ 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/cowswap-frontend/src/modules/notifications/state/readNotificationsAtom.ts (1)
12-26:⚠️ Potential issue | 🟠 Major
markNotificationsAsReadAtommutates persisted state in-place; two atoms with diverging semantics now coexist
state.push(...)on line 15 mutates the array retrieved fromreadNotificationsAtomand then callsset(readNotificationsAtom, state)with the same reference. Referential equality is important in Jotai, and sinceObject.is(state, state) === true, the store treats the atom as unchanged and will skip notifying dependents — meaning sidebar-notification read state may silently fail to propagate to subscribers (e.g. unread badge counts).
markNotificationsAsReadCloneArrayAtom(introduced here) is the correct approach — it creates a newSet/array so Jotai sees a fresh reference. However,NotificationsList/index.tsxstill imports and uses the old, mutating atom, leaving the sidebar path broken while speech-bubble notifications use the correct one.Recommended resolution: replace the body of
markNotificationsAsReadAtomwith the clone approach and deletemarkNotificationsAsReadCloneArrayAtom, then update all callers. If the two atoms really must coexist for a transitional period, renamemarkNotificationsAsReadCloneArrayAtomto something that describes intent, not implementation (e.g.markSpeechBubbleNotificationsAsReadAtom).🔧 Proposed fix — merge both atoms into one correct implementation
export const markNotificationsAsReadAtom = atom(null, (get, set, ids: number[]) => { - const state = get(readNotificationsAtom) - - state.push(...ids.filter((id) => !state.includes(id))) - - set(readNotificationsAtom, state) + const state = new Set(get(readNotificationsAtom)) + + ids.forEach((id) => state.add(id)) + + set(readNotificationsAtom, [...state]) }) - -export const markNotificationsAsReadCloneArrayAtom = atom(null, (get, set, ids: number[]) => { - const state = new Set(get(readNotificationsAtom)) - - ids.forEach((id) => state.add(id)) - - set(readNotificationsAtom, [...state]) -})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cowswap-frontend/src/modules/notifications/state/readNotificationsAtom.ts` around lines 12 - 26, markNotificationsAsReadAtom currently mutates the array in-place (state.push(...)) causing Jotai to skip updates; replace its body with the clone/set approach used in markNotificationsAsReadCloneArrayAtom (create new Set(get(readNotificationsAtom)), add ids, then set(readNotificationsAtom, [...state])) to ensure a new reference is written, remove markNotificationsAsReadCloneArrayAtom and update all callers (e.g. NotificationsList/index.tsx and any speech-bubble notification callers) to use markNotificationsAsReadAtom — if you must keep both during a transition, rename markNotificationsAsReadCloneArrayAtom to a clear intent-based name like markSpeechBubbleNotificationsAsReadAtom and update callers accordingly.
🧹 Nitpick comments (4)
libs/core/src/cms/types.ts (1)
40-42: Add a constant for'speechBubble'to matchNOTIFICATION_LOCATION_DEFAULT
NOTIFICATION_LOCATION_DEFAULTis exported for'default'but there is no corresponding constant for'speechBubble'. As a result, filter predicates infilterNotifications.utils.tsand any future code hardcode the raw string'speechBubble'. Adding the companion constant keeps naming consistent and prevents string drift.🔧 Proposed addition
export const NOTIFICATION_LOCATION_DEFAULT: NotificationLocation = 'default' +export const NOTIFICATION_LOCATION_SPEECH_BUBBLE: NotificationLocation = 'speechBubble'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/core/src/cms/types.ts` around lines 40 - 42, Add a companion exported constant for the 'speechBubble' NotificationLocation to match NOTIFICATION_LOCATION_DEFAULT: define and export e.g. NOTIFICATION_LOCATION_SPEECH_BUBBLE with type NotificationLocation and value 'speechBubble' so callers (and filterNotifications.utils.ts) can reference the constant instead of hardcoding the string; update any internal usages to use NOTIFICATION_LOCATION_SPEECH_BUBBLE where appropriate.apps/cowswap-frontend/src/modules/notifications/utils/filterNotifications.utils.ts (1)
3-9: Use theNOTIFICATION_LOCATION_SPEECH_BUBBLEconstant and defineisSidebarNotificationas the complement ofisSpeechBubbleNotificationBoth predicates hardcode
'speechBubble'instead of importing the constant from@cowprotocol/core. Additionally, defining both predicates independently means a future addition toNotificationLocation(e.g.'modal') could silently route unexpected locations to the sidebar without a compile-time warning.🔧 Proposed refactor
-import { NotificationModel } from '@cowprotocol/core' +import { NOTIFICATION_LOCATION_SPEECH_BUBBLE, NotificationModel } from '@cowprotocol/core' -export function isSidebarNotification(notification: NotificationModel): boolean { - return notification.location !== 'speechBubble' -} - export function isSpeechBubbleNotification(notification: NotificationModel): boolean { - return notification.location === 'speechBubble' + return notification.location === NOTIFICATION_LOCATION_SPEECH_BUBBLE +} + +export function isSidebarNotification(notification: NotificationModel): boolean { + return !isSpeechBubbleNotification(notification) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cowswap-frontend/src/modules/notifications/utils/filterNotifications.utils.ts` around lines 3 - 9, Replace the hardcoded 'speechBubble' checks by importing NOTIFICATION_LOCATION_SPEECH_BUBBLE from `@cowprotocol/core` and use it inside isSpeechBubbleNotification(notification: NotificationModel); then implement isSidebarNotification(notification: NotificationModel) as the logical complement of isSpeechBubbleNotification (e.g., return !isSpeechBubbleNotification(notification)) so future NotificationLocation variants (like 'modal') won't be silently misrouted; update the functions isSpeechBubbleNotification and isSidebarNotification accordingly.apps/cowswap-frontend/src/common/utils/startTypingAnimation.ts (1)
1-1: Revert the type import regression
DispatchandSetStateActionare TypeScript interfaces with no runtime representation. The AI summary notes this was intentionally changed fromimport typeto a value import — that's a step backward: value imports for type-only constructs increase bundle metadata and obscure intent.🔧 Proposed fix
-import { Dispatch, SetStateAction } from 'react' +import type { Dispatch, SetStateAction } from 'react'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cowswap-frontend/src/common/utils/startTypingAnimation.ts` at line 1, The import of React types is currently a runtime value import; change it to a type-only import so Dispatch and SetStateAction are erased at runtime—replace the value import of Dispatch and SetStateAction with a type import (i.e., use a type-only import for Dispatch and SetStateAction) in the file containing startTypingAnimation to avoid bundling runtime metadata while keeping the same identifiers.apps/cowswap-frontend/src/modules/application/containers/AppContainer/AppContainer.container.tsx (1)
186-201: HoistuseSpeechBubbleNotification()fromFooterSectionintoAppContainerand thread results down as props.
FooterSectionis a module-internal helper, but invoking a domain-specific hook here means it always runs even whenshow === false(injected widget mode), and it couples the helper to the notification layer. Per the guideline, non-built-in state should be hoisted to the caller and passed via props.♻️ Proposed refactor sketch
In
FooterSectionProps:interface FooterSectionProps { show: boolean showCowSpeechBubble: boolean pageScene: ReactNode | null + currentNotification: NotificationModel | null + onDismissNotification: () => void }In
AppContainer:+ const { currentNotification, dismiss } = useSpeechBubbleNotification() // ... <FooterSection show={!isInjectedWidgetMode} showCowSpeechBubble={shouldRenderCowSpeechBubble} pageScene={pageScene} + currentNotification={currentNotification} + onDismissNotification={dismiss} />In
FooterSection: remove theuseSpeechBubbleNotification()call and use props instead.As per coding guidelines, "Pure React components can use only built-in hooks, hooks that provide state of the current module, and common state providing hooks like
useTheme(); hoist other dependencies into callers and pass results via props."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cowswap-frontend/src/modules/application/containers/AppContainer/AppContainer.container.tsx` around lines 186 - 201, The FooterSection helper currently calls the domain hook useSpeechBubbleNotification() internally which runs even when show === false and couples the helper to notification state; move the hook call up into AppContainer, add currentNotification and dismiss to FooterSectionProps, and pass those props into FooterSection so FooterSection no longer invokes useSpeechBubbleNotification(); inside FooterSection replace the local hook usage with the passed-in currentNotification and dismiss when rendering CowSpeechBubbleNotificationBanner or CowSpeechBubbleHiringBanner (also ensure showCowSpeechBubble prop is still honored).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/cowswap-frontend/src/common/hooks/useDelay.ts`:
- Around line 6-14: The effect in useDelay does not reset hasDelayElapsed when
delayMs changes, so update the useEffect in the useDelay hook to explicitly
reset state when starting a new positive delay: if delayMs is a positive number
call setHasDelayElapsed(false) at the top of the effect before creating the
timeout (keeping the existing typeof/<=0 guard), then start the timeout which
will call setHasDelayElapsed(true) and still clear the timeout in the existing
cleanup; this ensures hasDelayElapsed (and the setHasDelayElapsed state updates)
correctly reflect new delayMs changes for the useDelay hook.
In
`@apps/cowswap-frontend/src/modules/application/containers/App/CowSpeechBubbleNotificationBanner.tsx`:
- Line 52: The current substring check in CowSpeechBubbleNotificationBanner's
linkTarget calculation wrongly classifies external URLs as internal; replace the
url.includes(window.location.host) test by parsing the URL with the URL
constructor (use new URL(url, window.location.origin) to handle relative URLs)
and compare its hostname or host strictly to window.location.hostname/host (or
window.location.origin for scheme+host checks); keep the existing relative-path
check (url.startsWith('/')) and set target to '_parent' only when the parsed URL
is same-origin, otherwise '_blank'.
- Around line 61-73: Add a descriptive aria-label to the NotificationLink so
screen readers hear context (e.g., aria-label={t`View: ${title}`}) and change
the rel prop to use undefined instead of '' when linkTarget !== '_blank' so
React omits the attribute (i.e., set rel={linkTarget === '_blank' ? 'noopener
noreferrer' : undefined}); update the NotificationLink JSX where href={url},
target={linkTarget}, title and id are available and the data-click-event uses
toCowSwapGtmEvent.
In
`@apps/cowswap-frontend/src/modules/application/containers/AppContainer/AppContainer.container.tsx`:
- Around line 193-201: Replace the mutable let reassignment for bubbleElement
with an immutable const using a nested ternary and the imported ReactNode type:
declare const bubbleElement: ReactNode = showCowSpeechBubble ?
(currentNotification ? <CowSpeechBubbleNotificationBanner
currentNotification={currentNotification} onClose={dismiss} /> :
<CowSpeechBubbleHiringBanner />) : null; ensure you reference the existing
symbols showCowSpeechBubble, currentNotification, dismiss,
CowSpeechBubbleNotificationBanner and CowSpeechBubbleHiringBanner and remove
React.ReactNode in favor of the imported ReactNode.
In
`@apps/cowswap-frontend/src/modules/notifications/containers/NotificationsList/index.tsx`:
- Around line 88-91: The fallback "|| []" is dead code because the early guard
"if (!sidebarNotifications) return" ensures sidebarNotifications is non-null;
remove the redundant "|| []" in the setTimeout call so markNotificationsAsRead
receives the mapped array directly (i.e., call
markNotificationsAsRead(sidebarNotifications.map(({ id }) => id))). This touches
the setTimeout invocation where sidebarNotifications and markNotificationsAsRead
are used.
In
`@apps/cowswap-frontend/src/modules/notifications/hooks/useSpeechBubbleNotification.ts`:
- Around line 20-26: In useSpeechBubbleNotification, the memoized computation
for speechBubbleNotifications redundantly uses (notifications || []) despite an
early guard returning [] when !notifications; update the callback to use
notifications directly (e.g., notifications.filter(...).sort(...)) and remove
the dead `|| []` expression so the code is simpler and avoids unnecessary
fallback logic while preserving the filter by isSpeechBubbleNotification and the
sort by createdAt.
- Line 40: The hook useSpeechBubbleNotification returns a new object each render
which breaks reference equality; wrap the returned container object in useMemo
so { currentNotification, dismiss } is memoized and only changes when
currentNotification or dismiss change, ensuring consumers that rely on stable
refs (e.g., React.memo, atoms, effect deps) do not re-run unnecessarily.
- Around line 28-32: The null guard inside the useMemo that computes
currentNotification is unreachable because speechBubbleNotifications is always
an array; remove the if (!speechBubbleNotifications) return null check and
simply return speechBubbleNotifications.find(({ id }) =>
unreadNotifications[id]) || null so currentNotification (in the useMemo) relies
directly on the array, keeping the dependency list [speechBubbleNotifications,
unreadNotifications] unchanged.
---
Outside diff comments:
In
`@apps/cowswap-frontend/src/modules/notifications/state/readNotificationsAtom.ts`:
- Around line 12-26: markNotificationsAsReadAtom currently mutates the array
in-place (state.push(...)) causing Jotai to skip updates; replace its body with
the clone/set approach used in markNotificationsAsReadCloneArrayAtom (create new
Set(get(readNotificationsAtom)), add ids, then set(readNotificationsAtom,
[...state])) to ensure a new reference is written, remove
markNotificationsAsReadCloneArrayAtom and update all callers (e.g.
NotificationsList/index.tsx and any speech-bubble notification callers) to use
markNotificationsAsReadAtom — if you must keep both during a transition, rename
markNotificationsAsReadCloneArrayAtom to a clear intent-based name like
markSpeechBubbleNotificationsAsReadAtom and update callers accordingly.
---
Nitpick comments:
In `@apps/cowswap-frontend/src/common/utils/startTypingAnimation.ts`:
- Line 1: The import of React types is currently a runtime value import; change
it to a type-only import so Dispatch and SetStateAction are erased at
runtime—replace the value import of Dispatch and SetStateAction with a type
import (i.e., use a type-only import for Dispatch and SetStateAction) in the
file containing startTypingAnimation to avoid bundling runtime metadata while
keeping the same identifiers.
In
`@apps/cowswap-frontend/src/modules/application/containers/AppContainer/AppContainer.container.tsx`:
- Around line 186-201: The FooterSection helper currently calls the domain hook
useSpeechBubbleNotification() internally which runs even when show === false and
couples the helper to notification state; move the hook call up into
AppContainer, add currentNotification and dismiss to FooterSectionProps, and
pass those props into FooterSection so FooterSection no longer invokes
useSpeechBubbleNotification(); inside FooterSection replace the local hook usage
with the passed-in currentNotification and dismiss when rendering
CowSpeechBubbleNotificationBanner or CowSpeechBubbleHiringBanner (also ensure
showCowSpeechBubble prop is still honored).
In
`@apps/cowswap-frontend/src/modules/notifications/utils/filterNotifications.utils.ts`:
- Around line 3-9: Replace the hardcoded 'speechBubble' checks by importing
NOTIFICATION_LOCATION_SPEECH_BUBBLE from `@cowprotocol/core` and use it inside
isSpeechBubbleNotification(notification: NotificationModel); then implement
isSidebarNotification(notification: NotificationModel) as the logical complement
of isSpeechBubbleNotification (e.g., return
!isSpeechBubbleNotification(notification)) so future NotificationLocation
variants (like 'modal') won't be silently misrouted; update the functions
isSpeechBubbleNotification and isSidebarNotification accordingly.
In `@libs/core/src/cms/types.ts`:
- Around line 40-42: Add a companion exported constant for the 'speechBubble'
NotificationLocation to match NOTIFICATION_LOCATION_DEFAULT: define and export
e.g. NOTIFICATION_LOCATION_SPEECH_BUBBLE with type NotificationLocation and
value 'speechBubble' so callers (and filterNotifications.utils.ts) can reference
the constant instead of hardcoding the string; update any internal usages to use
NOTIFICATION_LOCATION_SPEECH_BUBBLE where appropriate.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/cowswap-frontend/src/common/hooks/useDelay.tsapps/cowswap-frontend/src/common/hooks/useTypingProgress.tsapps/cowswap-frontend/src/common/utils/startTypingAnimation.tsapps/cowswap-frontend/src/modules/application/containers/App/CowSpeechBubble.tsxapps/cowswap-frontend/src/modules/application/containers/App/CowSpeechBubbleHiringBanner.tsxapps/cowswap-frontend/src/modules/application/containers/App/CowSpeechBubbleNotificationBanner.tsxapps/cowswap-frontend/src/modules/application/containers/AppContainer/AppContainer.container.tsxapps/cowswap-frontend/src/modules/notifications/containers/NotificationsList/index.tsxapps/cowswap-frontend/src/modules/notifications/hooks/useSpeechBubbleNotification.tsapps/cowswap-frontend/src/modules/notifications/state/readNotificationsAtom.tsapps/cowswap-frontend/src/modules/notifications/utils/filterNotifications.utils.tslibs/core/src/cms/types.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/cowswap-frontend/src/modules/application/containers/App/CowSpeechBubbleNotificationBanner.tsx (2)
62-74:rel=""(should beundefined) and non-descriptive"View"link text — previously flagged and still unresolved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cowswap-frontend/src/modules/application/containers/App/CowSpeechBubbleNotificationBanner.tsx` around lines 62 - 74, The NotificationLink usage sets rel to an empty string when linkTarget !== '_blank' and uses non-descriptive link text "View"; change rel to undefined instead of '' to avoid rendering an empty attribute (update the rel prop in the NotificationLink JSX) and replace the generic link text with a descriptive label (e.g., include the notification title or context such as `View notification: ${title}` or add an aria-label that includes title) so the link is meaningful for screen readers and accessibility; locate the NotificationLink element that supplies href, target, rel, data-click-event (toCowSwapGtmEvent) and title/id to implement these changes.
53-53:url.includes(window.location.host)false-positive internal URL classification — previously flagged and still unresolved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cowswap-frontend/src/modules/application/containers/App/CowSpeechBubbleNotificationBanner.tsx` at line 53, The current linkTarget calculation (const linkTarget = url && (url.includes(window.location.host) || url.startsWith('/')) ? '_parent' : '_blank') misclassifies external URLs that merely contain the host string; update it to safely parse the URL and compare hosts instead: in the code that sets linkTarget, attempt to construct new URL(url, window.location.href) inside a try/catch and treat the link as internal only if parsedUrl.host === window.location.host or url.startsWith('/'); otherwise fall back to '_blank' — reference the linkTarget assignment to locate where to change.
🧹 Nitpick comments (3)
apps/cowswap-frontend/src/modules/application/containers/App/CowSpeechBubbleNotificationBanner.tsx (1)
12-22:styled.spanwithdisplay: flex/display: block— preferdivfor layout containers.
NotificationTextandNotificationDescriptionare rendered as<span>elements yet assigned block-level/flex display via CSS. This is valid but semantically inconsistent:<span>is phrasing content and block-level layout is better expressed with<div>.♻️ Proposed fix
-const NotificationText = styled.span` +const NotificationText = styled.div` display: flex; flex-direction: column; gap: 12px; text-align: left; ` -const NotificationDescription = styled.span` +const NotificationDescription = styled.p` display: block; font-weight: 400; `🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cowswap-frontend/src/modules/application/containers/App/CowSpeechBubbleNotificationBanner.tsx` around lines 12 - 22, NotificationText and NotificationDescription are defined with styled.span but given block/flex layout; change both component definitions to use styled.div (i.e., replace styled.span with styled.div for NotificationText and NotificationDescription) so the markup is semantically correct for block-level layout while keeping the existing styles and names unchanged.apps/cowswap-frontend/src/modules/application/containers/App/CowSpeechBubbleHiringBanner.tsx (1)
15-15: Use namespace import for styled files instead of named imports.Named destructuring from a
.styledfile deviates from the project convention.♻️ Proposed fix
-import { Arrow, Cursor, JobsLink, TypingLine } from './CowSpeechBubble.styled' +import * as styledEl from './CowSpeechBubble.styled'Then update usages:
styledEl.TypingLine,styledEl.Cursor,styledEl.JobsLink,styledEl.Arrow.Based on learnings: "import styles as
import * as styledEl from './X.styled'for styled files."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cowswap-frontend/src/modules/application/containers/App/CowSpeechBubbleHiringBanner.tsx` at line 15, Replace the named destructured import from the styled module with a namespace import and update all usages accordingly: change the import of Arrow, Cursor, JobsLink, TypingLine from './CowSpeechBubble.styled' to a namespace import (e.g., import * as styledEl from './CowSpeechBubble.styled') and then update references in this file to use styledEl.TypingLine, styledEl.Cursor, styledEl.JobsLink, and styledEl.Arrow wherever those symbols are used.apps/cowswap-frontend/src/modules/application/containers/App/CowSpeechBubble.tsx (1)
3-3: Use namespace import for styled files instead of named imports.♻️ Proposed fix
-import { Bubble, BubbleContent, CloseButton } from './CowSpeechBubble.styled' +import * as styledEl from './CowSpeechBubble.styled'Then update usages:
styledEl.Bubble,styledEl.BubbleContent,styledEl.CloseButton.Based on learnings: "import styles as
import * as styledEl from './X.styled'for styled files."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cowswap-frontend/src/modules/application/containers/App/CowSpeechBubble.tsx` at line 3, Replace the named imports from the styled module with a namespace import and update all uses accordingly: change the import of Bubble, BubbleContent, CloseButton from './CowSpeechBubble.styled' to a single namespace import (e.g., import * as styledEl from './CowSpeechBubble.styled') and then update all occurrences of Bubble, BubbleContent, and CloseButton in this file to styledEl.Bubble, styledEl.BubbleContent, and styledEl.CloseButton respectively so the component references the namespaced styled exports.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/cowswap-frontend/src/modules/application/containers/App/CowSpeechBubbleHiringBanner.tsx`:
- Around line 47-61: The JobsLink is hidden via CSS (opacity/pointer-events) but
remains keyboard-focusable; update the JobsLink render to remove it from the tab
order and hide it from assistive tech while typing is in progress by setting
tabIndex to -1 and aria-hidden to true when isTypingComplete is false (and
restore tabIndex/aria-hidden when true); reference the JobsLink element and the
isTypingComplete prop to implement this change so keyboard users cannot tab to
the invisible link during the typing animation.
---
Duplicate comments:
In
`@apps/cowswap-frontend/src/modules/application/containers/App/CowSpeechBubbleNotificationBanner.tsx`:
- Around line 62-74: The NotificationLink usage sets rel to an empty string when
linkTarget !== '_blank' and uses non-descriptive link text "View"; change rel to
undefined instead of '' to avoid rendering an empty attribute (update the rel
prop in the NotificationLink JSX) and replace the generic link text with a
descriptive label (e.g., include the notification title or context such as `View
notification: ${title}` or add an aria-label that includes title) so the link is
meaningful for screen readers and accessibility; locate the NotificationLink
element that supplies href, target, rel, data-click-event (toCowSwapGtmEvent)
and title/id to implement these changes.
- Line 53: The current linkTarget calculation (const linkTarget = url &&
(url.includes(window.location.host) || url.startsWith('/')) ? '_parent' :
'_blank') misclassifies external URLs that merely contain the host string;
update it to safely parse the URL and compare hosts instead: in the code that
sets linkTarget, attempt to construct new URL(url, window.location.href) inside
a try/catch and treat the link as internal only if parsedUrl.host ===
window.location.host or url.startsWith('/'); otherwise fall back to '_blank' —
reference the linkTarget assignment to locate where to change.
---
Nitpick comments:
In
`@apps/cowswap-frontend/src/modules/application/containers/App/CowSpeechBubble.tsx`:
- Line 3: Replace the named imports from the styled module with a namespace
import and update all uses accordingly: change the import of Bubble,
BubbleContent, CloseButton from './CowSpeechBubble.styled' to a single namespace
import (e.g., import * as styledEl from './CowSpeechBubble.styled') and then
update all occurrences of Bubble, BubbleContent, and CloseButton in this file to
styledEl.Bubble, styledEl.BubbleContent, and styledEl.CloseButton respectively
so the component references the namespaced styled exports.
In
`@apps/cowswap-frontend/src/modules/application/containers/App/CowSpeechBubbleHiringBanner.tsx`:
- Line 15: Replace the named destructured import from the styled module with a
namespace import and update all usages accordingly: change the import of Arrow,
Cursor, JobsLink, TypingLine from './CowSpeechBubble.styled' to a namespace
import (e.g., import * as styledEl from './CowSpeechBubble.styled') and then
update references in this file to use styledEl.TypingLine, styledEl.Cursor,
styledEl.JobsLink, and styledEl.Arrow wherever those symbols are used.
In
`@apps/cowswap-frontend/src/modules/application/containers/App/CowSpeechBubbleNotificationBanner.tsx`:
- Around line 12-22: NotificationText and NotificationDescription are defined
with styled.span but given block/flex layout; change both component definitions
to use styled.div (i.e., replace styled.span with styled.div for
NotificationText and NotificationDescription) so the markup is semantically
correct for block-level layout while keeping the existing styles and names
unchanged.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/cowswap-frontend/src/modules/application/containers/App/CowSpeechBubble.styled.tsxapps/cowswap-frontend/src/modules/application/containers/App/CowSpeechBubble.tsxapps/cowswap-frontend/src/modules/application/containers/App/CowSpeechBubbleHiringBanner.tsxapps/cowswap-frontend/src/modules/application/containers/App/CowSpeechBubbleNotificationBanner.tsx
Comment on lines
+47
to
+61
| <JobsLink | ||
| href={CAREERS_URL} | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| $visible={isTypingComplete} | ||
| aria-label={t`View jobs (opens in a new tab)`} | ||
| data-click-event={toCowSwapGtmEvent({ | ||
| category: CowSwapAnalyticsCategory.COWSWAP, | ||
| action: 'Click speech bubble jobs link', | ||
| label: CAREERS_URL, | ||
| })} | ||
| > | ||
| <Trans>View jobs</Trans> | ||
| <Arrow aria-hidden="true">→</Arrow> | ||
| </JobsLink> |
Contributor
There was a problem hiding this comment.
JobsLink is invisible but still keyboard-focusable while typing is in progress.
opacity: 0 + pointer-events: none hides and disables mouse interaction, but the link remains in the natural tab order. A keyboard user pressing Tab during the typing animation will land on an invisible, unfocusable-looking element.
♿ Proposed fix — remove from tab order when not yet visible
<JobsLink
href={CAREERS_URL}
target="_blank"
rel="noopener noreferrer"
$visible={isTypingComplete}
+ tabIndex={isTypingComplete ? undefined : -1}
+ aria-hidden={!isTypingComplete}
aria-label={t`View jobs (opens in a new tab)`}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <JobsLink | |
| href={CAREERS_URL} | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| $visible={isTypingComplete} | |
| aria-label={t`View jobs (opens in a new tab)`} | |
| data-click-event={toCowSwapGtmEvent({ | |
| category: CowSwapAnalyticsCategory.COWSWAP, | |
| action: 'Click speech bubble jobs link', | |
| label: CAREERS_URL, | |
| })} | |
| > | |
| <Trans>View jobs</Trans> | |
| <Arrow aria-hidden="true">→</Arrow> | |
| </JobsLink> | |
| <JobsLink | |
| href={CAREERS_URL} | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| $visible={isTypingComplete} | |
| tabIndex={isTypingComplete ? undefined : -1} | |
| aria-hidden={!isTypingComplete} | |
| aria-label={t`View jobs (opens in a new tab)`} | |
| data-click-event={toCowSwapGtmEvent({ | |
| category: CowSwapAnalyticsCategory.COWSWAP, | |
| action: 'Click speech bubble jobs link', | |
| label: CAREERS_URL, | |
| })} | |
| > | |
| <Trans>View jobs</Trans> | |
| <Arrow aria-hidden="true">→</Arrow> | |
| </JobsLink> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/cowswap-frontend/src/modules/application/containers/App/CowSpeechBubbleHiringBanner.tsx`
around lines 47 - 61, The JobsLink is hidden via CSS (opacity/pointer-events)
but remains keyboard-focusable; update the JobsLink render to remove it from the
tab order and hide it from assistive tech while typing is in progress by setting
tabIndex to -1 and aria-hidden to true when isTypingComplete is false (and
restore tabIndex/aria-hidden when true); reference the JobsLink element and the
isTypingComplete prop to implement this change so keyboard users cannot tab to
the invisible link during the typing animation.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/cowswap-frontend/src/modules/application/containers/AppContainer/AppContainer.container.tsx (1)
194-202: 🛠️ Refactor suggestion | 🟠 MajorReplace
letreassignment withconst+ nested ternary.This pattern was flagged in a previous review but remains unaddressed. The sequential
letreassignment violates the coding guideline favoringconstwith ternaries/early returns.♻️ Proposed fix
- let bubbleElement: ReactNode = null - - if (showCowSpeechBubble) { - if (currentNotification) { - bubbleElement = <CowSpeechBubbleNotificationBanner currentNotification={currentNotification} onClose={dismiss} /> - } else { - bubbleElement = <CowSpeechBubbleHiringBanner /> - } - } + const bubbleElement: ReactNode = showCowSpeechBubble + ? currentNotification + ? <CowSpeechBubbleNotificationBanner currentNotification={currentNotification} onClose={dismiss} /> + : <CowSpeechBubbleHiringBanner /> + : nullAs per coding guidelines: "Never reassign via
let; favorconstand ternaries/early returns in TypeScript."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cowswap-frontend/src/modules/application/containers/AppContainer/AppContainer.container.tsx` around lines 194 - 202, Replace the mutable let bubbleElement with an immutable const using a nested ternary: compute bubbleElement from showCowSpeechBubble ? (currentNotification ? <CowSpeechBubbleNotificationBanner currentNotification={currentNotification} onClose={dismiss} /> : <CowSpeechBubbleHiringBanner />) : null so you no longer reassign; keep references to showCowSpeechBubble, currentNotification, CowSpeechBubbleNotificationBanner, CowSpeechBubbleHiringBanner, and dismiss to preserve behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@apps/cowswap-frontend/src/modules/application/containers/AppContainer/AppContainer.container.tsx`:
- Around line 194-202: Replace the mutable let bubbleElement with an immutable
const using a nested ternary: compute bubbleElement from showCowSpeechBubble ?
(currentNotification ? <CowSpeechBubbleNotificationBanner
currentNotification={currentNotification} onClose={dismiss} /> :
<CowSpeechBubbleHiringBanner />) : null so you no longer reassign; keep
references to showCowSpeechBubble, currentNotification,
CowSpeechBubbleNotificationBanner, CowSpeechBubbleHiringBanner, and dismiss to
preserve behavior.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/cowswap-frontend/src/modules/application/containers/AppContainer/AppContainer.container.tsx
elena-zh
requested changes
Feb 27, 2026
Contributor
elena-zh
left a comment
elena-zh
left a comment
There was a problem hiding this comment.
Hey @Danziger , great jib, thank you!
Some nitpicks from my side:
- original bubble has an animated text, however, a notification one does not have. Might it be possible to implement it?
- UI properties of the default 'Hiring' bubble have need changed:
- it became wider
- the arrow's position has been changed
See the comparison with the prod version:
- Notification template: there is no point in 'empty' state for it: I'd pick 'default' as a default value since an empty option duplicates a default behavior.
- On some screen resolutions it is not clear that a bubble comes from the cow (might be related to the p.2 above)
Thanks.
Contributor
Author
|
@elena-zh I'm aware of those, but I don't think at this point it makes more sense to invest more time into this feature, but feel free to create an issue with those in case we want to refine it later. To add more context:
|
Contributor
|
Hey @Danziger
If you aware of some issues, please report them in the description so I will not spend my time on reporting them back you. Thank you for understanding. As for the
I asked @fairlighteth to take a look at this component |
Contributor
20 tasks
* feat: implement CowSpeechBubble component and related notifications * feat: enhance CowSpeechBubble styling and layout for better responsiveness * fix: adjust max-height of CowSpeechBubble for improved layout
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Summary
Add a "speechBubble"-type notification.
To use them, simply create a new notification in the CMS with the
locationfield set to "speechBubble". These will be shown one by one in the cow speech bubble instead of on the sidebar, and are only dismissed when clicking the "x" icon in the speech bubble.CMS PR: cowprotocol/cms#86
To Test
locationfield set to "speechBubble", or use the existing "Test cow speech bubble notification" template. See https://www.notion.so/cownation/Send-a-CoW-Notification-to-users-fec6aa0a78584475b54d0c9a266a67d8Also note:
Summary by CodeRabbit
New Features
Refactor
Utilities/Types