resizable attachment + MultiSelect keyboard shortcuts + more test coverage
#376
Conversation
…rops - Replace direct file.node bindings with reactive node_refs array to avoid binding_property_non_reactive warning - Add effect to sync node_refs back to files.node for external access - Use derived any_open state for button text - Spread rest props onto element for better composability - Remove commented-out hljs code - Rename test file to follow *.svelte.test.ts convention
- Extend Action generic to include optional group property - Improve dialog_style comment for clarity - Add comprehensive tests for CmdPalette functionality
- New resizable() attachment for making elements resizable by edge dragging - Supports configurable edges (top, right, bottom, left) - Min/max width and height constraints - Customizable handle size - Callbacks for resize start, resize, and resize end events - Add comprehensive tests for resizable behavior
- New shortcuts prop for customizing keyboard actions - Default shortcuts: ctrl+a (select all), ctrl+shift+a (clear all) - Add KeyboardShortcuts interface to types.ts - Support modifier keys: ctrl, shift, alt, meta/cmd - Shortcuts can be disabled by setting to null - Add comprehensive keyboard shortcut tests
- Add data-sveltekit-preload-data="hover" to prev/next links - Improves page transition performance - Update tests to verify preload attribute
- Add Icon.test.ts with tests for icon rendering and props - Add Wiggle.test.ts with tests for wiggle animation behavior
- Refactor Toggle.test.ts with improved structure and assertions - Expand index.test.ts with additional export and util tests - Update MultiSelect playwright tests
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds keyboard-shortcut support to MultiSelect, a new resizable attachment action, refactors FileDetails to manage DOM refs and spread HTML attributes, extends CmdPalette Action generic with optional grouping, adds SvelteKit preload attrs in PrevNext, and includes many test additions/updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Element as ResizableElement
participant Resizer as resizable action
participant Document as document
Note over User,Element: User presses and drags an edge handle
User ->> Element: mousedown on edge
Element ->> Resizer: init drag (record start dims & edge)
Resizer ->> Document: add mousemove & mouseup listeners
par dragging
Document ->> Resizer: mousemove events
Resizer ->> Resizer: compute clamped width/height, adjust left/top if needed
Resizer ->> Element: apply style updates (width/height/left/top)
Resizer ->> Resizer: call on_resize callback (if provided)
and release
User ->> Document: mouseup
Document ->> Resizer: mouseup event
Resizer ->> Resizer: emit on_resize_end, cleanup listeners, reset cursor
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
tests/vitest/Wiggle.test.ts (1)
77-84: Verify timer behavior with duration=0.The test advances fake timers by 0ms to verify immediate reset, but
vi.advanceTimersByTime(0)may not actually trigger timer callbacks. If the implementation usessetTimeout(..., 0), the test should advance by at least 1ms or usevi.runAllTimers()to ensure the callback executes.🔎 More robust test approach
test(`resets wiggle immediately with duration=0`, () => { const { props, get_value } = create_bindable_wiggle(true, { duration: 0 }) mount(Wiggle, { target, props }) expect(get_value()).toBe(true) - vi.advanceTimersByTime(0) + vi.runAllTimers() expect(get_value()).toBe(false) })tests/vitest/MultiSelect.svelte.test.ts (2)
2827-2847: Consider test coverage for edge cases.The keyboard shortcuts test suite is comprehensive, but consider adding tests for:
- Shortcuts when
disabled={true}(should they be blocked?)- Invalid shortcut string formats (e.g.,
"ctrl+","invalid+key")- Shortcuts when dropdown is closed vs. open (do all shortcuts require
open={true}?)These edge cases would strengthen the test coverage and prevent potential runtime issues.
Do you want me to generate additional test cases for these scenarios?
2873-2901: Consider extracting common test setup.The keyboard shortcut tests follow a consistent pattern with repeated setup code. Consider extracting a helper function to reduce duplication:
🔎 Example helper function
function setup_shortcut_test(props: Partial<MultiSelectProps>) { const state = $state<MultiSelectProps>({ options: [`a`, `b`, `c`], open: true, ...props, }) mount(MultiSelect, { target: document.body, props: state }) const input = doc_query<HTMLInputElement>(`input[autocomplete]`) input.focus() return { props: state, input } } // Usage: test(`ctrl+a selects all options when selectAllOption is enabled`, async () => { const { props, input } = setup_shortcut_test({ selectAllOption: true, selected: [] }) input.dispatchEvent( new KeyboardEvent(`keydown`, { key: `a`, ctrlKey: true, bubbles: true }), ) await tick() expect(props.selected).toEqual([`a`, `b`, `c`]) })src/lib/attachments.ts (2)
23-37: Consider exportingDimensionsandResizeCallbacktypes.The type definitions look clean and well-structured. However,
DimensionsandResizeCallbackare used in the exportedResizableOptionsinterface but aren't themselves exported, which prevents users from easily referencing these types in their callback implementations.🔎 Proposed enhancement
-type Dimensions = { width: number; height: number } -type ResizeCallback = (event: MouseEvent, dimensions: Dimensions) => void +export type Dimensions = { width: number; height: number } +export type ResizeCallback = (event: MouseEvent, dimensions: Dimensions) => void
138-163: Good setup with reasonable defaults.The function initialization is clean with sensible default values. The option destructuring makes the code readable and maintainable.
One note: Line 163 modifies the element's position style from
statictorelativeto enable left/top positioning during resize. While necessary for the feature to work, this side effect persists after cleanup. Consider storing and restoring the original position value if you want full cleanup isolation.🔎 Proposed enhancement for complete cleanup
(options: ResizableOptions = {}): Attachment => (element: Element) => { if (options.disabled) return const node = element as HTMLElement + const original_position = getComputedStyle(node).position const { edges = [`right`, `bottom`], // ... rest of options } = options // ... resize logic ... return () => { node.removeEventListener(`mousedown`, on_mousedown) node.removeEventListener(`mousemove`, on_hover) globalThis.removeEventListener(`mousemove`, on_mousemove) globalThis.removeEventListener(`mouseup`, on_mouseup) node.style.cursor = `` + if (original_position === `static`) { + node.style.position = original_position + } } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/lib/CmdPalette.sveltesrc/lib/FileDetails.sveltesrc/lib/MultiSelect.sveltesrc/lib/PrevNext.sveltesrc/lib/attachments.tssrc/lib/types.tstests/playwright/MultiSelect.test.tstests/vitest/CmdPalette.svelte.test.tstests/vitest/FileDetails.svelte.test.tstests/vitest/FileDetails.test.tstests/vitest/Icon.test.tstests/vitest/MultiSelect.svelte.test.tstests/vitest/PrevNext.test.tstests/vitest/Toggle.test.tstests/vitest/Wiggle.test.tstests/vitest/attachments.test.tstests/vitest/index.test.ts
💤 Files with no reviewable changes (1)
- tests/vitest/FileDetails.test.ts
🧰 Additional context used
🧬 Code graph analysis (5)
tests/vitest/Icon.test.ts (1)
src/lib/icons.ts (2)
icon_data(1-65)IconName(67-67)
tests/vitest/FileDetails.svelte.test.ts (1)
tests/vitest/index.ts (1)
doc_query(4-8)
tests/vitest/index.test.ts (1)
src/lib/index.ts (1)
scroll_into_view_if_needed_polyfill(22-40)
tests/vitest/attachments.test.ts (1)
src/lib/attachments.ts (2)
draggable(42-136)resizable(139-249)
tests/vitest/MultiSelect.svelte.test.ts (2)
tests/vitest/index.ts (1)
doc_query(4-8)src/lib/types.ts (1)
MultiSelectProps(117-219)
🔇 Additional comments (34)
tests/vitest/index.test.ts (5)
21-36: Well-structured mock state management.The
MockStatetype and sharedmockvariable provide clean organization for the IntersectionObserver mock. The structure correctly captures callback, instance, and spy functions needed for testing.
38-61: Clean mock observer implementation.The
create_mock_observerhelper effectively creates a controllable IntersectionObserver mock. The approach of resetting spies on each call and capturing the callback for manual invocation is a good pattern for testing observer-based code.
63-66: Good test isolation with proper cleanup.The
afterEachcorrectly resets both the mock state and unstubs globals, ensuring tests don't leak state to each other.
68-107: Comprehensive parameterized test coverage.The test cases correctly cover all branches of the
scroll_into_view_if_needed_polyfillfunction:
ratio <= 0 && centerIfNeeded→centerratio > 0 || !centerIfNeeded→nearestratio >= 1→ no scrollThe verification that
disconnectis always called (line 105) correctly matches the unconditionalobs.disconnect()in the implementation.
109-122: Good coverage of default parameter behavior.This test correctly verifies that
centerIfNeededdefaults totruewhen omitted, ensuring the function scrolls tocenterwhen the element is not visible (ratio=0).tests/vitest/Toggle.test.ts (5)
11-14: Nice refactoring withtest.each.Consolidating the checked prop tests into a single parameterized test improves maintainability and reduces duplication.
25-36: Well-structured test for Enter key behavior.Merging the Enter key toggle tests into a single test that verifies both the toggle behavior and change event emission improves test clarity.
38-45: Good use of parameterized testing for non-toggle keys.Using
test.eachto verify that multiple keys don't trigger toggling reduces code duplication and makes it easy to add more keys in the future.
47-66: Solid test for Enter key event handling.The test correctly verifies that:
preventDefaultis calledonkeydownhandler fires before click- Event order is preserved
The use of
call_orderarray to track execution sequence is a clean approach.
68-80: Good coverage of class and style props.The test correctly verifies that custom classes and styles are applied to both the label and input elements.
tests/vitest/Icon.test.ts (4)
11-23: Comprehensive icon rendering test.The parameterized test effectively verifies all icons in
icon_data, checking viewBox, fill, and path attributes. The type assertion on line 11 is appropriate sinceObject.keysreturnsstring[]but we know they'reIconName.
25-56: Excellent pattern for testing rest props.The parameterized test with tuple structure
[attr, value, getter, expected]is clean and extensible. Usingas constensures type safety, and the variety of getter functions appropriately handles different attribute types (classList vs getAttribute).
58-74: Robust fallback testing with proper mock cleanup.The test covers important edge cases (invalid, empty, whitespace strings) and correctly mocks and restores
console.error. The use of@ts-expect-erroron line 63 is appropriate for testing runtime behavior with invalid icon names.
76-81: Good practice testing computed styles.Using
getComputedStylecorrectly verifies the actual rendered styles rather than just inline attributes, ensuring the component's default visual behavior is correct.src/lib/PrevNext.svelte (1)
103-105: LGTM! SvelteKit preload enhancement.The addition of
data-sveltekit-preload-data="hover"enables efficient hover-based preloading for prev/next navigation links, improving user experience without changing existing navigation behavior.Also applies to: 118-120
tests/vitest/MultiSelect.svelte.test.ts (1)
2767-2767: LGTM! Improved selector consistency.Changing to
input[autocomplete]aligns with the selector pattern used throughout the test file, improving consistency and maintainability.tests/vitest/CmdPalette.svelte.test.ts (1)
594-727: LGTM! Comprehensive grouping test coverage.The grouping tests thoroughly validate:
- Group header rendering and content
- Backward compatibility with ungrouped actions
- Collapsible functionality with aria-expanded state
- Mixed grouped/ungrouped scenarios
- Initial collapsed state control via props
The test structure is clear and the assertions properly verify both DOM structure and behavior.
src/lib/types.ts (1)
217-230: LGTM! Well-designed keyboard shortcuts API.The
KeyboardShortcutsinterface is thoughtfully designed:
- Clear documentation with format examples
Partial<>type allows selective customizationstring | nullenables disabling individual shortcuts- Consistent naming convention (snake_case for action names)
The integration into
MultiSelectPropsas an optional prop maintains backward compatibility while enabling the new feature.tests/playwright/MultiSelect.test.ts (2)
233-237: LGTM! Improved test synchronization for remove-all button.The changes properly handle the flip animation by waiting for the button to become visible before clicking, and then asserting it's hidden after the action. This reduces flakiness in tests that depend on animation timing.
1061-1095: LGTM! Enhanced keyboard navigation test with better synchronization.The test now properly:
- Waits for state to settle after toggling
keyboardExpandsCollapsedGroups- Waits for the options list to become visible
- Verifies aria-expanded states before navigating
- Queries active option after UI settles
- Includes descriptive comments for each navigation step
These changes improve reliability, especially in CI environments where timing can vary.
tests/vitest/attachments.test.ts (2)
640-695: LGTM! Comprehensive tests for draggable edge cases.Good coverage for:
- Non-fixed positioning using
offsetLeft/offsetTopinstead ofgetBoundingClientRect- Ignoring
mousemoveevents when not actively dragging- Ignoring
mouseupevents when not actively draggingThese tests prevent regressions in the draggable behavior for different positioning contexts.
999-1500: LGTM! Thorough test coverage for the new resizable feature.The test suite comprehensively covers:
- Edge hover cursor updates (right, bottom, left, top edges)
- Custom
handle_sizeconfiguration- Min/max width/height constraints with proper clamping
- Resize lifecycle callbacks (
on_resize_start,on_resize,on_resize_end)- Position adjustments for left/top edge resizes
- Disabled state behavior
- Static vs non-static positioned elements
- Cleanup and event listener removal
- Non-drag interaction guards
One minor observation: the
on_resize_endcallback test at line 1283-1286 uses the mockedoffsetWidth/offsetHeightvalues (200, 150) rather than the actual resized dimensions (255, 150). This is expected since the mock doesn't update dynamically, but worth noting for future maintainers.tests/vitest/FileDetails.svelte.test.ts (2)
38-79: LGTM! Toggle-all button test covers key scenarios.Good coverage of:
- Initial closed state verification
- Opening all details via button click
- Closing all details via button click
- Handling partial open state (some manually opened, then close all)
81-126: No action needed. The test code properly uses$statefor reactive state management, and the Vitest/Svelte configuration already fully supports this pattern. The@sveltejs/kit/viteplugin automatically handles preprocessing of all Svelte files, including.svelte.test.ts, and$stateis already in widespread use across multiple test files in the same test suite (MultiSelect.svelte.test.ts, CmdPalette.svelte.test.ts, FileDetails.svelte.test.ts).Likely an incorrect or invalid review comment.
src/lib/CmdPalette.svelte (1)
3-3: LGTM! Generic type extended for optional grouping support.The
Actiongeneric now includes an optionalgroup?: stringproperty, enabling grouped actions in the command palette. This is a backward-compatible change that aligns with the grouping capabilities in the underlyingMultiSelectcomponent.src/lib/FileDetails.svelte (2)
31-43: Good reactive refs management approach.The
node_refsstate properly addresses the binding warning and includes memory leak prevention by trimming stale refs when thefilesarray shrinks. The sync back tofiles[idx].nodemaintains API compatibility for external consumers.One consideration: The effect runs whenever
node_refsorfileschanges. The trimming logic (node_refs.splice(files.length)) mutatesnode_refsin place, which could trigger the effect again. However, since the conditionnode_refs.length > files.lengthwill be false after the splice, it should stabilize without infinite loops.
48-54: Correct approach to read fresh DOM state in toggle_all.The comment explains why
$derivedany_openisn't used here - reading the DOM state fresh vianode_refs.some((node) => node?.open)ensures the toggle logic uses current state rather than potentially stale derived values. This is the right pattern for imperative DOM mutations.tests/vitest/PrevNext.test.ts (2)
32-47: LGTM! Data-driven test pattern for prev/next link scenarios.Using
test.eachwith descriptive test cases (middle item,first item wraps,last item wraps) improves test readability and reduces duplication. The test clearly covers wrap-around behavior for navigation.
156-172: LGTM! Updated link_props test with preload attribute verification.The test now verifies that
data-sveltekit-preload-data="hover"is applied to navigation links, aligning with the component's updated behavior for SvelteKit preloading optimization.src/lib/MultiSelect.svelte (3)
153-186: LGTM! Well-structured shortcut parsing utilities.The
parse_shortcutfunction correctly handles:
- Case-insensitive parsing
- Multiple modifiers (
ctrl,shift,alt,meta/cmd)- The key as the last part after splitting on
+The
matches_shortcutfunction properly checks all modifier keys match exactly (not just that required ones are present), which prevents false matches.One edge case to consider: if a user specifies
"a"(no modifiers), this will match anyakeypress including with modifiers. This seems intentional given the exact matching of modifier booleans.
188-199: LGTM! Sensible default shortcuts with proper merging.Default shortcuts:
select_all:ctrl+a- industry standardclear_all:ctrl+shift+a- logical inverse with shift modifieropen/close:null- disabled by defaultThe
$derivedmerge with user-providedshortcutsallows overriding defaults or disabling them by setting tonull.
711-751: LGTM! Keyboard shortcuts checked early with proper conditions.The shortcut handling:
- Checks shortcuts before other key logic (Escape, Tab, Enter, Arrow keys)
- Each shortcut has an appropriate condition:
select_all: Only when enabled and multiple options existclear_all: Only when selections existopen/close: Based on current open state- Properly prevents default and stops propagation to avoid browser shortcuts (e.g.,
Ctrl+Aselecting all text)The early return after handling a shortcut prevents the rest of
handle_keydownfrom executing, which is the correct behavior.src/lib/attachments.ts (2)
165-219: LGTM! Resize logic is mathematically correct.The edge detection and resize calculations are implemented correctly:
- Edge detection properly identifies which edge is under the cursor based on
handle_size- Resize calculations correctly handle all four edges with proper position compensation for left/top edges
- Clamping enforces min/max constraints as expected
- Callbacks are invoked at appropriate lifecycle points (start, during, end)
The position adjustment math for left/top edges ensures the opposite edge stays fixed while resizing, which is the expected behavior.
221-249: LGTM! Event handling and cleanup are properly implemented.The event handlers follow best practices:
- Proper cleanup of global mouse listeners in
on_mouseupto prevent memory leaks- Cursor feedback in
on_hoverprovides clear visual indication of resizable edges- Comprehensive cleanup function removes all event listeners and resets cursor state
- Preserves resized dimensions after cleanup (intentional - the resize should persist)
Also: - Export Dimensions and ResizeCallback types from attachments.ts - Refactor keyboard shortcut tests to use test.each for better coverage - Add tests for disabled state, invalid shortcuts, and dropdown closed state - Fix Toggle test onchange prop location - Fix Wiggle test timer handling
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/vitest/Toggle.test.ts (1)
6-9: Test isolation issue remains unaddressed.The shared
document.bodytarget without cleanup between tests causes DOM pollution. Each test mounts a new Toggle without removing previous ones, soget_input()may return stale elements.
🧹 Nitpick comments (4)
src/lib/attachments.ts (2)
26-37: Consider validating min/max constraint relationships.The interface allows configurations where
min_width > max_widthormin_height > max_height, which would make resizing impossible. While theclampfunction (line 160-161) handles this gracefully, it may be better to validate these constraints and warn users about invalid configurations during initialization.🔎 Suggested validation
export const resizable = (options: ResizableOptions = {}): Attachment => (element: Element) => { if (options.disabled) return const node = element as HTMLElement const { edges = [`right`, `bottom`], min_width = 50, min_height = 50, max_width = Infinity, max_height = Infinity, handle_size = 8, on_resize_start, on_resize, on_resize_end, } = options + if (min_width > max_width || min_height > max_height) { + console.warn( + `resizable: min dimensions exceed max dimensions (min_width=${min_width}, max_width=${max_width}, min_height=${min_height}, max_height=${max_height})` + ) + } + let active_edge: string | null = null
163-163: Document position override side effect.The function automatically changes
position: statictoposition: relative, which is necessary for resizing but could interfere with existing layouts. Consider documenting this behavior in JSDoc comments or the function's description to help users understand this side effect.🔎 Suggested documentation
+// Svelte 5 attachment factory to make an element resizable by dragging its edges +// @param options - Configuration options for resizing behavior +// @returns Attachment function that sets up resizing on an element +// @note Automatically sets position: relative on elements with position: static export const resizable = (options: ResizableOptions = {}): Attachment => (element: Element) => {tests/vitest/MultiSelect.svelte.test.ts (1)
2827-4216: Excellent keyboard shortcuts test coverage!The test suite comprehensively covers:
- Basic shortcuts (select all, clear all)
- Custom shortcut overrides
- Disabled shortcuts (null)
- Constraint respect (maxSelect, minSelect)
- Mac keyboard support (meta/cmd)
- Various modifier combinations
- Disabled state blocking
- Invalid formats handling
- Dropdown open/closed states
The helper function
test_shortcut(lines 4173-4199) is a nice touch that reduces boilerplate while maintaining clarity.One optional enhancement to consider: add a test case for shortcuts that conflict with browser defaults (e.g., ctrl+s) to document whether the component prevents default browser behavior.
src/lib/MultiSelect.svelte (1)
153-170: Consider edge case handling in parse_shortcut.The
parse_shortcutfunction correctly handles common cases, but could behave unexpectedly with edge cases:
- Empty string: returns
{ key: '', ctrl: false, ... }- Trailing
+:"ctrl+"returns{ key: '', ctrl: true, ... }- Single key without modifiers:
"a"returns{ key: 'a', ... }(works correctly)While these edge cases are caught by
matches_shortcutreturning false (line 176 checks!shortcut), the empty key will cause all events with matching modifiers to match. Consider validating the shortcut format or explicitly checking for non-empty keys.🔎 Suggested validation
function parse_shortcut(shortcut: string): { key: string ctrl: boolean shift: boolean alt: boolean meta: boolean } { const parts = shortcut.toLowerCase().split(`+`) const key = parts.pop() ?? `` + // Validate that key is non-empty after parsing + if (!key) { + console.warn(`Invalid shortcut format: "${shortcut}" (missing key)`) + } return { key, ctrl: parts.includes(`ctrl`), shift: parts.includes(`shift`), alt: parts.includes(`alt`), meta: parts.includes(`meta`) || parts.includes(`cmd`), } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/lib/MultiSelect.sveltesrc/lib/attachments.tstests/vitest/MultiSelect.svelte.test.tstests/vitest/Toggle.test.tstests/vitest/Wiggle.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
tests/vitest/MultiSelect.svelte.test.ts (2)
tests/vitest/index.ts (1)
doc_query(4-8)src/lib/types.ts (1)
MultiSelectProps(117-219)
🔇 Additional comments (17)
tests/vitest/Wiggle.test.ts (3)
15-30: Excellent helper design.The
create_bindable_wigglehelper elegantly simulates Svelte's two-way binding with a getter/setter pattern, making it straightforward to test state changes triggered by the component. This is a clean, reusable approach.
32-39: LGTM!Good baseline test verifying the component renders with expected transform properties.
41-84: Strong edge-case coverage.The test suite thoroughly exercises the Wiggle component's auto-reset behavior across multiple scenarios (varying durations, starting states, and edge cases like
duration=0). The parameterized test and comprehensive prop validation are well-structured.tests/vitest/Toggle.test.ts (9)
11-14: LGTM!Clean parameterized test covering both initial checked states.
16-23: LGTM!Correctly verifies bidirectional toggle on click.
25-36: LGTM!Correctly passes
onchangeviainput_propsand verifies Enter key toggles the checkbox while firing the change event.
47-66: LGTM!Correctly verifies that Enter key calls
preventDefault, invokesonkeydownhandler, and maintains proper event ordering.
68-80: LGTM!Good coverage of styling props, correctly distinguishing between label-level props and
input_props.
82-95: LGTM!Clean parameterized approach for event emission tests. All handlers correctly passed via
input_props.
97-102: LGTM!Simple structural smoke test verifying expected DOM elements.
104-123: LGTM!Correctly tests event-based state synchronization. The manual
changeevent dispatch is necessary in JSDOM since syntheticclick()may not automatically fire change events.
38-45: Remove concern about Space key blocking.The code does not prevent Space from toggling. The
handle_keydownfunction only intercepts Enter (callingpreventDefault()and simulating a click to ensure thechangeevent fires). Space retains native checkbox behavior, as confirmed by the comment on line 18: "normally input type=checkbox toggles on space bar, this handler also responds to enter." The test uses programmatically dispatched events, which don't trigger native browser toggle behavior—a real user pressing Space would correctly toggle the input.src/lib/attachments.ts (1)
139-249: LGTM! Well-structured resizable implementation.The
resizableattachment factory is well-implemented with:
- Proper edge detection with configurable handle size
- Dimension clamping to prevent invalid sizes
- Correct position adjustment when resizing from left/top edges
- Lifecycle callbacks for integration with parent components
- Proper cleanup of event listeners
The implementation correctly handles the complexity of maintaining element position when resizing from edges that affect both size and position (left/top).
tests/vitest/MultiSelect.svelte.test.ts (1)
2767-2767: LGTM! Improved selector consistency.Changing from
input:not(.form-control)toinput[autocomplete]is more explicit and aligns with the selector pattern used consistently throughout the test file. This makes the test more maintainable and less brittle.src/lib/MultiSelect.svelte (3)
172-186: LGTM! Correct shortcut matching logic.The
matches_shortcutfunction correctly:
- Handles null/undefined shortcuts (returns false)
- Performs case-insensitive key matching
- Requires exact modifier matches (prevents false positives)
The strict matching ensures that
ctrl+awon't trigger onctrl+shift+a, which is the correct behavior.
714-753: Well-implemented keyboard shortcut handling!The shortcut handling is correctly integrated:
✅ Proper placement: Shortcuts are checked before other key handling (line 714-753 before line 755), ensuring shortcuts take precedence over default behaviors.
✅ Proper conditions: Each action has appropriate guards:
select_all: requiresselectAllOptionenabled, navigable options exist, and not single-selectclear_all: requires selected items existopen: only when dropdown closedclose: only when dropdown open✅ Proper event handling: Calls
preventDefault()andstopPropagation()to prevent browser defaults and bubbling.✅ Respects constraints:
select_allandclear_alldelegate to existing functions that already enforcemaxSelectandminSelectconstraints.
712-712: LGTM! Proper disabled state handling.Moving the disabled check to the top of
handle_keydownensures all keyboard interactions, including shortcuts, are blocked when the component is disabled. This is the correct behavior and is properly covered by tests (test file lines 4095-4115).
Prevents malformed shortcuts like 'ctrl+' from matching any keypress with the ctrl modifier. Now requires a non-empty key in the shortcut. refactor: improve resizable docs and keyboard shortcut tests - Add JSDoc documentation to resizable attachment factory - Add validation warning when min dimensions exceed max - Integrate preventDefault check into first keyboard shortcut test - Remove redundant standalone preventDefault test
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib/MultiSelect.sveltesrc/lib/attachments.tstests/vitest/MultiSelect.svelte.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/MultiSelect.svelte
🔇 Additional comments (6)
tests/vitest/MultiSelect.svelte.test.ts (2)
2767-2767: LGTM!The selector change from
input:not(.form-control)toinput[autocomplete]is consistent with other tests in this file and correctly targets the main input element.
3827-4223: Comprehensive keyboard shortcuts test coverage.The test suite thoroughly covers:
- Default shortcuts (
ctrl+a,ctrl+shift+a)- Custom shortcut overrides
- Disabling shortcuts via
null- Constraint enforcement (
maxSelect,minSelect)- Mac-specific modifiers (
meta+a,cmd+a)- Complex modifier combinations
- Disabled component state
- Edge cases (invalid formats, no-op when already in target state)
- Dropdown state independence
The
test_shortcuthelper at lines 4180-4206 is a nice pattern to reduce boilerplate.src/lib/attachments.ts (4)
23-37: Clean type definitions for the resizable API.The exported types provide a well-defined interface for the feature. The
ResizableOptionsinterface is comprehensive with sensible defaults documented inline.
138-140: Side effect documented appropriately.The auto-setting of
position: relativeon static elements is a reasonable default that enables the resize functionality. The doc comment on lines 138-139 correctly warns users about this behavior.Also applies to: 170-170
202-226: Resize calculation logic is correct for single-edge resizing.The implementation correctly:
- Handles left/top edge resizing by adjusting position alongside dimension
- Uses clamping to enforce min/max constraints
- Updates styles in a performant manner
Note that corner resizing (e.g., bottom-right simultaneously) is not supported by design, which is fine given the
edgesinterface only accepts single edges.
249-256: Cleanup function handles all listeners correctly.The cleanup properly removes both node-level listeners (
mousedown,mousemovefor hover) and global listeners (mousemove,mouseup) that may have been attached during an active resize operation. This prevents memory leaks if the component unmounts mid-resize.
…x(MultiSelect): trim whitespace from shortcut parts Prevents undefined behavior where clamp() would produce inconsistent results when min dimensions exceed max dimensions. Allows shortcuts with spaces like 'ctrl + a' to work correctly by trimming whitespace from each part after splitting on '+'.
janosh
changed the title
resizable attachment + MultiSelect keyboard shortcuts + more test coverage
janosh
added
enhancement
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/lib/MultiSelect.svelte (1)
713-797: Document shortcut override behavior and add test coverage for edge cases.Shortcuts are evaluated before built-in key handlers by design (comment on line 716 confirms "Check keyboard shortcuts first"). While default shortcut values (open=null, close=null) prevent conflicts, users can set
shortcuts={{ open: "enter" }}to override Enter key behavior. This is technically possible and intentional for customization, but:
The API documentation (types.ts:225-229) doesn't explicitly clarify this precedence behavior—comments hint at it ("Escape already works", "use existing behavior") but don't explicitly warn users about potential override scenarios.
Test coverage includes various shortcut configurations but lacks test cases verifying shortcut overrides of built-in keys (e.g., custom open/close with Enter/Escape keys), which would validate this behavior and document expected outcomes.
Consider clarifying the KeyboardShortcuts documentation to explicitly state that custom shortcuts take precedence over built-in key handling, and add test cases demonstrating shortcut override behavior to prevent future surprises.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/MultiSelect.sveltesrc/lib/attachments.ts
🔇 Additional comments (10)
src/lib/attachments.ts (5)
23-24: LGTM!Type definitions are clean and appropriate for the resizable API.
26-37: LGTM!The interface is well-designed with sensible optional parameters and follows the same callback pattern as
draggable.
157-162: LGTM! Past review concern addressed.The early return after validation prevents
clamp()from producing inconsistent results when min exceeds max.
489-492: LGTM! Good security documentation.The clarified comment explicitly warns about XSS risks when using user-provided content in tooltips, making it clear that sanitization is the caller's responsibility.
138-257: LGTM! Solid implementation with correct resize mathematics.The resizable attachment is well-implemented:
- Edge detection logic correctly identifies resize handles
- Resize calculations for left/top edges properly adjust position while keeping opposite edges fixed
- Dimension clamping prevents invalid sizes
- Thorough cleanup and defense-in-depth listener removal
- The automatic
position: relativechange for static elements is documented and necessary- Svelte 5.45.6 attachment API is stable and the factory pattern used here (options → Attachment function) is fully compatible with the official API
src/lib/MultiSelect.svelte (5)
10-10: LGTM: Import addition is correct.The KeyboardShortcuts type import is properly added to support the new shortcuts feature.
148-149: LGTM: Prop initialization is appropriate.The shortcuts prop with an empty object default allows users to optionally override specific shortcuts while inheriting defaults.
153-170: LGTM: Shortcut parsing logic is sound.The function correctly parses modifier+key combinations and handles the cmd→meta alias for Mac compatibility. Empty or malformed shortcuts are safely handled by the matches_shortcut guard (line 179).
172-188: LGTM: Shortcut matching logic is robust.The function correctly guards against null/undefined shortcuts and empty keys, preventing false positives like "ctrl+" matching any key with ctrl pressed. The exact modifier matching (line 185-187) is the correct behavior.
190-201: LGTM: Shortcut defaults are well-chosen.The default shortcuts follow conventions (ctrl+a for select all), and open/close being null by default prevents unexpected behavior. The merge strategy correctly allows user overrides.
- Add explicit documentation in KeyboardShortcuts interface explaining that custom shortcuts are evaluated before built-in key handlers - Add test.each for shortcut override edge cases (Enter, Escape, ArrowDown) - Consolidate 4 individual tests into parameterized test.each for conciseness
Summary by CodeRabbit
New Features
Breaking Changes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.