[CSM Portal] File-based dashboard definitions, dashboard/team types, startup-resolved team registry, and widget criteria key rename - #1341
Merged
Rashmika998 merged 17 commits intoAug 3, 2026
Conversation
…amed branches
The cross-field-OR key on the case-search filter contract was awkward twice
over: `orGroups` said "groups" without saying what a group meant, and each
branch was a bare array whose AND semantics were invisible. The public API
now reads:
"anyOf": [ { "filters": [ ... ] }, { "filters": [ ... ] } ]
Semantics are unchanged: predicates within a `filters` array AND together,
`anyOf` branches OR together, and the whole `anyOf` result ANDs with the
sibling `filters` array.
The ServiceNow wire format is deliberately NOT renamed. `snCaseFilters` still
emits `orGroups` in its original flat named-field branch shape, because the
CaseUtils Script Include reads that key and silently ignores JSON keys it does
not recognise -- a renamed wire key would return an unfiltered count with no
error anywhere. `TestSNCaseService_SearchCases_AnyOfKeepsSNOrGroupsWireFormat`
asserts this against the RAW outgoing JSON (a typed decode would happily
re-map a renamed key), so the guard cannot rot. No backing-service change is
needed.
Validation is unchanged; only the error messages move from `orGroups...` to
`anyOf...` so they name the field the caller actually sent.
A dashboard widget's criteria object nested one `filters` inside another
(`filters.filters`), which read as a stutter and made it ambiguous which
level a reader meant. The widget-config key is now `query`, on both the
widget and its pie/bar slices:
{ "id": "...", "query": { "filters": [ ... ], "anyOf": [ ... ] } }
The criteria object's OWN inner `filters` array keeps its name, and so does
the search request body's `filters` property -- only the config key that
carries the criteria object changed, plus the cross-field-OR key inside it
(`orGroups` -> `anyOf`, tracking the entity-service contract).
The loader accepts both spellings, which is not optional here: DASHBOARDS_CONFIG
lives in an environment variable, so the rename is never atomic with a config
rollout, and encoding/json leaves an unknown key's field at its zero value --
an un-migrated config would give every widget a nil query and render 0 with no
error, no failed request, and nothing to notice. A legacy config is therefore
migrated in place at load with one deprecation warning per widget or slice it
had to touch, so a deployment still on the old shape is visible in the logs
rather than silently working forever. Where both spellings are present the
current one wins.
…AND isTeamBased together The initial dashboard pick used isDefault and isTeamBased as two independent signals (any isTeamBased entry for a user with a team, any isDefault entry otherwise), so a team-based dashboard that wasn't also flagged isDefault could still win over the BE's own default for a user with a team, and a non-default team-based dashboard could be picked over a non-team default. Per product's revised rule, a user with a resolved team now requires the SAME entry to carry both isDefault and isTeamBased; a user without one requires isDefault and NOT isTeamBased on the same entry. Falls back to any isDefault entry, then the first dashboard in the list, so a registry with no exact match never renders nothing.
…p shadow hover for border/tint - Count-shape widget numbers: fontWeight 700 -> 400, per product's revised visual preference for the big number. - Removes the per-widget refresh IconButton (and the now-dead refetch plumbing it alone used, incl. useWidgetPieData's own refetch) from every shape, reintroducing the earlier info-icon affordance in its place for shape "count" specifically -- matching where it lived before it was replaced by the refresh button, and now with real content: a Tooltip-wrapped, keyboard-accessible icon showing the widget's own `description` when one is set. - Replaces the count-shape click-through's boxShadow hover, and the pie/bar tile-level target's plain action-hover, with a single shared border+background-tint+lift idiom (no box-shadow) -- matching the customer-portal app's own clickable summary-card hover (see UpdateCardBreakdown.tsx), just without its box-shadow component per the "instead of shadow" ask, so shape "count" and shape "pie"/"bar" share one hover look.
… and lock in the team-switch cache-key fix Adds coverage for DashboardWidgetTile no longer rendering a refresh button on any shape, and for the shape "count" info icon rendering only when a description is actually set. Also adds a regression guard for the team-switch caching bug: switching selectedTeamGroupId must produce a fresh, differently-scoped request rather than reusing a prior team's cached response. Verified this guard fails against a query key built from the raw (unresolved) filters prop -- the queryKey already uses the resolved filters on this branch (see the 'default to the user's own team...' commit), so there is no remaining production-code bug to fix here; this test just locks that fix in.
The family field classified a team as cre or sre. It now also records whether the team is an account-based team: cre-abt, cre, sre-abt, sre. A dashboard scoped to one discipline offers only that discipline's ABTs in its team picker, which the two-value enum could not express. The registry row format is unchanged (teamKey|displayName[|family [|groupSysID]]) and existing cre/sre rows keep parsing exactly as before; only the accepted vocabulary widens, still case-insensitively. An unrecognised family is now rejected at startup naming the offending row, reversing this parser's previous pass-through behaviour. Passing an unknown value through was safe while nothing branched on the family. Now that the team picker and default-dashboard selection both do, a typo like "sre_abt" errors nowhere and instead silently removes a team from every picker. A rejected deploy is the cheaper failure.
… type DASHBOARDS_CONFIG held every dashboard in one environment variable. Two dashboards and 46 widgets in, it is unreviewable in a diff and an error about it has nothing to name. Definitions now live in a directory, one JSON file per dashboard, pointed at by DASHBOARDS_DIR. The filename is irrelevant: id, displayName and type all come from the file's content; files are read in filename order purely so the picker's order is stable. Read once at startup and held in memory, so there is no per-request disk I/O. DASHBOARDS_HOT_RELOAD=true instead re-reads the directory on every request, for local development only. Every failure fails startup naming the offending file: unreadable, malformed, missing/duplicate id, missing displayName, unknown type. None of them is recoverable by skipping the file, because a dropped dashboard is invisible -- the picker just has one fewer entry with nothing saying why. Malformed DASHBOARDS_CONFIG is now fatal for the same reason; it used to log once and silently empty every dashboard in the product. The hot-reload path is deliberately the exception: a failed re-read logs loudly and keeps serving the last known-good definitions. The startup load already proved the directory good, so a failure there is almost always an editor mid-save, and blanking a dev server's dashboards on every keystroke is not a usable loop. Adds "type" (cre | sre | cs) alongside isDefault and isTeamBased rather than deriving any of the three. Three independent fields can contradict each other, so the loader rejects the states that cannot mean anything: a team-scoped cre/sre dashboard with isTeamBased false, an organisation-wide cs dashboard with isTeamBased true, and two isDefault dashboards sharing a type (selection asks for "the default of type X"). DASHBOARDS_CONFIG stays as a deprecated fallback, used only when no directory is set and warning when it is. "type" is not required there: values already deployed predate the field, and a definition without one just gets a warning and stays invisible to automatic selection. The live definitions directory is gitignored, in the same class as .env: its widget criteria reference record ids that differ per environment. dashboards.example/ carries the schema.
github-actions
Bot
added
App/CSM Portal
Area/Backend
Area/Frontend
Entity Service
Platform/Web
labels
Contributor
|
Warning Review limit reached
Next review available in: 29 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (26)
📝 WalkthroughWalkthroughThe PR adds directory-backed dashboard and reference registries, validates configuration at startup, supports optional dashboard hot reload, updates dashboard criteria to ChangesDashboard registry and query migration
Reference directory and user integration
Entity service search contract updates
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
apps/csm-portal/backend/cmd/server/main.go (1)
277-300: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWarn when
DASHBOARDS_CONFIGis set but ignored.When
DASHBOARDS_DIRis set, line 278 takes the directory branch andDASHBOARDS_CONFIGis never read. Nothing records that. A deployment that still carries the old variable during migration gets no signal that its content has no effect.Add one warning in the directory branch. This keeps the migration state visible in the logs.
♻️ Proposed refactor
hotReload := strings.EqualFold(strings.TrimSpace(os.Getenv("DASHBOARDS_HOT_RELOAD")), "true") + if strings.TrimSpace(os.Getenv("DASHBOARDS_CONFIG")) != "" { + slog.Warn("DASHBOARDS_CONFIG is set but ignored because DASHBOARDS_DIR takes precedence; remove DASHBOARDS_CONFIG", + "dir", dir) + } registry, err := dashboard.NewDirRegistry(dir, hotReload)🤖 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 `@apps/csm-portal/backend/cmd/server/main.go` around lines 277 - 300, Add a warning in loadDashboards when DASHBOARDS_DIR is non-empty and DASHBOARDS_CONFIG is also set, indicating that DASHBOARDS_CONFIG is ignored because directory-based definitions are active. Keep the existing directory registry behavior unchanged and emit the warning only within that branch.apps/csm-portal/backend/internal/handler/dashboards_test.go (1)
41-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd wire coverage for the new
typefield.Neither fixture dashboard sets
type.dashboardListItemView.TypeanddashboardDetailView.Typeboth usejson:"type,omitempty", sotypenever appears in any response this file asserts. The new field thatopenapi.yamldeclares at lines 5405 and 5437 stays unverified here.Set
"type"on the fixture dashboards and assert it in the list and detail responses. Keep the values consistent with the loader rules:csrequiresisTeamBasedfalse, andcrerequires it true.♻️ Proposed fixture change
const testDashboardsConfigJSON = `[ - {"id":"sample-dashboard","displayName":"Sample Dashboard","isDefault":true,"targetTeam":"sample-team","widgets":[ + {"id":"sample-dashboard","displayName":"Sample Dashboard","type":"cs","isDefault":true,"targetTeam":"sample-team","widgets":[- {"id":"sample-team-dashboard","displayName":"Sample Team Dashboard","targetTeam":"sample-team","isTeamBased":true,"widgets":[ + {"id":"sample-team-dashboard","displayName":"Sample Team Dashboard","type":"cre","targetTeam":"sample-team","isTeamBased":true,"widgets":[Then add
"type"todashboardListItemJSONKeysanddashboardDetailJSONKeys, and assert the value per dashboard id.🤖 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 `@apps/csm-portal/backend/internal/handler/dashboards_test.go` around lines 41 - 61, Update the fixture dashboards in TestMain to include valid type values: use “cs” for the non-team dashboard and “cre” for the isTeamBased dashboard. Extend dashboardListItemJSONKeys and dashboardDetailJSONKeys to include type, then update the list and detail response assertions to verify the expected type for each dashboard ID.
🤖 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 `@apps/csm-portal/backend/internal/dashboard/registry.go`:
- Around line 291-320: Move the isDefault duplicate-tracking block in the
dashboard validation flow before the d.Type == "" branch, so untyped definitions
are also checked using the empty type key. Preserve the existing type
validation, warnings, and contradictory-configuration checks, while ensuring
multiple untyped defaults return the same duplicate error instead of being
skipped by continue.
In `@apps/csm-portal/backend/internal/dashboard/widgets.go`:
- Around line 234-296: Update finalize and migrateLegacyWidgetKeys to pass each
sourced.source value through migration, and include it in all legacy-key
warnings so directory-loaded files identify their filename or config index. In
migrateLegacyCriteriaKeys, check for an existing anyOf before deleting orGroups,
preserving the deprecated key and warning for half-migrated criteria. Also warn
and leave the key intact when orGroups is not an array instead of silently
dropping it.
In `@entity-service/internal/service/case_filters.go`:
- Around line 565-568: The ParseCaseFieldFilterGroups loop must rewrite errors
from ParseCaseFieldFilters to include the branch location as
anyOf[index].filters before returning them. Preserve the existing validation
details while adding the indexed anyOf path, and add coverage for invalid fields
and operators within a branch.
In `@entity-service/openapi.yaml`:
- Around line 3032-3038: Update the family properties in
entity-service/openapi.yaml at lines 3032-3038, 3337-3341, and 3397-3398 to use
the closed enum values cre-abt, cre, sre-abt, and sre. At lines 3032-3038,
document that an unclassified team omits the optional property rather than
returning an empty string; apply the enum consistently at all three sites.
---
Nitpick comments:
In `@apps/csm-portal/backend/cmd/server/main.go`:
- Around line 277-300: Add a warning in loadDashboards when DASHBOARDS_DIR is
non-empty and DASHBOARDS_CONFIG is also set, indicating that DASHBOARDS_CONFIG
is ignored because directory-based definitions are active. Keep the existing
directory registry behavior unchanged and emit the warning only within that
branch.
In `@apps/csm-portal/backend/internal/handler/dashboards_test.go`:
- Around line 41-61: Update the fixture dashboards in TestMain to include valid
type values: use “cs” for the non-team dashboard and “cre” for the isTeamBased
dashboard. Extend dashboardListItemJSONKeys and dashboardDetailJSONKeys to
include type, then update the list and detail response assertions to verify the
expected type for each dashboard ID.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4bc8edd0-276f-4994-8bc4-a76bef4e0738
📒 Files selected for processing (38)
apps/csm-portal/backend/.env.exampleapps/csm-portal/backend/.gitignoreapps/csm-portal/backend/cmd/server/main.goapps/csm-portal/backend/dashboards.example/sample-dashboard.jsonapps/csm-portal/backend/dashboards.example/sample-team-dashboard.jsonapps/csm-portal/backend/internal/dashboard/registry.goapps/csm-portal/backend/internal/dashboard/registry_test.goapps/csm-portal/backend/internal/dashboard/widgets.goapps/csm-portal/backend/internal/dashboard/widgets_test.goapps/csm-portal/backend/internal/handler/dashboards.goapps/csm-portal/backend/internal/handler/dashboards_test.goapps/csm-portal/backend/openapi.yamlapps/csm-portal/webapp/src/api/backend/types.tsapps/csm-portal/webapp/src/features/csm-dashboard/api/useDashboard.test.tsxapps/csm-portal/webapp/src/features/csm-dashboard/api/useWidgetPieData.test.tsxapps/csm-portal/webapp/src/features/csm-dashboard/api/useWidgetPieData.tsapps/csm-portal/webapp/src/features/csm-dashboard/components/AgentsLandingPagePilot.test.tsxapps/csm-portal/webapp/src/features/csm-dashboard/components/AgentsLandingPagePilot.tsxapps/csm-portal/webapp/src/features/csm-dashboard/components/DashboardWidgetTile.test.tsxapps/csm-portal/webapp/src/features/csm-dashboard/components/DashboardWidgetTile.tsxapps/csm-portal/webapp/src/features/csm-dashboard/config/widgetResourceConfig.tsapps/csm-portal/webapp/src/features/csm-dashboard/pages/CsmDashboardPage.test.tsxapps/csm-portal/webapp/src/features/csm-dashboard/pages/CsmDashboardPage.tsxapps/csm-portal/webapp/src/features/csm-dashboard/utils/widgetFilterMerge.tsentity-service/.env.exampleentity-service/CLAUDE.mdentity-service/README.mdentity-service/internal/config/config.goentity-service/internal/domain/abt_team.goentity-service/internal/domain/abt_team_test.goentity-service/internal/domain/entity.goentity-service/internal/service/case_filters.goentity-service/internal/service/case_filters_test.goentity-service/internal/service/case_service.goentity-service/internal/service/case_service_test.goentity-service/internal/service/sn_case_service.goentity-service/internal/service/sn_case_service_test.goentity-service/openapi.yaml
…backend
Both were entity-service configuration, which meant every team-catalogue
request and every team-name lookup crossed a service boundary to read a value
that cannot change while the process runs.
They are now the portal backend's, resolved once at startup into an in-memory
index (internal/directory): team key <-> display name <-> backing group id <->
this platform's UUID form of that id, plus the assignable-role allow-list. The
whole mapping is derivable from the configured rows alone, so POST /teams/search
and POST /roles/search now make no upstream call at all -- on the first request
or any later one. The resolved team and role counts are logged at startup, and a
malformed row still fails startup naming the offending row rather than quietly
emptying a dropdown at the first request.
What could not move is membership: which users belong to which group is live
state. So the entity-service keeps the membership query and gains what it needs
to run one without a registry:
- GET /users/me returns the caller's `groups` instead of a resolved `team`. The
portal backend maps a group name to a team from its own index; the membership
call itself is unchanged, one per request as before.
- POST /users/search takes `groupNames` in place of `teamIds`. The portal
backend translates each team key to its group name before forwarding, and
rejects an unknown key with a 400 instead of returning a confidently empty
page. groupNames rather than groupIds because the registry is keyed by name:
ids differ per environment, and several configured teams carry no id at all.
- GET /users/{id} returns `groups` only; the portal backend derives the `teams`
block from them, so its own response shape is unchanged.
roleIds validation moved with the allow-list, so the role dropdown and the
filter still come from one list and cannot disagree.
The portal backend's public contract is unchanged throughout -- /teams/search,
/roles/search, /users/me team, /users/{id} teams and the /users/search teamIds
filter all keep their shapes, including `family`, which the team picker filters
on. integrationCsTeam is untouched: it already takes resolved ids and never
consulted the registry.
Verified against a local stack on a real backing data source: /users/me,
/teams/search, /roles/search, /users/search with a team filter and
GET /users/{id} all return byte-identical responses before and after, and 50
consecutive catalogue requests issue zero upstream requests.
… source in migration warnings Two review findings in the definition loader, both on the path shared by DASHBOARDS_DIR and the deprecated DASHBOARDS_CONFIG. Untyped dashboards escaped the duplicate-isDefault check entirely. validate continued past an untyped definition before it reached the isDefault block, so the deprecated variable -- the only path that tolerates a missing type -- could carry two isDefault dashboards and pass. Which one the frontend then picked came down to the order they happened to appear in the JSON array, the exact failure the rule exists to prevent. The check now runs before the type branch. Untyped definitions share the empty type key, so keying defaultByType on "" groups them without any special case, and one untyped default still loads. The deprecated-key migration named DASHBOARDS_CONFIG in every warning even though finalize runs it for both loaders, so an operator on DASHBOARDS_DIR was told to fix a variable their deployment does not set, with no filename to go on. migrateLegacyWidgetKeys now takes one dashboard plus its sourced.source -- a file path or a DASHBOARDS_CONFIG[i] index -- and logs it, and the messages no longer name a configuration mechanism at all. Two silent drops in the same function now warn: an orGroups deleted because anyOf was already set, and an orGroups that is not an array. Both were keys the loader recognised and then discarded with nothing in the logs.
A branch is parsed by ParseCaseFieldFilters, which roots every validation message it raises at the top-level "filters" path. ParseCaseFieldFilterGroups returned those errors unchanged, so an invalid field or operator inside a branch came back as "filters: unsupported field: x" -- pointing a client at a filter array that may not even exist on the request, instead of the anyOf[index].filters the contract promises and the client actually has to edit. Branch errors are now relocated to anyOf[i].filters, carrying the offending branch's own index rather than always the first. The ValidationError type is preserved so the response is still a 400, and an error not rooted at "filters:" is returned untouched rather than having a path glued onto a message that never carried one. The index format matches the anyOf[%d] messages the group validator in sn_case_service.go already emits. Tests cover an invalid field, an invalid operator, an unsupported field/op combination, and each of those in a later branch to pin the index.
…nd "" The runtime parses exactly four family values and fails startup on anything else, but all three response schemas carrying a family declared a bare string, so the published contract permitted any value the implementation would refuse. Each now declares enum [cre-abt, cre, sre-abt, sre]. They also documented "may be empty" for an unclassified team. An empty string is not one of the four values, and two of the three responses already omit the property instead. The odd one out was the caller's own team on GET /users/me, which serialised "family": "" -- now omitempty like the others, so absence is expressed one way across the contract. The frontend already treats family as optional and truthiness-guards every read of it, so nothing changes for it. Same finding as the one raised against the entity-service schema before the team registry moved into this backend; these are where those schemas live now.
Contributor
|
🤖 Claude review complete — 12 comment(s) posted (view review) 🟠 Medium: 5 · 🟡 Minor: 7 Reviewed by Claude on behalf of @shayanmalinda |
rksk
changed the title
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Went through all 66 files: the dashboard loader, the entity-service changes, the webapp, and the team/role move.
I left 12 comments — 5 medium, 7 minor. Start with the one about the config relocation. The PR description says moving CSM_TEAM_REGISTRY and CSM_USER_ROLES to the portal was deliberately left out of this PR, but that move is actually the biggest part of the diff, so it's easy to skip over when reviewing.
Both Go modules build and pass their tests. I couldn't run the webapp typecheck because node_modules isn't installed here.
"anyOf": [{}] decoded cleanly and reached the backing data source as an
unconstrained orGroups entry. Because branches are OR'd against each
other, one such branch matched every case and silently widened the whole
result set -- a 200 with wrong data and nothing in any log.
ParseCaseFieldFilterGroups now rejects a branch with an empty (or absent)
filters array with a 400 naming the branch index, and the schema carries
minItems: 1. A predicate whose values array is empty was already
rejected, so an empty branch was the only way to produce an
unconstrained group.
Their only consumer was defaultUserRoles in internal/service/user_roles.go, deleted earlier in this branch when the assignable-role allow-list moved into the portal backend's directory package. The UserRole type stays -- SearchUsersFilters.RoleIDs uses it -- but the constants read as validation this service no longer performs.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
entity-service/internal/service/case_filters.go (1)
592-607: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winIndex errors from
rejectUnsupportedOrGroupFields.Line 609 returns these errors without the known branch index. Unsupported branch fields still report an unindexed
anyOf:path. ReturnanyOf[i].filters:so clients can locate the invalid branch.
entity-service/internal/service/case_filters.go#L592-L607: PassiintorejectUnsupportedOrGroupFields, or wrap its validation errors withanyOf[i].filters.entity-service/internal/service/case_filters_test.go#L394-L457: Add first-branch and later-branch cases for unsupported branch fields.The PR objective requires branch errors at
anyOf[i].filters.🤖 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 `@entity-service/internal/service/case_filters.go` around lines 592 - 607, The validation errors returned by rejectUnsupportedOrGroupFields currently lose the OR-branch index. In entity-service/internal/service/case_filters.go:592-607, pass i into rejectUnsupportedOrGroupFields or wrap its errors so unsupported fields report the anyOf[i].filters path. In entity-service/internal/service/case_filters_test.go:394-457, add coverage for unsupported fields in both the first and a later branch, asserting the indexed error paths.apps/csm-portal/backend/openapi.yaml (1)
1021-1028: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winTeam.groupId schema does not declare
format: uuiddespite the updated description.The postTeamsSearch description now states the response includes "the UUID form of its backing group id."
Team.groupId(schema, unchanged in this diff) is declared as plaintype: stringwith noformat: uuid.directory.go'sNew()populatesGroupIDthroughsourceIDToUUID, so the value is always UUID-shaped when present. Addformat: uuidtoTeam.groupIdso the schema matches the documented and actual contract.🛠️ Proposed fix for the Team.groupId schema
groupId: type: string + format: uuid description: > The backing group's id, suitable for the case search integrationCsTeamIds filter. Present only when the deployment's team registry configured a backing group id for this team; a team without one is still listed, just not filter-scopable.🤖 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 `@apps/csm-portal/backend/openapi.yaml` around lines 1021 - 1028, Update the Team.groupId schema to declare the UUID format alongside its existing string type. Keep the current optionality and other schema properties unchanged, so the OpenAPI contract matches the UUID value produced by directory.New through sourceIDToUUID.
🧹 Nitpick comments (4)
apps/csm-portal/backend/internal/handler/users_test.go (1)
93-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the leftover discard statement.
entityCallsis read by the assertion at line 116, so_ = entityCallsis not needed to compile. The statement conventionally marks a variable as deliberately unused, which contradicts that assertion and can mislead a reader into thinking the call count is not checked.♻️ Proposed fix
} - _ = entityCalls h := NewUsersHandler(&mockSCIMClient{}, entityClient, testDirectory(t))🤖 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 `@apps/csm-portal/backend/internal/handler/users_test.go` at line 93, Remove the unnecessary `_ = entityCalls` discard statement from the test, leaving the existing assertion that reads `entityCalls` unchanged.apps/csm-portal/backend/internal/handler/user_teams_test.go (1)
148-156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the cap message, and cover the
roleIdscap.This test builds keys that are also unknown to the registry. The unknown-team check and the cap check both return 400, so the test passes even if the cap is removed. Assert the message to pin the cause.
The
roleIDFilterLimitcap has no test. Both caps protect the same upstreamINclause described inuser_teams.golines 25-28.♻️ Proposed test additions
func TestSearchUsers_EnforcesFilterCaps(t *testing.T) { tooManyTeams := make([]string, teamIDFilterLimit+1) for i := range tooManyTeams { tooManyTeams[i] = fmt.Sprintf("team-%d", i) } encoded, _ := json.Marshal(tooManyTeams) _, w := capturedSearch(t, `{"filters":{"teamIds":`+string(encoded)+`}}`) assertStatus(t, w, http.StatusBadRequest) + assertErrorMessage(t, w, fmt.Sprintf("teamIds cannot contain more than %d values", teamIDFilterLimit)) + + tooManyRoles := make([]string, roleIDFilterLimit+1) + for i := range tooManyRoles { + tooManyRoles[i] = "agent" + } + encodedRoles, _ := json.Marshal(tooManyRoles) + _, w = capturedSearch(t, `{"filters":{"roleIds":`+string(encodedRoles)+`}}`) + assertStatus(t, w, http.StatusBadRequest) + assertErrorMessage(t, w, fmt.Sprintf("roleIds cannot contain more than %d values", roleIDFilterLimit)) }🤖 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 `@apps/csm-portal/backend/internal/handler/user_teams_test.go` around lines 148 - 156, Strengthen TestSearchUsers_EnforcesFilterCaps by asserting the response message specifically identifies the team ID filter-limit violation, using valid registered team IDs so the test cannot pass through unknown-team validation. Add equivalent coverage for roleIds exceeding roleIDFilterLimit and assert its cap-specific message.apps/csm-portal/backend/openapi.yaml (1)
6018-6026: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated team-family enum into a shared schema component.
UserTeam.family,Team.family, andUserTeamRef.familyrepeat the identicalenum: [cre-abt, cre, sre-abt, sre]and description verbatim in three places. Define oneTeamFamilyschema and reference it with$reffrom all three properties, so a future change to the family set or its description needs one edit instead of three.♻️ Proposed refactor: shared TeamFamily component
+ TeamFamily: + type: string + enum: [cre-abt, cre, sre-abt, sre] + description: > + Team family classification. The `-abt` variants are account-based teams; the + bare variants classify a member of the same discipline who is not on an + account-based team. Omitted when the team is not classified into a family: + not every team is. A team picker scoped to one discipline filters on this — + e.g. an SRE-scoped dashboard offers only `sre-abt` teams. + UserTeam: ... family: - type: string - enum: [cre-abt, cre, sre-abt, sre] - description: > - ... + allOf: + - $ref: '`#/components/schemas/TeamFamily`'Apply the same replacement to
Team.familyandUserTeamRef.family.Also applies to: 6130-6138, 6191-6199
🤖 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 `@apps/csm-portal/backend/openapi.yaml` around lines 6018 - 6026, Define a shared TeamFamily schema component containing the existing enum and description, then replace the inline family definitions in UserTeam.family, Team.family, and UserTeamRef.family with references to that component. Preserve the current values and documentation while ensuring all three properties use the same $ref.entity-service/openapi.yaml (1)
2951-2981: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd
groupstoGetUserMeResponse.required.The
requiredlist onGetUserMeResponseis[id, email, lastName, roles]. The corresponding Go fieldGroups []UserGroupRef json:"groups"has noomitemptyand is always emitted, even as an empty array on a best-effort lookup failure. Other non-nullable array response fields in this file (for exampleSearchSNUsersResponse.users) are listed inrequired. Listgroupsas required here too, so the schema matches the actual response guarantee.🔧 Proposed fix
GetUserMeResponse: type: object - required: [id, email, lastName, roles] + required: [id, email, lastName, roles, groups]🤖 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 `@entity-service/openapi.yaml` around lines 2951 - 2981, Update the GetUserMeResponse schema’s required list to include groups, preserving the existing required fields so the OpenAPI contract reflects that the response always emits groups, including an empty array.
🤖 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 `@apps/csm-portal/backend/internal/directory/directory.go`:
- Around line 140-145: Update sourceIDToUUID to normalize valid 32-character
hexadecimal IDs to lowercase before constructing the hyphenated UUID string,
using the strings package. Preserve unchanged return behavior for invalid or
non-32-character IDs.
In `@apps/csm-portal/backend/internal/directory/teams.go`:
- Around line 163-165: Validate fields[3] before assigning it to team.GroupID in
the team-row parser: accept either a compact 32-character hexadecimal ID or a
canonical UUID, and reject all other non-empty groupId values with the parser’s
existing error path. Reuse the existing sourceIDToUUID or UUID validation
utilities where appropriate, while preserving current handling when no groupId
is supplied.
---
Outside diff comments:
In `@apps/csm-portal/backend/openapi.yaml`:
- Around line 1021-1028: Update the Team.groupId schema to declare the UUID
format alongside its existing string type. Keep the current optionality and
other schema properties unchanged, so the OpenAPI contract matches the UUID
value produced by directory.New through sourceIDToUUID.
In `@entity-service/internal/service/case_filters.go`:
- Around line 592-607: The validation errors returned by
rejectUnsupportedOrGroupFields currently lose the OR-branch index. In
entity-service/internal/service/case_filters.go:592-607, pass i into
rejectUnsupportedOrGroupFields or wrap its errors so unsupported fields report
the anyOf[i].filters path. In
entity-service/internal/service/case_filters_test.go:394-457, add coverage for
unsupported fields in both the first and a later branch, asserting the indexed
error paths.
---
Nitpick comments:
In `@apps/csm-portal/backend/internal/handler/user_teams_test.go`:
- Around line 148-156: Strengthen TestSearchUsers_EnforcesFilterCaps by
asserting the response message specifically identifies the team ID filter-limit
violation, using valid registered team IDs so the test cannot pass through
unknown-team validation. Add equivalent coverage for roleIds exceeding
roleIDFilterLimit and assert its cap-specific message.
In `@apps/csm-portal/backend/internal/handler/users_test.go`:
- Line 93: Remove the unnecessary `_ = entityCalls` discard statement from the
test, leaving the existing assertion that reads `entityCalls` unchanged.
In `@apps/csm-portal/backend/openapi.yaml`:
- Around line 6018-6026: Define a shared TeamFamily schema component containing
the existing enum and description, then replace the inline family definitions in
UserTeam.family, Team.family, and UserTeamRef.family with references to that
component. Preserve the current values and documentation while ensuring all
three properties use the same $ref.
In `@entity-service/openapi.yaml`:
- Around line 2951-2981: Update the GetUserMeResponse schema’s required list to
include groups, preserving the existing required fields so the OpenAPI contract
reflects that the response always emits groups, including an empty array.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2148d6c3-a2eb-46ae-b794-28c17dbabfd4
📒 Files selected for processing (40)
apps/csm-portal/backend/cmd/server/main.goapps/csm-portal/backend/internal/dashboard/registry.goapps/csm-portal/backend/internal/dashboard/registry_test.goapps/csm-portal/backend/internal/dashboard/widgets.goapps/csm-portal/backend/internal/directory/directory.goapps/csm-portal/backend/internal/directory/directory_test.goapps/csm-portal/backend/internal/directory/roles.goapps/csm-portal/backend/internal/directory/search.goapps/csm-portal/backend/internal/directory/teams.goapps/csm-portal/backend/internal/entity/customer.goapps/csm-portal/backend/internal/handler/helpers_test.goapps/csm-portal/backend/internal/handler/reference.goapps/csm-portal/backend/internal/handler/reference_test.goapps/csm-portal/backend/internal/handler/user_teams.goapps/csm-portal/backend/internal/handler/user_teams_test.goapps/csm-portal/backend/internal/handler/users.goapps/csm-portal/backend/internal/handler/users_test.goapps/csm-portal/backend/openapi.yamlentity-service/cmd/api/main.goentity-service/internal/config/config.goentity-service/internal/domain/abt_team.goentity-service/internal/domain/abt_team_test.goentity-service/internal/domain/entity.goentity-service/internal/handler/reference_handler.goentity-service/internal/server/directory_routes_test.goentity-service/internal/server/routes.goentity-service/internal/service/case_filters.goentity-service/internal/service/case_filters_test.goentity-service/internal/service/catalog_pagination_test.goentity-service/internal/service/interfaces.goentity-service/internal/service/role_service.goentity-service/internal/service/sn_user_directory_test.goentity-service/internal/service/sn_user_service.goentity-service/internal/service/sn_user_service_test.goentity-service/internal/service/team_service.goentity-service/internal/service/team_service_test.goentity-service/internal/service/user_roles.goentity-service/internal/service/user_roles_test.goentity-service/internal/service/user_service.goentity-service/openapi.yaml
💤 Files with no reviewable changes (14)
- entity-service/internal/service/catalog_pagination_test.go
- entity-service/internal/domain/abt_team.go
- entity-service/internal/server/routes.go
- entity-service/internal/handler/reference_handler.go
- entity-service/internal/service/user_roles.go
- apps/csm-portal/backend/internal/entity/customer.go
- entity-service/internal/service/team_service_test.go
- entity-service/internal/service/team_service.go
- entity-service/internal/service/interfaces.go
- entity-service/internal/service/user_roles_test.go
- entity-service/internal/config/config.go
- entity-service/cmd/api/main.go
- entity-service/internal/service/role_service.go
- entity-service/internal/domain/abt_team_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/csm-portal/backend/internal/dashboard/widgets.go
…l template No entity-service Go code reads CSM_TEAM_REGISTRY or CSM_USER_ROLES any more, but its .env.example, README and CLAUDE.md still documented them, and the portal backend's .env.example -- where they are now required -- mentioned neither. An operator following the templates would configure the service that ignores them and leave the live one unset, which starts fine and then returns nothing from every team picker and team filter. Both blocks now live in apps/csm-portal/backend/.env.example with the portal's own semantics (the four-field row including groupId, startup resolution, duplicate rejection); the entity-service files carry a pointer saying the variables are not read there.
…gets, tighten the default rule
Three loader corrections, all the same shape: something wrong loaded
cleanly and only misbehaved later.
- w.legacyFilters was cleared unconditionally, so a definition carrying
BOTH "query" and the deprecated "filters" lost the legacy one in
silence. Worst when "query" is an empty {}: it still wins, and every
widget renders 0 with nothing in the logs. Now warns, mirroring the
sibling orGroups/anyOf drop, for widgets and pie slices alike.
- validate never looped d.Widgets, so a typo'd resourceType or shape, a
blank or duplicated widget id, and a gridWidth outside 1-12 all
loaded. Each is dead or broken in the browser only. Added a per-widget
pass, on the DASHBOARDS_CONFIG path too: unlike "type", none of these
fields is new.
- The default-dashboard rule allowed one isDefault per type, but nothing
selects on type yet (CsmDashboardPage picks on isDefault plus
isTeamBased, and type is not on the dashboard-list response), so a
second typed default would have been resolved by LoadDir's filename
ordering. Held at one default overall until the frontend is type-aware,
and the docs that asserted type-aware selection and family-scoped team
pickers now say those are follow-ups.
Also parses DASHBOARDS_HOT_RELOAD with strconv.ParseBool: the "true"
string compare read 1, yes and on as off and never reported a typo.
…lly shows the description mergeWidgetFilters reconciled only the inner `filters` array, so a widget with a base `anyOf` and a slice that also set one lost every base branch to the object spread -- silently WIDENING that slice's count instead of narrowing it. Not hypothetical: the backend loader now produces `anyOf` itself by migrating the legacy `orGroups` key. Merged branch-wise rather than documented as unsupported. ANDing two OR sets distributes, so (B1|B2) AND (S1|S2) becomes the four pairwise-merged branches, each pair reconciled by `field` on the same "slice wins" rule the flat array already used. Only applies when both sides set `anyOf`; a one-sided case was already correct, and an unrecognised shape still falls through to last-writer-wins rather than being mangled. The tooltip test was named for showing the description but only asserted the info button existed, so an empty tooltip passed. It now hovers and asserts the tooltip's text content, and holds widgetId constant across the negative rerender so only `description` varies. Verified it fails when the Tooltip title is blanked. The keyboard path is left unasserted: it opens on :focus-visible, which needs user-event, not a dependency here.
…ed ones Two CodeRabbit findings on the new directory package, both of the same shape: config that is wrong in a way nothing errors on. sourceIDToUUID kept the configured id's case, but isHex accepts uppercase digits while canonical UUID text is lowercase, and this value is compared against ids the entity service renders. An uppercase configured id therefore produced an uppercase groupId that matched nothing on the case-search integrationCsTeam filter -- an empty team-scoped result with no error anywhere. A supplied groupId was also stored unvalidated, while empty teamKey, empty displayName and unknown family are all rejected precisely because each degrades silently. sourceIDToUUID passes anything that is not exactly 32 hex characters through unchanged, so a 31-character typo yielded a malformed id that likewise matched nothing. An absent id stays legal: that team is still listed, it just cannot scope the filter. Both regression tests were confirmed to fail against the pre-fix code -- the uppercase id leaked through, and all three malformed-id cases were accepted -- while the absent-id case passes on both sides so the check is not over-rejecting.
Rashmika998
approved these changes
Aug 3, 2026
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
3 participants
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.
Purpose
Goals
type(cre|sre|cs) and widen the ABT teamfamilyto four variants (cre-abt,cre,sre-abt,sre) so the frontend can auto-select the right dashboard and team for a user.CSM_TEAM_REGISTRY/CSM_USER_ROLESinto the portal backend, resolving team key ↔ name ↔ id ↔ UUID once at startup so the catalogue endpoints stop calling the entity-service at all.filters.filters→query.filters, andorGroups→anyOfwith named branches.Approach
Dashboard directory (
DASHBOARDS_DIR,DASHBOARDS_HOT_RELOAD). Every*.jsonin the directory is one dashboard; the filename is irrelevant —id,displayNameandtypecome from the file's own content. Read once at startup, held in memory, no per-request disk I/O.DASHBOARDS_CONFIGstill works as a deprecated fallback with a warning.Fail-loud at startup, forgiving on hot-reload. Startup aborts naming the offending file on: unreadable, malformed, empty/duplicate
id, emptydisplayName, missing/unknowntype, and contradictory flags. That is right at boot but wrong for hot-reload — an editor saving a half-written JSON mid-keystroke is a common race, and killing a dev server for it is hostile. So in hot-reload mode a failed re-read logs at ERROR and keeps serving the last known-good set, recovering on its own when the file parses again. Startup still fails hard in both modes.Also newly fatal: a malformed
DASHBOARDS_CONFIG. It previously logged once and returned nil — silently emptying every dashboard in the product.Contradictory config is rejected, not normalised.
type,isDefaultandisTeamBasedcoexist by design, which means they can disagree. Rejected at startup:type: cre|srewithisTeamBased: false;type: cswithisTeamBased: true; twoisDefault: truesharing onetype(including two untyped defaults, which share the empty type key); unknown or missingtype.Registry and roles move to the portal backend, resolved at startup. The split that made this tractable: team key ↔ display name ↔ group id ↔ platform UUID is derivable from the registry rows alone — they already carry the group id, and the id→UUID conversion is a pure function — so it needs no service call at all and is built into an in-memory index at startup. Only membership (which users are in which group) is inherently live.
POST /teams/searchandPOST /roles/searchare now pure memory reads;/users/mestill costs exactly one entity call. The entity-service now holds no organisation vocabulary at all, and its dead registry plumbing is removed.Membership filtering is driven by group names, not group ids — deliberately: some registry rows legitimately carry no group id, and an id-based filter would silently under-filter exactly those rows.
Key renames.
filters.filtersstuttered, andorGroups' bare nested arrays hid their AND-within-a-branch semantics. Now:The ServiceNow wire format is deliberately unchanged — the SN adapter still emits
orGroups, guarded by a test asserting on the raw outgoing JSON (a typed decode would silently re-map a renamed key and pass). Renaming it would have been silently catastrophic: ServiceNow ignores unrecognised keys and returns an unfiltered count rather than erroring. No Ballerina or ServiceNow change is required by this PR.Backward compatibility. The loader accepts the legacy
filters/orGroupsshape and migrates in memory with a per-entry deprecation warning that names the source file (not a config variable the deployment may not even use). Verified against the real pre-migration config. Without it, an un-migrated deployment would render every widget as 0 with no error, since Go zero-values an unknown key.Behaviour changes worth calling out
familyvalue now fails startup, naming the row. The previous parser deliberately passed unknowns through lowercased — a typo would silently drop a team from every picker./users/searcherror messages are now specific (teamIds contains unknown team: …) instead of a masked"Failed to search users.". Status is still 400.GET /users/menow returns the caller'sgroupsinstead of a resolvedteam; the portal backend re-derivesteams[], so its own response shape is byte-identical. No other consumer reads that endpoint.User stories
Release note
Documentation
Automation tests
Security checks
Related PRs
Migrations (if applicable)
Test environment
Summary by CodeRabbit
anyOffilter branches.querycriteria and include dashboard type metadata.