Skip to content

Commit f03a366

Browse files
authored
fix(analytics): repair dashboard timezone crash and click-to-message ad attribution (#1114)
1 parent b022e93 commit f03a366

47 files changed

Lines changed: 1607 additions & 387 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

apps/builder/__tests__/ads-analytics-context-memoization.test.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,12 @@ vi.mock("@chatbotx.io/business", () => ({
3636
listForChannel: mocks.listForChannel,
3737
},
3838
listCachedMessagingAdAccounts: vi.fn(),
39+
// The union's workspace-wide leg checks for the integration up front
40+
// (absence is a normal state, not a logged failure) — these suites
41+
// exercise the messaging leg, so report it as present.
42+
integrationFacebookAdsService: {
43+
findByWorkspaceId: vi.fn(async () => ({ id: "ifa-1" })),
44+
},
3945
buildMessagingAdsContext: vi.fn(),
4046
isAdsEligibleChannel: (channel: unknown) =>
4147
channel === "whatsapp" ||

apps/builder/__tests__/ads-analytics-timeseries.test.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,12 @@ vi.mock("@chatbotx.io/business", () => ({
1919
listForChannel: mocks.listForChannel,
2020
},
2121
listCachedMessagingAdAccounts: vi.fn(),
22+
// The union's workspace-wide leg checks for the integration up front
23+
// (absence is a normal state, not a logged failure) — these suites
24+
// exercise the messaging leg, so report it as present.
25+
integrationFacebookAdsService: {
26+
findByWorkspaceId: vi.fn(async () => ({ id: "ifa-1" })),
27+
},
2228
buildMessagingAdsContext: vi.fn(),
2329
isAdsEligibleChannel: (channel: unknown) =>
2430
channel === "whatsapp" ||

apps/builder/__tests__/ads-analytics-view-retarget.test.tsx

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -279,12 +279,17 @@ describe("AdsAnalyticsView — single-channel retarget row actions", () => {
279279
const buttons = Array.from(
280280
container.querySelectorAll<HTMLButtonElement>("button[data-menu-item]"),
281281
)
282-
const purchasesButton = buttons.find(
283-
(button) => button.textContent === "ads.analytics.thoseWhoPurchased",
282+
// Was "ads.analytics.thoseWhoPurchased" — that entry is
283+
// TEMPORARILY HIDDEN (conversion tracking unfinished), so this drives the same
284+
// regression through the one segment still rendered. Switch it back when
285+
// the purchases/leads entries are restored.
286+
const conversationsButton = buttons.find(
287+
(button) =>
288+
button.textContent === "ads.analytics.thoseWhoStartedConversation",
284289
)
285290

286291
await act(async () => {
287-
purchasesButton?.click()
292+
conversationsButton?.click()
288293
await flush()
289294
})
290295

apps/builder/__tests__/ads-analytics-view-revenue-delivery.test.tsx

Lines changed: 24 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,21 @@ describe("AdsAnalyticsView revenue and delivery", () => {
187187
container.remove()
188188
})
189189

190-
test("renders revenue, ROAS, and delivery status details", async () => {
190+
// Renamed and narrowed: revenue/ROAS/delivery are
191+
// TEMPORARILY HIDDEN (conversion tracking unfinished). When they come back,
192+
// restore the name and these assertions:
193+
// expect(container.textContent).toContain("ads.analytics.revenue")
194+
// expect(container.textContent).toContain("ads.analytics.roas")
195+
// expect(container.textContent).toContain("2.50x")
196+
// expect(container.textContent).toContain("ads.analytics.delivery.title")
197+
// expect(container.textContent).toContain("ads.analytics.delivery.sent")
198+
// expect(container.textContent).toContain("ads.analytics.delivery.skippedNoScope")
199+
// expect(container.textContent).toContain("ads.analytics.delivery.noScopeWarning")
200+
// expect(container.textContent).toContain("ads.analytics.delivery.reconnectCta")
201+
// expect(Array.from(container.querySelectorAll("a")).some((anchor) =>
202+
// anchor.href.includes("/whatsapps/iw-1/ads"))).toBe(true)
203+
// expect(container.textContent).not.toContain("ads.analytics.delivery.skippedRegion")
204+
test("renders the Insights-sourced spend metrics", async () => {
191205
await act(async () => {
192206
root.render(
193207
<QueryClientProvider client={queryClient}>
@@ -209,9 +223,6 @@ describe("AdsAnalyticsView revenue and delivery", () => {
209223
await Promise.resolve()
210224
})
211225

212-
expect(container.textContent).toContain("ads.analytics.revenue")
213-
expect(container.textContent).toContain("ads.analytics.roas")
214-
expect(container.textContent).toContain("2.50x")
215226
expect(container.textContent).toContain("ads.analytics.impressions")
216227
expect(container.textContent).toContain("5,000")
217228
expect(container.textContent).toContain("ads.analytics.clicks")
@@ -221,28 +232,14 @@ describe("AdsAnalyticsView revenue and delivery", () => {
221232
expect(container.textContent).toContain("4.00%")
222233
expect(container.textContent).toContain("ads.analytics.cpm")
223234
expect(container.textContent).toContain("ads.analytics.costPerConversation")
224-
expect(container.textContent).toContain("ads.analytics.delivery.title")
225-
expect(container.textContent).toContain("ads.analytics.delivery.sent")
226-
expect(container.textContent).toContain(
227-
"ads.analytics.delivery.skippedNoScope",
228-
)
229-
expect(container.textContent).toContain(
230-
"ads.analytics.delivery.noScopeWarning",
231-
)
232-
expect(container.textContent).toContain(
233-
"ads.analytics.delivery.reconnectCta",
234-
)
235-
expect(
236-
Array.from(container.querySelectorAll("a")).some((anchor) =>
237-
anchor.href.includes("/whatsapps/iw-1/ads"),
238-
),
239-
).toBe(true)
240-
expect(container.textContent).not.toContain(
241-
"ads.analytics.delivery.skippedRegion",
242-
)
235+
// Guards the hidden state itself: the delivery card must not render while
236+
// the conversion-rule engine is unfinished. Delete this when restoring.
237+
expect(container.textContent).not.toContain("ads.analytics.delivery.title")
243238
})
244239

245-
test("omits the reconnect CTA link in the aggregate (no account) view", async () => {
240+
// TEMPORARILY HIDDEN (conversion tracking unfinished): the delivery card this
241+
// exercises is commented out in AdsAnalyticsView. Un-skip when it returns.
242+
test.skip("omits the reconnect CTA link in the aggregate (no account) view", async () => {
246243
await act(async () => {
247244
root.render(
248245
<QueryClientProvider client={queryClient}>
@@ -272,7 +269,9 @@ describe("AdsAnalyticsView revenue and delivery", () => {
272269
)
273270
})
274271

275-
test("shows a messenger-channel reconnect CTA linked to the messenger ads settings page", async () => {
272+
// TEMPORARILY HIDDEN (conversion tracking unfinished): the delivery card this
273+
// exercises is commented out in AdsAnalyticsView. Un-skip when it returns.
274+
test.skip("shows a messenger-channel reconnect CTA linked to the messenger ads settings page", async () => {
276275
await act(async () => {
277276
root.render(
278277
<QueryClientProvider client={queryClient}>

apps/builder/__tests__/channel-ad-accounts.test.ts

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ const mocks = vi.hoisted(() => ({
77
listCachedMessagingAdAccounts: vi.fn(),
88
listForChannel: vi.fn(),
99
getCachedAdAccounts: vi.fn(),
10+
findByWorkspaceId: vi.fn(),
1011
warn: vi.fn(),
1112
}))
1213

@@ -15,6 +16,9 @@ vi.mock("@chatbotx.io/business", () => ({
1516
messagingAdsConnectionService: {
1617
listForChannel: mocks.listForChannel,
1718
},
19+
integrationFacebookAdsService: {
20+
findByWorkspaceId: mocks.findByWorkspaceId,
21+
},
1822
}))
1923

2024
vi.mock("@/features/integration-facebook-ads/queries", () => ({
@@ -28,6 +32,9 @@ vi.mock("@/lib/log", () => ({
2832
describe("resolveChannelAdAccountSources", () => {
2933
beforeEach(() => {
3034
vi.clearAllMocks()
35+
// Default: the workspace HAS a workspace-wide Facebook Ads integration, so
36+
// existing cases keep exercising the `getCachedAdAccounts` leg unchanged.
37+
mocks.findByWorkspaceId.mockResolvedValue({ id: "ifa-1" })
3138
})
3239

3340
test("integrationId given -> narrows to that integration's own connection, tagged with a messaging source", async () => {
@@ -236,4 +243,55 @@ describe("resolveChannelAdAccountSources", () => {
236243
])
237244
expect(mocks.listCachedMessagingAdAccounts).not.toHaveBeenCalled()
238245
})
246+
247+
// A workspace with no workspace-wide Facebook Ads integration is the normal
248+
// case now that each box connects its own token. Absence is a STATE, not a
249+
// failure: the union must simply skip that leg, without reaching for a
250+
// throwing lookup and without writing a stack trace to the log. The previous
251+
// implementation called `findByWorkspaceIdOrFail` through
252+
// `getCachedAdAccounts`, so every ordinary dashboard load logged
253+
// `WARN "Facebook Ads integration not found"` with a full stack — noise that
254+
// reads like a real fault while nothing is actually wrong.
255+
test("no workspace-wide integration -> skips that leg silently, no log", async () => {
256+
mocks.findByWorkspaceId.mockResolvedValue(undefined)
257+
mocks.listForChannel.mockResolvedValue([])
258+
259+
const result = await resolveChannelAdAccountSources({
260+
workspaceId: "ws-1",
261+
channel: "messenger",
262+
})
263+
264+
expect(result).toEqual([])
265+
expect(mocks.getCachedAdAccounts).not.toHaveBeenCalled()
266+
expect(mocks.warn).not.toHaveBeenCalled()
267+
})
268+
269+
test("a genuine failure loading the workspace-wide list is still warned about", async () => {
270+
mocks.listForChannel.mockResolvedValue([])
271+
mocks.getCachedAdAccounts.mockRejectedValue(new Error("graph exploded"))
272+
273+
const result = await resolveChannelAdAccountSources({
274+
workspaceId: "ws-1",
275+
channel: "messenger",
276+
})
277+
278+
expect(result).toEqual([])
279+
expect(mocks.warn).toHaveBeenCalledTimes(1)
280+
})
281+
// The integration lookup sits inside the same guard as the Graph call: a
282+
// database blip on that preliminary read must degrade the union to "no
283+
// workspace-wide accounts", exactly as a Graph failure does — never fail the
284+
// whole Ads page.
285+
test("a failing integration lookup degrades instead of failing the union", async () => {
286+
mocks.findByWorkspaceId.mockRejectedValue(new Error("connection reset"))
287+
mocks.listForChannel.mockResolvedValue([])
288+
289+
const result = await resolveChannelAdAccountSources({
290+
workspaceId: "ws-1",
291+
channel: "messenger",
292+
})
293+
294+
expect(result).toEqual([])
295+
expect(mocks.warn).toHaveBeenCalledTimes(1)
296+
})
239297
})

apps/builder/src/app/space/[workspaceId]/dashboard/ads/[channel]/page.tsx

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,7 @@
1-
import { perChannelIntegrationIds } from "@chatbotx.io/business"
1+
import {
2+
type CapiDeliverySummary,
3+
perChannelIntegrationIds,
4+
} from "@chatbotx.io/business"
25
import {
36
type AdsEligibleChannelType,
47
adsEligibleChannelTypes,
@@ -11,7 +14,6 @@ import { resolveChannelIntegrations } from "@/features/ads/lib/resolve-channel-i
1114
import {
1215
getAdsAnalyticsData,
1316
getAdsAnalyticsTimeseries,
14-
getCapiDeliveryData,
1517
} from "@/features/ads/queries/analytics"
1618
import { getAdsSwitcherData } from "@/features/ads/queries/switcher"
1719
import { adsAnalyticsSearchParamsCache } from "@/features/ads/schema/analytics"
@@ -74,7 +76,20 @@ export default async function AdsChannelAnalyticsPage(props: {
7476

7577
const promises = Promise.all([
7678
getAdsAnalyticsData(workspaceId, analyticsRange),
77-
getCapiDeliveryData(workspaceId, analyticsRange),
79+
// TEMPORARILY HIDDEN (conversion tracking unfinished): the delivery card
80+
// that consumes this is commented out in `AdsAnalyticsView`, so running
81+
// the query would cost every dashboard load a round trip nothing renders.
82+
// A resolved zero summary keeps the tuple shape — and therefore the
83+
// component's `promises` prop type — unchanged, so restoring the card is
84+
// swapping this one line back for:
85+
// getCapiDeliveryData(workspaceId, analyticsRange),
86+
Promise.resolve<CapiDeliverySummary>({
87+
sent: 0,
88+
pending: 0,
89+
failed: 0,
90+
skippedNoScope: 0,
91+
skippedRegion: 0,
92+
}),
7893
getAdsAnalyticsTimeseries(workspaceId, analyticsRange),
7994
])
8095

apps/builder/src/app/space/[workspaceId]/dashboard/layout.tsx

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,15 @@ export default async function DashboardLayout({
3838
const cloud = isCloud()
3939
const { targetWorkspace } = userAndWorkspace
4040
const [inboxesResult, { blocked, blockReason }] = await Promise.all([
41-
listInboxes({ workspaceId, includes: ["integration"] }),
41+
// Disconnected inboxes stay listed: Settings -> Channels shows an
42+
// integration whether or not its inbox is still connected, so hiding them
43+
// here made a channel silently disappear from one surface while remaining
44+
// on the other, with nothing on screen explaining the difference.
45+
listInboxes({
46+
workspaceId,
47+
includes: ["integration"],
48+
statuses: ["connected", "disconnected"],
49+
}),
4250
resolveWorkspaceBlockState(targetWorkspace.ownerId),
4351
])
4452
const inboxes = inboxesResult.data.filter((inbox) => inbox.channel !== "smtp")

0 commit comments

Comments
 (0)