-
Notifications
You must be signed in to change notification settings - Fork 5.5k
FrontApp source improvements #18472
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
FrontApp source improvements #18472
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/frontapp/sources/new-conversation-created/new-conversation-created.mjs (1)
33-38: Avoid missing items with identical timestampsUse >= so items created in the same second as lastTs aren’t skipped.
Apply this diff:
- await this.startEvent(25, (item, lastTs) => this._getItemTs(item) > lastTs); + await this.startEvent(25, (item, lastTs) => this._getItemTs(item) >= lastTs);- await this.startEvent(0, (item, lastTs) => this._getItemTs(item) > lastTs); + await this.startEvent(0, (item, lastTs) => this._getItemTs(item) >= lastTs);components/frontapp/sources/new-message-template-created/new-message-template-created.mjs (1)
33-38: Avoid missing templates with same-second timestampsUse >= to include items with the same created_at as lastTs; “unique” dedupe prevents re-emits.
Apply this diff:
- await this.startEvent(25, (item, lastTs) => this._getItemTs(item) > lastTs); + await this.startEvent(25, (item, lastTs) => this._getItemTs(item) >= lastTs);- await this.startEvent(0, (item, lastTs) => this._getItemTs(item) > lastTs); + await this.startEvent(0, (item, lastTs) => this._getItemTs(item) >= lastTs);
🧹 Nitpick comments (2)
components/frontapp/sources/common/base.mjs (1)
46-47: Sort using _getItemTs to avoid field couplingMinor: sort via helper to centralize timestamp logic.
Apply this diff:
- responseArray.sort((a, b) => b.created_at - a.created_at); + responseArray.sort((a, b) => this._getItemTs(b) - this._getItemTs(a));components/frontapp/sources/new-conversation-state-change/new-conversation-state-change.mjs (1)
31-33: Sorting params LGTM; verify after/field alignmentYou sort by created_at and filter with q[after] = lastTs, but emit ts uses emitted_at. Confirm that q[after] applies to created_at vs emitted_at to avoid gaps/duplicates. Consider aligning ts to the filtered field if needed.
If docs confirm q[after] filters on created_at, consider using created_at in _getEmit for ts to keep lastTs consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
components/frontapp/package.json(1 hunks)components/frontapp/sources/common/base.mjs(2 hunks)components/frontapp/sources/new-conversation-created/new-conversation-created.mjs(1 hunks)components/frontapp/sources/new-conversation-state-change/new-conversation-state-change.mjs(2 hunks)components/frontapp/sources/new-conversation-tag/new-conversation-tag.mjs(2 hunks)components/frontapp/sources/new-message-template-created/new-message-template-created.mjs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: pnpm publish
- GitHub Check: Lint Code Base
- GitHub Check: Publish TypeScript components
- GitHub Check: Verify TypeScript components
🔇 Additional comments (8)
components/frontapp/package.json (1)
3-3: Version bump LGTMNo behavioral changes. Safe to publish.
components/frontapp/sources/new-conversation-created/new-conversation-created.mjs (2)
9-9: Version bump LGTM
19-21: sort_bycreated_atunsupported—use client‐side sorting or q filters
The Front Conversations endpoint doesn’t acceptsort_by=created_at; it always orders by last‐activity ("date"). To paginate by creation time, fetch pages sorted by"date"then sort or filter locally onconversation.created_at, or use theq[before]/q[after]/q[during]filters on message timestamps.Likely an incorrect or invalid review comment.
components/frontapp/sources/new-message-template-created/new-message-template-created.mjs (1)
9-9: Version bump LGTMcomponents/frontapp/sources/common/base.mjs (2)
17-18: Good: remove arbitrary initial timestampDefaulting to 0 addresses the 2001-epoch issue and is predictable.
39-41: Early-break requires order by the same timestamp used in filterFnThis break is correct only if iteration is sorted by the same field used in filterFn (created_at). Ensure all sources using filterFn set sort_by=created_at desc; otherwise, you can miss newer items.
components/frontapp/sources/new-conversation-tag/new-conversation-tag.mjs (1)
9-9: Version bump LGTMcomponents/frontapp/sources/new-conversation-state-change/new-conversation-state-change.mjs (1)
9-10: Doc URL and version bump LGTM
Resolves #18465
The query object prop in the
list-conversationsendpoint only supports filtering by "statuses". However, we can limit pagination by exiting the loop when we get to a timestamp older thanlastTs.Summary by CodeRabbit
Improvements
Chores