Skip to content

trunk-merge/pr-108005/e50729e7-aea3-46a3-bd47-47bbefa42f08 - #108021

Closed
trunk-io[bot] wants to merge 99 commits into
masterfrom
trunk-merge/pr-108005/e50729e7-aea3-46a3-bd47-47bbefa42f08
Closed

trunk-io[bot] wants to merge 99 commits into
masterfrom
trunk-merge/pr-108005/e50729e7-aea3-46a3-bd47-47bbefa42f08

Conversation

@trunk-io

@trunk-io trunk-io Bot commented Sep 28, 2026

Copy link
Copy Markdown
Trunk Merge Pull Request Banner

This pull request was created and is being managed by Trunk Merge.

This pull request is based on the master branch at SHA e7c14387b61da5bc584ace3e554f7ee09416bcaa.

See more details here.

When CI completes, this pull request will be closed automatically.

Pull Requests Being Tested

This pull request is testing the changes from pull request 108005.

Dependencies

This pull request depends on the changes from pull requests 107975 and 107756.

posthog Bot and others added 30 commits September 14, 2026 11:37
…s manually

The manual "add users" list on the new-cohort page went silently blank whenever its person search query failed, and gave no way to see or remove someone already picked without scrolling back to them in the list.

- `PersonSelectList` now shows an error state with a retry button when the person search query fails, instead of the same "Search for persons to add" text used for a genuinely empty search.
- The new-cohort page now lists selected people as removable chips, independent of the (possibly failing or paginated) search results.
- `personsToCreateStaticCohort` stores each person's display name alongside their id, so the chip list can show a name instead of a raw id.

Branch: `posthog/cohorts-new-person-select-error-state`.

Generated-By: PostHog Desktop
Task-Id: b877c64f-81de-4e80-8518-afe9bb970a2a
Avoid re-instantiating dataNodeLogic twice, drop a no-op nullish coalescing, and use the `in` operator for the selected-persons key check.

Generated-By: PostHog Desktop
Task-Id: b877c64f-81de-4e80-8518-afe9bb970a2a
The wrapper around the new error state had a height but wasn't a flex container, so `InsightErrorState`'s own `flex-1` (which needs a flex parent to stretch) never took effect — the content sat at its natural height near the top of the box instead of filling and centering in it.

Generated-By: PostHog Desktop
Task-Id: b877c64f-81de-4e80-8518-afe9bb970a2a
Customers frame embeddable documents on their own sites, so the directive sent a report for every embed load. Chrome cuts the document URL of those reports to the origin, so they looked the same as blocked frames of app pages.
…gent

A customer triages "what is broken" by taking errored MCP sessions to a
coding agent. The Sessions tab had no outcome filter: the only route was
the shared $mcp_is_error property filter, which narrows events, so it
hid every successful call from the row counts and the session detail,
and "sessions without errors" was not expressible.

- Sessions list API gains has_errors (true/false/omitted) and returns
  error_calls per session, counted over the same matching calls as
  tool_calls. Cached per value. Exposed on the
  mcp-analytics-sessions-list MCP tool.
- Sessions tab: "All sessions / With errors / Without errors" select,
  synced to ?has_errors=, plus an error count on each session row.
- Session detail: "Copy errors for agent" reuses formatErrorContext, so
  telemetry stays inside indented blocks. errorType became optional
  because session tool calls do not carry it.

Tested:
- pytest test_api.py + test_presentation.py: 156 passed. The new
  has_errors case fails 2/3 with the HAVING clause removed.
- jest mcpSessionsLogic + errorContext: 19 passed. The deep-link case
  fails when has_errors is dropped from the request.
- tsgo clean, oxlint/oxfmt/ruff clean, OpenAPI + MCP codegen rerun.
- Storybook Sessions story: select sends has_errors=true once, copy
  button writes one header plus one section per errored call.
Pre-PR gate (Opus + Fable) findings applied:

- Copy errors for agent fetched only the loaded page of calls (100), so a
  long session could hide the button or copy fewer errors than its row
  showed. It now shows on the session's error_calls and pages through the
  tool-calls endpoint with an $mcp_is_error filter before copying.
- has_errors combined with a property filter was untested; two cases now
  fail if error_calls counts calls the shared filters exclude.
- error_calls help text says it counts shared-filter matches.
- Outcome select values are the tri-state as strings, decoded by
  parseUrlBoolean. The empty state counts shared property filters.
- CreateFixTaskButton requires errorType via MCPBucketedErrorContext.

Tested: pytest test_api + test_presentation 158 passed; jest
products/mcp_analytics/frontend 394 passed; tsgo clean; codegen rerun.
…filtering as a filter

CodeRabbit --deep findings: the copy action gave no feedback when a page
request failed, the empty state ignored filter_test_accounts, and the
errorContext test fixture was typed any.

Tested: jest products/mcp_analytics/frontend 394 passed; tsgo clean.
2 updated
Run: c793ebaf-f679-4278-b1e6-1425923cdb8e

Co-authored-by: lucasheriques <12522524+lucasheriques@users.noreply.github.com>
The session-errors copy button ships without the failing call's
$mcp_parameters or typed error fields, so an agent cannot reproduce a
failure from it. Split it out to iterate after an investigation and
dogfooding pass; the work is kept on lucas/mcp-session-copy-errors.
This PR keeps the has_errors filter and the per-row error counts.

Tested: jest products/mcp_analytics/frontend 392 passed; tsgo clean.
Greptile: an empty list under a narrowed date range said "No MCP sessions
yet". The "any filter active" check moves into a hasActiveFilters
selector that covers search, has_errors, the date range, and the shared
filters.

Tested: jest products/mcp_analytics/frontend 398 passed (the date case
fails without the date clause); tsgo clean.
The catalog names each table by its fully-qualified name, so a SQL-standard
`table_schema = 'system' AND table_name = 'insights'` filter matched no row.
When a bare table_name is not a visible table but `<schema>.<name>` is, the
tables and columns surfaces now report that table under the bare name.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Generated-By: PostHog Desktop
Task-Id: f20abd54-e3a7-4c25-8195-ac545b27dde6
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Generated-By: PostHog Desktop
Task-Id: f20abd54-e3a7-4c25-8195-ac545b27dde6
Resolve a bare table_name against the table's classified schema bucket rather
than its name prefix, so `public` + `ai_events` finds `posthog.ai_events` and
`information_schema` + `columns` finds `system.information_schema.columns`.
Skip a bare name that matches more than one table in the schema. Clarify both
supported filter forms in the execute-sql prompt.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Generated-By: PostHog Desktop
Task-Id: f20abd54-e3a7-4c25-8195-ac545b27dde6
CI's typegen:write + git diff check failed: the hasActiveFilters selector
was missing from the inline __keaTypeGenInternalSelectorTypes block.
Output of pnpm --filter=@posthog/frontend typegen:write, formatted.
2 updated
Run: c702b9e5-5789-451f-a18e-a8ebf375fd67

Co-authored-by: lucasheriques <12522524+lucasheriques@users.noreply.github.com>
4 updated
Run: d0ca3ae4-fb4e-49be-8c34-36fb77a45df0
Gilbert09 and others added 25 commits September 28, 2026 20:46
Google Analytics rejects a runReport request with 400 when a dimension or
metric name is invalid, e.g. a custom report using snake_case instead of
GA4's camelCase field names. The request never changes between retries, so
Temporal kept retrying it until the sync failed anyway.

Add "400 Client Error" for the GA4 Data API host to NonRetryableErrors,
alongside the existing 401/403 entries, with a message pointing at the
likely cause. Add a parameterized test covering the 400/401/403 shapes
`_run_report` raises via `raise_for_status()`.

Generated-By: PostHog Desktop
Task-Id: a749b992-953c-4985-aae3-950829e56efe
2 updated
Run: 0d3259f3-a20a-4dd5-a494-345deade68c0
@coderabbitai

coderabbitai Bot commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

📝 Walkthrough

Walkthrough

The marketing analytics source menu now uses shared notice logic and has new interaction stories and snapshots. Session recording template filter applicability now uses the presence of template filters. The enrichment scoring story waits for editor readiness and formula updates before completing its interaction.

Priority: ➖ Normal

Merge Risk: 🔵 Low · up to a083b

The menu-state implementation should be brought into line with the frontend guidance. No material user-facing failure is established, so this is a bounded merge concern.

Security Architecture Review

Security architecture risk: 🔵 Low · up to a083b

The reviewed changes primarily affect frontend notices and filter interactions. The shared menu retains its project-admin restriction, and no new security exposure was established. Server-side permission enforcement for the linked source-creation flow was not fully verified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The observed notice-state change is scoped to the current user's announcement visibility, while the source menu's project restriction is evaluated separately. No change to backend source-creation authority was established.

Trust Boundaries and Controls

  • observed — The frontend access check uses the current project's effective membership level; the source API lists create and setup among write-scoped actions. End-to-end server permission enforcement for those actions was not fully traced.
🚥 Pre-merge checks | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description only documents Trunk Merge execution, the tested PR, the base commit, and dependencies. It does not include the required Problem, Changes, How did you test this code?, Release status, … Replace or supplement the merge-queue text with a standalone description that follows the repository template. Explain the user problem, visible and mechanical changes, automated tests and untested areas, select exactly one release-status o…
Full details: Description check

Explanation

The description only documents Trunk Merge execution, the tested PR, the base commit, and dependencies. It does not include the required Problem, Changes, How did you test this code?, Release status, Automatic notifications, Docs update, or Agent context sections.

Resolution

Replace or supplement the merge-queue text with a standalone description that follows the repository template. Explain the user problem, visible and mechanical changes, automated tests and untested areas, select exactly one release-status option, address changelog and docs updates, and complete or remove the Agent context section as applicable. Keep the merge-queue details only as supporting information if needed.

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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

@coderabbitai coderabbitai 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.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: PostHog/posthog/.coderabbit.yaml

Review profile: QUIET

Plan: Enterprise

Run ID: b61a0fde-7a33-4fc0-9ec1-77cc7796aa1d

📥 Commits

Reviewing files that changed from the base of the PR and between e7c1438 and a083b45.

📒 Files selected for processing (12)
  • frontend/snapshots.yml
  • frontend/src/scenes/session-recordings/templates/sessionRecordingTemplatesLogic.test.ts
  • frontend/src/scenes/session-recordings/templates/sessionRecordingTemplatesLogic.tsx
  • frontend/src/scenes/web-analytics/tabs/marketing-analytics/frontend/components/MarketingAnalyticsFilters/AddIntegrationButton.tsx
  • frontend/src/scenes/web-analytics/tabs/marketing-analytics/frontend/components/MarketingAnalyticsFilters/MarketingAnalyticsFilters.tsx
  • frontend/src/scenes/web-analytics/tabs/marketing-analytics/frontend/components/settings/ExternalDataSourceConfiguration.tsx
  • products/growth/frontend/aiEnrichment/EnrichmentScoring.stories.tsx
  • products/marketing_analytics/frontend/components/AddIntegrationButton.stories.tsx
  • products/marketing_analytics/frontend/components/AddIntegrationButton.tsx
  • products/marketing_analytics/frontend/components/newAdSourcesLogic.test.ts
  • products/marketing_analytics/frontend/components/newAdSourcesLogic.ts
  • tsconfig.json
💤 Files with no reviewable changes (1)
  • frontend/src/scenes/web-analytics/tabs/marketing-analytics/frontend/components/MarketingAnalyticsFilters/AddIntegrationButton.tsx

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 0 remain after this review.

Comment on lines +34 to +35
const [showPopover, setShowPopover] = useState(false)
const [showNoticeInMenu, setShowNoticeInMenu] = useState(false)

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.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

🔎 Supported by static analysis

🏁 Script executed:

sed -n '1,145p' products/marketing_analytics/frontend/components/AddIntegrationButton.tsx
rg -n 'Don.t use `useState`|Always implement data layer in Kea|local state|keyed logic' frontend/src/AGENTS.md products/marketing_analytics frontend/.cursor products/.cursor 2>/dev/null | head -65

Repository: PostHog/posthog

Length of output: 5407


🏁 Script executed:

set -e
printf '%s\n' '--- logic definition ---'
sed -n '1,220p' products/marketing_analytics/frontend/components/newAdSourcesLogic.ts 2>/dev/null || true
printf '%s\n' '--- component usages ---'
rg -n -C 3 'AddIntegrationButton' products/marketing_analytics frontend/src
printf '%s\n' '--- logic usages ---'
rg -n -C 3 'newAdSourcesLogic' products/marketing_analytics/frontend

Repository: PostHog/posthog

Length of output: 25295


🏁 Script executed:

set -e
printf '%s\n' '--- logic definition ---'
fd -i 'newAdSourcesLogic.ts' products frontend | xargs -r -n1 sh -c 'echo "--- $0"; sed -n "1,220p" "$0"'
printf '%s\n' '--- component usages ---'
rg -n -C 4 'AddIntegrationButton' products/marketing_analytics frontend/src
printf '%s\n' '--- logic usages ---'
rg -n -C 4 'newAdSourcesLogic' products/marketing_analytics/frontend

Repository: PostHog/posthog

Length of output: 29655


🏁 Script executed:

python3 - <<'PY'
from pathlib import Path
for p in Path("products/marketing_analytics").rglob("*"):
    if p.name == "newAdSourcesLogic.ts":
        print(f"--- {p} ---")
        print("".join(p.read_text().splitlines(True)[:220]))
print("--- AddIntegrationButton call sites ---")
for root in (Path("products/marketing_analytics"), Path("frontend/src")):
    for p in root.rglob("*"):
        if p.suffix in {".ts", ".tsx"} and p.is_file():
            text = p.read_text(errors="replace")
            if "AddIntegrationButton" in text:
                print(f"--- {p} ---")
                lines = text.splitlines()
                for i, line in enumerate(lines):
                    if "AddIntegrationButton" in line:
                        lo, hi = max(0, i-4), min(len(lines), i+8)
                        print("\n".join(f"{j+1}: {lines[j]}" for j in range(lo, hi)))
PY

Repository: PostHog/posthog

Length of output: 10654


Move the menu state into a per-button Kea logic.

The frontend guidance does not exempt transient menu state from its useState rule. Move showPopover and showNoticeInMenu out of React state.

Keep newAdSourcesLogic global for the shared new-source notice and dismissal. Do not add the menu values to that logic. The component renders twice in the story, so the new menu logic must be keyed per button instance. Otherwise, one button's visibility state can affect the other.

@trunk-io trunk-io Bot closed this Sep 28, 2026
@trunk-io
trunk-io Bot deleted the trunk-merge/pr-108005/e50729e7-aea3-46a3-bd47-47bbefa42f08 branch September 28, 2026 21:27
@trunk-io

trunk-io Bot commented Sep 28, 2026 •

Copy link
Copy Markdown
Author

Static Badge   Static Badge   Static Badge

Failed Test Failure Summary Logs
Scenes-App/SidePanels SidePanelNotebooks smoke-test The test timed out while waiting for a loading indicator or spinner to disappear. Logs ↗︎

View Full Report ↗︎ ⋅ Docs

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants