improvement(knowledge): polish tag filter dropdowns#4816
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryLow Risk Overview ResourceOptionsBar treats Filter and Sort as one menu bar: shared open state so switching between them is a single click, with end-aligned popovers and matching open/close animation. New shared EMCN pieces: Reviewed by Cursor Bugbot for commit ab9fb09. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ab9fb09. Configure here.
| const [view, setView] = useState(() => { | ||
| const base = selected ?? today | ||
| return { month: base.getMonth(), year: base.getFullYear() } | ||
| }) |
There was a problem hiding this comment.
Calendar month ignores value
Medium Severity
The calendar keeps its visible month in local state initialized only on mount. If the selected value is set or changed later, or the user navigates to another month and closes the picker without picking a day, reopening can show a month that does not contain the current selection, so the chosen date may not appear selected.
Reviewed by Cursor Bugbot for commit ab9fb09. Configure here.
Greptile SummaryThis PR replaces the knowledge base filter controls with chip-style dropdowns (
Confidence Score: 4/5Safe to merge; all three filter pages work correctly with the new chip controls and the openMenu coordination logic handles all prop combinations without duplicating UI elements. The Calendar component's view state is lazy-initialized from the value prop but never re-synchronizes while mounted. In every current call site the calendar is inside a Popover that unmounts on close, so users never see a stale month, but the exported Calendar API has a subtle footgun for future direct-embedding use cases. The remaining findings are minor duplication and a string cast. No data loss or broken filtering paths are introduced by this PR. apps/sim/components/emcn/components/calendar/calendar.tsx — the view-state initialization pattern warrants a second look before Calendar is used outside a popover context. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[ResourceOptionsBar render] --> B{onFilterToggle?}
B -- yes --> C[Render toggle Button\nuncontrolled]
B -- no --> D{filter ReactNode?}
D -- yes --> E[Render Popover.Root\nopen = openMenu === 'filter'\nAnchor wraps Filter + Sort]
D -- no --> F[Render null]
C --> G{sort?}
E --> H{sort?}
F --> I{sort?}
G -- yes --> J[Standalone SortDropdown\nuncontrolled]
H -- yes --> K[Controlled SortDropdown\ninside Anchor\nopen = openMenu === 'sort']
I -- yes --> L[Standalone SortDropdown\nuncontrolled]
E --> M[PopoverContent\nalign=end, sideOffset=6\nanimated]
M --> N[filter children\ne.g. AutoWidthPanel with\nChipDropdown + TagFilterSection]
style E fill:#d4edda
style K fill:#d4edda
style J fill:#fff3cd
style L fill:#fff3cd
Reviews (1): Last reviewed commit: "improvement(knowledge): soften list filt..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const [view, setView] = useState(() => { | ||
| const base = selected ?? today | ||
| return { month: base.getMonth(), year: base.getFullYear() } | ||
| }) |
There was a problem hiding this comment.
view state is lazy-initialized once from value and never re-synchronizes when value changes externally while Calendar is mounted. In the current usage (inside a Popover that unmounts on close) this is safe, but Calendar is exported as a public component and embedding it directly — e.g. in a form where a parent resets the date — would leave the calendar stuck showing the wrong month. Adding a useEffect that resets the view whenever value changes keeps the public API correct.
| const [view, setView] = useState(() => { | |
| const base = selected ?? today | |
| return { month: base.getMonth(), year: base.getFullYear() } | |
| }) | |
| const [view, setView] = useState(() => { | |
| const base = selected ?? today | |
| return { month: base.getMonth(), year: base.getFullYear() } | |
| }) | |
| useEffect(() => { | |
| if (selected) { | |
| setView({ month: selected.getMonth(), year: selected.getFullYear() }) | |
| } | |
| }, [selected]) |
| const POPOVER_ANIMATION_CLASSES = | ||
| 'data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2 data-[state=closed]:animate-out data-[state=open]:animate-in motion-reduce:animate-none' |
There was a problem hiding this comment.
POPOVER_ANIMATION_CLASSES string here is byte-for-byte identical to FILTER_POPOVER_ANIMATION_CLASSES in resource-options-bar.tsx. Extracting it to a shared constant removes the duplication and ensures animation tweaks are applied consistently.
| const POPOVER_ANIMATION_CLASSES = | |
| 'data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2 data-[state=closed]:animate-out data-[state=open]:animate-in motion-reduce:animate-none' | |
| /** | |
| * Shared Radix popover enter/exit animation classes — keeps chip-family popovers | |
| * visually consistent. Also used in resource-options-bar.tsx. | |
| */ | |
| export const POPOVER_ANIMATION_CLASSES = | |
| 'data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2 data-[state=closed]:animate-out data-[state=open]:animate-in motion-reduce:animate-none' |
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!
| onChange={(value) => { | ||
| setEnabledFilter(value as 'all' | 'enabled' | 'disabled') |
There was a problem hiding this comment.
as cast widens the type without a runtime guard. If STATUS_FILTER_OPTIONS is ever extended without updating this handler, the cast silently accepts the new value and setEnabledFilter receives a string outside the union, breaking the filter logic.
| onChange={(value) => { | |
| setEnabledFilter(value as 'all' | 'enabled' | 'disabled') | |
| onChange={(value) => { | |
| if (value !== 'all' && value !== 'enabled' && value !== 'disabled') return | |
| setEnabledFilter(value) |
1 participant
Summary
ChipDatePicker,ChipMultiSelect, andCalendar.Type of Change
Testing
bunx biome checkpassed for touched files.git diff --checkpassed.Checklist
Screenshots/Videos