Skip to content

fix(cohorts): accept BOM-prefixed CSV when building a static cohort - #82082

Merged
trunk-io[bot] merged 4 commits into
masterfrom
posthog-self-driving/fixcohorts-accept-bom-prefixed-csv-58af1b
Aug 25, 2026
Merged

trunk-io[bot] merged 4 commits into
masterfrom
posthog-self-driving/fixcohorts-accept-bom-prefixed-csv-58af1b

Conversation

@posthog

@posthog posthog Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Problem

  • Someone builds a static cohort from a CSV that Excel or Google Sheets saved, and the upload is rejected — around 35 people a month hit this, and one recorded session ended with the user deleting the cohort.
  • The error is self-contradictory: it says the file "must contain at least one column with a supported ID header … Found columns: person_id, email" — it names the exact columns it claims are missing.
  • Cause: Excel and Sheets prefix the first header with a byte order mark, so the header arrives as person_id. The upload decoded with plain utf-8 and normalized headers with only .strip(), which does not remove , so the match failed. The byte order mark renders as nothing, so both the file and the error look correct.
  • Quieter variant: a single-column CSV with a BOM-prefixed person_id header fell through to the backwards-compatible path and imported the header text itself as one junk cohort member.

Refs #42539

Changes

  • Decode CSV uploads with utf-8-sig, which strips the leading byte order mark.
  • Add a shared _normalize_header that also removes , so _find_id_column, _is_person_id_header, and _is_email_header all agree — a second layer that holds even if a mark appears mid-file.
  • Quote the echoed column names in the missing-header message ('name', 'age'), so an empty or invisible header reads as a file problem instead of a contradiction.

Not in scope: the report also notes conflicting save toasts in cohortEditLogic.ts. That is a separate, lower-severity UX issue in the frontend flow and does not cause the blocking error above, so it is left for a follow-up.

How did you test this code?

  • Added two backend tests: a multi-column and a single-column CSV, each with a BOM-prefixed person_id header.
    • Multi-column: catches the reported 400 — the file now builds the cohort with the two intended members.
    • Single-column: catches the silent regression — the person_id header text is treated as a header, not imported as a member.
  • Ran the new tests plus the existing missing-header and person-uuid CSV tests locally: 7 passed.
  • Did not manually reproduce in the running app; the symptom is a synchronous validation path fully covered by the backend tests.

Automatic notifications

  • Publish to changelog?

Docs update

None.

🤖 Agent context

Autonomy: Fully autonomous

  • Authored by an agent from a PostHog inbox report on a recurring static-cohort upload failure.
  • Skills consulted for the DRF serializer edit, test additions, user-facing error copy, and this description.
  • Decision: fix at both layers — utf-8-sig at decode plus BOM stripping in header normalization — rather than only one, so the single-column path and any mid-file mark are also covered. The message change avoids echoing an invisible character back as a contradiction.
  • Public-repo check: the added CSV fixtures use invented UUIDs and example.com addresses; nothing from the source report is committed.

Created with PostHog Desktop from this inbox report.

Files saved by Excel or Google Sheets prefix the first header with a byte
order mark, so `person_id` did not match the supported ID headers and
the upload failed with a self-contradictory error that listed the very
columns it rejected.

Decode uploads with utf-8-sig and strip the byte order mark in the shared
header normalization, so the person_id, distinct_id, and email checks agree.
Quote the echoed column names in the missing-header message so an invisible
character reads as a file problem, not a contradiction.

Generated-By: PostHog Desktop
Task-Id: da273aaa-f95e-4768-aa0a-f3a70c917714
@trunk-io

trunk-io Bot commented Aug 12, 2026 •

Copy link
Copy Markdown

😎 Merged successfully - details.

@trunk-io

trunk-io Bot commented Aug 12, 2026 •

Copy link
Copy Markdown

Static Badge   Static Badge   Static Badge

View Full Report ↗︎ ⋅ Docs

@scheduled-actions-posthog

Copy link
Copy Markdown
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, please remove the stale label – otherwise this will be closed in another week. If you want to permanently keep it open, use the waiting label.

@patricio-posthog patricio-posthog self-assigned this Aug 24, 2026
@patricio-posthog
patricio-posthog self-requested a review August 24, 2026 18:48
@posthog-project-board-bot posthog-project-board-bot Bot moved this from Todo to In Progress in Feature Flags Aug 24, 2026
@patricio-posthog
patricio-posthog marked this pull request as ready for review August 24, 2026 19:18
@posthog-project-board-bot posthog-project-board-bot Bot moved this from In Progress to In Review in Feature Flags Aug 24, 2026
@pr-assigner-resolver-posthog
pr-assigner-resolver-posthog Bot requested a review from a team August 24, 2026 19:19
@patricio-posthog
patricio-posthog removed the request for review from a team August 24, 2026 19:20
@github-actions

github-actions Bot commented Aug 24, 2026 •

Copy link
Copy Markdown
Contributor

🤖 CI report

⚠️ Trunk lane — backend Python lane

This PR is assigned to the backend Python lane. It runs backend Python tests and may merge in parallel with PRs in other lanes.

⚠️ Playwright — 1 failed

🎭 Playwright report · View test results →

❌ 1 failed test:

  • Hover chart to see tooltip with data point values (chromium)

These issues are not necessarily caused by your changes.
Annoyed by this section? Help fix flakies and failures and it will go green!

✅ ClickHouse migration SQL — none

No ClickHouse migrations in the latest push.

Generated-By: PostHog Desktop
Task-Id: d226f8d2-c9c7-454b-848e-88abdd2dff40
@github-project-automation github-project-automation Bot moved this from In Review to Approved in Feature Flags Aug 25, 2026
@trunk-io
trunk-io Bot merged commit 7c176f9 into master Aug 25, 2026
221 checks passed
@trunk-io
trunk-io Bot deleted the posthog-self-driving/fixcohorts-accept-bom-prefixed-csv-58af1b branch August 25, 2026 13:48
@github-project-automation github-project-automation Bot moved this from Approved to Done in Feature Flags Aug 25, 2026
@deployment-status-posthog

deployment-status-posthog Bot commented Aug 25, 2026 •

Copy link
Copy Markdown

Deploy status

Environment Status Deployed At Workflow
dev ✅ Deployed 2026-08-25 14:15 UTC Run
prod-us ✅ Deployed 2026-08-25 14:46 UTC Run
prod-eu ✅ Deployed 2026-08-25 14:52 UTC Run

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

Labels

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

1 participant