Conversation
Greptile SummaryThis PR defers the fetch and rendering of large request bodies until after the Radix sheet slide-in animation completes (~520 ms), preventing heavy JSON-tree work from blocking the main thread and causing animation stutter (fixes #1749).
Confidence Score: 4/5Safe to merge; the change is a UI timing optimisation that does not touch data-fetching logic or navigation state in a breaking way. The animation-delay approach is sound and the skeleton fallback handles the loading window correctly. The only notable concern is that the hard-coded 520 ms delay fires unconditionally even when prefers-reduced-motion eliminates the animation, leaving those users with an unexplained half-second stall. The isOpeningBeforeStateSync pattern is also non-obvious and would benefit from a comment. Neither concern affects correctness for most users. frontend/src/features/requests/components/request-body-drawer.tsx — the hard-coded delay and the ref-before-effect pattern both warrant a second look. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Sheet as Radix Sheet
participant Comp as RequestBodyDrawer
participant Timer as setTimeout (520ms)
participant Query as useRequest
User->>Comp: "open = true"
activate Comp
Comp->>Sheet: open sheet (slide-in animation begins)
Comp->>Comp: "canRenderBody = false → show Skeleton"
Comp->>Timer: schedule setCanRenderBody(true)
Note over Sheet,Timer: ~520ms — animation plays freely
Timer-->>Comp: "canRenderBody = true"
Comp->>Query: "enabled = true → fetch request detail"
Query-->>Comp: request data
Comp->>Comp: render full body (JsonViewer, Tabs)
deactivate Comp
User->>Comp: "open = false"
activate Comp
Comp->>Timer: clearTimeout (if still pending)
Comp->>Comp: "canRenderBody = false, displayedRequestRef = null"
deactivate Comp
Reviews (1): Last reviewed commit: "opt: request detail sheet annimation, cl..." | Re-trigger Greptile |
| } | ||
|
|
||
| setCanRenderBody(false); | ||
| const timeoutId = setTimeout(() => setCanRenderBody(true), OPEN_ANIMATION_DELAY_MS); |
There was a problem hiding this comment.
The 520 ms wait is unconditional. When a user has prefers-reduced-motion: reduce set, Radix UI shortens or eliminates the sheet animation entirely, yet the skeleton still displays for the full 520 ms. The user sees the drawer pop open instantly and then stare at a placeholder for half a second before content appears — the opposite of the intended trade-off. Reading the media query at runtime and clamping the delay to 0 would avoid the regression for these users.
|
|
||
| // Reset when the drawer is (re)opened for a different request. | ||
| const prevOpenRef = useRef(false); | ||
| const isOpeningBeforeStateSync = open && !prevOpenRef.current; |
There was a problem hiding this comment.
isOpeningBeforeStateSync reads ref that an effect updates later
prevOpenRef.current is mutated inside the useEffect on line 81–88, but isOpeningBeforeStateSync is evaluated during the render that precedes that effect. This creates a one-render window where the variable is true and the component falls back to initialRequests/initialIndex instead of internal state. The logic is correct today, but the coupling between a ref, an inline variable, and an effect that runs asynchronously is subtle. Adding a brief comment above the declaration that explains why this is intentional would make the intent clear to future contributors.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| return () => { | ||
| clearTimeout(timeoutId); | ||
| }; | ||
| }, [open, initialRequestId]); |
There was a problem hiding this comment.
🟡 canRenderBody effect's initialRequestId dependency causes spurious 520ms skeleton flash when drawer is already open
When the user clicks a different request body in the table while the drawer is already open, initialRequestId changes but the drawer's internal navigation state (currentIndex, allRequests) is NOT updated (because the justOpened check at request-body-drawer.tsx:81 is false — prevOpenRef.current is already true). However, the canRenderBody effect at line 101–114 has initialRequestId in its dependency array, so it fires and resets canRenderBody to false, showing a skeleton for 520ms. After the timeout, canRenderBody becomes true again and the drawer renders the SAME old request — the new initialRequestId was never navigated to. The net result is a confusing 520ms skeleton flash with no navigation, a UX regression introduced by this change.
Prompt for agents
The `canRenderBody` effect at line 101-114 includes `initialRequestId` in its dependency array. The comment states the purpose is to let the Radix sheet finish its enter animation before rendering heavy content. However, when `initialRequestId` changes while the drawer is already open (user clicks a different row in the table), no enter animation is happening, yet `canRenderBody` gets reset to false, causing a 520ms skeleton flash. Since the drawer's internal state (allRequests, currentIndex) is not updated in this case (see the justOpened check at line 81), the drawer ends up showing the same old request after the flash.
Two possible approaches:
1. Remove `initialRequestId` from the effect's dependency array (simplest fix — only defer rendering on actual open transitions).
2. If the intent is to support re-targeting the drawer to a different request while open, also update the internal navigation state when `initialRequestId` changes while already open (i.e., handle the case where `justOpened` is false but `initialRequestId` has changed).
Was this helpful? React with 👍 or 👎 to provide feedback.
Uh oh!
There was an error while loading. Please reload this page.