PT-2015 | Upgrade to React 19 and HDS 6, replace legacy UI dependencies#475
Merged
Conversation
- hds-core installation and local publishing - hds-design-tokens installation and local publishing - hds-react installation and local publishing Includes setup for react-19-update branch development using yalc. Refs: Pt-2015
- Update CI workflow to use Node.js 24 - Update .nvmrc to v24.15.0 - Update Dockerfile base image to nodejs-24 - Update package.json engine requirement to ^24 - Update README documentation Refs: PT-2015
Refs: PT-2015
Refs: PT-2015
- bump react and react-dom to 19.2.5 - bump @types/react and @types/react-dom to 19.x - update @types/react resolution and related note in package.json - fix React 19 typing changes (useRef null init and element typing) - update enrolment modal snapshots for checkbox markup Refs: PT-2015
Refs: PT-2015
Refs: PT-2015
Replace react-helsinki-headless-cms imports with @city-of-helsinki/react-helsinki-headless-cms and use the public /apollo subpath for Apollo hooks. Update TypeScript module resolution to Bundler so package exports are resolved correctly in the Vite setup. Refs: PT-2015
Refs: PT-2015
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdates the app to use a shared notifications context and HDS dialog components, removes ChangesApplication UI and platform modernization
Sequence Diagram(s)sequenceDiagram
participant App as App
participant NotificationsProvider as NotificationsProvider
participant ApolloClient as initializeApolloClient
participant BaseDialogModal as BaseDialogModal
App->>NotificationsProvider: provide addNotification
App->>ApolloClient: initialize with addNotification
ApolloClient->>NotificationsProvider: emit GraphQL error notification
BaseDialogModal->>NotificationsProvider: render dialog actions with shared close behavior
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
KULTUS-ADMIN-UI branch is deployed to platta: https://kultus-admin-ui-pr475.dev.hel.ninja 🚀🚀🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/domain/occurrence/CreateOccurrencePage.tsx (1)
137-146:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent duplicate occurrence writes in publish flow.
On Line 145, occurrence submit is triggered whenever the child form is valid, even when it is already saved. Since Line 169 calls this during “Go to publishing”, this can re-run non-idempotent add-occurrence mutations and create duplicates.
Suggested fix
const submitOccurrenceFormIfNeeded = async () => { if ('submitForm' in context.current) { const { submitForm: submitOccurrenceForm, isValid: occurrenceFormIsValid, + dirty: occurrenceFormIsDirty, } = context.current; - if (occurrenceFormIsValid) { + if (occurrenceFormIsValid && occurrenceFormIsDirty) { await submitOccurrenceForm(); } } };src/clients/apiReportClient/useReportClientQuery.ts (2)
9-16:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRevoke blob URLs after download trigger.
Line 10 creates an object URL but it is never released, which can leak memory across repeated downloads.
Suggested change
function downloadFile(data: string, filename: string) { const url = window.URL.createObjectURL(new Blob([data])); const link = document.createElement('a'); link.href = url; link.setAttribute('download', filename); document.body.appendChild(link); link.click(); link.remove(); + window.URL.revokeObjectURL(url); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/clients/apiReportClient/useReportClientQuery.ts` around lines 9 - 16, The downloadFile function creates an object URL (const url) but never releases it; update downloadFile to revoke the created object URL after triggering the download (e.g., call window.URL.revokeObjectURL(url) after link.click(), or schedule revocation with setTimeout to ensure the download starts before revoking) and keep the existing cleanup of the link element; reference downloadFile, url and link when making the change.
24-48:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle rejected CSV requests so users still get feedback.
Line 26 can throw on network/non-2xx failures, so the current
elsenotification path is skipped and callers get a rejected promise without user-facing error messaging.Suggested change
return async () => { const filename = `kultus_events_approved_enrolments.csv`; - const response = await apiReportClient.get( - `${ROUTES.EVENTS_ENROLMENTS}?ids=${pEventId}`, - { - responseType: 'blob', - headers: { - 'Content-Type': 'text/csv; charset=utf-8', - /* TODO: Accept and Content-Disposition headers would be good to have, - but they won't work until API supports them. */ - // Accept: 'text/csv; charset=utf-8', - // 'Content-Disposition': `attachment; filename=${filename}`, - }, - } - ); - // Hack to download files with axios: - // https://gist.github.com/javilobo8/097c30a233786be52070986d8cdb1743 - if (response.status === 200 && response?.data?.type === 'text/csv') { - downloadFile(response.data, filename); - } else { + try { + const response = await apiReportClient.get( + `${ROUTES.EVENTS_ENROLMENTS}?ids=${pEventId}`, + { + responseType: 'blob', + headers: { + 'Content-Type': 'text/csv; charset=utf-8', + }, + } + ); + if (response.status === 200 && response?.data?.type === 'text/csv') { + downloadFile(response.data, filename); + return; + } + } catch (_error) { + // fall through to notification + } + addNotification({ type: 'error', label: t('eventEnrolmentsReport.downloadError'), }); - } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/clients/apiReportClient/useReportClientQuery.ts` around lines 24 - 48, Wrap the apiReportClient.get call in a try/catch inside useReportClientQuery's async handler so network/non-2xx errors are caught and the user still receives feedback; on catch call addNotification with the same error label (t('eventEnrolmentsReport.downloadError')) and optionally log the error, and only proceed to check response.status/response.data.type and call downloadFile(response.data, filename) when the request succeeds. Ensure you reference the existing symbols apiReportClient.get, downloadFile, addNotification, and t in your changes.package.json (1)
62-62:⚠️ Potential issue | 🟡 MinorUpgrade
eslint-plugin-react-hooksto the latest 7.x (7.1.1+)
package.jsonpins"eslint-plugin-react-hooks"at^5.2.0; React 19.2 guidance emphasizes using the latest plugin so you get the newest Hook lint rules. Update it to the current latest version.Suggested change
- "eslint-plugin-react-hooks": "^5.2.0", + "eslint-plugin-react-hooks": "^7.1.1",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` at line 62, The dependency "eslint-plugin-react-hooks" in package.json is pinned to ^5.2.0 and needs upgrading to the latest 7.x (>=7.1.1); update the version string for "eslint-plugin-react-hooks" in package.json to the current 7.x release (for example "7.1.1" or the latest published 7.x), then run your package manager (npm/yarn/pnpm) to install and verify linting still passes; ensure any lockfile (package-lock.json / yarn.lock / pnpm-lock.yaml) is updated accordingly.
🧹 Nitpick comments (2)
src/domain/occurrence/enrolmentTable/enrolmentModals/EnrolmentModal.tsx (1)
104-113: ⚡ Quick winUse instance-scoped IDs for dialog ARIA wiring.
Line 104 hardcodes
alert-modal, which can cause duplicateid/aria-labelledby/aria-describedbyvalues if multiple instances mount. Prefer per-instance IDs.♻️ Suggested update
const EnrolmentModal: React.FC<EnrolmentModalProps> = ({ ... }) => { const { t } = useTranslation(); + const uid = React.useId(); const [showMessageTextArea, setShowMessageTextArea] = React.useState(false); const hasEnrollees = enrollees && enrollees.length > 0; - const id = 'alert-modal'; - const titleId = `${id}-title`; - const descriptionId = `${id}-content`; + const id = `${uid}-enrolment-modal`; + const titleId = `${id}-title`; + const descriptionId = `${id}-content`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain/occurrence/enrolmentTable/enrolmentModals/EnrolmentModal.tsx` around lines 104 - 113, The modal currently uses a hardcoded id ('alert-modal') causing duplicate id/aria attributes when multiple EnrolmentModal instances mount; update the EnrolmentModal component to generate per-instance IDs (e.g., React's useId() or a stable UUID) and derive titleId and descriptionId from that instance id, then pass those instance-scoped id, aria-labelledby and aria-describedby values into the Dialog props; update references to id, titleId and descriptionId in the component (the variables defined around the Dialog render) so each modal instance has unique ARIA wiring.src/domain/app/apollo/__tests__/apolloClient.test.ts (1)
22-25: ⚡ Quick winAdd a regression test for notification callback refresh on client reuse.
Please add a test that initializes once with callback A and then with callback B, and verifies the active callback is B. This guards the singleton path against stale notification handlers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain/app/apollo/__tests__/apolloClient.test.ts` around lines 22 - 25, Add a regression test that verifies initializeApolloClient updates the notification callback when the singleton client is reused: call initializeApolloClient once with callbackA (vi.fn()), then call it again with callbackB (vi.fn()), assert the returned client is the same instance as the first (to exercise the reuse path), and then assert that the active notification handler is callbackB (e.g., by triggering whatever notification dispatch/update method the client exposes or by checking the stored callback reference on the client). Ensure you reference initializeApolloClient and the two mock callbacks (callbackA, callbackB) in the test so the singleton path refresh behavior is validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Around line 25-27: The listed Testing Library packages (`@testing-library/dom`,
`@testing-library/jest-dom`, `@testing-library/react` — and add
`@testing-library/user-event` if present) are test-only and should be moved out of
dependencies into devDependencies: remove these entries from the "dependencies"
block in package.json and add equivalent entries (same versions) under
"devDependencies" so the test suite keeps access but they are not bundled for
production; ensure package names and versions match the originals and run
npm/Yarn install to update lockfile.
In `@src/common/components/modal/AlertModal.tsx`:
- Around line 22-30: The AlertModal currently uses fixed IDs (id, titleId,
descriptionId) which can collide when multiple instances mount; update
AlertModal to generate per-instance unique IDs (e.g., use React.useId() or a
small unique-id helper) and use those generated values for id, titleId, and
descriptionId passed into the Dialog aria-labelledby/aria-describedby props so
each AlertModal instance has unique ARIA targets.
In `@src/common/components/modal/ConfirmationModal.tsx`:
- Around line 24-33: The modal currently uses a fixed base id
('confirmation-modal') causing duplicate id/aria attributes across multiple
instances; update ConfirmationModal to generate a stable unique id per component
instance (e.g., React's useId or a useRef with a UUID/nanoid) and derive titleId
and descriptionId from that instance id (replace the hardcoded id, titleId,
descriptionId assignments). Ensure the generated id is stable across renders so
aria-labelledby and aria-describedby remain consistent for the Dialog component.
In `@src/common/components/modal/NotificationModal.tsx`:
- Around line 22-31: NotificationModal currently uses a fixed id
('notification-modal') and derived titleId/descriptionId which can collide
across multiple mounted instances; update the component to generate a unique id
per instance (e.g., React's useId() or a UUID) and derive titleId and
descriptionId from that generated id, then pass those unique ids into Dialog's
id, aria-labelledby and aria-describedby props (referencing the
NotificationModal component and the id/titleId/descriptionId symbols to locate
where to change).
In
`@src/common/components/notificationsContext/__tests__/NotificationsContext.test.tsx`:
- Around line 13-17: The test components call addNotification inside a useEffect
without a dependency array, causing the effect to run on every re-render; update
those useEffect calls to include the memoized dependency so they run once on
mount by changing their closing to include [addNotification] (i.e., useEffect(()
=> { addNotification(...); }, [addNotification])); locate the useEffect usages
in NotificationsContext.test.tsx where addNotification is invoked and add the
[addNotification] dependency array to each.
In `@src/common/components/table/Table.tsx`:
- Around line 128-134: Compute a guarded colSpan for the expanded-row cell
instead of using row.getVisibleCells().length - expandedAreaOffset directly;
ensure the computed value is at least 1 (e.g. remainingColSpan = Math.max(1,
row.getVisibleCells().length - expandedAreaOffset)) and use that for the td
colSpan so you never render a non-positive colSpan, referencing
expandedAreaOffset and row.getVisibleCells() in the same component that renders
the empty <td> and the expanded-content <td>.
- Around line 40-46: The onExpandedChange handler currently resolves a
functional updater using the closed-over expanded value, risking lost/batched
updates; change it so updaterOrValue is passed directly to setExpanded (i.e.,
call setExpanded(updaterOrValue)) instead of computing newValue with expanded.
Update the onExpandedChange implementation in Table.tsx (the function handling
onExpandedChange/updaterOrValue) to forward the updater or value directly to
setExpanded so React handles functional updaters correctly.
In `@src/domain/app/apollo/apolloClient.ts`:
- Around line 30-34: The singleton Apollo client currently closes over the
initial addNotification callback in createApolloClient, so later initializations
can't update the handler; change the implementation to hold addNotification in a
mutable reference (e.g. a module-scoped variable or setter like
setAddNotification) and have all links/error handlers (the error handling link
referenced where addNotification is used — e.g., the errorLink/auth-related
links at the other noted spots) call the latest function from that reference
instead of the closed-over parameter; also update createApolloClient to
set/update that reference on each call so notifications always route to the most
recent callback (apply same change for the other uses mentioned at the other
locations).
In `@src/domain/occurrence/enrolmentTable/EnrolmentTable.tsx`:
- Around line 122-127: The toggle button in EnrolmentTable.tsx currently uses
stopPropagation/unnecessary navigation guards even though Table.tsx only
triggers onRowClick for TD targets; remove any
stopPropagation()/navigation-prevention logic from the button click handler and
rely on row.toggleExpanded() and row.getIsExpanded() for expansion state, and
add type="button" to the button element to prevent accidental form submits;
locate the button that calls row.toggleExpanded() and remove the extra
propagation code while adding the type attribute.
In `@src/domain/occurrences/occurrencesTable/OccurrencesTable.tsx`:
- Around line 95-97: selectedOccurrences contains occurrence IDs but the
checkbox uses the TanStack row key (row.id) for checked while
handleCheckboxChange toggles row.original.id, causing desync; update the
checkbox id prop and checked expression to use row.original.id (i.e.,
id={`${id}_${row.original.id}_checkbox`} and
checked={selectedOccurrences.includes(row.original.id)}) so both checked state
and onChange use the same occurrence identifier (row.original.id) as used by
handleCheckboxChange.
In `@src/domain/occurrences/occurrencesTableReadOnly/OccurrencesTableSummary.tsx`:
- Around line 142-147: The time column definition in OccurrencesTableSummary.tsx
(the column object with id: 'time' and accessorFn that uses isMultidayOccurrence
and formatTimeRange) is missing a header label; add a header property (e.g.,
header: 'Time' or a localized string) to that column object so the table
displays a proper column title for the time values.
---
Outside diff comments:
In `@package.json`:
- Line 62: The dependency "eslint-plugin-react-hooks" in package.json is pinned
to ^5.2.0 and needs upgrading to the latest 7.x (>=7.1.1); update the version
string for "eslint-plugin-react-hooks" in package.json to the current 7.x
release (for example "7.1.1" or the latest published 7.x), then run your package
manager (npm/yarn/pnpm) to install and verify linting still passes; ensure any
lockfile (package-lock.json / yarn.lock / pnpm-lock.yaml) is updated
accordingly.
In `@src/clients/apiReportClient/useReportClientQuery.ts`:
- Around line 9-16: The downloadFile function creates an object URL (const url)
but never releases it; update downloadFile to revoke the created object URL
after triggering the download (e.g., call window.URL.revokeObjectURL(url) after
link.click(), or schedule revocation with setTimeout to ensure the download
starts before revoking) and keep the existing cleanup of the link element;
reference downloadFile, url and link when making the change.
- Around line 24-48: Wrap the apiReportClient.get call in a try/catch inside
useReportClientQuery's async handler so network/non-2xx errors are caught and
the user still receives feedback; on catch call addNotification with the same
error label (t('eventEnrolmentsReport.downloadError')) and optionally log the
error, and only proceed to check response.status/response.data.type and call
downloadFile(response.data, filename) when the request succeeds. Ensure you
reference the existing symbols apiReportClient.get, downloadFile,
addNotification, and t in your changes.
---
Nitpick comments:
In `@src/domain/app/apollo/__tests__/apolloClient.test.ts`:
- Around line 22-25: Add a regression test that verifies initializeApolloClient
updates the notification callback when the singleton client is reused: call
initializeApolloClient once with callbackA (vi.fn()), then call it again with
callbackB (vi.fn()), assert the returned client is the same instance as the
first (to exercise the reuse path), and then assert that the active notification
handler is callbackB (e.g., by triggering whatever notification dispatch/update
method the client exposes or by checking the stored callback reference on the
client). Ensure you reference initializeApolloClient and the two mock callbacks
(callbackA, callbackB) in the test so the singleton path refresh behavior is
validated.
In `@src/domain/occurrence/enrolmentTable/enrolmentModals/EnrolmentModal.tsx`:
- Around line 104-113: The modal currently uses a hardcoded id ('alert-modal')
causing duplicate id/aria attributes when multiple EnrolmentModal instances
mount; update the EnrolmentModal component to generate per-instance IDs (e.g.,
React's useId() or a stable UUID) and derive titleId and descriptionId from that
instance id, then pass those instance-scoped id, aria-labelledby and
aria-describedby values into the Dialog props; update references to id, titleId
and descriptionId in the component (the variables defined around the Dialog
render) so each modal instance has unique ARIA wiring.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d41a8076-5d13-40f6-bc21-944078dc6a74
⛔ Files ignored due to path filters (17)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlsrc/common/components/deleteButton/__tests__/__snapshots__/DeleteButton.test.tsx.snapis excluded by!**/*.snapsrc/common/components/fieldLabel/__tests__/__snapshots__/FieldLabel.test.tsx.snapis excluded by!**/*.snapsrc/domain/app/__tests__/__snapshots__/apolloRHHC.test.tsx.snapis excluded by!**/*.snapsrc/domain/app/footer/__tests__/__snapshots__/Footer.test.tsx.snapis excluded by!**/*.snapsrc/domain/app/header/__tests__/__snapshots__/Header.test.tsx.snapis excluded by!**/*.snapsrc/domain/app/layout/__tests__/__snapshots__/PageLayout.test.tsx.snapis excluded by!**/*.snapsrc/domain/errorPage/__tests__/__snapshots__/ErrorPage.test.tsx.snapis excluded by!**/*.snapsrc/domain/event/eventCard/__tests__/__snapshots__/EventCard.test.tsx.snapis excluded by!**/*.snapsrc/domain/event/eventPreviewCard/__tests__/__snapshots__/EventPreviewCard.test.tsx.snapis excluded by!**/*.snapsrc/domain/notFound/__tests__/__snapshots__/NotFoundPage.test.tsx.snapis excluded by!**/*.snapsrc/domain/occurrence/enrolmentDetails/__tests__/__snapshots__/EnrolmentDetails.test.tsx.snapis excluded by!**/*.snapsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/__snapshots__/ApproveEnrolmentModal.test.tsx.snapis excluded by!**/*.snapsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/__snapshots__/DeclineEnrolmentModal.test.tsx.snapis excluded by!**/*.snapsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/__snapshots__/DeleteEnrolmentModal.test.tsx.snapis excluded by!**/*.snapsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/__snapshots__/EnrolmentModal.test.tsx.snapis excluded by!**/*.snapsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/__snapshots__/PickQueueEnrolmentModal.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (103)
.github/workflows/ci.ymlpackage.jsonsrc/clients/apiReportClient/useReportClientQuery.tssrc/common/components/autoSuggest/AutoSuggest.tsxsrc/common/components/autoSuggest/__test__/AutoSuggest.test.tsxsrc/common/components/imageInput/ImageInput.tsxsrc/common/components/menuDropdown/MenuDropdown.tsxsrc/common/components/menuDropdown/__tests__/MenuDropdown.test.tsxsrc/common/components/menuDropdown/menuDropdown.module.scsssrc/common/components/modal/AlertModal.tsxsrc/common/components/modal/ConfirmationModal.tsxsrc/common/components/modal/Modal.tsxsrc/common/components/modal/NotificationModal.tsxsrc/common/components/modal/confirmModal.modulesrc/common/components/modal/modal.module.scsssrc/common/components/notificationsContext/NotificationsContext.tsxsrc/common/components/notificationsContext/__tests__/NotificationsContext.test.tsxsrc/common/components/notificationsContext/hooks/useNotificationsContext.tssrc/common/components/resetFocus/ResetFocus.tsxsrc/common/components/skeletonLoader/SkeletonLoader.tsxsrc/common/components/table/Table.tsxsrc/common/components/table/types.tssrc/domain/app/App.tsxsrc/domain/app/__tests__/apolloRHHC.test.tsxsrc/domain/app/apollo/__tests__/apolloClient.test.tssrc/domain/app/apollo/apolloClient.tssrc/domain/app/footer/Footer.tsxsrc/domain/app/header/Header.tsxsrc/domain/app/header/HeaderLink.tsxsrc/domain/app/header/__tests__/Header.test.tsxsrc/domain/app/routes/__tests__/AppRoutes.test.tsxsrc/domain/auth/KultusAdminHDSLoginCallbackHandler.tsxsrc/domain/auth/constants.tssrc/domain/auth/useAuthorization.tssrc/domain/auth/useIsAuthorizationReady.tssrc/domain/enrolment/EditEnrolmentPage.tsxsrc/domain/enrolment/enrolmentForm/enrolmentForm.module.scsssrc/domain/event/CreateEventPage.tsxsrc/domain/event/EditEventPage.tsxsrc/domain/event/EventSummaryPage.tsxsrc/domain/event/__tests__/EventDetailsPage.test.tsxsrc/domain/event/__tests__/EventSummaryPage.test.tsxsrc/domain/event/eventForm/eventForm.module.scsssrc/domain/event/eventForm/useEventFormSubmitRequests.tsxsrc/domain/event/eventPreviewBasicInfo/eventPreviewBasicInfo.module.scsssrc/domain/event/eventPreviewPage.module.scsssrc/domain/event/eventPublish/EventPublish.tsxsrc/domain/event/types.tssrc/domain/events/__tests__/EventsPage.test.tsxsrc/domain/events/utils.tssrc/domain/image/utils.tssrc/domain/keyword/KeywordSelector.tsxsrc/domain/languages/constants.tssrc/domain/myProfile/CreateMyProfile.tsxsrc/domain/myProfile/MyProfilePage.tsxsrc/domain/occurrence/CreateOccurrencePage.tsxsrc/domain/occurrence/__tests__/CreateOccurrencePageEnrolmentInfo.test.tsxsrc/domain/occurrence/__tests__/CreateOccurrencePageEnrolmentType.test.tsxsrc/domain/occurrence/__tests__/CreateOccurrencePageSimultaneousSave.test.tsxsrc/domain/occurrence/__tests__/CreateOccurrencePageVenueInfo.test.tsxsrc/domain/occurrence/enrolmentDetails/EnrolmentDetails.tsxsrc/domain/occurrence/enrolmentDetails/__tests__/EnrolmentDetails.test.tsxsrc/domain/occurrence/enrolmentDetails/enrolmentDetails.module.scsssrc/domain/occurrence/enrolmentTable/EnrolmentQueueTable.tsxsrc/domain/occurrence/enrolmentTable/EnrolmentTable.tsxsrc/domain/occurrence/enrolmentTable/actionsDropdown/ActionsDropdown.tsxsrc/domain/occurrence/enrolmentTable/actionsDropdown/QueueActionsDropdown.tsxsrc/domain/occurrence/enrolmentTable/actionsDropdown/__tests__/ActionsDropdown.test.tsxsrc/domain/occurrence/enrolmentTable/actionsDropdown/__tests__/QueueActionsDropdown.test.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/ApproveEnrolmentModal.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/DeclineEnrolmentModal.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/DeleteEnrolmentModal.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/EnrolmentModal.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/PickQueueEnrolmentModal.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/ApproveEnrolmentModal.test.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/DeclineEnrolmentModal.test.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/DeleteEnrolmentModal.test.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/EnrolmentModal.test.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/PickQueueEnrolmentModal.test.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/enrolmentModals.module.scsssrc/domain/occurrence/occurrencesFormPart/OccurrencesFormPart.tsxsrc/domain/occurrence/occurrencesFormPart/occurrencesFormPart.module.scsssrc/domain/occurrences/occurrencesTable/ActionsDropdown.tsxsrc/domain/occurrences/occurrencesTable/CancelOccurrenceModal.tsxsrc/domain/occurrences/occurrencesTable/OccurrencesTable.tsxsrc/domain/occurrences/occurrencesTable/__tests__/ActionsDropdown.test.tsxsrc/domain/occurrences/occurrencesTableReadOnly/OccurrencesTableSummary.tsxsrc/domain/venue/types.tssrc/headless-cms/__tests__/services.test.tssrc/headless-cms/apollo/apolloClient.tssrc/headless-cms/components/CmsPageContent.tsxsrc/headless-cms/components/__tests__/CmsPage.test.tsxsrc/headless-cms/components/cmsSidebarContent/CmsSidebarContentLayoutLinkList.tsxsrc/hooks/useRHHCConfig.tsxsrc/index.tsxsrc/test/apollo-mocks/footerMenuMock.tssrc/test/apollo-mocks/headerMenuMock.tssrc/test/apollo-mocks/languagesMock.tssrc/test/apollo-mocks/types.tssrc/utils/__tests__/getLanguageCode.test.tssrc/utils/getLanguageCode.tssrc/utils/testUtils.tsxtsconfig.json
💤 Files with no reviewable changes (20)
- src/common/components/menuDropdown/tests/MenuDropdown.test.tsx
- src/domain/enrolment/enrolmentForm/enrolmentForm.module.scss
- src/domain/auth/useIsAuthorizationReady.ts
- src/common/components/menuDropdown/menuDropdown.module.scss
- src/common/components/skeletonLoader/SkeletonLoader.tsx
- src/domain/event/eventPreviewBasicInfo/eventPreviewBasicInfo.module.scss
- src/domain/app/header/HeaderLink.tsx
- src/common/components/modal/Modal.tsx
- src/domain/auth/constants.ts
- src/domain/venue/types.ts
- src/domain/occurrence/enrolmentDetails/enrolmentDetails.module.scss
- src/common/components/menuDropdown/MenuDropdown.tsx
- src/common/components/modal/modal.module.scss
- src/domain/occurrence/enrolmentTable/enrolmentModals/PickQueueEnrolmentModal.tsx
- src/common/components/resetFocus/ResetFocus.tsx
- src/index.tsx
- src/domain/occurrence/occurrencesFormPart/occurrencesFormPart.module.scss
- src/domain/event/eventForm/eventForm.module.scss
- src/domain/occurrence/enrolmentTable/enrolmentModals/ApproveEnrolmentModal.tsx
- src/domain/auth/useAuthorization.ts
Refs: PT-2015
jorilindell
force-pushed
the
PT-2015-react-19
branch
from
10ce31c to
ddb0d1c
Compare
Contributor
Author
|
@CodeRabbit review again |
|
✅ Action performedFull review finished. |
|
KULTUS-ADMIN-UI branch is deployed to platta: https://kultus-admin-ui-pr475.dev.hel.ninja 🚀🚀🚀 |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
src/domain/occurrences/occurrencesTable/OccurrencesTable.tsx (1)
95-97:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse domain occurrence ID consistently in checkbox state.
Line 96 reads selection from
row.id, but Line 97 toggles usingrow.original.id; this can desync checked state.Suggested fix
- <Checkbox - id={`${id}_${row.id}_checkbox`} - checked={selectedOccurrences.includes(row.id)} + <Checkbox + id={`${id}_${row.original.id}_checkbox`} + checked={selectedOccurrences.includes(row.original.id)} onChange={() => handleCheckboxChange(row.original)}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain/occurrences/occurrencesTable/OccurrencesTable.tsx` around lines 95 - 97, The checkbox uses row.id for its checked state but toggles using row.original.id, which can desync selection; update the checkbox to consistently use the same occurrence identifier (either row.id or row.original.id) across the id attribute, checked prop and the onChange handler (adjust the onChange call to pass row.id to handleCheckboxChange or change checked to use row.original.id) so that id, checked and handleCheckboxChange all reference the same unique symbol (row.id or row.original.id).src/common/components/table/Table.tsx (2)
128-134:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard expanded-row colSpan against non-positive values.
Line 133 can produce invalid
colSpanwhenexpandedAreaOffsetis greater than or equal to visible cell count.Suggested fix
- {expandedAreaOffset > 0 && ( + {expandedAreaOffset > 0 && + expandedAreaOffset < row.getVisibleCells().length && ( <td colSpan={expandedAreaOffset}></td> )} <td - colSpan={ - row.getVisibleCells().length - expandedAreaOffset - } + colSpan={Math.max( + 1, + row.getVisibleCells().length - expandedAreaOffset + )} >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/common/components/table/Table.tsx` around lines 128 - 134, The colSpan calculation for the expanded-row <td> can become zero or negative when expandedAreaOffset >= row.getVisibleCells().length; update the render in Table.tsx to guard that value by computing a positive span (e.g., remaining = row.getVisibleCells().length - expandedAreaOffset and use Math.max(1, remaining)) or conditionally skip/render the <td> only when remaining > 0; adjust the JSX that sets colSpan (the <td colSpan={...}> near the expandedAreaOffset check) to use this guarded value so an invalid colSpan is never passed.
40-46:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winForward TanStack updater directly to state setter.
Line 40 still resolves updater functions against a closed-over
expandedvalue; this can lose queued expansion updates.Suggested fix
- onExpandedChange: (updaterOrValue) => { - const newValue = - typeof updaterOrValue === 'function' - ? updaterOrValue(expanded) - : updaterOrValue; - setExpanded(newValue); - }, + onExpandedChange: setExpanded,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/common/components/table/Table.tsx` around lines 40 - 46, The onExpandedChange handler resolves updater functions against a stale closed-over expanded value which can drop queued updates; change it to forward the incoming updaterOrValue directly to setExpanded so React/ TanStack can apply functional updates (i.e. call setExpanded with updaterOrValue when it's a function, or pass the value through) — update the onExpandedChange implementation referencing onExpandedChange, updaterOrValue, setExpanded, and expanded accordingly so state updates use the setter's functional form instead of reading the closed-over expanded variable.src/domain/occurrences/occurrencesTableReadOnly/OccurrencesTableSummary.tsx (1)
142-147:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a header label for the time column.
Line 142 defines the time accessor but the column still has no header text.
Suggested fix
{ + header: t('occurrences.table.columnTime'), accessorFn: (row) => isMultidayOccurrence(row) ? null : formatTimeRange(new Date(row.startTime), new Date(row.endTime)), id: 'time', },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain/occurrences/occurrencesTableReadOnly/OccurrencesTableSummary.tsx` around lines 142 - 147, The time column lacks a visible header; in the column definition inside OccurrencesTableSummary (the object with id: 'time' and accessorFn using isMultidayOccurrence and formatTimeRange), add a header property (for example header: 'Time' or header: () => 'Time') to provide the column label so the table shows "Time" above the values.
🧹 Nitpick comments (2)
package.json (1)
75-76: Bumpeslint-plugin-react-hooksalongside the React 19.2 upgrade (for rule/preset currency).
package.jsonpinseslint-plugin-react-hooksto^5.2.0whilereact/react-domare upgraded to^19.2.5;eslint.config.mjsconsumesreactHooksPlugin.configs.recommended.rules. React 19 doesn’t require exactly v6+, but upgrading to the latest v7+ keeps hook-related lint rules/presets aligned with modern React/ESLint behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` around lines 75 - 76, Update the pinned eslint-plugin-react-hooks dependency to a current v7+ release in package.json so hook lint rules stay aligned with React 19; change the version spec for "eslint-plugin-react-hooks" (the dependency referenced by eslint.config.mjs via reactHooksPlugin.configs.recommended.rules) to a modern 7.x+ range, run npm/yarn install to update lockfile, and verify linting still passes against eslint.config.mjs's hooks config.src/common/components/notificationsContext/__tests__/NotificationsContext.test.tsx (1)
7-7: Avoid globalconfigure({ defaultHidden: true })in this test
src/common/components/notificationsContext/__tests__/NotificationsContext.test.tsxsets:configure({ defaultHidden: true });…but the
NotificationsProviderrendershds-react<Notification />without any hidden/aria-hidden handling, and the role queries at lines 27–29 and 52–56 look forrole="alert"items without needing hidden-elements lookup. Remove the globaldefaultHidden(or, if hidden elements are ever required, pass{ hidden: true }on the specific queries).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/common/components/notificationsContext/__tests__/NotificationsContext.test.tsx` at line 7, Remove the global testing-library configuration call configure({ defaultHidden: true }) in the NotificationsContext.test.tsx file; instead, let the NotificationsProvider render hds-react <Notification /> normally (so role="alert" queries in the tests find visible alerts), and only use the { hidden: true } option on individual getByRole/queryByRole calls if you later need to target hidden elements—locate the configure() call and delete it or replace it with per-query hidden options where necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/common/components/autoSuggest/AutoSuggest.tsx`:
- Around line 96-100: The optionLabelToString callback is being called from the
AutoSuggest render path (used for the multi-value deselect button aria-label)
and must be pure and side-effect free; remove the third parameter
(addNotification) from the optionLabelToString signature and stop passing
addNotification when calling optionLabelToString in AutoSuggest (keep only
option and locale), then update all implementations (e.g., UnitPlaceSelector,
PlaceSelector, KeywordSelector) to stop accepting/using the third arg (move any
notification/error handling out of formatting into async logic or error
handlers) and update the TypeScript type for optionLabelToString to the
two-argument form so the contract is enforced.
In `@src/common/components/modal/AlertModal.tsx`:
- Line 1: The file AlertModal.tsx uses React types (React.FC and React.ReactNode
for the children prop) but doesn't import React, causing TS errors; add the
appropriate React type import (e.g. import React from 'react' or import type {
FC, ReactNode } from 'react') and update the component signature (AlertModal /
props using React.FC and children: React.ReactNode) to use the imported types,
then run the typechecker to verify.
In `@src/common/components/notificationsContext/NotificationsContext.tsx`:
- Around line 58-66: The stacking math currently reads DOM heights inside the
useMemo "heights" calculation before new notification nodes are committed,
producing zeros and causing overlaps; change this to track heights in local
state measured in a useLayoutEffect after render: create a heights state (e.g.,
[heights, setHeights]), remove the DOM-measuring useMemo, and in useLayoutEffect
iterate notifications and call document.getElementById(id)?.clientHeight to
populate setHeights so measurements happen post-commit; then use that heights
state when computing per-notification top offsets (the same place that
previously referenced the useMemo "heights") so offsets are accurate on batched
adds.
In `@src/domain/occurrence/CreateOccurrencePage.tsx`:
- Around line 455-459: Await the Apollo refetch Promise to ensure eventDataRef
is updated before continuing the save-to-publishing flow: in
handleSaveEventInfo, change the refetchEvent() call to await refetchEvent()
before calling addNotification, and in handleGoToPublishingClick ensure you
await submitForm() and then await refetchEvent() (or call the awaited
handleSaveEventInfo) so that eventDataRef.current?.event.pEvent.occurrences
reflects the refreshed data before running the publishing validation; reference
refetchEvent, handleSaveEventInfo, handleGoToPublishingClick, and eventDataRef
to locate the edits.
---
Duplicate comments:
In `@src/common/components/table/Table.tsx`:
- Around line 128-134: The colSpan calculation for the expanded-row <td> can
become zero or negative when expandedAreaOffset >= row.getVisibleCells().length;
update the render in Table.tsx to guard that value by computing a positive span
(e.g., remaining = row.getVisibleCells().length - expandedAreaOffset and use
Math.max(1, remaining)) or conditionally skip/render the <td> only when
remaining > 0; adjust the JSX that sets colSpan (the <td colSpan={...}> near the
expandedAreaOffset check) to use this guarded value so an invalid colSpan is
never passed.
- Around line 40-46: The onExpandedChange handler resolves updater functions
against a stale closed-over expanded value which can drop queued updates; change
it to forward the incoming updaterOrValue directly to setExpanded so React/
TanStack can apply functional updates (i.e. call setExpanded with updaterOrValue
when it's a function, or pass the value through) — update the onExpandedChange
implementation referencing onExpandedChange, updaterOrValue, setExpanded, and
expanded accordingly so state updates use the setter's functional form instead
of reading the closed-over expanded variable.
In `@src/domain/occurrences/occurrencesTable/OccurrencesTable.tsx`:
- Around line 95-97: The checkbox uses row.id for its checked state but toggles
using row.original.id, which can desync selection; update the checkbox to
consistently use the same occurrence identifier (either row.id or
row.original.id) across the id attribute, checked prop and the onChange handler
(adjust the onChange call to pass row.id to handleCheckboxChange or change
checked to use row.original.id) so that id, checked and handleCheckboxChange all
reference the same unique symbol (row.id or row.original.id).
In `@src/domain/occurrences/occurrencesTableReadOnly/OccurrencesTableSummary.tsx`:
- Around line 142-147: The time column lacks a visible header; in the column
definition inside OccurrencesTableSummary (the object with id: 'time' and
accessorFn using isMultidayOccurrence and formatTimeRange), add a header
property (for example header: 'Time' or header: () => 'Time') to provide the
column label so the table shows "Time" above the values.
---
Nitpick comments:
In `@package.json`:
- Around line 75-76: Update the pinned eslint-plugin-react-hooks dependency to a
current v7+ release in package.json so hook lint rules stay aligned with React
19; change the version spec for "eslint-plugin-react-hooks" (the dependency
referenced by eslint.config.mjs via reactHooksPlugin.configs.recommended.rules)
to a modern 7.x+ range, run npm/yarn install to update lockfile, and verify
linting still passes against eslint.config.mjs's hooks config.
In
`@src/common/components/notificationsContext/__tests__/NotificationsContext.test.tsx`:
- Line 7: Remove the global testing-library configuration call configure({
defaultHidden: true }) in the NotificationsContext.test.tsx file; instead, let
the NotificationsProvider render hds-react <Notification /> normally (so
role="alert" queries in the tests find visible alerts), and only use the {
hidden: true } option on individual getByRole/queryByRole calls if you later
need to target hidden elements—locate the configure() call and delete it or
replace it with per-query hidden options where necessary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0e5b66da-94d7-4833-8661-b4616b926310
⛔ Files ignored due to path filters (17)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlsrc/common/components/deleteButton/__tests__/__snapshots__/DeleteButton.test.tsx.snapis excluded by!**/*.snapsrc/common/components/fieldLabel/__tests__/__snapshots__/FieldLabel.test.tsx.snapis excluded by!**/*.snapsrc/domain/app/__tests__/__snapshots__/apolloRHHC.test.tsx.snapis excluded by!**/*.snapsrc/domain/app/footer/__tests__/__snapshots__/Footer.test.tsx.snapis excluded by!**/*.snapsrc/domain/app/header/__tests__/__snapshots__/Header.test.tsx.snapis excluded by!**/*.snapsrc/domain/app/layout/__tests__/__snapshots__/PageLayout.test.tsx.snapis excluded by!**/*.snapsrc/domain/errorPage/__tests__/__snapshots__/ErrorPage.test.tsx.snapis excluded by!**/*.snapsrc/domain/event/eventCard/__tests__/__snapshots__/EventCard.test.tsx.snapis excluded by!**/*.snapsrc/domain/event/eventPreviewCard/__tests__/__snapshots__/EventPreviewCard.test.tsx.snapis excluded by!**/*.snapsrc/domain/notFound/__tests__/__snapshots__/NotFoundPage.test.tsx.snapis excluded by!**/*.snapsrc/domain/occurrence/enrolmentDetails/__tests__/__snapshots__/EnrolmentDetails.test.tsx.snapis excluded by!**/*.snapsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/__snapshots__/ApproveEnrolmentModal.test.tsx.snapis excluded by!**/*.snapsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/__snapshots__/DeclineEnrolmentModal.test.tsx.snapis excluded by!**/*.snapsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/__snapshots__/DeleteEnrolmentModal.test.tsx.snapis excluded by!**/*.snapsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/__snapshots__/EnrolmentModal.test.tsx.snapis excluded by!**/*.snapsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/__snapshots__/PickQueueEnrolmentModal.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (104)
.github/workflows/ci.ymlpackage.jsonsrc/clients/apiReportClient/useReportClientQuery.tssrc/common/components/autoSuggest/AutoSuggest.tsxsrc/common/components/autoSuggest/__test__/AutoSuggest.test.tsxsrc/common/components/imageInput/ImageInput.tsxsrc/common/components/menuDropdown/MenuDropdown.tsxsrc/common/components/menuDropdown/__tests__/MenuDropdown.test.tsxsrc/common/components/menuDropdown/menuDropdown.module.scsssrc/common/components/modal/AlertModal.tsxsrc/common/components/modal/BaseDialogModal.tsxsrc/common/components/modal/ConfirmationModal.tsxsrc/common/components/modal/Modal.tsxsrc/common/components/modal/NotificationModal.tsxsrc/common/components/modal/confirmModal.modulesrc/common/components/modal/modal.module.scsssrc/common/components/notificationsContext/NotificationsContext.tsxsrc/common/components/notificationsContext/__tests__/NotificationsContext.test.tsxsrc/common/components/notificationsContext/hooks/useNotificationsContext.tssrc/common/components/resetFocus/ResetFocus.tsxsrc/common/components/skeletonLoader/SkeletonLoader.tsxsrc/common/components/table/Table.tsxsrc/common/components/table/types.tssrc/domain/app/App.tsxsrc/domain/app/__tests__/apolloRHHC.test.tsxsrc/domain/app/apollo/__tests__/apolloClient.test.tssrc/domain/app/apollo/apolloClient.tssrc/domain/app/footer/Footer.tsxsrc/domain/app/header/Header.tsxsrc/domain/app/header/HeaderLink.tsxsrc/domain/app/header/__tests__/Header.test.tsxsrc/domain/app/routes/__tests__/AppRoutes.test.tsxsrc/domain/auth/KultusAdminHDSLoginCallbackHandler.tsxsrc/domain/auth/constants.tssrc/domain/auth/useAuthorization.tssrc/domain/auth/useIsAuthorizationReady.tssrc/domain/enrolment/EditEnrolmentPage.tsxsrc/domain/enrolment/enrolmentForm/enrolmentForm.module.scsssrc/domain/event/CreateEventPage.tsxsrc/domain/event/EditEventPage.tsxsrc/domain/event/EventSummaryPage.tsxsrc/domain/event/__tests__/EventDetailsPage.test.tsxsrc/domain/event/__tests__/EventSummaryPage.test.tsxsrc/domain/event/eventForm/eventForm.module.scsssrc/domain/event/eventForm/useEventFormSubmitRequests.tsxsrc/domain/event/eventPreviewBasicInfo/eventPreviewBasicInfo.module.scsssrc/domain/event/eventPreviewPage.module.scsssrc/domain/event/eventPublish/EventPublish.tsxsrc/domain/event/types.tssrc/domain/events/__tests__/EventsPage.test.tsxsrc/domain/events/utils.tssrc/domain/image/utils.tssrc/domain/keyword/KeywordSelector.tsxsrc/domain/languages/constants.tssrc/domain/myProfile/CreateMyProfile.tsxsrc/domain/myProfile/MyProfilePage.tsxsrc/domain/occurrence/CreateOccurrencePage.tsxsrc/domain/occurrence/__tests__/CreateOccurrencePageEnrolmentInfo.test.tsxsrc/domain/occurrence/__tests__/CreateOccurrencePageEnrolmentType.test.tsxsrc/domain/occurrence/__tests__/CreateOccurrencePageSimultaneousSave.test.tsxsrc/domain/occurrence/__tests__/CreateOccurrencePageVenueInfo.test.tsxsrc/domain/occurrence/enrolmentDetails/EnrolmentDetails.tsxsrc/domain/occurrence/enrolmentDetails/__tests__/EnrolmentDetails.test.tsxsrc/domain/occurrence/enrolmentDetails/enrolmentDetails.module.scsssrc/domain/occurrence/enrolmentTable/EnrolmentQueueTable.tsxsrc/domain/occurrence/enrolmentTable/EnrolmentTable.tsxsrc/domain/occurrence/enrolmentTable/actionsDropdown/ActionsDropdown.tsxsrc/domain/occurrence/enrolmentTable/actionsDropdown/QueueActionsDropdown.tsxsrc/domain/occurrence/enrolmentTable/actionsDropdown/__tests__/ActionsDropdown.test.tsxsrc/domain/occurrence/enrolmentTable/actionsDropdown/__tests__/QueueActionsDropdown.test.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/ApproveEnrolmentModal.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/DeclineEnrolmentModal.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/DeleteEnrolmentModal.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/EnrolmentModal.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/PickQueueEnrolmentModal.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/ApproveEnrolmentModal.test.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/DeclineEnrolmentModal.test.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/DeleteEnrolmentModal.test.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/EnrolmentModal.test.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/PickQueueEnrolmentModal.test.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/enrolmentModals.module.scsssrc/domain/occurrence/occurrencesFormPart/OccurrencesFormPart.tsxsrc/domain/occurrence/occurrencesFormPart/occurrencesFormPart.module.scsssrc/domain/occurrences/occurrencesTable/ActionsDropdown.tsxsrc/domain/occurrences/occurrencesTable/CancelOccurrenceModal.tsxsrc/domain/occurrences/occurrencesTable/OccurrencesTable.tsxsrc/domain/occurrences/occurrencesTable/__tests__/ActionsDropdown.test.tsxsrc/domain/occurrences/occurrencesTableReadOnly/OccurrencesTableSummary.tsxsrc/domain/venue/types.tssrc/headless-cms/__tests__/services.test.tssrc/headless-cms/apollo/apolloClient.tssrc/headless-cms/components/CmsPageContent.tsxsrc/headless-cms/components/__tests__/CmsPage.test.tsxsrc/headless-cms/components/cmsSidebarContent/CmsSidebarContentLayoutLinkList.tsxsrc/hooks/useRHHCConfig.tsxsrc/index.tsxsrc/test/apollo-mocks/footerMenuMock.tssrc/test/apollo-mocks/headerMenuMock.tssrc/test/apollo-mocks/languagesMock.tssrc/test/apollo-mocks/types.tssrc/utils/__tests__/getLanguageCode.test.tssrc/utils/getLanguageCode.tssrc/utils/testUtils.tsxtsconfig.json
💤 Files with no reviewable changes (20)
- src/domain/app/header/HeaderLink.tsx
- src/domain/enrolment/enrolmentForm/enrolmentForm.module.scss
- src/domain/occurrence/occurrencesFormPart/occurrencesFormPart.module.scss
- src/index.tsx
- src/common/components/modal/Modal.tsx
- src/domain/auth/useAuthorization.ts
- src/common/components/skeletonLoader/SkeletonLoader.tsx
- src/common/components/menuDropdown/MenuDropdown.tsx
- src/domain/auth/constants.ts
- src/common/components/menuDropdown/menuDropdown.module.scss
- src/common/components/modal/modal.module.scss
- src/common/components/menuDropdown/tests/MenuDropdown.test.tsx
- src/common/components/resetFocus/ResetFocus.tsx
- src/domain/auth/useIsAuthorizationReady.ts
- src/domain/occurrence/enrolmentTable/enrolmentModals/ApproveEnrolmentModal.tsx
- src/domain/event/eventPreviewBasicInfo/eventPreviewBasicInfo.module.scss
- src/domain/venue/types.ts
- src/domain/event/eventForm/eventForm.module.scss
- src/domain/occurrence/enrolmentDetails/enrolmentDetails.module.scss
- src/domain/occurrence/enrolmentTable/enrolmentModals/PickQueueEnrolmentModal.tsx
Refs: PT-2015
jorilindell
force-pushed
the
PT-2015-react-19
branch
from
ddb0d1c to
48d564a
Compare
Contributor
Author
|
@CodeRabbit review again |
|
✅ Action performedFull review finished. |
|
KULTUS-ADMIN-UI branch is deployed to platta: https://kultus-admin-ui-pr475.dev.hel.ninja 🚀🚀🚀 |
There was a problem hiding this comment.
♻️ Duplicate comments (5)
src/domain/occurrences/occurrencesTableReadOnly/OccurrencesTableSummary.tsx (1)
141-147:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThe time column is missing a
headerproperty.This leaves an unlabeled header cell in the table, which impacts accessibility and user experience. This was noted in a previous review and appears to still be unaddressed.
Suggested fix
{ + header: t('occurrences.table.columnTime'), accessorFn: (row) => isMultidayOccurrence(row) ? null : formatTimeRange(new Date(row.startTime), new Date(row.endTime)), id: 'time', },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain/occurrences/occurrencesTableReadOnly/OccurrencesTableSummary.tsx` around lines 141 - 147, The time column definition is missing a header, leaving the table header cell unlabeled; update the column object (the entry with id: 'time', accessorFn, isMultidayOccurrence and formatTimeRange) to include a descriptive header property (e.g., header: 'Time' or a localized string) so the column displays an accessible label in the table header.src/domain/app/apollo/apolloClient.ts (1)
30-34:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRefresh the notification callback even when reusing the singleton Apollo client.
The current setup captures the first
addNotificationfunction in the error link; laterinitializeApolloClient(...)calls cannot update it, so notifications can route to a stale/no-op handler.Proposed minimal fix
+let notify: (props: NotificationProps) => void = () => {}; + function createApolloClient({ - addNotification, -}: { - addNotification: (props: NotificationProps) => void; -}): ApolloClient<NormalizedCacheObject> { +}: {}): ApolloClient<NormalizedCacheObject> { @@ case 'PERMISSION_DENIED_ERROR': - addNotification({ + notify({ label: i18n.t('apollo.graphQLErrors.permissionDeniedError'), type: 'error', }); break; @@ export function initializeApolloClient( initialState: NormalizedCacheObject | null = null, addNotification: (props: NotificationProps) => void ): ApolloClient<NormalizedCacheObject> { - const _apolloClient = apolloClient ?? createApolloClient({ addNotification }); + notify = addNotification; + const _apolloClient = apolloClient ?? createApolloClient({});Also applies to: 50-53, 106-110
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain/app/apollo/apolloClient.ts` around lines 30 - 34, The error link currently closes over the initial addNotification callback so subsequent initializeApolloClient calls cannot update it; modify createApolloClient/initializeApolloClient to store addNotification in a mutable reference (e.g., a module-scoped let or an object like notificationRef.current) and have the error link call that reference instead of the captured function; ensure initializeApolloClient updates that reference whenever it's called so the error link routes notifications to the latest addNotification handler (update usages in createApolloClient, the error link factory, and initializeApolloClient to read/update the shared reference).src/common/components/notificationsContext/__tests__/NotificationsContext.test.tsx (1)
36-42:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd dependency array to Line 36
useEffectto prevent repeated enqueueing.Without dependencies, the effect reruns after each render and keeps adding notifications, which destabilizes this test.
Proposed fix
useEffect(() => { addNotification({ autoCloseDuration: 50, label: 'Notification 1', type: 'success', }); - }); + }, [addNotification]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/common/components/notificationsContext/__tests__/NotificationsContext.test.tsx` around lines 36 - 42, The test's useEffect (in NotificationsContext.test.tsx) currently runs on every render and repeatedly calls addNotification; change it to run only once by adding a dependency array—preferably [addNotification] (or [] if addNotification is guaranteed stable/mocked) to the useEffect invocation so the notification is enqueued a single time during the test.src/common/components/notificationsContext/NotificationsContext.tsx (1)
58-66:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix pre-commit height measurement; stacked notifications can overlap.
At Line 58, heights are read before new notifications are committed to the DOM, so batched adds get
0heights and offsets at Line 80 become too small.Proposed fix
import React, { CSSProperties, createContext, FC, PropsWithChildren, useCallback, + useLayoutEffect, useMemo, useState, } from 'react'; @@ export const NotificationsProvider: FC<PropsWithChildren> = ({ children }) => { const [notifications, setNotifications] = useState<NotificationProps[]>([]); + const [heights, setHeights] = useState<number[]>([]); @@ - const heights = useMemo( - () => - // Height of last added notification is 0 because it's not rendered yet. - // In this case it doesn't matter because it's not used for top-margin calculation - notifications.map( - ({ id }) => document.getElementById(id as string)?.clientHeight ?? 0 - ), - [notifications] - ); + useLayoutEffect(() => { + setHeights( + notifications.map( + ({ id }) => document.getElementById(id as string)?.clientHeight ?? 0 + ) + ); + }, [notifications]);Also applies to: 80-80
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/common/components/notificationsContext/NotificationsContext.tsx` around lines 58 - 66, The current heights computed in the useMemo run before newly added notifications are committed, producing 0 for recent items and causing overlapping — replace the synchronous useMemo measurement with a post-render measurement: create a heights state (e.g. heightsState) and populate it inside a useLayoutEffect (or effect with requestAnimationFrame) that runs when notifications change, iterate notifications by id and read document.getElementById(id)?.clientHeight ?? 0 into that state, and then use that heights state (instead of the old heights useMemo) for the offset/top-margin calculation (the code referencing heights and the offset computation in NotificationsContext).src/common/components/modal/AlertModal.tsx (1)
1-1:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winImport React types (or stop using
React.*typings).
AlertModal.tsxusesReact.ReactNodeon Line 12 andReact.FCon Line 15, but there is noreactimport in this module. This can fail TS type-check in module mode.Proposed minimal fix
import { Button, ButtonVariant } from 'hds-react'; +import type { FC, ReactNode } from 'react'; import { useTranslation } from 'react-i18next'; @@ - children?: React.ReactNode; + children?: ReactNode; } -const AlertModal: React.FC<Props> = ({ +const AlertModal: FC<Props> = ({#!/bin/bash set -euo pipefail FILE="src/common/components/modal/AlertModal.tsx" echo "== Relevant lines ==" nl -ba "$FILE" | sed -n '1,80p' echo echo "== React namespace usages in file ==" rg -n 'React\.(FC|ReactNode|ComponentProps|useId)' "$FILE" || true echo echo "== React imports in file ==" rg -n "^import .* from 'react'|^import type .* from 'react'" "$FILE" || true echo echo "== tsconfig allowUmdGlobalAccess settings (if any) ==" fd 'tsconfig.*json' | xargs -r rg -n '"allowUmdGlobalAccess"\s*:'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/common/components/modal/AlertModal.tsx` at line 1, The file AlertModal.tsx uses React.ReactNode and React.FC but doesn't import React types; fix by adding a react type import or switching to direct named types: import type React from 'react' or import type { FC, ReactNode } from 'react', then update the component signature and prop types accordingly (referencing the React.ReactNode usage and the component declared as React.FC) so TypeScript module mode type-checking succeeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/common/components/modal/AlertModal.tsx`:
- Line 1: The file AlertModal.tsx uses React.ReactNode and React.FC but doesn't
import React types; fix by adding a react type import or switching to direct
named types: import type React from 'react' or import type { FC, ReactNode }
from 'react', then update the component signature and prop types accordingly
(referencing the React.ReactNode usage and the component declared as React.FC)
so TypeScript module mode type-checking succeeds.
In
`@src/common/components/notificationsContext/__tests__/NotificationsContext.test.tsx`:
- Around line 36-42: The test's useEffect (in NotificationsContext.test.tsx)
currently runs on every render and repeatedly calls addNotification; change it
to run only once by adding a dependency array—preferably [addNotification] (or
[] if addNotification is guaranteed stable/mocked) to the useEffect invocation
so the notification is enqueued a single time during the test.
In `@src/common/components/notificationsContext/NotificationsContext.tsx`:
- Around line 58-66: The current heights computed in the useMemo run before
newly added notifications are committed, producing 0 for recent items and
causing overlapping — replace the synchronous useMemo measurement with a
post-render measurement: create a heights state (e.g. heightsState) and populate
it inside a useLayoutEffect (or effect with requestAnimationFrame) that runs
when notifications change, iterate notifications by id and read
document.getElementById(id)?.clientHeight ?? 0 into that state, and then use
that heights state (instead of the old heights useMemo) for the
offset/top-margin calculation (the code referencing heights and the offset
computation in NotificationsContext).
In `@src/domain/app/apollo/apolloClient.ts`:
- Around line 30-34: The error link currently closes over the initial
addNotification callback so subsequent initializeApolloClient calls cannot
update it; modify createApolloClient/initializeApolloClient to store
addNotification in a mutable reference (e.g., a module-scoped let or an object
like notificationRef.current) and have the error link call that reference
instead of the captured function; ensure initializeApolloClient updates that
reference whenever it's called so the error link routes notifications to the
latest addNotification handler (update usages in createApolloClient, the error
link factory, and initializeApolloClient to read/update the shared reference).
In `@src/domain/occurrences/occurrencesTableReadOnly/OccurrencesTableSummary.tsx`:
- Around line 141-147: The time column definition is missing a header, leaving
the table header cell unlabeled; update the column object (the entry with id:
'time', accessorFn, isMultidayOccurrence and formatTimeRange) to include a
descriptive header property (e.g., header: 'Time' or a localized string) so the
column displays an accessible label in the table header.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be18aed7-7762-4e87-b7eb-8ac49f92267d
⛔ Files ignored due to path filters (17)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlsrc/common/components/deleteButton/__tests__/__snapshots__/DeleteButton.test.tsx.snapis excluded by!**/*.snapsrc/common/components/fieldLabel/__tests__/__snapshots__/FieldLabel.test.tsx.snapis excluded by!**/*.snapsrc/domain/app/__tests__/__snapshots__/apolloRHHC.test.tsx.snapis excluded by!**/*.snapsrc/domain/app/footer/__tests__/__snapshots__/Footer.test.tsx.snapis excluded by!**/*.snapsrc/domain/app/header/__tests__/__snapshots__/Header.test.tsx.snapis excluded by!**/*.snapsrc/domain/app/layout/__tests__/__snapshots__/PageLayout.test.tsx.snapis excluded by!**/*.snapsrc/domain/errorPage/__tests__/__snapshots__/ErrorPage.test.tsx.snapis excluded by!**/*.snapsrc/domain/event/eventCard/__tests__/__snapshots__/EventCard.test.tsx.snapis excluded by!**/*.snapsrc/domain/event/eventPreviewCard/__tests__/__snapshots__/EventPreviewCard.test.tsx.snapis excluded by!**/*.snapsrc/domain/notFound/__tests__/__snapshots__/NotFoundPage.test.tsx.snapis excluded by!**/*.snapsrc/domain/occurrence/enrolmentDetails/__tests__/__snapshots__/EnrolmentDetails.test.tsx.snapis excluded by!**/*.snapsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/__snapshots__/ApproveEnrolmentModal.test.tsx.snapis excluded by!**/*.snapsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/__snapshots__/DeclineEnrolmentModal.test.tsx.snapis excluded by!**/*.snapsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/__snapshots__/DeleteEnrolmentModal.test.tsx.snapis excluded by!**/*.snapsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/__snapshots__/EnrolmentModal.test.tsx.snapis excluded by!**/*.snapsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/__snapshots__/PickQueueEnrolmentModal.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (105)
.github/workflows/ci.ymlpackage.jsonsrc/clients/apiReportClient/useReportClientQuery.tssrc/common/components/autoSuggest/AutoSuggest.tsxsrc/common/components/autoSuggest/__test__/AutoSuggest.test.tsxsrc/common/components/imageInput/ImageInput.tsxsrc/common/components/menuDropdown/MenuDropdown.tsxsrc/common/components/menuDropdown/__tests__/MenuDropdown.test.tsxsrc/common/components/menuDropdown/menuDropdown.module.scsssrc/common/components/modal/AlertModal.tsxsrc/common/components/modal/BaseDialogModal.tsxsrc/common/components/modal/ConfirmationModal.tsxsrc/common/components/modal/Modal.tsxsrc/common/components/modal/NotificationModal.tsxsrc/common/components/modal/confirmModal.modulesrc/common/components/modal/modal.module.scsssrc/common/components/notificationsContext/NotificationsContext.tsxsrc/common/components/notificationsContext/__tests__/NotificationsContext.test.tsxsrc/common/components/notificationsContext/hooks/useNotificationsContext.tssrc/common/components/resetFocus/ResetFocus.tsxsrc/common/components/skeletonLoader/SkeletonLoader.tsxsrc/common/components/table/Table.tsxsrc/common/components/table/types.tssrc/domain/app/App.tsxsrc/domain/app/__tests__/apolloRHHC.test.tsxsrc/domain/app/apollo/__tests__/apolloClient.test.tssrc/domain/app/apollo/apolloClient.tssrc/domain/app/footer/Footer.tsxsrc/domain/app/header/Header.tsxsrc/domain/app/header/HeaderLink.tsxsrc/domain/app/header/__tests__/Header.test.tsxsrc/domain/app/routes/__tests__/AppRoutes.test.tsxsrc/domain/auth/KultusAdminHDSLoginCallbackHandler.tsxsrc/domain/auth/constants.tssrc/domain/auth/useAuthorization.tssrc/domain/auth/useIsAuthorizationReady.tssrc/domain/enrolment/EditEnrolmentPage.tsxsrc/domain/enrolment/enrolmentForm/enrolmentForm.module.scsssrc/domain/event/CreateEventPage.tsxsrc/domain/event/EditEventPage.tsxsrc/domain/event/EventSummaryPage.tsxsrc/domain/event/__tests__/EventDetailsPage.test.tsxsrc/domain/event/__tests__/EventSummaryPage.test.tsxsrc/domain/event/eventForm/eventForm.module.scsssrc/domain/event/eventForm/useEventFormSubmitRequests.tsxsrc/domain/event/eventPreviewBasicInfo/eventPreviewBasicInfo.module.scsssrc/domain/event/eventPreviewPage.module.scsssrc/domain/event/eventPublish/EventPublish.tsxsrc/domain/event/types.tssrc/domain/events/__tests__/EventsPage.test.tsxsrc/domain/events/utils.tssrc/domain/image/utils.tssrc/domain/keyword/KeywordSelector.tsxsrc/domain/languages/constants.tssrc/domain/myProfile/CreateMyProfile.tsxsrc/domain/myProfile/MyProfilePage.tsxsrc/domain/occurrence/CreateOccurrencePage.tsxsrc/domain/occurrence/__tests__/CreateOccurrencePageEnrolmentInfo.test.tsxsrc/domain/occurrence/__tests__/CreateOccurrencePageEnrolmentType.test.tsxsrc/domain/occurrence/__tests__/CreateOccurrencePageSimultaneousSave.test.tsxsrc/domain/occurrence/__tests__/CreateOccurrencePageVenueInfo.test.tsxsrc/domain/occurrence/enrolmentDetails/EnrolmentDetails.tsxsrc/domain/occurrence/enrolmentDetails/__tests__/EnrolmentDetails.test.tsxsrc/domain/occurrence/enrolmentDetails/enrolmentDetails.module.scsssrc/domain/occurrence/enrolmentTable/EnrolmentQueueTable.tsxsrc/domain/occurrence/enrolmentTable/EnrolmentTable.tsxsrc/domain/occurrence/enrolmentTable/actionsDropdown/ActionsDropdown.tsxsrc/domain/occurrence/enrolmentTable/actionsDropdown/QueueActionsDropdown.tsxsrc/domain/occurrence/enrolmentTable/actionsDropdown/__tests__/ActionsDropdown.test.tsxsrc/domain/occurrence/enrolmentTable/actionsDropdown/__tests__/QueueActionsDropdown.test.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/ApproveEnrolmentModal.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/DeclineEnrolmentModal.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/DeleteEnrolmentModal.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/EnrolmentModal.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/PickQueueEnrolmentModal.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/ApproveEnrolmentModal.test.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/DeclineEnrolmentModal.test.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/DeleteEnrolmentModal.test.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/EnrolmentModal.test.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/PickQueueEnrolmentModal.test.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/enrolmentModals.module.scsssrc/domain/occurrence/occurrencesFormPart/OccurrencesFormPart.tsxsrc/domain/occurrence/occurrencesFormPart/occurrencesFormPart.module.scsssrc/domain/occurrences/occurrencesTable/ActionsDropdown.tsxsrc/domain/occurrences/occurrencesTable/CancelOccurrenceModal.tsxsrc/domain/occurrences/occurrencesTable/OccurrencesTable.tsxsrc/domain/occurrences/occurrencesTable/__tests__/ActionsDropdown.test.tsxsrc/domain/occurrences/occurrencesTable/occurrencesTable.module.scsssrc/domain/occurrences/occurrencesTableReadOnly/OccurrencesTableSummary.tsxsrc/domain/venue/types.tssrc/headless-cms/__tests__/services.test.tssrc/headless-cms/apollo/apolloClient.tssrc/headless-cms/components/CmsPageContent.tsxsrc/headless-cms/components/__tests__/CmsPage.test.tsxsrc/headless-cms/components/cmsSidebarContent/CmsSidebarContentLayoutLinkList.tsxsrc/hooks/useRHHCConfig.tsxsrc/index.tsxsrc/test/apollo-mocks/footerMenuMock.tssrc/test/apollo-mocks/headerMenuMock.tssrc/test/apollo-mocks/languagesMock.tssrc/test/apollo-mocks/types.tssrc/utils/__tests__/getLanguageCode.test.tssrc/utils/getLanguageCode.tssrc/utils/testUtils.tsxtsconfig.json
💤 Files with no reviewable changes (22)
- src/domain/event/eventForm/eventForm.module.scss
- src/domain/occurrence/enrolmentDetails/enrolmentDetails.module.scss
- src/domain/occurrences/occurrencesTable/occurrencesTable.module.scss
- src/domain/occurrence/enrolmentTable/enrolmentModals/PickQueueEnrolmentModal.tsx
- src/domain/occurrence/occurrencesFormPart/occurrencesFormPart.module.scss
- src/domain/event/eventPreviewBasicInfo/eventPreviewBasicInfo.module.scss
- src/domain/venue/types.ts
- src/domain/occurrence/enrolmentTable/enrolmentModals/ApproveEnrolmentModal.tsx
- src/common/components/modal/Modal.tsx
- src/common/components/skeletonLoader/SkeletonLoader.tsx
- src/domain/enrolment/enrolmentForm/enrolmentForm.module.scss
- src/common/components/menuDropdown/menuDropdown.module.scss
- src/domain/auth/useAuthorization.ts
- src/common/components/menuDropdown/MenuDropdown.tsx
- src/domain/auth/useIsAuthorizationReady.ts
- src/common/components/menuDropdown/tests/MenuDropdown.test.tsx
- src/common/components/modal/modal.module.scss
- src/common/components/resetFocus/ResetFocus.tsx
- src/domain/app/header/HeaderLink.tsx
- src/index.tsx
- src/domain/auth/constants.ts
- src/domain/occurrences/occurrencesTable/OccurrencesTable.tsx
jorilindell
force-pushed
the
PT-2015-react-19
branch
from
48d564a to
a5bd77c
Compare
Contributor
Author
|
@CodeRabbit review again |
|
✅ Action performedFull review finished. |
|
KULTUS-ADMIN-UI branch is deployed to platta: https://kultus-admin-ui-pr475.dev.hel.ninja 🚀🚀🚀 |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
src/common/components/modal/AlertModal.tsx (1)
1-1:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing React import still unresolved.
Lines 12 and 15 use
React.ReactNodeandReact.FCwithout importing React. In React 19, the global JSX namespace is removed, so this will cause TypeScript error:Cannot find namespace 'React'.🔧 Add the missing import
import { Button, ButtonVariant } from 'hds-react'; +import type { FC, ReactNode } from 'react'; import { useTranslation } from 'react-i18next';Then update the type annotations:
interface Props { confirmButtonText: string; isOpen: boolean; onConfirm: () => void; title: string; toggleModal: () => void; - children?: React.ReactNode; + children?: ReactNode; } -const AlertModal: React.FC<Props> = ({ +const AlertModal: FC<Props> = ({Also applies to: 12-12, 15-15
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/common/components/modal/AlertModal.tsx` at line 1, The file uses React.ReactNode and React.FC (e.g., in the AlertModal component/type annotations) but doesn't import React; add a React import at the top of AlertModal.tsx (for example import React from 'react' or import React, { type FC, type ReactNode } from 'react') so the TypeScript compiler can resolve React.ReactNode and React.FC symbols used in the props and component definitions.src/common/components/modal/NotificationModal.tsx (1)
26-26:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFixed modal ID can cause ARIA collisions (past concern persists).
As flagged in the previous review, using a constant
id="notification-modal"breaks ARIA uniqueness if multiple instances mount simultaneously. The past review noted this was "Addressed," but the current code still uses a fixed string. Use React 19'suseId()to generate a unique ID per instance and derivetitleId/descriptionIdfrom it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/common/components/modal/NotificationModal.tsx` at line 26, NotificationModal currently uses a fixed id="notification-modal" causing ARIA collisions; update the component to call React's useId() (inside NotificationModal) to generate a baseId and derive unique titleId and descriptionId (e.g., `${baseId}-title`, `${baseId}-desc`), replace the hardcoded id with baseId for the modal container, and ensure the modal's aria-labelledby and aria-describedby reference the derived titleId/descriptionId so each instance has unique ARIA IDs.src/common/components/autoSuggest/AutoSuggest.tsx (1)
96-100:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftRender-time state updates risk from
optionLabelToStringcallback (past concern still valid).As flagged in the previous review,
optionLabelToStringis called during render at line 374 (inside themultiValuemap function). PassingaddNotificationas a parameter allows implementations to trigger state updates during render, violating React's rules and potentially causing errors or infinite loops. While line 223's usage is safe (event handler context), the render-path usage at line 374 makes the contract unsafe.Also applies to: 129-129, 223-223, 374-374
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/common/components/autoSuggest/AutoSuggest.tsx` around lines 96 - 100, The optionLabelToString callback currently accepts addNotification and is invoked during render (in the multiValue map) which allows callers to perform state updates during render; change the contract so optionLabelToString(option: AutoSuggestOption, locale: Language) => string (remove addNotification) and update all call sites (including the render path in the multiValue mapping and any other uses at lines around 129, 223, 374) to stop passing addNotification during render; for places that need to create notifications, provide a separate safe handler (e.g., call addNotification from event handlers or expose a ref/callback like notifyOutsideRender on the AutoSuggest component) and update the TypeScript type and any implementations accordingly so render-time calls are side-effect-free.src/common/components/notificationsContext/__tests__/NotificationsContext.test.tsx (1)
36-42:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd missing dependency array to
useEffect.Line 42 closes the
useEffectwithout a dependency array, causing it to run after every render. Line 17 shows the correct pattern ([addNotification]). Without the array, any re-render will queue duplicate notifications.🔧 Proposed fix
useEffect(() => { addNotification({ autoCloseDuration: 50, label: 'Notification 1', type: 'success', }); - }); + }, [addNotification]); return null;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/common/components/notificationsContext/__tests__/NotificationsContext.test.tsx` around lines 36 - 42, The useEffect in the test is missing its dependency array which causes addNotification to run on every render; update the useEffect that calls addNotification to include the dependency array (e.g., [addNotification]) so the effect runs only when addNotification changes, preventing duplicate notifications during re-renders.src/domain/occurrence/CreateOccurrencePage.tsx (1)
455-455:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAwait
refetchEvent()before continuing the save-to-publishing flow.
handleSaveEventInfocallsrefetchEvent()but doesn't await it (line 455). SincehandleGoToPublishingClickimmediately validateseventDataRef.current?.event.pEvent.occurrencesafterawait submitForm(),eventDataRef.currentcan still reflect the pre-refetch render, causing the publishing validation to fail. Apollorefetch()returns a Promise, and the existingdisabled={loading}guard doesn't reliably cover refetch-in-flight.Suggested fix
await Promise.all(requests); formikHelpers.resetForm({ values }); - refetchEvent(); + await refetchEvent(); addNotification({ type: 'success', label: t('eventForm.saveSuccesful'),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain/occurrence/CreateOccurrencePage.tsx` at line 455, handleSaveEventInfo calls refetchEvent() but doesn’t await it, which lets handleGoToPublishingClick validate eventDataRef.current?.event.pEvent.occurrences against stale data after await submitForm(); update handleSaveEventInfo to await the Promise returned by refetchEvent() (or return that Promise so callers can await it), ensuring refetchEvent() completes before continuing the save-to-publishing flow and before any validation in handleGoToPublishingClick that relies on eventDataRef.current.
🧹 Nitpick comments (2)
src/domain/image/utils.ts (1)
10-10: 💤 Low valuePass
nullinstead ofundefinedfor consistency.The
initializeApolloClientsignature defaults the first parameter tonull. Passingundefinedworks but is inconsistent with the intended convention and other call sites (e.g., apolloClient.test.ts).♻️ Suggested change
- const apolloClient = initializeApolloClient(undefined, addNotification); + const apolloClient = initializeApolloClient(null, addNotification);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain/image/utils.ts` at line 10, The call to initializeApolloClient currently passes undefined; change this to pass null to match the function's default and other call sites (e.g., apolloClient.test.ts). Locate the invocation apolloClient = initializeApolloClient(undefined, addNotification) and replace the first argument with null so the call becomes initializeApolloClient(null, addNotification) to maintain consistent convention across the codebase.src/domain/occurrence/enrolmentTable/enrolmentModals/EnrolmentModal.tsx (1)
119-119: ⚡ Quick winConsider moving inline style to SCSS module.
The inline
style={{ width: 660 }}should preferably be defined in the SCSS module for consistency and maintainability, unless the Dialog component API requires inline styles.♻️ Suggested refactor
Add to
enrolmentModals.module.scss:.modal { width: 660px; }Then remove the inline style:
<Dialog id={id} aria-labelledby={titleId} aria-describedby={descriptionId} className={styles.modal} isOpen={true} variant={variant} title={title} close={onClose} closeButtonLabelText={t('common.close')} - style={{ width: 660 }} >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain/occurrence/enrolmentTable/enrolmentModals/EnrolmentModal.tsx` at line 119, Move the inline style on the Dialog in EnrolmentModal (the inline style={{ width: 660 }}) into the SCSS module: add a class (e.g., .modal with width: 660px) to enrolmentModals.module.scss and replace the inline style by applying that class to the Dialog (e.g., className={styles.modal}) in the EnrolmentModal component so styling is consistent and maintainable; keep inline styling only if the Dialog API strictly requires it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/domain/app/apollo/apolloClient.ts`:
- Around line 107-109: The notify callback may remain a no-op if
initializeApolloClient is called without addNotification before the Apollo
client is created; update initializeApolloClient to ensure notify is set before
any client creation by either making addNotification a required parameter
(throwing a clear error if missing) or, if keeping it optional, wrap the notify
invocation to detect the no-op and emit a development-only warning when
PERMISSION_DENIED_ERROR (or any GraphQL error) would be reported; target the
initializeApolloClient function and the notify variable assignment to implement
the chosen approach so GraphQL errors always surface to the user.
- Line 42: Sentry.captureException is being called with the string variable
errorMessage which loses stack and grouping; change the call in apolloClient.ts
(the Sentry.captureException invocation) to pass an Error object instead—if you
have an actual Error (e.g., error or err) pass that directly, otherwise wrap the
message with new Error(errorMessage) so Sentry receives an Error instance and
preserves stack/grouping.
---
Duplicate comments:
In `@src/common/components/autoSuggest/AutoSuggest.tsx`:
- Around line 96-100: The optionLabelToString callback currently accepts
addNotification and is invoked during render (in the multiValue map) which
allows callers to perform state updates during render; change the contract so
optionLabelToString(option: AutoSuggestOption, locale: Language) => string
(remove addNotification) and update all call sites (including the render path in
the multiValue mapping and any other uses at lines around 129, 223, 374) to stop
passing addNotification during render; for places that need to create
notifications, provide a separate safe handler (e.g., call addNotification from
event handlers or expose a ref/callback like notifyOutsideRender on the
AutoSuggest component) and update the TypeScript type and any implementations
accordingly so render-time calls are side-effect-free.
In `@src/common/components/modal/AlertModal.tsx`:
- Line 1: The file uses React.ReactNode and React.FC (e.g., in the AlertModal
component/type annotations) but doesn't import React; add a React import at the
top of AlertModal.tsx (for example import React from 'react' or import React, {
type FC, type ReactNode } from 'react') so the TypeScript compiler can resolve
React.ReactNode and React.FC symbols used in the props and component
definitions.
In `@src/common/components/modal/NotificationModal.tsx`:
- Line 26: NotificationModal currently uses a fixed id="notification-modal"
causing ARIA collisions; update the component to call React's useId() (inside
NotificationModal) to generate a baseId and derive unique titleId and
descriptionId (e.g., `${baseId}-title`, `${baseId}-desc`), replace the hardcoded
id with baseId for the modal container, and ensure the modal's aria-labelledby
and aria-describedby reference the derived titleId/descriptionId so each
instance has unique ARIA IDs.
In
`@src/common/components/notificationsContext/__tests__/NotificationsContext.test.tsx`:
- Around line 36-42: The useEffect in the test is missing its dependency array
which causes addNotification to run on every render; update the useEffect that
calls addNotification to include the dependency array (e.g., [addNotification])
so the effect runs only when addNotification changes, preventing duplicate
notifications during re-renders.
In `@src/domain/occurrence/CreateOccurrencePage.tsx`:
- Line 455: handleSaveEventInfo calls refetchEvent() but doesn’t await it, which
lets handleGoToPublishingClick validate
eventDataRef.current?.event.pEvent.occurrences against stale data after await
submitForm(); update handleSaveEventInfo to await the Promise returned by
refetchEvent() (or return that Promise so callers can await it), ensuring
refetchEvent() completes before continuing the save-to-publishing flow and
before any validation in handleGoToPublishingClick that relies on
eventDataRef.current.
---
Nitpick comments:
In `@src/domain/image/utils.ts`:
- Line 10: The call to initializeApolloClient currently passes undefined; change
this to pass null to match the function's default and other call sites (e.g.,
apolloClient.test.ts). Locate the invocation apolloClient =
initializeApolloClient(undefined, addNotification) and replace the first
argument with null so the call becomes initializeApolloClient(null,
addNotification) to maintain consistent convention across the codebase.
In `@src/domain/occurrence/enrolmentTable/enrolmentModals/EnrolmentModal.tsx`:
- Line 119: Move the inline style on the Dialog in EnrolmentModal (the inline
style={{ width: 660 }}) into the SCSS module: add a class (e.g., .modal with
width: 660px) to enrolmentModals.module.scss and replace the inline style by
applying that class to the Dialog (e.g., className={styles.modal}) in the
EnrolmentModal component so styling is consistent and maintainable; keep inline
styling only if the Dialog API strictly requires it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a755b54d-f58a-4c4d-9790-fb5ca4b09a05
⛔ Files ignored due to path filters (17)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlsrc/common/components/deleteButton/__tests__/__snapshots__/DeleteButton.test.tsx.snapis excluded by!**/*.snapsrc/common/components/fieldLabel/__tests__/__snapshots__/FieldLabel.test.tsx.snapis excluded by!**/*.snapsrc/domain/app/__tests__/__snapshots__/apolloRHHC.test.tsx.snapis excluded by!**/*.snapsrc/domain/app/footer/__tests__/__snapshots__/Footer.test.tsx.snapis excluded by!**/*.snapsrc/domain/app/header/__tests__/__snapshots__/Header.test.tsx.snapis excluded by!**/*.snapsrc/domain/app/layout/__tests__/__snapshots__/PageLayout.test.tsx.snapis excluded by!**/*.snapsrc/domain/errorPage/__tests__/__snapshots__/ErrorPage.test.tsx.snapis excluded by!**/*.snapsrc/domain/event/eventCard/__tests__/__snapshots__/EventCard.test.tsx.snapis excluded by!**/*.snapsrc/domain/event/eventPreviewCard/__tests__/__snapshots__/EventPreviewCard.test.tsx.snapis excluded by!**/*.snapsrc/domain/notFound/__tests__/__snapshots__/NotFoundPage.test.tsx.snapis excluded by!**/*.snapsrc/domain/occurrence/enrolmentDetails/__tests__/__snapshots__/EnrolmentDetails.test.tsx.snapis excluded by!**/*.snapsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/__snapshots__/ApproveEnrolmentModal.test.tsx.snapis excluded by!**/*.snapsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/__snapshots__/DeclineEnrolmentModal.test.tsx.snapis excluded by!**/*.snapsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/__snapshots__/DeleteEnrolmentModal.test.tsx.snapis excluded by!**/*.snapsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/__snapshots__/EnrolmentModal.test.tsx.snapis excluded by!**/*.snapsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/__snapshots__/PickQueueEnrolmentModal.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (105)
.github/workflows/ci.ymlpackage.jsonsrc/clients/apiReportClient/useReportClientQuery.tssrc/common/components/autoSuggest/AutoSuggest.tsxsrc/common/components/autoSuggest/__test__/AutoSuggest.test.tsxsrc/common/components/imageInput/ImageInput.tsxsrc/common/components/menuDropdown/MenuDropdown.tsxsrc/common/components/menuDropdown/__tests__/MenuDropdown.test.tsxsrc/common/components/menuDropdown/menuDropdown.module.scsssrc/common/components/modal/AlertModal.tsxsrc/common/components/modal/BaseDialogModal.tsxsrc/common/components/modal/ConfirmationModal.tsxsrc/common/components/modal/Modal.tsxsrc/common/components/modal/NotificationModal.tsxsrc/common/components/modal/confirmModal.modulesrc/common/components/modal/modal.module.scsssrc/common/components/notificationsContext/NotificationsContext.tsxsrc/common/components/notificationsContext/__tests__/NotificationsContext.test.tsxsrc/common/components/notificationsContext/hooks/useNotificationsContext.tssrc/common/components/resetFocus/ResetFocus.tsxsrc/common/components/skeletonLoader/SkeletonLoader.tsxsrc/common/components/table/Table.tsxsrc/common/components/table/types.tssrc/domain/app/App.tsxsrc/domain/app/__tests__/apolloRHHC.test.tsxsrc/domain/app/apollo/__tests__/apolloClient.test.tssrc/domain/app/apollo/apolloClient.tssrc/domain/app/footer/Footer.tsxsrc/domain/app/header/Header.tsxsrc/domain/app/header/HeaderLink.tsxsrc/domain/app/header/__tests__/Header.test.tsxsrc/domain/app/routes/__tests__/AppRoutes.test.tsxsrc/domain/auth/KultusAdminHDSLoginCallbackHandler.tsxsrc/domain/auth/constants.tssrc/domain/auth/useAuthorization.tssrc/domain/auth/useIsAuthorizationReady.tssrc/domain/enrolment/EditEnrolmentPage.tsxsrc/domain/enrolment/enrolmentForm/enrolmentForm.module.scsssrc/domain/event/CreateEventPage.tsxsrc/domain/event/EditEventPage.tsxsrc/domain/event/EventSummaryPage.tsxsrc/domain/event/__tests__/EventDetailsPage.test.tsxsrc/domain/event/__tests__/EventSummaryPage.test.tsxsrc/domain/event/eventForm/eventForm.module.scsssrc/domain/event/eventForm/useEventFormSubmitRequests.tsxsrc/domain/event/eventPreviewBasicInfo/eventPreviewBasicInfo.module.scsssrc/domain/event/eventPreviewPage.module.scsssrc/domain/event/eventPublish/EventPublish.tsxsrc/domain/event/types.tssrc/domain/events/__tests__/EventsPage.test.tsxsrc/domain/events/utils.tssrc/domain/image/utils.tssrc/domain/keyword/KeywordSelector.tsxsrc/domain/languages/constants.tssrc/domain/myProfile/CreateMyProfile.tsxsrc/domain/myProfile/MyProfilePage.tsxsrc/domain/occurrence/CreateOccurrencePage.tsxsrc/domain/occurrence/__tests__/CreateOccurrencePageEnrolmentInfo.test.tsxsrc/domain/occurrence/__tests__/CreateOccurrencePageEnrolmentType.test.tsxsrc/domain/occurrence/__tests__/CreateOccurrencePageSimultaneousSave.test.tsxsrc/domain/occurrence/__tests__/CreateOccurrencePageVenueInfo.test.tsxsrc/domain/occurrence/enrolmentDetails/EnrolmentDetails.tsxsrc/domain/occurrence/enrolmentDetails/__tests__/EnrolmentDetails.test.tsxsrc/domain/occurrence/enrolmentDetails/enrolmentDetails.module.scsssrc/domain/occurrence/enrolmentTable/EnrolmentQueueTable.tsxsrc/domain/occurrence/enrolmentTable/EnrolmentTable.tsxsrc/domain/occurrence/enrolmentTable/actionsDropdown/ActionsDropdown.tsxsrc/domain/occurrence/enrolmentTable/actionsDropdown/QueueActionsDropdown.tsxsrc/domain/occurrence/enrolmentTable/actionsDropdown/__tests__/ActionsDropdown.test.tsxsrc/domain/occurrence/enrolmentTable/actionsDropdown/__tests__/QueueActionsDropdown.test.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/ApproveEnrolmentModal.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/DeclineEnrolmentModal.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/DeleteEnrolmentModal.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/EnrolmentModal.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/PickQueueEnrolmentModal.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/ApproveEnrolmentModal.test.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/DeclineEnrolmentModal.test.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/DeleteEnrolmentModal.test.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/EnrolmentModal.test.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/__tests__/PickQueueEnrolmentModal.test.tsxsrc/domain/occurrence/enrolmentTable/enrolmentModals/enrolmentModals.module.scsssrc/domain/occurrence/occurrencesFormPart/OccurrencesFormPart.tsxsrc/domain/occurrence/occurrencesFormPart/occurrencesFormPart.module.scsssrc/domain/occurrences/occurrencesTable/ActionsDropdown.tsxsrc/domain/occurrences/occurrencesTable/CancelOccurrenceModal.tsxsrc/domain/occurrences/occurrencesTable/OccurrencesTable.tsxsrc/domain/occurrences/occurrencesTable/__tests__/ActionsDropdown.test.tsxsrc/domain/occurrences/occurrencesTable/occurrencesTable.module.scsssrc/domain/occurrences/occurrencesTableReadOnly/OccurrencesTableSummary.tsxsrc/domain/venue/types.tssrc/headless-cms/__tests__/services.test.tssrc/headless-cms/apollo/apolloClient.tssrc/headless-cms/components/CmsPageContent.tsxsrc/headless-cms/components/__tests__/CmsPage.test.tsxsrc/headless-cms/components/cmsSidebarContent/CmsSidebarContentLayoutLinkList.tsxsrc/hooks/useRHHCConfig.tsxsrc/index.tsxsrc/test/apollo-mocks/footerMenuMock.tssrc/test/apollo-mocks/headerMenuMock.tssrc/test/apollo-mocks/languagesMock.tssrc/test/apollo-mocks/types.tssrc/utils/__tests__/getLanguageCode.test.tssrc/utils/getLanguageCode.tssrc/utils/testUtils.tsxtsconfig.json
💤 Files with no reviewable changes (22)
- src/domain/event/eventForm/eventForm.module.scss
- src/domain/auth/useAuthorization.ts
- src/common/components/menuDropdown/tests/MenuDropdown.test.tsx
- src/common/components/skeletonLoader/SkeletonLoader.tsx
- src/common/components/menuDropdown/menuDropdown.module.scss
- src/domain/auth/constants.ts
- src/common/components/menuDropdown/MenuDropdown.tsx
- src/domain/occurrences/occurrencesTable/occurrencesTable.module.scss
- src/domain/auth/useIsAuthorizationReady.ts
- src/common/components/resetFocus/ResetFocus.tsx
- src/domain/occurrence/enrolmentTable/enrolmentModals/ApproveEnrolmentModal.tsx
- src/common/components/modal/Modal.tsx
- src/domain/occurrence/enrolmentTable/enrolmentModals/PickQueueEnrolmentModal.tsx
- src/common/components/modal/modal.module.scss
- src/domain/event/eventPreviewBasicInfo/eventPreviewBasicInfo.module.scss
- src/domain/enrolment/enrolmentForm/enrolmentForm.module.scss
- src/domain/app/header/HeaderLink.tsx
- src/domain/occurrence/occurrencesFormPart/occurrencesFormPart.module.scss
- src/index.tsx
- src/domain/occurrence/enrolmentDetails/enrolmentDetails.module.scss
- src/domain/occurrences/occurrencesTable/OccurrencesTable.tsx
- src/domain/venue/types.ts
jorilindell
force-pushed
the
PT-2015-react-19
branch
from
a5bd77c to
cb9d167
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
|
KULTUS-ADMIN-UI branch is deployed to platta: https://kultus-admin-ui-pr475.dev.hel.ninja 🚀🚀🚀 |
jorilindell
force-pushed
the
PT-2015-react-19
branch
from
cb9d167 to
0250bab
Compare
|
KULTUS-ADMIN-UI branch is deployed to platta: https://kultus-admin-ui-pr475.dev.hel.ninja 🚀🚀🚀 |
jorilindell
force-pushed
the
PT-2015-react-19
branch
from
0250bab to
bd7f2c4
Compare
|
KULTUS-ADMIN-UI branch is deployed to platta: https://kultus-admin-ui-pr475.dev.hel.ninja 🚀🚀🚀 |
Replace react-modal with Helsinki Design System (HDS) Dialog component across the application for better accessibility and consistency with the design system. Changes: - Replace react-modal with hds-react Dialog in common modals (AlertModal, ConfirmationModal, NotificationModal) - Migrate enrolment-related modals to use HDS Dialog - Update modal tests to use accessible queries (getByRole instead of getByText) - Remove unused react-modal CSS styles and Modal wrapper component - Remove Modal.setAppElement() calls from tests and index.tsx - Remove react-modal and @types/react-modal dependencies Refs: PT-2015
- Remove useAuthorization hook and Authorization type - Remove useIsAuthorizationReady hook (marked todo for removal) - Remove API_TOKEN_ACTIONS constant (marked todo for deletion) - Remove unused EventInList type from event/types.ts - Remove unused OldVenueDataFields type from venue/types.ts - Unexport internal CMS language entry constants Refs: PT-2015
- Remove unused .privacyPolicyLink from enrolmentForm - Remove unused .keywordRow, .isVirtualCheckbox, .neededOccurrencesRow from eventForm - Remove unused .organizationLink from eventPreviewBasicInfo - Remove unused .detailsArea from enrolmentDetails - Remove unused .noOccurrencesCheckBox from occurrencesFormPart - Delete empty eventPreviewPage.module.scss Refs: PT-2015
- replace react-table with @tanstack/react-table - remove @types/react-table - refactor shared Table to use useReactTable and flexRender - manage row expansion with ExpandedState - update table utility types to TanStack v8 - migrate enrolment and occurrence table columns from v7 to v8 Refs: PT-2015
Introduce a NotificationsProvider/useNotificationsContext system backed by HDS Notification components and migrate all notification call-sites away from react-toastify. - Add NotificationsContext, NotificationsProvider and useNotificationsContext hook - Wrap App in NotificationsProvider; move hook usage to AppContent child so it is always called within the provider tree - Add NotificationsProvider to shared test wrappers (customRender, renderWithRoute) and to per-file render helpers that bypass them - Migrate all addNotification call-sites: Apollo client, auth callback, event/enrolment/occurrence pages, image/keyword/profile utilities, occurrences form and actions dropdowns, events list load-more error Refs: PT-2015
jorilindell
force-pushed
the
PT-2015-react-19
branch
from
bd7f2c4 to
1811222
Compare
|
|
KULTUS-ADMIN-UI branch is deployed to platta: https://kultus-admin-ui-pr475.dev.hel.ninja 🚀🚀🚀 |
mikkojamG
approved these changes
Jun 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR upgrades the app to React 19 and HDS 6, and migrates parts of the UI away from older third-party libraries to HDS-based implementations.
Main changes
Upgraded React stack:
Upgraded HDS:
Replaced third-party modal implementation:
Replaced third-party toast notifications:
Migrated table implementation:
Other dependency and cleanup updates:
Refs: PT-2015
Summary by CodeRabbit
New Features
Improvements
Updates