Conversation
Preview:
|
|
|
Posted 6 actionable inline findings. Tests and TypeScript checks pass. |
|
Posted 3 actionable inline findings. CI checks pass. |
|
|
Posted 3 actionable inline findings. No additional findings. |
|
Posted 6 actionable inline findings. |
Posted 5 additional inline findings. |
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| setMoveAnnouncement(`${item.name} moved to position ${normalizedDestination.index + 1} in ${ | ||
| normalizedDestination.parent?.name ?? label | ||
| }.`); |
There was a problem hiding this comment.
🟡 Repeated move announcements are silent
When successful moves produce identical text, setMoveAnnouncement reuses the current string and React makes no live-region change. Screen readers miss the later move.
Learn more
A live region announces changes to its text content. Keeping it mounted allows the first update to be detected, but assigning its current string again creates no DOM mutation. This occurs whenever separate moves generate the same item name, destination position, and parent name.
Example: Two items named Draft are each moved to position 1 in Archive. The first move sets the region to Draft moved to position 1 in Archive. The second setter receives the same string, so React leaves the region unchanged and the second move is silent.
Recommended fix: Keep the live region mounted, but clear each completed announcement before publishing the next text in a later render. Guard delayed publication with operationId, and cancel any scheduled update during cleanup. An alternating pair of live regions is also suitable if it guarantees a content change for identical consecutive messages.
Was this helpful? React with 👍 or 👎 to provide feedback.
| longPressStartRef.current = null; | ||
| longPressPointerIdRef.current = null; | ||
| }; | ||
| useEffect(() => cancelLongPress, []); |
There was a problem hiding this comment.
[P2] Cancel an in-flight long press when it becomes ineligible. This cleanup only runs on unmount, so if longPressAction/enabled turns false while a finger is down (notably when a resize crosses the action-drawer breakpoint), the old timer still invokes the stale drawer callback. The rerender also removes the pointer-up handlers, leaving pressed set and the subsequent click suppressed. Reconcile the timer and pressed state when eligibility changes.
| if (!useActionDrawer) { | ||
| return ( | ||
| <ContextMenu.Root open={actionsOpen} onOpenChange={onActionsOpenChange}> | ||
| <ContextMenu.Trigger render={row} /> |
There was a problem hiding this comment.
[P2] Apply interaction.longPressDelayMs in this presentation too. On a wide touch device this Kumo trigger owns long press with its fixed 500 ms delay, while the custom delay is wired only into the narrow drawer path. As a result a consumer that raises the documented delay to avoid accidental action activation still gets a menu after 500 ms whenever the viewport is wider than the drawer breakpoint.
| id: insertionTargetId(parent, destinationIndex), | ||
| destination: { parent, index: destinationIndex }, | ||
| indicatorDepth: depth, | ||
| edge: after ? "bottom" : "top", |
There was a problem hiding this comment.
[P2] Do not draw this bottom edge on an expanded non-droppable folder row. In that case the lower half resolves to { parent: folderParent, index: index + 1 }, which places the source after the entire folder subtree, but updateDropIndicator positions this edge immediately below the folder row, above its visible children. The advertised insertion line and the resulting move therefore disagree; use the after-subtree zone/geometry for this destination.
|
|
||
| useEffect(() => { | ||
| const draggedItem = dragController.draggedItem; | ||
| if (draggedItem && (!dragAndDrop || !findItemPosition(items, draggedItem.id))) { |
There was a problem hiding this comment.
[P2] Also end the session when the current source is no longer draggable. If props retain the same item ID but change draggable to false during a touch drag, StyledRow removes the captured handle, so its up/cancel callbacks cannot run; this condition still considers the source valid and leaves the preview and optional auto-scroll active indefinitely. Resolve the current item here and clear the controller when its drag eligibility is revoked.
|
Posted 4 actionable inline findings. |
Summary
This is the foundation PR for shared hierarchical navigation. It introduces the reusable UI layer and component without adopting it in an application yet.
Shared UI package
@gadgets/uiworkspace package for runtime UI shared by the Workshop and gatekeeper management appsHierarchical list
HierarchicalListPrimitivethat recursively renders semantic nested lists while leaving row presentation to the consumerHierarchicalListadapter using Kumo controls and tokens, Phosphor icons, and Motion-powered insertion indicators and drag previewsFrontend conventions
@gadgets/uias the shared runtime UI ownership boundary while keeping Kumo as the primitive and token foundationfrontend-conventionsskill with the full conventions for React work underpackages/Scope
onMoveresults to their tree dataTesting
pnpm --filter @gadgets/ui test:run(33 tests)pnpm install --lockfile-only --frozen-lockfile