refactor(soup): rework group by to better support optimistic updates#3565
Open
dev-rb wants to merge 32 commits into
Open
refactor(soup): rework group by to better support optimistic updates#3565dev-rb wants to merge 32 commits into
dev-rb wants to merge 32 commits into
Conversation
…s to swagger for gen
…soup-normalized # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR refactors soup list pagination from per-group infinite queries to a unified grouped query model with per-group cursors. The grouped response now stores items in an id-keyed record with per-group 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/app/packages/app/component/next-soup/soup-view/soup-view-context.tsx (1)
458-520:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep grouped rows behind the same client-side filter pipeline.
This branch builds grouped rows from
itemsQuery.data.itemsByIddirectly, sosoup.predicates.test(...)inbaseEntities()stops applying as soon as grouping is enabled. That makes grouped views disagree with ungrouped views under active chips / assignee predicates. BuildgroupEntitiesfrom the already-filtered entity set instead of the raw grouped cache.🤖 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 `@js/app/packages/app/component/next-soup/soup-view/soup-view-context.tsx` around lines 458 - 520, The grouped-rows loop currently pulls items from itemsQuery.data.itemsById and thus bypasses client-side filters; change the groupEntities construction to source entities from the already-filtered entity set (entities() / baseEntities()) instead of the raw itemsById. Concretely, inside the createMemo rows function where you iterate apiGroup.itemIds and build groupEntities, look up each id in the filtered entities map (use entities() or the same filtered collection baseEntities() relies on), skip missing entries, and preserve ordering so soup.buildRow calls (header, each entity, loadmore) use those filtered originals; keep buildGroupMeta, apiGroup.itemIds, soup.buildRow, and globalIndex logic unchanged.
🧹 Nitpick comments (1)
js/app/packages/app/constants/list-views.ts (1)
52-76: ⚡ Quick winUse
matchfrom ts-pattern for exhaustive switch.Per coding guidelines, exhaustive switch statements should use
matchfromts-patterninstead of native switch.♻️ Suggested refactor
+import { match } from 'ts-pattern'; + -export const soupItemMatchesListView = ( - item: SoupApiItem, - view: ListView | undefined -): boolean => { - switch (view) { - case 'agents': - return item.tag === 'chat'; - case 'mail': - return item.tag === 'emailThread'; - case 'documents': - return item.tag === 'document' && item.data.subType?.type !== 'task'; - case 'tasks': - return item.tag === 'document' && item.data.subType?.type === 'task'; - case 'channels': - return item.tag === 'channel'; - case 'calls': - return item.tag === 'call'; - case 'folders': - return item.tag === 'project'; - case 'inbox': - case 'search': - case undefined: - return true; - } -}; +export const soupItemMatchesListView = ( + item: SoupApiItem, + view: ListView | undefined +): boolean => + match(view) + .with('agents', () => item.tag === 'chat') + .with('mail', () => item.tag === 'emailThread') + .with('documents', () => item.tag === 'document' && item.data.subType?.type !== 'task') + .with('tasks', () => item.tag === 'document' && item.data.subType?.type === 'task') + .with('channels', () => item.tag === 'channel') + .with('calls', () => item.tag === 'call') + .with('folders', () => item.tag === 'project') + .with('inbox', 'search', undefined, () => true) + .exhaustive();As per coding guidelines: "For exhaustive switch statements in TypeScript, use
matchfromts-patterninstead of native switch".🤖 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 `@js/app/packages/app/constants/list-views.ts` around lines 52 - 76, Replace the native switch in soupItemMatchesListView with ts-pattern's match to enforce exhaustiveness: import match from 'ts-pattern', call match(view) and provide explicit .with clauses for each ListView variant ('agents', 'mail', 'documents', 'tasks', 'channels', 'calls', 'folders', 'inbox', 'search', and undefined) returning the same boolean expressions currently used (e.g., for 'agents' return item.tag === 'chat', for 'documents' return item.tag === 'document' && item.data.subType?.type !== 'task', etc.), and finish with .exhaustive() so the function returns a boolean and the compiler enforces handling all cases; ensure the undefined case is represented (e.g., .with(undefined, () => true)) to preserve existing behavior.
🤖 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 `@js/app/packages/queries/soup/grouped/api.ts`:
- Around line 97-105: computeGroupKeysForItem currently returns concrete keys
for non-empty 'project' and EntityReference groups even when
resolveGroupMetaForKey cannot synthesize label/displayOrder, causing items to be
optimistically placed without metadata; change computeGroupKeysForItem so that
for 'project' groups (the NOT_SET_GROUP_KEY exception aside) and for
EntityReference-like groups you return undefined (i.e., do not emit a group key)
whenever the code cannot also derive the corresponding metadata
client-side—update the logic in computeGroupKeysForItem and any mirrored
branches (the similar blocks around the other ranges noted) to invalidate
(return undefined) instead of returning a key when resolveGroupMetaForKey would
be undefined.
- Around line 84-126: The switch over groupBy.type should be replaced with
ts-pattern's match to follow project convention and ensure exhaustiveness; in
resolveGroupMetaForKey (and similarly in computeGroupKeysForItem) import match
from "ts-pattern", keep the early return when groupBy is undefined, then replace
the switch with match(groupBy.type).with('entity_type', () => { /* copy existing
ENTITY_TYPE logic using ENTITY_TYPE_META and return same ResolvedGroupMeta */
}).with('project', () => { /* copy project branch including NOT_SET_GROUP_KEY
handling */ }).with('property', () => { /* copy property branch, call
getGroupedPropertyValueType(item, groupBy) and preserve SelectOption handling */
}).with('date', () => { return; }).exhaustive(); ensure returned shapes and
null/displayOrder semantics are unchanged and references to ENTITY_TYPE_META,
NOT_SET_GROUP_KEY, getGroupedPropertyValueType, and function names remain the
same.
In `@js/app/packages/queries/soup/normalized-cache/operations.ts`:
- Around line 108-149: When computeGroupKeysForItem(...) or
reconcileGroupedMembership(...) returns undefined we currently just continue and
leave the grouped cache stale; instead mark the query for invalidation so
clients refresh. In the loop over keys (after extracting groupBy with
extractGroupByFromKey and computing nextGroupKeys via computeGroupKeysForItem
and reconciling via reconcileGroupedMembership), if nextGroupKeys === undefined
or reconcileGroupedMembership returns undefined (sets needsInvalidation), call
queryClient.invalidateQueries(key) (or set a tombstone via
queryClient.setQueryData(key, prev => ({ ...prev, invalidated: true })) if you
prefer) and then continue; preserve the existing changed logic but ensure any
undefined path triggers the invalidate call using the key variable so grouped
pages are refreshed rather than left stale.
- Around line 396-404: When insertIntoGroupedPage(firstPage, item,
extractGroupByFromKey(key)) returns falsy (nextPage == null) you must invalidate
the affected query instead of silently continuing; update the code path that
checks nextPage to call the cache invalidation routine for that query/key (e.g.,
invoke the existing invalidateQuery/invalidateQueries or markQueryStale helper
with the same key) so the client refetches the grouped results when group
membership cannot be determined client-side; keep the original variables
(firstPage, item, key) and the insertIntoGroupedPage/extractGroupByFromKey calls
to locate the spot to add the invalidation.
In `@js/app/packages/service-clients/service-storage/openapi.json`:
- Around line 10576-10604: The GroupedSoupPage schema currently allows "groups"
to be null and doesn't require it, weakening the contract for the
/items/soup/ast/grouped response; update the GroupedSoupPage definition so
"groups" is non-nullable (type "array" of ApiGroupMeta) and add "groups" to the
schema's required array (alongside "items") so clients always receive group
metadata for grouped responses (update the "groups" property and the "required"
list referencing GroupedSoupPage and ApiGroupMeta).
---
Outside diff comments:
In `@js/app/packages/app/component/next-soup/soup-view/soup-view-context.tsx`:
- Around line 458-520: The grouped-rows loop currently pulls items from
itemsQuery.data.itemsById and thus bypasses client-side filters; change the
groupEntities construction to source entities from the already-filtered entity
set (entities() / baseEntities()) instead of the raw itemsById. Concretely,
inside the createMemo rows function where you iterate apiGroup.itemIds and build
groupEntities, look up each id in the filtered entities map (use entities() or
the same filtered collection baseEntities() relies on), skip missing entries,
and preserve ordering so soup.buildRow calls (header, each entity, loadmore) use
those filtered originals; keep buildGroupMeta, apiGroup.itemIds, soup.buildRow,
and globalIndex logic unchanged.
---
Nitpick comments:
In `@js/app/packages/app/constants/list-views.ts`:
- Around line 52-76: Replace the native switch in soupItemMatchesListView with
ts-pattern's match to enforce exhaustiveness: import match from 'ts-pattern',
call match(view) and provide explicit .with clauses for each ListView variant
('agents', 'mail', 'documents', 'tasks', 'channels', 'calls', 'folders',
'inbox', 'search', and undefined) returning the same boolean expressions
currently used (e.g., for 'agents' return item.tag === 'chat', for 'documents'
return item.tag === 'document' && item.data.subType?.type !== 'task', etc.), and
finish with .exhaustive() so the function returns a boolean and the compiler
enforces handling all cases; ensure the undefined case is represented (e.g.,
.with(undefined, () => true)) to preserve existing behavior.
🪄 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
Run ID: 7c36b84a-df81-474d-b73e-59fa5dbb20a9
⛔ Files ignored due to path filters (21)
js/app/packages/service-clients/service-storage/generated/schemas/apiGroupByField.tsis excluded by!**/generated/**js/app/packages/service-clients/service-storage/generated/schemas/apiGroupByFieldOneOf.tsis excluded by!**/generated/**js/app/packages/service-clients/service-storage/generated/schemas/apiGroupByFieldOneOfProperty.tsis excluded by!**/generated/**js/app/packages/service-clients/service-storage/generated/schemas/apiGroupByFieldOneOfPropertyEntityType.tsis excluded by!**/generated/**js/app/packages/service-clients/service-storage/generated/schemas/apiGroupMeta.tsis excluded by!**/generated/**js/app/packages/service-clients/service-storage/generated/schemas/apiGroupMetaDisplayOrder.tsis excluded by!**/generated/**js/app/packages/service-clients/service-storage/generated/schemas/apiGroupMetaNextCursor.tsis excluded by!**/generated/**js/app/packages/service-clients/service-storage/generated/schemas/apiPropertyEntityType.tsis excluded by!**/generated/**js/app/packages/service-clients/service-storage/generated/schemas/groupedParams.tsis excluded by!**/generated/**js/app/packages/service-clients/service-storage/generated/schemas/groupedParamsGroupKey.tsis excluded by!**/generated/**js/app/packages/service-clients/service-storage/generated/schemas/groupedParamsLimit.tsis excluded by!**/generated/**js/app/packages/service-clients/service-storage/generated/schemas/groupedParamsSortMethod.tsis excluded by!**/generated/**js/app/packages/service-clients/service-storage/generated/schemas/groupedSoupPage.tsis excluded by!**/generated/**js/app/packages/service-clients/service-storage/generated/schemas/groupedSoupPageGroups.tsis excluded by!**/generated/**js/app/packages/service-clients/service-storage/generated/schemas/groupedSoupPageItems.tsis excluded by!**/generated/**js/app/packages/service-clients/service-storage/generated/schemas/groupedSoupPageNextCursor.tsis excluded by!**/generated/**js/app/packages/service-clients/service-storage/generated/schemas/groupedSoupSort.tsis excluded by!**/generated/**js/app/packages/service-clients/service-storage/generated/schemas/index.tsis excluded by!**/generated/**js/app/packages/service-clients/service-storage/generated/schemas/postGroupedSoupAstRequest.tsis excluded by!**/generated/**js/app/packages/service-clients/service-storage/generated/schemas/postItemsSoupAstGroupedParams.tsis excluded by!**/generated/**js/app/packages/service-clients/service-storage/generated/zod.tsis excluded by!**/generated/**
📒 Files selected for processing (21)
js/app/packages/app/component/next-soup/create-soup-state.tsjs/app/packages/app/component/next-soup/filters/query-filters.tsjs/app/packages/app/component/next-soup/soup-view/create-infinite-queries.tsjs/app/packages/app/component/next-soup/soup-view/soup-view-context.tsxjs/app/packages/app/component/next-soup/soup-view/soup-view.tsxjs/app/packages/app/component/next-soup/soup-view/use-soup-view-hotkeys.tsjs/app/packages/app/constants/list-views.tsjs/app/packages/queries/soup/grouped/api.tsjs/app/packages/queries/soup/grouped/types.tsjs/app/packages/queries/soup/grouped/use-fetch-group-page.tsjs/app/packages/queries/soup/items.tsjs/app/packages/queries/soup/keys.tsjs/app/packages/queries/soup/normalized-cache/normalizer.tsjs/app/packages/queries/soup/normalized-cache/operations.test.tsjs/app/packages/queries/soup/normalized-cache/operations.tsjs/app/packages/queries/soup/transform-utils.tsjs/app/packages/service-clients/service-storage/client.tsjs/app/packages/service-clients/service-storage/openapi.jsonrust/cloud-storage/document_storage_service/src/api/swagger.rsrust/cloud-storage/soup/src/domain/models/grouping.rsrust/cloud-storage/soup/src/inbound/axum_router.rs
💤 Files with no reviewable changes (2)
- js/app/packages/app/component/next-soup/filters/query-filters.ts
- js/app/packages/app/component/next-soup/soup-view/create-infinite-queries.ts
Comment on lines
+84
to
+126
| export function resolveGroupMetaForKey( | ||
| groupBy: GroupByField | undefined, | ||
| key: string, | ||
| item?: SoupApiItem | ||
| ): ResolvedGroupMeta | undefined { | ||
| if (!groupBy) return; | ||
|
|
||
| switch (groupBy.type) { | ||
| case 'entity_type': { | ||
| const meta = ENTITY_TYPE_META[key] ?? { label: 'Other', displayOrder: 6 }; | ||
| return { key, label: meta.label, displayOrder: meta.displayOrder }; | ||
| } | ||
|
|
||
| case 'project': | ||
| if (key === NOT_SET_GROUP_KEY) { | ||
| return { | ||
| key, | ||
| label: 'No Project', | ||
| displayOrder: Number.MAX_SAFE_INTEGER, | ||
| }; | ||
| } | ||
| return; | ||
|
|
||
| case 'property': { | ||
| if (key === NOT_SET_GROUP_KEY) { | ||
| return { key, label: 'Not Set', displayOrder: Number.MAX_SAFE_INTEGER }; | ||
| } | ||
|
|
||
| const propertyValueType = getGroupedPropertyValueType(item, groupBy); | ||
| if (propertyValueType === 'SelectOption') { | ||
| return { | ||
| key, | ||
| label: key, | ||
| displayOrder: null, | ||
| }; | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| case 'date': | ||
| return; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the full function and surrounding context
sed -n '1,220p' js/app/packages/queries/soup/grouped/api.ts | nl -ba | sed -n '60,230p'
# Check imports to see whether ts-pattern is already used
sed -n '1,120p' js/app/packages/queries/soup/grouped/api.ts | nl -ba | sed -n '1,120p'
# Search for ts-pattern usage within the file and nearby packages
rg -n "ts-pattern|match\(" js/app/packages/queries/soup/grouped/api.ts js/app/packages/queries/soup/grouped -S || true
# Search repo-wide for match usage in TS to confirm convention
rg -n "from ['\"]ts-pattern['\"]|match\(" js/app/packages/queries -S | head -n 50 || trueRepository: macro-inc/macro
Length of output: 102
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="js/app/packages/queries/soup/grouped/api.ts"
# Print the relevant section with line numbers via awk (no nl dependency)
awk 'NR>=60 && NR<=230 {printf "%5d:%s\n", NR, $0}' "$FILE"
# Show top of file to inspect imports
awk 'NR>=1 && NR<=140 {printf "%5d:%s\n", NR, $0}' "$FILE"
# Search for ts-pattern usage in this file and directory
rg -n "ts-pattern|match\\(" "$FILE" js/app/packages/queries/soup/grouped -S || true
# Search wider in the repo for ts-pattern + match usage patterns
rg -n "from ['\"]ts-pattern['\"]|\\bmatch\\(" js/app -S --glob '*.ts' --glob '*.tsx' | head -n 50 || trueRepository: macro-inc/macro
Length of output: 14157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate GroupByField definition within the grouped package
rg -n "export type GroupByField|type GroupByField" js/app/packages/queries/soup/grouped -S
# Also show types file contents around GroupByField if found
TYPES_FILE="$(rg -l "GroupByField" js/app/packages/queries/soup/grouped/types.* 2>/dev/null || true)"
echo "TYPES_FILE_CANDIDATES: $TYPES_FILE"
# Fallback: print likely types file(s) named types.ts/tsx
for f in $(fd -a "types.ts" js/app/packages/queries/soup/grouped 2>/dev/null || true); do
echo "---- $f ----"
awk 'NR>=1 && NR<=260 {print NR ":" $0}' "$f" | rg -n "GroupByField|groupBy\.type" || true
done
# Directly dump the types file(s) that mention GroupByField (first few lines around match)
for f in $(rg -l "GroupByField" js/app/packages/queries/soup/grouped -S); do
echo "---- Inspecting: $f ----"
# show only region with GroupByField
awk 'NR>=1 && NR<=260 {print NR ":" $0}' "$f" | sed -n '1,260p' | head -n 260
doneRepository: macro-inc/macro
Length of output: 12710
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find exact files & line numbers containing GroupByField type
mapfile -t hits < <(rg -n "export type GroupByField|type GroupByField" js/app/packages/queries/soup/grouped -S)
echo "HITS_COUNT=${`#hits`[@]}"
printf "%s\n" "${hits[@]}" | head -n 20
# For each hit, print ~80 lines around it
for h in "${hits[@]}"; do
file="${h%%:*}"
line="${h#*:}"
line="${line%%:*}"
start=$((line-40))
end=$((line+80))
echo "---- $file:$line ----"
awk -v s=$start -v e=$end 'NR>=s && NR<=e {print NR ":" $0}' "$file" | head -n 160
done
# Also check any OTHER groupBy.type switches that might indicate non-exhaustiveness
rg -n "switch \\(groupBy\\.type\\)" js/app/packages/queries/soup/grouped/api.ts -n || trueRepository: macro-inc/macro
Length of output: 116
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find exact files & line numbers containing GroupByField type
mapfile -t hits < <(rg -n "export type GroupByField|type GroupByField" js/app/packages/queries/soup/grouped -S)
echo "HITS_COUNT=${`#hits`[@]}"
printf "%s\n" "${hits[@]}" | head -n 20
# For each hit, print ~80 lines around it
for h in "${hits[@]}"; do
file="${h%%:*}"
line="${h#*:}"
line="${line%%:*}"
start=$((line-40))
end=$((line+80))
echo "---- $file:$line ----"
awk -v s=$start -v e=$end 'NR>=s && NR<=e {print NR ":" $0}' "$file" | head -n 160
done
# Also check any OTHER groupBy.type switches that might indicate non-exhaustiveness
rg -n "switch \\(groupBy\\.type\\)" js/app/packages/queries/soup/grouped/api.ts -n || trueRepository: macro-inc/macro
Length of output: 116
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find exact files & line numbers containing GroupByField type
mapfile -t hits < <(rg -n "export type GroupByField|type GroupByField" js/app/packages/queries/soup/grouped -S)
echo "HITS_COUNT=${`#hits`[@]}"
printf "%s\n" "${hits[@]}" | head -n 20
# For each hit, print ~80 lines around it
for h in "${hits[@]}"; do
file="${h%%:*}"
line="${h#*:}"
line="${line%%:*}"
start=$((line-40))
end=$((line+80))
echo "---- $file:$line ----"
awk -v s=$start -v e=$end 'NR>=s && NR<=e {print NR ":" $0}' "$file" | head -n 160
done
# Also check for other groupBy.type switches in this file
rg -n "switch \\(groupBy\\.type\\)" js/app/packages/queries/soup/grouped/api.ts -n || trueRepository: macro-inc/macro
Length of output: 116
Use match from ts-pattern instead of native switch for exhaustive groupBy.type branching.
GroupByField['type'] is only 'date' | 'entity_type' | 'project' | 'property', so the switch (groupBy.type) in resolveGroupMetaForKey (84-126) and computeGroupKeysForItem (152-209) should follow the repo convention and use match.
🤖 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 `@js/app/packages/queries/soup/grouped/api.ts` around lines 84 - 126, The
switch over groupBy.type should be replaced with ts-pattern's match to follow
project convention and ensure exhaustiveness; in resolveGroupMetaForKey (and
similarly in computeGroupKeysForItem) import match from "ts-pattern", keep the
early return when groupBy is undefined, then replace the switch with
match(groupBy.type).with('entity_type', () => { /* copy existing ENTITY_TYPE
logic using ENTITY_TYPE_META and return same ResolvedGroupMeta */
}).with('project', () => { /* copy project branch including NOT_SET_GROUP_KEY
handling */ }).with('property', () => { /* copy property branch, call
getGroupedPropertyValueType(item, groupBy) and preserve SelectOption handling */
}).with('date', () => { return; }).exhaustive(); ensure returned shapes and
null/displayOrder semantics are unchanged and references to ENTITY_TYPE_META,
NOT_SET_GROUP_KEY, getGroupedPropertyValueType, and function names remain the
same.
Comment on lines
+97
to
+105
| case 'project': | ||
| if (key === NOT_SET_GROUP_KEY) { | ||
| return { | ||
| key, | ||
| label: 'No Project', | ||
| displayOrder: Number.MAX_SAFE_INTEGER, | ||
| }; | ||
| } | ||
| return; |
Contributor
There was a problem hiding this comment.
Invalidate when you can't synthesize the destination group.
computeGroupKeysForItem() returns concrete keys for non-empty project groups and EntityReference groups, but resolveGroupMetaForKey() still returns undefined for those same keys. That leaves optimistic reconciliation with a group key but no label / displayOrder when an item moves into a group that is not already cached.
A safer fallback is to return undefined from computeGroupKeysForItem() for those cases until this file can also derive the corresponding metadata client-side.
Suggested safe fallback
case 'project': {
if (item.tag === 'channel' || item.tag === 'call') return;
const projectId = (item.data as { projectId?: string | null }).projectId;
- return [projectId ?? NOT_SET_GROUP_KEY];
+ return projectId == null ? [NOT_SET_GROUP_KEY] : undefined;
}
@@
if (value.type === 'EntityReference' && Array.isArray(value.value)) {
- return value.value.length > 0
- ? (value.value as Array<{ entity_id: string }>).map(
- (r) => r.entity_id
- )
- : [NOT_SET_GROUP_KEY];
+ return value.value.length === 0 ? [NOT_SET_GROUP_KEY] : undefined;
}Also applies to: 107-120, 162-165, 196-203
🤖 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 `@js/app/packages/queries/soup/grouped/api.ts` around lines 97 - 105,
computeGroupKeysForItem currently returns concrete keys for non-empty 'project'
and EntityReference groups even when resolveGroupMetaForKey cannot synthesize
label/displayOrder, causing items to be optimistically placed without metadata;
change computeGroupKeysForItem so that for 'project' groups (the
NOT_SET_GROUP_KEY exception aside) and for EntityReference-like groups you
return undefined (i.e., do not emit a group key) whenever the code cannot also
derive the corresponding metadata client-side—update the logic in
computeGroupKeysForItem and any mirrored branches (the similar blocks around the
other ranges noted) to invalidate (return undefined) instead of returning a key
when resolveGroupMetaForKey would be undefined.
Comment on lines
+108
to
+149
| const groupBy = extractGroupByFromKey(key); | ||
|
|
||
| if (!groupBy) continue; | ||
|
|
||
| const nextGroupKeys = computeGroupKeysForItem(entity, groupBy); | ||
|
|
||
| if (nextGroupKeys === undefined) { | ||
| continue; | ||
| } | ||
|
|
||
| let changed = false; | ||
| let needsInvalidation = false; | ||
|
|
||
| const pages = prev.pages.map((page) => { | ||
| if (!isGroupedPage(page)) return page; | ||
|
|
||
| const next = reconcileGroupedMembership( | ||
| page, | ||
| entityId, | ||
| nextGroupKeys, | ||
| entity, | ||
| groupBy | ||
| ); | ||
|
|
||
| if (!next) { | ||
| needsInvalidation = true; | ||
| return page; | ||
| } | ||
|
|
||
| if (next !== page) changed = true; | ||
|
|
||
| return next; | ||
| }); | ||
|
|
||
| if (needsInvalidation) { | ||
| continue; | ||
| } | ||
|
|
||
| if (!changed) continue; | ||
|
|
||
| queryClient.setQueryData(key, { ...prev, pages }); | ||
| } |
Contributor
There was a problem hiding this comment.
Invalidate grouped queries when optimistic regrouping can't be derived.
If computeGroupKeysForItem(...) or reconcileGroupedMembership(...) returns undefined, this loop just skips that cache. The entity has already been merged into Normy, so date/unresolved groupings can keep stale groups[].itemIds until a manual refresh.
Suggested fallback
const nextGroupKeys = computeGroupKeysForItem(entity, groupBy);
if (nextGroupKeys === undefined) {
+ queryClient.invalidateQueries({ queryKey: key });
continue;
}
@@
if (needsInvalidation) {
+ queryClient.invalidateQueries({ queryKey: key });
continue;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const groupBy = extractGroupByFromKey(key); | |
| if (!groupBy) continue; | |
| const nextGroupKeys = computeGroupKeysForItem(entity, groupBy); | |
| if (nextGroupKeys === undefined) { | |
| continue; | |
| } | |
| let changed = false; | |
| let needsInvalidation = false; | |
| const pages = prev.pages.map((page) => { | |
| if (!isGroupedPage(page)) return page; | |
| const next = reconcileGroupedMembership( | |
| page, | |
| entityId, | |
| nextGroupKeys, | |
| entity, | |
| groupBy | |
| ); | |
| if (!next) { | |
| needsInvalidation = true; | |
| return page; | |
| } | |
| if (next !== page) changed = true; | |
| return next; | |
| }); | |
| if (needsInvalidation) { | |
| continue; | |
| } | |
| if (!changed) continue; | |
| queryClient.setQueryData(key, { ...prev, pages }); | |
| } | |
| const groupBy = extractGroupByFromKey(key); | |
| if (!groupBy) continue; | |
| const nextGroupKeys = computeGroupKeysForItem(entity, groupBy); | |
| if (nextGroupKeys === undefined) { | |
| queryClient.invalidateQueries({ queryKey: key }); | |
| continue; | |
| } | |
| let changed = false; | |
| let needsInvalidation = false; | |
| const pages = prev.pages.map((page) => { | |
| if (!isGroupedPage(page)) return page; | |
| const next = reconcileGroupedMembership( | |
| page, | |
| entityId, | |
| nextGroupKeys, | |
| entity, | |
| groupBy | |
| ); | |
| if (!next) { | |
| needsInvalidation = true; | |
| return page; | |
| } | |
| if (next !== page) changed = true; | |
| return next; | |
| }); | |
| if (needsInvalidation) { | |
| queryClient.invalidateQueries({ queryKey: key }); | |
| continue; | |
| } | |
| if (!changed) continue; | |
| queryClient.setQueryData(key, { ...prev, pages }); | |
| } |
🤖 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 `@js/app/packages/queries/soup/normalized-cache/operations.ts` around lines 108
- 149, When computeGroupKeysForItem(...) or reconcileGroupedMembership(...)
returns undefined we currently just continue and leave the grouped cache stale;
instead mark the query for invalidation so clients refresh. In the loop over
keys (after extracting groupBy with extractGroupByFromKey and computing
nextGroupKeys via computeGroupKeysForItem and reconciling via
reconcileGroupedMembership), if nextGroupKeys === undefined or
reconcileGroupedMembership returns undefined (sets needsInvalidation), call
queryClient.invalidateQueries(key) (or set a tombstone via
queryClient.setQueryData(key, prev => ({ ...prev, invalidated: true })) if you
prefer) and then continue; preserve the existing changed logic but ensure any
undefined path triggers the invalidate call using the key variable so grouped
pages are refreshed rather than left stale.
Comment on lines
+396
to
+404
| const nextPage = insertIntoGroupedPage( | ||
| firstPage, | ||
| item, | ||
| extractGroupByFromKey(key) | ||
| ); | ||
|
|
||
| if (!nextPage) { | ||
| continue; | ||
| } |
Contributor
There was a problem hiding this comment.
Fallback grouped inserts should invalidate instead of no-op.
When insertIntoGroupedPage(...) cannot place the new item into a grouped cache, this path silently leaves that query untouched. Under groupings whose keys/labels can't be derived client-side, optimistic inserts will simply not appear.
Suggested fallback
const nextPage = insertIntoGroupedPage(
firstPage,
item,
extractGroupByFromKey(key)
);
if (!nextPage) {
+ queryClient.invalidateQueries({ queryKey: key });
continue;
}🤖 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 `@js/app/packages/queries/soup/normalized-cache/operations.ts` around lines 396
- 404, When insertIntoGroupedPage(firstPage, item, extractGroupByFromKey(key))
returns falsy (nextPage == null) you must invalidate the affected query instead
of silently continuing; update the code path that checks nextPage to call the
cache invalidation routine for that query/key (e.g., invoke the existing
invalidateQuery/invalidateQueries or markQueryStale helper with the same key) so
the client refetches the grouped results when group membership cannot be
determined client-side; keep the original variables (firstPage, item, key) and
the insertIntoGroupedPage/extractGroupByFromKey calls to locate the spot to add
the invalidation.
Comment on lines
+10576
to
+10604
| "GroupedSoupPage": { | ||
| "type": "object", | ||
| "description": "Response for grouped soup queries.\n\nItems are returned as a pool keyed by id; each `ApiGroupMeta.item_ids`\ndescribes the ordered membership of one group.", | ||
| "required": ["items"], | ||
| "properties": { | ||
| "groups": { | ||
| "type": ["array", "null"], | ||
| "items": { | ||
| "$ref": "#/components/schemas/ApiGroupMeta" | ||
| }, | ||
| "description": "Group metadata - present when group_by is specified in the request" | ||
| }, | ||
| "items": { | ||
| "type": "object", | ||
| "description": "Items in this page, keyed by id. Ordering lives in `groups[].item_ids`.", | ||
| "additionalProperties": { | ||
| "$ref": "#/components/schemas/SoupApiItem" | ||
| }, | ||
| "propertyNames": { | ||
| "type": "string", | ||
| "format": "uuid" | ||
| } | ||
| }, | ||
| "next_cursor": { | ||
| "type": ["string", "null"], | ||
| "description": "Cursor to load the next page (global pagination)" | ||
| } | ||
| } | ||
| }, |
Contributor
There was a problem hiding this comment.
Make groups required for the grouped response.
/items/soup/ast/grouped always requires group_by, so allowing GroupedSoupPage.groups to be omitted or null weakens the generated client contract for the main payload this endpoint returns. Downstream code will have to treat a successful grouped response as if it might not contain group metadata at all.
Suggested contract fix
"GroupedSoupPage": {
"type": "object",
"description": "Response for grouped soup queries.\n\nItems are returned as a pool keyed by id; each `ApiGroupMeta.item_ids`\ndescribes the ordered membership of one group.",
- "required": ["items"],
+ "required": ["groups", "items"],
"properties": {
"groups": {
- "type": ["array", "null"],
+ "type": "array",
"items": {
"$ref": "`#/components/schemas/ApiGroupMeta`"
},
- "description": "Group metadata - present when group_by is specified in the request"
+ "description": "Ordered group metadata for this grouped response."
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "GroupedSoupPage": { | |
| "type": "object", | |
| "description": "Response for grouped soup queries.\n\nItems are returned as a pool keyed by id; each `ApiGroupMeta.item_ids`\ndescribes the ordered membership of one group.", | |
| "required": ["items"], | |
| "properties": { | |
| "groups": { | |
| "type": ["array", "null"], | |
| "items": { | |
| "$ref": "#/components/schemas/ApiGroupMeta" | |
| }, | |
| "description": "Group metadata - present when group_by is specified in the request" | |
| }, | |
| "items": { | |
| "type": "object", | |
| "description": "Items in this page, keyed by id. Ordering lives in `groups[].item_ids`.", | |
| "additionalProperties": { | |
| "$ref": "#/components/schemas/SoupApiItem" | |
| }, | |
| "propertyNames": { | |
| "type": "string", | |
| "format": "uuid" | |
| } | |
| }, | |
| "next_cursor": { | |
| "type": ["string", "null"], | |
| "description": "Cursor to load the next page (global pagination)" | |
| } | |
| } | |
| }, | |
| "GroupedSoupPage": { | |
| "type": "object", | |
| "description": "Response for grouped soup queries.\n\nItems are returned as a pool keyed by id; each `ApiGroupMeta.item_ids`\ndescribes the ordered membership of one group.", | |
| "required": ["groups", "items"], | |
| "properties": { | |
| "groups": { | |
| "type": "array", | |
| "items": { | |
| "$ref": "`#/components/schemas/ApiGroupMeta`" | |
| }, | |
| "description": "Ordered group metadata for this grouped response." | |
| }, | |
| "items": { | |
| "type": "object", | |
| "description": "Items in this page, keyed by id. Ordering lives in `groups[].item_ids`.", | |
| "additionalProperties": { | |
| "$ref": "`#/components/schemas/SoupApiItem`" | |
| }, | |
| "propertyNames": { | |
| "type": "string", | |
| "format": "uuid" | |
| } | |
| }, | |
| "next_cursor": { | |
| "type": ["string", "null"], | |
| "description": "Cursor to load the next page (global pagination)" | |
| } | |
| } | |
| }, |
🤖 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 `@js/app/packages/service-clients/service-storage/openapi.json` around lines
10576 - 10604, The GroupedSoupPage schema currently allows "groups" to be null
and doesn't require it, weakening the contract for the /items/soup/ast/grouped
response; update the GroupedSoupPage definition so "groups" is non-nullable
(type "array" of ApiGroupMeta) and add "groups" to the schema's required array
(alongside "items") so clients always receive group metadata for grouped
responses (update the "groups" property and the "required" list referencing
GroupedSoupPage and ApiGroupMeta).
synoet
requested changes
May 27, 2026
Contributor
synoet
left a comment
synoet
left a comment
There was a problem hiding this comment.
I think the normalized grouped response is a sound direction, but I’m not sold on the current API shape.
Right now the same request/response type represents two different operations:
- “Fetch the initial grouped soup view across multiple groups”
- “Fetch the next page for one specific group”
Using group_key?: string as the switch between those modes feels too implicit. It also makes fields like limit and top-level next_cursor ambiguous, because their meaning differs between the initial fetch and the group-page fetch.
I think we should either split these operations, or keep one endpoint but model the request/response as explicit separate shapes, e.g.
type Request =
| (Filters & {
mode: 'initial';
group_by: GroupByField;
per_group_limit?: number | null;
sort_method?: GroupedSoupSort | null;
})
| (Filters & {
mode: 'group_page';
group_by: GroupByField;
group_key: string;
cursor?: string | null;
limit?: number | null;
sort_method?: GroupedSoupSort | null;
});
type Response =
| {
mode: 'initial';
items_by_id: Record<string, SoupApiItem>;
groups: Array<{
key: string;
label: string;
display_order?: number | null;
total_count: number;
item_ids: string[];
next_cursor?: string | null;
}>;
}
| {
mode: 'group_page';
items_by_id: Record<string, SoupApiItem>;
group: {
key: string;
label: string;
display_order?: number | null;
total_count: number;
item_ids: string[];
next_cursor?: string | null;
};
};
| /// Group metadata - present when group_by is specified in the request | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub groups: Option<Vec<ApiGroupMeta>>, | ||
| /// Ordered group metadata for this grouped response. |
Member
There was a problem hiding this comment.
will there always be values in the group vec? if not, i think its worth doing a skip_serializing if Vec::is_empty. might make consuming on the FE safer
whutchinson98
approved these changes
May 28, 2026
Member
whutchinson98
left a comment
whutchinson98
left a comment
There was a problem hiding this comment.
On the rust side seems fine to me
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.
No description provided.