Conversation
|
@jjagielka is attempting to deploy a commit to the Themesberg Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughUpdates dialog outside-click handling to detect clicks inside dialog content, adds optional form wrapping when open, removes a placeholder layout element, introduces backward-compatible Drawer props (hidden, activateClickOutside), adds dismissable/outsideclose wiring and modal forwarding, and adjusts Drawer/Modal theme imports and variants. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DOM
participant Dialog
participant Content
User->>DOM: click
DOM->>Dialog: _onclick(MouseEvent)
Dialog->>Content: getBoundingClientRect()
Dialog->>Dialog: compute clickedInContent (point vs rect)
alt outsideclose && !clickedInContent
Dialog->>Dialog: cancel(dlg)
else if autoclose && target is button && !permanent
Dialog->>Dialog: close(dlg)
else
Dialog->>Dialog: no-op
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these settings in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@shinokada - should we add deprecation warnings and backward compatibility? $effect(() => {
if (activateClickOutside && restProps.outsideclose === undefined) restProps.outsideclose = activateClickOutside;
});
$effect(() => {
open = !hidden;
});
$effect(() => {
hidden = !open;
}); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/lib/dialog/Dialog.svelte(2 hunks)src/lib/drawer/Drawer.svelte(2 hunks)src/lib/drawer/theme.ts(1 hunks)src/lib/modal/theme.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/lib/drawer/theme.ts (2)
src/lib/theme/themeUtils.ts (1)
Classes(14-16)src/lib/dialog/theme.ts (1)
dialog(6-21)
🔇 Additional comments (9)
src/lib/modal/theme.ts (1)
3-3: Switching to the themed dialog import is correctImporting dialog from $lib/dialog/theme aligns Modal with the shared dialog theme. This is the right dependency for consistent theming.
src/lib/drawer/Drawer.svelte (2)
10-10: New dismissable prop default looks goodExposing dismissable with a default of false matches the PR intent and avoids accidentally rendering a close button.
27-29: Forwarding modal through restProps is fineForwarding modal via {...restProps} and also feeding it to the theme keeps behavioral and visual variants in sync.
src/lib/drawer/theme.ts (4)
3-3: Importing the dialog theme is the right dependencyExtending from $lib/dialog/theme ensures Drawer inherits the same baseline dialog structure and slots (e.g., base/form/close).
10-11: Base slot adjustments look correctSwitching to "p-4 max-h-none max-w-none" removes artificial clamping and gives the drawer more flexibility.
16-17: Full-width top/bottom placementsUsing !w-full ensures the drawer spans the viewport horizontally for top/bottom placements. Good for predictability across container contexts.
24-27: Modal variant is well-aligned with the runtime modal propThe modal variant cleanly separates styling concerns:
- false: "fixed inset-0" for non-modal drawers (ensures fixed positioning)
- true: no override (inherits from dialog theme)
This pairs well with Drawer.svelte passing restProps.modal into the theme call.
src/lib/dialog/Dialog.svelte (2)
37-45: More robust outside-click detectionUsing getBoundingClientRect() and client coordinates gives a reliable check for clicks outside the content area (including ::backdrop). This should behave better than strict ev.target === dlg checks across browsers.
110-119: Conditional form wrapper is correct and removes stray DOMWrapping content with
only when form is true is the right approach and keeps the non-form path clean.
|
Yes, we should add deprecation warnings and backward compatibility. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/lib/drawer/Drawer.svelte (2)
31-31: Passclassesinto the tv() call so slot overrides workUser-supplied slot classes are ignored if
classesisn’t passed to the theme instance.-const { base } = $derived(drawer({ placement, width, modal: restProps.modal })); +const { base } = $derived(drawer({ placement, width, modal: restProps.modal, classes }));
44-44: Includeclasses?.basewhen computing the base slot classWithout this, consumer overrides for the base slot are dropped.
-<Dialog bind:open {transition} {dismissable} {outsideclose} transitionParams={transition_params} {...restProps} class={base({ class: clsx(theme?.base, className) })}> +<Dialog bind:open {transition} {dismissable} {outsideclose} transitionParams={transition_params} {...restProps} class={base({ class: clsx(theme?.base, classes?.base, className) })}>
🧹 Nitpick comments (1)
src/lib/drawer/Drawer.svelte (1)
12-16: Deprecation warning messages: good addition (minor nit on wording)Nice to warn on legacy props. Minor nit: “compability” → “compatibility” in types file. Also consider using a consistent deprecation phrasing across the codebase (“Will be removed in next minor version”) as in other components.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/lib/drawer/Drawer.svelte(2 hunks)src/lib/types.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/lib/types.ts (1)
src/lib/drawer/theme.ts (1)
DrawerVariants(5-5)
| let { children, open = $bindable(false), hidden = $bindable(), width, dismissable = false, placement = "left", class: className, classes, transitionParams, transition = fly, outsideclose, activateClickOutside, ...restProps }: DrawerProps = $props(); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Prop typing mismatch: children isn’t declared on DrawerProps
You destructure children from $props() and render it, but DrawerProps (types.ts) doesn’t currently declare children?: Snippet. This breaks TS ergonomics.
If you accept the types change I suggested in types.ts (adding children?: Snippet), this resolves cleanly.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo ">>> Locate Drawer.svelte"
if command -v fd >/dev/null 2>&1; then
drawer_path=$(fd --hidden --follow --type f Drawer.svelte | head -n1 || true)
else
drawer_path=$(find . -type f -name Drawer.svelte | head -n1 || true)
fi
if [ -z "$drawer_path" ]; then
echo "Drawer.svelte not found"
else
echo "Found: $drawer_path"
echo "---- Drawer.svelte (lines 1-160) ----"
sed -n '1,160p' "$drawer_path"
echo "---- Drawer.svelte (lines 1-40, showing destructuring lines around 10-11) ----"
sed -n '1,40p' "$drawer_path"
fi
echo
echo ">>> Search for DrawerProps declaration"
# Search for likely type/interface declaration using rg if available, otherwise use grep+find
if command -v rg >/dev/null 2>&1; then
rg --hidden -n --line-number "type\s+DrawerProps|interface\s+DrawerProps|DrawerProps\s*=" || true
else
find . -type f -name '*.ts' -or -name '*.d.ts' | xargs grep -n --line-number -E "type\s+DrawerProps|interface\s+DrawerProps|DrawerProps\s*=" || true
fi
echo
echo ">>> If a file containing DrawerProps was found above, show the first matching file (first 240 lines)"
first_file=$(
if command -v rg >/dev/null 2>&1; then
rg --hidden -n --files-with-matches -S "type\s+DrawerProps|interface\s+DrawerProps|DrawerProps\s*=" | head -n1 || true
else
find . -type f -name '*.ts' -or -name '*.d.ts' | xargs grep -l -E "type\s+DrawerProps|interface\s+DrawerProps|DrawerProps\s*=" | head -n1 || true
fi
)
if [ -n "$first_file" ]; then
echo "Found DrawerProps in: $first_file"
sed -n '1,240p' "$first_file"
echo
echo "---- Search for 'children' within that file ----"
rg -n "children" "$first_file" || grep -n "children" "$first_file" || true
else
echo "No DrawerProps declaration found. Listing types.ts files for inspection..."
if command -v fd >/dev/null 2>&1; then
fd --hidden --type f types.ts || true
else
find . -type f -name 'types.ts' | sed -n '1,120p' || true
fi
fiLength of output: 21427
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/lib/types.ts"
echo ">>> Locate export interface DrawerProps"
rg --hidden -n "export interface DrawerProps" "$file" || true
start_line=$(rg --hidden -n "export interface DrawerProps" "$file" | cut -d: -f1 || true)
if [ -z "$start_line" ]; then
echo "DrawerProps not found in $file"
exit 0
fi
echo ">>> Showing DrawerProps (start at line $start_line, next 120 lines)"
sed -n "${start_line},$((start_line+120))p" "$file" || true
echo
echo ">>> Search for DialogProps declaration (to see inherited props)"
rg --hidden -n "export interface DialogProps|interface DialogProps|type DialogProps" "$file" || true
dialog_line=$(rg --hidden -n "export interface DialogProps|interface DialogProps|type DialogProps" "$file" | head -n1 | cut -d: -f1 || true)
if [ -n "$dialog_line" ]; then
echo ">>> Showing DialogProps (start at line $dialog_line, next 200 lines)"
sed -n "${dialog_line},$((dialog_line+200))p" "$file" || true
else
echo "DialogProps declaration not found in $file"
fiLength of output: 10200
Add missing children?: Snippet to DrawerProps
Drawer.svelte destructures and renders children, but DrawerProps (src/lib/types.ts) doesn't declare it — add the prop to restore correct TS ergonomics.
- Files to update:
- src/lib/drawer/Drawer.svelte — destructures
childrenat ~lines 10–11 and renders it. - src/lib/types.ts —
export interface DrawerPropsat line ~571.
- src/lib/drawer/Drawer.svelte — destructures
Suggested change (types.ts):
export interface DrawerProps extends DrawerVariants, DialogProps {
activateClickOutside?: boolean; // for backward compability
hidden?: boolean | null; // for backward compability
children?: Snippet;
}
🤖 Prompt for AI Agents
In src/lib/drawer/Drawer.svelte around lines 10–11 the component destructures
and renders `children` but the DrawerProps type in src/lib/types.ts (around line
~571) does not declare `children`, causing TypeScript ergonomics to break;
update the DrawerProps interface to include an optional children?: Snippet
property (alongside existing DrawerVariants and DialogProps extensions and the
backward-compatibility props) so the prop is typed correctly and no runtime
changes are needed.
src/lib/types.ts
Outdated
| export interface DrawerProps extends DrawerVariants, DialogProps { | ||
| activateClickOutside?: boolean; // for backward compability | ||
| hidden?: boolean | null; // for backward compability | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid conflicting redeclaration of hidden; add deprecation JSDoc and missing children type
DrawerProps extends DialogProps, which already includes the HTML hidden attribute via HTMLDialogAttributes. Redeclaring hidden here with a different type can cause type incompatibilities and confuses semantics (your hidden is a back-compat inverse of open, not the HTML attribute). Also, children is used by Drawer.svelte but isn’t declared on DrawerProps.
Refactor to omit hidden from DialogProps, annotate deprecations, and add children?: Snippet:
-export interface DrawerProps extends DrawerVariants, DialogProps {
- activateClickOutside?: boolean; // for backward compability
- hidden?: boolean | null; // for backward compability
-}
+export interface DrawerProps extends DrawerVariants, Omit<DialogProps, "hidden"> {
+ /** @deprecated Use `outsideclose` instead. Will be removed in next minor version. */
+ activateClickOutside?: boolean;
+ /** @deprecated Use `open` instead. Will be removed in next minor version. */
+ hidden?: boolean | null;
+ // Drawer renders arbitrary content
+ children?: Snippet;
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/lib/types.ts around lines 571-574, avoid redeclaring the HTML `hidden`
attribute by changing DrawerProps to extend Omit<DialogProps, "hidden">, add a
deprecated JSDoc on the backward-compat `hidden?: boolean | null` explaining it
is not the HTML attribute and should not be used going forward, and add the
missing children?: Snippet property (ensure Snippet is imported/available). This
preserves the DialogProps HTML `hidden`, prevents type conflict, documents
deprecation, and declares the children used by Drawer.svelte.
|
Can you complete the boxes above? |
|
Done. |
|
When I run Interface 'DrawerProps' cannot simultaneously extend types 'DrawerVariants' and 'Omit<DialogProps, "hidden">'. |
|
My VSCode shows this type error on requestClose in Dialog.svelte: Property 'requestClose' does not exist on type 'HTMLDialogElement'.ts(2339) 
|
Small typo. I will correct it in the next PR. |
if (typeof dlg.requestClose === "function") ...we can add |
2 participants
Closes #
📑 Description
Corrections to new
Drawerimplementation<div>thanks to new click outside implementationStatus
✅ Checks
api-checkdirectory as requiredpnpm check && pnpm test:e2eℹ Additional Information
Summary by CodeRabbit
New Features
Bug Fixes
Style