[CSM Portal] mobile-device banner + fix {{currentTeam}} token in section headings - #1363
Conversation
|
Warning Review limit reached
Next review available in: 44 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe portal adds configurable mobile-device detection and a dismissible app-store banner. The layout mounts the banner globally. Dashboard section titles and refresh labels now resolve the ChangesMobile app prompt
Dashboard section text resolution
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant AppLayout
participant MobileAppBanner
participant Config
participant DeviceDetection
participant Browser
AppLayout->>MobileAppBanner: Render
MobileAppBanner->>Config: Read prompt settings
MobileAppBanner->>DeviceDetection: Detect mobile device
DeviceDetection-->>MobileAppBanner: Return device information
MobileAppBanner->>Config: Resolve platform store URL
MobileAppBanner-->>AppLayout: Render dismissible banner
MobileAppBanner->>Browser: Open validated store URL
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
…Super App Ports the customer portal's mobile-device detection to CSM Portal, but as a dismissible banner instead of a full-page block: CS engineers may need emergency mobile access, unlike external customers on the customer-facing site. Config keys are CSM-prefixed (CSM_PORTAL_MOBILE_APP_*) and independent of the customer portal's own. Built on Alert directly rather than NotificationBanner, since NotificationBanner/Alert only auto-renders its own close icon when no custom action node is supplied -- a banner needing both a "Download" action and a close control has to pack both into one action slot, matching the existing pattern in ErrorBanner.tsx.
…gs too
Section titles support the same {{currentTeam}} interpolation as an
individual widget's own displayName/description, but the section
heading itself was rendered raw -- a section named e.g.
"Overall - {{currentTeam}}" showed the literal token instead of the
selected team's name.
68135f3 to
d00d1bd
Compare
rksk
changed the title
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/csm-portal/webapp/src/features/csm-dashboard/components/AgentsLandingPagePilot.test.tsx (1)
253-255: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the resolved refresh label.
This test checks the heading only. It does not check the
RefreshButtonlabel changed at Lines 231-234 inAgentsLandingPagePilot.tsx. Add an assertion that the refresh control exposesRefresh Overall - Castor.🤖 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 `@apps/csm-portal/webapp/src/features/csm-dashboard/components/AgentsLandingPagePilot.test.tsx` around lines 253 - 255, Add an assertion in the existing AgentsLandingPagePilot test to verify the refresh control exposes the resolved label “Refresh Overall - Castor,” alongside the current heading and unresolved-template checks. Target the rendered RefreshButton control rather than only asserting visible heading text.apps/csm-portal/webapp/src/components/mobile-app-banner/MobileAppBanner.tsx (1)
73-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the inactive dismissal-reset effect.
The result of
getMobileAppConfig()is memoized at Line 57. No reactive input can changevisibleduring this mount. The effect cannot restore a dismissed banner, but it suppressesreact-hooks/set-state-in-effect. Remove the effect. If live configuration updates are required, make configuration a reactive input and use a previous-value guard during render.Proposed cleanup
-import { useEffect, useMemo, useState, type JSX } from "react"; +import { useMemo, useState, type JSX } from "react"; @@ - // Reset the dismissed state when the visibility configuration changes to true. - useEffect(() => { - if (visible) { - // eslint-disable-next-line react-hooks/set-state-in-effect -- reset dismissal when banner is re-shown - setDismissed(false); - } - }, [visible]);Based on learnings: do not call
setStateinsideuseEffect; adjust derived state during render.🤖 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 `@apps/csm-portal/webapp/src/components/mobile-app-banner/MobileAppBanner.tsx` around lines 73 - 79, Remove the visibility-based useEffect that calls setDismissed(false) in MobileAppBanner; keep the existing dismissal state and memoized getMobileAppConfig flow unchanged, without suppressing the set-state-in-effect lint rule.Source: Learnings
apps/csm-portal/webapp/src/utils/deviceDetection.test.ts (1)
46-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the iPadOS desktop-UA fallback.
The tests use a user agent that contains
"iPad". They do not execute theMacIntelandmaxTouchPointsfallback. Add a user agent without"iPad"and assertnullby default andDeviceType.TabletwhenincludeTabletsistrue.Proposed test
+ it("should detect an iPadOS desktop user agent when tablets are included", () => { + mockNavigator({ + userAgent: + "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/13.0.4 Safari/604.1", + platform: NavigatorPlatform.MacIntel, + maxTouchPoints: 5, + }); + + expect(detectMobileDevice()).toBeNull(); + expect(detectMobileDevice({ includeTablets: true })).toEqual({ + os: MobileOs.Ios, + deviceType: DeviceType.Tablet, + }); + });🤖 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 `@apps/csm-portal/webapp/src/utils/deviceDetection.test.ts` around lines 46 - 69, Extend the iPad coverage in the tests around detectMobileDevice with a desktop-style iPadOS user agent that omits “iPad” while retaining MacIntel and maxTouchPoints. Assert it returns null by default and returns an iOS Tablet result when includeTablets is true, matching the existing cases.
🤖 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 `@apps/csm-portal/webapp/src/components/mobile-app-banner/MobileAppBanner.tsx`:
- Around line 64-68: Validate the URL returned by getMobileAppStoreUrl before
computing visible in MobileAppBanner, suppressing the banner for invalid URLs
and passing only the validated URL to window.open in handleDownload. In
apps/csm-portal/webapp/src/components/mobile-app-banner/MobileAppBanner.tsx
lines 64-68, update the store URL flow accordingly; in
apps/csm-portal/webapp/src/components/mobile-app-banner/MobileAppBanner.test.tsx
lines 132-143, add an invalid-URL case that asserts the banner is suppressed and
retain the no-window.open assertion.
---
Nitpick comments:
In `@apps/csm-portal/webapp/src/components/mobile-app-banner/MobileAppBanner.tsx`:
- Around line 73-79: Remove the visibility-based useEffect that calls
setDismissed(false) in MobileAppBanner; keep the existing dismissal state and
memoized getMobileAppConfig flow unchanged, without suppressing the
set-state-in-effect lint rule.
In
`@apps/csm-portal/webapp/src/features/csm-dashboard/components/AgentsLandingPagePilot.test.tsx`:
- Around line 253-255: Add an assertion in the existing AgentsLandingPagePilot
test to verify the refresh control exposes the resolved label “Refresh Overall -
Castor,” alongside the current heading and unresolved-template checks. Target
the rendered RefreshButton control rather than only asserting visible heading
text.
In `@apps/csm-portal/webapp/src/utils/deviceDetection.test.ts`:
- Around line 46-69: Extend the iPad coverage in the tests around
detectMobileDevice with a desktop-style iPadOS user agent that omits “iPad”
while retaining MacIntel and maxTouchPoints. Assert it returns null by default
and returns an iOS Tablet result when includeTablets is true, matching the
existing cases.
🪄 Autofix
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 Plus
Run ID: 88aa0c1e-0183-4020-9626-0d8171bfb3be
📒 Files selected for processing (11)
apps/csm-portal/webapp/public/config.js.exampleapps/csm-portal/webapp/src/components/mobile-app-banner/MobileAppBanner.test.tsxapps/csm-portal/webapp/src/components/mobile-app-banner/MobileAppBanner.tsxapps/csm-portal/webapp/src/config/authConfig.tsapps/csm-portal/webapp/src/config/mobileAppConfig.tsapps/csm-portal/webapp/src/features/csm-dashboard/components/AgentsLandingPagePilot.test.tsxapps/csm-portal/webapp/src/features/csm-dashboard/components/AgentsLandingPagePilot.tsxapps/csm-portal/webapp/src/layouts/AppLayout.tsxapps/csm-portal/webapp/src/types/mobileDevice.tsapps/csm-portal/webapp/src/utils/deviceDetection.test.tsapps/csm-portal/webapp/src/utils/deviceDetection.ts
visible only checked storeUrl's truthiness, so a misconfigured store URL (unsupported scheme, or a string that fails to parse) rendered a Download button that silently did nothing on click. Both the visibility check and window.open now use the same validated URL.
2 participants
Purpose
Two small, independent CSM Portal changes bundled into one PR:
{{currentTeam}}text token (e.g. "Overall - {{currentTeam}}") showed the literal, unresolved token instead of the selected team's name — the token was only wired into individual widgets' owndisplayName/description, not into section headings.Goals
{{currentTeam}}the same way individual widgets already do.Approach
Mobile banner: ported the customer portal's device-detection utilities (
detectMobileDevice/shouldPromptForMobileApp,MobileOs/DeviceTypetypes) into CSM Portal unchanged. Added an independent config surface with CSM-prefixed keys (CSM_PORTAL_MOBILE_APP_PROMPT_ENABLED/_IOS_STORE_URL/_ANDROID_STORE_URL/_INCLUDE_TABLETS), separate from the customer portal's ownCUSTOMER_PORTAL_MOBILE_APP_*keys. Built the banner directly on MUIAlertrather than this app'sNotificationBannerwrapper:NotificationBanner/Alertonly auto-renders its own close icon when no customactionnode is supplied, so a banner needing both a "Download" action and a close control has to pack both intoactionitself — matching the existing pattern already established inErrorBanner.tsxfor the same reason. Wired ahead of the existing maintenance banner (GlobalNotificationBanner) inAppLayout.tsx— the device nudge is per-session/dismissible, the maintenance banner is an admin-broadcast operational notice that should keep its position.Section-title token:
resolveWidgetText(already used for widgetdisplayName/description) is now also applied toAgentsLandingPagePilot's section heading and its refresh button's accessible label, using the sameselectedTeamLabelalready threaded into the component.No screenshot included — item 1 is a small info-severity
Alertbanner consistent with the existingErrorBanner/GlobalNotificationBannervisual pattern; item 2 is a one-line text-resolution fix with no visual/layout change beyond the text itself now resolving correctly.User stories
{{currentTeam}}-templated section title, I see the actual team name (or "All ABTs") instead of the literal template token.Release note
{{currentTeam}}text token, so a section like "Overall - {{currentTeam}}" now correctly shows e.g. "Overall - Castor" or "Overall - All ABTs".Documentation
N/A — both changes are internal portal UI behavior, no external doc impact.
Training
N/A — not training content.
Certification
N/A — no certification exam impact.
Marketing
N/A — internal UX addition and a bug fix, not marketable features.
Automation tests
Mobile banner:
deviceDetection.test.ts(9 tests) +MobileAppBanner.test.tsx(7 tests), all passing. Section-title fix: new case inAgentsLandingPagePilot.test.tsxasserting{{currentTeam}}resolves in a section heading and the literal token never renders; full file re-run 7/7 passing. Fullcsm-dashboardsuite re-verified on this branch after rebasing onto the now-mergedmain: 141/141 passing.eslint/tsc -b/vite buildall clean.N/A — no e2e specs cover either interaction yet.
Security checks
eslint/tsc -b, both cleanpublic/config.js.exampleonly documents the new mobile-app keys, the realpublic/config.jsis gitignored and untouchedSamples
N/A — no sample app impact.
Related PRs
None (cs-tools#1362, which this branch was originally alongside, has since merged; this branch was rebased onto the resulting
main).Migrations (if applicable)
N/A — no data/schema migration.
Test environment
Verified locally on this branch (post-rebase onto merged
main):eslintclean,pnpm run build(tsc -b && vite build) clean,vitest runon the mobile-banner tests (16/16) and the fullcsm-dashboardsuite (141/141), all passing.Learning
Confirmed via MUI
Alertsource that it only auto-renders its own close button whenactionis unset — documented inMobileAppBanner's own doc comment for future maintainers extendingNotificationBanner-based banners with an action.Summary by CodeRabbit