fix(regulations-admin): Fixing bugs in editor and admin flow #17027
Conversation
WalkthroughThe changes include modifications to several components within the regulations admin portal. Key updates involve the introduction of a new custom hook, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Datadog ReportBranch report: ✅ 0 Failed, 24 Passed, 0 Skipped, 12.17s Total Time 🔻 Code Coverage Decreases vs Default Branch (1)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (17)
libs/portals/admin/regulations-admin/src/utils/hooks.ts (3)
140-144: Optimize performance with useCallbackThe returned functions should be memoized to prevent unnecessary re-renders in consuming components.
Apply this diff:
+ import { useCallback } from 'react' return { - addPristineRegulation, - removePristineRegulation, - isPristineRegulation, + addPristineRegulation: useCallback(addPristineRegulation, [pristineRegulations]), + removePristineRegulation: useCallback(removePristineRegulation, [pristineRegulations]), + isPristineRegulation: useCallback(isPristineRegulation, [pristineRegulations]), }
109-109: Add JSDoc documentation for the hookAdd documentation to explain the hook's purpose, usage, and return value for better maintainability.
Apply this diff:
+/** + * Hook to manage pristine state of regulations using session storage. + * Used to track which regulations haven't been modified in the editor. + * + * @returns {Object} Object containing functions to manage pristine state + * @returns {(regId: string) => void} addPristineRegulation - Adds a regulation to pristine state + * @returns {(regId: string) => void} removePristineRegulation - Removes a regulation from pristine state + * @returns {(regId: string) => boolean} isPristineRegulation - Checks if a regulation is pristine + */ export const usePristineRegulations = () => {
111-111: Extract storage key to a constantThe storage key is used in multiple places and should be extracted to a constant.
Apply this diff:
+const PRISTINE_REGULATIONS_STORAGE_KEY = 'PRISTINE_REGULATIONS' export const usePristineRegulations = () => { const [pristineRegulations, setPristineRegulations] = useState(() => { - const storedRegulations = sessionStorage.getItem('PRISTINE_REGULATIONS') + const storedRegulations = sessionStorage.getItem(PRISTINE_REGULATIONS_STORAGE_KEY) // ... }) // In addPristineRegulation sessionStorage.setItem( - 'PRISTINE_REGULATIONS', + PRISTINE_REGULATIONS_STORAGE_KEY, JSON.stringify(updatedRegulations), ) // In removePristineRegulation sessionStorage.setItem( - 'PRISTINE_REGULATIONS', + PRISTINE_REGULATIONS_STORAGE_KEY, JSON.stringify(updatedRegulations), )Also applies to: 119-121, 131-133
libs/portals/admin/regulations-admin/src/components/impacts/ImpactAmendingSelection.tsx (1)
Line range hint
20-35: Consider enhancing component reusability.While the component follows good practices, its reusability could be improved:
- The component is tightly coupled to the regulations domain
- Error handling is not exposed through props
- Search debounce time is hardcoded
Consider these improvements:
type ImpactAmendingSelectionProps = { setImpactRegOption: (option: SelRegOption) => void onError?: (error: Error) => void debounceMs?: number searchMinLength?: number formatNotFoundMessage?: (value?: string) => string }This would make the component more configurable and reusable across different contexts within the admin portal.
libs/portals/admin/regulations-admin/src/utils/formatAmendingUtils.ts (5)
32-37: Consider making the title array readonly for better type safety.The implementation is good, but we can further improve type safety.
-const titleArray = ['article__title', 'subchapter__title', 'chapter__title'] +const titleArray = ['article__title', 'subchapter__title', 'chapter__title'] as const
39-48: Consider extracting text mappings to a constant object.The current implementation is correct, but we can improve maintainability by centralizing the text mappings.
+const ARTICLE_TYPE_TEXT: Record<ArticleTitleType, string> = { + chapter: 'nýr kafli', + subchapter: 'nýr undirkafli', + article: 'ný grein', +} as const export const getArticleTypeText = (titleType?: ArticleTitleType): string => { - switch (titleType) { - case 'chapter': - return 'nýr kafli' - case 'subchapter': - return 'nýr undirkafli' - default: - return 'ný grein' - } + return titleType ? ARTICLE_TYPE_TEXT[titleType] : ARTICLE_TYPE_TEXT.article }
50-58: Consider using the titleArray constant for consistency.The function could leverage the existing
titleArrayconstant to maintain consistency and reduce duplication.export const getArticleTitleType = (element: HTMLElement): ArticleTitleType => { - if (element.classList.contains('subchapter__title')) { + if (element.classList.contains(titleArray[1])) { // subchapter__title return 'subchapter' - } else if (element.classList.contains('chapter__title')) { + } else if (element.classList.contains(titleArray[2])) { // chapter__title return 'chapter' } else { return 'article' } }
63-64: Add detailed documentation for the regex pattern.While extracting the regex improves maintainability, its complexity warrants detailed documentation explaining each part of the pattern.
+/** + * Regex pattern for matching article and chapter titles: + * - \d+\. gr\. : Matches article numbers (e.g., "1. gr.") + * - (?:[\p{L}]\.)? : Optionally matches a letter suffix (e.g., "a.") + * - (?:\d+|[IVXLCDM]+)\.?\s*Kafli : Matches chapter numbers in Arabic or Roman numerals + */ const extractRegex = /^\d+\. gr\.(?: [\p{L}]\.)?(?= |$)|^(?:\d+|[IVXLCDM]+)\.?\s*Kafli(?=\b| |$)/iu
32-33: Consider exporting all types for better reusability.To enhance reusability across different NextJS apps, consider creating and exporting an index of all types used in this utility file.
+// Types +export { + ArticleTitleType, + AdditionObject, +}libs/portals/admin/regulations-admin/src/components/EditBasics.tsx (2)
165-172: Consider adding error handling for state updates.While the logic is correct, the state updates could benefit from error handling to ensure atomic operations.
Consider wrapping the state updates in a try-catch block:
- // Update appendixes - if (!hasUpdatedAppendix) { - updateAppendixes() - setHasUpdatedAppendix(true) - } - - // Remove from session storage - removePristineRegulation(draft.id) + try { + // Update appendixes + if (!hasUpdatedAppendix) { + updateAppendixes() + setHasUpdatedAppendix(true) + } + // Remove from session storage + removePristineRegulation(draft.id) + } catch (error) { + console.error('Failed to update regulation:', error) + setHasUpdatedAppendix(false) + }
175-175: Consider extracting modal logic into a custom hook.The modal's visibility logic and state management could be encapsulated in a custom hook to improve reusability and maintainability.
Consider creating a new hook:
const useConfirmationModal = (draftId: string, onConfirm: () => void) => { const [isModalVisible, setIsModalVisible] = useState<boolean>(true); const [hasConfirmed, setHasConfirmed] = useState<boolean>(false); const [hasSeenModal, setHasSeenModal] = useState<boolean>(false); const { isPristineRegulation } = usePristineRegulations(); const shouldShowModal = isPristineRegulation(draftId); const handleConfirm = () => { onConfirm(); setIsModalVisible(false); setHasConfirmed(true); }; const handleVisibilityChange = (visibility: boolean) => { setIsModalVisible(visibility); if (visibility === false && !hasSeenModal) { setHasSeenModal(true); } }; return { isModalVisible, hasConfirmed, hasSeenModal, shouldShowModal, handleConfirm, handleVisibilityChange, }; };Also applies to: 334-355
libs/portals/admin/regulations-admin/src/components/impacts/EditChange.tsx (2)
288-291: Enhance error message specificityThe generic error message might not provide enough context to users about what went wrong.
Consider including more specific error information:
- toast.error(t(msg.errorOnSaveReg)) + toast.error(t(msg.errorOnSaveReg, { details: error.message }))Also applies to: 293-294
Line range hint
282-326: Reduce code duplication in save operationsThe create and update operations share similar error handling and success logic.
Consider extracting the common logic into a shared function:
const handleSaveResult = async (promise: Promise<any>) => { try { const res = await promise if (res.errors?.length > 1) { throw res.errors[0] } addPristineRegulation(draft.id) closeModal(true) return { success: true, error: undefined } } catch (error) { toast.error(t(msg.errorOnSaveReg)) return { success: false, error: error as Error } } } // Usage in saveChange: if (!activeChange.id) { await handleSaveResult( createDraftRegulationChange({ variables: { input: { // ... create input }, }, }) ) } else { await handleSaveResult( updateDraftRegulationChange({ variables: { input: { // ... update input }, }, }) ) }libs/portals/admin/regulations-admin/src/lib/messages.ts (1)
574-578: LGTM! Consider enhancing the error message with recovery steps.The error message is well-structured and follows the established patterns. However, we could improve the user experience by providing more specific recovery steps.
Consider this enhancement:
errorOnSaveReg: { id: 'ap.regulations-admin:error-on-save-reg', defaultMessage: - 'Villa kom upp við að vista texta. Vinsamlegast afritið texta og reynið aftur síðar.', + 'Villa kom upp við að vista texta. Vinsamlegast afritið texta í tryggt skjal, endurhlaðið síðunni og reynið aftur. Ef vandamálið er viðvarandi, hafið samband við kerfisstjóra.', },libs/portals/admin/regulations-admin/src/utils/formatAmendingRegulation.ts (3)
267-267: Consider extractingelementTypedetermination into a separate functionTo enhance code maintainability, consider refactoring the determination of
elementTypeinto a separate helper function. This would reduce complexity and make the code easier to understand.
293-293: Refactor repetitive code blocks in conditionalsThe code handling
isSectionTitlefor deletion, addition, and change scenarios is repetitive. Refactoring these into a single function or utility can reduce duplication and improve code maintainability.Example refactor:
- // Deletion case if (isSectionTitle) { pushHtml = `<p>Fyrirsögn ${articleTitle} ${regNameDisplay} fellur brott.</p>` as HTMLText } - // Addition case if (isSectionTitle) { pushHtml = `<p>Fyrirsögn ${articleTitle} ${regNameDisplay} orðast svo:</p><p>${newText}</p>` as HTMLText } - // Modification case if (isSectionTitle) { pushHtml = `<p>Fyrirsögn ${articleTitle} ${regNameDisplay} breytist og orðast svo:</p><p>${newText}</p>` as HTMLText } + // Refactored code function generateSectionTitleHtml(action: 'deleted' | 'added' | 'modified', articleTitle: string, regNameDisplay: string, newText?: string) { const actionText = { deleted: 'fellur brott', added: 'orðast svo', modified: 'breytist og orðast svo', } const newTextHtml = newText ? `<p>${newText}</p>` : '' return `<p>Fyrirsögn ${articleTitle} ${regNameDisplay} ${actionText[action]}.</p>${newTextHtml}` as HTMLText } // Then use pushHtml = generateSectionTitleHtml('deleted', articleTitle, regNameDisplay)Also applies to: 322-322, 355-355
383-383: Avoid code duplication by abstracting repeated logicThe code segments assigning and using
articleTitleNumberand constructing HTML strings are repeated. Consider creating helper functions to abstract this logic, which will enhance code readability and reduce potential errors.Also applies to: 394-394, 413-419
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
libs/portals/admin/regulations-admin/src/components/EditBasics.tsx(4 hunks)libs/portals/admin/regulations-admin/src/components/LawChaptersSelect.tsx(2 hunks)libs/portals/admin/regulations-admin/src/components/impacts/EditChange.tsx(5 hunks)libs/portals/admin/regulations-admin/src/components/impacts/ImpactAmendingSelection.tsx(2 hunks)libs/portals/admin/regulations-admin/src/lib/messages.ts(1 hunks)libs/portals/admin/regulations-admin/src/utils/formatAmendingRegulation.ts(10 hunks)libs/portals/admin/regulations-admin/src/utils/formatAmendingUtils.ts(2 hunks)libs/portals/admin/regulations-admin/src/utils/hooks.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- libs/portals/admin/regulations-admin/src/components/LawChaptersSelect.tsx
🧰 Additional context used
📓 Path-based instructions (7)
libs/portals/admin/regulations-admin/src/components/EditBasics.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/portals/admin/regulations-admin/src/components/impacts/EditChange.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/portals/admin/regulations-admin/src/components/impacts/ImpactAmendingSelection.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/portals/admin/regulations-admin/src/lib/messages.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/portals/admin/regulations-admin/src/utils/formatAmendingRegulation.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/portals/admin/regulations-admin/src/utils/formatAmendingUtils.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/portals/admin/regulations-admin/src/utils/hooks.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 (11)
libs/portals/admin/regulations-admin/src/components/impacts/ImpactAmendingSelection.tsx (1)
5-9: LGTM! Clean type imports.
The imports follow TypeScript best practices and maintain good tree-shaking capabilities by importing only the required types.
libs/portals/admin/regulations-admin/src/utils/formatAmendingUtils.ts (2)
14-15: LGTM! Good refactoring of title detection logic.
The change improves maintainability by centralizing the title class detection logic into a reusable function.
72-77: LGTM! Well-implemented subtitle detection.
The function is well-structured, reuses existing code, and follows good practices.
libs/portals/admin/regulations-admin/src/components/EditBasics.tsx (1)
32-32: LGTM! Verify hook's reusability across apps.
The hook implementation follows React conventions and TypeScript patterns.
Let's verify the hook's reusability across different NextJS apps:
Also applies to: 47-48
libs/portals/admin/regulations-admin/src/components/impacts/EditChange.tsx (2)
10-10: LGTM: Clean import additions
The new imports for toast notifications and pristine regulations management are well-organized and appropriately scoped.
Also applies to: 55-58
81-81: LGTM: Clean hook integration
The usePristineRegulations hook is properly integrated into the component's state management.
libs/portals/admin/regulations-admin/src/utils/formatAmendingRegulation.ts (5)
12-12: Addition of utility functions enhances title processing
The import of containsAnyTitleClass, getArticleTitleType, getArticleTypeText, and hasSubtitle from formatAmendingUtils is appropriate and necessary for the improved handling of article titles and their types.
Also applies to: 15-16, 20-20
210-214: Encapsulate title information within titleObject for clarity
Adding titleObject to testGroup improves data organization by grouping related properties (text, hasSubtitle, type) together, enhancing code readability and maintainability.
220-224: Initialize titleObject with default values to prevent undefined properties
Properly initializing titleObject ensures that all its properties have default values, preventing potential runtime errors from undefined properties.
236-237: Efficient use of containsAnyTitleClass to detect title elements
Utilizing containsAnyTitleClass(element) simplifies the logic for detecting if an element is a title, improving the readability of the code.
242-245: Assigning title properties correctly to testGroup.titleObject
Assigning hasSubtitle, type, and text to testGroup.titleObject accurately captures the necessary title information for further processing.
libs/portals/admin/regulations-admin/src/components/impacts/ImpactAmendingSelection.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
libs/portals/admin/regulations-admin/src/utils/hooks.ts (2)
Line range hint
75-79: Remove or document commented codeThere's commented out code that adds a self-affecting option. If this code is no longer needed, it should be removed. If it's intended for future use, please document why it's commented out or track it in a separate issue.
- // options.push({ - // type: selfType, - // value: 'self', - // label: selfAffectingText, - // })
110-110: Move constant outside hookThe
P_REG_KEYconstant should be moved outside the hook to prevent recreation on each render.+const PRISTINE_REGULATIONS_KEY = 'PRISTINE_REGULATIONS' + export const usePristineRegulations = () => { - const P_REG_KEY = 'PRISTINE_REGULATIONS'
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
libs/portals/admin/regulations-admin/src/utils/hooks.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/portals/admin/regulations-admin/src/utils/hooks.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 (1)
libs/portals/admin/regulations-admin/src/utils/hooks.ts (1)
109-150: 🛠️ Refactor suggestion
Export interface for better type safety and reusability
To improve type safety and reusability across different NextJS apps, export the hook's interface.
The previous review comment about exporting types is still valid. Please implement the suggested changes to export the PristineRegulationsHook interface.
thordurhhh
added
deprecated:automerge
thordurhhh
added
the
deprecated:automerge
* Pristine handling and error on save handling * Remove guess for mentioned lawchapters. Sort search results by best match * Update guesswork for preset changereg * Fix prist hook --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
What
Why
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor