chore(island-ui): Update reakit to latest version of ariakit #18848
Conversation
WalkthroughThis change refactors several UI components to replace the usage of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MenuButton (ContextMenu.render)
participant ContextMenu
participant Menu
participant MenuItem
User->>MenuButton: Clicks menu button
MenuButton->>ContextMenu: setOpen(true)
ContextMenu->>Menu: Renders menu with items
User->>MenuItem: Clicks menu item
MenuItem->>ContextMenu: handleClick
ContextMenu->>Menu: setOpen(false), setLoading(true)
alt Menu item has href
ContextMenu->>Router: Navigate to href
else Menu item has onClick
ContextMenu->>MenuItem: Call onClick
end
ContextMenu->>Menu: setLoading(false)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
View your CI Pipeline Execution ↗ for commit 9fc2824.
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/judicial-system/web/src/components/IconButton/IconButton.tsx (2)
3-3: Verify sub‑path import of Ariakit ButtonAriakit exposes all primitives through the main entry (
@ariakit/react).
While sub‑path exports such as@ariakit/react/buttonexist in v0.5+, they were absent in several earlier 0.4.x builds. Should your lock‑file ever resolve to an older patch version the build will fail with
“Cannot find module '@ariakit/react/button'”.If you don’t need the granular entry for tree‑shaking, consider importing from the root to be future‑proof:
-import { Button } from '@ariakit/react/button' +import { Button } from '@ariakit/react'
36-36: Hard‑coded aria‑label reduces context for screen readers
"Valmynd"(“Menu”) is generic and no longer tells the user what the menu controls (previously: “Opna valmöguleika fyrir mál”).
For better accessibility, expose the label as a prop so callers can specify a descriptive text, falling back to the generic value if omitted.-interface Props { +interface Props { ... + ariaLabel?: string } ... - aria-label="Valmynd" + aria-label={ariaLabel ?? 'Valmynd'}Call‑sites such as
ContextMenucan then pass a domain‑specific string when needed.apps/judicial-system/web/src/routes/Shared/Cases/Cases.tsx (1)
88-91: String literal bypasses i18n pipeline
"Nýtt mál"is now hard‑coded, whereas the surrounding file consistently usesformatMessage(...)for translatable text.
Please move the string to theCases.strings.tsmessages file (or reuse an existing key) to avoid breaking localisation.apps/judicial-system/web/src/components/ContextMenu/ContextMenu.tsx (1)
114-119: Provide accessible name when a customrenderis suppliedWhen
renderis passed the inner text is set tonull, leaving the button without an accessible name unless the custom element already carries one.
Recommend requiring callers to supplyaria-labelvia props or adding a fallback:<MenuButton render={render ?? <Button icon="add" loading={isLoading} />} store={menu} ref={ref} + aria-label={title} > {render ? null : title} </MenuButton>This guarantees screen‑reader users always get a label.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
apps/judicial-system/web/src/components/AppealCaseFilesOverview/AppealCaseFilesOverview.tsx(1 hunks)apps/judicial-system/web/src/components/ContextMenu/ContextMenu.spec.tsx(0 hunks)apps/judicial-system/web/src/components/ContextMenu/ContextMenu.tsx(4 hunks)apps/judicial-system/web/src/components/IconButton/IconButton.tsx(2 hunks)apps/judicial-system/web/src/components/Table/Table.tsx(1 hunks)apps/judicial-system/web/src/routes/Shared/Cases/Cases.tsx(1 hunks)libs/island-ui/core/src/lib/DatePicker/DatePicker.tsx(1 hunks)libs/island-ui/core/src/lib/Input/Input.tsx(2 hunks)package.json(1 hunks)
💤 Files with no reviewable changes (1)
- apps/judicial-system/web/src/components/ContextMenu/ContextMenu.spec.tsx
🧰 Additional context used
📓 Path-based instructions (2)
`apps/**/*`: "Confirm that the code adheres to the following: - NextJS best practices, including file structure, API routes, and static generation methods. - Efficient state manage...
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/web/src/components/AppealCaseFilesOverview/AppealCaseFilesOverview.tsxapps/judicial-system/web/src/components/IconButton/IconButton.tsxapps/judicial-system/web/src/components/Table/Table.tsxapps/judicial-system/web/src/routes/Shared/Cases/Cases.tsxapps/judicial-system/web/src/components/ContextMenu/ContextMenu.tsx
`libs/**/*`: "Confirm that the code adheres to the following: - Reusability of components and hooks across different NextJS apps. - TypeScript usage for defining props and exportin...
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/island-ui/core/src/lib/DatePicker/DatePicker.tsxlibs/island-ui/core/src/lib/Input/Input.tsx
🧠 Learnings (1)
apps/judicial-system/web/src/routes/Shared/Cases/Cases.tsx (1)
Learnt from: gudjong
PR: island-is/island.is#16760
File: apps/judicial-system/web/src/routes/Shared/Cases/PrisonCases.tsx:112-118
Timestamp: 2024-11-07T16:05:06.610Z
Learning: In `apps/judicial-system/web/src/routes/Prosecutor/components/CasesAwaitingConfirmationTable/CasesAwaitingConfirmationTable.tsx`, `apps/judicial-system/web/src/routes/Court/components/CasesAwaitingAssignmentTable/CasesAwaitingAssignmentTable.tsx`, and one instance in `apps/judicial-system/web/src/routes/Shared/Cases/PrisonCases.tsx`, it's correct to pass only the `type` prop to `ColumnCaseType` without the `decision` and `parentCaseId` props.
🧬 Code Graph Analysis (1)
apps/judicial-system/web/src/routes/Shared/Cases/Cases.tsx (1)
apps/judicial-system/web/src/components/ContextMenu/ContextMenu.tsx (1)
ContextMenu(63-155)
🔇 Additional comments (6)
libs/island-ui/core/src/lib/DatePicker/DatePicker.tsx (1)
17-17: Import successfully updated to the ariakit package.The import for
VisuallyHiddenhas been correctly updated from the legacyreakitpackage to the newer@ariakit/reactpackage. This aligns with the broader migration effort described in the PR objectives.package.json (1)
78-78: New dependency added correctly.The
@ariakit/reactpackage has been properly added to the dependencies with a specific version (0.4.17). This supports the migration fromreakitto@ariakit/reactwhile allowing both packages to coexist temporarily, as mentioned in the PR objectives.apps/judicial-system/web/src/components/Table/Table.tsx (1)
335-352: Context menu API updated correctly.The
ContextMenuimplementation has been properly updated to use the new API with arenderprop instead of the previousdisclosureandmenuLabelprops. This pattern gives more control over the menu button rendering and should fix the scrolling issue mentioned in the PR objectives.The implementation maintains the same functionality while using the new API:
- Uses a motion.div for animations
- Properly stops event propagation to prevent row click when clicking the menu button
- Renders the same IconButton as the menu trigger
apps/judicial-system/web/src/components/AppealCaseFilesOverview/AppealCaseFilesOverview.tsx (1)
181-191: Context menu API updated correctly.The
ContextMenucomponent usage has been properly updated to use the new API with arenderprop instead of the previous approach. This is consistent with the pattern used in other components and aligns with the migration to@ariakit/react.The implementation properly:
- Maintains the same IconButton look and feel
- Preserves the disabled state for unavailable files
- Correctly stops event propagation when clicking the menu button
libs/island-ui/core/src/lib/Input/Input.tsx (2)
2-3: Import style is correct & forward‑compatibleGood switch to the root
@ariakit/reactexport; the package suppliesVisuallyHiddendirectly so no sub‑path is required.
No action needed.
258-303: Typed functional component improves clarityChanging
AsideIconstoFC<AsideProps>and extracting props inside the body increases readability and type safety.
Implementation looks solid.
| const handleClick = (evt: React.MouseEvent, item: ContextMenuItem) => { | ||
| evt.stopPropagation() | ||
| evt.preventDefault() | ||
|
|
||
| setOpen(false) | ||
| setIsLoading(true) | ||
|
|
||
| if (item.href) { | ||
| router.push(item.href) | ||
| return | ||
| } | ||
|
|
||
| if (item.onClick) { | ||
| item.onClick() | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Loading state never resets when onClick does not navigate
setIsLoading(true) is set, but false is never restored when an item’s onClick handler performs an in‑place action (e.g. opens a modal).
Consider wrapping the action in a try/finally and resetting the flag, or make onClick return a promise you can await.
- setIsLoading(true)
+ setIsLoading(true)
+
+const complete = () => setIsLoading(false)
if (item.href) {
router.push(item.href)
- return
+ complete()
+ return
}
if (item.onClick) {
- item.onClick()
- return
+ const maybePromise = item.onClick()
+ if (maybePromise?.finally) {
+ maybePromise.finally(complete)
+ } else {
+ complete()
+ }
+ return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleClick = (evt: React.MouseEvent, item: ContextMenuItem) => { | |
| evt.stopPropagation() | |
| evt.preventDefault() | |
| setOpen(false) | |
| setIsLoading(true) | |
| if (item.href) { | |
| router.push(item.href) | |
| return | |
| } | |
| if (item.onClick) { | |
| item.onClick() | |
| return | |
| } | |
| } | |
| const handleClick = (evt: React.MouseEvent, item: ContextMenuItem) => { | |
| evt.stopPropagation() | |
| evt.preventDefault() | |
| setOpen(false) | |
| setIsLoading(true) | |
| const complete = () => setIsLoading(false) | |
| if (item.href) { | |
| router.push(item.href) | |
| complete() | |
| return | |
| } | |
| if (item.onClick) { | |
| const maybePromise = item.onClick() | |
| if (maybePromise?.finally) { | |
| maybePromise.finally(complete) | |
| } else { | |
| complete() | |
| } | |
| return | |
| } | |
| } |
Datadog ReportAll test runs ❌ 10 Total Test Services: 1 Failed, 9 Passed Test ServicesThis report shows up to 10 services
❌ Failed Tests (6)
|
Update reakit to latest version of ariakit
Asana
What
This PR updates ariakit (formerly reakit) to it's latest version.
Why
This fixes a bug where if you have multiple
menucomponents (from ariakit) on a single page. If you were to open one, then scroll that open menu out of view and try to open another one, the first one would scroll back into view.It's also nice when packages are up to date.
Sidenote
This PR adds the ariakit package and uses it it the
judicial-systemproject and in a few places in island-ui. I decided not to update everywhere, due to the complexity that would entail. Rather, I'd like to make other teams responsible for updating their own code an let both packages live in the meantime.Checklist:
Summary by CodeRabbit