feat(customer-analytics): add default pinned account properties - #108076
arthurdedeus wants to merge 3 commits into
Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
Game on, robots! @veria-ai @greptile-apps @parameterai |
|
Took a look at the diff — the new |
🤖 CI report
|
| Function | Location | Complexity | Limit |
|---|---|---|---|
errors |
products/customer_analytics/frontend/scenes/CustomerAnalyticsConfigurationScene/account/customPropertyDefinitionsLogic.ts:950 |
18 | 10 |
submit |
products/customer_analytics/frontend/scenes/CustomerAnalyticsConfigurationScene/account/customPropertyDefinitionsLogic.ts:984 |
17 | 10 |
openEditModal |
products/customer_analytics/frontend/scenes/CustomerAnalyticsConfigurationScene/account/customPropertyDefinitionsLogic.ts:1311 |
15 | 10 |
DefaultPinnedAccountProperties |
products/customer_analytics/frontend/scenes/CustomerAnalyticsConfigurationScene/account/DefaultPinnedAccountProperties.tsx:12 |
14 | 10 |
loadSelectedTableColumns |
products/customer_analytics/frontend/scenes/CustomerAnalyticsConfigurationScene/account/customPropertyDefinitionsLogic.ts:845 |
12 | 10 |
✅ Duplication (Python) — clean
New Python code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.
✅ Duplication (TypeScript) — clean
New TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.
⚠️ Bundle size — 🔺 +9.5 KiB (+0.0%)
Uncompressed size of every built .js bundle, compared against the base branch.
Total: 68.88 MiB · 🔺 +9.5 KiB (+0.0%)
| File | Size | Δ vs base |
|---|---|---|
posthog-app/_parent/products/signals/frontend/inbox/InboxScene.js |
474.9 KiB | 🔺 +7.4 KiB (+1.6%) |
render-query/src/render-query/render-query.js |
20.16 MiB | 🔺 +1.5 KiB (+0.0%) |
Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report
✅ Eager graph — within budget
How much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy import() / React.lazy chunks are not counted.
| Root | Eager (shipped) | Δ vs base | Budget |
|---|---|---|---|
entry (logged-out pages, app bootstrap)src/index.tsx |
1.57 MiB · 22 files | 🔺 +33 B (+0.0%) | █████████░ 85.5% of 1.84 MiB |
logged-out boot: index + App + bootApp (preloaded by every page, including /login)src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts |
3.51 MiB · 629 files | 🔺 +200 B (+0.0%) | █████████░ 87.2% of 4.03 MiB |
authenticated shell (every logged-in page)src/scenes/AuthenticatedShell.tsx |
7.33 MiB · 2,332 files | 🔺 +1.3 KiB (+0.0%) | █████████░ 87.9% of 8.34 MiB |
🟢 node_modules/monaco-editor/ stays out of src/index.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 node_modules/monaco-editor/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/layout/navigation-3000/navigationLogic.tsx stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/scenes/dashboard/dashboardLogic.tsx stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/lib/lemon-ui/LemonMarkdown/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/lib/components/RichContentEditor/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/lib/components/CodeSnippet/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/taxonomy/core-filter-definitions-by-group.json stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 node_modules/monaco-editor/ stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/scenes/session-recordings/player/sessionRecordingPlayerLogic.ts stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
Largest files eagerly shipped from src/index.tsx
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/index.tsx |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 854 B | src/scenes/ChunkLoadErrorBoundary.tsx |
Largest files eagerly shipped from src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
| Size | File |
|---|---|
| 301.8 KiB | ../node_modules/.pnpm/posthog-js@1.434.14_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 216.0 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 100.5 KiB | src/lib/api.ts |
| 88.4 KiB | src/products.tsx |
| 69.4 KiB | src/lib/lemon-ui/icons/icons.tsx |
| 40.1 KiB | src/lib/utils/eventUsageLogic.ts |
| 38.7 KiB | ../node_modules/.pnpm/@dnd-kit+core@6.0.8_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@dnd-kit/core/dist/core.esm.js |
| 33.9 KiB | ../node_modules/.pnpm/kea@4.0.0-pre.6_patch_hash=139b8d1f1304f9d9da452a9a1244c94ea679dbcb85687d8999563146879fb6f5_react@18.3.1/node_modules/kea/lib/index.cjs.js |
| 28.4 KiB | src/scenes/scenes.ts |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 301.8 KiB | ../node_modules/.pnpm/posthog-js@1.434.14_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 271.7 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 216.0 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 153.7 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 100.5 KiB | src/lib/api.ts |
| 98.5 KiB | ../packages/quill/packages/quill/dist/index.js |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
| 88.4 KiB | src/products.tsx |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
✅ Toolbar bundle — eager 2.16 MiB within budget
What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.
| Metric | Size | Δ vs base | Budget |
|---|---|---|---|
| Eager (shipped) entry + static imports |
2.16 MiB · 19 files | 🔺 +91 B (+0.0%) | ████░░░░░░ 37.7% of 5.72 MiB |
| Deferred (lazy) | 2.10 MiB · 44 files | no change | n/a — loads on demand |
Loader dist/toolbar.js |
1.2 KiB | no change | █░░░░░░░░░ 6.0% of 19.5 KiB |
Largest eagerly-shipped chunks
| Size | File |
|---|---|
| 800.5 KiB | dist/toolbar/toolbar-app-QUJ43CJ4.css |
| 651.6 KiB | dist/toolbar/chunk-chunk-ZPCK2O6G.js |
| 259.4 KiB | dist/toolbar/chunk-chunk-CV2VU6SQ.js |
| 138.3 KiB | dist/toolbar/chunk-chunk-DYPTRYMF.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-FDH2IBXT.js |
| 75.2 KiB | dist/toolbar/toolbar-app-4HYNQ5KU.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-TSAL54PB.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-HOKNU4ZL.js |
| 21.0 KiB | dist/toolbar/chunk-chunk-Z5ELNJKM.js |
| 6.8 KiB | dist/toolbar/chunk-chunk-DV7IWQNF.js |
Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile
✅ Dist folder size — 🔺 +249.8 KiB (+0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 946.31 MiB · 🔺 +249.8 KiB (+0.0%)
ℹ️ MCP UI apps size — 33 app(s), 17631.0 KB JS
Built size of each MCP UI app (main.js + styles.css).
| App | JS | CSS |
|---|---|---|
| debug | 597.9 KB | 196.2 KB |
| action | 454.1 KB | 196.2 KB |
| action-list | 564.2 KB | 196.2 KB |
| cohort | 453.1 KB | 196.2 KB |
| cohort-list | 563.2 KB | 196.2 KB |
| email-template | 452.9 KB | 196.2 KB |
| error-details | 469.6 KB | 196.2 KB |
| error-issue | 453.8 KB | 196.2 KB |
| error-issue-list | 564.1 KB | 196.2 KB |
| experiment | 561.3 KB | 196.2 KB |
| experiment-list | 564.9 KB | 196.2 KB |
| experiment-results | 566.3 KB | 196.2 KB |
| feature-flag | 566.8 KB | 196.2 KB |
| feature-flag-list | 570.5 KB | 196.2 KB |
| feature-flag-testing | 457.3 KB | 196.2 KB |
| inline-scan | 453.6 KB | 196.2 KB |
| insight-actors | 562.3 KB | 196.2 KB |
| invite-email-preview | 452.3 KB | 196.2 KB |
| llm-costs | 559.3 KB | 196.2 KB |
| session-recording | 455.3 KB | 196.2 KB |
| survey | 454.7 KB | 196.2 KB |
| survey-global-stats | 561.9 KB | 196.2 KB |
| survey-list | 564.9 KB | 196.2 KB |
| survey-stats | 561.9 KB | 196.2 KB |
| trace-span | 453.5 KB | 196.2 KB |
| trace-span-list | 564.1 KB | 196.2 KB |
| vision-observation-list | 563.3 KB | 196.2 KB |
| workflow | 453.4 KB | 196.2 KB |
| workflow-list | 563.5 KB | 196.2 KB |
| loops-review | 457.8 KB | 196.2 KB |
| query-results | 774.1 KB | 196.2 KB |
| render-ui | 857.4 KB | 196.2 KB |
| visual-review-snapshots | 457.9 KB | 196.2 KB |
⚠️ MCP snapshots — 1 updated (1 modified, 0 added, 0 deleted)
Snapshots: MCP unit test snapshots updated
Changes: 1 snapshots (1 modified, 0 added, 0 deleted)
What this means:
- Snapshots have been automatically updated to match current output
Next steps:
- Review the changes to ensure they're intentional
- If unexpected, investigate what caused the output to change
⚠️ Django migration SQL — 2 new migrations to review
We've detected new migrations on this PR. Review the SQL output for each migration:
products/customer_analytics/backend/migrations/0059_teamcustomeranalyticsconfig_default_pinned_properties.py
/opt/hostedtoolcache/Python/3.14.7/x64/lib/python3.14/site-packages/anyio/from_thread.py:119: SyntaxWarning: 'return' in a 'finally' block
return result
/opt/hostedtoolcache/Python/3.14.7/x64/lib/python3.14/site-packages/structlog/stdlib.py:1166: UserWarning: Remove `format_exc_info` from your processor chain if you want pretty exceptions.
ed = p(logger, meth_name, ed) # type: ignore[arg-type]
2026-09-28T23:44:52.831402Z [error ] Path must be a valid database or directory containing databases. [posthog.exceptions_capture] pid=8032 tid=140669969460096
Traceback (most recent call last):
File "/home/runner/work/posthog/posthog/posthog/geoip.py", line 15, in <module>
geoip: Optional[GeoIP2] = GeoIP2(cache=8)
~~~~~~^^^^^^^^^
File "/opt/hostedtoolcache/Python/3.14.7/x64/lib/python3.14/site-packages/django/contrib/gis/geoip2.py", line 116, in __init__
raise GeoIP2Exception(
"Path must be a valid database or directory containing databases."
)
django.contrib.gis.geoip2.GeoIP2Exception: Path must be a valid database or directory containing databases.
/opt/hostedtoolcache/Python/3.14.7/x64/lib/python3.14/site-packages/sshtunnel.py:1040: SyntaxWarning: 'return' in a 'finally' block
return (ssh_host,
/opt/hostedtoolcache/Python/3.14.7/x64/lib/python3.14/site-packages/langchain_core/_api/deprecation.py:27: UserWarning: Core Pydantic V1 functionality isn't compatible with Python 3.14 or greater.
from pydantic.v1.fields import FieldInfo as FieldInfoV1
System check identified some issues:
WARNINGS:
?: (axes.W001) You are using the django-axes cache handler for login attempt tracking. Your cache configuration is however invalid and will not work correctly with django-axes. This can leave security holes in your login systems as attempts are not tracked correctly. Reconfigure settings.AXES_CACHE and settings.CACHES per django-axes configuration documentation.
?: (staticfiles.W004) The directory '/home/runner/work/posthog/posthog/frontend/dist' in the STATICFILES_DIRS setting does not exist.
BEGIN;
--
-- Add field default_pinned_properties to teamcustomeranalyticsconfig
--
ALTER TABLE "customer_analytics_teamcustomeranalyticsconfig" ADD COLUMN "default_pinned_properties" jsonb DEFAULT '[]'::jsonb NOT NULL;
ALTER TABLE "customer_analytics_teamcustomeranalyticsconfig" ALTER COLUMN "default_pinned_properties" DROP DEFAULT;
COMMIT;products/customer_analytics/backend/migrations/0060_inherit_default_pinned_properties.py
/opt/hostedtoolcache/Python/3.14.7/x64/lib/python3.14/site-packages/structlog/stdlib.py:1166: UserWarning: Remove `format_exc_info` from your processor chain if you want pretty exceptions.
ed = p(logger, meth_name, ed) # type: ignore[arg-type]
2026-09-28T23:45:19.046127Z [error ] Path must be a valid database or directory containing databases. [posthog.exceptions_capture] pid=8897 tid=140426605357952
Traceback (most recent call last):
File "/home/runner/work/posthog/posthog/posthog/geoip.py", line 15, in <module>
geoip: Optional[GeoIP2] = GeoIP2(cache=8)
~~~~~~^^^^^^^^^
File "/opt/hostedtoolcache/Python/3.14.7/x64/lib/python3.14/site-packages/django/contrib/gis/geoip2.py", line 116, in __init__
raise GeoIP2Exception(
"Path must be a valid database or directory containing databases."
)
django.contrib.gis.geoip2.GeoIP2Exception: Path must be a valid database or directory containing databases.
/opt/hostedtoolcache/Python/3.14.7/x64/lib/python3.14/site-packages/langchain_core/_api/deprecation.py:27: UserWarning: Core Pydantic V1 functionality isn't compatible with Python 3.14 or greater.
from pydantic.v1.fields import FieldInfo as FieldInfoV1
System check identified some issues:
WARNINGS:
?: (axes.W001) You are using the django-axes cache handler for login attempt tracking. Your cache configuration is however invalid and will not work correctly with django-axes. This can leave security holes in your login systems as attempts are not tracked correctly. Reconfigure settings.AXES_CACHE and settings.CACHES per django-axes configuration documentation.
?: (staticfiles.W004) The directory '/home/runner/work/posthog/posthog/frontend/dist' in the STATICFILES_DIRS setting does not exist.
BEGIN;
--
-- Raw SQL operation
--
UPDATE customer_analytics_usercustomeranalyticsconfig
SET properties = properties - 'pinned_properties'
WHERE properties -> 'pinned_properties' = '[]'::jsonb
AND cardinality(pinned_custom_property_definition_ids) = 0
;
COMMIT;Last updated: 2026-09-28 23:45 UTC (eed080b)
❌ Django migration risk — blocked migration detected
We've analyzed your migrations for potential risks.
Summary: 0 Safe | 1 Needs Review | 1 Blocked
❌ Blocked
Causes locks or breaks compatibility
customer_analytics.0060_inherit_default_pinned_properties
└─ #1 ❌ RunSQL: RunSQL with UPDATE/DELETE needs careful review for locking
⚠️ Needs Review
May have performance impact
customer_analytics.0059_teamcustomeranalyticsconfig_default_pinned_properties
└─ #1 ⚠️ AddField
Adding NOT NULL field with callable default (list) - verify it's stable
model: teamcustomeranalyticsconfig, field: default_pinned_properties, default: list
📚 How to Deploy These Changes Safely
RunSQL:
Break large updates into batches to avoid long locks:
- Batch size: 1,000-10,000 rows per batch
- Add pauses between batches
- Use WHERE clauses to limit scope
- Consider background jobs for very large updates (millions of rows)
See the migration safety guide
Last updated: 2026-09-28 23:45 UTC (eed080b)
|
[High risk] Adds database schema and API for default pinned account properties. The PR is not ready to merge until deployment-overlap inheritance and paginated relationship loading are addressed. Reviews (1) · Last reviewed commit: "chore(customer-analytics): add default p..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| sql=""" | ||
| UPDATE customer_analytics_usercustomeranalyticsconfig | ||
| SET properties = properties - 'pinned_properties' | ||
| WHERE properties -> 'pinned_properties' = '[]'::jsonb | ||
| AND cardinality(pinned_custom_property_definition_ids) = 0 | ||
| """, |
There was a problem hiding this comment.
pinned_properties: []. The new read path treats that list as a personal override, so the user will not inherit project defaults. Make normalization safe for rows created during the deployment overlap.
Prompt To Fix With AI
This is a comment left during a code review.
Path: products/customer_analytics/backend/migrations/0060_inherit_default_pinned_properties.py
Line: 14-19
Comment:
**Empty rows miss inheritance** If an old application instance creates a user configuration after this migration runs, it can still write `pinned_properties: []`. The new read path treats that list as a personal override, so the user will not inherit project defaults. Make normalization safe for rows created during the deployment overlap.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| relationshipDefinitionsLogic, | ||
| [ | ||
| 'definitions as relationshipDefinitions', | ||
| 'definitionsLoading as relationshipDefinitionsLoading', | ||
| 'definitionsLoadFailed as relationshipDefinitionsLoadFailed', | ||
| ], |
There was a problem hiding this comment.
Prompt To Fix With AI
This is a comment left during a code review.
Path: products/customer_analytics/frontend/scenes/CustomerAnalyticsConfigurationScene/account/defaultPinnedAccountPropertiesLogic.ts
Line: 147-152
Comment:
**Later relationships disappear** When a project has more relationships than fit on the first API page, the connected loader provides only that page. Existing pins from later pages appear unavailable, and saving the draft can remove them. Load all relationship pages before resolving or editing defaults.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| migrations.RunSQL( | ||
| sql=""" | ||
| UPDATE customer_analytics_usercustomeranalyticsconfig | ||
| SET properties = properties - 'pinned_properties' | ||
| WHERE properties -> 'pinned_properties' = '[]'::jsonb | ||
| AND cardinality(pinned_custom_property_definition_ids) = 0 | ||
| """, |
There was a problem hiding this comment.
Prompt To Fix With AI
This is a comment left during a code review.
Path: products/customer_analytics/backend/migrations/0060_inherit_default_pinned_properties.py
Line: 13-19
Comment:
**Unbounded migration update** On a populated user-configuration table, this single transactional UPDATE can hold row locks and delay configuration writes while it scans and rewrites historical rows. Batch the normalization or otherwise bound its lock and transaction impact.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughThe change adds ordered project defaults for pinned account properties and relationships. The team API validates and stores these defaults, and user reads return them when no personal pin list is stored. Migrations remove legacy empty pin lists to enable inheritance. An admin interface lets project admins configure defaults, reorder pins, and save an empty list. Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to This change adds project-default pinned properties. Before merging, resolve the blocked migration and its interaction with rolling deploys. Also fix the partial writes on invalid input, the possible loss of valid relationship defaults when saving, and child-environment defaults that users may never see. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Project admins gain shared defaults, but the migration can turn some users’ previously empty personal selections into inherited defaults. The change has a bounded permission model, while that preference change and its rollback remain material design risks. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (3)
posthog/api/team.py-1142-1143 (1)
1142-1143: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winBind customer analytics validation before other config writes.
When a PATCH includes valid revenue or marketing settings and invalid default pins,
TeamSerializer.updatewrites the earlier settings before_update_customer_analytics_configraises a validation error. The project endpoint does not add an atomic wrapper, andATOMIC_REQUESTSis disabled. The API can therefore return HTTP 400 after persisting part of the request.Bind the nested serializer to the existing customer analytics config during outer validation:
🐛 Suggested fix
- @staticmethod - def validate_customer_analytics_config(value): + def validate_customer_analytics_config(self, value): if value is None: return None - serializer = TeamCustomerAnalyticsConfigSerializer(data=value) + instance = self.instance.customer_analytics_config if self.instance is not None else None + serializer = TeamCustomerAnalyticsConfigSerializer(instance=instance, data=value) if not serializer.is_valid(): raise exceptions.ValidationError(_format_serializer_errors(serializer.errors)) return serializer.validated_dataproducts/customer_analytics/backend/migrations/0060_inherit_default_pinned_properties.py-14-19 (1)
14-19: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winThe migration risk check is blocking this
RunSQLUPDATE.The
UPDATEscanscustomer_analytics_usercustomeranalyticsconfigand rewrites the matching rows in one transaction. This table has one row per user per project, so the row count grows with both. Take one of these actions to pass the check:
- Confirm that the table is small and record the size in a comment that the check accepts.
- Run the update in batches by primary key.
- Move the update to a background job.
Source: Pipeline failures
products/customer_analytics/backend/logic/user_customer_analytics_config.py-72-77 (1)
72-77: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winWrite customer analytics defaults to the canonical team row.
get_or_create_configcanonicalizes child team IDs, soread_pinned_propertiesreads the parent team's defaults.TeamSerializer._update_customer_analytics_configsavesinstance.customer_analytics_configdirectly. A PATCH for a child environment can therefore write the child row, while reads use the parent row. Resolve the canonical team before the write or before the default read.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 4a46d752-d5fa-401b-9528-63b9ecb821b8
⛔ Files ignored due to path filters (5)
frontend/src/generated/core/api.schemas.tsis excluded by!**/generated/**products/customer_analytics/frontend/generated/api.schemas.tsis excluded by!**/generated/**products/customer_analytics/frontend/generated/api.tsis excluded by!**/generated/**products/experiments/frontend/generated/api.schemas.tsis excluded by!**/generated/**services/mcp/src/generated/core/api.tsis excluded by!**/generated/**
📒 Files selected for processing (33)
docs/internal/customer-analytics/accounts-table.mdfrontend/src/queries/schema.jsonfrontend/src/queries/schema/schema-general.tsposthog/api/team.pyposthog/api/test/test_project.pyposthog/schema.pyposthog/schema_enums.pyproducts/customer_analytics/backend/facade/account_property_pins.pyproducts/customer_analytics/backend/facade/api.pyproducts/customer_analytics/backend/facade/contracts.pyproducts/customer_analytics/backend/facade/enums.pyproducts/customer_analytics/backend/logic/account_property_pins.pyproducts/customer_analytics/backend/logic/user_customer_analytics_config.pyproducts/customer_analytics/backend/migrations/0059_teamcustomeranalyticsconfig_default_pinned_properties.pyproducts/customer_analytics/backend/migrations/0060_inherit_default_pinned_properties.pyproducts/customer_analytics/backend/migrations/max_migration.txtproducts/customer_analytics/backend/models/team_customer_analytics_config.pyproducts/customer_analytics/backend/presentation/views/serializers.pyproducts/customer_analytics/backend/presentation/views/views.pyproducts/customer_analytics/backend/test/test_default_pinned_properties_migration.pyproducts/customer_analytics/backend/test/test_user_customer_analytics_config_api.pyproducts/customer_analytics/frontend/components/Accounts/AGENTS.mdproducts/customer_analytics/frontend/scenes/CustomerAnalyticsAccountScene/components/AccountPropertyConfigurator.tsxproducts/customer_analytics/frontend/scenes/CustomerAnalyticsConfigurationScene/account/CustomerAnalyticsAccountConfig.tsxproducts/customer_analytics/frontend/scenes/CustomerAnalyticsConfigurationScene/account/DefaultPinnedAccountProperties.stories.tsxproducts/customer_analytics/frontend/scenes/CustomerAnalyticsConfigurationScene/account/DefaultPinnedAccountProperties.test.tsxproducts/customer_analytics/frontend/scenes/CustomerAnalyticsConfigurationScene/account/DefaultPinnedAccountProperties.tsxproducts/customer_analytics/frontend/scenes/CustomerAnalyticsConfigurationScene/account/customPropertyDefinitionsLogic.tsproducts/customer_analytics/frontend/scenes/CustomerAnalyticsConfigurationScene/account/defaultPinnedAccountPropertiesLogic.test.tsproducts/customer_analytics/frontend/scenes/CustomerAnalyticsConfigurationScene/account/defaultPinnedAccountPropertiesLogic.tsproducts/customer_analytics/frontend/scenes/CustomerAnalyticsConfigurationScene/account/relationshipDefinitionsLogic.tsservices/mcp/src/api/generated.tsservices/mcp/tests/unit/__snapshots__/tool-schemas/project-settings-update.json
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
| class AccountPropertyPinKind(str, Enum): | ||
| CUSTOM_PROPERTY = "custom_property" | ||
| RELATIONSHIP = "relationship" | ||
| class AccountPropertyPinKind(models.TextChoices): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Keep Django choices out of the facade enum.
AccountPropertyPinKind now depends on models.TextChoices. Keep this enum as a standard-library string enum and construct Django or DRF choices at the API boundary. As per coding guidelines, facade enums.py files allow “No Django imports (from django.*).”
Source: Coding guidelines
| migrations.RunSQL( | ||
| sql=""" | ||
| UPDATE customer_analytics_usercustomeranalyticsconfig | ||
| SET properties = properties - 'pinned_properties' | ||
| WHERE properties -> 'pinned_properties' = '[]'::jsonb | ||
| AND cardinality(pinned_custom_property_definition_ids) = 0 | ||
| """, | ||
| reverse_sql=migrations.RunSQL.noop, | ||
| ), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Old application servers can undo this migration during a rolling deploy.
Migrations run before all old servers stop. The old get_or_create_config writes {"pinned_properties": []} in two cases: when it creates a row through defaults, and when a row has no legacy IDs. Each GET that an old server handles after this UPDATE adds the empty key back. Those users then have an explicit empty override and do not inherit project defaults. The PR already accepts some data loss here, and this timing gap adds more.
Fix: move this cleanup to a follow-up migration that ships after the code in this PR is fully deployed. The new code no longer writes the empty key. As an alternative, run the cleanup again as a post-deploy step.
| resolvedPinnedPropertyKeys: [ | ||
| (s) => [s.defaultPinnedProperties, s.propertyOptions], | ||
| (references: PinnedAccountPropertyApi[], options: AccountPropertyOption[]): string[] => { | ||
| const availableKeys = new Set(options.map(({ key }) => key)) | ||
| return references.map(pinnedPropertyToConfiguratorKey).filter((key) => availableKeys.has(key)) | ||
| }, | ||
| ], | ||
| stalePinnedProperties: [ | ||
| (s) => [s.defaultPinnedProperties, s.propertyOptions], | ||
| (references: PinnedAccountPropertyApi[], options: AccountPropertyOption[]): PinnedAccountPropertyApi[] => { | ||
| const availableKeys = new Set(options.map(({ key }) => key)) | ||
| return references.filter((reference) => !availableKeys.has(pinnedPropertyToConfiguratorKey(reference))) | ||
| }, | ||
| ], |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
rg -n -C6 'loadDefinitions:\s*async' products/customer_analytics/frontend/scenes/CustomerAnalyticsConfigurationScene/account/
rg -n -C3 'page_size|PAGE_SIZE|pagination_class' products/customer_analytics/backend/presentation/views/views.py | head -60Repository: PostHog/posthog
Length of output: 7477
Paginate relationship definitions before resolving pinned properties.
relationshipDefinitionsLogic.loadDefinitions returns only response.results from accountRelationshipDefinitionsList. If the API response is paginated, a default that points to a later-page relationship is classified as stale, omitted from the configurator draft, and removed on the next save. Make the relationship loader fetch all definition pages before using its results to classify defaults.
|
|
This branch has not been deployed
1 participant
Problem
Project admins cannot choose the account properties people see before saving a personal sidebar selection.
Every user starts with an empty account sidebar and must configure the same useful properties independently.
Changes
Warning
Existing data cannot distinguish an auto-created empty selection from an intentional clear. Those users may need to clear the defaults once again.
Before this change, Accounts settings had no default-pins control. The configured state now shows the ordered project defaults:
How did you test this code?
pnpm --filter=@posthog/frontend typescript:checkuv run mypy --cache-fine-grained .hogli product:lint customer_analyticshogli ci:preflight --strict👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Automatic notifications
Docs update
Updated the Customer analytics Accounts table guide and generated API descriptions. The Inkeep docs agent can assess published documentation changes.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: pi,
gpt-5.6-sol/django-migrations,/improving-drf-endpoints,/writing-tests,/writing-ui-components,/writing-user-facing-copy,/writing-kea-logics,/editing-agents-md,/running-ci-preflight,/write-snapshot-test,/setting-feature-flags-in-storybook,/writing-pr-descriptions, and/ship-it.