Add enable-notifications banner above Concierge composer - #90365
Conversation
Cross-platform helper for reading and requesting the OS/browser notification permission. Web reads window.Notification.permission and requests via Notification.requestPermission(); native wraps Airship's getNotificationStatus / enableUserNotifications. Used by the upcoming Concierge enable-notifications banner.
Session-scoped boolean flag for dismissing the Concierge enable-notifications banner. RAM-only so dismissal does not persist across reloads, per the spec on issue #90136.
Adds 'prompt' and 'cta' strings under translations.concierge.enableNotifications across all locales. These power the upcoming composer-attached banner that asks Concierge users to enable browser/push notifications.
Composer-attached banner shown above the message composer in the Concierge chat when the user has not yet granted notification permission. Offers 'Notify me' (triggers the OS/browser permission prompt) and 'Not now' (session-scoped dismiss). Returns null outside Concierge or once permission is already granted or denied.
Inserts the banner in the happy-path branch only, directly above ReportActionCompose. The banner self-gates on Concierge identity and permission status, so no extra guards are needed at this call site.
prefer-actions-set-data forbids components from writing Onyx directly. Extract the RAM_ONLY_HAS_DISMISSED_CONCIERGE_NOTIFICATION_BANNER write into src/libs/actions/ConciergeNotificationBanner.dismissForSession() and call it from the banner.
This comment has been minimized.
This comment has been minimized.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
The RAM_ONLY_ prefix is just a naming convention; Onyx persists every key by default unless it is listed in the ramOnlyKeys array in src/setup/index.ts. Without this, the dismissal flag survives reloads via IndexedDB and the banner never reappears, contrary to the spec.
When the banner is shown, render it and the composer inside a single rounded off-white container so they visually belong together (instead of the banner sitting as a separate rectangle above the composer). When the banner is hidden, the composer renders unchanged. Also drops the bell icon and uses the default theme text color so the prompt is legible in light mode.
Switch the prompt from a declarative 'Get notified...' to a question 'Want to be notified when Concierge responds?' and shorten the CTA from 'Notify me' to 'Notify' across all 10 locales.
This comment has been minimized.
This comment has been minimized.
The wrap approach left cream visible on the composer's sides and bottom, which didn't match the design. Make the banner a standalone element above the composer and pull the composer up via marginTop so its top edge overlaps the banner. The composer's white covers the banner's bottom half while the rounded top corners expose the cream behind them, giving the composer the look of sitting partially into the banner. Extract useShouldShowEnableNotificationsBanner so the banner and ReportFooter agree on whether the overlap should be applied.
Mirror the native impl's defensive logging — requestPermission is spec'd not to reject but secure-context edge cases can throw.
dismissForSession passes straight to onPress; the notify-me handler moves to a module-level function. Neither needs memoization since the buttons aren't memoized children.
ReportFooter calls the hook before its reportID-non-null narrowing, so typing it as required forced a string-coerce-to-empty workaround at the call site. Loosen the type and handle undefined internally.
The banner's paddingBottom and the composer's negative marginTop must agree to position the cream behind the composer's top. Export the value from the banner so the relationship is visible in one place.
EnableNotificationsBanner had its own gate via the hook, which meant both ReportFooter and the banner invoked the hook (and its async permission probe) separately. Lift the gate to ReportFooter so the hook runs once per render, and let the banner just render itself when asked.
Both the banner's container style and ReportFooter's composer-overlap style were object literals recreated on every render. Lift them to module-level constants — stable identities, fewer allocations.
The banner appears asynchronously once the permission probe resolves. accessibilityRole=alert lets assistive tech announce it on render.
Three new suites: - useShouldShowEnableNotificationsBanner covers each gate (non-Concierge report, dismissed flag, granted/denied permission, undefined reportID). - ConciergeNotificationBanner action covers the RAM-only write. - EnableNotificationsBanner UI covers the prompt + button labels and verifies the action plumbing: 'Not now' dismisses; 'Notify' requests and dismisses only on grant.
Banner: - Rounded top corners only (componentBorderRadiusLarge / 16px), flat bottom - Padding 12/12/16 (top/right/left) per Figma; paddingBottom = visible gap below text (12) + the overlap hidden behind the composer (32) - Button margins: ml3 (Notify) then ml1 (Not now) for 12px and 4px spacing - Add accessibilityLiveRegion=polite to satisfy rulesdir/require-live-region-for-status-updates with the alert role - Hoist BANNER_VISIBLE_BOTTOM_GAP_PX so the relationship between top padding and visible-bottom-gap is explicit Hook: - Use getNonEmptyStringOnyxID(reportID) in the Onyx key template instead of the bare `?? ''` fallback (rulesdir/no-default-id-values) Test: - Replace Onyx.multiSet (TS-narrowed key types didn't agree) with individual Onyx.merge calls for the Concierge / report fixtures
Verifies the wiring between useShouldShowEnableNotificationsBanner and ReportFooter: when the hook returns true the banner stub renders; when it returns false only the composer stub renders. The hook and banner internals are mocked so this test focuses purely on the conditional render in ReportFooter.
Restore es.ts to main (eslint-disable comments were inadvertently removed) and keep ja.ts / zh-hans.ts in their original multi-line shape. Add only the new concierge.enableNotifications key.
Adds buttonText, shouldShowSecondaryButton, secondaryButtonText, onSecondaryButtonPress, and shouldUseSmallButtons so banners that need two side-by-side actions (e.g. the upcoming Concierge notification prompt) can reuse Banner instead of hand-rolling a View. Defaults preserve the current behavior for existing callers.
Replaces the hand-rolled View + Text + two Button layout with the shared Banner component now that Banner supports a secondary action and small buttons. Container styling overrides preserve the rounded-top + flat-bottom shape that overlaps the composer.
The hardcoded 32 was coupled to the composer's rounded radius (24) plus 8px of visual headroom. Deriving from variables.componentBorderRadiusRounded keeps the two in sync if the composer's radius ever changes.
Matches the web implementation's fallback. With 'default' the banner would render and the user would tap "Notify me", causing another Airship call that's likely to fail the same way. 'denied' fails closed and is consistent across platforms.
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
All good on my end.
|
🚧 @Gonals has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/Gonals in version: 9.3.89-1 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. This change introduces a small web-only banner above the Concierge composer that prompts the user to enable browser notifications via a Notify button (with a Not now button to dismiss for the session). It's a self-explanatory in-product nudge that:
Existing articles (e.g.,
|
|
Deploy Blocker #92038 was identified to be related to this PR. |
[CP Staging] Revert "Add enable-notifications banner above Concierge composer" (#90365)
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.3.89-7 🚀
|
|
🚀 Deployed to staging by https://github.com/Gonals in version: 9.3.90-0 🚀
Bundle Size Analysis (Sentry): |
|
🤖 I reviewed the changes in this PR against the help site ( Docs update needed: Yes — a small one. This PR adds a new user-visible, web-only banner in the Concierge chat that prompts users to enable browser notifications, and that behavior wasn't documented anywhere. The Concierge help article doesn't mention notifications at all. What I changed: Added a FAQ entry "Can I get notified when Concierge responds?" to Draft help site PR: #92192 (labeled I verified the exact UI copy against the live web app before writing — banner prompt is "Want to be notified when Concierge responds?", with buttons Notify and Not now. I also confirmed the banner does not appear in non-Concierge chats.
@justinpersaud, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.3.90-3 🚀
|
Keep the original composer tree untouched when the banner is hidden, so full-size compose still fills the footer. When the banner shows, wrap the composer in a view that keeps flex:1 in full-size mode. The reverted PR Expensify#90365 wrapped the composer unconditionally, which broke the flex chain and collapsed the expanded composer (App Expensify#92038).
11 participants
Explanation of Change
Adds a new banner in the concierge DM (web only) above the composer to prompt the user to enable notifications.
Fixed Issues
$ #90136
PROPOSAL:
Tests
Offline tests
n/a
QA Steps
Same as tests above
PR Author Checklist
/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)