feat(rental-agreement): Project structure & minimal functionality #16214
Conversation
This reverts commit fda88fb.
…sland.is into rental-agreement-application
…ardcoded text to message translation text.
WalkthroughThis pull request adds support for a new rental agreement application type. It extends the application template loader to dynamically import the rental agreement template, updates the application types enum and configuration, and introduces a dedicated rental agreement module. New enums, a helper function, a Zod data schema for validation, and internationalized messages for housing condition inspection have been added. Additionally, the module’s exports are consolidated in an index file, and a new CODEOWNERS entry is established. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant AL as Application Loader
participant RA as Rental Agreement Module
participant DS as Data Schema Validator
participant IM as Intl Messages
U->>AL: Request rental agreement application form
AL->>RA: Dynamically import RENTAL_AGREEMENT template
RA->>DS: Validate application data using Zod schema
RA->>IM: Retrieve internationalized housing condition messages
DS-->>RA: Return validated data
RA-->>AL: Render and return filled application template
Suggested labels
Suggested reviewers
✨ Finishing Touches
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
|
Datadog ReportAll test runs ✅ 10 Total Test Services: 0 Failed, 10 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (1)
|
addi
changed the title
…idations (#17519) * Add validation in dataSchema for fire protection inputs. * Add validation in dataSchema for other fees. * Fix dataschema name for signatureInfo.
* Add validation in dataSchema for fire protection inputs. * Add validation in dataSchema for other fees. * Add validation for inputs based on payment method chosen. * Update README.md with info about localization in Contentful. * Update summary for payment method and format outputs.
Datadog ReportAll test runs ✅ 10 Total Test Services: 0 Failed, 10 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (1)
|
|
View your CI Pipeline Execution ↗ for commit c9cb5ec.
☁️ Nx Cloud last updated this comment at |
* Add property search section and custom component * Update property search mock data options. * Update custom search component. Move data to separate file. * Update mock data, render search results and update info in summary. * Remove console logs. * Remove unused definitions * Replace fetch api(localhost) with local json file. * Change data from .json to .ts for import.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/CODEOWNERS(2 hunks)
🔇 Additional comments (1)
.github/CODEOWNERS (1)
131-132: LGTM!The ownership entry for the rental agreement module is properly formatted and assigned to the correct team.
.github/CODEOWNERS
Outdated
| @@ -1,3 +1,5 @@ | |||
| import { application } from "express" | |||
There was a problem hiding this comment.
Remove the incorrect import statement.
The CODEOWNERS file is a GitHub configuration file and does not support or require JavaScript/TypeScript imports.
-import { application } from "express"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { application } from "express" |
…18107) * Add validation in dataSchema for fire protection inputs. * Add validation in dataSchema for other fees. * Add validation for inputs based on payment method chosen. * Update README.md with info about localization in Contentful. * Update and add states. Add landlords and tenants as assignees. * Update states and template flow. Cleanup and rename files. * Update * fix .github codeowners * Filter assignees. Fix change button not appearig in summary. * Fix lint warnings * Refetch application after submit on submitFields where required.
* Add validation in dataSchema for fire protection inputs. * Add validation in dataSchema for other fees. * Add validation for inputs based on payment method chosen. * Update README.md with info about localization in Contentful. * Update and add states. Add landlords and tenants as assignees. * Update states and template flow. Cleanup and rename files. * Update * fix .github codeowners * Update property search with new data structure. Add table with property search results. * Filter assignees. Fix change button not appearig in summary. * Fix lint warnings * Fix unintentional moved folder. Delete unused data files. * Update property search results. Add clearOnChange for some radio/checkbox/select forms. * Remove unused data/schema, other minor lint fixes.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
libs/application/templates/rental-agreement/src/lib/utils.ts (3)
28-29: 🛠️ Refactor suggestionConsider handling edge cases in the
insertAtfunction.The current implementation doesn't handle edge cases such as negative positions or positions greater than the string length. This could lead to unexpected behavior.
-export const insertAt = (str: string, sub: string, pos: number) => - `${str.slice(0, pos)}${sub}${str.slice(pos)}` +export const insertAt = (str: string, sub: string, pos: number) => { + if (pos < 0 || pos > str.length) { + return str; // or throw an error, depending on your use case + } + return `${str.slice(0, pos)}${sub}${str.slice(pos)}`; +}
31-32: 🛠️ Refactor suggestionImprove error handling in
formatNationalIdfunction.The current implementation returns '-' for invalid inputs, which might mask errors. Consider throwing an error for invalid inputs or returning the original string.
-export const formatNationalId = (nationalId: string) => - insertAt(nationalId.replace('-', ''), '-', 6) || '-' +export const formatNationalId = (nationalId: string) => { + const cleaned = nationalId.replace('-', ''); + if (cleaned.length !== 10 || !/^\d+$/.test(cleaned)) { + throw new Error('Invalid national ID format'); + } + return insertAt(cleaned, '-', 6); +}
56-108: 🛠️ Refactor suggestionUse type guards instead of type assertions in
getApplicationAnswers.Type assertions might be unsafe if the retrieved values don't match the expected types. Consider using type guards for better type safety.
You could implement a simple type guard function to validate the enum values:
function isEnumValue<T extends Record<string, string | number>>( value: unknown, enumObj: T ): value is T[keyof T] { return Object.values(enumObj).includes(value as T[keyof T]); }Then validate each value before returning:
if (propertyTypeOptions && !isEnumValue(propertyTypeOptions, RentalHousingCategoryTypes)) { throw new Error('Invalid property type option'); } // Repeat for other enum values
🧹 Nitpick comments (31)
libs/application/templates/rental-agreement/src/assets/IconCircleClose.tsx (1)
1-24: Enhance accessibility and reusability of the SVG icon component.The component is well-structured but could benefit from the following improvements:
- Add accessibility attributes to the SVG:
<svg height="24px" width="24px" viewBox="0 0 24 24" fill="none" xmlns="http://www.w3.org/2000/svg" + aria-hidden="true" + role="img" + aria-label="Close" >
- Consider making the component more reusable by accepting color props:
-const IconCircleClose: FC<React.PropsWithChildren<unknown>> = () => ( +interface IconCircleCloseProps { + circleColor?: string; + lineColor?: string; +} + +const IconCircleClose: FC<IconCircleCloseProps> = ({ + circleColor = "#00E4CA", + lineColor = "#00003C" +}) => ( <svg> <path d="M0 12C0 5.37258 5.37258 0 12 0C18.6274 0 24 5.37258 24 12C24 18.6274 18.6274 24 12 24C5.37258 24 0 18.6274 0 12Z" - fill="#00E4CA" + fill={circleColor} /> <path fillRule="evenodd" clipRule="evenodd" d="M7 12C7 11.7239 7.22386 11.5 7.5 11.5H16.5C16.7761 11.5 17 11.7239 17 12C17 12.2761 16.7761 12.5 16.5 12.5H7.5C7.22386 12.5 7 12.2761 7 12Z" - fill="#00003C" + fill={lineColor} />
- Since you're not using children, use a more appropriate type:
-const IconCircleClose: FC<React.PropsWithChildren<unknown>> = () => ( +const IconCircleClose: FC<IconCircleCloseProps> = ({libs/application/templates/rental-agreement/src/forms/rentalPeriod/rentalPeriodDetails.ts (1)
13-20: Improve null checking with optional chaining.The helper function can be improved by using optional chaining as suggested by static analysis:
const rentalPeriodIsDefinite = (answers: FormValue) => { const rentalPeriodDefinite = getValueViaPath( answers, 'rentalPeriod.isDefinite', [], ) as string[] - return rentalPeriodDefinite && rentalPeriodDefinite.includes(TRUE) + return rentalPeriodDefinite?.includes(TRUE) ?? false }This change makes the code more concise and follows modern TypeScript best practices for null/undefined handling.
🧰 Tools
🪛 Biome (1.9.4)
[error] 19-19: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
libs/application/templates/rental-agreement/src/forms/rentalHousing/rentalHousingCondition.ts (1)
69-70: Consider defining file size limits for uploads.While you've specified accepted file types, adding size limits would prevent excessively large uploads and improve user experience.
uploadAccept: '.pdf, .doc, .docx, .rtf, .jpg, .jpeg, .png, .heic', uploadMultiple: true, + maxSize: 10, // Size in MB forImageUpload: true,libs/application/templates/rental-agreement/src/fields/Summary/PropertyInfoSummary.tsx (1)
28-38: Consider grouping route props for better maintainability.The Props interface has several route-related properties. Consider grouping them into a nested object for better organization and maintainability.
interface Props extends FieldBaseProps { goToScreen?: (id: string) => void - categoryRoute?: Routes - propertyInfoRoute?: Routes - propertyDescriptionRoute?: Routes - specialProvisionsRoute?: Routes - propertyConditionRoute?: Routes - fileUploadRoute?: Routes - fireProtectionsRoute?: Routes + routes?: { + category?: Routes + propertyInfo?: Routes + propertyDescription?: Routes + specialProvisions?: Routes + propertyCondition?: Routes + fileUpload?: Routes + fireProtections?: Routes + } hasChangeButton: boolean }libs/application/templates/rental-agreement/src/lib/messages/housing/registerProperty.ts (1)
51-56: Message ID inconsistency in error section.The error message ID contains "category" in its path, but it's located in the "info" section of the object structure.
// Error messages searchResultsEmptyError: { - id: 'ra.application:registerProperty.category.searchResultsEmptyError', + id: 'ra.application:registerProperty.info.searchResultsEmptyError', defaultMessage: 'Skráning leiguhúsnæðis þarf að vera til staðar til að halda áfram', description: 'Error message when no search results', },libs/application/templates/rental-agreement/src/fields/Summary/RentalInfoSummary.tsx (2)
50-55: Simplify security deposit condition variables.Three similar variables are defined to check security deposit conditions. Consider consolidating these into a single helper object or function for better maintainability.
- const isSecurityDepositRequired = - answers.rentalAmount.isPaymentInsuranceRequired?.includes(AnswerOptions.YES) - const isSecurityDepositAmount = - answers.securityDeposit?.securityAmount || - answers.securityDeposit?.securityAmountOther - const isSecurityDepositType = answers.securityDeposit?.securityType + const securityDeposit = { + isRequired: answers.rentalAmount.isPaymentInsuranceRequired?.includes(AnswerOptions.YES), + hasAmount: answers.securityDeposit?.securityAmount || answers.securityDeposit?.securityAmountOther, + hasType: !!answers.securityDeposit?.securityType + }Then update references like:
- {isSecurityDepositRequired && ( + {securityDeposit.isRequired && (
226-265: Refactor repetitive conditional rendering for security deposit types.The code contains multiple similar conditional blocks for different security deposit types. Consider extracting this into a mapping function for better maintainability.
- {answers.securityDeposit.securityType === - SecurityDepositTypeOptions.BANK_GUARANTEE && ( - <KeyValue - label={summary.securityTypeInstitutionLabel} - value={answers.securityDeposit.bankGuaranteeInfo || '-'} - /> - )} - {answers.securityDeposit.securityType === - SecurityDepositTypeOptions.THIRD_PARTY_GUARANTEE && ( - <KeyValue - label={summary.securityTypeThirdPartyGuaranteeLabel} - value={answers.securityDeposit.thirdPartyGuaranteeInfo || '-'} - /> - )} - // ...more similar blocks + {(() => { + const typeConfig = { + [SecurityDepositTypeOptions.BANK_GUARANTEE]: { + label: summary.securityTypeInstitutionLabel, + value: answers.securityDeposit.bankGuaranteeInfo + }, + [SecurityDepositTypeOptions.THIRD_PARTY_GUARANTEE]: { + label: summary.securityTypeThirdPartyGuaranteeLabel, + value: answers.securityDeposit.thirdPartyGuaranteeInfo + }, + [SecurityDepositTypeOptions.INSURANCE_COMPANY]: { + label: summary.securityTypeInsuranceLabel, + value: answers.securityDeposit.insuranceCompanyInfo + }, + [SecurityDepositTypeOptions.LANDLORDS_MUTUAL_FUND]: { + label: summary.securityTypeMutualFundLabel, + value: answers.securityDeposit.mutualFundInfo + }, + [SecurityDepositTypeOptions.OTHER]: { + label: summary.securityTypeOtherLabel, + value: answers.securityDeposit.otherInfo + } + }; + + const config = typeConfig[answers.securityDeposit.securityType]; + return config ? ( + <KeyValue + label={config.label} + value={config.value || '-'} + /> + ) : null; + })()}libs/application/templates/rental-agreement/src/fields/PropertySearch/index.tsx (5)
76-97: Remove console.error in production codeError logging should use a structured logging system rather than direct console methods. This will help with debugging in production environments where console access might be limited.
- console.error('Error parsing stored value:', error) + // Consider using a structured logging library or error tracking service + console.error('Error parsing stored value:', error) + // TODO: Implement proper error handling/reporting
221-226: Use optional chaining for cleaner codeThe nested conditional checks for matseiningByFasteignNr can be simplified using optional chaining.
- const isChecked = - matseiningByFasteignNr && - matseiningByFasteignNr[propertyId]?.some((matseiningar) => - matseiningar.matseiningar.some( - (matseining) => checkedMatseiningar[matseining.merking] === true, - ), - ) + const isChecked = matseiningByFasteignNr?.[propertyId]?.some((matseiningar) => + matseiningar.matseiningar.some( + (matseining) => checkedMatseiningar[matseining.merking] === true, + ), + )🧰 Tools
🪛 Biome (1.9.4)
[error] 221-226: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
391-407: Use optional chaining for matseiningByFasteignNr checksSimplify the nested conditional checks with optional chaining for better readability and maintainability.
- {matseiningByFasteignNr && - matseiningByFasteignNr[property.fastnum] && - matseiningByFasteignNr[property.fastnum].length > 0 && ( + {matseiningByFasteignNr?.[property.fastnum]?.length > 0 && (🧰 Tools
🪛 Biome (1.9.4)
[error] 391-392: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
438-564: Simplify nested rendering logic with optional chaining and early returnsThe deeply nested conditional rendering with Fragments can be simplified for better readability and performance.
- {matseiningByFasteignNr && - matseiningByFasteignNr[property.fastnum] && - matseiningByFasteignNr[property.fastnum].length > 0 && ( - <> - {matseiningByFasteignNr[property.fastnum].map( - (matseiningar) => { + {matseiningByFasteignNr?.[property.fastnum]?.length > 0 && + matseiningByFasteignNr[property.fastnum].map( + (matseiningar) => {The Fragment (
<>...</>) is unnecessary here since you're already returning from a map function.🧰 Tools
🪛 Biome (1.9.4)
[error] 438-439: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 441-563: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
113-132: Consider extracting table expansion logic to a custom hookThe useEffect for handling table expansion states based on checked items could be extracted to a custom hook for better reusability and maintainability.
// Example custom hook function useTableExpansion(checkedItems, itemsData) { const [expandedRows, setExpandedRows] = useState({}); useEffect(() => { if (itemsData) { const newExpandedRows = Object.keys(itemsData).reduce( (acc, id) => { // Expansion logic return acc; }, {} ); setExpandedRows(newExpandedRows); } }, [checkedItems, itemsData]); return expandedRows; }libs/application/templates/rental-agreement/src/forms/rentalPeriod/rentalPeriodAmount.ts (2)
27-34: Use optional chaining for more concise codeThe code can be simplified with optional chaining to improve readability.
const rentalAmountConnectedToIndex = (answers: FormValue) => { const isAmountConnectedToIndex = getValueViaPath( answers, 'rentalAmount.isIndexConnected', [], ) as string[] - return isAmountConnectedToIndex && isAmountConnectedToIndex.includes(TRUE) + return isAmountConnectedToIndex?.includes(TRUE) ?? false }🧰 Tools
🪛 Biome (1.9.4)
[error] 33-33: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
154-161: Consider adding clearer labels for checkbox fieldsThe checkbox field for payment insurance has an empty title but uses the label from options. For better accessibility and UX, consider adding a descriptive title.
buildCheckboxField({ id: 'rentalAmount.isPaymentInsuranceRequired', - title: '', + title: rentalAmount.paymentInsuranceTitle, options: [ { value: AnswerOptions.YES, label: rentalAmount.paymentInsuranceRequiredLabel, }, ], }),libs/application/templates/rental-agreement/src/forms/rentalHousing/rentalHousingPropertyInfo.ts (1)
144-150: Simplify condition logicThe condition function can be simplified for better readability and maintainability.
condition: (answers) => { - const { propertyClassOptions } = getApplicationAnswers(answers) - return ( - propertyClassOptions === RentalHousingCategoryClass.SPECIAL_GROUPS - ) + return getApplicationAnswers(answers).propertyClassOptions === RentalHousingCategoryClass.SPECIAL_GROUPS },libs/application/templates/rental-agreement/src/lib/dataSchema.ts (5)
289-305: Use optional chaining for cleaner codeThe isDefiniteChecked condition can be simplified with optional chaining.
- const isDefiniteChecked = data.isDefinite && data.isDefinite.includes(TRUE) + const isDefiniteChecked = data.isDefinite?.includes(TRUE)🧰 Tools
🪛 Biome (1.9.4)
[error] 289-289: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
348-358: Use optional chaining for validation conditionsThe conditional check can be simplified with optional chaining.
if ( - data.paymentDateOptions && - data.paymentDateOptions.includes(RentalAmountPaymentDateOptions.OTHER) && + data.paymentDateOptions?.includes(RentalAmountPaymentDateOptions.OTHER) && !data.paymentDateOther?.trim().length )🧰 Tools
🪛 Biome (1.9.4)
[error] 348-349: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
363-403: Simplify conditional checks with optional chainingThe validation for paymentMethodNationalId and paymentMethodBankAccountNumber can be simplified.
- if ( - data.paymentMethodNationalId && - data.paymentMethodNationalId?.trim().length && - !kennitala.isValid(data.paymentMethodNationalId) - ) { + if ( + data.paymentMethodNationalId?.trim().length && + !kennitala.isValid(data.paymentMethodNationalId) + ) {Apply similar refactoring to other conditions in this section.
🧰 Tools
🪛 Biome (1.9.4)
[error] 372-373: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 392-393: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
438-452: Consider edge cases in property size validationThe property size validation doesn't handle the case where
propertySizemight be undefined or an empty string before the replacement operation. Consider adding a fallback.- const propertySizeString = data.propertySize?.replace(',', '.') || '' + const propertySizeString = data.propertySize ? data.propertySize.replace(',', '.') : '0'
27-33: Utilize built-in Math functions instead of custom checkThe
checkIfNegativefunction can be simplified using built-in Math functions or direct comparison.- const checkIfNegative = (inputNumber: string) => { - if (Number(inputNumber) < 0) { - return false - } else { - return true - } - } + const checkIfNegative = (inputNumber: string) => Number(inputNumber) >= 0libs/application/templates/rental-agreement/src/lib/types.ts (1)
13-66: Consolidate common property fields.
Multiple interfaces (StadfangProps,FasteignByStadfangNrProps, etc.) share fields likestadfang_nr,landeign_nr,postnumer. Consider extracting common fields into a shared base interface for easier maintainability and reusability.libs/application/templates/rental-agreement/src/fields/PropertySearch/propertyData.ts (3)
1-243: Avoid hard-coding large datasets in source code.
ThestadfangDataarray is quite extensive. For better maintainability and to reduce bundle size, consider fetching it from an external API or loading it from a static JSON file instead of embedding it in code.
245-715: Refactor repeated fields infasteignByStadfangNrData.
Many objects share the same structure. If possible, break this data into separate definitions or consider a single typed structure to reduce duplication.
717-971: Validate or mock the large property data set.
While this is helpful for local testing, ensure this data aligns with production-ready APIs, or consider mocking it in your tests rather than committing large, potentially outdated static data.libs/application/templates/rental-agreement/src/fields/PropertySearch/propertySearch.css.ts (2)
6-13: Extract color tokens.
Consider referencing design system tokens or a shared theme for theborderandborderRadiusvalues to ensure consistent styling across the app and facilitate future theming.
60-71: Use a design system or transitions variable.
The inlinetransition: 'all 0.3s ease-in-out'and hard-coded background color can be replaced by a design system variable or an existing shared transition style, if available, for consistency.libs/application/templates/rental-agreement/src/lib/constants.ts (3)
6-6: Anticipate more events inEventstype.
WhileSUBMITandEDITmight suffice now, plan for future expansions such asREJECTorAPPROVEto avoid frequent refactoring.
22-35: Ensure route names align with app conventions.
Some routes use dot notation (registerProperty.info), others do not. Standardizing these patterns can improve developer clarity and reduce potential routing confusion.
110-116: Document thepruneAfterDaysfunction.
This life cycle logic is crucial. Provide a short comment explaining the rationale behind pruning and when it should be used, to assist other developers.libs/application/templates/rental-agreement/src/lib/utils.ts (2)
53-54: EnhanceformatCurrencyfunction to handle non-numeric inputs.The current implementation assumes the input is always a valid numeric string and doesn't handle potential errors.
-export const formatCurrency = (answer: string) => - answer.replace(/\B(?=(\d{3})+(?!\d))/g, '.') + ' ISK' +export const formatCurrency = (answer: string) => { + // Ensure we're working with a numeric string + if (!answer || isNaN(Number(answer))) { + return '0 ISK'; + } + return answer.replace(/\B(?=(\d{3})+(?!\d))/g, '.') + ' ISK'; +}
289-298: Fix inconsistent naming pattern forgetUserRoleOptions.Unlike other option getters that are functions,
getUserRoleOptionsis exported as a constant array. This breaks the naming convention established by the other functions.-export const getUserRoleOptions = [ +export const getUserRoleOptions = () => [ { label: m.userRole.landlordOptionLabel, value: UserRole.LANDLORD, }, { label: m.userRole.tenantOptionLabel, value: UserRole.TENANT, }, ]
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
libs/application/templates/rental-agreement/src/assets/IconCircleClose.tsx(1 hunks)libs/application/templates/rental-agreement/src/assets/IconCircleOpen.tsx(1 hunks)libs/application/templates/rental-agreement/src/fields/PropertySearch/index.tsx(1 hunks)libs/application/templates/rental-agreement/src/fields/PropertySearch/propertyData.ts(1 hunks)libs/application/templates/rental-agreement/src/fields/PropertySearch/propertySearch.css.ts(1 hunks)libs/application/templates/rental-agreement/src/fields/Summary/PropertyInfoSummary.tsx(1 hunks)libs/application/templates/rental-agreement/src/fields/Summary/RentalInfoSummary.tsx(1 hunks)libs/application/templates/rental-agreement/src/forms/rentalHousing/rentalHousingCondition.ts(1 hunks)libs/application/templates/rental-agreement/src/forms/rentalHousing/rentalHousingPropertyInfo.ts(1 hunks)libs/application/templates/rental-agreement/src/forms/rentalPeriod/rentalPeriodAmount.ts(1 hunks)libs/application/templates/rental-agreement/src/forms/rentalPeriod/rentalPeriodDetails.ts(1 hunks)libs/application/templates/rental-agreement/src/forms/rentalPeriod/rentalPeriodOtherFees.ts(1 hunks)libs/application/templates/rental-agreement/src/forms/rentalPeriod/rentalPeriodSecurityDeposit.ts(1 hunks)libs/application/templates/rental-agreement/src/lib/constants.ts(1 hunks)libs/application/templates/rental-agreement/src/lib/dataSchema.ts(1 hunks)libs/application/templates/rental-agreement/src/lib/messages/housing/registerProperty.ts(1 hunks)libs/application/templates/rental-agreement/src/lib/types.ts(1 hunks)libs/application/templates/rental-agreement/src/lib/utils.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- libs/application/templates/rental-agreement/src/assets/IconCircleOpen.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`libs/**/*`: "Confirm that the code adheres to the following...
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/rental-agreement/src/forms/rentalPeriod/rentalPeriodDetails.tslibs/application/templates/rental-agreement/src/forms/rentalHousing/rentalHousingCondition.tslibs/application/templates/rental-agreement/src/forms/rentalHousing/rentalHousingPropertyInfo.tslibs/application/templates/rental-agreement/src/forms/rentalPeriod/rentalPeriodOtherFees.tslibs/application/templates/rental-agreement/src/assets/IconCircleClose.tsxlibs/application/templates/rental-agreement/src/fields/Summary/PropertyInfoSummary.tsxlibs/application/templates/rental-agreement/src/lib/messages/housing/registerProperty.tslibs/application/templates/rental-agreement/src/forms/rentalPeriod/rentalPeriodAmount.tslibs/application/templates/rental-agreement/src/lib/types.tslibs/application/templates/rental-agreement/src/fields/PropertySearch/propertySearch.css.tslibs/application/templates/rental-agreement/src/fields/PropertySearch/propertyData.tslibs/application/templates/rental-agreement/src/fields/Summary/RentalInfoSummary.tsxlibs/application/templates/rental-agreement/src/fields/PropertySearch/index.tsxlibs/application/templates/rental-agreement/src/lib/utils.tslibs/application/templates/rental-agreement/src/lib/dataSchema.tslibs/application/templates/rental-agreement/src/lib/constants.tslibs/application/templates/rental-agreement/src/forms/rentalPeriod/rentalPeriodSecurityDeposit.ts
🪛 Biome (1.9.4)
libs/application/templates/rental-agreement/src/forms/rentalPeriod/rentalPeriodDetails.ts
[error] 19-19: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
libs/application/templates/rental-agreement/src/forms/rentalPeriod/rentalPeriodAmount.ts
[error] 33-33: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
libs/application/templates/rental-agreement/src/fields/PropertySearch/index.tsx
[error] 221-226: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 391-392: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 438-439: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 441-563: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
libs/application/templates/rental-agreement/src/lib/dataSchema.ts
[error] 289-289: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 348-349: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 372-373: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 392-393: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (20)
libs/application/templates/rental-agreement/src/forms/rentalPeriod/rentalPeriodSecurityDeposit.ts (1)
1-220: Well-structured and comprehensive security deposit form implementation.This implementation demonstrates excellent form architecture with:
- Strong type safety and proper usage of TypeScript
- Comprehensive conditional rendering logic that creates a dynamic user experience
- Well-organized field grouping with appropriate clearing behavior
- Proper internationalization through message constants
- Thorough handling of all security deposit types with specific input fields for each
The form design follows best practices for complex form management, with conditions that appropriately show/hide fields based on user selections, making for an intuitive user flow.
libs/application/templates/rental-agreement/src/forms/rentalPeriod/rentalPeriodDetails.ts (1)
22-66: Well-structured rental period form section.The RentalPeriodDetails implementation follows strong form design patterns with:
- Clean organization of date fields with proper validation
- Effective conditional rendering of the end date based on whether the period is definite
- Good use of the checkbox to control related fields
- Appropriate field clearing when selections change
- Proper internationalization through message constants
This creates an intuitive user experience for entering rental period information.
libs/application/templates/rental-agreement/src/forms/rentalPeriod/rentalPeriodOtherFees.ts (1)
1-186: Clean implementation with clear structure for future extensibility.This form section is well-implemented with:
- Strong separation of concerns using helper functions for conditional logic
- Appropriate field types and validation for different data inputs
- Well-structured grouping of related fields
- Proper internationalization through message constants
The commented-out code with the TODO for "otherCosts" is handled appropriately - it's clearly marked for future implementation while not interfering with current functionality.
Once you're ready to implement the commented "otherCosts" section (lines 132-182), remember to:
- Uncomment the code
- Ensure all imports are available (particularly
getValueViaPathandTRUEconstant)- Update any field identifiers if the data structure has changed
libs/application/templates/rental-agreement/src/forms/rentalHousing/rentalHousingCondition.ts (1)
13-76: Code looks well-structured and follows component architecture patterns.The RentalHousingCondition form section is properly implemented using the building blocks from the application core. It includes appropriate conditional logic, field types, and organization.
Some observations:
- Good use of conditional rendering for the inspector name field based on selection
- Well-organized hierarchy with titles and descriptions
- Appropriate field types for different input requirements
libs/application/templates/rental-agreement/src/fields/Summary/PropertyInfoSummary.tsx (2)
40-262: Well-structured component with good organization and helper functions.The PropertyInfoSummary component follows good practices for React components with appropriate typing, helper functions, and conditional rendering. The layout is responsive using GridColumn components.
124-125: Placeholder value for number of rooms.This line displays a placeholder "-" value, suggesting that real implementation for retrieving number of rooms is not yet complete.
Is this intentional as a placeholder for future implementation? If so, you might want to add a TODO comment to track this.
libs/application/templates/rental-agreement/src/lib/messages/housing/registerProperty.ts (2)
3-209: Well-structured internationalization messages.The message definitions are well-organized into logical sections and provide comprehensive coverage for the property registration UI elements. The naming is consistent and descriptive.
1-209:❓ Verification inconclusive
Ensure message IDs match implementation in the codebase.
The file contains a comprehensive set of internationalization messages. To maintain consistency, verify that all message IDs are correctly referenced in the components that use them.
🏁 Script executed:
#!/bin/bash # Find all files that might be using these messages echo "Checking for message imports and usages:" rg -A 3 "import.*registerProperty" --type ts --type tsx echo "Checking for message id usage patterns:" rg "ra\.application:registerProperty\." --type ts --type tsxLength of output: 356
Action Required: Further Manual Verification of Message ID Usage
The internationalization message IDs in the
registerPropertymessages file appear to be defined correctly. However, our initial automated search encountered file type recognition issues (specifically withtsx) that prevented a complete verification of their usage across the codebase.Please re-run the following updated shell script to confirm that all message IDs (e.g., those starting with
ra.application:registerProperty.) are consistently imported and referenced in your components:#!/bin/bash # Add file type definitions for TSX files rg --type-add 'tsx:*.tsx' echo "Checking for message imports and usages:" rg -A 3 "import.*registerProperty" --type ts --type tsx echo "Checking for message id usage patterns:" rg "ra\.application:registerProperty\." --type ts --type tsxOnce you verify that the message IDs are being used correctly in the codebase, please update the review accordingly.
libs/application/templates/rental-agreement/src/fields/Summary/RentalInfoSummary.tsx (1)
196-267: Ensure correct handling for undefined securityDeposit in the component.The component accesses
answers.securityDepositproperties directly without checking if the object itself exists. This could cause runtime errors if the securityDeposit object is undefined.Use optional chaining consistently throughout this section:
- answers.securityDeposit.securityType !== + answers.securityDeposit?.securityType !==libs/application/templates/rental-agreement/src/fields/PropertySearch/index.tsx (1)
313-315: Add error handling for selection castingThe casting of
selectedOptiontoStadfangPropscould lead to runtime errors if the types don't match. Consider adding validation or safer type handling.- setSelectedStadfang( - selectedOption as unknown as StadfangProps, - ) + // Ensure selectedOption has the required fields before casting + if (selectedOption && 'stadfang_nr' in selectedOption) { + setSelectedStadfang(selectedOption as unknown as StadfangProps) + }libs/application/templates/rental-agreement/src/forms/rentalPeriod/rentalPeriodAmount.ts (1)
53-60: Add internationalization to error messagesFollowing the previous review comments, ensure error messages for validation are included in the message files for proper internationalization.
For example, add input validation error messages to the messages file and reference them here for consistent user experience across languages.
libs/application/templates/rental-agreement/src/lib/dataSchema.ts (2)
38-42: Fix validation logic for nationalIdThe validation logic for nationalId appears to be correct now (after previous fixes), ensuring that only valid national IDs are accepted.
134-157: Consider enabling commented validation logicThere are several commented sections of validation logic that appear to be important for ensuring data integrity. Consider enabling these validations when the repeatable table functionality is fixed.
Plan a follow-up task to implement these validations to ensure that:
- The landlord table has at least one representative
- The landlord table has at least one non-representative
libs/application/templates/rental-agreement/src/lib/types.ts (3)
1-2: Well-structured import.
No concerns here. The import ofNationalRegistryIndividualfrom@island.is/application/typesis concise and correctly typed.
3-9: Validate presence of required fields forExternalData.
Ensure that the upstream implementation always provides validNationalRegistryIndividualdata, especially regarding optional vs. required fields. If fields might be missing, consider making them optional or adding runtime checks.
11-11: Use an enum forStatusProvider.
This matches a previous recommendation to switch from union string literals to an enum for maintainability and clarity.libs/application/templates/rental-agreement/src/fields/PropertySearch/propertySearch.css.ts (1)
1-5: Clear naming and consistent font sizes.
These constants are straightforward and correctly convey meaning for table cells.libs/application/templates/rental-agreement/src/lib/utils.ts (3)
20-20: Consider using named imports for better tree-shaking.Using wildcard imports like
import * as m from './messages'can hinder tree-shaking. Named imports would make it clearer which messages are actually used and help with bundle optimization.-import * as m from './messages' +import { registerProperty, housingCondition, rentalAmount, securityDeposit, otherFees, userRole } from './messages'
110-123: Consider memoizing option getter functions for React components.These functions create new arrays on each call, which can lead to unnecessary re-renders in React components.
Convert these functions into custom hooks that use React's
useMemo:import { useMemo } from 'react' export const usePropertyTypeOptions = () => useMemo(() => [ // options... ], [])This ensures the array is only created once and reused across renders.
Also applies to: 125-134, 136-163, 165-174, 176-189, 191-204, 206-231, 233-250, 252-261, 263-272, 274-287
158-162: Address the TODO comment for employee housing option.The code contains a commented-out option for employee housing with a TODO comment. Either implement this option or remove the comment to maintain cleaner code.
Could you check if the decision has been made regarding the employee housing option and either implement it or remove the comment?
| // TODO: Implement action to open file in new tab | ||
| window.open(file.key) | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement proper file opening mechanism.
The TODO comment indicates that the file opening mechanism is incomplete. Consider implementing a proper handler that ensures file URLs are properly sanitized and handled.
- // TODO: Implement action to open file in new tab
- window.open(file.key)
+ const fileUrl = new URL(file.key);
+ if (fileUrl.origin === window.location.origin || process.env.ALLOWED_FILE_DOMAINS?.includes(fileUrl.origin)) {
+ window.open(fileUrl.toString(), '_blank', 'noopener,noreferrer');
+ } else {
+ console.error('Attempted to open file from disallowed origin:', fileUrl.origin);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // TODO: Implement action to open file in new tab | |
| window.open(file.key) | |
| }} | |
| const fileUrl = new URL(file.key); | |
| if (fileUrl.origin === window.location.origin || process.env.ALLOWED_FILE_DOMAINS?.includes(fileUrl.origin)) { | |
| window.open(fileUrl.toString(), '_blank', 'noopener,noreferrer'); | |
| } else { | |
| console.error('Attempted to open file from disallowed origin:', fileUrl.origin); | |
| } | |
| }} |
| // TODO: Add correct value | ||
| <KeyValue |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Complete the property address implementation.
There's a TODO comment indicating the property address value logic needs to be completed. This should be addressed before final implementation.
| const fetchPropertiesByStadfang = (query = '') => { | ||
| if (query.length < 2) { | ||
| console.log('No data - query too short') | ||
| return | ||
| } | ||
| setPending(true) | ||
|
|
||
| // TODO: Update when actual fetch is implemented | ||
| // fetch('http://localhost:3001/properties') | ||
| // .then((res) => res.json()) | ||
| // .then((data: PropertyStadfang[]) => { | ||
| // setPending(false) | ||
| // const filteredData = data | ||
| // .filter((property: PropertyStadfang) => | ||
| // property.stadfang.toLowerCase().includes(query.toLowerCase()), | ||
| // ) | ||
| // .sort((a: PropertyStadfang, b: PropertyStadfang) => | ||
| // a.stadfang.localeCompare(b.stadfang), | ||
| // ) | ||
|
|
||
| // TODO: Update when actual fetch is implemented | ||
| setTimeout(() => { | ||
| const filteredData = stadfangData | ||
| .filter((item: StadfangProps) => | ||
| item.stadfang.toLowerCase().includes(query.toLowerCase()), | ||
| ) | ||
| .sort((a: StadfangProps, b: StadfangProps) => | ||
| a.stadfang.localeCompare(b.stadfang), | ||
| ) | ||
| .slice(0, 10) | ||
|
|
||
| setPending(false) | ||
|
|
||
| if (filteredData.length) { | ||
| setSearchOptions( | ||
| filteredData.map((property: StadfangProps) => ({ | ||
| label: `${property.stadfang}, ${property.postnumer} ${property.sveitarfelag_nafn}`, | ||
| value: `${property.stadfang}, ${property.postnumer} ${property.sveitarfelag_nafn}`, | ||
| stadfang_nr: property.stadfang_nr, | ||
| stadfang: property.stadfang, | ||
| sveitarfelag_nafn: property.sveitarfelag_nafn, | ||
| sveitarfelag_nr: property.sveitarfelag_nr, | ||
| birting_sveitarfelag_nr: property.birting_sveitarfelag_nr, | ||
| postnumer: property.postnumer, | ||
| landeign_nr: property.landeign_nr, | ||
| stadvisir: property.stadvisir, | ||
| stadgreinir: property.stadgreinir, | ||
| vidskeyti: property.vidskeyti, | ||
| })), | ||
| ) | ||
| } | ||
| }, 500) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace mock data implementation with actual API calls
The component currently uses mock data with setTimeout to simulate API calls. There are TODO comments indicating this needs to be replaced with actual fetch implementation.
Additionally, consider moving the API call logic to a separate service or custom hook to improve separation of concerns and testability.
| console.log('No data - query too short') | ||
| return | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove console.log statements before production release
Console logs shouldn't remain in production code as they can impact performance and potentially expose sensitive information.
- console.log('No data - query too short')
+ // No data - query too short📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log('No data - query too short') | |
| return | |
| } | |
| // No data - query too short | |
| return | |
| } |
| console.log('searchresults: ', answers.registerProperty) | ||
|
|
||
| // const propertyUsageValue = getValueViaPath( | ||
| // answers, | ||
| // 'answers.registerProperty.searchresults?.adalmatseiningByFasteignNr[2522506].notkun', | ||
| // ) | ||
| // const unitIdValue = getValueViaPath( | ||
| // answers, | ||
| // 'answers.registerProperty.searchresults?.propertiesByStadfangNr[0].merking', | ||
| // ) | ||
| // const sizeValue = getValueViaPath( | ||
| // answers, | ||
| // 'answers.registerProperty.searchresults?.propertiesByStadfangNr[0].flatarmal', | ||
| // ) | ||
| // const roomsValue = getValueViaPath( | ||
| // answers, | ||
| // 'answers.registerProperty.searchresults.numberOfRooms', | ||
| // ) | ||
|
|
||
| return [ | ||
| [ | ||
| // propertyUsageValue ? propertyUsageValue : '--', | ||
| // unitIdValue ? unitIdValue : '--', | ||
| // sizeValue ? `${sizeValue} m²` : '--', | ||
| // roomsValue ? roomsValue : '--', | ||
| 'Íbúð á hæð', | ||
| '010303', | ||
| '144 m²', | ||
| '3', | ||
| ], | ||
| ] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove console.log and uncomment dynamic data loading
The code includes a console.log statement and commented-out code for dynamic data loading. This should be removed or properly implemented before production.
- console.log('searchresults: ', answers.registerProperty)
+ // Get dynamic data from answers
- // const propertyUsageValue = getValueViaPath(
- // answers,
- // 'answers.registerProperty.searchresults?.adalmatseiningByFasteignNr[2522506].notkun',
- // )
+ const propertyUsageValue = getValueViaPath(
+ answers,
+ 'registerProperty.searchresults.adalmatseiningByFasteignNr[2522506].notkun',
+ )
// Continue uncommenting and fixing other property valuesAlso, hardcoded values should be replaced with dynamic data from the application state.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log('searchresults: ', answers.registerProperty) | |
| // const propertyUsageValue = getValueViaPath( | |
| // answers, | |
| // 'answers.registerProperty.searchresults?.adalmatseiningByFasteignNr[2522506].notkun', | |
| // ) | |
| // const unitIdValue = getValueViaPath( | |
| // answers, | |
| // 'answers.registerProperty.searchresults?.propertiesByStadfangNr[0].merking', | |
| // ) | |
| // const sizeValue = getValueViaPath( | |
| // answers, | |
| // 'answers.registerProperty.searchresults?.propertiesByStadfangNr[0].flatarmal', | |
| // ) | |
| // const roomsValue = getValueViaPath( | |
| // answers, | |
| // 'answers.registerProperty.searchresults.numberOfRooms', | |
| // ) | |
| return [ | |
| [ | |
| // propertyUsageValue ? propertyUsageValue : '--', | |
| // unitIdValue ? unitIdValue : '--', | |
| // sizeValue ? `${sizeValue} m²` : '--', | |
| // roomsValue ? roomsValue : '--', | |
| 'Íbúð á hæð', | |
| '010303', | |
| '144 m²', | |
| '3', | |
| ], | |
| ] | |
| // Get dynamic data from answers | |
| const propertyUsageValue = getValueViaPath( | |
| answers, | |
| 'registerProperty.searchresults.adalmatseiningByFasteignNr[2522506].notkun', | |
| ) | |
| // Continue uncommenting and fixing other property values | |
| // const unitIdValue = getValueViaPath( | |
| // answers, | |
| // 'registerProperty.searchresults.propertiesByStadfangNr[0].merking', | |
| // ) | |
| // const sizeValue = getValueViaPath( | |
| // answers, | |
| // 'registerProperty.searchresults.propertiesByStadfangNr[0].flatarmal', | |
| // ) | |
| // const roomsValue = getValueViaPath( | |
| // answers, | |
| // 'registerProperty.searchresults.numberOfRooms', | |
| // ) | |
| return [ | |
| [ | |
| // Replace the static values below with dynamic values, e.g.: | |
| // propertyUsageValue ? propertyUsageValue : '--', | |
| // unitIdValue ? unitIdValue : '--', | |
| // sizeValue ? `${sizeValue} m²` : '--', | |
| // roomsValue ? roomsValue : '--', | |
| 'Íbúð á hæð', | |
| '010303', | |
| '144 m²', | |
| '3', | |
| ], | |
| ] |
| export const formatDate = (date: string) => { | ||
| return format(parseISO(date), 'dd.MM.yyyy', { | ||
| locale: is, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to formatDate function.
The function doesn't handle invalid date strings, which could lead to runtime errors.
export const formatDate = (date: string) => {
+ try {
+ const parsedDate = parseISO(date);
+ if (!isValid(parsedDate)) {
+ throw new Error('Invalid date');
+ }
return format(parseISO(date), 'dd.MM.yyyy', {
locale: is,
})
+ } catch (error) {
+ console.error('Error formatting date:', error);
+ return date; // Or a fallback format
+ }
}Don't forget to import isValid from date-fns at the top of the file:
import isValid from 'date-fns/isValid'* Add validation in dataSchema for fire protection inputs. * Add validation in dataSchema for other fees. * Add validation for inputs based on payment method chosen. * Update README.md with info about localization in Contentful. * Update and add states. Add landlords and tenants as assignees. * Update states and template flow. Cleanup and rename files. * Update * fix .github codeowners * Update property search with new data structure. Add table with property search results. * Filter assignees. Fix change button not appearig in summary. * Fix lint warnings * Fix unintentional moved folder. Delete unused data files. * Update property search results. Add clearOnChange for some radio/checkbox/select forms. * Remove unused data/schema, other minor lint fixes. * Fix render of summary for assignee view. * Fix view for summary applicant view after renaming
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
libs/application/templates/rental-agreement/src/lib/RentalAgreementTemplate.ts (2)
43-111: States PREREQUISITES and DRAFT are well-structured.The transitions, roles, and forms used for these states follow a clear pattern. The
pruneAfterDays(30)specification is repeated multiple times across different states—consider extracting it into a shared constant for easier maintenance.
211-229: Remove console logs in production code.Logging
Assignees: ...may cause unnecessary console output. Switching to a proper logging mechanism or removing debug statements altogether helps maintain a clean production environment.- console.log('Assignees: ', assignees)libs/application/templates/rental-agreement/src/fields/Summary/SummaryNoEdit.tsx (1)
18-18: Consider removing the debug console log.The
console.log('applicationAnswers', application.answers)line may clutter production logs or expose sensitive information. Use a logger with the appropriate log level or remove it if it's purely for local debugging.- console.log('applicationAnswers', application.answers)libs/application/templates/rental-agreement/src/forms/summaryNoEditForm.ts (1)
12-53: Form structure looks good, but consider using translated title labelThe form is well-structured using the form building utilities. It appropriately disables navigation buttons for a read-only view and organizes the content in a clean hierarchical structure.
I notice that the title of the custom field (line 45) is hardcoded as "Samantekt" rather than using an internationalized string from the messages. Consider using a message from the imported summary messages object for better localization support.
buildCustomField({ id: 'SummaryNoEdit', - title: 'Samantekt', + title: summary.sectionName, component: 'SummaryNoEdit', }),libs/application/templates/rental-agreement/src/forms/summaryForm.ts (3)
15-17: Inconsistent form ID naming convention with SummaryNoEditFormWhile the implementation is correct, I noticed that this form uses a camelCase ID (
summaryForm) which is inconsistent with the PascalCase ID used inSummaryNoEditForm(SummaryNoEditForm). Consider standardizing the naming convention across all forms for better code consistency.export const SummaryForm: Form = buildForm({ - id: 'summaryForm', + id: 'SummaryForm', title: application.name,
47-50: Custom field ID conflicts with parent multiField IDBoth the custom field and its parent multiField have the same ID ('summary'), which could cause confusion or potential issues with DOM identification. Best practice is to use unique IDs for each field.
buildCustomField({ - id: 'summary', + id: 'summaryEdit', title: 'Samantekt', component: 'SummaryEdit', }),
47-50: Consider using localized title for consistencySimilar to the issue in SummaryNoEditForm, the title "Samantekt" is hardcoded rather than using the localization system. For better internationalization support, use the same approach consistently across forms.
buildCustomField({ id: 'summary', title: 'Samantekt', + // Or use summary.sectionName for consistency component: 'SummaryEdit', }),libs/application/templates/rental-agreement/src/fields/Summary/summaryStyles.css.ts (1)
3-6: Use theme variables for consistent spacingThe spacing values (.75rem) are hardcoded. For better maintainability and consistency with the design system, consider using theme spacing variables.
import { style } from '@vanilla-extract/css' +import { theme } from '@island.is/island-ui/theme' export const summaryWrap = style({ - paddingTop: '.75rem', - paddingBottom: '.75rem', + paddingTop: theme.spacing[2], + paddingBottom: theme.spacing[2], })libs/application/templates/rental-agreement/src/fields/Summary/SummaryEdit.tsx (3)
30-40: Consider abstracting validation logic for reusabilityThe validation logic for checking if specific sections are complete is scattered throughout the component. This could be extracted into separate helper functions for better readability and reusability.
+ const isComplete = (section: Record<string, any>, requiredFields: string[]) => { + return requiredFields.every(field => Boolean(section[field])); + } - const isFireProtectionsPresent = - answers.fireProtections.smokeDetectors && - answers.fireProtections.fireExtinguisher && - answers.fireProtections.emergencyExits + const isFireProtectionsPresent = isComplete( + answers.fireProtections, + ['smokeDetectors', 'fireExtinguisher', 'emergencyExits'] + ); - const isConditionPresent = answers.condition.resultsDescription + const isConditionPresent = isComplete( + answers.condition, + ['resultsDescription'] + ); - const isOtherFeesPresent = - answers.otherFees.electricityCost && - answers.otherFees.heatingCost && - answers.otherFees.housingFund + const isOtherFeesPresent = isComplete( + answers.otherFees, + ['electricityCost', 'heatingCost', 'housingFund'] + );
125-127: Use optional chaining for cleaner syntaxThe conditional check before calling
setSubmitButtonDisabledcan be simplified using optional chaining for more concise and readable code.<Button onClick={() => { - setSubmitButtonDisabled && - setSubmitButtonDisabled(false) + setSubmitButtonDisabled?.(false) goToScreen?.(condition.route) }} variant="text" size="small" >🧰 Tools
🪛 Biome (1.9.4)
[error] 125-126: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
112-142: Extract alert message to a separate component for clarityThe conditional rendering of the alert message is quite complex. For better readability and maintenance, consider extracting this logic into a separate component.
You could extract this into a MissingInformationAlert component:
type MissingInformationAlertProps = { conditions: Array<{ isFilled: boolean; route: string; message: MessageDescriptor; }>; setSubmitButtonDisabled?: (disabled: boolean) => void; goToScreen?: (route: string) => void; }; const MissingInformationAlert: FC<MissingInformationAlertProps> = ({ conditions, setSubmitButtonDisabled, goToScreen, }) => { const { formatMessage } = useLocale(); const unfilledConditions = conditions.filter( (condition) => !condition.isFilled, ); if (unfilledConditions.length === 0) { return null; } return ( <Box marginTop={4} marginBottom={4}> <AlertMessage type="error" title={formatMessage(summary.alertMissingInfoTitle)} message={ <BulletList> {unfilledConditions.map((condition, index) => ( <Bullet key={`${index}_${condition.route}`}> <Button onClick={() => { setSubmitButtonDisabled?.(false) goToScreen?.(condition.route) }} variant="text" size="small" > {formatMessage(condition.message)} </Button> </Bullet> ))} </BulletList> } /> </Box> ); };Then in your main component, you could simplify to:
<MissingInformationAlert conditions={AlertMessageConditions} setSubmitButtonDisabled={setSubmitButtonDisabled} goToScreen={goToScreen} />🧰 Tools
🪛 Biome (1.9.4)
[error] 125-126: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
libs/application/templates/rental-agreement/src/fields/Summary/SummaryEdit.tsx(1 hunks)libs/application/templates/rental-agreement/src/fields/Summary/SummaryNoEdit.tsx(1 hunks)libs/application/templates/rental-agreement/src/fields/Summary/summaryStyles.css.ts(1 hunks)libs/application/templates/rental-agreement/src/fields/index.tsx(1 hunks)libs/application/templates/rental-agreement/src/forms/summaryForm.ts(1 hunks)libs/application/templates/rental-agreement/src/forms/summaryNoEditForm.ts(1 hunks)libs/application/templates/rental-agreement/src/lib/RentalAgreementTemplate.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`libs/**/*`: "Confirm that the code adheres to the following...
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/rental-agreement/src/forms/summaryNoEditForm.tslibs/application/templates/rental-agreement/src/fields/index.tsxlibs/application/templates/rental-agreement/src/forms/summaryForm.tslibs/application/templates/rental-agreement/src/fields/Summary/SummaryNoEdit.tsxlibs/application/templates/rental-agreement/src/lib/RentalAgreementTemplate.tslibs/application/templates/rental-agreement/src/fields/Summary/summaryStyles.css.tslibs/application/templates/rental-agreement/src/fields/Summary/SummaryEdit.tsx
🪛 Biome (1.9.4)
libs/application/templates/rental-agreement/src/fields/Summary/SummaryEdit.tsx
[error] 125-126: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: prepare
- GitHub Check: prepare / install
- GitHub Check: GitGuardian Security Checks
🔇 Additional comments (7)
libs/application/templates/rental-agreement/src/lib/RentalAgreementTemplate.ts (4)
1-26: Good use of reusability and typed imports.The initial imports and type definitions look aligned with best practices for a shared library, ensuring reusability, consistent TypeScript usage, and facilitating effective bundling.
28-42: Feature flag configuration seems properly integrated.Defining
featureFlag: Features.rentalAgreementand referencingApplicationTypes.RENTAL_AGREEMENTis a clean approach for gating this functionality. This follows the recommended patterns for organizing feature flags in libs.
112-162: Verify entry and exit actions in SUMMARY state.The usage of
assignUserson entry andclearAssigneeson exit is logical if the application requires a fresh assignment approach for each entry into this state. Ensure this matches the intended user flow and that removing assignees upon exit does not discard needed data.
163-210: Check the self-transition under SIGNING.Lines 182–183 show that a
SUBMITevent transitions the state fromSIGNINGback to itself (SIGNING). If multiple signers are needed, this may be correct. Otherwise, consider introducing a new terminal or next state.libs/application/templates/rental-agreement/src/fields/index.tsx (1)
1-3: Exports for field components look good.Bundling these exports in a single index file promotes discoverability and encourages code reuse across the rental agreement application.
libs/application/templates/rental-agreement/src/fields/Summary/SummaryNoEdit.tsx (2)
1-16: Well-typed component properties.Adopting
FC<React.PropsWithChildren<FieldBaseProps>>forSummaryNoEditaligns with recommended TypeScript patterns.
20-69: Layout and structure look consistent.By neatly organizing multiple summary sections within a single container, you ensure a cohesive and maintainable approach to rendering read-only application details.
| borderBottom: '1px solid #F2F7FF', | ||
|
|
||
| selectors: { | ||
| '&:last-child': { | ||
| borderColor: '#CCDFFF', | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use theme colors instead of hardcoded values
Hardcoded color values (#F2F7FF, #CCDFFF) make it difficult to maintain consistent styling across the application. Use the theme colors from the island-ui theme for better maintainability and design consistency.
import { style } from '@vanilla-extract/css'
+import { theme } from '@island.is/island-ui/theme'
// ... other styles
export const divider = style({
marginTop: '.75rem',
marginBottom: '.75rem',
- borderBottom: '1px solid #F2F7FF',
+ borderBottom: `1px solid ${theme.color.blue100}`,
selectors: {
'&:last-child': {
- borderColor: '#CCDFFF',
+ borderColor: theme.color.blue200,
},
},
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| borderBottom: '1px solid #F2F7FF', | |
| selectors: { | |
| '&:last-child': { | |
| borderColor: '#CCDFFF', | |
| }, | |
| }, | |
| import { style } from '@vanilla-extract/css' | |
| import { theme } from '@island.is/island-ui/theme' | |
| // ... other styles | |
| export const divider = style({ | |
| marginTop: '.75rem', | |
| marginBottom: '.75rem', | |
| borderBottom: `1px solid ${theme.color.blue100}`, | |
| selectors: { | |
| '&:last-child': { | |
| borderColor: theme.color.blue200, | |
| }, | |
| }, | |
| }) |
Rental-agreement project structure & minimal functionality
This is pretty much just wireframe code around a new application system form.
Locked behind a feature flag created in ConfigCat and added to @island.is/feature-flags package as per instructions in github docs.
Why
We want to get this code in so we can start developing in a better workflow, small PRs, QA and such.
Checklist:
Summary by CodeRabbit
New Features
Chores