feat(inbox): track INBOX_REPORT_ACTION for bulk dismiss/snooze/delete/reingest#2750
feat(inbox): track INBOX_REPORT_ACTION for bulk dismiss/snooze/delete/reingest#2750andrewm4894 wants to merge 1 commit into
Conversation
…/reingest The selection-toolbar and dismiss flows previously fired no analytics, so bulk dismiss/snooze/delete/reingest actions were missing from INBOX_REPORT_ACTION entirely. Add a pure buildBulkActionEvents helper in engagement.ts (one event per succeeded report, is_bulk/bulk_size grouping, dismiss-only reason/note truncated to 500 chars) and thread a surface arg through useInboxBulkActions so the toolbar, per-row dismiss, and detail-pane dismiss each report their origin. Events fire only for reports the mutation actually succeeded for, resolved from the pre-invalidation list.
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/core/src/inbox/engagement.test.ts:68-87
**Two scenarios in one non-parameterised test**
The third test conflates "dismiss attaches metadata and truncates the note" with "non-dismiss action type drops the metadata" into a single `it()`, even though the rest of the file already uses `it.each` for exactly this pattern (see the `has_active_filters` table below). Splitting these into a parameterised row-per-action-type table would make it immediately obvious which action types should and shouldn't carry dismissal fields, and makes it trivial to add `delete`/`reingest` coverage later.
Reviews (1): Last reviewed commit: "feat(inbox): track INBOX_REPORT_ACTION f..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| it("attaches dismissal reason/note only for dismiss, truncating the note", () => { | ||
| const longNote = "x".repeat(600); | ||
| const [dismissed] = buildBulkActionEvents({ | ||
| reports: [fakeReport({ id: "a" })], | ||
| actionType: "dismiss", | ||
| surface: "toolbar", | ||
| dismissal: { reason: "not_relevant", note: longNote }, | ||
| }); | ||
| expect(dismissed.dismissal_reason).toBe("not_relevant"); | ||
| expect(dismissed.dismissal_note).toHaveLength(500); | ||
|
|
||
| const [snoozed] = buildBulkActionEvents({ | ||
| reports: [fakeReport({ id: "a" })], | ||
| actionType: "snooze", | ||
| surface: "toolbar", | ||
| dismissal: { reason: "not_relevant", note: longNote }, | ||
| }); | ||
| expect(snoozed.dismissal_reason).toBeUndefined(); | ||
| expect(snoozed.dismissal_note).toBeUndefined(); | ||
| }); |
There was a problem hiding this comment.
The third test conflates "dismiss attaches metadata and truncates the note" with "non-dismiss action type drops the metadata" into a single it(), even though the rest of the file already uses it.each for exactly this pattern (see the has_active_filters table below). Splitting these into a parameterised row-per-action-type table would make it immediately obvious which action types should and shouldn't carry dismissal fields, and makes it trivial to add delete/reingest coverage later.
Context Used: Do not attempt to comment on incorrect alphabetica... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/core/src/inbox/engagement.test.ts
Line: 68-87
Comment:
**Two scenarios in one non-parameterised test**
The third test conflates "dismiss attaches metadata and truncates the note" with "non-dismiss action type drops the metadata" into a single `it()`, even though the rest of the file already uses `it.each` for exactly this pattern (see the `has_active_filters` table below). Splitting these into a parameterised row-per-action-type table would make it immediately obvious which action types should and shouldn't carry dismissal fields, and makes it trivial to add `delete`/`reingest` coverage later.
**Context Used:** Do not attempt to comment on incorrect alphabetica... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d2189cf6e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const bulkSize = reports.length; | ||
| const isBulk = bulkSize > 1; |
There was a problem hiding this comment.
When a user selects multiple reports and only one succeeds, trackBulkAction passes only the succeeded reports into this helper, so reports.length becomes 1 and the emitted INBOX_REPORT_ACTION is marked is_bulk: false with bulk_size: 1. That makes partial failures of bulk dismiss/snooze/delete/reingest actions indistinguishable from single-report actions in analytics; carry the attempted selection size through separately from the succeeded report list.
Useful? React with 👍 / 👎.
| const byId = new Map(reports.map((report) => [report.id, report])); | ||
| const succeeded = result.succeededIds | ||
| .map((id) => byId.get(id)) | ||
| .filter((report): report is SignalReport => report !== undefined); |
There was a problem hiding this comment.
Because the inbox list polls every 3 seconds, a longer bulk mutation can succeed for some reports and then refetch them out of reports before onSuccess runs for the whole batch. This current-list lookup then drops those IDs even though result.succeededIds contains them, so successful dismiss/snooze/delete/reingest operations can emit no INBOX_REPORT_ACTION for those reports; snapshot the selected report metadata at mutation start or emit fallback events from the succeeded IDs.
Useful? React with 👍 / 👎.
1 participant
What
Fills a gap in inbox analytics: bulk dismiss / snooze / delete / reingest actions fired no
INBOX_REPORT_ACTIONevents, so these interactions were missing from the funnel entirely. This wires them up, reusing the existing event shape.Changes
packages/core/src/inbox/engagement.ts— new purebuildBulkActionEventshelper. Builds oneINBOX_REPORT_ACTIONpayload per report, withis_bulk/bulk_sizegrouping, and dismiss-onlydismissal_reason/dismissal_note(note truncated to 500 chars). Pure so it's unit-testable and reusable across the toolbar, per-row dismiss, and detail-pane dismiss.useInboxBulkActions.ts— threads asurfacearg through and emits one event per report the mutation actually succeeded for, resolving report metadata from the pre-invalidation list (the query is invalidated immediately after, dropping the affected reports). Skips firing when nothing landed.InboxReportListTab.tsx/useInboxReportDismissAction.tsx— pass the correctsurface(list_row/detail_pane) so each origin is distinguishable from the defaulttoolbar.Tests
engagement.test.tscovers single-vs-bulk flagging, one-event-per-report, and dismiss-only metadata with note truncation.pnpm typecheck+ Biome pass (pre-commit hook).Notes
Split out from the agents-page analytics work (#2747, now merged). No backend or UI-behavior changes — analytics only.