fix: handling various linter warnings #486
Conversation
… server components
… fix/handling-various-linter-warnings
… fix/handling-various-linter-warnings
WalkthroughRefactors many server-side block components to use a single post-try/catch render path, tightens linting by adding --max-warnings=0 across packages and adding a docs-specific ESLint config, removes unused docs imports/components, applies small JSX/text escapes and anchor attribute tweaks, and converts many runtime imports to type-only imports. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (27)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (19)
⏰ 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)
🔇 Additional comments (7)
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 |
… fix/handling-various-linter-warnings
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (12)
packages/blocks/featured-service-list/src/frontend/FeaturedServiceList.server.tsx (1)
19-41: Unified return pattern looks good; consider adding type annotation.The refactoring to use a unified return-path pattern (declare → fetch → return component) improves consistency across the codebase and likely addresses linter warnings about control flow.
However,
let data;lacks an explicit type annotation, which may reduce type safety in strict TypeScript configurations.💡 Consider adding an explicit type annotation:
- let data; + let data: Awaited<ReturnType<typeof sdk.blocks.getFeaturedServiceList>>; try { data = await sdk.blocks.getFeaturedServiceList(packages/blocks/payments-history/src/frontend/PaymentsHistory.server.tsx (1)
13-13: Add explicit type annotation for type safety.The untyped
let data;declaration may prevent TypeScript from properly inferring types, which could allow prop mismatches when spreading{...data}intoPaymentsHistoryDynamicto go undetected.🔎 Add explicit type annotation:
- let data; + let data: Awaited<ReturnType<typeof sdk.blocks.getPaymentsHistory>>;This ensures TypeScript validates the spread props against
PaymentsHistoryDynamic's expected prop types.packages/blocks/pricing-section/src/frontend/PricingSection.server.tsx (1)
19-31: LGTM! Clean refactoring to unified data flow.The pattern correctly separates data fetching from rendering. The control flow ensures
datais always assigned before use (early return on error prevents undefined access).Optional: Add explicit type annotation for improved clarity
While TypeScript's control flow analysis handles this correctly, an explicit type annotation can improve code clarity:
- let data; + let data: Awaited<ReturnType<typeof sdk.blocks.getPricingSection>>;This makes the intended type immediately visible to readers.
packages/blocks/ticket-details/src/frontend/TicketDetails.server.tsx (1)
20-34: Verify TypeScript type inference for the uninitialized variable.The refactored pattern with
let data;declared outside the try block is safe from a control-flow perspective. However, without an explicit type annotation, TypeScript must infer the type solely from the assignment on line 22, which may reduce type precision or IDE support.Consider adding an explicit type annotation to improve maintainability:
🔎 Suggested refactor with explicit typing:
- let data; + let data: Awaited<ReturnType<typeof sdk.blocks.getTicketDetails>>; try { data = await sdk.blocks.getTicketDetails(Additionally, please verify which specific ESLint rule this pattern addresses, as documenting the rationale would help future maintainers understand why this pattern was chosen over the more concise early-return approach.
packages/modules/surveyjs/src/frontend/Questions/CustomSurveyQuestionText.tsx (1)
113-116: Same direct mutation pattern repeated.This handler also directly mutates
question.valuewith the same ESLint suppression. If the verification confirms that direct mutation is the correct SurveyJS pattern, consider adding a shared comment at the component level explaining this design decision, rather than repeating suppressions throughout the code.packages/blocks/order-details/src/frontend/OrderDetails.server.tsx (2)
21-21: Add explicit type annotation to improve type safety.The
datavariable is declared without a type annotation, which may compromise type safety in strict TypeScript environments. Consider adding an explicit type annotation to ensure TypeScript can properly validate the usage ofdatathroughout the function.🔎 Apply this diff to add explicit type annotation:
- let data; + let data: Awaited<ReturnType<typeof sdk.blocks.getOrderDetails>>;
21-47: Verify the benefits of the unified return-path pattern.This refactor introduces a single return path by moving the final render outside the try-catch block. While this creates consistency across similar components (as mentioned in the PR summary), it also introduces some tradeoffs:
Tradeoffs to consider:
- Requires a mutable
let datavariable instead of a more idiomaticconst data = await ...with return inside try- The early-return pattern (returning inside try-catch blocks) is more common in React server components and arguably more readable
- The control flow is slightly less obvious compared to explicit returns in each code path
Questions:
- Which specific ESLint rule or coding standard is this pattern addressing?
- Has the team evaluated whether the consistency benefit outweighs the readability tradeoff?
The pattern is functional and logically sound, but consider documenting the rationale in your team's coding guidelines if this becomes the standard pattern for server components.
apps/docs/src/components/CookieConsent/index.tsx (1)
7-7: Consider removing the commented import instead of leaving it.Commented-out code adds noise and should generally be removed rather than preserved. If this import is no longer needed, delete the line entirely. If it may be needed in the future, version control already tracks the history.
🔎 Suggested fix
-// import styles from '../../css/cookie-consent.css';apps/docs/eslint.config.mjs (1)
10-11: Consider keeping thereact/no-unescaped-entitiesrule enabled.While the comment indicates this is to "Allow unescaped entities (apostrophes) in documentation text," the codebase already properly escapes apostrophes (e.g., using
'inHomepageFeatures/index.tsxline 34). Disabling this rule may allow unescaped entities to slip through in future changes, reducing consistency and potentially causing issues with certain characters.🔎 Consider this adjustment:
- // Allow unescaped entities (apostrophes) in documentation text - 'react/no-unescaped-entities': 'off',apps/docs/src/components/DXPFeaturesSection/index.tsx (1)
10-17: Consider removing unusedbuttonTextandbuttonUrlprops entirely.The
buttonTextandbuttonUrlproperties are defined in theFeatureCardPropsinterface (lines 14-15), provided in thefeaturesarray (lines 28-29, 40-41, 52-53), but are now commented out in the component's destructuring (lines 62-63) and not used since the button UI is also commented out (lines 92-94). This creates dead code that should be cleaned up.🔎 Complete the cleanup by removing these unused props:
Update the interface:
interface FeatureCardProps { title: string; icon: React.ReactNode; features: string[]; - buttonText: string; - buttonUrl: string; borderColor?: 'gradient' | 'blue' | 'green' | 'light'; }Remove from each feature in the array:
{ title: 'Modern Frontend Foundation', icon: ( <div className="absolute inset-[16.67%_8.33%]"> <CodeIcon className="w-7 h-7 text-[#21d99a]" /> </div> ), features: ['Next.js based frontend', 'UI components & content types', 'Optimized for SEO, a11y & web perf'], - buttonText: 'Learn more', - buttonUrl: '/docs/app-starters/dxp/overview', borderColor: 'light', },And remove the commented destructuring in the component:
const FeatureCard: React.FC<FeatureCardProps> = ({ title, icon, features, - // buttonText, - // buttonUrl, borderColor = 'blue', }) => {Also applies to: 19-56, 58-65
packages/blocks/payments-summary/src/frontend/PaymentsSummary.server.tsx (1)
19-43: Refactoring maintains correctness.The control-flow refactor is functionally equivalent and safe—since the catch block returns early,
datais guaranteed to be assigned before the final render. This pattern aligns with the PR objective of resolving linter warnings.Optional improvement for type safety:
Consider adding a type annotation to
datafor better TypeScript inference:🔎 View suggestion
- let data; + let data: Awaited<ReturnType<typeof sdk.blocks.getPaymentsSummary>>;Note: This same pattern appears across all 8 files in this review; subsequent file comments will be brief.
packages/blocks/ticket-summary/src/frontend/TicketSummary.server.tsx (1)
13-13: Add explicit type annotation todatafor better type safety.Declaring
datawithout a type annotation means TypeScript will initially infer it asundefined. While control flow analysis should understand thatdatais assigned before line 27 (since the catch block returns early), adding an explicit type annotation improves clarity and type safety.🔎 Apply this diff to add a type annotation:
- let data; + let data: Awaited<ReturnType<typeof sdk.blocks.getTicketSummary>>;
📜 Review details
Configuration used: Organization 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 (66)
apps/docs/eslint.config.mjs(1 hunks)apps/docs/package.json(1 hunks)apps/docs/src/components/BenefitsSection/index.tsx(0 hunks)apps/docs/src/components/CookieConsent/index.tsx(1 hunks)apps/docs/src/components/DXPComplexPlatformsSection/index.tsx(1 hunks)apps/docs/src/components/DXPFeaturesSection/index.tsx(1 hunks)apps/docs/src/components/GetStartedButtons/index.tsx(1 hunks)apps/docs/src/components/HomepageArchitectureSection/index.tsx(0 hunks)apps/docs/src/components/HomepageFeatures/index.tsx(4 hunks)apps/docs/src/components/HomepageFeaturesSection/index.tsx(1 hunks)apps/docs/src/components/HomepageJoinTheWaitlistSection/index.tsx(2 hunks)apps/docs/src/components/HubspotForm/index.tsx(1 hunks)apps/docs/src/components/SubscribeSection/index.tsx(1 hunks)apps/docs/src/components/WaitlistForm/index.tsx(1 hunks)apps/docs/src/pages/contact/index.tsx(1 hunks)apps/docs/src/pages/dxp/index.tsx(0 hunks)apps/docs/src/pages/index.tsx(1 hunks)apps/docs/src/pages/product/features.tsx(1 hunks)apps/docs/src/pages/product/integrations.tsx(1 hunks)apps/docs/src/pages/product/starters.tsx(1 hunks)apps/docs/src/pages/support/developers.tsx(0 hunks)apps/docs/src/theme/Blog/Components/Author/index.tsx(0 hunks)apps/docs/src/theme/BlogPostItem/Header/Info/index.tsx(0 hunks)apps/docs/src/theme/Footer/Copyright/index.tsx(0 hunks)apps/docs/src/theme/Footer/Layout/index.tsx(0 hunks)apps/docs/src/theme/Footer/Links/MultiColumn/index.tsx(1 hunks)apps/docs/src/theme/Footer/Links/Simple/index.tsx(1 hunks)apps/docs/src/theme/Footer/index.tsx(0 hunks)apps/docs/src/theme/NavbarItem/DropdownNavbarItem/index.tsx(2 hunks)package.json(1 hunks)packages/blocks/article-list/src/frontend/ArticleList.server.tsx(1 hunks)packages/blocks/article-search/src/frontend/ArticleSearch.server.tsx(1 hunks)packages/blocks/article/src/frontend/Article.server.tsx(1 hunks)packages/blocks/bento-grid/src/frontend/BentoGrid.server.tsx(1 hunks)packages/blocks/category/src/frontend/Category.server.tsx(1 hunks)packages/blocks/cta-section/src/frontend/CtaSection.server.tsx(1 hunks)packages/blocks/document-list/src/frontend/DocumentList.server.tsx(1 hunks)packages/blocks/faq/src/frontend/Faq.server.tsx(1 hunks)packages/blocks/feature-section-grid/src/frontend/FeatureSectionGrid.server.tsx(1 hunks)packages/blocks/feature-section/src/frontend/FeatureSection.server.tsx(1 hunks)packages/blocks/featured-service-list/src/frontend/FeaturedServiceList.renderer.tsx(1 hunks)packages/blocks/featured-service-list/src/frontend/FeaturedServiceList.server.tsx(1 hunks)packages/blocks/hero-section/src/frontend/HeroSection.server.tsx(1 hunks)packages/blocks/invoice-list/src/frontend/InvoiceList.renderer.tsx(0 hunks)packages/blocks/invoice-list/src/frontend/InvoiceList.server.tsx(1 hunks)packages/blocks/media-section/src/frontend/MediaSection.server.tsx(1 hunks)packages/blocks/notification-details/src/frontend/NotificationDetails.server.tsx(2 hunks)packages/blocks/notification-list/src/frontend/NotificationList.server.tsx(1 hunks)packages/blocks/notification-summary/src/frontend/NotificationSummary.server.tsx(1 hunks)packages/blocks/order-details/src/frontend/OrderDetails.server.tsx(2 hunks)packages/blocks/order-list/src/frontend/OrderList.server.tsx(1 hunks)packages/blocks/orders-summary/src/frontend/OrdersSummary.server.tsx(2 hunks)packages/blocks/payments-history/src/frontend/PaymentsHistory.server.tsx(2 hunks)packages/blocks/payments-summary/src/frontend/PaymentsSummary.server.tsx(2 hunks)packages/blocks/pricing-section/src/frontend/PricingSection.server.tsx(1 hunks)packages/blocks/product-list/src/frontend/ProductList.server.tsx(1 hunks)packages/blocks/service-details/src/frontend/ServiceDetails.server.tsx(2 hunks)packages/blocks/service-list/src/frontend/ServiceList.server.tsx(1 hunks)packages/blocks/surveyjs-form/src/frontend/SurveyJs.server.tsx(1 hunks)packages/blocks/ticket-details/src/frontend/TicketDetails.server.tsx(2 hunks)packages/blocks/ticket-recent/src/frontend/TicketRecent.renderer.tsx(0 hunks)packages/blocks/ticket-recent/src/frontend/TicketRecent.server.tsx(1 hunks)packages/blocks/ticket-summary/src/frontend/TicketSummary.server.tsx(1 hunks)packages/blocks/user-account/src/frontend/UserAccount.server.tsx(1 hunks)packages/modules/surveyjs/eslint.config.mjs(1 hunks)packages/modules/surveyjs/src/frontend/Questions/CustomSurveyQuestionText.tsx(2 hunks)
💤 Files with no reviewable changes (11)
- apps/docs/src/pages/dxp/index.tsx
- packages/blocks/ticket-recent/src/frontend/TicketRecent.renderer.tsx
- apps/docs/src/components/BenefitsSection/index.tsx
- apps/docs/src/theme/Footer/Layout/index.tsx
- packages/blocks/invoice-list/src/frontend/InvoiceList.renderer.tsx
- apps/docs/src/components/HomepageArchitectureSection/index.tsx
- apps/docs/src/theme/BlogPostItem/Header/Info/index.tsx
- apps/docs/src/theme/Footer/index.tsx
- apps/docs/src/pages/support/developers.tsx
- apps/docs/src/theme/Blog/Components/Author/index.tsx
- apps/docs/src/theme/Footer/Copyright/index.tsx
🧰 Additional context used
🧠 Learnings (4)
📚 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/frontend/NotificationSummary.server.tsx
📚 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/featured-service-list/src/frontend/FeaturedServiceList.renderer.tsx
📚 Learning: 2025-12-03T12:34:20.707Z
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 419
File: apps/frontend/package.json:52-52
Timestamp: 2025-12-03T12:34:20.707Z
Learning: In the openselfservice repository, the application does not use Next.js cache components (use cache/cacheComponents), so next-intl compatibility limitations related to those experimental features are not a concern.
Applied to files:
packages/blocks/featured-service-list/src/frontend/FeaturedServiceList.renderer.tsx
📚 Learning: 2025-11-14T14:34:25.489Z
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 357
File: packages/blocks/ticket-summary/eslint.config.mjs:1-18
Timestamp: 2025-11-14T14:34:25.489Z
Learning: ESLint v9+ provides `defineConfig` as an export from 'eslint/config' module, which supports an `extends` array property in flat config objects for composing configurations.
Applied to files:
packages/modules/surveyjs/eslint.config.mjsapps/docs/eslint.config.mjs
🧬 Code graph analysis (12)
packages/blocks/notification-summary/src/frontend/NotificationSummary.server.tsx (1)
packages/blocks/notification-summary/src/sdk/index.ts (1)
sdk(24-28)
packages/blocks/cta-section/src/frontend/CtaSection.server.tsx (1)
apps/frontend/src/i18n/routing.ts (1)
routing(8-18)
packages/blocks/bento-grid/src/frontend/BentoGrid.server.tsx (1)
apps/frontend/src/i18n/routing.ts (1)
routing(8-18)
packages/blocks/product-list/src/frontend/ProductList.server.tsx (1)
packages/blocks/product-list/src/sdk/index.ts (1)
sdk(24-28)
packages/blocks/invoice-list/src/frontend/InvoiceList.server.tsx (1)
apps/frontend/src/i18n/routing.ts (1)
routing(8-18)
packages/blocks/order-details/src/frontend/OrderDetails.server.tsx (1)
apps/frontend/src/i18n/routing.ts (1)
routing(8-18)
packages/blocks/pricing-section/src/frontend/PricingSection.server.tsx (1)
apps/frontend/src/i18n/routing.ts (1)
routing(8-18)
packages/blocks/faq/src/frontend/Faq.server.tsx (1)
apps/frontend/src/i18n/routing.ts (1)
routing(8-18)
packages/blocks/hero-section/src/frontend/HeroSection.server.tsx (1)
apps/frontend/src/i18n/routing.ts (1)
routing(8-18)
packages/blocks/ticket-summary/src/frontend/TicketSummary.server.tsx (3)
packages/blocks/notification-summary/src/sdk/index.ts (1)
sdk(24-28)packages/blocks/ticket-summary/src/sdk/index.ts (1)
sdk(24-28)apps/frontend/src/i18n/routing.ts (1)
routing(8-18)
packages/blocks/user-account/src/frontend/UserAccount.server.tsx (2)
apps/frontend/src/i18n/routing.ts (1)
routing(8-18)apps/frontend/src/auth/auth.config.ts (1)
onSignOut(41-41)
packages/blocks/article-list/src/frontend/ArticleList.server.tsx (1)
apps/frontend/src/i18n/routing.ts (1)
routing(8-18)
🪛 ast-grep (0.40.0)
apps/docs/src/theme/Footer/Links/Simple/index.tsx
[warning] 16-16: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
apps/docs/src/theme/Footer/Links/MultiColumn/index.tsx
[warning] 15-15: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
⏰ 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 (50)
apps/docs/src/components/DXPComplexPlatformsSection/index.tsx (1)
31-31: LGTM! Apostrophe properly escaped in JSX text.This change correctly escapes the apostrophe using the HTML entity
', addressing the ESLintreact/no-unescaped-entitieswarning. While functionally equivalent to the literal apostrophe, this follows JSX best practices and aligns with the PR's goal of achieving zero linter warnings.packages/blocks/featured-service-list/src/frontend/FeaturedServiceList.renderer.tsx (1)
9-9: LGTM! Unused prop removed.Removing the unused
slugprop appropriately addresses the linter warning without affecting functionality.apps/docs/src/theme/NavbarItem/DropdownNavbarItem/index.tsx (2)
43-43: LGTM: Correct prop filtering to prevent forwarding.The underscore-prefixed destructuring (
_onClick) correctly prevents theonClickprop from being forwarded via...propstoNavbarNavLink. Since line 87 already defines its ownonClickhandler, this avoids potential prop conflicts while satisfying the linter's unused-variable check.
124-124: LGTM: Proper prop exclusion for mobile context.The underscore-prefixed destructuring (
_position) correctly prevents thepositionprop from being forwarded in the mobile context where it's not applicable. The inline comment clearly documents the intent, and this pattern aligns with standard ESLint conventions for intentionally unused variables.packages/blocks/payments-history/src/frontend/PaymentsHistory.server.tsx (1)
28-30: LGTM! Single return path maintains correct error handling.The refactored control flow correctly preserves the error handling semantics while providing a unified return path. The early return in the catch block (line 25) ensures
datais always defined when reaching this return statement.packages/blocks/pricing-section/src/frontend/PricingSection.server.tsx (1)
33-42: LGTM! Unified return path improves code clarity.Moving the render logic outside the try/catch block creates a cleaner separation of concerns: data fetching → error handling → rendering. This pattern enhances readability and makes the component's flow easier to follow.
packages/blocks/ticket-details/src/frontend/TicketDetails.server.tsx (1)
36-46: LGTM—Consistent unified return pattern.The single return statement after the try-catch block successfully unifies the rendering path. The control flow guarantees that
datais assigned before reaching this point, and spreading the data props intoTicketDetailsDynamicis safe and correct.packages/modules/surveyjs/src/frontend/Questions/CustomSurveyQuestionText.tsx (1)
92-96: SurveyJS uses direct property mutation—the ESLint suppression is correct.SurveyJS documentation confirms that to access and update a question value, you assign a new value to the question's value property. Official SurveyJS examples for custom widgets use direct assignment (e.g.,
question.value = editor.getData()). Thequestionobject is a mutable model fromsurvey-core, not an immutable React prop, so the ESLint suppression is appropriate and correctly applied.Likely an incorrect or invalid review comment.
packages/blocks/notification-details/src/frontend/NotificationDetails.server.tsx (1)
36-46: Pattern aligns with PR goals.The single-return refactor successfully addresses linter warnings while maintaining correct runtime behavior. The control flow ensures
datais properly initialized before spreading (catch block returns early). With the type annotation suggested above, this pattern provides both linter compliance and type safety.apps/docs/src/components/SubscribeSection/index.tsx (1)
100-109: Underscore convention for unused catch parameter is appropriate.The
_errorrename correctly signals the intentionally unused variable and silences the linter. However, consider whether logging the actual error (even to console in development) would aid debugging network failures.apps/docs/src/components/WaitlistForm/index.tsx (1)
73-79: LGTM!Consistent use of the underscore prefix convention to indicate an intentionally unused catch parameter, matching the pattern in
SubscribeSection.package.json (1)
11-11: Good enforcement of zero-warning policy.Adding
--max-warnings=0ensures that warnings are treated as errors, preventing gradual accumulation of linter warnings across the codebase. This aligns well with the PR objectives.packages/modules/surveyjs/eslint.config.mjs (1)
1-17: LGTM!The quote style normalization is consistent with the broader PR changes. The ESLint flat config structure using
defineConfigwith theextendsarray property is appropriate for ESLint v9+. Based on learnings, this pattern is the correct approach for composing configurations.apps/docs/src/components/HubspotForm/index.tsx (1)
225-225: LGTM! Proper handling of unused catch parameter.Prefixing the unused error parameter with an underscore follows TypeScript/ESLint conventions and eliminates linter warnings without changing behavior.
apps/docs/src/pages/product/features.tsx (1)
289-291: LGTM! Proper apostrophe escaping in JSX.Using the HTML entity
'for apostrophes in JSX text content is correct and prevents potential parsing issues. This change aligns with the broader text escaping improvements across the docs site.apps/docs/package.json (1)
16-16: LGTM! Enhanced lint script with TypeScript and ESLint.The updated lint script now runs both TypeScript type-checking and ESLint with auto-fix, which aligns perfectly with the PR's goal of enforcing stricter linting standards across the codebase.
apps/docs/src/pages/contact/index.tsx (1)
46-49: LGTM! Proper apostrophe escaping in JSX.The change to use
'HTML entity and wrap the content in a JSX fragment is correct and consistent with the text escaping improvements throughout this PR.apps/docs/src/pages/product/integrations.tsx (1)
271-272: LGTM! Proper apostrophe escaping in JSX.The change to use
'HTML entity for the apostrophe in "That's" is correct and consistent with the text escaping improvements applied throughout the docs site in this PR.apps/docs/src/components/HomepageFeaturesSection/index.tsx (1)
7-7: LGTM! Clean removal of unused import.The removal of
H4from the Typography imports aligns with the file's actual usage and contributes to the linting cleanup goals of this PR.apps/docs/eslint.config.mjs (1)
8-9: LGTM! Appropriate rule override for Docusaurus.Disabling
@next/next/no-img-elementis correct since Docusaurus doesn't use Next.js's optimized Image component.apps/docs/src/components/HomepageJoinTheWaitlistSection/index.tsx (1)
35-35: LGTM! Security best practice applied.Adding
rel="noreferrer"to external links withtarget="_blank"prevents the new page from accessingwindow.openerand prevents referrer information leakage, improving the security posture of the application.Also applies to: 45-45
apps/docs/src/components/GetStartedButtons/index.tsx (2)
2-2: LGTM! Unused import removed.Removing the unused
useStateimport cleans up the code and aligns with the linting improvements in this PR.
11-16: LGTM! Security attributes properly applied.The addition of
rel="noreferrer"(line 15) and updating torel="noopener noreferrer"(line 23) on external links follows security best practices by preventing referrer leakage and blocking access towindow.opener.Also applies to: 23-23
apps/docs/src/components/HomepageFeatures/index.tsx (2)
4-7: LGTM! Cleaner static imports replace dynamic require().Replacing
require(...)with static named imports improves code clarity, enables better tree-shaking, and provides better TypeScript support.
21-21: LGTM! Svg assignments updated correctly.The Svg properties now reference the statically imported components, consistent with the import changes at the top of the file.
Also applies to: 31-31, 41-41
apps/docs/src/pages/index.tsx (1)
19-19: LGTM! Unused import removed.Removing
BodyBoldfrom the Typography imports aligns with the linting cleanup, as the component is not used in the active code.packages/blocks/feature-section-grid/src/frontend/FeatureSectionGrid.server.tsx (1)
13-27: The control flow refactoring is logically sound.The success path assigns
dataand returns the component on Line 27, while the error path returnsnullfrom the catch block. However, I could not identify which specific ESLint rule from the repository's configuration (TypeScript ESLint recommended) requires this single-return-path pattern over the previous approach of returning directly from within the try block. Both patterns are valid and commonly used in JavaScript/TypeScript projects. If this change was made to satisfy a linter warning, consider documenting which rule it addresses for future maintainers.packages/blocks/user-account/src/frontend/UserAccount.server.tsx (1)
25-50: The current code is type-safe and does not require an explicit type annotation.With
strict: trueenabled in the project's TypeScript configuration, control flow analysis correctly recognizes thatdatais always assigned before use because the catch block returns early. No explicit type annotation is needed for correctness, and the code compiles without errors.Likely an incorrect or invalid review comment.
packages/blocks/product-list/src/frontend/ProductList.server.tsx (2)
26-26: Final return is functionally correct.The return statement correctly spreads the fetched
datatoProductListDynamic. This will work as expected if the type safety issue with thedatavariable (flagged above) is addressed.
13-24: This pattern is not a type safety concern—TypeScript's control flow analysis handles this correctly.The
let data;declaration without explicit type annotation is safe here because:
- TypeScript's control flow analysis recognizes that
datais always assigned in the try block before reaching the return statement (line 26), since the catch block returns early.- The function signature specifies
strict: truein tsconfig, enabling full type checking.- This pattern is consistently used across 34+ server components in this codebase without type errors.
- At line 26, TypeScript correctly infers
dataasProductListBlock, matching the expected type for spreading intoProductListDynamic.The refactor maintains the same type safety as the original code.
packages/blocks/category/src/frontend/Category.server.tsx (1)
33-45: This pattern is type-safe and consistent across the codebase—no action needed.The early return in the catch block (line 30) prevents
datafrom being used uninitialized, making the control flow safe. This guard-clause pattern is consistently applied across 30+ block components and appears to be an intentional architectural choice. No ESLint rule in the project configuration enforces or necessitates this pattern, confirming it is a deliberate design decision rather than a workaround for a linter warning.Likely an incorrect or invalid review comment.
packages/blocks/hero-section/src/frontend/HeroSection.server.tsx (1)
13-36: Refactoring pattern looks good.The control-flow restructuring to use a single return path after try-catch is consistent with the broader PR pattern and successfully addresses linter warnings. The catch block ensures
datais never used uninitialized.Optional: Add explicit TypeScript typing for better type safety
- let data; + let data: Awaited<ReturnType<typeof sdk.blocks.getHeroSection>>; try {This preserves type information that would otherwise be inferred.
packages/blocks/surveyjs-form/src/frontend/SurveyJs.server.tsx (1)
17-39: LGTM - Consistent refactoring pattern.This follows the same unified return-path pattern as other server components in the PR, successfully addressing linter requirements while maintaining functional equivalence.
packages/blocks/service-details/src/frontend/ServiceDetails.server.tsx (1)
20-46: LGTM - Consistent with PR pattern.The refactoring aligns with the unified return-path approach used across all server components in this PR.
packages/blocks/order-list/src/frontend/OrderList.server.tsx (1)
11-33: LGTM - Refactoring pattern applied correctly.This maintains the consistent single-return-path approach used throughout the PR.
packages/blocks/article-list/src/frontend/ArticleList.server.tsx (1)
20-43: LGTM - Consistent refactoring approach.The unified return path successfully addresses linter warnings while maintaining functional equivalence.
packages/blocks/notification-list/src/frontend/NotificationList.server.tsx (1)
19-41: LGTM - Pattern applied correctly.This follows the consistent refactoring pattern used across all server components in the PR.
packages/blocks/article-search/src/frontend/ArticleSearch.server.tsx (1)
19-41: LGTM - Refactoring pattern consistent.The single-return-path approach aligns with the PR-wide pattern and successfully addresses linter requirements.
packages/blocks/notification-summary/src/frontend/NotificationSummary.server.tsx (1)
13-27: LGTM - Refactoring pattern applied correctly.This completes the consistent refactoring across all server components, maintaining the unified return-path approach while preserving error logging.
packages/blocks/bento-grid/src/frontend/BentoGrid.server.tsx (1)
11-25: LGTM—consistent refactoring pattern.Same control-flow refactor as other files in this PR. Functionally correct. Optionally add type annotation to
dataas suggested in PaymentsSummary.server.tsx.packages/blocks/invoice-list/src/frontend/InvoiceList.server.tsx (1)
19-41: LGTM—consistent refactoring pattern.Control-flow refactor is correct. Optionally add type annotation to
datafor improved type safety.packages/blocks/faq/src/frontend/Faq.server.tsx (1)
18-41: LGTM—consistent refactoring pattern.Functionally equivalent refactor. Optionally add type annotation to
data.packages/blocks/cta-section/src/frontend/CtaSection.server.tsx (1)
11-25: LGTM—consistent refactoring pattern.Refactor is correct. Optionally add type annotation to
data.packages/blocks/feature-section/src/frontend/FeatureSection.server.tsx (1)
13-27: LGTM—consistent refactoring pattern.Functionally correct. Optionally add type annotation to
data.packages/blocks/service-list/src/frontend/ServiceList.server.tsx (1)
13-35: LGTM—consistent refactoring pattern.Refactor maintains correctness. Optionally add type annotation to
data.packages/blocks/document-list/src/frontend/DocumentList.server.tsx (1)
13-36: LGTM—consistent refactoring pattern.Final file with the same control-flow refactor. Functionally correct. Optionally add type annotation to
data.packages/blocks/orders-summary/src/frontend/OrdersSummary.server.tsx (1)
38-47: LGTM! Consistent refactoring pattern.The control flow refactor matches the pattern applied across other server components in this PR, providing a single return path and improving code consistency. The error handling is preserved correctly with the early return in the catch block.
packages/blocks/article/src/frontend/Article.server.tsx (1)
24-33: Single return path improves control flow clarity, but the specific ESLint warning it addresses remains unclear.The refactoring from returning inside the try block to a single return path after try-catch is a valid code clarity improvement. However, after examining the ESLint configuration and standard rule sets (js.configs.recommended, tseslint.configs.recommended, and React/Next plugins), I cannot definitively identify which specific ESLint rule would flag the original pattern. The change itself is sound—moving the return statement outside the try block creates a clearer control flow—but verification of the exact linter warning driving this change is needed to ensure it satisfies the
--max-warnings=0enforcement.packages/blocks/media-section/src/frontend/MediaSection.server.tsx (1)
13-27: The code pattern is type-safe and compiles without errors in strict mode. TypeScript's control flow analysis correctly narrows the type after the catch block's early return, sodatais proven defined at the spread point. Withstrict: trueenabled (includingstrictNullChecksandnoImplicitAny) and an explicit return type (Promise<Model.MediaSectionBlock>) onsdk.blocks.getMediaSection, TypeScript successfully validates thatdatacannot be undefined when used on line 27. The pattern appears 34 times across the codebase with no type suppressions, confirming the refactor compiles successfully.Likely an incorrect or invalid review comment.
packages/blocks/ticket-summary/src/frontend/TicketSummary.server.tsx (1)
13-27: This TypeScript pattern compiles without errors.TypeScript 3.9+ fixed control flow analysis for try/catch blocks, and the project uses TypeScript 5.9.3, which properly handles this pattern. Since the catch block returns
null(exiting the function), the only way to reach line 27 is if the try block completes successfully, guaranteeingdatais assigned. TypeScript's control flow analysis recognizes this path, and the type inference correctly identifiesdataasModel.TicketSummaryBlock, which aligns with whatTicketSummaryDynamicexpects when spread alongside the additional props.
packages/blocks/feature-section-grid/src/frontend/FeatureSectionGrid.server.tsx
Outdated
Show resolved
Hide resolved
packages/blocks/notification-details/src/frontend/NotificationDetails.server.tsx
Outdated
Show resolved
Hide resolved
packages/blocks/orders-summary/src/frontend/OrdersSummary.server.tsx
Outdated
Show resolved
Hide resolved
packages/blocks/ticket-recent/src/frontend/TicketRecent.server.tsx
Outdated
Show resolved
Hide resolved
packages/blocks/ticket-recent/src/frontend/TicketRecent.server.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
packages/blocks/cta-section/src/frontend/CtaSection.server.tsx (1)
4-4: Consider reverting to the original early-return pattern.The refactoring introduces an uninitialized variable (
data) that relies on control-flow analysis to prove it's always assigned before use. While TypeScript should handle this correctly (the catch block always returns, so line 26 is only reachable ifdatawas assigned), the original pattern—returning directly inside the try block—is more idiomatic, maintainable, and safer:try { const data = await sdk.blocks.getCtaSection( { id }, { 'x-locale': locale }, accessToken, ); return <CtaSectionDynamic {...data} id={id} accessToken={accessToken} locale={locale} routing={routing} />; } catch (error) { console.error('Error fetching CtaSection block', error); return null; }Benefits of the original pattern:
- No uninitialized variables
- Clearer data flow (fetch → use immediately)
- Less fragile (if someone later modifies the catch block to not always return, the current code would use an uninitialized variable)
Since the PR objective is to resolve linter warnings, please verify that this specific refactoring actually addresses a concrete linter rule (e.g., a rule requiring single return points). If this change is resolving a warning, that warning might indicate a broader configuration issue worth revisiting.
Could you confirm which specific linter warning this refactoring resolves? If it's a rule enforcing single return points, consider whether that rule aligns with modern JavaScript/TypeScript best practices (where early returns are generally preferred for clarity and safety).
Also applies to: 12-12, 14-26
packages/blocks/ticket-list/src/frontend/TicketList.server.tsx (1)
4-4: LGTM! Type annotations correctly fix linter warnings.The explicit type import and annotation on
dataresolve implicit-any warnings, which aligns with the PR objective to enforce--max-warnings=0. The control flow is sound—TypeScript's definite assignment analysis correctly recognizes thatdatais always assigned before use or the function returns early on error.💡 Optional: Refactor to use const and simplify control flow
Consider moving the return statement into the try block for slightly cleaner code:
- let data: Model.TicketListBlock; try { - data = await sdk.blocks.getTicketList( + const data = await sdk.blocks.getTicketList( { id, preview: isDraftModeEnabled, }, { 'x-locale': locale }, accessToken, ); + return ( + <TicketListDynamic + {...data} + id={id} + accessToken={accessToken} + locale={locale} + routing={routing} + hasPriority={hasPriority} + /> + ); } catch (_error) { return null; } - - return ( - <TicketListDynamic - {...data} - id={id} - accessToken={accessToken} - locale={locale} - routing={routing} - hasPriority={hasPriority} - /> - );Benefits: uses
constinstead oflet, eliminates the need for explicit type annotation (type is inferred), and makes the control flow more explicit. However, the current implementation is perfectly valid.Also applies to: 19-19
packages/blocks/invoice-list/src/frontend/InvoiceList.server.tsx (1)
20-42: Refactor looks good; consider adding error logging for consistency.The unified return path successfully addresses the linter requirements. The pattern is safe because the catch block returns early, ensuring
datais always assigned before the final render.However, unlike HeroSection and DocumentList which log fetch errors, this catch block silently suppresses errors. Consider logging the error to aid debugging in production.
🔎 Optional: Add error logging for consistency
- } catch (_error) { + } catch (error) { + console.error('Error fetching InvoiceList block', error); return null; }packages/blocks/article-list/src/frontend/ArticleList.server.tsx (1)
21-44: Refactor achieves the goal; consider error logging.The single return path refactor is correct and safe. TypeScript ensures
datais assigned before use since the catch block exits early.Like InvoiceList, this file suppresses errors (via
_error) while HeroSection and DocumentList log them. Adding logging would improve observability across all blocks.🔎 Optional: Add error logging for consistency
- } catch (_error) { + } catch (error) { + console.error('Error fetching ArticleList block', error); return null; }packages/blocks/faq/src/frontend/Faq.server.tsx (1)
19-42: Refactor is correct; error logging would enhance observability.The unified return path meets the linter requirements and is type-safe. The catch block's early return ensures
datais always initialized before the final render.For consistency with HeroSection and DocumentList, consider logging the fetch error to aid production debugging.
🔎 Optional: Add error logging for consistency
- } catch (_error) { + } catch (error) { + console.error('Error fetching Faq block', error); return null; }turbo/generators/templates/block/frontend/server.hbs (1)
14-28: Consider initializingdatafor safer control flow.The refactored pattern declares
datawithout initialization (line 14), relying on TypeScript's control flow analysis to ensure it's assigned before use on line 28. While functionally correct (since the catch block always returns on line 25), this pattern is fragile:
- Implicit assumption: The code safety depends on the catch block always returning, which isn't enforced by the type system.
- Future modifications: If someone later modifies the catch block to conditionally return,
datacould be used uninitialized.- Readability: The original pattern (const declaration inside try) was clearer about data's scope and lifetime.
If this refactor addresses specific ESLint rules (e.g., no-return-in-try or prefer-single-return), consider these alternatives:
🔎 Safer alternative: Initialize with undefined
- let data: Model.{{ pascalCase name }}Block; + let data: Model.{{ pascalCase name }}Block | undefined; try { data = await sdk.blocks.get{{ pascalCase name }}( { id, }, { 'x-locale': locale }, accessToken, ); } catch (error) { console.error('Error fetching {{ pascalCase name }} block', error); return null; } + if (!data) { + return null; + } return <{{ pascalCase name }}Dynamic {...data} id={id} accessToken={accessToken} locale={locale} routing={routing} />;This makes the control flow explicit and doesn't rely on catch always returning.
Since this is a template file, the pattern will be replicated across all generated blocks. Please verify which specific linting rule this refactor addresses, as that context would help evaluate whether the trade-off is justified.
packages/blocks/feature-section/src/frontend/FeatureSection.server.tsx (1)
14-28: Clarify which ESLint rule requires this refactored pattern.The refactor moves from an early return inside the
tryblock to a hoistedletvariable with a single return aftertry/catch. While technically correct (TypeScript's control flow analysis recognizes thatdatais definitely assigned at line 28 since thecatchalways returns), this pattern is less intuitive than an early return.Can you confirm which specific ESLint rule (from the
--max-warnings=0enforcement) necessitates this pattern? If no linter rule requires it, the original pattern would be clearer:try { const data = await sdk.blocks.getFeatureSection(...); return <FeatureSectionDynamic {...data} ... />; } catch (error) { console.error('Error fetching FeatureSection block', error); return null; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (67)
.changeset/evil-results-nail.md(1 hunks)apps/docs/package.json(1 hunks)apps/frontend/package.json(2 hunks)packages/blocks/article-list/package.json(1 hunks)packages/blocks/article-list/src/frontend/ArticleList.server.tsx(2 hunks)packages/blocks/article-search/package.json(1 hunks)packages/blocks/article-search/src/frontend/ArticleSearch.server.tsx(2 hunks)packages/blocks/article/package.json(1 hunks)packages/blocks/article/src/frontend/Article.server.tsx(1 hunks)packages/blocks/bento-grid/src/frontend/BentoGrid.server.tsx(1 hunks)packages/blocks/category-list/package.json(1 hunks)packages/blocks/category-list/src/frontend/CategoryList.server.tsx(2 hunks)packages/blocks/category/package.json(1 hunks)packages/blocks/category/src/frontend/Category.server.tsx(2 hunks)packages/blocks/cta-section/src/frontend/CtaSection.server.tsx(1 hunks)packages/blocks/document-list/src/frontend/DocumentList.server.tsx(2 hunks)packages/blocks/faq/package.json(1 hunks)packages/blocks/faq/src/frontend/Faq.server.tsx(2 hunks)packages/blocks/feature-section-grid/src/frontend/FeatureSectionGrid.server.tsx(2 hunks)packages/blocks/feature-section/src/frontend/FeatureSection.server.tsx(2 hunks)packages/blocks/featured-service-list/package.json(1 hunks)packages/blocks/featured-service-list/src/frontend/FeaturedServiceList.server.tsx(2 hunks)packages/blocks/hero-section/src/frontend/HeroSection.server.tsx(2 hunks)packages/blocks/invoice-list/package.json(1 hunks)packages/blocks/invoice-list/src/frontend/InvoiceList.server.tsx(2 hunks)packages/blocks/media-section/src/frontend/MediaSection.server.tsx(2 hunks)packages/blocks/notification-details/package.json(1 hunks)packages/blocks/notification-details/src/frontend/NotificationDetails.server.tsx(3 hunks)packages/blocks/notification-list/package.json(1 hunks)packages/blocks/notification-list/src/frontend/NotificationList.server.tsx(2 hunks)packages/blocks/notification-summary/package.json(1 hunks)packages/blocks/notification-summary/src/frontend/NotificationSummary.server.tsx(2 hunks)packages/blocks/order-details/package.json(1 hunks)packages/blocks/order-details/src/frontend/OrderDetails.server.tsx(3 hunks)packages/blocks/order-list/package.json(1 hunks)packages/blocks/order-list/src/frontend/OrderList.server.tsx(1 hunks)packages/blocks/orders-summary/package.json(1 hunks)packages/blocks/orders-summary/src/frontend/OrdersSummary.server.tsx(3 hunks)packages/blocks/payments-history/package.json(1 hunks)packages/blocks/payments-history/src/frontend/PaymentsHistory.server.tsx(3 hunks)packages/blocks/payments-summary/package.json(1 hunks)packages/blocks/payments-summary/src/frontend/PaymentsSummary.server.tsx(3 hunks)packages/blocks/pricing-section/src/frontend/PricingSection.server.tsx(2 hunks)packages/blocks/product-list/src/frontend/ProductList.server.tsx(2 hunks)packages/blocks/quick-links/package.json(1 hunks)packages/blocks/quick-links/src/frontend/QuickLinks.server.tsx(2 hunks)packages/blocks/service-details/package.json(1 hunks)packages/blocks/service-details/src/frontend/ServiceDetails.server.tsx(3 hunks)packages/blocks/service-list/package.json(1 hunks)packages/blocks/service-list/src/frontend/ServiceList.server.tsx(2 hunks)packages/blocks/surveyjs-form/package.json(1 hunks)packages/blocks/surveyjs-form/src/frontend/SurveyJs.server.tsx(2 hunks)packages/blocks/ticket-details/package.json(1 hunks)packages/blocks/ticket-details/src/frontend/TicketDetails.server.tsx(3 hunks)packages/blocks/ticket-list/package.json(1 hunks)packages/blocks/ticket-list/src/frontend/TicketList.server.tsx(2 hunks)packages/blocks/ticket-recent/package.json(1 hunks)packages/blocks/ticket-recent/src/frontend/TicketRecent.server.tsx(2 hunks)packages/blocks/ticket-summary/package.json(1 hunks)packages/blocks/ticket-summary/src/frontend/TicketSummary.server.tsx(2 hunks)packages/blocks/user-account/package.json(1 hunks)packages/blocks/user-account/src/frontend/UserAccount.server.tsx(2 hunks)packages/configs/integrations/package.json(2 hunks)packages/modules/surveyjs/package.json(1 hunks)packages/utils/api-harmonization/package.json(1 hunks)packages/utils/frontend/package.json(1 hunks)turbo/generators/templates/block/frontend/server.hbs(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/blocks/ticket-summary/package.json
- packages/blocks/order-details/package.json
🚧 Files skipped from review as they are similar to previous changes (16)
- packages/blocks/ticket-summary/src/frontend/TicketSummary.server.tsx
- packages/blocks/product-list/src/frontend/ProductList.server.tsx
- packages/blocks/notification-summary/src/frontend/NotificationSummary.server.tsx
- apps/docs/package.json
- packages/blocks/featured-service-list/src/frontend/FeaturedServiceList.server.tsx
- packages/blocks/payments-summary/src/frontend/PaymentsSummary.server.tsx
- packages/blocks/feature-section-grid/src/frontend/FeatureSectionGrid.server.tsx
- packages/blocks/user-account/src/frontend/UserAccount.server.tsx
- packages/blocks/order-details/src/frontend/OrderDetails.server.tsx
- packages/blocks/media-section/src/frontend/MediaSection.server.tsx
- packages/blocks/payments-history/src/frontend/PaymentsHistory.server.tsx
- packages/blocks/category/src/frontend/Category.server.tsx
- packages/blocks/notification-details/src/frontend/NotificationDetails.server.tsx
- packages/blocks/order-list/src/frontend/OrderList.server.tsx
- packages/blocks/ticket-recent/src/frontend/TicketRecent.server.tsx
- packages/blocks/surveyjs-form/src/frontend/SurveyJs.server.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 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/package.jsonpackages/blocks/notification-list/src/frontend/NotificationList.server.tsx
📚 Learning: 2025-11-26T11:57:00.632Z
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 411
File: packages/framework/src/modules/cms/models/blocks/product-list.model.ts:29-46
Timestamp: 2025-11-26T11:57:00.632Z
Learning: In the framework layer (packages/framework/src/modules/cms/models/blocks/*.model.ts), block classes like ProductListBlock should NOT include explicit __typename discriminators. The __typename field is added at the API harmonization layer (packages/blocks/*/src/api-harmonization/*.model.ts) where it's needed for discriminated unions. The framework maintains normalized data models without these discriminators.
Applied to files:
packages/blocks/quick-links/src/frontend/QuickLinks.server.tsxpackages/blocks/document-list/src/frontend/DocumentList.server.tsxturbo/generators/templates/block/frontend/server.hbspackages/blocks/category-list/src/frontend/CategoryList.server.tsxpackages/blocks/ticket-list/src/frontend/TicketList.server.tsx
🧬 Code graph analysis (5)
packages/blocks/service-details/src/frontend/ServiceDetails.server.tsx (1)
packages/framework/src/modules/cms/models/blocks/service-details.model.ts (1)
ServiceDetailsBlock(6-18)
packages/blocks/service-list/src/frontend/ServiceList.server.tsx (1)
packages/framework/src/modules/cms/models/blocks/service-list.model.ts (1)
ServiceListBlock(6-23)
packages/blocks/ticket-details/src/frontend/TicketDetails.server.tsx (2)
packages/framework/src/modules/cms/models/blocks/ticket-details.model.ts (1)
TicketDetailsBlock(5-19)apps/frontend/src/i18n/routing.ts (1)
routing(8-18)
packages/blocks/cta-section/src/frontend/CtaSection.server.tsx (1)
apps/frontend/src/i18n/routing.ts (1)
routing(8-18)
packages/blocks/faq/src/frontend/Faq.server.tsx (1)
apps/frontend/src/i18n/routing.ts (1)
routing(8-18)
🔇 Additional comments (53)
packages/blocks/faq/package.json (1)
18-18: LGTM! Enforcing zero warnings improves code quality.The addition of
--max-warnings=0correctly enforces that ESLint warnings will fail the lint script, preventing code with warnings from being committed. This aligns with the PR objective to enforce stricter linting standards across all packages.packages/blocks/featured-service-list/package.json (1)
18-18: LGTM! Enforcing zero warnings improves code quality.Adding
--max-warnings=0to the lint script aligns with the PR objective and ensures that ESLint warnings are treated as errors. This is a best practice for maintaining code quality and preventing warning accumulation in the codebase.packages/blocks/service-list/package.json (1)
18-18: LGTM! Stricter linting improves code quality.The addition of
--max-warnings=0enforces zero-tolerance for ESLint warnings, which aligns with the PR's goal of consistent code quality across all packages.packages/blocks/ticket-recent/package.json (1)
18-18: LGTM! Enforcing zero warnings improves code quality.The addition of
--max-warnings=0ensures ESLint warnings are treated as errors, preventing technical debt from accumulating. This change aligns with the PR objective of enforcing stricter linting standards across all packages.packages/utils/frontend/package.json (1)
16-16: LGTM! Zero-warning enforcement aligns with PR objectives.The addition of
--max-warnings=0successfully implements the stricter linting policy described in the PR objectives. This ensures that any future ESLint warnings will cause the lint script to fail, helping maintain consistent code quality.packages/blocks/payments-summary/package.json (1)
18-18: LGTM! Enforcing zero warnings improves code quality.Adding
--max-warnings=0to the lint script is a best practice that prevents warning accumulation and ensures consistent code quality. The script properly sequences type checking, auto-fixing, and warning enforcement.packages/blocks/quick-links/package.json (1)
18-18: LGTM! Enforcing zero warnings improves code quality.The addition of
--max-warnings=0correctly enforces stricter linting standards and aligns with the PR objective. The command structure is optimal: type checking first, then auto-fixing, then failing on any remaining warnings.packages/blocks/invoice-list/package.json (1)
18-18: LGTM! Zero-warnings enforcement added correctly.The addition of
--max-warnings=0enforces strict linting and prevents warnings from accumulating, which aligns perfectly with the PR objectives. The flag placement is correct.packages/blocks/notification-summary/package.json (1)
18-18: LGTM! Zero-warnings policy correctly enforced.The addition of
--max-warnings=0aligns with the PR objective to enforce stricter linting standards across all packages.packages/blocks/payments-history/package.json (1)
18-18: LGTM! Stricter linting enforced.The addition of
--max-warnings=0correctly enforces a zero-warning policy, treating all ESLint warnings as errors. This aligns with the PR objective to tighten code quality standards across all packages.packages/modules/surveyjs/package.json (1)
18-18: LGTM! Enforcing zero warnings improves code quality.The addition of
--max-warnings=0aligns with the PR objective and ensures stricter linting enforcement. This prevents warnings from accumulating and maintains consistent code quality across the package.packages/blocks/quick-links/src/frontend/QuickLinks.server.tsx (1)
4-4: LGTM! Type annotation correctly fixes implicit any warning.The explicit type annotation on
dataresolves ESLint warnings while maintaining type safety. TypeScript's control flow analysis correctly recognizes thatdatais definitely assigned before use, thanks to the early return in the catch block at line 30.Also applies to: 19-19
packages/blocks/orders-summary/package.json (1)
18-18: LGTM! Enforces zero-warning policy.The addition of
--max-warnings=0aligns with the PR objective to treat ESLint warnings as errors across the repository, ensuring consistent code quality enforcement.packages/blocks/bento-grid/src/frontend/BentoGrid.server.tsx (1)
4-26: LGTM! Clean refactoring to unify return paths.The changes successfully consolidate the component's success path into a single return statement outside the try/catch block, which aligns with the PR objective of resolving ESLint warnings. TypeScript's control flow analysis correctly handles this pattern—since the catch block returns early, the compiler understands that
datais always assigned when execution reaches line 26.The explicit
Model.BentoGridBlocktype annotation improves type safety, and the error handling logic remains intact.packages/blocks/pricing-section/src/frontend/PricingSection.server.tsx (2)
4-4: LGTM: Import added for type annotation.The import is necessary to type the
datavariable on line 20.
20-43: LGTM: Refactor unifies the render path.The refactor correctly separates data fetching from rendering by:
- Declaring a typed variable to hold the fetched data
- Handling errors with an early return
- Rendering the component in a single path after the try/catch
This pattern addresses linter requirements for consistent return paths while maintaining correct control flow—
datais guaranteed to be assigned when the render path is reached because any error triggers an early return in the catch block.packages/blocks/service-list/src/frontend/ServiceList.server.tsx (3)
4-4: LGTM: Type import added correctly.The
Modeltype import is necessary for typing thedatavariable and follows proper import conventions.
14-25: Verify TypeScript accepts the uninitialized variable pattern.The variable
datais declared without initialization and assigned only within the try block. While the control flow ensuresdatais always assigned before use (since the catch block returns), TypeScript's definite assignment analysis may flag this as an error depending on strictness settings.Please verify that this pattern compiles without errors in your TypeScript configuration. If issues arise, consider one of these alternatives:
Alternative approaches if TypeScript complains
Option 1: Initialize with undefined and add type guard
-let data: Model.ServiceListBlock; +let data: Model.ServiceListBlock | undefined; try { data = await sdk.blocks.getServiceList( { id, }, { 'x-locale': locale }, accessToken, ); } catch (_error) { return null; } + +if (!data) { + return null; +} return ( <ServiceListDynamic {...data} id={id} accessToken={accessToken} locale={locale} routing={routing} hasPriority={hasPriority} /> );Option 2: Use const with immediate return (more explicit)
-let data: Model.ServiceListBlock; +let data: Model.ServiceListBlock | null = null; try { data = await sdk.blocks.getServiceList( { id, }, { 'x-locale': locale }, accessToken, ); } catch (_error) { return null; } +if (data === null) { + return null; +} + return ( <ServiceListDynamic {...data} id={id} accessToken={accessToken} locale={locale} routing={routing} hasPriority={hasPriority} /> );
27-36: LGTM: Single return path improves consistency.The refactoring successfully unifies the component's return path by moving the render logic outside the try block. This approach:
- Addresses the linter requirements mentioned in the PR objectives
- Improves code consistency and readability
- Correctly spreads the fetched data and passes through the required props
packages/blocks/hero-section/src/frontend/HeroSection.server.tsx (1)
14-37: LGTM! Refactor unifies return path successfully.The introduction of the hoisted
datavariable and single return path after try/catch aligns with the PR's goal of addressing linter warnings. TypeScript's control flow analysis ensuresdatais always assigned before use (the catch block returns early), making this pattern safe.packages/blocks/document-list/src/frontend/DocumentList.server.tsx (1)
14-37: LGTM! Pattern is safe and consistent.The refactor successfully consolidates the return path while maintaining proper error handling with logging. TypeScript's control flow ensures the final return is only reachable when
datahas been assigned.packages/blocks/article/src/frontend/Article.server.tsx (4)
4-4: LGTM! Import supports the explicit type annotation.The import of
Modelis necessary for the type annotation on line 12 and addresses the previous review feedback.
12-12: Explicit type annotation successfully addresses previous feedback.The type annotation
Model.ArticleBlockresolves the previous concern about implicitanytype inference. TypeScript's control flow analysis correctly recognizes thatdatais definitely assigned before use at line 27 (either assigned in the try block or early return in catch).
21-23: Verify silent error handling is intentional.The catch block returns
nullwithout logging or reporting errors. While this may be intentional for graceful degradation (e.g., hiding unavailable articles), it could also mask important issues like network failures, authentication problems, or API errors that should be surfaced.Consider whether errors should be logged for debugging purposes, especially in server components where error visibility is limited.
25-34: LGTM! Clean single return path.The refactored rendering with a single return statement improves code clarity and aligns with ESLint best practices. The spread of
datafollowed by explicit props is correct and maintains the component's contract.packages/blocks/ticket-details/package.json (1)
18-18: LGTM! Enforcing zero warnings improves code quality.This change directly implements the PR objective to enforce
--max-warnings=0across all packages, ensuring that ESLint warnings are treated as errors and preventing warning accumulation.packages/blocks/ticket-details/src/frontend/TicketDetails.server.tsx (2)
21-35: LGTM! Control flow ensures type safety.The refactoring moves the return statement outside the try/catch block while maintaining type safety. TypeScript's control flow analysis recognizes that
datais always assigned before use because the catch block returns early on error.This pattern change addresses linter warnings (per PR objective) and unifies the return paths across server components.
37-47: LGTM! Clean return statement.The return statement correctly renders the dynamic component with all necessary props. Spreading
datafirst followed by explicit props is a good pattern that allows explicit props to override any conflicting properties from the data object.packages/blocks/surveyjs-form/package.json (1)
18-18: LGTM! Enforcing zero-warnings policy.The addition of
--max-warnings=0correctly implements the PR objective of treating all ESLint warnings as errors across packages. This is a good practice for maintaining consistent code quality.turbo/generators/templates/block/frontend/server.hbs (1)
4-4: Appropriate import addition for type safety.The Model import from the api-harmonization layer is correctly added to support the type annotation on line 14.
packages/blocks/notification-list/src/frontend/NotificationList.server.tsx (1)
4-4: LGTM! Solid refactoring that unifies the return path.The refactoring successfully addresses the PR objectives by:
- Adding type safety via
Model.NotificationListBlock- Moving to a unified return pattern (single return after try/catch instead of early return in try)
- Improving code maintainability and consistency across the codebase
TypeScript's control flow analysis correctly handles the uninitialized
datavariable since the catch block returnsnull, ensuringdatais always assigned when execution reaches line 33.The prop spreading pattern (
{...data}followed by explicit props) is intentional—explicit props take precedence over spread properties, which ensures the component always receives the correctid,accessToken,locale,routing, andhasPriorityvalues.Also applies to: 20-42
packages/blocks/orders-summary/src/frontend/OrdersSummary.server.tsx (2)
5-5: Type safety concern from previous review has been addressed.The explicit type annotation
Model.OrdersSummaryBlockfor thedatavariable successfully addresses the type safety concern raised in the previous review. Using the Model import is a cleaner, more maintainable approach than usingAwaited<ReturnType<...>>, as it's more explicit and decouples the type from the SDK implementation details.Also applies to: 22-22
38-48: LGTM: Cleaner control flow with return outside try/catch.Moving the return statement outside the try/catch block improves readability and maintains a clear separation of concerns: data fetching, error handling, and rendering. TypeScript's control flow analysis correctly recognizes that
datais definitely assigned when reaching this return statement, since the catch block returns early on errors.packages/blocks/article-search/src/frontend/ArticleSearch.server.tsx (2)
4-4: LGTM! Clean refactoring to unify return paths.The refactoring improves code clarity by separating data fetching from rendering. The pattern of hoisting
data, assigning it within the try block, and rendering after the try/catch is consistent and maintainable. TypeScript's control flow analysis correctly handles the definite assignment—if the assignment succeeds,datais defined; if it fails, the function returns early on line 30.Also applies to: 20-28, 33-42
29-31: Verify error handling and logging approach.The catch block returns
nullsilently without explicit logging. While this may be intentional for optional blocks (allowing the page to continue rendering without this component), confirm that:
- Silent failure is the desired UX for missing article search data
- Error logging is handled by the SDK or a global error boundary if needed for production debugging
packages/blocks/category-list/src/frontend/CategoryList.server.tsx (2)
4-4: LGTM! Proper import for type safety.The import of
Modelfrom the API harmonization layer is appropriate and enables explicit typing of the block data.
21-21: Excellent type safety improvement!The explicit type annotation correctly matches the return type of
sdk.blocks.getCategoryListand improves code clarity. The control flow ensures TypeScript recognizes thatdatais definitely assigned before use on line 37, with the catch block's early return preventing undefined variable access.packages/blocks/service-details/src/frontend/ServiceDetails.server.tsx (3)
4-4: LGTM - Variable hoisting pattern for unified return.The import and hoisted variable declaration correctly set up the pattern where data is fetched in a try block and rendered after. TypeScript's control flow analysis should recognize that the catch block returns, ensuring
datais assigned when reaching the final return.Also applies to: 21-21
37-47: LGTM - Clean unified return pattern.The final return statement correctly renders the dynamic component with both the fetched data (spread) and the additional props. This unification of the return path after data fetching improves code clarity and aligns with the PR's objective to standardize block components.
23-35: Code pattern is correct.The explicit type annotation on line 21 (
let data: Model.ServiceDetailsBlock;) combined with the try-catch structure allows TypeScript's control flow analysis to correctly determine thatdatais definitely assigned before use. The catch block's early return ensuresdatais never accessed in the error path. This compiles without errors under strict mode.packages/blocks/category-list/package.json (1)
18-18: LGTM! Stricter linting enforced.The addition of
--max-warnings=0ensures ESLint warnings are treated as errors, preventing warning accumulation and maintaining code quality. This aligns with the PR's objective to enforce strict linting across all packages.packages/blocks/article-search/package.json (1)
18-18: LGTM! Consistent with repo-wide linting policy.packages/blocks/order-list/package.json (1)
18-18: LGTM! Consistent with repo-wide linting policy.packages/blocks/article-list/package.json (1)
18-18: LGTM! Consistent with repo-wide linting policy.packages/blocks/service-details/package.json (1)
18-18: LGTM! Consistent with repo-wide linting policy.packages/blocks/notification-details/package.json (1)
18-18: LGTM! Consistent with repo-wide linting policy.packages/blocks/article/package.json (1)
18-18: LGTM! Consistent with repo-wide linting policy.packages/utils/api-harmonization/package.json (1)
16-16: LGTM! Consistent with repo-wide linting policy.packages/blocks/category/package.json (1)
18-18: LGTM! Enforcing zero warnings improves code quality.The addition of
--max-warnings=0to the lint script aligns with the PR objective and is a best practice for maintaining strict code quality standards. The change is syntactically correct and the flag is a valid ESLint option.packages/blocks/ticket-list/package.json (1)
18-18: Lint script execution timed out—investigate before merging.The lint script execution exceeded the timeout threshold (120+ seconds), which suggests either the script is hanging or takes an unusually long time to complete. This needs investigation before the
--max-warnings=0flag change can be verified.Please verify that:
- The lint script completes successfully in a reasonable timeframe
- No ESLint warnings exist that would cause the zero-warnings check to fail
packages/configs/integrations/package.json (1)
15-15: LGTM: Consistent enforcement of zero-warning policy.The lint script update matches the pattern applied across other packages in this PR, maintaining consistency in lint enforcement.
packages/blocks/user-account/package.json (1)
18-18: LGTM: Consistent with PR-wide lint enforcement.The lint script update correctly applies the
--max-warnings=0flag, aligning with the zero-warning policy being enforced across all packages.packages/blocks/notification-list/package.json (1)
18-18: LGTM: Completes consistent lint enforcement across packages.The lint script update correctly includes
--max-warnings=0, maintaining consistency with the other package updates in this PR.
packages/blocks/feature-section/src/frontend/FeatureSection.server.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/blocks/media-section/src/frontend/MediaSection.types.ts (1)
15-17: Consider cleaning up the no-opOmit.
Omit<MediaSectionProps, ''>with an empty string doesn't omit any properties—it's effectively equivalent toMediaSectionProps. While this isn't a changed line, you might consider simplifying it in a follow-up if the intent was to extend without omitting:🔎 Suggested simplification
-export type MediaSectionRendererProps = Omit<MediaSectionProps, ''> & { +export type MediaSectionRendererProps = MediaSectionProps & { slug: string[]; };packages/blocks/featured-service-list/src/frontend/FeaturedServiceList.server.tsx (1)
20-42: Refactoring to single return path looks good.The control flow is correct: the catch block returns null, ensuring
datais always assigned when the final return executes. This pattern provides consistency across block components.Optional observation: The original pattern (declaring
const datainside the try block and returning immediately) was more idiomatic and avoided mutable state. However, given this is part of a deliberate consistency effort across multiple blocks, the trade-off is acceptable.packages/blocks/surveyjs-form/src/frontend/SurveyJs.server.tsx (1)
4-4: Refactor to single return path works correctly.The changes successfully move the render logic outside the try/catch block using a mutable
datavariable. The control flow is correct: errors returnnullearly, and the success path renders the component with the fetched data.The previous pattern (declaring
const datainside the try block with an immediate return) was slightly more explicit and avoided mutable variables, but the current approach is valid and aligns with the PR's goal of standardizing the pattern across similar server components.Alternative: inline const pattern (optional)
If the linter rules permit, consider keeping the previous pattern for slightly better immutability and explicitness:
try { const data = await sdk.blocks.getSurveyjsBlock( { id }, { 'x-locale': locale }, accessToken, ); return ( <SurveyJsDynamic {...data} id={id} accessToken={accessToken} locale={locale} routing={routing} hasPriority={hasPriority} /> ); } catch (_error) { return null; }This avoids
let, keepsdatascoped to where it's used, and doesn't rely on control flow analysis to prove initialization.Also applies to: 18-40
packages/blocks/ticket-summary/src/frontend/TicketSummary.server.tsx (1)
14-28: Consider reverting to the original pattern for clarity and type safety.The refactoring from declaring
const datainside the try block with an early return to pre-declaringlet dataand returning after the catch works correctly, but introduces subtle maintainability risks:
- Less idiomatic: Modern TypeScript favors
constand early returns over mutableletvariables- Fragile control flow: Future modifications to the catch block (removing
return null;or adding logic between catch and return) could introduce bugs wheredatais used uninitialized- Unclear motivation: The original pattern is standard for async error handling in React Server Components
Since the PR aims to fix linter warnings, could you clarify which ESLint rule this pattern addresses? Without a compelling linting or functional reason, the original pattern may be preferable.
🔎 Alternative: original pattern
- let data: Model.TicketSummaryBlock; try { - data = await sdk.blocks.getTicketSummary( + const data = await sdk.blocks.getTicketSummary( { id, }, { 'x-locale': locale }, accessToken, ); + return <TicketSummaryDynamic {...data} id={id} accessToken={accessToken} locale={locale} routing={routing} />; } catch (error) { console.error('Error fetching TicketSummary block', error); return null; } - - return <TicketSummaryDynamic {...data} id={id} accessToken={accessToken} locale={locale} routing={routing} />;packages/blocks/ticket-details/src/frontend/TicketDetails.server.tsx (1)
21-47: Consider restructuring for improved clarity—alternative patterns are more idiomatic.The current uninitialized variable pattern is less explicit than alternatives and requires readers to trace control flow to understand the guarantee that
datais assigned before use.Option 1 (Recommended): Return inside try block
try { const data = await sdk.blocks.getTicketDetails(...); return <TicketDetailsDynamic {...data} ... />; } catch (_error) { return null; }Option 2: Initialize with explicit type union
let data: Model.TicketDetailsBlock | undefined; try { data = await sdk.blocks.getTicketDetails(...); } catch (_error) { return null; } return <TicketDetailsDynamic {...data} ... />;Option 1 is more idiomatic for React Server Components and eliminates the need to reason about control flow guarantees.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (72)
packages/blocks/article-list/src/frontend/ArticleList.server.tsx(2 hunks)packages/blocks/article-list/src/frontend/ArticleList.types.ts(1 hunks)packages/blocks/article-search/src/frontend/ArticleSearch.client.tsx(1 hunks)packages/blocks/article-search/src/frontend/ArticleSearch.server.tsx(2 hunks)packages/blocks/article-search/src/frontend/ArticleSearch.types.ts(1 hunks)packages/blocks/article/src/frontend/Article.server.tsx(1 hunks)packages/blocks/article/src/frontend/Article.types.ts(1 hunks)packages/blocks/bento-grid/src/frontend/BentoGrid.server.tsx(1 hunks)packages/blocks/bento-grid/src/frontend/BentoGrid.types.ts(1 hunks)packages/blocks/category-list/src/frontend/CategoryList.server.tsx(2 hunks)packages/blocks/category-list/src/frontend/CategoryList.types.ts(1 hunks)packages/blocks/category/src/frontend/Category.server.tsx(2 hunks)packages/blocks/category/src/frontend/Category.types.ts(1 hunks)packages/blocks/cta-section/src/frontend/CtaSection.server.tsx(1 hunks)packages/blocks/cta-section/src/frontend/CtaSection.types.ts(1 hunks)packages/blocks/document-list/src/frontend/DocumentList.server.tsx(2 hunks)packages/blocks/document-list/src/frontend/DocumentList.types.ts(1 hunks)packages/blocks/faq/src/frontend/Faq.server.tsx(2 hunks)packages/blocks/faq/src/frontend/Faq.types.ts(1 hunks)packages/blocks/feature-section-grid/src/frontend/FeatureSectionGrid.server.tsx(2 hunks)packages/blocks/feature-section-grid/src/frontend/FeatureSectionGrid.types.ts(1 hunks)packages/blocks/feature-section/src/frontend/FeatureSection.server.tsx(2 hunks)packages/blocks/feature-section/src/frontend/FeatureSection.types.ts(1 hunks)packages/blocks/featured-service-list/src/frontend/FeaturedServiceList.server.tsx(2 hunks)packages/blocks/featured-service-list/src/frontend/FeaturedServiceList.types.ts(1 hunks)packages/blocks/hero-section/src/frontend/HeroSection.server.tsx(2 hunks)packages/blocks/hero-section/src/frontend/HeroSection.types.ts(1 hunks)packages/blocks/invoice-list/src/frontend/InvoiceList.server.tsx(2 hunks)packages/blocks/invoice-list/src/frontend/InvoiceList.types.ts(1 hunks)packages/blocks/media-section/src/frontend/MediaSection.server.tsx(2 hunks)packages/blocks/media-section/src/frontend/MediaSection.types.ts(1 hunks)packages/blocks/notification-details/src/frontend/NotificationDetails.server.tsx(3 hunks)packages/blocks/notification-details/src/frontend/NotificationDetails.types.ts(1 hunks)packages/blocks/notification-list/src/frontend/NotificationList.server.tsx(2 hunks)packages/blocks/notification-list/src/frontend/NotificationList.types.ts(1 hunks)packages/blocks/notification-summary/src/frontend/NotificationSummary.server.tsx(2 hunks)packages/blocks/notification-summary/src/frontend/NotificationSummary.types.ts(1 hunks)packages/blocks/order-details/src/frontend/OrderDetails.server.tsx(3 hunks)packages/blocks/order-details/src/frontend/OrderDetails.types.ts(1 hunks)packages/blocks/order-list/src/frontend/OrderList.server.tsx(1 hunks)packages/blocks/order-list/src/frontend/OrderList.types.ts(1 hunks)packages/blocks/orders-summary/src/frontend/OrdersSummary.server.tsx(3 hunks)packages/blocks/orders-summary/src/frontend/OrdersSummary.types.ts(1 hunks)packages/blocks/payments-history/src/frontend/PaymentsHistory.server.tsx(3 hunks)packages/blocks/payments-history/src/frontend/PaymentsHistory.types.ts(1 hunks)packages/blocks/payments-summary/src/frontend/PaymentsSummary.server.tsx(3 hunks)packages/blocks/payments-summary/src/frontend/PaymentsSummary.types.ts(1 hunks)packages/blocks/pricing-section/src/frontend/PricingSection.server.tsx(2 hunks)packages/blocks/pricing-section/src/frontend/PricingSection.types.ts(1 hunks)packages/blocks/product-list/src/frontend/ProductList.client.tsx(1 hunks)packages/blocks/product-list/src/frontend/ProductList.server.tsx(2 hunks)packages/blocks/product-list/src/frontend/ProductList.types.ts(1 hunks)packages/blocks/quick-links/src/frontend/QuickLinks.server.tsx(2 hunks)packages/blocks/quick-links/src/frontend/QuickLinks.types.ts(1 hunks)packages/blocks/service-details/src/frontend/ServiceDetails.server.tsx(3 hunks)packages/blocks/service-details/src/frontend/ServiceDetails.types.ts(1 hunks)packages/blocks/service-list/src/frontend/ServiceList.server.tsx(2 hunks)packages/blocks/service-list/src/frontend/ServiceList.types.ts(1 hunks)packages/blocks/surveyjs-form/src/frontend/SurveyJs.server.tsx(2 hunks)packages/blocks/surveyjs-form/src/frontend/SurveyJs.types.ts(1 hunks)packages/blocks/ticket-details/src/frontend/TicketDetails.server.tsx(3 hunks)packages/blocks/ticket-details/src/frontend/TicketDetails.types.ts(1 hunks)packages/blocks/ticket-list/src/frontend/TicketList.server.tsx(2 hunks)packages/blocks/ticket-list/src/frontend/TicketList.types.ts(1 hunks)packages/blocks/ticket-recent/src/frontend/TicketRecent.server.tsx(2 hunks)packages/blocks/ticket-recent/src/frontend/TicketRecent.types.ts(1 hunks)packages/blocks/ticket-summary/src/frontend/TicketSummary.server.tsx(2 hunks)packages/blocks/ticket-summary/src/frontend/TicketSummary.types.ts(1 hunks)packages/blocks/user-account/src/frontend/UserAccount.server.tsx(2 hunks)packages/blocks/user-account/src/frontend/UserAccount.types.ts(1 hunks)turbo/generators/templates/block/frontend/server.hbs(1 hunks)turbo/generators/templates/block/frontend/types.hbs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/blocks/article-search/src/frontend/ArticleSearch.client.tsx
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/blocks/faq/src/frontend/Faq.server.tsx
- packages/blocks/article-list/src/frontend/ArticleList.server.tsx
- packages/blocks/ticket-list/src/frontend/TicketList.server.tsx
- packages/blocks/notification-list/src/frontend/NotificationList.server.tsx
- packages/blocks/order-details/src/frontend/OrderDetails.server.tsx
- packages/blocks/feature-section-grid/src/frontend/FeatureSectionGrid.server.tsx
- packages/blocks/article-search/src/frontend/ArticleSearch.server.tsx
- packages/blocks/feature-section/src/frontend/FeatureSection.server.tsx
- packages/blocks/bento-grid/src/frontend/BentoGrid.server.tsx
- packages/blocks/user-account/src/frontend/UserAccount.server.tsx
- packages/blocks/service-list/src/frontend/ServiceList.server.tsx
- packages/blocks/hero-section/src/frontend/HeroSection.server.tsx
- packages/blocks/payments-summary/src/frontend/PaymentsSummary.server.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-26T11:57:00.632Z
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 411
File: packages/framework/src/modules/cms/models/blocks/product-list.model.ts:29-46
Timestamp: 2025-11-26T11:57:00.632Z
Learning: In the framework layer (packages/framework/src/modules/cms/models/blocks/*.model.ts), block classes like ProductListBlock should NOT include explicit __typename discriminators. The __typename field is added at the API harmonization layer (packages/blocks/*/src/api-harmonization/*.model.ts) where it's needed for discriminated unions. The framework maintains normalized data models without these discriminators.
Applied to files:
packages/blocks/orders-summary/src/frontend/OrdersSummary.types.tspackages/blocks/ticket-details/src/frontend/TicketDetails.types.tspackages/blocks/ticket-list/src/frontend/TicketList.types.tspackages/blocks/service-list/src/frontend/ServiceList.types.tspackages/blocks/ticket-recent/src/frontend/TicketRecent.server.tsxpackages/blocks/document-list/src/frontend/DocumentList.server.tsxpackages/blocks/product-list/src/frontend/ProductList.server.tsxpackages/blocks/article-search/src/frontend/ArticleSearch.types.tspackages/blocks/category/src/frontend/Category.server.tsxpackages/blocks/order-details/src/frontend/OrderDetails.types.tspackages/blocks/order-list/src/frontend/OrderList.types.tspackages/blocks/category-list/src/frontend/CategoryList.server.tsxpackages/blocks/featured-service-list/src/frontend/FeaturedServiceList.server.tsxpackages/blocks/product-list/src/frontend/ProductList.client.tsxpackages/blocks/invoice-list/src/frontend/InvoiceList.types.tspackages/blocks/document-list/src/frontend/DocumentList.types.tspackages/blocks/order-list/src/frontend/OrderList.server.tsxpackages/blocks/quick-links/src/frontend/QuickLinks.server.tsxpackages/blocks/notification-list/src/frontend/NotificationList.types.tspackages/blocks/service-details/src/frontend/ServiceDetails.types.tspackages/blocks/quick-links/src/frontend/QuickLinks.types.tspackages/blocks/pricing-section/src/frontend/PricingSection.types.tsturbo/generators/templates/block/frontend/server.hbspackages/blocks/category-list/src/frontend/CategoryList.types.tspackages/blocks/feature-section/src/frontend/FeatureSection.types.tspackages/blocks/user-account/src/frontend/UserAccount.types.tspackages/blocks/hero-section/src/frontend/HeroSection.types.tspackages/blocks/article/src/frontend/Article.types.tspackages/blocks/feature-section-grid/src/frontend/FeatureSectionGrid.types.tspackages/blocks/featured-service-list/src/frontend/FeaturedServiceList.types.tspackages/blocks/category/src/frontend/Category.types.tsturbo/generators/templates/block/frontend/types.hbspackages/blocks/product-list/src/frontend/ProductList.types.tspackages/blocks/faq/src/frontend/Faq.types.tspackages/blocks/article-list/src/frontend/ArticleList.types.tspackages/blocks/notification-details/src/frontend/NotificationDetails.server.tsx
📚 Learning: 2025-11-14T14:24:34.907Z
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 357
File: packages/blocks/ticket-summary/src/frontend/TicketSummary.types.ts:14-16
Timestamp: 2025-11-14T14:24:34.907Z
Learning: In the openselfservice repository, the pattern `Omit<PropsType, ''>` in TypeScript type definitions is intentionally used as a template for future prop omissions, even though it currently does nothing. This convention is preferred for consistency across blocks, making it easier to add omissions when needed later.
Applied to files:
packages/blocks/service-details/src/frontend/ServiceDetails.types.ts
📚 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/frontend/NotificationSummary.server.tsxpackages/blocks/notification-details/src/frontend/NotificationDetails.server.tsx
🧬 Code graph analysis (7)
packages/blocks/cta-section/src/frontend/CtaSection.server.tsx (2)
packages/blocks/cta-section/src/frontend/CtaSection.types.ts (1)
CtaSectionProps(5-11)apps/frontend/src/i18n/routing.ts (1)
routing(8-18)
packages/blocks/product-list/src/frontend/ProductList.server.tsx (2)
packages/blocks/product-list/src/api-harmonization/product-list.model.ts (1)
ProductListBlock(7-29)packages/blocks/product-list/src/sdk/index.ts (1)
sdk(24-28)
packages/blocks/category/src/frontend/Category.server.tsx (2)
packages/framework/src/modules/cms/models/blocks/category.model.ts (1)
CategoryBlock(5-19)apps/frontend/src/blocks/renderBlocks.tsx (1)
renderBlocks(56-89)
packages/blocks/article/src/frontend/Article.server.tsx (2)
packages/blocks/article/src/frontend/Article.types.ts (1)
ArticleProps(5-12)apps/frontend/src/i18n/routing.ts (1)
routing(8-18)
packages/blocks/order-list/src/frontend/OrderList.server.tsx (2)
packages/blocks/order-list/src/frontend/OrderList.types.ts (1)
OrderListProps(5-12)apps/frontend/src/i18n/routing.ts (1)
routing(8-18)
packages/blocks/payments-history/src/frontend/PaymentsHistory.server.tsx (1)
packages/framework/src/modules/cms/models/blocks/payments-history.model.ts (1)
PaymentsHistoryBlock(3-10)
packages/blocks/notification-details/src/frontend/NotificationDetails.server.tsx (1)
packages/framework/src/modules/cms/models/blocks/notification-details.model.ts (1)
NotificationDetailsBlock(5-14)
🔇 Additional comments (60)
packages/blocks/order-details/src/frontend/OrderDetails.types.ts (1)
3-3: LGTM! Type-only import reduces bundle size.Converting to a type-only import is correct since
Modelis only used in type positions (line 14). This follows TypeScript best practices, reduces the runtime bundle size, and likely addresses the ESLint warning this PR aims to fix.packages/blocks/surveyjs-form/src/frontend/SurveyJs.types.ts (1)
3-3: LGTM! Proper type-only import optimization.Converting to
import typeis appropriate sinceModelis only used in type positions (line 13:Model.SurveyjsBlock). This improves bundle optimization and aligns with TypeScript best practices and the@typescript-eslint/consistent-type-importsrule.packages/blocks/notification-details/src/frontend/NotificationDetails.types.ts (1)
3-3: LGTM! Type-only import correctly applied.Converting to
import typeis appropriate here sinceModelis used exclusively in type positions (line 14). This reduces the runtime bundle size and aligns with the PR's objective to eliminate linter warnings.packages/blocks/payments-summary/src/frontend/PaymentsSummary.types.ts (1)
3-3: LGTM! Type-only import correctly applied.Converting to
import typeis appropriate sinceModelis only used in type positions (line 13). This follows TypeScript best practices, eliminates unnecessary runtime imports, and aligns with the PR's goal of addressing linter warnings.packages/blocks/user-account/src/frontend/UserAccount.types.ts (1)
3-3: LGTM! Type-only import correctly optimizes the code.Changing to
import typeis appropriate sinceModelis only used in type positions (line 15). This eliminates the unnecessary runtime import, reduces bundle size, and addresses the ESLint warning that the PR aims to fix.packages/blocks/service-list/src/frontend/ServiceList.types.ts (1)
3-3: LGTM! Type-only import correctly applied.Converting to
import typeis appropriate sinceModelis only used in type positions (line 13's type alias). This change improves tree-shaking, makes the intent explicit, and aligns with TypeScript best practices for addressing linter warnings.packages/blocks/ticket-list/src/frontend/TicketList.types.ts (1)
3-3: LGTM: Type-only import correctly applied.Converting
Modelto a type-only import is correct since it's only used in type positions (line 15). This follows TypeScript best practices, improves tree-shaking, and aligns with the PR's goal of eliminating linter warnings.packages/blocks/bento-grid/src/frontend/BentoGrid.types.ts (1)
3-3: LGTM! Type-only import correctly applied.Converting to
import typeis the appropriate fix sinceModelis only used in type positions (line 13). This eliminates an unnecessary runtime import and resolves the ESLint warning, aligning with the PR's objective to enforce--max-warnings=0.turbo/generators/templates/block/frontend/types.hbs (1)
3-3: LGTM! Type-only import is appropriate here.Converting to
import typeis correct sinceModelis used only in a type position (line 12). This template change will ensure all future generated blocks follow TypeScript best practices for type-only imports and satisfy the@typescript-eslint/consistent-type-importslinter rule.packages/blocks/order-list/src/frontend/OrderList.types.ts (1)
3-3: LGTM! Correct use of type-only import.Converting to
import typeis appropriate sinceModelis only used in type positions (line 18). This improves tree-shaking, clarifies intent, and resolves the linter warning.packages/blocks/media-section/src/frontend/MediaSection.server.tsx (2)
4-4: Good use of type-only import.Using
import typeforModelensures the import is erased at runtime since it's only used for type annotations. This aligns with the linter warning fixes and TypeScript best practices.
14-28: LGTM! Clean refactor to unified post-try/catch render pattern.The control flow is correct: TypeScript's flow analysis ensures
datais definitely assigned when line 28 is reached, since the catch block returns early. This pattern aligns with the consistent approach adopted across other blocks in the repository.packages/blocks/media-section/src/frontend/MediaSection.types.ts (1)
3-3: Good use of type-only import.Since
Modelis only used in type positions (Model.MediaSectionBlock), theimport typesyntax is correct and ensures the import is stripped at compile time.packages/blocks/article-search/src/frontend/ArticleSearch.types.ts (1)
3-3: LGTM! Type-only import correctly applied.Converting to
import typeis appropriate sinceModelis used exclusively in type positions (line 13). This follows TypeScript best practices, improves tree-shaking, and aligns with the PR's objective to eliminate ESLint warnings.packages/blocks/notification-list/src/frontend/NotificationList.types.ts (1)
3-3: LGTM! Type-only import is correctly applied.The change to
import typeis appropriate sinceModelis only used in type positions (line 14). This follows TypeScript best practices and aligns with the PR's objective to eliminate linter warnings.packages/blocks/quick-links/src/frontend/QuickLinks.types.ts (1)
3-3: LGTM! Type-only import is correct.Converting to
import typeis appropriate sinceModelis only used in type positions (line 14). This follows TypeScript best practices and likely resolves an ESLint warning from@typescript-eslint/consistent-type-imports.packages/blocks/feature-section/src/frontend/FeatureSection.types.ts (1)
3-3: LGTM! Type-only import correctly applied.Converting to
import typeis the right approach sinceModelis only referenced in type positions. This improves clarity and can reduce bundle size.packages/blocks/quick-links/src/frontend/QuickLinks.server.tsx (1)
4-4: LGTM! Type-only import and explicit type annotation correctly address linter warnings.The changes improve type safety and reduce runtime dependencies:
- Line 4: Using
import typeclarifies thatModelis only needed at compile time, reducing bundle size.- Line 19: Explicit type annotation on
dataprevents implicitanyand ensures type safety.The control flow remains safe: if the fetch fails, the function returns
nullbeforedatais accessed, so TypeScript's control flow analysis correctly validates thatdatais always assigned when used.Also applies to: 19-19
packages/blocks/notification-summary/src/frontend/NotificationSummary.types.ts (1)
3-3: LGTM!Correct use of
import typesinceModelis only referenced in type positions (line 12). This change satisfies the@typescript-eslint/consistent-type-importsrule and helps bundlers with dead-code elimination.packages/blocks/notification-summary/src/frontend/NotificationSummary.server.tsx (2)
4-4: LGTM!Type-only import is appropriate since
Modelis solely used for the type annotation on line 14.
14-28: LGTM!The refactored pattern correctly leverages TypeScript's control flow analysis—since the
catchblock returns early,datais guaranteed to be assigned when line 28 executes. This single-return approach improves readability and aligns with the consistent pattern applied across other block components in this PR.packages/blocks/notification-details/src/frontend/NotificationDetails.server.tsx (1)
4-4: LGTM! Type safety improvement addresses previous review feedback.The explicit type annotation
Model.NotificationDetailsBlockfor thedatavariable successfully addresses the past review comment and improves type safety. The refactored single-return pattern is cleaner and more maintainable than the previous immediate-return approach.The control flow is safe: TypeScript's control flow analysis correctly recognizes that
datais always assigned before use (the catch block returns early, so the final return statement only executes when assignment succeeds).Also applies to: 21-47
packages/blocks/featured-service-list/src/frontend/FeaturedServiceList.types.ts (1)
3-3: LGTM! Type-only import is the correct choice.Since
Modelis used exclusively in type position (line 13), converting to a type-only import improves tree-shaking and reduces the runtime bundle size.packages/blocks/featured-service-list/src/frontend/FeaturedServiceList.server.tsx (1)
4-4: LGTM! Type-only import is appropriate.The
Modeltype is used only for type annotation (line 20), so a type-only import is correct and helps with bundle optimization.packages/blocks/pricing-section/src/frontend/PricingSection.types.ts (1)
3-3: LGTM: Type-only import improves tree-shaking.Converting to
import typeis a best practice whenModelis only used for type annotations. This enables TypeScript to elide the import at runtime, improving bundle size.packages/blocks/pricing-section/src/frontend/PricingSection.server.tsx (1)
4-4: LGTM: Single return path pattern addresses linter requirements.The refactoring consolidates rendering logic to a single return statement after error handling. TypeScript's control flow analysis correctly recognizes that
datais always assigned when reaching line 34, since the catch block provides an early return.This pattern aligns with ESLint rules that prefer consistent return structures and is applied uniformly across block components in this PR.
Also applies to: 20-43
packages/blocks/ticket-recent/src/frontend/TicketRecent.types.ts (1)
3-3: LGTM! Type-only import improves build optimization.Converting to
import typeis the correct approach sinceModelis only used in type positions. This eliminates unnecessary runtime imports and aligns with TypeScript best practices.packages/blocks/ticket-recent/src/frontend/TicketRecent.server.tsx (1)
4-4: LGTM! Improved type safety and cleaner control flow.The refactored pattern addresses the previous type safety concern and improves code clarity:
- Type-only import reduces bundle size
- Explicit type annotation (
Model.TicketRecentBlock) provides compile-time safety- Single return path after try-catch makes the control flow more explicit and easier to reason about
This pattern aligns well with the broader refactoring across other block components in this PR.
Also applies to: 14-36
packages/blocks/payments-history/src/frontend/PaymentsHistory.types.ts (1)
1-1: LGTM! Type-only import is the correct approach.Converting to a type-only import is best practice since
Modelis used exclusively as a type. This improves tree-shaking and makes the intent explicit.packages/blocks/payments-history/src/frontend/PaymentsHistory.server.tsx (2)
4-4: LGTM! Type-only import aligns with usage.The type-only import is correct since
Modelis used exclusively for type annotation.
14-31: LGTM! Refactoring improves code structure.The refactoring successfully separates data fetching from rendering logic. The control flow is sound: if the API call fails, the function returns
nullearly (line 26), and if it succeeds,datais definitely assigned before being used in the render (line 30).packages/blocks/ticket-summary/src/frontend/TicketSummary.types.ts (1)
3-3: LGTM! Proper use of type-only import.Since
Modelis only used in type positions (line 12), making it a type-only import correctly eliminates the runtime dependency and reduces bundle size.packages/blocks/document-list/src/frontend/DocumentList.types.ts (1)
3-3: LGTM! Type-only import optimization.Converting to
import typeis the correct approach sinceModelis only used in type positions. This helps with tree-shaking and reduces bundle size.packages/blocks/cta-section/src/frontend/CtaSection.types.ts (1)
3-3: LGTM! Type-only import optimization.The type-only import is correct since
Modelis only referenced in type annotations (line 13).packages/blocks/service-details/src/frontend/ServiceDetails.types.ts (1)
3-3: LGTM! Type-only import optimization.Correctly uses
import typesinceModelis only used in type positions (line 14).packages/blocks/service-details/src/frontend/ServiceDetails.server.tsx (1)
21-47: LGTM! Clean refactoring to single return path.The control flow is safe:
datais assigned in the try block, and the catch block returns early withnull, ensuringdatais always initialized when reaching the final return statement. This pattern improves consistency across the codebase.packages/blocks/invoice-list/src/frontend/InvoiceList.types.ts (1)
3-3: LGTM! Type-only import optimization.Correct use of
import typesinceModelappears only in type annotations (line 13).packages/blocks/hero-section/src/frontend/HeroSection.types.ts (1)
3-3: LGTM! Type-only import optimization.The type-only import is appropriate since
Modelis only used in type positions (line 13).packages/blocks/category/src/frontend/Category.types.ts (1)
6-6: LGTM! Type-only import optimization.Correctly converts to
import typesinceModelis only referenced in type annotations (line 19).packages/blocks/faq/src/frontend/Faq.types.ts (1)
3-3: LGTM! Type-only import optimization.The type-only import is correct since
Modelis only used in type annotations (line 14).packages/blocks/feature-section-grid/src/frontend/FeatureSectionGrid.types.ts (1)
3-3: LGTM! Proper type-only import optimization.Converting to a type-only import is correct since
Modelis only used in type positions (line 13). This reduces the runtime bundle size.packages/blocks/category/src/frontend/Category.server.tsx (1)
21-46: LGTM! Type-safe refactoring with proper control flow.This refactoring addresses the previous type safety concern correctly. The explicit type annotation on line 21 combined with the early return in the catch block (line 31) ensures TypeScript's control flow analysis can prove
datais definitely assigned when rendering occurs (lines 34-46).The pattern is valid because:
- If an error occurs, execution returns
nulland never reaches the render code- If no error occurs,
datais assigned and the render proceeds with a guaranteed non-undefined valueThis approach is cleaner than alternatives like
let data: Model.CategoryBlock | undefinedbecause it avoids unnecessary undefined handling in the render path.packages/blocks/invoice-list/src/frontend/InvoiceList.server.tsx (1)
20-42: LGTM! Consistent type-safe pattern.The refactoring follows the same type-safe pattern as other blocks in this PR, with proper control flow ensuring
datais definitely assigned when rendering.packages/blocks/document-list/src/frontend/DocumentList.server.tsx (1)
14-37: LGTM! Consistent refactoring with error logging.This follows the same type-safe pattern as other blocks, with the added benefit of error logging (line 24) for debugging purposes.
packages/blocks/cta-section/src/frontend/CtaSection.server.tsx (1)
12-26: LGTM! Clean refactoring with consistent pattern.The refactoring maintains type safety while creating a unified return path, consistent with the PR-wide improvements to server components.
packages/blocks/article-list/src/frontend/ArticleList.types.ts (1)
3-3: LGTM! Proper type-only import.Correctly converts to a type-only import since
Modelis only used in type positions.packages/blocks/category-list/src/frontend/CategoryList.server.tsx (1)
21-44: LGTM! Consistent type-safe refactoring.The refactoring follows the established pattern with proper typing and control flow, ensuring type safety through the early return in the catch block.
packages/blocks/orders-summary/src/frontend/OrdersSummary.types.ts (1)
3-3: LGTM! Proper type-only import optimization.Converting to a type-only import is correct since
Modelis only used in type positions (line 13).packages/blocks/category-list/src/frontend/CategoryList.types.ts (1)
3-3: LGTM! Type-only import improves tree-shaking.Converting to
import typeis correct sinceModelis only referenced in type positions (line 14). This prevents the Model namespace from being included in the runtime bundle.packages/blocks/product-list/src/frontend/ProductList.server.tsx (1)
4-27: LGTM! Clean separation of data fetching and rendering.The refactor improves code structure:
- Type-only import reduces bundle size
- Explicit typing on the data variable provides clear type safety
- Single return path after try/catch makes control flow more maintainable
- TypeScript's definite assignment analysis correctly tracks that
datais always assigned before use (try assigns, catch exits early)This pattern is consistent across other server components in the PR.
packages/blocks/orders-summary/src/frontend/OrdersSummary.server.tsx (1)
5-48: LGTM! Addresses previous review feedback.The explicit type annotation
let data: Model.OrdersSummaryBlockon line 22 properly addresses the concern raised in the previous review. The refactored control flow with a single return path after try/catch is clean and maintainable.packages/blocks/product-list/src/frontend/ProductList.types.ts (1)
3-3: LGTM! Correct use of type-only import.The
import typesyntax is appropriate sinceModelis only used in type positions throughout the file, ensuring it won't be included in the runtime bundle.packages/blocks/article/src/frontend/Article.types.ts (1)
3-3: LGTM! Consistent type-only import pattern.This change aligns with the repository-wide migration to type-only imports for Model, improving tree-shaking across all block types files.
packages/blocks/product-list/src/frontend/ProductList.client.tsx (1)
19-19: LGTM! Type-only import is correct.
Modelis only used in type positions (lines 87 and 92:Model.Product), so the type-only import correctly ensures it's not bundled in the client JavaScript.packages/blocks/article/src/frontend/Article.server.tsx (1)
4-34: LGTM! Well-structured refactor with explicit typing.The changes address the previous review feedback by adding explicit type annotation on line 12. The refactored control flow with:
- Type-only import for better tree-shaking
- Explicitly typed data variable
- Single return path after error handling
makes the code more maintainable and type-safe.
turbo/generators/templates/block/frontend/server.hbs (2)
4-28: Request verification that this refactor addresses ESLint warnings.The PR objectives state this change enforces
--max-warnings=0by addressing various linter warnings. However, the refactoring from an in-try return to a final return outside the try-catch block doesn't obviously correlate with common ESLint rules.Please confirm which specific ESLint rule(s) or warning(s) this pattern change resolves (e.g.,
no-useless-return, a custom rule about try-catch patterns, etc.) to ensure the template generates code that passes the stricter linting requirements.
14-28: The pre-declareddatavariable pattern is valid and preferred.The control flow in this template is sound. TypeScript's flow analysis correctly handles the pattern where
let data: Type;is declared, assigned in the try block, and used after the try-catch because the catch block returns early (preventingdatafrom being undefined at the usage point). This approach avoids non-null assertion operators, which aligns with strict ESLint rules that discourage their use. The refactor leverages TypeScript's definite assignment analysis to achieve type safety without unsafe!operators.packages/blocks/order-list/src/frontend/OrderList.server.tsx (2)
4-4: LGTM! Good type import.Adding the
Modeltype import from the API harmonization layer provides proper type safety for the data variable.
12-34: Refactoring to single-return pattern is correct.The code successfully uses a valid control flow pattern: declaring
databefore the try block, assigning it inside the try block, and returning in the catch block. Since the catch block always returns, TypeScript's control flow analysis recognizes thatdatais definitely assigned before being used on line 25. Withstrict: trueenabled in your TypeScript configuration, this pattern compiles without errors.packages/blocks/ticket-details/src/frontend/TicketDetails.types.ts (1)
3-3: LGTM! Type-only import improves tree-shaking.Converting to a type-only import is correct since
Modelis only used in type positions. This eliminates unnecessary runtime dependencies and likely resolves an ESLint warning.
…linter-warnings # Conflicts: # package-lock.json
…linter-warnings # Conflicts: # package-lock.json # package.json # packages/blocks/article-list/package.json # packages/blocks/article-search/package.json # packages/blocks/article/package.json # packages/blocks/category-list/package.json # packages/blocks/category/package.json # packages/blocks/faq/package.json # packages/blocks/featured-service-list/package.json # packages/blocks/invoice-list/package.json # packages/blocks/notification-details/package.json # packages/blocks/notification-list/package.json # packages/blocks/notification-summary/package.json # packages/blocks/order-details/package.json # packages/blocks/order-list/package.json # packages/blocks/orders-summary/package.json # packages/blocks/payments-history/package.json # packages/blocks/payments-summary/package.json # packages/blocks/quick-links/package.json # packages/blocks/service-details/package.json # packages/blocks/service-list/package.json # packages/blocks/surveyjs-form/package.json # packages/blocks/ticket-details/package.json # packages/blocks/ticket-list/package.json # packages/blocks/ticket-recent/package.json # packages/blocks/ticket-summary/package.json # packages/blocks/user-account/package.json
What does this PR do?
Key Changes
How to test
Summary by CodeRabbit
Bug Fixes
Improvements
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.