Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 36 additions & 1 deletion src/web-view/src/core/shadcn/dialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,41 @@ function DialogOverlay({
);
}

function CustomOverlay({
className,
...props
}: React.ComponentProps<"div">) {
React.useEffect(() => {
document.body.classList.add('dialog-open');

return () => {
document.body.classList.remove('dialog-open');
};
}, []);
Comment on lines +51 to +57

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The current implementation for scroll locking by toggling the dialog-open class on document.body is not robust against multiple dialogs being open concurrently. If two dialogs are open and one is closed, it will remove the dialog-open class, re-enabling scrolling while the second dialog is still open.

To fix this, you would need to track the number of active dialogs. A common approach is to use a module-level counter that is incremented when a dialog mounts and decremented when it unmounts. The class on <body> would only be added when the counter goes from 0 to 1, and removed when it goes from 1 to 0.

If the application design guarantees that only one dialog can be open at any time, the current approach is acceptable, but it would be good practice to add a comment to the useEffect hook to clarify this assumption for future maintainability.


return (
<div
className={cn(
"absolute inset-0 z-50 bg-black/50",
className
)}
onClick={(e) => {
if (e.target === e.currentTarget) {
const escEvent = new KeyboardEvent('keydown', {
key: 'Escape',
code: 'Escape',
keyCode: 27,
which: 27,
bubbles: true
});
document.dispatchEvent(escEvent);
}
}}
{...props}
/>
);
}

function DialogContent({
className,
children,
Expand All @@ -54,7 +89,7 @@ function DialogContent({
}) {
return (
<DialogPortal data-slot="dialog-portal">
<DialogOverlay />
<CustomOverlay />
<DialogPrimitive.Content
data-slot="dialog-content"
className={cn(
Expand Down
4 changes: 4 additions & 0 deletions src/web-view/src/style.css
Original file line number Diff line number Diff line change
Expand Up @@ -121,3 +121,7 @@
@apply bg-background text-foreground;
}
}

body.dialog-open {
overflow: hidden !important;
}
Comment on lines +125 to +127

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using !important is generally discouraged as it can make styles harder to manage and override later, because it breaks the natural CSS cascade. While it might be necessary in this case to override styles injected by the VSCode webview environment, it's important to document why it's being used. Please add a comment explaining the necessity of !important here.

body.dialog-open {
  /* Using !important is necessary to override VSCode webview styles and prevent layout shift. */
  overflow: hidden !important;
}

Binary file modified versions/quick-command-buttons-0.1.4.vsix
Binary file not shown.