fix(ojoi): Updated controllers and bugfixes #17146
Conversation
…ted a previous advert the inputs fields were not updating. Switched out controller components since we have our own.
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request includes significant updates across several components in the official journal application. Key changes involve the replacement of the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
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
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17146 +/- ##
=======================================
Coverage 35.75% 35.75%
=======================================
Files 6925 6925
Lines 147603 147603
Branches 42022 42022
=======================================
Hits 52772 52772
Misses 94831 94831
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 ✅ 4 Total Test Services: 0 Failed, 4 Passed Test Services
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (15)
libs/application/templates/official-journal-of-iceland/src/fields/Preview.tsx (2)
Line range hint
58-74: LGTM! Consider extracting download name generation logic.The type-safe access pattern and URL cleanup in the finally block are well implemented. However, the download name generation logic could be extracted into a separate function for better readability and reusability.
Consider refactoring like this:
+ const getDownloadName = (type?: string, title?: string, fallbackId?: string) => { + let downloadName = type ? type.replace('.', '') : ''; + if (title) { + downloadName += ` ${title}`; + } + return downloadName || `Innsending ${fallbackId}`; + }; - let downloadName - const type = currentApplication.answers.advert?.type?.title - if (type) { - downloadName = type.replace('.', '') - } - - const title = currentApplication.answers.advert?.title - if (title) { - downloadName += ` ${title}` - } - - if (!downloadName) { - downloadName = `Innsending ${application.id}` - } - - downloadName += '.pdf' + const downloadName = `${getDownloadName( + currentApplication.answers.advert?.type?.title, + currentApplication.answers.advert?.title, + application.id + )}.pdf`;
Line range hint
1-224: Consider enhancing error handling and testing coverage.As this is a library component, consider the following architectural improvements:
- Add error boundaries to handle PDF generation failures gracefully
- Extract PDF download logic into a custom hook for reusability
- Add unit tests for the new type structure handling
Would you like me to help create:
- An error boundary implementation
- A reusable PDF download hook
- Unit test templates for the component?
libs/application/templates/official-journal-of-iceland/src/lib/OJOIApplication.ts (1)
Line range hint
1-44: Enhance type exports for reusabilityFollowing the coding guidelines for
libs/**/*, consider exporting the type structure forInputFields.advert.typeto improve reusability across different NextJS apps.Consider adding type exports like this:
export interface AdvertType { title: string; // other type properties } export interface AdvertFields { type: AdvertType; title: string; // other advert fields }libs/application/templates/official-journal-of-iceland/src/fields/Summary.tsx (4)
72-74: Improve optional chaining consistencyThe cleanup function uses
&&operator while the rest of the code uses optional chaining. Consider using optional chaining for consistency.- setSubmitButtonDisabled && setSubmitButtonDisabled(false) + setSubmitButtonDisabled?.setSubmitButtonDisabled(false)🧰 Tools
🪛 Biome (1.9.4)
[error] 73-73: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Line range hint
61-70: Consider optimizing the effect dependenciesThe effect's logic could be simplified by combining the validation checks into a single derived value. This would reduce the number of effect dependencies and make the code more maintainable.
+ const isValid = advertValidationCheck.success && + signatureValidationCheck.success && + publishingCheck.success; useEffect(() => { - if ( - advertValidationCheck.success && - signatureValidationCheck.success && - publishingCheck.success - ) { - setSubmitButtonDisabled && setSubmitButtonDisabled(false) - } else { - setSubmitButtonDisabled && setSubmitButtonDisabled(true) - } + setSubmitButtonDisabled?.(!isValid) return () => { - setSubmitButtonDisabled && setSubmitButtonDisabled(false) + setSubmitButtonDisabled?.(false) } - }, [ - advertValidationCheck, - signatureValidationCheck, - publishingCheck, - setSubmitButtonDisabled, - ]) + }, [isValid, setSubmitButtonDisabled])🧰 Tools
🪛 Biome (1.9.4)
[error] 69-69: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 73-73: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Line range hint
89-185: Reduce validation message duplicationThe validation message rendering logic is repeated three times with similar patterns. Consider extracting this into a reusable component to improve maintainability.
Example implementation:
type ValidationMessageProps = { validationResult: SafeParseReturnType<any, any>; title: string; route: Routes; sectionName: string; goToScreen?: (route: Routes) => void; setSubmitButtonDisabled?: (disabled: boolean) => void; }; const ValidationMessage = ({ validationResult, title, route, sectionName, goToScreen, setSubmitButtonDisabled, }: ValidationMessageProps) => { if (validationResult.success) return null; return ( <AlertMessage type="warning" title={title} message={ <Stack space={2}> <BulletList color="black"> {validationResult.error.issues.map((issue) => { const parsedIssue = parseZodIssue(issue as ZodCustomIssue) return ( <Bullet key={issue.path.join('.')}> {f(parsedIssue.message)} </Bullet> ) })} </BulletList> <Button onClick={() => { setSubmitButtonDisabled?.(false) goToScreen?.(route) }} size="small" variant="text" preTextIcon="arrowBack" > {`Opna kafla ${sectionName}`} </Button> </Stack> } /> ); };
Line range hint
30-40: Consider improving component reusabilityAs this component is part of a library, consider:
- Extracting validation logic into a custom hook
- Making the component more configurable through props
- Documenting the component's API and usage patterns
Would you like assistance in implementing these architectural improvements?
libs/application/templates/official-journal-of-iceland/src/hooks/useTypes.ts (2)
70-70: Typo: Correct 'initalTypes' to 'initialTypes'There is a typographical error in the return object. The key
initalTypesshould be corrected toinitialTypesto ensure consistency and avoid confusion.Apply this diff to fix the typo:
- initalTypes: data?.officialJournalOfIcelandTypes.types, + initialTypes: data?.officialJournalOfIcelandTypes.types,
71-73: Improve error handling by combining 'error' and 'lazyTypesError'Currently, only the
errorfrom the initialuseQueryis returned, whilelazyTypesErroris returned separately. For consistent error handling, consider combining both errors or handling them appropriately to ensure errors from both queries are accounted for.Apply this diff to adjust the error handling:
- loading: loading || lazyTypesLoading, - error, + loading: loading || lazyTypesLoading, + error: error || lazyTypesError,libs/application/templates/official-journal-of-iceland/src/components/input/OJOISelectController.tsx (2)
57-57: Simplify function call with optional chainingYou can simplify the conditional function call by using optional chaining for better readability.
Apply this diff:
- onBeforeChange && onBeforeChange(newAnswers, value) + onBeforeChange?.(newAnswers, value)🧰 Tools
🪛 Biome (1.9.4)
[error] 57-57: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
62-62: Simplify function call with optional chainingYou can simplify the conditional function call by using optional chaining for better readability.
Apply this diff:
- onChange && onChange(value) + onChange?.(value)🧰 Tools
🪛 Biome (1.9.4)
[error] 62-62: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
libs/application/templates/official-journal-of-iceland/src/screens/AdvertScreen.tsx (1)
38-38: Simplify function call with optional chainingYou can use optional chaining to simplify the conditional function call to
props.refetch.Apply this diff:
- props.refetch && props.refetch() + props.refetch?.()🧰 Tools
🪛 Biome (1.9.4)
[error] 38-38: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
libs/application/templates/official-journal-of-iceland/src/fields/Publishing.tsx (2)
Line range hint
58-69: Consider performance optimization for category updatesWhile the type safety improvements with Zod are excellent, using
structuredClonefor the entire answers object might be inefficient for large state trees.Consider using a more targeted update approach:
-const currentAnswers = structuredClone(currentApplication.answers) -const selectedCategories = currentAnswers.advert?.categories || [] +const selectedCategories = [...(currentApplication.answers.advert?.categories || [])]
127-142: UI improvement with better accessibilityGood improvement using
Inlinecomponent for consistent spacing andTagcomponents for better interaction. However, consider adding aria-label for better accessibility.<Tag disabled={isUpdatingCategory} onClick={() => onCategoryChange(c)} outlined key={c.id} + aria-label={`Remove category ${c.title}`} >libs/application/templates/official-journal-of-iceland/src/lib/dataSchema.ts (1)
145-155: Consider extracting shared validation logic.The validation logic is duplicated between
advertValidationSchemaandpreviewValidationSchema. Consider extracting the common validation rules into a shared function or schema.// Create a shared validation schema const sharedEntityValidation = (errorMessage: MessageDescriptor) => baseEntitySchema .optional() .nullable() .refine((value) => value !== null && value !== undefined, { params: errorMessage, }); // Use in both schemas const advertValidationSchema = z.object({ advert: z.object({ department: sharedEntityValidation(error.missingDepartment), type: sharedEntityValidation(error.missingType), // ... rest of the schema }), });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (13)
libs/application/templates/official-journal-of-iceland/src/components/input/OJOIInputController.tsx(2 hunks)libs/application/templates/official-journal-of-iceland/src/components/input/OJOISelectController.tsx(3 hunks)libs/application/templates/official-journal-of-iceland/src/fields/Advert.tsx(1 hunks)libs/application/templates/official-journal-of-iceland/src/fields/AdvertModal.tsx(4 hunks)libs/application/templates/official-journal-of-iceland/src/fields/Preview.tsx(2 hunks)libs/application/templates/official-journal-of-iceland/src/fields/Publishing.tsx(6 hunks)libs/application/templates/official-journal-of-iceland/src/fields/Summary.tsx(5 hunks)libs/application/templates/official-journal-of-iceland/src/hooks/useTypes.ts(2 hunks)libs/application/templates/official-journal-of-iceland/src/lib/OJOIApplication.ts(1 hunks)libs/application/templates/official-journal-of-iceland/src/lib/dataSchema.ts(5 hunks)libs/application/templates/official-journal-of-iceland/src/lib/types.ts(2 hunks)libs/application/templates/official-journal-of-iceland/src/lib/utils.ts(2 hunks)libs/application/templates/official-journal-of-iceland/src/screens/AdvertScreen.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (13)
libs/application/templates/official-journal-of-iceland/src/lib/utils.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/application/templates/official-journal-of-iceland/src/lib/OJOIApplication.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/application/templates/official-journal-of-iceland/src/fields/Summary.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."
libs/application/templates/official-journal-of-iceland/src/fields/Preview.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."
libs/application/templates/official-journal-of-iceland/src/lib/types.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/application/templates/official-journal-of-iceland/src/screens/AdvertScreen.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."
libs/application/templates/official-journal-of-iceland/src/hooks/useTypes.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/application/templates/official-journal-of-iceland/src/fields/Publishing.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."
libs/application/templates/official-journal-of-iceland/src/fields/AdvertModal.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."
libs/application/templates/official-journal-of-iceland/src/components/input/OJOIInputController.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."
libs/application/templates/official-journal-of-iceland/src/fields/Advert.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."
libs/application/templates/official-journal-of-iceland/src/components/input/OJOISelectController.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."
libs/application/templates/official-journal-of-iceland/src/lib/dataSchema.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."
🪛 Biome (1.9.4)
libs/application/templates/official-journal-of-iceland/src/fields/Summary.tsx
[error] 73-73: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
libs/application/templates/official-journal-of-iceland/src/screens/AdvertScreen.tsx
[error] 38-38: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
libs/application/templates/official-journal-of-iceland/src/components/input/OJOISelectController.tsx
[error] 57-57: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 62-62: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (15)
libs/application/templates/official-journal-of-iceland/src/fields/Preview.tsx (1)
101-108: LGTM! Verify markup generation with the new type structure.
The type-safe access pattern is consistently applied. Let's verify that the markup generation works correctly with the new nested type structure.
✅ Verification successful
Type-safe access pattern is consistently implemented across markup generation
The verification shows that the new type structure (advert?.type?.title) is consistently used across all relevant files:
- Preview.tsx: Used in markup generation, hasMarkup condition, and download name logic
- Advert.tsx: Used in title preview markup generation
- Summary.tsx: Used in property display
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any other instances of markup generation to ensure consistency
# Search for other markup generation patterns
ast-grep --pattern 'getAdvertMarkup({
$$$
})'
# Search for type access patterns to ensure consistency
rg -A 2 'advert\?\.type\?\.title'
Length of output: 2961
libs/application/templates/official-journal-of-iceland/src/lib/OJOIApplication.ts (1)
44-44: Verify type safety and data structure changes
The path change from InputFields.advert.typeName to ${InputFields.advert.type}.title suggests a structural change in how advert types are stored. This change needs verification to ensure type safety and proper data handling.
✅ Verification successful
Change from typeName to type.title is valid and consistent
The verification shows that:
- In
types.ts,InputFields.advert.typeis correctly defined as'advert.type' - The usage of
type.titleis consistent with the API schema, as seen inOJOIAdvert.tsxwhereadvert.type.titleis used - No remaining usages of the old
typeNamepattern were found in the OJOI application context
The change aligns with the codebase structure and appears to be a valid refactoring to match the API schema's structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the type structure and usage across the codebase
# Check the InputFields type definition
ast-grep --pattern 'interface InputFields {
advert: {
$$$
type: $_
$$$
}
}'
# Check for any remaining uses of the old path
rg "advert\.typeName" -t ts -t tsx
# Check for consistent usage of the new path structure
rg "advert\.type\.title" -t ts -t tsx
Length of output: 226
Script:
#!/bin/bash
# Let's try a different approach to verify the type structure and usage
# Search for InputFields interface definition
ast-grep --pattern 'interface InputFields {
$$$
}'
# Check for type-related imports and definitions
rg "InputFields" --type ts
# Look for advert type-related patterns
rg "advert\.type" --type ts
# Check for any remaining uses of typeName
rg "typeName" --type ts
Length of output: 19642
Script:
#!/bin/bash
# Let's check the specific type definition and structure
# Check the types.ts file content
cat libs/application/templates/official-journal-of-iceland/src/lib/types.ts
# Look for any references to advert.type.title
rg "advert\.type\.title" --type ts -A 2 -B 2
Length of output: 2999
libs/application/templates/official-journal-of-iceland/src/fields/Summary.tsx (1)
41-45: Verify if price loading state is still necessary
While other loading states have been removed, loadingPrice is still being used. Consider if this loading state is necessary or if it can be simplified similar to other data fetching in this component.
✅ Verification successful
Let me search for more context about the usePrice hook and its usage pattern.
Loading state for price is correctly implemented and necessary
The price loading state is required as it's actively used in the Property component to show loading state while fetching price data via GraphQL. The implementation is consistent with GraphQL data fetching patterns where:
- The usePrice hook performs a GraphQL query that may take time to complete
- The loading state is properly propagated to the UI through the Property component
- The price calculation requires a server round-trip, making it different from other removed loading states that might have been client-side only
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of usePrice hook to understand the loading pattern
ast-grep --pattern 'const { price, loading: $_ } = usePrice($$$)'
Length of output: 404
Script:
#!/bin/bash
# Search for usePrice hook definition and implementation
ast-grep --pattern 'export const usePrice = ($$$) => {
$$$
}'
# Search for other price loading usages in the component
rg "loadingPrice" libs/application/templates/official-journal-of-iceland/src/fields/Summary.tsx -A 2 -B 2
Length of output: 2162
libs/application/templates/official-journal-of-iceland/src/fields/AdvertModal.tsx (1)
Line range hint 28-198: Code changes effectively simplify state management
The modifications improve state handling by directly storing the selected advert object. The logic is clear, and the implementation enhances the component's efficiency.
🧰 Tools
🪛 Biome (1.9.4)
[error] 108-108: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
libs/application/templates/official-journal-of-iceland/src/components/input/OJOIInputController.tsx (1)
Line range hint 1-82: Refactor to use 'Input' component improves code simplicity
The replacement of InputController with the Input component simplifies the code and aligns with updated UI component usage. The changes are appropriate and well-implemented.
libs/application/templates/official-journal-of-iceland/src/lib/types.ts (2)
41-42: LGTM! Field labels updated consistently
The human-readable labels have been updated to match the new field names while maintaining the same meaning.
14-15: Verify the impact of field name changes across the codebase
The renaming from departmentId/typeId to department/type suggests a shift in data structure. While the changes look good, we should verify the impact.
libs/application/templates/official-journal-of-iceland/src/fields/Advert.tsx (3)
17-21: Props simplified and application state management improved
Good improvement in component props by removing unused timeStamp. The useApplication hook usage is now more focused.
36-43: Type-safe department options mapping
Good implementation of type-safe options with complete department entity information.
66-79: Verify proper cleanup of dependent fields
The cleanup of the type field when department changes is good, but we should verify the handling of other dependent fields.
libs/application/templates/official-journal-of-iceland/src/lib/dataSchema.ts (4)
46-50: LGTM! Well-structured base entity schema.
The schema provides a solid foundation for entity validation with required id, title, and slug fields.
70-71: LGTM! Enhanced type safety with baseEntitySchema.
The changes improve type safety by replacing string IDs with structured entities, while maintaining flexibility with optional and nullable fields where appropriate.
Also applies to: 75-75
116-127: LGTM! Robust validation with proper null checks.
The validation logic properly handles null and undefined cases while maintaining consistent error messaging.
181-184: LGTM! Consistent schema usage.
The categories validation properly uses the baseEntitySchema while maintaining the existing refinement logic.
libs/application/templates/official-journal-of-iceland/src/lib/utils.ts (1)
134-137: LGTM! Well-implemented type guard.
The type guard follows TypeScript best practices and maintains consistency with other type guards in the file.
libs/application/templates/official-journal-of-iceland/src/fields/Summary.tsx
Show resolved
Hide resolved
...lication/templates/official-journal-of-iceland/src/components/input/OJOISelectController.tsx
Outdated
Show resolved
Hide resolved
* Updated data schema to make less api calls, fixed bug when user selected a previous advert the inputs fields were not updating. Switched out controller components since we have our own. * Returning false instead of undefined. --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
What
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Documentation