-
Notifications
You must be signed in to change notification settings - Fork 14
chore(KNO-11486): exclude metadata when refetching feed after new message received #853
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: mattmik-kno-11394-support-compact-mode-for-feeds
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: f71b7c7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
@cursor review |
|
@cursor review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
|
|
||
| // Always include the default params, if they have been set | ||
| const queryParams: FetchFeedOptionsForRequest = { | ||
| ...this.defaultOptions, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it okay we are not using mergedOptions here? i.e. we don't include options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. This was working because the destructure of ...mergeDateRangeParams(options) accomplishes the same thing. But I think it’s better if we use mergedOptions everywhere, so I made that change.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## mattmik-kno-11394-support-compact-mode-for-feeds #853 +/- ##
====================================================================================
+ Coverage 68.45% 68.58% +0.12%
====================================================================================
Files 193 193
Lines 8062 8088 +26
Branches 1065 1076 +11
====================================================================================
+ Hits 5519 5547 +28
+ Misses 2518 2516 -2
Partials 25 25
|
| "inserted_at.gte"?: string; | ||
| "inserted_at.lte"?: string; | ||
| "inserted_at.gt"?: string; | ||
| "inserted_at.lt"?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we mean to add these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it’s a mistake that they weren’t already in the FetchFeedOptionsForRequest type.
FetchFeedOptionsForRequest is meant to represent the params included in the API request. Notice that the mergeDateRangeParams function transforms the inserted_at_date_range option on FeedClientOptions into the appropriate date params, e.g. inserted_at.gte, inserted_at.lte, etc.
javascript/packages/client/src/clients/feed/utils.ts
Lines 25 to 50 in 9313398
| export function mergeDateRangeParams(options: FeedClientOptions) { | |
| const { inserted_at_date_range, ...rest } = options; | |
| if (!inserted_at_date_range) { | |
| return rest; | |
| } | |
| const dateRangeParams: Record<string, string> = {}; | |
| // Determine which operators to use based on the inclusive flag | |
| const isInclusive = inserted_at_date_range.inclusive ?? false; | |
| // For start date: use gte if inclusive, gt if not | |
| if (inserted_at_date_range.start) { | |
| const startOperator = isInclusive ? "inserted_at.gte" : "inserted_at.gt"; | |
| dateRangeParams[startOperator] = inserted_at_date_range.start; | |
| } | |
| // For end date: use lte if inclusive, lt if not | |
| if (inserted_at_date_range.end) { | |
| const endOperator = isInclusive ? "inserted_at.lte" : "inserted_at.lt"; | |
| dateRangeParams[endOperator] = inserted_at_date_range.end; | |
| } | |
| return { ...rest, ...dateRangeParams }; | |
| } |

Description
This PR updates the client SDK so that:
excludeoption, used to exclude specified fields from the list feed items response payload"new-message"is received, the request we make to the list feed items endpoint automatically excludes themetafield from its response.metacontains the badge counts, which are already present on the socket event payload, so querying formetais redundant.Todos
Checklist
Loom demo
https://www.loom.com/share/56c2bb427c31485ea8b837c3c416be04