feat(cli): filter users list by department, role, and more#48
Conversation
Document department/role/job-title/email/name filters on abs users list.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThis PR extends users list support across the API client, core layer, and CLI. Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (2)
src/commands/users/index.ts (2)
281-290: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFiltered footer doesn't indicate remaining pages.
printFilteredFooter(total, ...)only prints the total matched count, unlikeprintPaginationFooterwhich hintsNext: --page N+1. With client-side filtering, a user seeing "200 results (filtered)" but only 20 rows has no cue that--page 2would surface more, unlike the native path. Consider passing page/items through so the filtered footer can offer the same next-page hint.🤖 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/commands/users/index.ts` around lines 281 - 290, The filtered footer path in users pagination only reports the matched total and omits the next-page hint, unlike the native pagination footer. Update the `printFilteredFooter` flow in `src/commands/users/index.ts` so it receives the current page and page size (similar to `printPaginationFooter`), and adjust `printFilteredFooter` to append a “Next: --page N+1” style cue when more filtered results may exist.
139-155: 🚀 Performance & Scalability | 🔵 TrivialFull-org scan latency for client-side filters.
fetchAllUserssequentially pages through every user (200/page) whenever any of--department/--role/--job-title/--nameis supplied, before filtering/paginating in memory. For large organizations this means many sequential API round-trips per invocation, with no visible progress indicator or backoff between calls. Worth keeping an eye on rate limits/latency as user counts grow; consider surfacing a progress hint or exploring incremental/streaming filtering if this becomes a bottleneck in practice.🤖 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/commands/users/index.ts` around lines 139 - 155, The client-side filtering path in users list currently triggers a full-org sequential scan via fetchAllUsers whenever clientFiltersActive is true, which can cause high latency for large orgs. Update the users command flow around fetchAllUsers/filterUsers to either surface a lightweight progress hint during the scan or refactor toward incremental/streaming filtering so pagination doesn’t require waiting for all pages first. Keep the existing scanOptions and matched pagination behavior intact.
🤖 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 `@src/commands/users/index.ts`:
- Around line 90-91: The users command option parsing currently allows both
--asc and --desc to be passed, and the current ternary in the users command
silently picks asc over desc. Update the option definitions in the users command
(the Option chain where --asc and --desc are declared, and the related handling
that reads options.asc/options.desc) to declare these flags as mutually
exclusive using Commander’s conflicts support so the CLI errors instead of
choosing one. Apply the same validation wherever the same asc/desc option pair
is defined in the command setup to keep behavior consistent.
In `@src/core/users/filter.test.ts`:
- Around line 44-48: The `hasClientFilters` test is using an invalid input
shape, since `search` and `sort` are not part of that function’s parameter type.
Update the `hasClientFilters` spec to only pass correctly typed filter flags,
and move the `search`/`sort` expectation to the caller-level test that accepts
those options or replace it with a valid object type for `hasClientFilters`.
---
Nitpick comments:
In `@src/commands/users/index.ts`:
- Around line 281-290: The filtered footer path in users pagination only reports
the matched total and omits the next-page hint, unlike the native pagination
footer. Update the `printFilteredFooter` flow in `src/commands/users/index.ts`
so it receives the current page and page size (similar to
`printPaginationFooter`), and adjust `printFilteredFooter` to append a “Next:
--page N+1” style cue when more filtered results may exist.
- Around line 139-155: The client-side filtering path in users list currently
triggers a full-org sequential scan via fetchAllUsers whenever
clientFiltersActive is true, which can cause high latency for large orgs. Update
the users command flow around fetchAllUsers/filterUsers to either surface a
lightweight progress hint during the scan or refactor toward
incremental/streaming filtering so pagination doesn’t require waiting for all
pages first. Keep the existing scanOptions and matched pagination behavior
intact.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 066fbecf-cd4b-4b7d-8336-73a610e13661
📒 Files selected for processing (10)
README.mdpackage.jsonsrc/api-client/api-client.test.tssrc/api-client/api-client.tssrc/commands/users/index.tssrc/commands/users/users.test.tssrc/core/users/filter.test.tssrc/core/users/filter.tssrc/core/users/list.tssrc/core/users/users.test.ts
| .option('--asc', 'sort in ascending order') | ||
| .option('--desc', 'sort in descending order') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
No validation for conflicting --asc/--desc.
options.asc ? true : options.desc ? false : undefined silently prefers --asc when both flags are passed, with no warning to the user. Commander supports .conflicts() on Option instances for exactly this case.
🐛 Proposed fix
const sortAsc = options.asc ? true : options.desc ? false : undefined;
+ if (options.asc && options.desc) {
+ console.error('Error: --asc and --desc cannot be used together');
+ process.exit(1);
+ }Also applies to: 124-124
🤖 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/commands/users/index.ts` around lines 90 - 91, The users command option
parsing currently allows both --asc and --desc to be passed, and the current
ternary in the users command silently picks asc over desc. Update the option
definitions in the users command (the Option chain where --asc and --desc are
declared, and the related handling that reads options.asc/options.desc) to
declare these flags as mutually exclusive using Commander’s conflicts support so
the CLI errors instead of choosing one. Apply the same validation wherever the
same asc/desc option pair is defined in the command setup to keep behavior
consistent.
| describe('hasClientFilters', () => { | ||
| it('is false when no client filter flags are set', () => { | ||
| expect(hasClientFilters({})).toBe(false); | ||
| expect(hasClientFilters({ search: 'x', sort: 'name' })).toBe(false); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant files.
git ls-files 'src/core/users/filter.test.ts' 'src/core/users/filter.ts'
# Show the test around the cited lines.
echo '--- src/core/users/filter.test.ts ---'
cat -n src/core/users/filter.test.ts | sed -n '35,60p'
# Show the implementation and the parameter type for hasClientFilters.
echo '--- src/core/users/filter.ts ---'
cat -n src/core/users/filter.ts | sed -n '1,220p'
# Check whether TypeScript tooling is available for a quick read-only probe.
node -e "try { require('typescript'); console.log('typescript:available') } catch (e) { console.log('typescript:missing') }"Repository: absmartly/cli-ts
Length of output: 9891
Fix the invalid hasClientFilters test input hasClientFilters({ search: 'x', sort: 'name' }) does not match the function’s parameter type, so this test will fail tsc --noEmit. Move the search/sort assertion to the caller that accepts those options, or use a correctly typed input here.
🤖 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/core/users/filter.test.ts` around lines 44 - 48, The `hasClientFilters`
test is using an invalid input shape, since `search` and `sort` are not part of
that function’s parameter type. Update the `hasClientFilters` spec to only pass
correctly typed filter flags, and move the `search`/`sort` expectation to the
caller-level test that accepts those options or replace it with a valid object
type for `hasClientFilters`.
Source: Coding guidelines
Address PR review:
- resolveRoleIds now validates numeric role IDs via a batched listRoles({ids})
lookup and errors on unknown IDs (symmetric with name resolution), instead of
silently matching zero users on a typo'd ID.
- Document why userGlobalRoleIds uses Record<string,unknown>: user_team_roles is
API-populated but absent from the generated OpenAPI schema.
- Add command test for page 2 of a client-filtered matched set spanning two scan
pages, plus numeric-ID validation tests.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/users/filter.ts (1)
151-169: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSequential per-name role lookups could be parallelized.
Each name token in
valuestriggers its own sequentialawait client.listRoles(...)call. With multiple--rolenames, resolution latency scales linearly with the number of names. Since each lookup is independent, this could run concurrently viaPromise.all, while still applyingadd()in the original token order afterwards.🤖 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/core/users/filter.ts` around lines 151 - 169, The role resolution loop in filter logic is doing independent `client.listRoles` lookups one at a time, which makes multiple `--role` names resolve slower than necessary. Update the `names` handling in the role filter path to start all lookups concurrently with `Promise.all`, then process the resolved results in the original token order so `add(Number(exact[0]!.id))` still preserves input order and the existing “no role”/“multiple roles” errors remain unchanged.
🤖 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.
Nitpick comments:
In `@src/core/users/filter.ts`:
- Around line 151-169: The role resolution loop in filter logic is doing
independent `client.listRoles` lookups one at a time, which makes multiple
`--role` names resolve slower than necessary. Update the `names` handling in the
role filter path to start all lookups concurrently with `Promise.all`, then
process the resolved results in the original token order so
`add(Number(exact[0]!.id))` still preserves input order and the existing “no
role”/“multiple roles” errors remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1603ae7f-51fd-4d0e-bd6f-ad2d0287bb90
📒 Files selected for processing (3)
src/commands/users/users.test.tssrc/core/users/filter.test.tssrc/core/users/filter.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/commands/users/users.test.ts
- src/core/users/filter.test.ts
…y filters Address second round of PR review: - Roles column (and its GET /roles scan) now only fires for client field filters, not plain --search/--ids. - printFilteredFooter shows 'page X/Y' + 'Next: --page N+1' when the matched set spans multiple pages, matching the --metric footer parity claim. - A filter flag that parses to zero usable values (e.g. --department ' , ') now errors instead of silently returning the full list. - Clarify in comments that /users only returns global-team roles (backend include filters is_global_team) and that role resolution scans all roles. Verified the two review 'critical' shape/scope claims against the captured live /users payload (absmartly-api-mocks live-payloads/users-list.json): role_ids is the array form and the entry is the global team, so the existing code is correct.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/utils/pagination.ts (1)
10-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent parameter ordering across sibling footer helpers.
printPaginationFooter(count, items, page, outputFormat),printFilteredFooter(count, outputFormat, page, items), andprintMetricFooter(total, metricName, metricId, page, items, outputFormat)all placeoutputFormat/page/itemsin different positions. This is an easy source of accidental argument-order bugs at new call sites.Consider standardising these three signatures (e.g. always
outputFormatlast, or accept a single options object) to reduce the risk of transposed arguments in future call sites.Also applies to: 24-37, 40-56
🤖 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/utils/pagination.ts` around lines 10 - 22, The footer helper signatures are inconsistent across printPaginationFooter, printFilteredFooter, and printMetricFooter, which makes call sites prone to transposed arguments. Standardize the parameter order for these sibling helpers by choosing one convention (for example, keep outputFormat last everywhere, or switch them all to a single options object). Update the helper declarations and every call site that uses these functions so the ordering is uniform and easy to follow.
🤖 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.
Nitpick comments:
In `@src/lib/utils/pagination.ts`:
- Around line 10-22: The footer helper signatures are inconsistent across
printPaginationFooter, printFilteredFooter, and printMetricFooter, which makes
call sites prone to transposed arguments. Standardize the parameter order for
these sibling helpers by choosing one convention (for example, keep outputFormat
last everywhere, or switch them all to a single options object). Update the
helper declarations and every call site that uses these functions so the
ordering is uniform and easy to follow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 21f62f93-e258-46b4-9c04-1307313a895f
📒 Files selected for processing (7)
.gitignoreREADME.mdsrc/commands/users/index.tssrc/commands/users/users.test.tssrc/core/users/filter.tssrc/lib/utils/pagination.test.tssrc/lib/utils/pagination.ts
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/commands/users/index.ts
- src/core/users/filter.ts
1 participant
Summary
Adds department, role, job-title, email, and name filters to
abs users list, with multi-value support — addressing a customer request for role-assignment reviews across ~600 users (e.g. "everyone in Ancillaries", "everyone with the API User role").The ABsmartly
/usersAPI has no server-side department/role filter and only a single fuzzysearchparam, so the field-specific, exact, multi-value filters are applied client-side (scan all pages at 200/page, filter in memory, paginate the matched set) — mirroring the--metricpattern from PR #47. Native--search/--sort/--idsare also newly exposed and passed through to the API.Behavior
New flags on
abs users list:--department <values>--role <values>role_ids--job-title <values>--email <values>--name <values>--contains--search <query>--sort/--asc/--desc--ids <ids>--department Ancillaries,Finance --role "API User"→ (Ancillaries OR Finance) AND has-API-User-role.--roleaccepts names (resolved viaGET /roles, exact case-insensitive; errors on unmatched/ambiguous) or numeric IDs.rolescolumn is shown and a filtered footer prints the matched count.Examples
Testing
src/core/users/filter.test.ts— 38 unit tests (parsing, role resolution incl. error paths, 200/page scan, OR/AND predicates,--contains, role formatting).src/api-client/api-client.test.ts+src/core/users/users.test.ts— native param serialization/passthrough.src/commands/users/users.test.ts— 14 command tests (native passthrough without scan, client scan+filter, role resolution + roles column,--contains, raw passthrough, error handling).Carries its own minor bump to 1.13.0 per the repo's one-minor-per-release convention.
🤖 Generated with Claude Code
Summary by CodeRabbit
users listwith--search,--sort(with--asc/--desc) and--ids, plus exact filters for--department,--role,--job-title,--email, and--name.--containsfor case-insensitive substring matching, with improved local filtering and pagination.--rawoutput is unchanged.