Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 54 additions & 0 deletions packages/core/src/inbox/engagement.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import type { SignalReport } from "@posthog/shared/types";
import { describe, expect, it } from "vitest";
import {
buildBulkActionEvents,
buildInboxViewedProperties,
type InboxDetailTab,
inboxDetailTabReports,
Expand Down Expand Up @@ -33,6 +34,59 @@ const NO_FILTERS = {
isDefaultScope: true,
};

describe("buildBulkActionEvents", () => {
it("marks single-report actions as non-bulk", () => {
const [event, ...rest] = buildBulkActionEvents({
reports: [fakeReport({ id: "a", priority: "P1" })],
actionType: "snooze",
surface: "detail_pane",
});

expect(rest).toHaveLength(0);
expect(event).toMatchObject({
report_id: "a",
action_type: "snooze",
surface: "detail_pane",
is_bulk: false,
bulk_size: 1,
priority: "P1",
});
});

it("emits one bulk-flagged event per report", () => {
const events = buildBulkActionEvents({
reports: [fakeReport({ id: "a" }), fakeReport({ id: "b" })],
actionType: "delete",
surface: "toolbar",
});

expect(events.map((e) => e.report_id)).toEqual(["a", "b"]);
expect(events.every((e) => e.is_bulk && e.bulk_size === 2)).toBe(true);
expect(events.every((e) => e.action_type === "delete")).toBe(true);
});

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();
});
Comment on lines +68 to +87

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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)

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!

});

describe("buildInboxViewedProperties", () => {
it("counts visible reports, tab badges, and total", () => {
const props = buildInboxViewedProperties({
Expand Down
51 changes: 51 additions & 0 deletions packages/core/src/inbox/engagement.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import type {
InboxReportActionProperties,
InboxReportActionSurface,
InboxViewedProperties,
} from "@posthog/shared/analytics-events";
import type { SignalReport } from "@posthog/shared/domain-types";
Expand Down Expand Up @@ -119,6 +120,56 @@ export function resolveActionProperties(
return { rank, list_size: listSize, priority, actionability };
}

/** Bulk-capable report actions fired from the selection toolbar / dismiss flows. */
export type InboxBulkActionType = Extract<
InboxReportActionProperties["action_type"],
"dismiss" | "snooze" | "delete" | "reingest"
>;

export interface BuildBulkActionEventsInput {
/** Reports the action actually succeeded for (one event is built per report). */
reports: SignalReport[];
actionType: InboxBulkActionType;
surface: InboxReportActionSurface;
/** Dismissal metadata, only meaningful for `dismiss`. Note is truncated to 500 chars. */
dismissal?: { reason?: string; note?: string };
}

/**
* Build `INBOX_REPORT_ACTION` payloads for a bulk (or single-report) dismiss /
* snooze / delete / reingest. Pure so it can be unit-tested and reused across
* the toolbar, the per-row dismiss action, and detail-screen dismiss.
*
* `is_bulk` / `bulk_size` carry the grouping; `rank` / `list_size` are left at 0
* because these flows act on a selection, not a positional list slot.
*/
export function buildBulkActionEvents(
input: BuildBulkActionEventsInput,
): InboxReportActionProperties[] {
const { reports, actionType, surface, dismissal } = input;
const bulkSize = reports.length;
const isBulk = bulkSize > 1;
Comment on lines +150 to +151

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve bulk metadata on partial success

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 👍 / 👎.

return reports.map((report) => ({
report_id: report.id,
report_title: report.title ?? null,
report_age_hours: reportAgeHours(report.created_at),
priority: report.priority ?? null,
actionability: report.actionability ?? null,
action_type: actionType,
surface,
is_bulk: isBulk,
bulk_size: bulkSize,
rank: 0,
list_size: 0,
...(actionType === "dismiss" && dismissal?.reason
? { dismissal_reason: dismissal.reason }
: {}),
...(actionType === "dismiss" && dismissal?.note
? { dismissal_note: dismissal.note.slice(0, 500) }
: {}),
}));
}

export interface InboxViewedFilterState {
sourceProductFilter: string[];
priorityFilter: string[];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,11 @@ export function InboxReportListTab({
);

const dismissTargetId = dismissReport?.id ?? null;
const dismissBulkActions = useInboxBulkActions(allReports, dismissTargetId);
const dismissBulkActions = useInboxBulkActions(
allReports,
dismissTargetId,
"list_row",
);

const handleDismissDialogOpenChange = useCallback((open: boolean) => {
if (!open) setDismissReport(null);
Expand Down
46 changes: 45 additions & 1 deletion packages/ui/src/features/inbox/hooks/useInboxBulkActions.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,16 @@
import {
buildBulkActionEvents,
type InboxBulkActionType,
} from "@posthog/core/inbox/engagement";
import { inboxStatusLabel } from "@posthog/core/inbox/reportPresentation";
import type { InboxReportActionSurface } from "@posthog/shared/analytics-events";
import { ANALYTICS_EVENTS } from "@posthog/shared/analytics-events";
import type { SignalReport } from "@posthog/shared/types";
import type { DismissReportDialogResult } from "@posthog/ui/features/inbox/components/DismissReportDialog";
import { reportKeys } from "@posthog/ui/features/inbox/hooks/useInboxReports";
import { useInboxReportSelectionStore } from "@posthog/ui/features/inbox/stores/inboxReportSelectionStore";
import { useAuthenticatedMutation } from "@posthog/ui/hooks/useAuthenticatedMutation";
import { track } from "@posthog/ui/shell/analytics";
import { useQueryClient } from "@tanstack/react-query";
import { useCallback, useMemo } from "react";
import { toast } from "sonner";
Expand Down Expand Up @@ -187,6 +194,7 @@ export function buildSuppressDisabledReasonMap(
export function useInboxBulkActions(
reports: SignalReport[],
selection: InboxBulkSelection,
surface: InboxReportActionSurface = "toolbar",
) {
const queryClient = useQueryClient();
const clearSelection = useInboxReportSelectionStore(
Expand All @@ -196,6 +204,38 @@ export function useInboxBulkActions(
(state) => state.removeFromSelection,
);

/**
* Emit one `INBOX_REPORT_ACTION` per report the action succeeded for. Resolves
* report metadata from the pre-mutation list (the query is invalidated right
* after, dropping the affected reports), and skips firing when nothing landed.
*/
const trackBulkAction = useCallback(
(
actionType: InboxBulkActionType,
result: BulkActionResult,
dismissal?: DismissReportDialogResult,
) => {
if (result.successCount === 0) return;
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);
Comment on lines +219 to +222

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Snapshot report metadata before the mutation

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 👍 / 👎.

if (succeeded.length === 0) return;
const events = buildBulkActionEvents({
reports: succeeded,
actionType,
surface,
dismissal: dismissal
? { reason: dismissal.reason, note: dismissal.note }
: undefined,
});
for (const event of events) {
track(ANALYTICS_EVENTS.INBOX_REPORT_ACTION, event);
}
},
[reports, surface],
);

/**
* Reflect a bulk-action result in the selection: drop succeeded ids so the
* user can retry the failed subset; keep everything if nothing succeeded so
Expand Down Expand Up @@ -247,7 +287,8 @@ export function useInboxBulkActions(
);
},
{
onSuccess: async (result) => {
onSuccess: async (result, variables) => {
trackBulkAction("dismiss", result, variables.dismissal);
await invalidateInboxQueries();
applyBulkResultToSelection(result);

Expand All @@ -274,6 +315,7 @@ export function useInboxBulkActions(
),
{
onSuccess: async (result) => {
trackBulkAction("snooze", result);
await invalidateInboxQueries();
applyBulkResultToSelection(result);

Expand All @@ -297,6 +339,7 @@ export function useInboxBulkActions(
),
{
onSuccess: async (result) => {
trackBulkAction("delete", result);
await invalidateInboxQueries();
applyBulkResultToSelection(result);

Expand All @@ -320,6 +363,7 @@ export function useInboxBulkActions(
),
{
onSuccess: async (result) => {
trackBulkAction("reingest", result);
await invalidateInboxQueries();
applyBulkResultToSelection(result);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ export function useInboxReportDismissAction(report: SignalReport): {
const bulkActions = useInboxBulkActions(
reportsForActions,
open ? report.id : null,
"detail_pane",
);

const isPending = bulkActions.isSuppressing || bulkActions.isSnoozing;
Expand Down
Loading