fix(web-grants): Various fixes #16985
Conversation
WalkthroughThe changes in this pull request involve modifications across multiple files primarily related to the handling of grants within the application. Key updates include adjustments to routing paths, enhancements to breadcrumb navigation and error handling, localization improvements, and restructuring of GraphQL queries. Additionally, utility functions for generating status tags have been introduced, and the grant model has been streamlined to simplify status handling. Overall, these changes aim to improve the organization and clarity of the code while maintaining existing functionalities. Changes
Suggested labels
Suggested reviewers
Possibly related PRs
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: 6
🧹 Outside diff range and nitpick comments (13)
apps/web/screens/Grants/utils.ts (2)
6-9: Consider using a discriminated union type for better type safety.While the current type safety is good, we can make it even better by using a discriminated union type for the return value.
Consider this type improvement:
+type StatusTag = { + label: string; + variant: TagVariant; +} export const generateStatusTag = ( status: GrantStatus, formatMessage: FormatMessage, -): { label: string; variant: TagVariant } | undefined => { +): StatusTag | undefined => {
13-25: Document the semantic meaning of tag variants.The color variants ('mint', 'rose', 'purple') seem to have specific semantic meanings for different statuses. Consider adding a comment explaining the color coding convention.
Add a comment like this at the top of the function:
/** * Generates a status tag with appropriate styling based on grant status: * - mint: Open for applications * - rose: Closed for applications * - purple: Special case, see description */libs/cms/src/lib/models/grant.model.ts (1)
33-34: Consider using an enum for applicationDeadlineStatusThe field follows TypeScript and GraphQL best practices. However, since this represents a status, consider using an enum to restrict possible values and improve type safety.
enum ApplicationDeadlineStatus { OPEN = 'OPEN', CLOSED = 'CLOSED', UPCOMING = 'UPCOMING' } @Field(() => ApplicationDeadlineStatus, { nullable: true }) applicationDeadlineStatus?: ApplicationDeadlineStatusapps/web/screens/Grants/Grant/GrantSidebar.tsx (2)
Line range hint
48-54: Consider adding accessibility attributes for new tab behaviorWhile opening links in new tabs can be useful, ensure proper accessibility by adding appropriate ARIA attributes and visual indicators.
Consider adding these accessibility enhancements:
<LinkV2 {...linkResolver(grant.fund.link.type as LinkType, [ grant.fund.link.slug, ])} newTab color="blue400" underline="normal" underlineVisibility="hover" + aria-label={`${grant.fund.title} (opens in new tab)`} > {grant.fund.title} + <span className="visually-hidden">(opens in new tab)</span> </LinkV2>
Line range hint
1-134: Well-structured component with clean separation of concernsThe component follows React/NextJS best practices with:
- Proper use of hooks (useMemo for derived data)
- Clear separation of UI elements and data transformation
- Type-safe props and GraphQL integration
Consider extracting the panel generation logic into separate components if this pattern is reused across other sidebars in the application.
apps/web/screens/Grants/SearchResults/SearchResultsFilter.tsx (1)
Line range hint
23-40: Consider performance optimizationsThe component could benefit from memoization and custom hooks to optimize performance and improve code organization.
Consider these improvements:
- Memoize filter transformations:
const categoryFilters = useMemo(() => tags?.filter((t) => t.genericTagGroup?.slug === 'grant-category'), [tags] ); const typeFilters = useMemo(() => tags?.filter((t) => t.genericTagGroup?.slug === 'grant-type'), [tags] );
- Extract filter logic to a custom hook:
const useGrantFilters = (tags: Array<GenericTag>) => { const categoryFilters = useMemo(() => tags?.filter((t) => t.genericTagGroup?.slug === 'grant-category'), [tags] ); const typeFilters = useMemo(() => tags?.filter((t) => t.genericTagGroup?.slug === 'grant-type'), [tags] ); return { categoryFilters, typeFilters }; };apps/web/screens/Grants/SearchResults/SearchResultsContent.tsx (2)
57-61: Consider adding type safety to tag generationThe status tag implementation is clean and well-organized. Consider adding type assertion to ensure
grant.statusis of typeGrantStatus.- tag={ - grant.status - ? generateStatusTag(grant.status, formatMessage) - : undefined - } + tag={ + grant.status + ? generateStatusTag(grant.status as GrantStatus, formatMessage) + : undefined + }
86-92: Address the TODO comment regarding deadline textThe comment indicates that the deadline text implementation is incomplete. This should be addressed before merging.
Would you like me to help implement the deadline text formatting or create an issue to track this task?
apps/web/screens/Grants/Home/GrantsHome.tsx (3)
Line range hint
77-77: Address TODO comment for categoriesThe TODO comment indicates pending work for category implementation. Since the categories are being mapped and rendered, this comment might be outdated.
Consider either implementing the pending work or removing the comment if it's no longer relevant.
Line range hint
183-184: Address TODO comment for organizationsThe comment suggests that more organizations need to be added. This should be tracked and implemented if still relevant.
Would you like me to help create a GitHub issue to track this enhancement?
Line range hint
206-208: Consider NextJS optimization opportunitiesWhile the implementation follows NextJS best practices, consider these optimizations:
- Add
getStaticPropsinstead of dynamic data fetching if the grant categories don't change frequently- Implement proper error boundaries for GraphQL query failures
- Consider adding loading states for better UX
apps/web/screens/Grants/Grant/Grant.tsx (2)
43-43: Consider memoizing the breadcrumb items arrayThe breadcrumb items array is recreated on every render. Consider using useMemo to optimize performance:
- const breadcrumbItems = [ + const breadcrumbItems = useMemo(() => [ { title: 'Ísland.is', href: linkResolver('homepage', [], locale).href, }, { title: formatMessage(m.home.title), href: baseUrl, }, { title: formatMessage(m.search.results), href: searchUrl, }, { title: grant?.name ?? formatMessage(m.home.grant), href: currentUrl, isTag: true, }, - ] + ], [baseUrl, searchUrl, currentUrl, locale, grant?.name, formatMessage])Also applies to: 59-62
Line range hint
76-162: Consider component decomposition for better maintainabilityThe grant details section (description, who can apply, how to apply, etc.) contains multiple similar blocks. Consider extracting these into a reusable component to improve maintainability and reduce code duplication.
Example structure:
interface GrantSectionProps { title?: string; content: SliceType[]; locale: Locale; showDivider?: boolean; } const GrantSection: React.FC<GrantSectionProps> = ({ title, content, locale, showDivider }) => { if (!content?.length) return null; return ( <> <Box> {title && <Text variant="h3">{title}</Text>} <Box className="rs_read"> {webRichText(content, undefined, locale)} </Box> </Box> {showDivider && <Divider />} </> ); };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
libs/cms/src/lib/generated/contentfulTypes.d.tsis excluded by!**/generated/**
📒 Files selected for processing (12)
apps/web/hooks/useLinkResolver/useLinkResolver.ts(1 hunks)apps/web/screens/Grants/Grant/Grant.tsx(3 hunks)apps/web/screens/Grants/Grant/GrantSidebar.tsx(4 hunks)apps/web/screens/Grants/Home/GrantsHome.tsx(1 hunks)apps/web/screens/Grants/SearchResults/SearchResultsContent.tsx(5 hunks)apps/web/screens/Grants/SearchResults/SearchResultsFilter.tsx(2 hunks)apps/web/screens/Grants/messages.ts(2 hunks)apps/web/screens/Grants/utils.ts(1 hunks)apps/web/screens/queries/Grants.ts(2 hunks)libs/cms/src/lib/cms.elasticsearch.service.ts(0 hunks)libs/cms/src/lib/models/grant.model.ts(4 hunks)libs/cms/src/lib/search/importers/grants.service.ts(1 hunks)
💤 Files with no reviewable changes (1)
- libs/cms/src/lib/cms.elasticsearch.service.ts
🧰 Additional context used
📓 Path-based instructions (11)
apps/web/hooks/useLinkResolver/useLinkResolver.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/Grants/Grant/Grant.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/Grants/Grant/GrantSidebar.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/Grants/Home/GrantsHome.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/Grants/SearchResults/SearchResultsContent.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/Grants/SearchResults/SearchResultsFilter.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/Grants/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/Grants/utils.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/Grants.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/cms/src/lib/models/grant.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/cms/src/lib/search/importers/grants.service.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."
🔇 Additional comments (18)
apps/web/screens/Grants/utils.ts (1)
1-4: LGTM! Well-organized imports with proper type safety.
The imports are properly typed and follow TypeScript best practices.
apps/web/screens/queries/Grants.ts (2)
12-12: LGTM! Verify components using this query
The field rename from applicationDeadlineText to applicationDeadlineStatus improves semantics and aligns with the model changes.
Let's verify all components using this query have been updated:
✅ Verification successful
Field rename successfully implemented across the codebase
The verification confirms that:
- No instances of the old
applicationDeadlineTextfield remain in the codebase - The new
applicationDeadlineStatusfield is properly used in all relevant components:GrantSidebar.tsxSearchResultsContent.tsxGrants.ts(queries)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components using GET_GRANTS_QUERY and check for old field usage
rg -l "GET_GRANTS_QUERY" apps/web/screens/ | xargs rg "applicationDeadlineText"
Length of output: 77
Script:
#!/bin/bash
# First, let's find files that use GET_GRANTS_QUERY
rg -l "GET_GRANTS_QUERY" apps/web/
# Then, let's check for any remaining usage of applicationDeadlineText
rg "applicationDeadlineText" apps/web/
# Finally, let's see the actual usage of applicationDeadlineStatus
rg "applicationDeadlineStatus" apps/web/
Length of output: 805
70-71: LGTM! Verify type definitions
The field changes align with the model updates and maintain consistency with GET_GRANTS_QUERY.
Let's verify the GraphQL types are properly defined:
libs/cms/src/lib/models/grant.model.ts (2)
Line range hint 1-124: Well-structured model with proper TypeScript integration
The code follows best practices for a shared library:
- Strong TypeScript typing with proper decorators
- Clean exports for tree-shaking
- Reusable model structure suitable for cross-app usage
14-14: Verify the removal of OPENS_SOON and INACTIVE statuses
The consolidation of statuses could impact existing code. Let's verify all usages of the removed statuses.
✅ Verification successful
Let me try a different search strategy to verify the impact of these status changes.
Status consolidation change is properly implemented
The codebase search shows that:
- The old statuses (OPENS_SOON and INACTIVE) are not referenced anywhere in the codebase
- The new SEE_DESCRIPTION status is properly integrated in:
- The status mapping logic in
libs/cms/src/lib/models/grant.model.ts - The status handling in
apps/web/screens/Grants/utils.tswith appropriate label and styling
- The status mapping logic in
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of the old status values
echo "Searching for OPENS_SOON usage..."
rg -l "OPENS_SOON" --type ts
echo "Searching for INACTIVE usage..."
rg -l "INACTIVE" --type ts
# Search for components using GrantStatus enum
echo "Searching for components using GrantStatus..."
ast-grep --pattern 'import { $$$, GrantStatus, $$$ } from $path'
Length of output: 373
Script:
#!/bin/bash
# Search for any usage of GrantStatus enum with context
echo "Searching for GrantStatus usage with context..."
rg "GrantStatus\." -A 3 --type ts
# Search for any switch/case statements handling grant statuses
echo "Searching for switch statements with grant status..."
ast-grep --pattern 'switch ($expr) {
case GrantStatus.$_:
$$$
}'
# Search for conditional statements checking grant statuses
echo "Searching for conditional checks on grant status..."
rg "(OPENS_SOON|INACTIVE)" --type ts -A 2
Length of output: 1931
apps/web/screens/Grants/Grant/GrantSidebar.tsx (2)
11-12: LGTM: Clean localization refactor
The switch from useIntl to useLocale and the addition of the status tag utility improve code organization and maintainability.
Also applies to: 35-35
75-84: Verify status tag implementation consistency
The status display has been updated to use the new generateStatusTag utility. Let's verify its implementation across the codebase.
✅ Verification successful
Let me gather more information about the status tag generation implementation.
Let me search specifically for the implementation of generateStatusTag utility.
Status tag implementation is consistent with the model
The generateStatusTag utility in apps/web/screens/Grants/utils.ts correctly handles the GrantStatus enum defined in libs/cms/src/lib/models/grant.model.ts. The implementation properly maps grant statuses to appropriate display tags with consistent formatting.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of generateStatusTag and applicationDeadlineStatus
# Check for generateStatusTag usage
echo "Checking generateStatusTag usage patterns:"
ast-grep --pattern 'generateStatusTag($status, $formatMessage)'
# Check for applicationDeadlineStatus references
echo "Checking applicationDeadlineStatus in GraphQL queries:"
rg -A 2 'applicationDeadlineStatus'
Length of output: 2170
Script:
#!/bin/bash
# Search for generateStatusTag implementation and related utilities
echo "Searching for generateStatusTag implementation:"
rg -A 5 "export.*generateStatusTag"
echo "Searching for status tag related types and enums:"
rg -A 5 "enum.*Status|type.*Status"
echo "Searching for other status tag usages in components:"
rg -A 2 "generateStatusTag|StatusTag"
Length of output: 106027
Script:
#!/bin/bash
# Search for generateStatusTag implementation
echo "Searching for generateStatusTag implementation:"
rg -A 10 "export.*generateStatusTag"
# Search for GrantStatus enum
echo "Searching for GrantStatus enum definition:"
rg -A 5 "enum GrantStatus"
Length of output: 1252
apps/web/screens/Grants/SearchResults/SearchResultsFilter.tsx (1)
10-10: LGTM: Import statement follows best practices
The addition of GrantStatus import is well-organized alongside related GraphQL schema imports.
apps/web/screens/Grants/messages.ts (2)
51-54: LGTM: New message follows established patterns.
The new 'applicationSeeDescription' message follows the established message definition pattern and is appropriately placed in the search section.
21-21: Verify the impact of terminology change.
The message change from 'Leitarniðurstöður' (Search results) to 'Styrkir' (Grants) makes the UI more specific to grants. This aligns with the routing path changes mentioned in the summary.
Let's verify the usage of this message key to ensure consistent terminology:
✅ Verification successful
Terminology change is consistent and well-placed
The message change from 'Leitarniðurstöður' to 'Styrkir' is used in breadcrumb navigation in both the SearchResults and Grant components, making it a logical change that provides better context. The term appears in expected locations and maintains consistency across the grants section of the application.
- Used in
SearchResults/SearchResults.tsxfor the search results page breadcrumb - Used in
Grant/Grant.tsxfor the individual grant view breadcrumb - No conflicting or inconsistent usage found elsewhere in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components using this message key
rg -t tsx -t ts "search\.results" apps/web/screens/Grants
Length of output: 87
Script:
#!/bin/bash
# Search for components using this message key, without file type restrictions
rg "search\.results" apps/web/screens/Grants
Length of output: 495
Script:
#!/bin/bash
# Search for the message ID in the entire codebase
rg "web\.grants\.search\.results"
Length of output: 35
Script:
#!/bin/bash
# List all files in the Grants directory to understand the structure
fd . apps/web/screens/Grants
Length of output: 511
Script:
#!/bin/bash
# Check the content of the components using this message to understand the context
rg -A 5 "formatMessage\(m\.search\.results\)" apps/web/screens/Grants
Length of output: 914
Script:
#!/bin/bash
# Look for any other similar terminology changes in the codebase
rg "Styrkir" apps/web/screens/Grants
Length of output: 105
libs/cms/src/lib/search/importers/grants.service.ts (1)
Line range hint 1-24: LGTM! Code follows library guidelines.
The implementation adheres to the library coding guidelines with proper TypeScript usage, clear interface implementation, and maintainable dependency injection patterns.
apps/web/screens/Grants/SearchResults/SearchResultsContent.tsx (1)
6-6: LGTM: Import changes are well-organized
The import changes align well with the refactoring to use the new generateStatusTag utility and localization hook.
Also applies to: 15-16
apps/web/screens/Grants/Home/GrantsHome.tsx (3)
Line range hint 31-56: Well-structured component with proper TypeScript usage
The component follows NextJS best practices with:
- Proper type safety using TypeScript
- Clean separation of concerns between presentation and container components
- Effective use of custom hooks for internationalization and routing
Line range hint 182-204: Well-implemented server-side data fetching
The GraphQL integration follows best practices:
- Proper typing with QueryGetGenericTagsInTagGroupsArgs
- Handles locale correctly
- Uses server-side data fetching appropriately
54-54: Verify the isTag prop handling in Breadcrumbs component
The addition of isTag: true suggests a visual or behavioral change in the breadcrumb rendering. Please ensure that the Breadcrumbs component from @island.is/island-ui/core properly handles this prop.
✅ Verification successful
Let me gather more information about the Breadcrumbs component implementation to verify the isTag prop handling.
The isTag prop is properly handled by the Breadcrumbs component
The Breadcrumbs component from @island.is/island-ui/core correctly implements and handles the isTag prop:
- It's properly typed in the
BreadCrumbIteminterface - When
isTagis true, it renders the breadcrumb item using theTagcomponent instead of plain text - The implementation includes proper styling and accessibility considerations
- The usage pattern is consistent across multiple files in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Breadcrumbs component implementation to verify isTag prop handling
ast-grep --pattern 'interface $_ {
$$$
isTag$_
$$$
}'
# Search for Breadcrumbs usage with isTag prop
rg -A 2 'isTag.*true'
Length of output: 4200
Script:
#!/bin/bash
# Search for Breadcrumbs component implementation and interface
ast-grep --pattern 'interface BreadcrumbItem {
$$$
}'
# Look for the actual implementation of how isTag is used
rg -A 5 'const Breadcrumbs' libs/island-ui/core/src/lib/Breadcrumbs/
Length of output: 649
Script:
#!/bin/bash
# Search for the full Breadcrumbs component implementation
cat libs/island-ui/core/src/lib/Breadcrumbs/Breadcrumbs.tsx
# Check if there are any type definitions for BreadcrumbItem
rg -A 5 'interface BreadcrumbItem' libs/island-ui/core/src/lib/Breadcrumbs/
Length of output: 2815
apps/web/screens/Grants/Grant/Grant.tsx (2)
25-25: LGTM: Clean import addition
The CustomNextError import is well-placed and follows the project's import organization pattern.
Line range hint 183-186: LGTM: Proper error handling implementation
The error handling follows NextJS best practices by:
- Using appropriate 404 status code for missing resources
- Throwing the error before props are returned
- Providing a clear error message
apps/web/hooks/useLinkResolver/useLinkResolver.ts (1)
315-315: Verify route change impact and update documentation
The semantic improvement from /leit to /styrkir better reflects the content. However, this change requires verification:
- Ensure all components and links using this route are updated
- Update any relevant documentation referencing this route
- Consider adding a redirect for existing bookmarks
Let's verify the route usage across the codebase:
✅ Verification successful
Let me gather more information about the route usage and potential impacts.
Let me try one final verification with modified commands:
Based on the verification results, I can now provide a final response:
Route change is safe and properly contained
The route change from /styrkjatorg/leit to /styrkjatorg/styrkir is safe as:
- No references to the old route
/styrkjatorg/leitexist in the codebase - The new route
/styrkjatorg/styrkiris only defined inuseLinkResolver.ts - All route usages in the Grants feature components (
GrantsHome.tsx,SearchResults.tsx,Grant.tsx) use the linkResolver to generate URLs dynamically
The change is properly encapsulated within the link resolver system, and there are no hardcoded references to either the old or new route that would need updating.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential references to the old route and verify new route usage
# Check for old route references
echo "Checking for old route references..."
rg -l "styrkjatorg/leit"
# Check new route usage
echo "Checking new route usage..."
rg -l "styrkjatorg/styrkir"
# Check for potential hardcoded routes in components
echo "Checking for route usage in components..."
ast-grep --pattern 'href="*/styrkjatorg/leit"'
Length of output: 392
Script:
#!/bin/bash
# Check for any dynamic route construction or references
echo "Checking for dynamic route construction..."
rg -A 2 "styrkjatorg" --type ts --type tsx
# Check for potential test files that might need updates
echo "Checking test files..."
fd -e test.ts -e test.tsx -e spec.ts -e spec.tsx -x rg -l "styrkjatorg"
# Check for potential configuration files
echo "Checking configuration files..."
fd -e config.ts -e config.js -x rg -l "styrkjatorg"
# Check for any router.push or similar navigation calls
echo "Checking navigation calls..."
ast-grep --pattern 'router.push($$$"/styrkjatorg/$$$")'
Length of output: 542
Script:
#!/bin/bash
# Check for any route references with proper file type
echo "Checking route references..."
rg -A 2 "styrkjatorg" .
# Check for any Link components using this route
echo "Checking Link components..."
ast-grep --pattern 'Link $$$href$$$styrkjatorg$$$'
# Check for any navigation methods
echo "Checking navigation methods..."
rg "router\.(push|replace)" -A 2
Length of output: 80985
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16985 +/- ##
==========================================
- Coverage 35.62% 35.61% -0.01%
==========================================
Files 6914 6915 +1
Lines 146001 146011 +10
Branches 41451 41456 +5
==========================================
- Hits 52011 52009 -2
- Misses 93990 94002 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 11 Total Test Services: 0 Failed, 10 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (4) |
thorkellmani
added
the
deprecated:automerge
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
libs/cms/src/lib/models/grant.model.ts (1)
33-34: Add documentation for the new fieldPlease add JSDoc comments explaining the purpose and possible values of
applicationDeadlineStatus.+/** + * Status text describing the application deadline state + * @example "Open until March 1st, 2024" + */ @Field({ nullable: true }) applicationDeadlineStatus?: stringapps/web/screens/Grants/SearchResults/SearchResultsFilter.tsx (2)
76-82: Consider making the status filters more maintainableWhile using
GrantStatusenum values is an improvement, the implementation could be more maintainable.Consider this more maintainable approach:
-filters: [ - { - value: GrantStatus.Open.toString().toLowerCase(), - label: formatMessage(m.search.applicationOpen), - }, - { - value: GrantStatus.Closed.toString().toLowerCase(), - label: formatMessage(m.search.applicationClosed), - }, -], +filters: Object.values(GrantStatus).map((status) => ({ + value: status.toString().toLowerCase(), + label: formatMessage(m.search[`application${status}`]), +})),This approach:
- Automatically handles new status values
- Reduces duplicate code
- Maintains type safety
Line range hint
22-31: Consider using React Query for filter state managementThe current implementation manages filter state through props and callbacks. For better scalability and performance, consider using React Query:
- Enables server-state synchronization
- Provides automatic background updates
- Handles caching and loading states
Example implementation:
const { data: filterState, mutate } = useQuery(['filterState'], () => fetchFilterState(), { initialData: searchState } ); const handleFilterUpdate = async (categoryId: keyof SearchState, value: unknown) => { await mutate({ ...filterState, [categoryId]: value }); };Also applies to: 45-52
apps/web/screens/Grants/SearchResults/SearchResults.tsx (3)
Line range hint
128-150: Enhance error handling for better user experienceWhile the error handling is functional, consider these improvements for production-grade error management:
- Replace console.error with a proper error logging service
- Add error state management to display user-friendly error messages
- Implement an error boundary to gracefully handle rendering failures
const [grants, setGrants] = useState<Array<Grant>>(initialGrants ?? []) +const [error, setError] = useState<Error | null>(null) + const fetchGrants = useCallback(() => { if (initialRender) { setInitialRender(false) return } getGrants({ variables: { input: { // ... existing input }, }, }) .then((res) => { if (res.data) { setGrants(res.data.getGrants.items) + setError(null) } else if (res.error) { setGrants([]) - console.error('Error fetching grants', res.error) + setError(res.error) + errorLoggingService.log('Error fetching grants', res.error) } }) .catch((err) => { setGrants([]) - console.error('Error fetching grants', err) + setError(err) + errorLoggingService.log('Error fetching grants', err) }) }
361-368: Consider enhancing the filterArray utilityThe generic utility function is well-typed but could be improved:
- Move it to a shared utility file for reuse across components
- Add input validation for array elements
- Consider adding JSDoc documentation
Consider moving this to a shared utility file (
utils/array.ts):/** * Filters out empty or null arrays, returning undefined instead * @param array The input array to filter * @returns The original array if non-empty, undefined otherwise */ export const filterArray = <T extends string | number>( array: Array<T> | null | undefined, validate?: (item: T) => boolean ): Array<T> | undefined => { if (!array?.length) { return undefined; } if (validate) { const filtered = array.filter(validate); return filtered.length ? filtered : undefined; } return array; };
384-393: Optimize query parameter handlingWhile the implementation is type-safe, consider reducing repetition:
+const parseQueryArray = (param: unknown) => + filterArray<string>(arrayParser.parseServerSide(param)) + input: { lang: locale as ContentLanguage, page: parseAsInteger.withDefault(1).parseServerSide(query?.page), search: parseAsString.parseServerSide(query?.query) ?? undefined, - categories: filterArray<string>( - arrayParser.parseServerSide(query?.category), - ), - statuses: filterArray<string>( - arrayParser.parseServerSide(query?.status), - ), - types: filterArray<string>(arrayParser.parseServerSide(query?.type)), - organizations: filterArray<string>( - arrayParser.parseServerSide(query?.organization), - ), + categories: parseQueryArray(query?.category), + statuses: parseQueryArray(query?.status), + types: parseQueryArray(query?.type), + organizations: parseQueryArray(query?.organization), }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
libs/cms/src/lib/generated/contentfulTypes.d.tsis excluded by!**/generated/**
📒 Files selected for processing (10)
apps/web/screens/Grants/Grant/GrantSidebar.tsx(4 hunks)apps/web/screens/Grants/Home/GrantsHome.tsx(1 hunks)apps/web/screens/Grants/SearchResults/SearchResults.tsx(3 hunks)apps/web/screens/Grants/SearchResults/SearchResultsContent.tsx(4 hunks)apps/web/screens/Grants/SearchResults/SearchResultsFilter.tsx(3 hunks)apps/web/screens/Grants/messages.ts(2 hunks)libs/cms/src/lib/cms.elasticsearch.service.ts(2 hunks)libs/cms/src/lib/dto/getGrants.input.ts(1 hunks)libs/cms/src/lib/models/grant.model.ts(4 hunks)libs/cms/src/lib/search/importers/grants.service.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- libs/cms/src/lib/dto/getGrants.input.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/web/screens/Grants/Grant/GrantSidebar.tsx
- apps/web/screens/Grants/Home/GrantsHome.tsx
- apps/web/screens/Grants/SearchResults/SearchResultsContent.tsx
- apps/web/screens/Grants/messages.ts
- libs/cms/src/lib/cms.elasticsearch.service.ts
- libs/cms/src/lib/search/importers/grants.service.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/web/screens/Grants/SearchResults/SearchResults.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/Grants/SearchResults/SearchResultsFilter.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."
libs/cms/src/lib/models/grant.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."
🔇 Additional comments (3)
libs/cms/src/lib/models/grant.model.ts (1)
Line range hint 1-124: Code follows library guidelines
The implementation properly follows the library coding guidelines:
- ✓ Exports reusable models and types
- ✓ Uses TypeScript with proper type definitions
- ✓ Follows proper tree-shaking practices with named exports
apps/web/screens/Grants/SearchResults/SearchResultsFilter.tsx (2)
10-10: LGTM! Good use of GraphQL schema types
The addition of GrantStatus import from the GraphQL schema improves type safety and maintainability by using centralized enum values instead of hardcoded strings.
114-116: Verify the impact of organization filter changes
The organization value has been changed and some organizations have been removed from the filters.
Let's verify the impact of these changes:
✅ Verification successful
Let me gather more information to verify the impact comprehensively.
Based on the gathered information, I can now provide the final response:
Organization filter changes appear safe to proceed
The changes to organization filters are localized to the search results component and properly reflected in the organization cache. The codebase shows that:
- Organization filtering is handled generically through filter arrays and query parameters
- The new organization value 'rannsoknamidstoed-islands-rannis' is properly defined in the cache
- No hard-coded references to the removed organizations were found in the TypeScript files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for references to old organization values
rg -l "tonlistarmidstod|kvikmyndastod" --type ts --type tsx
# Check for references to the new organization value
rg "rannsoknamidstoed-islands-rannis" -A 2 -B 2
Length of output: 1011
Script:
#!/bin/bash
# Search for any references to organizations in TypeScript files
rg -t ts "organization.*filter" -A 2 -B 2
# Check for any GraphQL queries or types related to organizations
rg -t graphql -t ts "organization.*type|organization.*query" -A 2 -B 2
# Look for organization-related test files
fd "organization|grant" --type f --extension test.ts --extension test.tsx --extension spec.ts --extension spec.tsx
Length of output: 2869
* fix: ini * fix: change application deadlineg * feat: change routing * fix: missing crumb * fix: status dynamic message * fix: multiple stuff * fix: null check --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
What
leit->styrkirWhy
Checklist:
Summary by CodeRabbit
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Documentation