feat: resolveproject group in the segments middleware (CM-189)#4011
feat: resolveproject group in the segments middleware (CM-189)#4011
Conversation
|
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR updates segment scoping so the frontend can send project group (non-leaf) segment IDs, and the backend middleware will resolve them into leaf sub-project segments for downstream queries.
Changes:
- Backend: enhance
segmentMiddlewareto expand non-leaf segment IDs into active leaf sub-project segments. - Frontend: stop expanding project groups into subproject segments in a few call sites and instead pass
[projectGroup.id]. - Frontend: adjust organization merge suggestions calls to use the selected project group ID.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/modules/organization/services/organization.api.service.ts | Uses selected project group ID(s) for merge suggestions / create payloads. |
| frontend/src/modules/organization/organization-service.js | Sends only selected project group ID for merge suggestions. |
| frontend/src/modules/lf/layout/components/lf-banners.vue | Integration listing now scoped by [projectGroup.id]. |
| frontend/src/modules/activity/components/activity-timeline.vue | Activity query now scoped by selected project group ID when no explicit segment is selected. |
| backend/src/middlewares/segmentMiddleware.ts | Resolves provided segment IDs to leaf sub-project segments before attaching req.currentSegments. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ulemons
force-pushed
the
feat/standardize-segments-api
branch
from
765829e to
9d33a06
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ulemons
force-pushed
the
feat/standardize-segments-api
branch
from
ee72d68 to
5faf875
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (querySegments.length > 0) { | ||
| segments = { | ||
| rows: await resolveToLeafSegments(segmentRepository, querySegments, req), | ||
| } | ||
| } else if (bodySegments.length > 0) { | ||
| segments = { | ||
| rows: await resolveToLeafSegments(segmentRepository, bodySegments, req), | ||
| } | ||
| } else { | ||
| segments = await segmentRepository.querySubprojects({ limit: 1, offset: 0 }) | ||
| } | ||
|
|
||
| req.currentSegments = segments.rows | ||
| const options = req as unknown as IRepositoryOptions | ||
| options.currentSegments = segments.rows |
There was a problem hiding this comment.
If getSegmentSubprojects() resolves to an empty list (e.g. a project group with no active subprojects, or an invalid segment ID), the middleware sets currentSegments to []. Downstream DAL calls (e.g. queryActivities) explicitly throw when segmentIds is empty, which would turn a client scoping issue into a 500. Consider failing fast with a 400 when the request provided segments but they resolve to none, or falling back to the original fetched segments to keep currentSegments non-empty.
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
ulemons
force-pushed
the
feat/standardize-segments-api
branch
from
9a013b6 to
e58e568
Compare
|
|
||
| req.currentSegments = segments.rows | ||
| const options = req as unknown as IRepositoryOptions | ||
| options.currentSegments = segments.rows |
There was a problem hiding this comment.
Endpoints reading req.body.segments get unresolved project group IDs
High Severity
The frontend auth-axios interceptor now always sends [projectGroupId] as the segments fallback for all request types. The new backend middleware resolves this to leaf sub-projects in options.currentSegments, but does not modify req.body.segments. Multiple endpoints read req.body.segments directly — e.g., linkedinConnect.ts, redditOnboard.ts, organizationMerge.ts, memberExport.ts, and organizationExport.ts — and will now receive a project-group ID where they expect a leaf sub-project ID. Previously, getSegmentsFromProjectGroup expanded to sub-project IDs on the frontend for non-GET requests, so these endpoints worked correctly.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit e58e568. Configure here.
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const leafRecords = await segmentRepository.getSegmentSubprojects(segmentIds) | ||
|
|
||
| log.debug( | ||
| { | ||
| api: `${req.method} ${req.path}`, | ||
| input_segments: nonLeaf.map((s) => ({ id: s.id, name: s.name, level: segmentLevel(s) })), | ||
| resolved_leaf_segments: leafRecords.map((s: any) => ({ id: s.id, name: (s as any).name })), | ||
| resolved_count: leafRecords.length, | ||
| }, | ||
| 'Non-leaf segments resolved to leaf sub-projects', | ||
| ) | ||
|
|
||
| return leafRecords.map(populateSegmentRelations) | ||
| } |
There was a problem hiding this comment.
resolveToLeafSegments can introduce duplicate leaf segments when the input contains overlapping IDs (e.g. a projectGroup ID plus one of its subproject IDs). getSegmentSubprojects doesn't DISTINCT/dedupe, so currentSegments can end up with repeated IDs, which can break code that expects uniqueness (and may trip single-segment assertions). Consider deduplicating leafRecords by id (or adding DISTINCT/GROUP BY in the query) before returning.
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f90e081. Configure here.
| TanstackKey.MEMBERS_LIST, | ||
| selectedProjectGroup.value?.id, | ||
| queryParams.value.search, | ||
| filters.value, // Use filters.value directly to make it reactive |
There was a problem hiding this comment.
Filters removed from query key break list filtering
High Severity
filters.value was removed from membersQueryKey (and the equivalent organizationsQueryKey), but the queryFn still reads filters.value to build the API call. When a user changes only a filter (without changing search, sort, pagination, or segments), none of the remaining query key fields change, so TanStack Query returns cached stale data instead of refetching. The previously existing watch on filters that reset pagination was also removed, compounding the issue.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit f90e081. Configure here.
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
frontend/src/modules/organization/pages/organization-list-page.vue:153
organizationsQueryKeyno longer includesfilters(or any derived filter state), butqueryFnstill depends onfilters.valueviabuildApiFilter(...). With @tanstack/vue-query, changes tofilterswon’t trigger a refetch if the query key is unchanged, so the list can become stale when users adjust filters. Include a stable serialization of the active filters (or the transformed filter/orderBy) in the query key, or explicitly invalidate/refetch on filter changes (e.g., via a watch) to keep results consistent.
// Create a computed query key for organizations
const organizationsQueryKey = computed(() => [
TanstackKey.ORGANIZATIONS_LIST,
selectedProjectGroup.value?.id,
queryParams.value.search,
queryParams.value.offset,
queryParams.value.limit,
queryParams.value.orderBy,
queryParams.value.segments,
]);
frontend/src/modules/member/pages/member-list-page.vue:156
membersQueryKeyno longer includesfilters/customAttributesFilter, but thequeryFnstill builds the API filter from these reactive values. As a result, changing filters can leave the cached query “fresh” and prevent refetching, showing stale data. Add the relevant filter state (or its stable serialized form) tomembersQueryKey, or invalidate/refetch the query when filters change.
// Create a computed query key for members
const membersQueryKey = computed(() => [
TanstackKey.MEMBERS_LIST,
selectedProjectGroup.value?.id,
queryParams.value.search,
queryParams.value.offset,
queryParams.value.limit,
queryParams.value.orderBy,
queryParams.value.segments,
]);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Safely extracts a string[] from an unknown query/body value. | ||
| * Rejects ParsedQs objects (e.g. ?segments[key]=val) that would cause type confusion. | ||
| */ | ||
| function toStringArray(value: unknown): string[] { | ||
| if (value === undefined || value === null) return [] | ||
| const items = Array.isArray(value) ? value : [value] | ||
| return items.filter((v): v is string => typeof v === 'string') | ||
| } |
There was a problem hiding this comment.
The JSDoc for toStringArray says it “Rejects ParsedQs objects”, but the implementation silently filters non-strings and returns [] (which is not an explicit rejection and can change behavior by falling back to the default segment). Either update the comment to reflect the actual behavior, or explicitly reject non-string values (e.g., by throwing a 400) when segments is present but contains non-string entries.
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Note
Medium Risk
Changes how
segmentsare derived and applied for most API requests, which can affect data scoping/permissions and query results; also tweaks frontend caching keys for list pages.Overview
Centralizes segment expansion on the backend: the global
segmentMiddlewarenow safely parsessegmentsfrom query/body and resolves any project group/project IDs into their leaf sub-projects (with debug logging), and a new route-levelsegmentByIdMiddlewareis added toGET/PUT /segment/:segmentIdto ensure permission checks use the target segment.Updates the frontend to stop pre-expanding segment trees (
getSegmentsFromProjectGroup) and instead send only the selected project-group id (or[]) across activities, integrations fetches, merge suggestions, and the globalauth-axiosinterceptor; data-quality merge suggestion components likewise scope requests to[projectGroup]only. Also hardens activity timeline timestamp initialization for invalidjoinedAt, and adjusts member/org list query cache keys by removingfilters-based invalidation.In the data-access layer, collection creation now treats
description/slugas nullable and explicitly normalizes optional fields tonullwhen inserting.Reviewed by Cursor Bugbot for commit 4263dbf. Bugbot is set up for automated code reviews on this repo. Configure here.