refactor(customer-analytics): collapse accounts id/external_id into the name column tuple#60680
Merged
Merged
Conversation
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
products/customer_analytics/backend/hogql_queries/accounts_query_runner.py:174-183
The `if NAME_COLUMN in self.columns:` guard is always `True` after `__init__` — either `name` was in the caller's `select` and was added to `self.columns` during the loop, or it was prepended unconditionally by the `if NAME_COLUMN not in seen:` block. The condition is a superfluous part that can be removed.
```suggestion
results = self.paginator.results
name_index = self.columns.index(NAME_COLUMN)
results = [
[
{"name": cell[0], "external_id": cell[1], "id": cell[2]} if index == name_index else cell
for index, cell in enumerate(row)
]
for row in results
]
```
### Issue 2 of 2
products/customer_analytics/backend/hogql_queries/test/test_accounts_query_runner.py:332-338
Each assertion now runs the query twice — `_ids()` executes a full DB query and then `_run_query()` re-executes the exact same query just to inspect `hasMore`. The helper already returns enough to assert both; sharing the single run avoids 2 redundant ClickHouse round-trips per page check.
```suggestion
page_one_runner, page_one_response = self._run_query(limit=2, offset=0)
name_idx = page_one_runner.columns.index("name")
self.assertEqual([row[name_idx]["id"] for row in page_one_response.results], expected_reverse[:2])
self.assertTrue(page_one_response.hasMore)
page_three_runner, page_three_response = self._run_query(limit=2, offset=4)
name_idx = page_three_runner.columns.index("name")
self.assertEqual([row[name_idx]["id"] for row in page_three_response.results], expected_reverse[4:])
self.assertFalse(page_three_response.hasMore)
```
Reviews (1): Last reviewed commit: "revert(data-table): Drop hidden-column i..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
|
Size Change: 0 B Total Size: 80.8 MB ℹ️ View Unchanged
|
arthurdedeus
force-pushed
the
posthog-code/accounts-name-tuple-refactor
branch
2 times, most recently
from
2af43fe to
f415490
Compare
2 tasks
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Clean refactor that collapses hidden id/external_id columns into a name tuple, matching the groups table pattern. The DataTable.tsx revert from resultIndex to index is safe because the only other hidden:true column (web analytics' ui_fill_fraction) is appended at the end of allColumns — it doesn't shift any visible column indices. All inline review comments were resolved in follow-up commits and tests cover the new tuple contract.
github-actions
Bot
dismissed
their stale review
New commits pushed (delta classified non_trivial_delta) — stamphog approval dismissed; re-review running automatically.
Contributor
There was a problem hiding this comment.
Clean refactor that removes the hidden pinned columns pattern and consolidates id/external_id into the name tuple, mirroring the groups table approach. The DataTable.tsx revert from resultIndex to index is safe because the only remaining hidden columns (web analytics' ui_fill_fraction and cross_sell) are appended at the end of allColumns, not prepended before visible ones — so index and resultIndex are equivalent for those cases. All review comments are resolved.
Contributor
|
🎭 Playwright report · View test results →
These issues are not necessarily caused by your changes. |
…he name column tuple Mirrors groups_query_runner's group_name pattern. Drops the hidden `context.columns.id` / `context.columns.external_id` pinned aliases on the frontend and the matching column index hacks. `name` is now mandatory in the column configurator (Remove disabled, Edit hidden) and self-heals when a saved config or `setSelectColumns` call omits it. Generated-By: PostHog Code Task-Id: 54831127-24c6-44b4-b61b-cc4415f2f927
No longer needed now that AccountsHogQLTable doesn't rely on hidden columns to carry id / external_id — `name` is a tuple instead. This reverts commit e48980f. Generated-By: PostHog Code Task-Id: 54831127-24c6-44b4-b61b-cc4415f2f927
…ry runner - Drop the always-true `NAME_COLUMN in self.columns` guard - Pagination test: assert hasMore + ids off the same query instead of running twice Generated-By: PostHog Code Task-Id: 54831127-24c6-44b4-b61b-cc4415f2f927
The `name` SELECT alias is now a tuple, so an unqualified `name ILIKE ...` in the WHERE clause resolved to the tuple and failed with `Illegal type Tuple(String, Nullable(String), String) of argument of function ilike`. Prefix with `accounts.` so it resolves to the table column. Generated-By: PostHog Code Task-Id: 6b5096f0-2f58-4c56-8e91-a0cf77f9d81b
arthurdedeus
force-pushed
the
posthog-code/accounts-name-tuple-refactor
branch
from
83c025d to
0ab32f1
Compare
github-actions
Bot
dismissed
their stale review
New commits pushed (delta classified non_linear_history) — stamphog approval dismissed; re-review running automatically.
Contributor
There was a problem hiding this comment.
Clean refactor collapsing hidden pinned columns into the name tuple, mirroring the groups table pattern. The DataTable.tsx revert from resultIndex to index is safe: the only prepended-hidden-column consumer was the accounts table (now removed), and while web analytics' ui_fill_fraction sits before cross_sell in the result, the cross_sell renderer ignores the passed value entirely and reads record[0] directly. All resolved review comments were applied in follow-up commits, migration safety is handled, and tests cover the new tuple contract.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The accounts table pinned
idandexternal_idas hiddencontext.columns.Xcolumns just so the Account cell and row-expand could find them.Changes
nameastuple(name, external_id, toString(id))and transform to{name, external_id, id}in the response, mirroringgroups_query_runner.group_nameACCOUNTS_HOGQL_PINNED_SELECTand thecontext.columns.Xcell-lookup hacks; cells read id / external_id off thenametuplenamemandatory in the column configurator (Remove disabled, Edit hidden) and self-heal saved configs that omit itast.Exprper select so aliased columns likeaccounts.tags.names AS tag_nameskeep resolvingHow did you test this code?
Unit tests (backend + frontend),
ruff,tsc. Human author verified the query end-to-end.👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Automatic notifications
Docs update
No
🤖 Agent context
Authored by PostHog Code.
Created with PostHog Code