-
-
Notifications
You must be signed in to change notification settings - Fork 5
Feat: Parse Button Suggestion #87
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughReplaces the paste-dimensions dialog with a parsing dialog and adds a persisted suggestion manager that watches user dimension edits and document visibility to surface a Popover suggestion; the bag-input component now passes userDimensions into the parsing dialog. Changes
Sequence Diagram(s)sequenceDiagram
participant BagInput
participant ParsingDialogSuggestion
participant Document as "Document (visibility)"
participant ParsingDialog
BagInput->>ParsingDialogSuggestion: provide bagDimensionsGetter
Note right of ParsingDialogSuggestion: watches bag dimensions & document visibility
Document-->>ParsingDialogSuggestion: visibility change (hidden/visible)
BagInput->>ParsingDialogSuggestion: bag dimensions change
ParsingDialogSuggestion-->>BagInput: shouldShow() true
BagInput->>ParsingDialog: show Popover suggestion
ParsingDialog->>ParsingDialogSuggestion: on suggestion close/disable
ParsingDialog->>BagInput: openDialog (user action) -> close suggestion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/lib/components/bag-input/parsing-dialog.svelte`:
- Around line 36-45: The popover is being forced open because suggestionOpen is
continuously synced to showSuggestion by both watch and $effect; remove the
redundant watch/$effect and instead only set suggestionOpen = true when
showSuggestion transitions from false to true (i.e., a single watcher that opens
on the true transition), and allow user interactions (outside click) to set
suggestionOpen = false; when the popover closes via user interaction, notify the
parent/state by calling the appropriate callback or dispatching an event (e.g.,
call a provided disableSuggestion or setShowSuggestion(false) function or
dispatch a "close" event) so showSuggestion is cleared—this preserves manual
dismissal and drops the always-on sync (remove the existing watch(...) and
$effect(...) blocks and add a single-transition watcher plus an
onClose/outsideClick handler that sets suggestionOpen=false and notifies the
parent).
🧹 Nitpick comments (1)
src/lib/components/bag-input/parsing-dialog-suggestion.svelte.ts (1)
11-47: Consider explicit cleanup forwatchsubscriptions.If a
ParsingDialogSuggestioninstance is ever discarded (component unmount, hot reload, etc.), the twowatchsubscriptions could keep running. Recommend capturing their disposers and exposing adestroy()method, then calling it from the parent component’s teardown.♻️ Suggested approach (adjust to runed API)
export class ParsingDialogSuggestion { private disabled = new PersistedState('suggestion_dimensionParsing_disabled', false); + private stopFns: Array<() => void> = []; private inputBeforeHidden = $state(false); private wasHidden = $state(false); private detected = $state(false); constructor(bagDimensionsGetter: () => UserDimensions) { const visible = new IsDocumentVisible(); let lastDimensions = { ...bagDimensionsGetter() }; - watch( + const stopDims = watch( () => ({ ...bagDimensionsGetter() }), (dims) => { if (this.disabled.current || this.detected) return; // ... } ); + this.stopFns.push(stopDims); - watch( + const stopVis = watch( () => visible.current, (isVisible) => { if (this.disabled.current || this.detected) return; // ... } ); + this.stopFns.push(stopVis); } + public destroy() { + for (const stop of this.stopFns) stop(); + this.stopFns = []; + }Please verify the disposer/cleanup API for
watchin runed and wire teardown accordingly.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.