feat(blocks.notification-summary): add NotificationSummary block#348
feat(blocks.notification-summary): add NotificationSummary block#348
Conversation
…for priority notifications
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughAdds a new NotificationSummary block package with API, SDK, frontend (server/client/renderer), models, mapper, mocked data, Storybook stories, mappings, configs/tsconfigs, and registers the block in API and frontend; updates apps' package.json and a changeset. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Browser as Browser
participant Renderer as Block Renderer
participant Server as Server Component
participant SDK as Block SDK
participant API as NotificationSummary API
participant CMS as CMS Service
participant Notif as Notifications Service
participant Mapper as mapNotificationSummary
Browser->>Renderer: render block props (id, accessToken, routing)
Renderer->>Server: forward props + locale
Server->>SDK: sdk.blocks.getNotificationSummary({id}, headers, auth)
SDK->>API: GET /blocks/notification-summary
par Fetch CMS and notifications concurrently
API->>CMS: getCmsEntry(id, locale)
API->>Notif: getNotificationList(locale, pagination)
end
CMS-->>API: CMS block
Notif-->>API: Notifications list
API->>Mapper: mapNotificationSummary(cms, notifications, locale)
Mapper-->>API: NotificationSummaryBlock (infoCards)
API-->>SDK: response
SDK-->>Server: resolved block
Server->>Browser: render NotificationSummaryPure with infoCards
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30–45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 9
🧹 Nitpick comments (4)
packages/blocks/notification-summary/.prettierrc.mjs (1)
8-23: Well-structured override configuration.The path-based overrides correctly apply formatting presets to the API harmonization, frontend, and SDK directories. The structure aligns well with the monorepo sub-package organization.
Consider adding a base configuration outside the overrides array to ensure consistent formatting for files that don't match any override pattern (e.g., root-level configs, docs, or test files outside the specified directories). For example:
const config = { + ...frontendConfig, // or whichever makes sense as a default overrides: [ { files: "./src/api-harmonization/**/*",This is optional and depends on whether you want non-source files to follow project standards or Prettier defaults.
packages/blocks/notification-summary/src/frontend/NotificationSummary.client.tsx (1)
23-25: Consider using stable keys instead of array index.Using array index as the key can cause rendering issues if the
infoCardsarray is reordered or filtered. If the CMS data includes unique identifiers for each info card (e.g., priority level, id, or a combination of fields), use those instead for more stable component identity.Example if priority or unique ID is available:
-{infoCards.map((infoCard, index) => ( +{infoCards.map((infoCard) => ( <InfoCard - key={index} + key={infoCard.priority || infoCard.id || `${infoCard.title}-${infoCard.value}`}packages/blocks/notification-summary/package.json (1)
52-52: Use less restrictive Next.js version range.Specifying
"next": "^15.3.2"inpeerDependenciesis overly restrictive. Peer dependencies should typically allow broader version ranges to avoid conflicts.Apply this diff to use a more flexible version range:
- "next": "^15.3.2", + "next": "^15",packages/blocks/notification-summary/src/frontend/NotificationSummary.types.ts (1)
14-16: Drop the redundant Omit wrapper
Omit<NotificationSummaryProps, ''>removes nothing and only obscures the intent. ReuseNotificationSummaryPropsdirectly for clarity.-export type NotificationSummaryRendererProps = Omit<NotificationSummaryProps, ''> & { +export type NotificationSummaryRendererProps = NotificationSummaryProps & { slug: string[]; };
📜 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 (34)
.changeset/cold-showers-camp.md(1 hunks)apps/api-harmonization/src/app.module.ts(2 hunks)apps/api-harmonization/src/modules/page/page.model.ts(2 hunks)apps/frontend/src/blocks/renderBlocks.tsx(2 hunks)packages/blocks/notification-summary/.gitignore(1 hunks)packages/blocks/notification-summary/.prettierrc.mjs(1 hunks)packages/blocks/notification-summary/eslint.config.mjs(1 hunks)packages/blocks/notification-summary/lint-staged.config.mjs(1 hunks)packages/blocks/notification-summary/package.json(1 hunks)packages/blocks/notification-summary/src/api-harmonization/index.ts(1 hunks)packages/blocks/notification-summary/src/api-harmonization/notification-summary.client.ts(1 hunks)packages/blocks/notification-summary/src/api-harmonization/notification-summary.controller.ts(1 hunks)packages/blocks/notification-summary/src/api-harmonization/notification-summary.mapper.ts(1 hunks)packages/blocks/notification-summary/src/api-harmonization/notification-summary.model.ts(1 hunks)packages/blocks/notification-summary/src/api-harmonization/notification-summary.module.ts(1 hunks)packages/blocks/notification-summary/src/api-harmonization/notification-summary.request.ts(1 hunks)packages/blocks/notification-summary/src/api-harmonization/notification-summary.service.ts(1 hunks)packages/blocks/notification-summary/src/frontend/NotificationSummary.client.stories.tsx(1 hunks)packages/blocks/notification-summary/src/frontend/NotificationSummary.client.tsx(1 hunks)packages/blocks/notification-summary/src/frontend/NotificationSummary.renderer.tsx(1 hunks)packages/blocks/notification-summary/src/frontend/NotificationSummary.server.tsx(1 hunks)packages/blocks/notification-summary/src/frontend/NotificationSummary.types.ts(1 hunks)packages/blocks/notification-summary/src/frontend/index.ts(1 hunks)packages/blocks/notification-summary/src/sdk/index.ts(1 hunks)packages/blocks/notification-summary/src/sdk/notification-summary.ts(1 hunks)packages/blocks/notification-summary/tsconfig.api.json(1 hunks)packages/blocks/notification-summary/tsconfig.frontend.json(1 hunks)packages/blocks/notification-summary/tsconfig.json(1 hunks)packages/blocks/notification-summary/tsconfig.sdk.json(1 hunks)packages/framework/src/modules/cms/cms.model.ts(1 hunks)packages/framework/src/modules/cms/models/blocks/notification-summary.model.ts(1 hunks)packages/integrations/mocked/src/modules/cms/cms.service.ts(2 hunks)packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.notification-summary.mapper.ts(1 hunks)packages/integrations/mocked/src/modules/cms/mappers/mocks/pages/notification-list.page.ts(3 hunks)
🔇 Additional comments (34)
packages/blocks/notification-summary/eslint.config.mjs (1)
1-18: No issues found—configuration is correctly set up.All verification checks passed:
- Both imported configs (
@o2s/eslint-config/apiand@o2s/eslint-config/frontend-block) are properly exported with theconfignamed export- Directory structure (
src/api-harmonization/,src/frontend/,src/sdk/) exists and matches the glob patterns- Configuration pattern is consistent with all other block packages in the monorepo
packages/blocks/notification-summary/.prettierrc.mjs (3)
1-2: LGTM!The imports correctly reference shared Prettier configuration presets, promoting consistency across the monorepo.
4-7: LGTM!The JSDoc comment provides helpful documentation and type safety.
25-25: LGTM!Standard default export for the Prettier configuration.
packages/blocks/notification-summary/tsconfig.sdk.json (1)
1-19: LGTM!The SDK TypeScript configuration is well-structured, properly extends the base API config, and correctly includes both SDK sources and the necessary API harmonization files for type consistency.
packages/blocks/notification-summary/tsconfig.api.json (1)
1-14: LGTM!The API TypeScript configuration is properly structured and follows project conventions.
packages/blocks/notification-summary/lint-staged.config.mjs (1)
1-4: LGTM!The lint-staged configuration correctly sets up formatting with Prettier and type checking with TypeScript for the appropriate file types.
packages/blocks/notification-summary/tsconfig.frontend.json (1)
1-22: LGTM!The frontend TypeScript configuration properly extends the base frontend config and correctly includes the necessary source files, API harmonization types, and SDK for the NotificationSummary block's frontend build.
packages/blocks/notification-summary/src/frontend/NotificationSummary.client.stories.tsx (1)
1-108: Well-structured Storybook stories for comprehensive testing.The stories provide good coverage of different scenarios:
- Default horizontal layout
- Vertical layout variant
- All priority levels
- Single priority display
This enables effective visual testing and component development.
apps/api-harmonization/src/app.module.ts (2)
37-37: LGTM!The NotificationSummary module import follows the established pattern used by other block modules in the application.
137-137: LGTM!The NotificationSummary module registration is correctly placed with other block module registrations and follows the standard pattern of passing
AppConfigfor configuration.packages/blocks/notification-summary/src/api-harmonization/notification-summary.controller.ts (1)
17-24: Verify authorization consistency for notification-summary endpoint.The
{ roles: [] }pattern is used across 8 controllers (surveyjs, quick-links, faq, article, category, article-search, article-list, category-list), suggesting this is intentional. However, the closely relatednotification-listendpoint requires[Auth.Constants.Roles.ORG_USER, Auth.Constants.Roles.ORG_ADMIN], creating an inconsistency between notification-summary and notification-list.Confirm whether:
- This inconsistency is intentional (e.g., summary intended for public/anonymous access vs. list for authenticated users only)
- Both endpoints should use the same roles requirement
packages/framework/src/modules/cms/cms.model.ts (1)
36-36: LGTM!The export follows the established pattern for block model exports and is correctly positioned.
.changeset/cold-showers-camp.md (1)
1-9: LGTM!The changeset correctly identifies all affected packages and appropriately uses minor version bumps for this new feature addition.
packages/integrations/mocked/src/modules/cms/cms.service.ts (2)
14-14: LGTM!The import is correctly positioned alphabetically and follows the established pattern.
104-106: LGTM!The method implementation follows the established pattern for block retrieval methods, including proper locale handling and response delay.
packages/blocks/notification-summary/tsconfig.json (1)
1-18: LGTM!The TypeScript configuration is correctly set up for a block package with API harmonization, frontend, and SDK layers. The project references and compiler options are appropriate.
packages/integrations/mocked/src/modules/cms/mappers/mocks/pages/notification-list.page.ts (1)
26-29: LGTM!The NotificationSummaryBlock is consistently added to all three locale variants (EN, DE, PL) with identical structure and placement. The positioning before the NotificationListBlock makes logical sense for user experience.
Also applies to: 68-71, 110-113
packages/blocks/notification-summary/src/api-harmonization/notification-summary.service.ts (1)
11-16: LGTM!The service structure and dependency injection are correctly implemented following NestJS patterns.
apps/api-harmonization/src/modules/page/page.model.ts (1)
13-13: LGTM!The import statement is correctly positioned alphabetically, and the Blocks union type is properly extended to include the new NotificationSummaryBlock type.
Also applies to: 77-77
packages/blocks/notification-summary/src/frontend/NotificationSummary.client.tsx (3)
15-20: LGTM!The layout logic correctly handles vertical and horizontal layouts with appropriate responsive classes.
40-42: Verify icon name validation.The type assertion
infoCard.icon as DynamicIconProps['name']bypasses TypeScript's type checking. If the CMS provides an invalid icon name, this could result in rendering issues or errors.Ensure that:
- The CMS validates icon names against the allowed DynamicIcon names, or
- DynamicIcon gracefully handles invalid names (e.g., shows a fallback icon or nothing), or
- Runtime validation is added here before rendering.
If validation isn't present, consider adding a check:
icon={ infoCard.icon && isValidIconName(infoCard.icon) ? ( <DynamicIcon name={infoCard.icon as DynamicIconProps['name']} className={infoCard.color} /> ) : undefined }
32-37: LGTM!The description rendering correctly handles optional content and applies appropriate line clamping with the RichText component.
apps/frontend/src/blocks/renderBlocks.tsx (1)
14-14: LGTM!The NotificationSummary block integration follows the established pattern consistently with other blocks. The import is correctly placed in alphabetical order, and the switch case registration properly forwards all required props.
Also applies to: 133-134
packages/blocks/notification-summary/src/frontend/NotificationSummary.renderer.tsx (1)
17-31: LGTM! Proper Suspense boundary for server component.The Suspense boundary correctly wraps the async server component with an appropriate loading fallback, and all required props are properly forwarded.
packages/blocks/notification-summary/src/api-harmonization/notification-summary.client.ts (1)
1-4: LGTM!The client module follows the established pattern for API harmonization blocks, properly exporting the URL constant and re-exporting Model and Request namespaces.
packages/framework/src/modules/cms/models/blocks/notification-summary.model.ts (1)
1-10: LGTM!The model structure is clean and follows established patterns. The use of optional properties for priority-level InfoCards provides good flexibility for CMS configuration, and the layout union type clearly defines supported orientations.
packages/blocks/notification-summary/src/frontend/index.ts (1)
1-5: LGTM!The barrel export follows the established pattern for frontend block modules, providing clean aliases (Client, Server, Renderer) and a Types namespace for type imports.
packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.notification-summary.mapper.ts (1)
81-92: LGTM! Proper locale-based mock data resolution.The mapper function correctly returns locale-specific mock data with appropriate fallback to English.
packages/blocks/notification-summary/src/sdk/index.ts (2)
15-21: Re-export missing logger types from @o2s/framework/sdk package.The
@ts-expect-errorsuppressions indicate thatLogLevelandLogFormattypes are not accessible from the@o2s/framework/sdkpublic API. While these types are properly defined inpackages/framework/src/utils/logger.tsand are already optional inLoggerConfig, they must be re-exported frompackages/framework/src/sdk.tsto allow proper type checking throughout the codebase. Additionally, environment variables should be type-cast to the expected union types to eliminate the suppressions:logger: { level: (process.env.NEXT_PUBLIC_LOG_LEVEL as LogLevel) || undefined, format: (process.env.NEXT_PUBLIC_LOG_FORMAT as LogFormat) || undefined, colorsEnabled: process.env.NEXT_PUBLIC_LOG_COLORS_ENABLED === 'true', }This pattern is repeated across 25+ SDK files (notification-summary, order-list, order-details, user-account, ticket-, notification-, etc.). Consider adding
LogLevel,LogFormat, andLoggerConfigto the public exports in@o2s/framework/sdk.
1-3: No changes required—the unused Models import is a necessary and consistent pattern.The import is verified to be essential for TypeScript's module augmentation and type resolution. This pattern appears consistently across 17+ SDK files in the codebase (all
packages/blocks/*/src/sdk/index.tsfiles follow the identical structure). TypeScript requires at least one import from a module for the file to be treated as a module, enabling proper type resolution and augmentation merging—the import itself can be unused.packages/blocks/notification-summary/src/api-harmonization/index.ts (1)
1-8: LGTM! Clean barrel export pattern.The index file follows standard TypeScript conventions for re-exporting modules and establishing a clean public API surface for the notification-summary block.
packages/blocks/notification-summary/src/api-harmonization/notification-summary.model.ts (2)
3-9: LGTM! InfoCard model structure is appropriate.The
NotificationSummaryInfoCardclass defines a clean data structure for notification cards with appropriate required (title,value) and optional (icon,description,color) fields.
11-15: LGTM! Block model follows established patterns.The
NotificationSummaryBlockproperly extends the base Block model and uses TypeScript's discriminated union pattern with__typename. The layout union type and infoCards array align well with the PR objectives.
packages/blocks/notification-summary/src/api-harmonization/notification-summary.request.ts
Show resolved
Hide resolved
packages/blocks/notification-summary/src/api-harmonization/notification-summary.service.ts
Show resolved
Hide resolved
packages/blocks/notification-summary/src/frontend/NotificationSummary.client.stories.tsx
Show resolved
Hide resolved
packages/blocks/notification-summary/src/frontend/NotificationSummary.renderer.tsx
Show resolved
Hide resolved
packages/blocks/notification-summary/src/frontend/NotificationSummary.server.tsx
Show resolved
Hide resolved
packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.notification-summary.mapper.ts
Show resolved
Hide resolved
|
the new block needs to be put into package.json in api-harmonization and frontend apps, otherwise there is an error regarding a missing dependency: the easiest way is to use the command from https://www.openselfservice.com/docs/main-components/blocks/usage#installing-an-existing-block |
packages/blocks/notification-summary/src/api-harmonization/notification-summary.mapper.ts
Outdated
Show resolved
Hide resolved
packages/blocks/notification-summary/src/frontend/NotificationSummary.client.tsx
Outdated
Show resolved
Hide resolved
packages/blocks/notification-summary/src/api-harmonization/notification-summary.mapper.ts
Outdated
Show resolved
Hide resolved
packages/blocks/notification-summary/src/frontend/NotificationSummary.renderer.tsx
Outdated
Show resolved
Hide resolved
packages/blocks/notification-summary/src/frontend/NotificationSummary.client.tsx
Outdated
Show resolved
Hide resolved
marcinkrasowski
changed the title
…for priority notifications
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/blocks/notification-summary/src/frontend/NotificationSummary.client.tsx (2)
20-21: Address past comment: expand grid columns for better desktop layout.The current layout uses 2 columns on medium+ screens, but a past comment suggested showing tiles in one row on larger screens. Consider extending the grid to use more columns on desktop breakpoints for better space utilization.
Apply this diff to show tiles in a single row on larger screens:
- const isVertical = layout === 'vertical'; - const containerClass = cn('w-full gap-6', isVertical ? 'flex flex-col' : 'grid grid-cols-1 md:grid-cols-2'); + const containerClass = cn( + 'w-full gap-6', + layout === 'vertical' ? 'flex flex-col' : 'grid grid-cols-1 md:grid-cols-2 lg:grid-cols-3 xl:grid-cols-4' + );Based on past review comments.
25-34: Replace array index with a stable unique key.Using the array index as the
keyprop is a React anti-pattern that can cause rendering issues if the order of items changes. Consider using a stable unique identifier from the data, such as combining the variant with the title, or add anidfield to each info card.If each card is guaranteed to have a unique combination of variant/title, consider:
- return ( <InfoCard - key={index} + key={`${infoCard.variant}-${infoCard.title}`} title={infoCard.title}Or, if the infoCards schema can be extended, add a unique
idfield to each card.packages/blocks/notification-summary/src/frontend/NotificationSummary.client.stories.tsx (1)
1-108: Consider adding an empty state story.The current stories provide good coverage of various layouts and card counts. To complement the empty state handling fix suggested for the client component, consider adding a story that demonstrates the behavior when
infoCardsis an empty array.Add an empty state story:
export const EmptyState: Story = { args: { ...Default.args, infoCards: [], }, };This would help verify that the component correctly handles the case when no notifications are present.
📜 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 (8)
apps/api-harmonization/package.json(1 hunks)apps/frontend/package.json(1 hunks)packages/blocks/notification-summary/src/api-harmonization/notification-summary.mapper.ts(1 hunks)packages/blocks/notification-summary/src/api-harmonization/notification-summary.model.ts(1 hunks)packages/blocks/notification-summary/src/frontend/NotificationSummary.client.stories.tsx(1 hunks)packages/blocks/notification-summary/src/frontend/NotificationSummary.client.tsx(1 hunks)packages/utils/frontend/src/mappings/index.ts(1 hunks)packages/utils/frontend/src/mappings/notification-summary.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/blocks/notification-summary/src/api-harmonization/notification-summary.model.ts
- packages/blocks/notification-summary/src/api-harmonization/notification-summary.mapper.ts
🧰 Additional context used
🪛 Gitleaks (8.29.0)
packages/blocks/notification-summary/src/frontend/NotificationSummary.client.stories.tsx
[high] 47-47: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
🔇 Additional comments (5)
apps/api-harmonization/package.json (1)
39-39: Dependency added correctly; resolves reported build error.The NotificationSummary block dependency is properly registered with correct alphabetical ordering and version constraint format matching all existing block dependencies.
Per the PR comments, please confirm that the corresponding dependency has also been added to the frontend app's
package.jsonto avoid TS2307 errors during the build there as well.apps/frontend/package.json (1)
60-61: Dependency version and placement look correct.The new block dependency follows the established pattern: using wildcard versioning and placed logically after the other surveyjs-form block dependency.
However, the PR comments indicate a build error in the api-harmonization module that requires updating
apps/api/package.jsonas well. Please verify that the new block has also been added to the API package.json file using the installation command from the docs as mentioned in the PR resolution steps.packages/utils/frontend/src/mappings/index.ts (1)
3-3: LGTM! Export follows the established pattern.The NotificationSummary namespace export is correctly placed alphabetically and follows the same barrel export pattern used throughout this file.
packages/utils/frontend/src/mappings/notification-summary.ts (2)
1-1: LGTM! Type definition is clear and explicit.The union type correctly defines all four priority levels mentioned in the PR objectives.
3-8: LGTM! Variant mappings align with the PR requirements.The CSS class mappings correctly implement the color-coding scheme described in the PR:
- CRITICAL/HIGH → destructive styling
- MEDIUM → secondary styling
- LOW → muted styling
The use of
Record<NotificationSummaryVariant, string>ensures type safety and guarantees all variants are mapped.
packages/blocks/notification-summary/src/frontend/NotificationSummary.client.tsx
Show resolved
Hide resolved
…out for better responsiveness
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/blocks/notification-summary/src/frontend/NotificationSummary.client.tsx (1)
41-41: Consider using a stable key instead of array index.Using array
indexas a React key can cause issues if the list is ever reordered, filtered, or modified, leading to incorrect component reconciliation and potential state bugs. If eachinfoCardhas a unique identifier (e.g.,id,variant, or combination of properties), prefer using that instead.Example:
- key={index} + key={infoCard.variant || index}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/blocks/notification-summary/src/frontend/NotificationSummary.client.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/blocks/notification-summary/src/frontend/NotificationSummary.client.tsx (5)
packages/blocks/notification-summary/src/frontend/NotificationSummary.types.ts (1)
NotificationSummaryPureProps(12-12)packages/ui/src/lib/utils.ts (1)
cn(5-7)packages/framework/src/utils/models/info-card.ts (1)
InfoCard(3-9)packages/ui/src/components/DynamicIcon/DynamicIcon.tsx (1)
DynamicIcon(8-38)packages/ui/src/components/DynamicIcon/DynamicIcon.types.ts (1)
DynamicIconProps(1-7)
🔇 Additional comments (2)
packages/blocks/notification-summary/src/frontend/NotificationSummary.client.tsx (2)
17-22: LGTM! Past comment addressed.The early return when
infoCardsis empty or undefined correctly prevents rendering an empty div, addressing the concern raised in previous reviews.
24-28: LGTM! Desktop layout improved.The responsive grid with
lg:grid-cols-4creates a multi-column layout on large screens, addressing the past comment about improving desktop display. The progression from single column (mobile) to 2 columns (tablet) to 4 columns (desktop) provides a good responsive experience.
packages/blocks/notification-summary/src/frontend/NotificationSummary.client.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/blocks/notification-summary/src/frontend/NotificationSummary.renderer.tsx (1)
4-4: Remove unused import.The
Containerimport is not used anywhere in this component.Apply this diff:
-import { Container } from '@o2s/ui/components/Container'; import { Loading } from '@o2s/ui/components/Loading';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/blocks/notification-summary/src/frontend/NotificationSummary.renderer.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-13T15:40:10.509Z
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.509Z
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/notification-summary/src/frontend/NotificationSummary.renderer.tsx
🧬 Code graph analysis (1)
packages/blocks/notification-summary/src/frontend/NotificationSummary.renderer.tsx (3)
packages/blocks/notification-summary/src/frontend/index.ts (2)
NotificationSummaryRenderer(3-3)NotificationSummary(2-2)packages/blocks/notification-summary/src/frontend/NotificationSummary.types.ts (1)
NotificationSummaryRendererProps(14-16)packages/blocks/notification-summary/src/frontend/NotificationSummary.server.tsx (1)
NotificationSummary(12-29)
🔇 Additional comments (2)
packages/blocks/notification-summary/src/frontend/NotificationSummary.renderer.tsx (2)
10-14: Verify the unusedslugprop.The
NotificationSummaryRendererPropstype includes aslug: string[]property (see NotificationSummary.types.ts), but it's not destructured or used in this component. Please confirm whether:
- The
slugprop should be forwarded to the server component- It's intentionally unused and can be removed from the type definition
If
slugshould be used, apply this diff:export const NotificationSummaryRenderer: React.FC<NotificationSummaryRendererProps> = ({ id, accessToken, routing, + slug, }) => {Then forward it to the server component on line 29.
18-27: Verify the fallback UI matches the real component appearance.Based on previous feedback, the fallback should show "one row with two tiles." The current implementation shows two
Loadingcomponents withbars={0}side-by-side. Please confirm this accurately represents the expected layout and appearance of the actual NotificationSummary component when loaded.
packages/blocks/notification-summary/src/api-harmonization/notification-summary.model.ts
Outdated
Show resolved
Hide resolved
packages/blocks/notification-summary/src/api-harmonization/notification-summary.mapper.ts
Outdated
Show resolved
Hide resolved
packages/blocks/notification-summary/src/api-harmonization/notification-summary.mapper.ts
Outdated
Show resolved
Hide resolved
… use NotificationPriority type
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/blocks/notification-summary/src/api-harmonization/notification-summary.model.ts (1)
12-16: Clean block model implementation.The block model is well-structured and aligns with the PR objectives. The optional
layoutfield with specific string literals provides type-safe configuration options, and the requiredinfoCardsarray ensures the block always has content to display.Consider adding JSDoc comments to document the purpose of each field and expected values. For example:
+/** + * Represents an individual notification summary card displaying count and details for a priority level. + */ export class NotificationSummaryInfoCard { + /** Display title for the card */ title!: string; + /** Optional icon identifier */ icon?: string; + /** Notification count for this priority level */ value!: number; + /** Optional description text */ description?: string; + /** Priority level determining card styling */ variant!: Notifications.Model.NotificationPriority; } + +/** + * Block component that displays notification counts grouped by priority. + */ export class NotificationSummaryBlock extends Models.Block.Block { __typename!: 'NotificationSummaryBlock'; + /** Layout orientation: vertical stack or horizontal row */ layout?: 'vertical' | 'horizontal'; + /** Collection of priority-grouped notification cards */ infoCards!: NotificationSummaryInfoCard[]; }packages/blocks/notification-summary/src/api-harmonization/notification-summary.mapper.ts (1)
22-22: No type safety violation found—hardcoded values match theNotificationPrioritytype definition.Verification confirms that all four hardcoded priorities (
'CRITICAL','HIGH','MEDIUM','LOW') are valid according to theNotificationPrioritytype definition. Theas-cast works correctly, and there's no current runtime risk.However, for better maintainability and to future-proof the code against type changes, consider importing and using the actual type values instead of relying on the
as-cast.
📜 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 (2)
packages/blocks/notification-summary/src/api-harmonization/notification-summary.mapper.ts(1 hunks)packages/blocks/notification-summary/src/api-harmonization/notification-summary.model.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-13T15:35:13.879Z
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 348
File: packages/blocks/notification-summary/src/api-harmonization/notification-summary.service.ts:23-27
Timestamp: 2025-11-13T15:35:13.879Z
Learning: In the notification-summary block service (packages/blocks/notification-summary/src/api-harmonization/notification-summary.service.ts), the hardcoded limit of 1000 notifications when calling notificationService.getNotificationList() is intentional, as there is no server-side aggregation API available for fetching notification counts by priority.
Applied to files:
packages/blocks/notification-summary/src/api-harmonization/notification-summary.mapper.tspackages/blocks/notification-summary/src/api-harmonization/notification-summary.model.ts
🧬 Code graph analysis (1)
packages/blocks/notification-summary/src/api-harmonization/notification-summary.mapper.ts (2)
packages/blocks/notification-summary/src/api-harmonization/notification-summary.model.ts (2)
NotificationSummaryBlock(12-16)NotificationSummaryInfoCard(5-11)packages/framework/src/modules/cms/models/blocks/notification-summary.model.ts (1)
NotificationSummaryBlock(3-10)
🔇 Additional comments (5)
packages/blocks/notification-summary/src/api-harmonization/notification-summary.model.ts (2)
1-3: LGTM!The imports are clean and appropriate for the model definitions.
5-11: Well-structured model with proper type reuse.The implementation correctly addresses previous review feedback by making
variantrequired and reusingNotifications.Model.NotificationPriority. This ensures every info card has an associated priority level and maintains type consistency across the codebase.packages/blocks/notification-summary/src/api-harmonization/notification-summary.mapper.ts (3)
12-18: LGTM! Clean aggregation logic.The priority counting logic is clear and type-safe, correctly using the
Record<NotificationPriority, number>type as suggested in previous review feedback.
24-42: LGTM! Info cards building logic is sound.The forEach implementation correctly:
- Maps priority levels to CMS configuration fields via lowercasing
- Handles optional CMS fields with the conditional check
- Maps CMS
messagefield to outputdescriptionfield- Provides a fallback value of 0 for missing priority counts
The type assertion, while verbose, ensures type safety when accessing the CMS fields.
44-50: LGTM! Return statement correctly maps all fields.The return object properly constructs the
NotificationSummaryBlockwith all required fields including the GraphQL__typename, id, layout, and the assembled info cards.
|
The new Notification Summary component has been merged in a separate PR together with Ticket Summary. |
2 participants
Key Changes
Added NotificationSummary Block: Created a new block component that displays notification counts grouped by priority (CRITICAL, HIGH, MEDIUM, LOW).
Priority-based Summary Display: The block aggregates notifications by priority and displays them using
InfoCardcomponents, showing:Layout Options: Implemented support for both vertical and horizontal layouts, configurable via CMS (
layout: 'vertical' | 'horizontal').Code Quality Improvements:
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.