Skip to content

feat(advertising): add audience segments - #11658

Merged
lalalune merged 3 commits into
developfrom
feat/11597-audience-segments
Jul 3, 2026
Merged

lalalune merged 3 commits into
developfrom
feat/11597-audience-segments

Conversation

@lalalune

@lalalune lalalune commented Jul 2, 2026

Copy link
Copy Markdown
Member

Summary

  • add organization-scoped advertising audience segment storage, repository, migration, service methods, and API routes
  • allow campaigns and app promotion advertising config to apply a saved segment by audienceSegmentId
  • validate and normalize targeting, map supported fields into Meta targeting, and expose a saved-segment selector in the promotion dialog

Fixes #11597

Evidence

  • Evidence file: .github/issue-evidence/11597-audience-segments.md
  • bun install passed
  • bun run build:core passed
  • bun run --cwd packages/cloud/api codegen passed, regenerated 622 mounted routes after rebase
  • bun run --cwd packages/cloud/shared typecheck passed
  • bun run --cwd packages/cloud/api typecheck passed
  • bun run --cwd packages/ui typecheck passed
  • bun test packages/cloud/shared/src/lib/services/__tests__/ad-audience-segments.test.ts passed: 4 tests, 11 assertions
  • git diff --check HEAD~1..HEAD passed

App audit

  • bun run --cwd packages/app audit:app ran after the UI change: 348/349 Playwright checks passed
  • The final aggregate gate failed on two unrelated minimalism ratchets: plugin-inbox-gui @ mobile-landscape and plugin-screenshare-gui @ mobile-portrait
  • The touched reachable /apps views passed across mobile portrait, mobile landscape, desktop landscape, and iPad portrait

N/A

  • Live LLM trajectory: N/A - no prompt/model/agent behavior changed
  • Live ad spend/provider campaign creation: N/A - no live ad account credentials or spend used locally; provider path covered by service tests with adapter stubs
  • Video walkthrough: N/A - selector-only dashboard change; audit screenshots were captured, with unrelated aggregate ratchet failure documented above

@coderabbitai

coderabbitai Bot commented Jul 2, 2026 •

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b1a34795-c797-44e3-b64e-2afcf7c86072

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/11597-audience-segments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@lalalune

lalalune commented Jul 3, 2026

Copy link
Copy Markdown
Member Author

Review (adversarial, local verification)

Verdict: rebase + fixes pushed; recommend MERGE (mark ready). Work is complete despite draft status.

What I verified locally (worktree off the PR head, rebased onto develop)

  • bun test .../ad-audience-segments.test.ts → 4 pass / 0 fail (11 assertions).
  • Mutation check: weakened the cross-org guard in resolveAudienceTargeting (dropped the organization_id !== organizationId half) → the cross-org denial test goes red. The test is real, not a mock-of-the-thing.
  • Authz: all 6 new routes use requireUserOrApiKeyWithOrg; every service path (get/update/delete/resolve/apply→updateCampaign) re-checks organization_id and fails closed (not found → 404 via failureResponse). Cross-org apply is denied before any account/provider work (asserted in test).
  • Performance: segments are resolved only at campaign create/update via PK lookup; list is org-indexed (ad_audience_segments_organization_idx). No per-ad-request evaluation exists (targeting is pushed to the provider), so no full-table-scan-per-request risk.
  • Migration proven on PGlite: applies cleanly, idempotent on re-run (IF NOT EXISTS), 3 indexes + pkey created, FK rejects orphan org, org cascade-delete works.
  • tsgo --noEmit clean for cloud/shared and ui; biome clean on all touched files. cloud/api typecheck shows 301 errors in untouched files — all dual-drizzle-instance artifacts of my shared node_modules setup, zero in PR files.
  • PGlite pushSchema-bundle trap: N/A — new standalone table, no pgEnum, bundles import per-table schema files, not the index.

Fixes I pushed (commit a41b0bc7401)

  1. Migration number collision: 0165_ad_audience_segments.sql collided with develop's 0165_credit_reservation_settled_at.sql → renumbered to 0166.
  2. Missing drizzle journal entry: the PR never registered its migration in meta/_journal.json, so drizzle migrate would never apply it → added idx 164 / tag 0166_ad_audience_segments.
  3. Pre-existing brittle assertion: pooled-credentials-migration.test.ts asserted journal.entries.at(-1) === "0164_pooled_credentials" — already red on develop since 0165 landed; made it an order-independent some() membership check (verified red→green).
  4. Rebased onto develop (only conflict was the generated router; resolved by re-running codegen, 621 mounted routes).

Notes (non-blocking)

  • Applying a segment snapshots its targeting into the campaign; later segment edits don't re-sync applied campaigns. Reasonable semantics, worth documenting.
  • The PR also fixes a latent camelCase-vs-snake_case drift: campaigns previously stored route-shaped ageMin under a age_min-typed jsonb column. New writes are correct; pre-existing rows keep camelCase age keys (cosmetic on read, no backfill included).
  • placements → Meta publisher_platforms is free-text (bounded/deduped) rather than the Meta enum; invalid values fail at the provider with the existing refund path.

🤖 Generated with Claude Code

@lalalune

lalalune commented Jul 3, 2026

Copy link
Copy Markdown
Member Author

Reviewed and pushed a fix commit: a7f33fb0bb3 (fix(advertising): reject post-sync audience retargeting). Leaving draft/unmerged.

Finding fixed:

  • Applying an audience segment to an existing synced campaign went through AdvertisingService.updateCampaign, but the Meta provider updateCampaign only updates the campaign object name and does not update ad set targeting. That meant the DB could claim new targeting that was never applied on the ad platform.
  • The service now fails closed for targeting / audienceSegmentId on post-sync campaign updates with a 400 ValidationError. Create-time segment use still works because createCampaign sends targeting into the provider ad set creation path.

Validation run on the PR head after the fix:

  • bun --config=/dev/null test packages/cloud/shared/src/lib/services/__tests__/ad-audience-segments.test.ts — passed, 4 tests.
  • bun run --cwd packages/cloud/shared typecheck — passed.
  • bunx @biomejs/biome@2.5.2 check --files-ignore-unknown=true packages/cloud/shared/src/lib/services/advertising/index.ts packages/cloud/shared/src/lib/services/__tests__/ad-audience-segments.test.ts — passed.
  • git diff --check origin/develop...HEAD && git diff --check — passed.

The migration renumber/journal fix in a41b0bc also looks correct at a quick pass: new 0166_ad_audience_segments.sql is registered after 0165_credit_reservation_settled_at.

@lalalune
lalalune marked this pull request as ready for review July 3, 2026 01:18

@greptile-apps greptile-apps Bot left a comment

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.

Your trial has ended. Reactivate Greptile to resume code reviews.

claude and others added 3 commits July 2, 2026 21:25
…ster in drizzle journal

- 0165 collided with 0165_credit_reservation_settled_at on develop
- the migration was never added to meta/_journal.json, so drizzle migrate
  would never apply it
- make the 0164_pooled_credentials journal assertion order-independent
  (it asserted entries.at(-1), which went stale the moment 0165 landed)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@lalalune
lalalune force-pushed the feat/11597-audience-segments branch from a7f33fb to df6f6a3 Compare July 3, 2026 01:27

@greptile-apps greptile-apps Bot left a comment

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.

Your trial has ended. Reactivate Greptile to resume code reviews.

@lalalune
lalalune merged commit 003ab4b into develop Jul 3, 2026
36 of 66 checks passed
@lalalune
lalalune deleted the feat/11597-audience-segments branch July 3, 2026 01:28
@claude

claude Bot commented Jul 3, 2026 •

Copy link
Copy Markdown
Contributor

Claude encountered an error —— View job


I'll analyze this and get back to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cloud advertising: audience segment builder for campaign targeting (#11363)

2 participants