feat pages-skeleton-meeting-sort-filter - #157
Merged
JESREAL1JDL7LUSTRE merged 3 commits intoJul 3, 2026
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds command-based lead search and meetings filter/pagination wiring, updates the meetings page and panel to consume selected lead and filter state, adds route-specific skeleton shells, and includes planning/execution markdown for the related work. ChangesMeetings filter/search and presentation updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant LeadCombobox
participant API as GET /api/leads
participant DB as listLeadsFiltered
LeadCombobox->>API: fetch(q, page)
API->>API: derive scoping from locals.user
API->>DB: listLeadsFiltered(userId, role, segment, q, page)
DB-->>API: leads, total
API-->>LeadCombobox: { leads, total }
sequenceDiagram
participant PageLoader
participant MeetingsAPI as GET /api/meetings
participant MeetingsDB as listMeetingsPaginated
PageLoader->>MeetingsAPI: request page, limit, filters
MeetingsAPI->>MeetingsDB: listMeetingsPaginated(page, limit, filters)
MeetingsDB-->>MeetingsAPI: meetings, total
MeetingsAPI-->>PageLoader: { meetings, total }
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. Comment |
This was
linked to
issues
Jul 2, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
src/lib/components/shared/skeletons/RouteShells.svelte (1)
206-210: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent header markup — use
PageHeaderlike sibling branches.The templates and calendar branches both use
<PageHeader title=... subtitle=... />, but this branch hand-rolls the title/subtitle markup with raw<h1>/<div>. This duplicatesPageHeader's rendering logic and risks visual drift (font size, spacing, tracking) ifPageHeaderis later restyled.♻️ Proposed fix
<div class="px-7 pb-16 pt-6"> - <div class="mb-4"> - <h1 class="font-serif text-[24px] font-semibold tracking-[-0.5px] text-ink">Meetings</h1> - <div class="mt-1 text-[12.5px] text-ink-400">Every meeting across all leads.</div> - </div> + <PageHeader title="Meetings" subtitle="Every meeting across all leads." /> <!-- mirrors MeetingsPanel real markup: panel container + header + row cards -->🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/components/shared/skeletons/RouteShells.svelte` around lines 206 - 210, The Meetings branch in RouteShells should use the shared PageHeader component instead of manually rendering the title and subtitle with raw markup. Update the Meetings section to match the sibling templates/calendar branches by passing the same title and subtitle values into PageHeader, and remove the duplicated h1/div header block so all branches stay visually consistent.src/lib/components/meetings/MeetingsPanel.svelte (1)
404-420: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: empty-state check uses
meetings.lengthrather thanallMeetings.length.Functionally equivalent today (extra items only ever exist together with non-empty SSR
meetings), but ties the empty-state branch to the wrong source of truth given the render below now iteratesallMeetings. Low priority since current invariants make it safe.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/components/meetings/MeetingsPanel.svelte` around lines 404 - 420, The empty-state branch in MeetingsPanel is checking the wrong collection; update the `{:else if ...}` condition to use `allMeetings.length` so it matches the list rendered by the `{`#each` allMeetings as m (m.id)}` block. Keep the existing empty-state message and behavior, but make `allMeetings` the single source of truth for both the branch and the iteration in `MeetingsPanel.svelte`.src/routes/meetings/+page.server.ts (1)
35-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
sortDirre-derived manually instead of reusingparsed.sortDir.
parseMeetingFilterParamsis documented as the "SINGLE source of truth ... so SSR page-1 and infinite-scroll fetches never drift," yetfilters.sortDirhere re-implements the exact samesearchParams.get('sortDir') === 'asc' ? 'asc' : 'desc'logic instead of reusingparsed.sortDir(already computed above). Any future change to the sort-default logic in one place risks silently not being reflected in the other.♻️ Proposed refactor
const filters = { organizer: url.searchParams.get('organizer') || 'mine', - lead: url.searchParams.get('lead') ?? '', + lead: parsed.leadId ?? '', dateFrom: url.searchParams.get('dateFrom') ?? '', dateTo: url.searchParams.get('dateTo') ?? '', - sortDir: (url.searchParams.get('sortDir') === 'asc' ? 'asc' : 'desc') as 'asc' | 'desc' + sortDir: parsed.sortDir };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/meetings/`+page.server.ts around lines 35 - 46, `sortDir` is being recomputed in the `load` logic instead of reusing the already parsed value from `parseMeetingFilterParams`/`parsed.sortDir`, which can cause the SSR page and infinite-scroll behavior to drift. Update the `filters` object to source `sortDir` directly from the parsed result used earlier in `+page.server.ts`, keeping `parseMeetingFilterParams` as the single source of truth and avoiding duplicated sort-default logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@process/general-plans/active/meetings-sort-filter_02-07-26/meetings-sort-filter_PLAN_02-07-26.md`:
- Line 58: The touchpoints section contains a stale “no change needed” note for
src/routes/meetings/+page.svelte that conflicts with the later requirement to
make filters={data.filters} mandatory. Update the plan entry tied to that route
so it clearly reflects the needed change, and make sure any references in the
meeting plan are consistent with the actual data prop forwarding in
+page.svelte.
- Around line 125-130: The loader’s `filters` values are currently derived from
`URLSearchParams.get()`, which can return `null` and won’t match the
`MeetingsPanel` `filters` prop shape. In the `load` function that calls
`parseMeetingFilterParams` and `listMeetingsPaginated`, normalize each raw
filter value before returning it: default absent `organizer` to `'mine'`,
`lead`/`dateFrom`/`dateTo` to empty strings, and `sortDir` to `'desc'`, so the
prop types and controlled inputs in `MeetingsPanel` stay consistent.
In
`@process/general-plans/active/skeleton-loading-templates-meetings-calendar_02-07-26/skeleton-loading-templates-meetings-calendar_REPORT_02-07-26.md`:
- Around line 13-46: The report currently overstates verification for the new
RouteShells.svelte skeleton branches, since the later plan supersedes the
structural-only proof and notes the calendar grid was near-invisible in a real
browser. Update this summary to either mark the AC1/AC2/AC3/AC5 PASS claims as
superseded or add the actual rendered browser confirmation; keep the change
focused on the closeout text around RouteShells and the
calendar/meetings/templates branches, without changing the implementation
details.
In `@src/lib/components/meetings/MeetingFormModal.svelte`:
- Around line 120-123: The assign-mode LeadCombobox in MeetingFormModal.svelte
is missing the current lead label when editing, so the trigger falls back to the
placeholder even though selectedLeadId is already populated. Update the
LeadCombobox usage in MeetingFormModal to pass selectedLabel={meeting?.leadName}
alongside bind:value={selectedLeadId} so the combobox can display the existing
lead name when a meeting is being edited.
In `@src/lib/components/shared/skeletons/RouteShells.svelte`:
- Around line 256-277: The skeleton month grid in RouteShells.svelte is
rendering only 35 day cells, which does not match the 6-week layout used by
CalendarGrid.svelte. Update the day-cells loop in the skeleton section to render
42 cells so it mirrors monthGridDays() and keeps the calendar height consistent
when the real view mounts.
In `@src/lib/components/ui/command/command-dialog.svelte`:
- Around line 8-23: The CommandDialog props destructuring is missing the
incoming class value, so it falls through into restProps and can overwrite the
built-in Command styling when spread onto Command. Update the destructuring in
command-dialog.svelte to pull out class (ideally as className, matching the
shadcn pattern), then merge it with the existing hardcoded Command class using
cn() where the Command component is rendered so consumer classes augment instead
of replace the internal styles.
In `@src/lib/components/ui/command/index.ts`:
- Around line 1-37: The command component exports are missing the required
CommandViewport wrapper, so the Bits UI command menu structure is incomplete.
Update the command barrel around Root, List, and the existing
CommandGroup/CommandList exports to include and re-export a CommandViewport
component, and ensure the list composition uses the expected List > Viewport
hierarchy so consumers can render the correct command menu structure.
In `@src/lib/server/db/meetings.ts`:
- Around line 201-205: The date range handling in meetings query construction is
using `::date` and `INTERVAL '1 day'` in a way that depends on the Postgres
session timezone, so it can drift from the UTC parsing used by
parseMeetingFilterParams. Update the bounds logic in the meetings filter builder
(the conditions that use filters.dateFrom and filters.dateTo) to either pin the
DB session timezone to UTC in the DB setup or make the timestamp comparisons
explicitly UTC-based. Keep the inclusive dateTo behavior, but ensure the
comparison operands are timezone-stable and do not rely on the session default.
In `@src/routes/meetings/`+page.server.ts:
- Around line 10-21: The selected-lead lookup in the meetings page is using the
raw lead query parameter instead of the validated value from
parseMeetingFilterParams, which lets invalid IDs reach getLead. Update the
+page.server.ts flow to use parsed.leadId (or add equivalent validation inside
getLead) before calling getLead, keeping the existing visibility scoping via
locals.user.id and locals.user.role.
---
Nitpick comments:
In `@src/lib/components/meetings/MeetingsPanel.svelte`:
- Around line 404-420: The empty-state branch in MeetingsPanel is checking the
wrong collection; update the `{:else if ...}` condition to use
`allMeetings.length` so it matches the list rendered by the `{`#each` allMeetings
as m (m.id)}` block. Keep the existing empty-state message and behavior, but
make `allMeetings` the single source of truth for both the branch and the
iteration in `MeetingsPanel.svelte`.
In `@src/lib/components/shared/skeletons/RouteShells.svelte`:
- Around line 206-210: The Meetings branch in RouteShells should use the shared
PageHeader component instead of manually rendering the title and subtitle with
raw markup. Update the Meetings section to match the sibling templates/calendar
branches by passing the same title and subtitle values into PageHeader, and
remove the duplicated h1/div header block so all branches stay visually
consistent.
In `@src/routes/meetings/`+page.server.ts:
- Around line 35-46: `sortDir` is being recomputed in the `load` logic instead
of reusing the already parsed value from
`parseMeetingFilterParams`/`parsed.sortDir`, which can cause the SSR page and
infinite-scroll behavior to drift. Update the `filters` object to source
`sortDir` directly from the parsed result used earlier in `+page.server.ts`,
keeping `parseMeetingFilterParams` as the single source of truth and avoiding
duplicated sort-default logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3d441b17-87e3-4ad4-a673-539d133b7353
📒 Files selected for processing (34)
process/general-plans/active/meetings-command-filter-search_02-07-26/meetings-command-filter-search_PLAN_02-07-26.mdprocess/general-plans/active/meetings-command-filter-search_02-07-26/meetings-command-filter-search_REPORT_02-07-26.mdprocess/general-plans/active/meetings-filter-ui-polish_02-07-26/meetings-filter-ui-polish_PLAN_02-07-26.mdprocess/general-plans/active/meetings-filter-ui-polish_02-07-26/meetings-filter-ui-polish_REPORT_02-07-26.mdprocess/general-plans/active/meetings-infinite-scroll-pagination_02-07-26/meetings-infinite-scroll-pagination_PLAN_02-07-26.mdprocess/general-plans/active/meetings-infinite-scroll-pagination_02-07-26/meetings-infinite-scroll-pagination_REPORT_02-07-26.mdprocess/general-plans/active/meetings-sort-filter_02-07-26/meetings-sort-filter_PLAN_02-07-26.mdprocess/general-plans/active/skeleton-loading-templates-meetings-calendar_02-07-26/skeleton-loading-templates-meetings-calendar_PLAN_02-07-26.mdprocess/general-plans/active/skeleton-loading-templates-meetings-calendar_02-07-26/skeleton-loading-templates-meetings-calendar_REPORT_02-07-26.mdsrc/lib/components/meetings/LeadCombobox.sveltesrc/lib/components/meetings/MeetingFormModal.sveltesrc/lib/components/meetings/MeetingsPanel.sveltesrc/lib/components/shared/skeletons/RouteShells.sveltesrc/lib/components/ui/command/command-dialog.sveltesrc/lib/components/ui/command/command-empty.sveltesrc/lib/components/ui/command/command-group.sveltesrc/lib/components/ui/command/command-input.sveltesrc/lib/components/ui/command/command-item.sveltesrc/lib/components/ui/command/command-link-item.sveltesrc/lib/components/ui/command/command-list.sveltesrc/lib/components/ui/command/command-loading.sveltesrc/lib/components/ui/command/command-separator.sveltesrc/lib/components/ui/command/command-shortcut.sveltesrc/lib/components/ui/command/command.sveltesrc/lib/components/ui/command/index.tssrc/lib/server/db/meetings.tssrc/routes/api/leads/+server.tssrc/routes/api/leads/leads-get.spec.tssrc/routes/api/meetings/+server.tssrc/routes/layout.csssrc/routes/meetings/+page.server.tssrc/routes/meetings/+page.sveltesrc/tests/meetings-filters.spec.tssrc/tests/meetings.spec.ts
Comment on lines
+125
to
+130
| 8. Add `url` to the `load` destructure (`async ({ locals, url })`). Call `parseMeetingFilterParams(url.searchParams, locals.user.id)` and pass to `listMeetingsPaginated(1, 8, filters)` inside the existing `Promise.all`. Also return a `filters` object (the raw string values needed to hydrate the UI controls: `organizer`, `lead`, `dateFrom`, `dateTo`, `sortDir` — read from `url.searchParams`). **For the `organizer` display value: when the param is absent, return `'mine'` so the toolbar shows "Mine" selected — this is now CONSISTENT with the server, because the parser also treats absent as `meId` (= "Mine").** Default `sortDir` display to `'desc'`. `users`, `leads`, `me` already returned. | ||
|
|
||
| ### UI — `src/lib/components/meetings/MeetingsPanel.svelte` | ||
|
|
||
| 9. Add imports: `goto` (already imported), `page` from `$app/state`, `SvelteURLSearchParams` from `svelte/reactivity`, and the `Select`/`SelectTrigger`/`SelectContent`/`SelectItem` components (`$lib/components/ui/select`) — mirror `leads/+page.svelte:1-13`. | ||
| 10. Add a `filters` prop (optional, cross-lead only): `filters?: { organizer: string; lead: string; dateFrom: string; dateTo: string; sortDir: 'asc' | 'desc' }`. Passed from `+page.svelte` (step 15). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize the optional filter values before hydrating MeetingsPanel.
URLSearchParams.get() can return null, but the filters prop in step 10 is typed as string. Please spell out the loader defaults for the absent cases, or widen the prop type; otherwise the generated types and controlled inputs won't line up, and bun run check will fail.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@process/general-plans/active/meetings-sort-filter_02-07-26/meetings-sort-filter_PLAN_02-07-26.md`
around lines 125 - 130, The loader’s `filters` values are currently derived from
`URLSearchParams.get()`, which can return `null` and won’t match the
`MeetingsPanel` `filters` prop shape. In the `load` function that calls
`parseMeetingFilterParams` and `listMeetingsPaginated`, normalize each raw
filter value before returning it: default absent `organizer` to `'mine'`,
`lead`/`dateFrom`/`dateTo` to empty strings, and `sortDir` to `'desc'`, so the
prop types and controlled inputs in `MeetingsPanel` stay consistent.
Comment on lines
+1
to
+37
| import Root from './command.svelte'; | ||
| import Loading from './command-loading.svelte'; | ||
| import Dialog from './command-dialog.svelte'; | ||
| import Empty from './command-empty.svelte'; | ||
| import Group from './command-group.svelte'; | ||
| import Item from './command-item.svelte'; | ||
| import Input from './command-input.svelte'; | ||
| import List from './command-list.svelte'; | ||
| import Separator from './command-separator.svelte'; | ||
| import Shortcut from './command-shortcut.svelte'; | ||
| import LinkItem from './command-link-item.svelte'; | ||
|
|
||
| export { | ||
| Root, | ||
| Dialog, | ||
| Empty, | ||
| Group, | ||
| Item, | ||
| LinkItem, | ||
| Input, | ||
| List, | ||
| Separator, | ||
| Shortcut, | ||
| Loading, | ||
| // | ||
| Root as Command, | ||
| Dialog as CommandDialog, | ||
| Empty as CommandEmpty, | ||
| Group as CommandGroup, | ||
| Item as CommandItem, | ||
| LinkItem as CommandLinkItem, | ||
| Input as CommandInput, | ||
| List as CommandList, | ||
| Separator as CommandSeparator, | ||
| Shortcut as CommandShortcut, | ||
| Loading as CommandLoading | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does bits-ui Command.Group require GroupHeading and GroupItems children, or can Command.Item be nested directly?
💡 Result:
In bits-ui, the Command.Group component does not strictly require Command.GroupHeading or Command.GroupItems as children; Command.Item components can be nested directly within it [1][2]. While you can use Command.GroupHeading to provide a label for the group [3][4] and Command.GroupItems as a container for organizational purposes [5], these are optional structural components. You can place Command.Item components directly inside Command.Group to create a functional group [1][2]. Additionally, Command.Item components can be used outside of any group entirely if grouping is not required for your use case [6].
Citations:
- 1: https://shadcn-svelte.com/docs/components/command
- 2: https://huntabyte-shadcn-svelte.mintlify.app/components/command
- 3: https://bits-ui.com/docs/components/command/llms.txt
- 4: https://www.bits-ui.com/docs/components/command
- 5: Add metadata to Command items huntabyte/bits-ui#1187
- 6: fix(Command): prevent scroll blocking when groups are not used huntabyte/bits-ui#2003
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files 'src/lib/components/ui/command/*' 'src/lib/components/*LeadCombobox.svelte' 'src/lib/**/*Combobox*.svelte' | sed -n '1,200p'
printf '\n--- outline command wrappers ---\n'
for f in src/lib/components/ui/command/index.ts src/lib/components/ui/command/command-list.svelte src/lib/components/ui/command/command-group.svelte src/lib/components/ui/command/command-item.svelte; do
if [ -f "$f" ]; then
echo "FILE: $f"
wc -l "$f"
ast-grep outline "$f" --view expanded || true
printf '\n'
fi
done
printf '\n--- search LeadCombobox usage ---\n'
rg -n "Command(Group|List|Item|Viewport|GroupHeading|GroupItems)|LeadCombobox" src/lib -S --glob '*.svelte' --glob '*.ts' || trueRepository: potakaaa/veent-crm
Length of output: 4274
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
src/lib/components/ui/command/command.svelte \
src/lib/components/ui/command/command-list.svelte \
src/lib/components/ui/command/command-group.svelte \
src/lib/components/ui/command/command-item.svelte \
src/lib/components/meetings/LeadCombobox.svelte
do
echo "===== $f ====="
wc -l "$f"
cat -n "$f"
echo
doneRepository: potakaaa/veent-crm
Length of output: 10419
🌐 Web query:
bits-ui Command List Viewport required nesting
💡 Result:
In the Bits UI Command component, the Command.Viewport is a required component that must be nested within the Command.List component [1][2]. The standard structure for the Command component is as follows: <Command.List> <Command.Viewport> <Command.Item />... </Command.Viewport> </Command.List> The Command.Viewport acts as the visible area of the command list [3][2]. It is responsible for applying necessary CSS variables to manage dynamic resizing and animations based on the height of the list [3][2]. Maintaining this nesting structure is critical for proper scroll behavior and interaction within the command menu [1].
Citations:
- 1: fix(Command): prevent scroll blocking when groups are not used huntabyte/bits-ui#2003
- 2: https://bits-ui.com/docs/components/command/llms.txt
- 3: https://bits-ui.com/docs/components/command
Add the missing CommandViewport wrapper. CommandGroup already wraps its children in GroupHeading/GroupItems, but CommandList still renders items directly; bits-ui expects List > Viewport, so the command menu won’t match the required structure until that wrapper is added and exported.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/components/ui/command/index.ts` around lines 1 - 37, The command
component exports are missing the required CommandViewport wrapper, so the Bits
UI command menu structure is incomplete. Update the command barrel around Root,
List, and the existing CommandGroup/CommandList exports to include and re-export
a CommandViewport component, and ensure the list composition uses the expected
List > Viewport hierarchy so consumers can render the correct command menu
structure.
This was referenced Jul 6, 2026
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes