feat(ojoi-web): Categories page #16762
Conversation
WalkthroughThis pull request introduces enhancements to the Official Journal of Iceland application, including new utility functions for processing query strings, modifications to state management and filtering logic in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (13)
libs/api/domains/official-journal-of-iceland/src/lib/models/advert.model.ts (1)
21-23: Consider the impact of bidirectional relationships.The addition of the
categoriesfield creates a bidirectional relationship betweenAdvertMainCategoryandAdvertCategory. While the implementation is correct, be mindful of:
- Potential circular references during serialization
- The need to maintain consistency when updating either side of the relationship
Consider implementing proper serialization handling (e.g., using class-transformer's
@Transform()) if you need to serialize these objects to JSON.apps/web/components/OfficialJournalOfIceland/OJOIUtils.ts (3)
95-107: Consider enhancing input handling robustnessWhile the function correctly handles basic cases, consider adding these improvements:
- Trim whitespace from array elements
- Filter out empty strings
- Handle URL-encoded commas in the input string
Consider this enhanced implementation:
export const getStringArrayFromQueryString = ( value?: string | string[], ): string[] => { if (!value) { return [] } if (Array.isArray(value)) { - return value + return value.map(v => v.trim()).filter(Boolean) } - return value.split(',') + return value.split(',').map(v => v.trim()).filter(Boolean) }
109-121: Consider enhancing type safety and edge case handlingThe function handles basic cases well, but could be improved to handle edge cases more robustly:
- Trim input strings
- Handle empty strings consistently
- Add JSDoc documentation for clarity
Consider this enhanced implementation:
+/** + * Extracts a single string value from a query parameter which can be either a string or an array of strings. + * @param value - The query parameter value + * @returns A single string value, undefined if the input is empty or contains only whitespace + */ export const getStringFromQueryString = ( value?: string | string[], ): string | undefined => { if (!value) { return undefined } if (Array.isArray(value)) { - return value[0] + const firstValue = value[0]?.trim() + return firstValue || undefined } - return value + const trimmedValue = value.trim() + return trimmedValue || undefined }
94-121: Well-structured utility functions that support SSR transitionThe new utility functions are well-designed for server-side rendering:
- Pure functions with proper TypeScript types
- Consistent error handling
- Support for both client and server-side query parameter processing
This aligns well with the PR objective of transitioning to server-side rendering and optimizing filtering.
Consider adding unit tests to verify the behavior with various query parameter formats and edge cases.
apps/web/screens/queries/OfficialJournalOfIceland.ts (1)
177-181: Consider adding JSDoc documentation for the query structureTo improve maintainability, consider adding JSDoc comments explaining:
- The hierarchical relationship between main categories and their subcategories
- The purpose and usage of each field
- Example response structure
Example documentation:
/** * Query to fetch main categories with their nested subcategories. * * @example Response structure: * { * mainCategories: [{ * title: string; * slug: string; * description: string; * categories: [{ * id: string; * title: string; * slug: string; * }] * }] * } */ export const MAIN_CATEGORIES_QUERY = gql`libs/island-ui/core/src/lib/CategoryCard/CategoryCard.tsx (1)
167-189: Simplify the heading rendering logic to reduce duplication.The current implementation has duplicated props and logic between branches. Consider refactoring to improve maintainability.
Here's a suggested improvement:
- {href && !nestedHref ? ( - <Text - as={headingAs} - variant={headingVariant} - color={textColor} - truncate={truncateHeading} - title={heading} - > - {hyphenate ? <Hyphen>{heading}</Hyphen> : heading} - </Text> - ) : ( - <Link href={href}> - <Text - as={headingAs} - variant={headingVariant} - color={textColor} - truncate={truncateHeading} - title={heading} - > - {hyphenate ? <Hyphen>{heading}</Hyphen> : heading} - </Text> - </Link> - )} + {(() => { + const headingContent = ( + <Text + as={headingAs} + variant={headingVariant} + color={textColor} + truncate={truncateHeading} + title={heading} + > + {hyphenate ? <Hyphen>{heading}</Hyphen> : heading} + </Text> + ); + return nestedHref ? <Link href={href}>{headingContent}</Link> : headingContent; + })()}This refactoring:
- Eliminates prop duplication
- Simplifies the conditional logic
- Makes the code more maintainable
apps/web/screens/OfficialJournalOfIceland/OJOIHome.tsx (3)
13-13: Remove unused importTagThe
Tagcomponent is imported but not used in the code.- Tag,
171-178: Document the purpose of nestedHref propThe
nestedHref={true}prop appears to be related to fixing hydration errors, but its purpose isn't immediately clear. Consider adding a code comment explaining its significance.<CategoryCard + // Prevents link nesting when tags have hrefs nestedHref={true} key={category.slug} href={`${categoriesUrl}?yfirflokkur=${category.slug}`} heading={category.title} text={category.description} tags={subCategories} />
246-249: Document the rationale for pageSize limitConsider adding a comment explaining why the pageSize is set to 5 adverts.
input: { page: 1, + // Limit to 5 most recent adverts on the home page pageSize: 5, },apps/web/screens/OfficialJournalOfIceland/messages.ts (1)
63-71: LGTM! Error messages are well-structured and consistent.The new error messages for category fetching follow the established patterns:
- Unique IDs with appropriate namespace
- Consistent error message structure (title + detailed message)
- Matches the style of existing error messages (e.g.,
errorFetchingAdvertsTitle)Consider extracting common error message patterns into reusable templates to maintain consistency as more error scenarios are added. For example:
const createErrorMessages = (feature: string) => ({ title: { id: `web.ojoi:search.errorFetching${feature}Title`, defaultMessage: `Ekki tókst að sækja ${feature.toLowerCase()}`, }, message: { id: `web.ojoi:search.errorFetching${feature}Message`, defaultMessage: 'Ekki náðist samband við vefþjónustur Stjórnartíðinda, reynið aftur síðar.', }, });libs/clients/official-journal-of-iceland/public/src/clientConfig.json (1)
255-264: Add descriptions for theidsquery parameters.The
idsparameter has been consistently added across multiple endpoints, but it's missing a description field that would help API consumers understand its purpose and usage.Add descriptions like this:
{ "name": "ids", "required": false, + "description": "Filter results by specific IDs", "in": "query", "schema": { "default": [], "type": "array", "items": { "type": "string" } } }Also applies to: 305-314, 355-364, 454-463
apps/web/screens/OfficialJournalOfIceland/OJOICategories.tsx (2)
348-348: Remove commented-out codeThe
disabledprop is commented out. If it's no longer needed, consider removing it to keep the code clean and maintainable.Apply this diff to remove the commented code:
- // disabled={isDisabled}
449-450: Consider avoiding hardcoding 'pageSize'Hardcoding
pageSizeto 1000 may lead to performance issues or API limitations as the dataset grows. Consider using a constant, environment variable, or implementing proper pagination to handle larger datasets dynamically.For example, you could define a constant for
MAX_PAGE_SIZE:+ const MAX_PAGE_SIZE = 1000; ... variables: { params: { - pageSize: 1000, + pageSize: MAX_PAGE_SIZE, page: 1, }, },Also applies to: 458-459
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
apps/web/components/OfficialJournalOfIceland/OJOIUtils.ts(1 hunks)apps/web/screens/OfficialJournalOfIceland/OJOICategories.tsx(9 hunks)apps/web/screens/OfficialJournalOfIceland/OJOIHome.tsx(6 hunks)apps/web/screens/OfficialJournalOfIceland/messages.ts(1 hunks)apps/web/screens/queries/OfficialJournalOfIceland.ts(1 hunks)libs/api/domains/official-journal-of-iceland/src/lib/models/advert.model.ts(1 hunks)libs/clients/official-journal-of-iceland/public/src/clientConfig.json(5 hunks)libs/island-ui/core/src/lib/CategoryCard/CategoryCard.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
apps/web/components/OfficialJournalOfIceland/OJOIUtils.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/web/screens/OfficialJournalOfIceland/OJOICategories.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/web/screens/OfficialJournalOfIceland/OJOIHome.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/web/screens/OfficialJournalOfIceland/messages.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/web/screens/queries/OfficialJournalOfIceland.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
libs/api/domains/official-journal-of-iceland/src/lib/models/advert.model.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/clients/official-journal-of-iceland/public/src/clientConfig.json (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/island-ui/core/src/lib/CategoryCard/CategoryCard.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🪛 Biome
apps/web/screens/OfficialJournalOfIceland/OJOICategories.tsx
[error] 350-350: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
🔇 Additional comments (6)
apps/web/screens/queries/OfficialJournalOfIceland.ts (1)
177-181: Well-structured enhancement to support SSR and optimize filtering!
The addition of the categories field to MAIN_CATEGORIES_QUERY is a good improvement that:
- Reduces the need for separate API calls by including nested category data
- Supports the transition to server-side rendering
- Aligns with the filtering optimization goals
libs/island-ui/core/src/lib/CategoryCard/CategoryCard.tsx (2)
16-16: LGTM: Clean TypeScript prop definition.
The new nestedHref prop is well-typed as an optional boolean and follows TypeScript best practices.
Also applies to: 40-40
102-102: Implementation effectively prevents nested links.
The conditional href logic successfully prevents invalid HTML when tags have their own hrefs. The default false value maintains backward compatibility.
Let's verify there are no other instances of potential link nesting issues:
Also applies to: 126-126
✅ Verification successful
Implementation correctly prevents nested links
Based on the CategoryCard implementation, the code effectively prevents nested links through:
- Conditional rendering of href on FocusableBox (
href={!nestedHref ? href : undefined}) - Complementary conditional rendering of Link component when nestedHref is true
- Default value of
nestedHref = falsemaintains backward compatibility
The implementation ensures that either the outer FocusableBox has the href (when nestedHref is false) or the inner Link component is used (when nestedHref is true), but never both simultaneously, preventing invalid HTML nesting.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential nested link patterns in components
ast-grep --pattern 'href={$_} $$$
<Link $$$
</Link>'
Length of output: 54
Script:
#!/bin/bash
# Search for potential nested link patterns using different approaches
# Look for components with href prop that might contain Link components
ast-grep --pattern 'interface $_ {
$$$
href?: $_
$$$
}'
# Search for Link components that might be wrapped in elements with href
rg -U "href.*\n.*<Link" --type tsx
# Search for potential nested anchor patterns
rg -U "<a.*\n.*<a" --type tsx
# Find all Link component usage patterns for manual review
ast-grep --pattern '<Link $$$>
$$$
</Link>'
Length of output: 12321
Script:
#!/bin/bash
# Search for potential nested link patterns with file extension specified correctly
# Search for Link components that might be wrapped in elements with href
rg -U "href.*\n.*<Link" -t typescript -t tsx
# Search for potential nested anchor patterns
rg -U "<a.*\n.*<a" -t typescript -t tsx
# Look specifically at the CategoryCard implementation
rg -U "FocusableBox.*href" libs/island-ui/core/src/lib/CategoryCard/CategoryCard.tsx
Length of output: 245
Script:
#!/bin/bash
# Search with correct syntax for ripgrep
# Look for nested link patterns
rg -t ts -t tsx -U "href.*\n.*<Link"
# Look for nested anchor patterns
rg -t ts -t tsx -U "<a.*\n.*<a"
# Check CategoryCard implementation
cat libs/island-ui/core/src/lib/CategoryCard/CategoryCard.tsx
Length of output: 7504
apps/web/screens/OfficialJournalOfIceland/OJOIHome.tsx (2)
204-209: LGTM: Well-structured TypeScript interface
The interface properly defines the required props with appropriate types, ensuring type safety throughout the component hierarchy.
230-251: LGTM: Efficient server-side data fetching implementation
The implementation follows NextJS best practices:
- Uses concurrent requests with Promise.all
- Proper error handling
- Server-side data fetching aligns with the PR objective
libs/clients/official-journal-of-iceland/public/src/clientConfig.json (1)
1005-1036: LGTM! Well-structured MainCategory schema.
The updated MainCategory schema is well-designed with:
- Comprehensive field documentation
- Clear examples
- Proper hierarchical structure with categories
- All necessary fields for UI rendering
Datadog ReportAll test runs ✅ 34 Total Test Services: 0 Failed, 32 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (3) |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
apps/web/screens/OfficialJournalOfIceland/OJOICategories.tsx (1)
349-349: Simplify the boolean expression in outlined propThe expression can be simplified for better readability.
Apply this diff:
-outlined={!isActive} +outlined={!isActive}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
apps/web/screens/OfficialJournalOfIceland/OJOICategories.tsx(9 hunks)apps/web/screens/OfficialJournalOfIceland/OJOIHome.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/screens/OfficialJournalOfIceland/OJOIHome.tsx
🧰 Additional context used
📓 Path-based instructions (1)
apps/web/screens/OfficialJournalOfIceland/OJOICategories.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (4)
apps/web/screens/OfficialJournalOfIceland/OJOICategories.tsx (4)
104-165: LGTM! Well-structured filtering logic with proper memoization
The sequential filtering and grouping logic is efficient and well-organized. The useMemo dependencies are correctly specified.
167-240: LGTM! Well-implemented event handlers with proper debouncing
The event handlers effectively manage both state and URL updates. The use of debouncing for search and shallow routing for URL updates is optimal.
137-148:
Fix typo in variable name 'groupedByLetter'
The variable name contains a typo that was previously flagged but not addressed.
Apply this diff to fix the typo:
-const groupedByLetter = letters.reduce((acc, letter) => {
+const groupedByLetter = letters.reduce((acc, letter) => {Likely invalid or redundant comment.
447-459: Review the pagination limit of 1000 items
The hard-coded limit of 1000 items per query might need review:
- Consider if this limit is sufficient for future growth
- Consider implementing proper pagination if the data volume might exceed this limit
✅ Verification successful
Hard-coded pagination limit is consistently used across the Official Journal of Iceland module
The pagination limit of 1000 items appears to be a deliberate design choice consistently implemented across the Official Journal of Iceland module:
apps/web/screens/OfficialJournalOfIceland/OJOICategories.tsxapps/web/screens/OfficialJournalOfIceland/OJOISearch.tsxlibs/application/templates/official-journal-of-iceland/src/hooks/useTypes.tslibs/application/templates/official-journal-of-iceland/src/hooks/useCategories.ts
Since this is a consistent pattern across the module and likely based on known data volumes for the Official Journal of Iceland, the current implementation is acceptable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check current data volume
rg "categories\?: \[" --type graphql -A 5
Length of output: 75
Script:
#!/bin/bash
# Search for GraphQL schema definitions and type definitions related to categories
rg "type.*Categories?" --type ts -A 5
# Search for actual category data or usage patterns
rg "pageSize.*1000" --type ts -A 2
Length of output: 6928
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
libs/island-ui/core/src/lib/CategoryCard/CategoryCard.tsx (2)
126-126: Consider preserving the "entire card clickable" behavior.While the current implementation correctly prevents nested anchor tags, it changes the UX from having the entire card clickable to only the title being clickable when tags have hrefs. Consider using onClick handlers or router events to maintain the original UX while avoiding nested anchors.
Example approach:
-href={!hasNestedHref ? href : undefined} +onClick={!hasNestedHref ? undefined : (e) => { + // Prevent click if the click target is a tag + if (!(e.target as HTMLElement).closest('[data-tag]')) { + router.push(href); + } +}}
167-189: Reduce prop duplication in conditional rendering.The Text component props are duplicated between conditions. Consider extracting the common props to reduce duplication and improve maintainability.
Example approach:
+const textProps = { + as: headingAs, + variant: headingVariant, + color: textColor, + truncate: truncateHeading, + title: heading, + children: hyphenate ? <Hyphen>{heading}</Hyphen> : heading +} {href && hasNestedHref ? ( <LinkV2 href={href}> - <Text - as={headingAs} - variant={headingVariant} - color={textColor} - truncate={truncateHeading} - title={heading} - > - {hyphenate ? <Hyphen>{heading}</Hyphen> : heading} - </Text> + <Text {...textProps} /> </LinkV2> ) : ( - <Text - as={headingAs} - variant={headingVariant} - color={textColor} - truncate={truncateHeading} - title={heading} - > - {hyphenate ? <Hyphen>{heading}</Hyphen> : heading} - </Text> + <Text {...textProps} /> )}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
apps/web/screens/OfficialJournalOfIceland/OJOIHome.tsx(6 hunks)libs/island-ui/core/src/lib/CategoryCard/CategoryCard.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/screens/OfficialJournalOfIceland/OJOIHome.tsx
🧰 Additional context used
📓 Path-based instructions (1)
libs/island-ui/core/src/lib/CategoryCard/CategoryCard.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (2)
libs/island-ui/core/src/lib/CategoryCard/CategoryCard.tsx (2)
122-123: LGTM! Efficient implementation of nested href detection.
The implementation efficiently detects nested hrefs using Array.some(), addressing the invalid HTML issue of nested anchor tags as discussed in previous reviews.
Line range hint 1-266: LGTM! Compliant with coding guidelines.
The component follows all required guidelines:
- ✓ Reusable across NextJS apps with clear props interface
- ✓ Strong TypeScript usage with proper type definitions
- ✓ Effective tree-shaking with specific imports
* Moved adverts query to server side, added ui for main categories on the front page. * Updated client, added sub categories. * Set tag url to search * Malaflokkar page is now ready and functioning properly. * Removed console logs * Fixed PR comments. * Moved nested href property to a calculated value * Sorted imports * Simplified logic and updated from Link to LinkV2 --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
What
CategoryCardcomponent so that if you passtagsproperty withhrefthen thetagslinks wont be nested inside theCategoryCardlink causing a hydration error / invalid htmlWhy
So that users can easily search for cases with certain categories.
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
These updates enhance user experience by providing better search capabilities and more informative error handling.