Fix/reported bugs fixing#425
Conversation
…ent to button components
… fix/reported-bugs-fixing
… fix/reported-bugs-fixing
…ed on user authentication state
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
WalkthroughAdds an authentication-aware Sign In button to header navigation (desktop & mobile) using LOGIN_PATH; extends CMS Header model and mappers with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✅ 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/blocks/orders-summary/src/frontend/OrdersSummary.client.tsx (1)
72-80: Guard against empty ToggleGroup value in handleFilterRadix UI's ToggleGroup with
type="single"emits an empty string when the user deselects the currently selected item. Without a guard,value.split('-')yields[''],parts[1]!becomesundefined, and the assertion at line 99 throws a runtime error when trying to match againstcomponent.ranges. Add an early return for falsy values:const handleFilter = (value: string) => { + if (!value) { + return; + } startTransition(async () => { let dateFrom: string;This prevents the crash while keeping behavior unchanged for valid selections.
Also applies to: 93-100
♻️ Duplicate comments (1)
packages/integrations/strapi-cms/src/modules/cms/mappers/cms.header.mapper.ts (1)
44-44: Placeholder for Strapi CMS field addition.Similar to the Contentful mapper,
signInLabelis set toundefinedpending CMS integration. The UI verification comment from the Contentful mapper review applies here as well.
🧹 Nitpick comments (6)
packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.order-list.mapper.ts (1)
78-79: Status filter options correctly aligned with field mappings across localesThe added
REQUIRES_ACTIONandARCHIVEDoptions use the same keys and labels as infieldMapping.statusfor EN/DE/PL, so these statuses are now filterable without introducing inconsistencies. This is a clean, localized fix.If you see more status additions in the future, consider deriving these filter
optionsfrom a shared status definition to avoid similar misses between mapping and filters, but this is strictly optional for this PR.Also applies to: 185-186, 292-293
packages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.order-list.mapper.ts (1)
78-79: Contentful mocks now in sync with status mapping and mocked integrationThe new
REQUIRES_ACTIONandARCHIVEDstatus options for EN/DE/PL correctly reuse the existing mapping keys and translations, and they mirror the mocked integration file, so both data sources expose the same filter set.As a future improvement, you might pull these status options from a shared enum/config used by both integrations to prevent drift, but the current change is solid as-is.
Also applies to: 185-186, 292-293
packages/blocks/ticket-list/src/frontend/TicketList.client.tsx (1)
68-74: Topic column custom render with link looks correctChanging the
topiccolumn totype: 'custom'and rendering aButtonas aLinkComponenttoticket.detailsUrlis a clean way to make the subject clickable and truncated within the cell; assumingDataListalready supportscustomcolumns, this is good. If the entire row is also clickable elsewhere, consider styling theLinkComponentdirectly (without theButton) to avoid nested interactive semantics, but that’s optional.packages/ui/src/elements/toggle-group.tsx (1)
9-15: currentValue in ToggleGroupContext works for controlled groups; watch uncontrolled usageDeriving
currentValuefromprops.valueand passing it throughToggleGroupContextletsToggleGroupItemknow which values are active, which is ideal for controlled groups (wherevalueis set on the root). For groups that rely only ondefaultValue/ uncontrolled state,currentValuewill stayundefined, soactiveIconwill never render even though items can still be toggled on. If you intend to support icons there too, consider enforcing controlled usage at call sites or extending this logic to also track state viaonValueChange.Also applies to: 34-35, 47-49
packages/ui/src/globals.css (1)
63-63: New --color-card-light token is fine; Biome errors are likely tooling noise
--color-card-light: var(--card-light);matches the pattern of the surrounding--color-cardvariables and is valid inside a Tailwind v4@theme inlineblock. The Biome 2.1.2 parse /noUnknownPseudoClasserrors for this line are very likely due to incomplete support for Tailwind’s@themesyntax rather than a real CSS issue. I’d keep the token and address this at the tooling level (upgrade Biome or relax linting for these theme files) rather than removing the variable.apps/frontend/src/containers/Header/MobileNavigation/MobileNavigation.tsx (1)
37-45: Login detection logic is correct.The logic properly handles both localized and internal login paths, and correctly determines when to show the sign-in button. The defensive checks for undefined pathname and missing routing configuration are good practices.
Consider memoizing the
loginPathsarray to avoid recreating it on every render:+const loginPaths = React.useMemo( + () => [LOGIN_PATH, ...Object.values(routing.pathnames[LOGIN_PATH] || {})], + [] +); + // Check if we're on login page const normalizedPathname = pathname?.split('?')[0] || ''; -const loginPaths = [LOGIN_PATH, ...Object.values(routing.pathnames[LOGIN_PATH] || {})]; const isLoginPage = loginPaths.some( (loginPath) => normalizedPathname === loginPath || normalizedPathname.startsWith(`${loginPath}/`), );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (24)
apps/frontend/src/containers/Header/DesktopNavigation/DesktopNavigation.tsx(4 hunks)apps/frontend/src/containers/Header/DesktopNavigation/DesktopNavigation.types.ts(1 hunks)apps/frontend/src/containers/Header/Header.tsx(2 hunks)apps/frontend/src/containers/Header/MobileNavigation/MobileNavigation.tsx(3 hunks)apps/frontend/src/containers/Header/MobileNavigation/MobileNavigation.types.ts(1 hunks)apps/frontend/src/i18n/routing.ts(1 hunks)packages/blocks/notification-list/src/frontend/NotificationList.client.tsx(1 hunks)packages/blocks/orders-summary/src/frontend/OrdersSummary.client.tsx(2 hunks)packages/blocks/ticket-details/src/frontend/TicketDetails.client.tsx(1 hunks)packages/blocks/ticket-list/src/frontend/TicketList.client.tsx(1 hunks)packages/framework/src/modules/cms/models/header.model.ts(1 hunks)packages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.order-list.mapper.ts(3 hunks)packages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.ticket-details.mapper.ts(1 hunks)packages/integrations/contentful-cms/src/modules/cms/mappers/cms.header.mapper.ts(1 hunks)packages/integrations/contentful-cms/src/modules/cms/mappers/mocks/pages/service-details.page.ts(3 hunks)packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.order-list.mapper.ts(3 hunks)packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.ticket-details.mapper.ts(1 hunks)packages/integrations/mocked/src/modules/cms/mappers/cms.header.mapper.ts(3 hunks)packages/integrations/mocked/src/modules/cms/mappers/mocks/pages/service-details.page.ts(3 hunks)packages/integrations/strapi-cms/src/modules/cms/mappers/cms.header.mapper.ts(1 hunks)packages/ui/src/components/Filters/Filters.tsx(1 hunks)packages/ui/src/elements/toggle-group.tsx(3 hunks)packages/ui/src/globals.css(1 hunks)packages/ui/src/theme.css(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-13T15:40:10.528Z
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 348
File: packages/blocks/notification-summary/src/frontend/NotificationSummary.renderer.tsx:1-15
Timestamp: 2025-11-13T15:40:10.528Z
Learning: In next-intl 3.0+, hook-style APIs like useLocale(), useTranslations(), and useFormatter() can be used in non-async Server Components without the 'use client' directive. The library provides server-optimized implementations automatically. Only async Server Components need to use the await-based APIs like getLocale() from next-intl/server.
Applied to files:
packages/blocks/orders-summary/src/frontend/OrdersSummary.client.tsx
🧬 Code graph analysis (4)
apps/frontend/src/containers/Header/MobileNavigation/MobileNavigation.types.ts (2)
apps/frontend/src/containers/Header/Header.tsx (1)
Header(23-107)packages/framework/src/modules/cms/models/header.model.ts (1)
Header(4-24)
apps/frontend/src/containers/Header/DesktopNavigation/DesktopNavigation.tsx (3)
apps/frontend/src/containers/Header/DesktopNavigation/DesktopNavigation.types.ts (1)
DesktopNavigationProps(5-14)apps/frontend/src/i18n/routing.ts (2)
LOGIN_PATH(6-6)routing(8-18)packages/ui/src/elements/button.tsx (1)
Button(19-19)
apps/frontend/src/containers/Header/DesktopNavigation/DesktopNavigation.types.ts (2)
apps/frontend/src/containers/Header/Header.tsx (1)
Header(23-107)packages/framework/src/modules/cms/models/header.model.ts (1)
Header(4-24)
apps/frontend/src/containers/Header/MobileNavigation/MobileNavigation.tsx (2)
apps/frontend/src/containers/Header/MobileNavigation/MobileNavigation.types.ts (1)
MobileNavigationProps(5-16)apps/frontend/src/i18n/routing.ts (4)
usePathname(22-22)usePathname(25-25)LOGIN_PATH(6-6)routing(8-18)
🪛 Biome (2.1.2)
packages/ui/src/globals.css
[error] 63-63: expected , but instead found --color-card-light
Remove --color-card-light
(parse)
[error] 63-63: expected , but instead found (
Remove (
(parse)
[error] 63-63: expected , but instead found --card-light
Remove --card-light
(parse)
[error] 63-63: expected , but instead found )
Remove )
(parse)
[error] 63-63: Unexpected unknown pseudo-class var
See MDN web docs for more details.
(lint/correctness/noUnknownPseudoClass)
⏰ 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: deploy-preview
🔇 Additional comments (23)
packages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.ticket-details.mapper.ts (1)
76-80: PolishOPENstatus label change looks goodThe updated
"W rozpatrzeniu"label is consistent with the EN/DE meanings for the OPEN status and keeps the mapper shape unchanged. No functional risk here.packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.ticket-details.mapper.ts (1)
76-80: Mocked PLOPENstatus label aligned with Contentful mappingThe
"W rozpatrzeniu"value keeps mocked data consistent with the Contentful CMS mapper and UI wording, with no structural or behavioral changes.packages/ui/src/components/Filters/Filters.tsx (1)
86-86: LGTM! Mobile button order reversal matches PR objectives.The change from
flex-coltoflex-col-reversecorrectly reverses the button order on mobile viewports, placing the primary "Filters" action above the "Remove filters" action. On desktop (≥ sm breakpoint), theflex-rowlayout preserves the original left-to-right order.packages/blocks/ticket-details/src/frontend/TicketDetails.client.tsx (1)
138-138: LGTM! Styling change aligns with design fix objective.The addition of
bg-card-lightto the attachment container appropriately addresses the PR objective to "fix attachment color to match the design." The class is part of the broader UI theme updates mentioned in the AI summary.packages/blocks/notification-list/src/frontend/NotificationList.client.tsx (1)
80-88: Title column link implementation looks good; confirmdetailsUrlinvariantsThe switch to a custom cell with a link-styled
Buttonnicely matches the requirement to make notification titles clickable while preserving truncation and alignment. One thing to confirm: ifnotification.detailsUrlis ever optional or empty, this will render an invalid link. If that’s possible in any backend integration, consider a guard (e.g., fallback to plain text or disabling the link for such rows).packages/integrations/contentful-cms/src/modules/cms/mappers/mocks/pages/service-details.page.ts (1)
4-4: Service details mock IDs updated consistentlyUpdating the
idto'15'for EN/DE/PL keeps these Contentful mocks in sync across locales and with the mocked integration; no behavioral impact or issues from this change alone.Also applies to: 52-52, 100-100
packages/integrations/mocked/src/modules/cms/mappers/mocks/pages/service-details.page.ts (1)
4-4: Mocked service details IDs aligned with other CMS mocksSetting
idto'15'for all three locales keeps the mocked CMS pages consistent with the Contentful mocks and each other; no functional concerns here.Also applies to: 52-52, 100-100
packages/ui/src/elements/toggle-group.tsx (1)
55-60: ToggleGroupItem refactor and activeIcon rendering look solidThe forwardRef wrapper,
activeIcon/iconPositionprops, andisActivecomputation againstcontext.currentValuecorrectly handle both single and multi-value groups. Explicitly passingvaluethrough toToggleGroupPrimitive.Itempreserves Radix behavior, and the inner flex container cleanly arranges label and optional icon. No issues from this change.Also applies to: 61-97
packages/blocks/orders-summary/src/frontend/OrdersSummary.client.tsx (1)
4-4: Check icon as activeIcon integrates cleanly with ToggleGroupItemPassing
<Check className="h-4 w-4" />asactiveIconfor eachToggleGroupItemties directly into the new toggle-group API and will show a checkmark whenever the item’s value matches the group’s controlledvalue(${range.value}-${range.type}). This is a straightforward UX improvement with no behavioral side effects.Also applies to: 122-126
packages/integrations/mocked/src/modules/cms/mappers/cms.header.mapper.ts (1)
17-17: signInLabel added to mocked headers matches new Header surfaceAdding localized
signInLabelstrings for EN/DE/PL keeps the mocked header data in sync with the extendedHeadermodel and supports the new sign-in button in navigation without altering existing mapping logic.Also applies to: 100-100, 183-183
packages/framework/src/modules/cms/models/header.model.ts (1)
17-17: Header model extended with optional signInLabelAdding
signInLabel?: stringcleanly extends the Header model to support the new sign-in button while remaining backward compatible with existing data where this field is absent.apps/frontend/src/containers/Header/Header.tsx (1)
85-86: LGTM! Consistent prop propagation.The new
signInLabelandisSignedInprops are correctly passed to both navigation components, ensuring consistent sign-in functionality across desktop and mobile breakpoints.Also applies to: 99-100
apps/frontend/src/i18n/routing.ts (1)
6-6: LGTM! Centralized login path constant.Introducing
LOGIN_PATHas an exported constant enables consistent login route references across the codebase and properly integrates with the internationalized routing configuration.Also applies to: 12-16
apps/frontend/src/containers/Header/MobileNavigation/MobileNavigation.types.ts (1)
14-15: LGTM! Type-safe prop additions.The new optional properties align with the CMS header model and mirror the desktop navigation interface, ensuring consistent typing across navigation components.
apps/frontend/src/containers/Header/DesktopNavigation/DesktopNavigation.types.ts (1)
12-13: LGTM! Consistent type definitions.The optional properties maintain type safety through CMS model references and align with the mobile navigation interface.
packages/ui/src/theme.css (2)
64-64: LGTM! Format change with equivalent color value.The
--foregroundvalue change fromhsl(0 0% 98%)to#fafafamaintains the same visual appearance while switching to hex format.
6-6: Verify intentional use of identical--card-lightvalue across themes.The
--card-lightvariable uses the same HSL value (hsl(220 13.04% 95.49%)) in both light and dark themes. Typically, theme variables have different values to ensure proper contrast in each theme context. Confirm this is an intentional design decision and provides sufficient contrast when used in dark theme contexts.Also applies to: 67-67
apps/frontend/src/containers/Header/DesktopNavigation/DesktopNavigation.tsx (2)
39-47: LGTM! Robust login page detection and conditional rendering.The login page detection logic correctly:
- Strips query strings to avoid false negatives
- Checks both the base
LOGIN_PATHand all locale-specific variants from the routing configuration- Handles both exact matches and path prefixes
The
showSignInButtoncondition properly ensures the button only renders when the user is not signed in, not on a login page, andsignInLabelis provided (addressing the CMS mapperundefinedplaceholder).
191-196: LGTM! Accessible sign-in button implementation.The button implementation properly:
- Uses the
Buttoncomponent with appropriate variant- Wraps
NextLinkfor client-side navigation- Provides the CMS-sourced label for localization support
- Conditionally renders based on sign-in state and page context
packages/integrations/contentful-cms/src/modules/cms/mappers/cms.header.mapper.ts (1)
49-49: The UI already handles undefinedsignInLabelgracefully.Both
DesktopNavigation.tsxandMobileNavigation.tsxchecksignInLabelas a truthy condition (const showSignInButton = !isSignedIn && !isLoginPage && signInLabel). WhensignInLabelisundefined, the button simply doesn't render, which is the correct behavior. No action needed.apps/frontend/src/containers/Header/MobileNavigation/MobileNavigation.tsx (3)
19-19: LGTM! Import additions support login detection.The imported
LOGIN_PATHandroutingconstants are correctly used in the login page detection logic below.
32-33: LGTM! Props correctly added.The
signInLabelandisSignedInprops match the type definition and are properly utilized in the component logic.
124-129: LGTM! Sign-in button correctly implemented.The button is properly conditionally rendered, uses the internationalized
NextLinkcomponent which will automatically handle route localization, and includes appropriate styling and accessibility.
# Conflicts: # package-lock.json # packages/ui/src/components/Filters/Filters.tsx
… fix/reported-bugs-fixing
…in button visibility
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/gentle-eggs-report.md (1)
14-14: Enhance changelog message with more specificity.The changeset message is quite generic. Given the PR objectives detail multiple specific fixes across notifications, tickets, orders, and help & support, consider providing a more detailed changelog entry that captures the key improvements (e.g., "added notification and ticket links, reversed filter button order on mobile, unified ticket status names, added sign-in button to navigation, fixed attachment styling, added missing order status and icons").
Also, replace "related with" with "related to" for grammatical accuracy.
Apply this diff to improve the changelog message:
- fixing various bugs related with blocks and ui components, adding sign-in button in navigation + Fixed various bugs: reversed filter/remove buttons on mobile, added links in notifications and tickets, unified status names, fixed attachment colors, added sign-in button to navigation, added missing order statuses and icons.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
.changeset/gentle-eggs-report.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-14T14:21:20.382Z
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 357
File: .changeset/cold-showers-camp.md:1-9
Timestamp: 2025-11-14T14:21:20.382Z
Learning: In the o2sdev/openselfservice repository, new packages should always be added with a 'major' version bump in changesets to start at version 1.0.0, not 'minor'.
Applied to files:
.changeset/gentle-eggs-report.md
⏰ 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: deploy-preview
🔇 Additional comments (1)
.changeset/gentle-eggs-report.md (1)
2-12: Version bump strategy looks appropriate for a bug-fix release.All packages are marked with "minor" version bumps, which is correct for existing packages receiving bug fixes rather than new features. This aligns with standard semantic versioning practices.
… fix/reported-bugs-fixing
… fix/reported-bugs-fixing
2 participants
What does this PR do?
Fixes #162
Key Changes
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.