fix(feature-flags): show all projects' flags in comparison grid#63156
Draft
phillram wants to merge 1 commit into
Draft
fix(feature-flags): show all projects' flags in comparison grid#63156phillram wants to merge 1 commit into
phillram wants to merge 1 commit into
Conversation
The list-level "Projects" comparison grid drove its rows from the current project's feature flags only, so it could never show flags that exist in the compared projects but not the active one — contradicting its own "across your organization's projects" promise. Add an internal, paginated `keys` endpoint on the organization feature flag viewset that returns the distinct flag keys across the selected projects (with a representative flag per key for the row name and link), and point the grid's row loader at it instead of the project-scoped flag list. Rows now reload from the top when the set of compared projects changes. Generated-By: PostHog Code Task-Id: f60ed8c7-e8e8-4d7f-8e92-6be2a999df87
Contributor
|
Size Change: 0 B Total Size: 72.4 MB ℹ️ View Unchanged
|
Contributor
🕸️ Eager graphHow much code each root forces the browser to download and decode through static imports — the regression class total bundle size can't see.
✅ Largest files eagerly reachable from
|
| Size | File |
|---|---|
| 878.2 KiB | src/styles/global.scss |
| 609.0 KiB | public/hedgehog/burning-money-hog.png |
| 541.9 KiB | public/hedgehog/waving-hog.png |
| 448.2 KiB | public/hedgehog/stop-sign-hog.png |
| 362.0 KiB | public/hedgehog/phone-pair-hogs.png |
| 354.8 KiB | ../node_modules/.pnpm/@posthog+icons@0.36.6_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 335.6 KiB | public/hedgehog/desk-hog.png |
| 323.2 KiB | public/hedgehog/3-bears-hogs.png |
| 297.4 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 286.4 KiB | src/lib/api.ts |
Largest files eagerly reachable from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 878.2 KiB | src/styles/global.scss |
| 760.0 KiB | src/queries/validators.js |
| 609.0 KiB | public/hedgehog/burning-money-hog.png |
| 541.9 KiB | public/hedgehog/waving-hog.png |
| 448.2 KiB | public/hedgehog/stop-sign-hog.png |
| 398.7 KiB | ../node_modules/.pnpm/chart.js@4.5.1/node_modules/chart.js/dist/chart.js |
| 362.0 KiB | public/hedgehog/phone-pair-hogs.png |
| 354.8 KiB | ../node_modules/.pnpm/@posthog+icons@0.36.6_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 335.6 KiB | public/hedgehog/desk-hog.png |
| 323.2 KiB | public/hedgehog/3-bears-hogs.png |
Posted automatically by check-eager-graph · sizes are input-source bytes from the esbuild metafile · part of #32479
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/feature_flags/backend/api/organization_feature_flag.py:204-214
**Concurrent-deletion `KeyError` causes HTTP 500.** Between the `distinct_keys` slice that populates `page_keys` and the `page_flags` query that fills `representatives`, a flag can be soft-deleted. Any key that survives the first query but loses all its live flags before the second query will be absent from `representatives`, so `representatives[key]` raises an unhandled `KeyError` and the endpoint returns 500 instead of a valid (shorter) page.
```suggestion
results = [
{
"key": flag.key,
"name": flag.name or "",
"flag_id": flag.id,
"team_id": flag.team_id,
"filters": flag.get_filters(),
"active": flag.active,
}
for key in page_keys
if (flag := representatives.get(key)) is not None
]
```
### Issue 2 of 2
products/feature_flags/backend/api/test/test_organization_feature_flag.py:136-212
**Non-parameterised tests preferred.** Several test methods in `TestOrganizationFeatureFlagKeys` cover closely related variations of the same scenario (e.g. `test_team_ids_filters_to_selected_projects` and `test_search_filters_by_key` are both straightforward filter assertions). Consolidating these into a single `@pytest.mark.parametrize` block (or the equivalent Django pattern) would remove the repetitive `self.client.get / response.json()` boilerplate and make it easier to add more input/output pairs later.
Reviews (1): Last reviewed commit: "fix(feature-flags): show all projects' f..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+204
to
+214
| results = [ | ||
| { | ||
| "key": flag.key, | ||
| "name": flag.name or "", | ||
| "flag_id": flag.id, | ||
| "team_id": flag.team_id, | ||
| "filters": flag.get_filters(), | ||
| "active": flag.active, | ||
| } | ||
| for flag in (representatives[key] for key in page_keys) | ||
| ] |
Contributor
There was a problem hiding this comment.
KeyError causes HTTP 500. Between the distinct_keys slice that populates page_keys and the page_flags query that fills representatives, a flag can be soft-deleted. Any key that survives the first query but loses all its live flags before the second query will be absent from representatives, so representatives[key] raises an unhandled KeyError and the endpoint returns 500 instead of a valid (shorter) page.
Suggested change
| results = [ | |
| { | |
| "key": flag.key, | |
| "name": flag.name or "", | |
| "flag_id": flag.id, | |
| "team_id": flag.team_id, | |
| "filters": flag.get_filters(), | |
| "active": flag.active, | |
| } | |
| for flag in (representatives[key] for key in page_keys) | |
| ] | |
| results = [ | |
| { | |
| "key": flag.key, | |
| "name": flag.name or "", | |
| "flag_id": flag.id, | |
| "team_id": flag.team_id, | |
| "filters": flag.get_filters(), | |
| "active": flag.active, | |
| } | |
| for key in page_keys | |
| if (flag := representatives.get(key)) is not None | |
| ] |
Prompt To Fix With AI
This is a comment left during a code review.
Path: products/feature_flags/backend/api/organization_feature_flag.py
Line: 204-214
Comment:
**Concurrent-deletion `KeyError` causes HTTP 500.** Between the `distinct_keys` slice that populates `page_keys` and the `page_flags` query that fills `representatives`, a flag can be soft-deleted. Any key that survives the first query but loses all its live flags before the second query will be absent from `representatives`, so `representatives[key]` raises an unhandled `KeyError` and the endpoint returns 500 instead of a valid (shorter) page.
```suggestion
results = [
{
"key": flag.key,
"name": flag.name or "",
"flag_id": flag.id,
"team_id": flag.team_id,
"filters": flag.get_filters(),
"active": flag.active,
}
for key in page_keys
if (flag := representatives.get(key)) is not None
]
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+136
to
+212
| class TestOrganizationFeatureFlagKeys(APIBaseTest): | ||
| def setUp(self): | ||
| self.team_1 = self.team | ||
| self.team_2 = Team.objects.create(organization=self.organization) | ||
|
|
||
| # Shared key in both projects, plus a key unique to each project. | ||
| FeatureFlag.objects.create(team=self.team_1, created_by=self.user, key="shared", name="Shared flag") | ||
| FeatureFlag.objects.create(team=self.team_2, created_by=self.user, key="shared", name="Shared flag in team 2") | ||
| FeatureFlag.objects.create(team=self.team_1, created_by=self.user, key="only-team-1") | ||
| FeatureFlag.objects.create(team=self.team_2, created_by=self.user, key="only-team-2") | ||
| FeatureFlag.objects.create(team=self.team_2, created_by=self.user, key="deleted", deleted=True) | ||
|
|
||
| super().setUp() | ||
|
|
||
| def _url(self, query: str = "") -> str: | ||
| return f"/api/organizations/{self.organization.id}/feature_flags/keys{query}" | ||
|
|
||
| def test_returns_union_of_keys_across_selected_projects(self): | ||
| # Core fix: the union spans both projects, not just the current one. | ||
| response = self.client.get(self._url(f"?team_ids={self.team_1.id},{self.team_2.id}")) | ||
| self.assertEqual(response.status_code, status.HTTP_200_OK) | ||
|
|
||
| body = response.json() | ||
| self.assertEqual(body["count"], 3) | ||
| self.assertEqual([row["key"] for row in body["results"]], ["only-team-1", "only-team-2", "shared"]) | ||
|
|
||
| def test_excludes_deleted_flags(self): | ||
| response = self.client.get(self._url(f"?team_ids={self.team_2.id}")) | ||
| keys = [row["key"] for row in response.json()["results"]] | ||
| self.assertNotIn("deleted", keys) | ||
|
|
||
| def test_team_ids_filters_to_selected_projects(self): | ||
| response = self.client.get(self._url(f"?team_ids={self.team_2.id}")) | ||
| keys = [row["key"] for row in response.json()["results"]] | ||
| self.assertEqual(keys, ["only-team-2", "shared"]) | ||
| self.assertNotIn("only-team-1", keys) | ||
|
|
||
| def test_search_filters_by_key(self): | ||
| response = self.client.get(self._url(f"?team_ids={self.team_1.id},{self.team_2.id}&search=only-team-1")) | ||
| self.assertEqual([row["key"] for row in response.json()["results"]], ["only-team-1"]) | ||
|
|
||
| def test_representative_prefers_current_team(self): | ||
| response = self.client.get( | ||
| self._url(f"?team_ids={self.team_1.id},{self.team_2.id}¤t_team_id={self.team_1.id}") | ||
| ) | ||
| shared = next(row for row in response.json()["results"] if row["key"] == "shared") | ||
| self.assertEqual(shared["team_id"], self.team_1.id) | ||
| self.assertEqual(shared["name"], "Shared flag") | ||
|
|
||
| def test_pagination(self): | ||
| response = self.client.get(self._url(f"?team_ids={self.team_1.id},{self.team_2.id}&limit=2&offset=0")) | ||
| body = response.json() | ||
| self.assertEqual(body["count"], 3) | ||
| self.assertEqual(len(body["results"]), 2) | ||
| self.assertIsNotNone(body["next"]) | ||
| self.assertIsNone(body["previous"]) | ||
|
|
||
| response = self.client.get(self._url(f"?team_ids={self.team_1.id},{self.team_2.id}&limit=2&offset=2")) | ||
| body = response.json() | ||
| self.assertEqual(len(body["results"]), 1) | ||
| self.assertIsNone(body["next"]) | ||
| self.assertIsNotNone(body["previous"]) | ||
|
|
||
| def test_defaults_to_all_accessible_teams(self): | ||
| response = self.client.get(self._url()) | ||
| self.assertEqual(response.status_code, status.HTTP_200_OK) | ||
| self.assertEqual(response.json()["count"], 3) | ||
|
|
||
| def test_invalid_team_ids_returns_400(self): | ||
| response = self.client.get(self._url("?team_ids=not-an-int")) | ||
| self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) | ||
|
|
||
| def test_unauthorized(self): | ||
| self.client.logout() | ||
| response = self.client.get(self._url()) | ||
| self.assertEqual(response.status_code, status.HTTP_401_UNAUTHORIZED) | ||
|
|
Contributor
There was a problem hiding this comment.
TestOrganizationFeatureFlagKeys cover closely related variations of the same scenario (e.g. test_team_ids_filters_to_selected_projects and test_search_filters_by_key are both straightforward filter assertions). Consolidating these into a single @pytest.mark.parametrize block (or the equivalent Django pattern) would remove the repetitive self.client.get / response.json() boilerplate and make it easier to add more input/output pairs later.
Prompt To Fix With AI
This is a comment left during a code review.
Path: products/feature_flags/backend/api/test/test_organization_feature_flag.py
Line: 136-212
Comment:
**Non-parameterised tests preferred.** Several test methods in `TestOrganizationFeatureFlagKeys` cover closely related variations of the same scenario (e.g. `test_team_ids_filters_to_selected_projects` and `test_search_filters_by_key` are both straightforward filter assertions). Consolidating these into a single `@pytest.mark.parametrize` block (or the equivalent Django pattern) would remove the repetitive `self.client.get / response.json()` boilerplate and make it easier to add more input/output pairs later.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
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 feature flags Projects tab promises to "compare each flag's status, rollout, and recent usage across your organization's projects", but it only ever listed flags that exist in the currently active project. If you were on a project with 2 flags and compared it against a project with many more, you still saw only those 2 rows — the rest of the compared project's flags were invisible. Users expect every flag in the compared projects to appear.
Closes #63155
Changes
keysaction on the organization feature flag viewset (GET /api/organizations/{org}/feature_flags/keys/). It returns the distinct flag keys across the selected projects (intersected with the teams the user can access), each with a representative flag — preferring the current project — for the row's name and link.The endpoint is excluded from the public OpenAPI spec (
@extend_schema(exclude=True)) since it's an internal endpoint consumed only by this grid via a handwritten client.How did you test this code?
I'm an agent (PostHog Slack app). I did not perform manual testing. I added and updated automated tests but could not execute them in this environment (no installed JS/Python deps or test DB):
TestOrganizationFeatureFlagKeysclass covering the cross-project union,team_idsfiltering, deleted-flag exclusion, search, representative preference for the current team, pagination, default-to-accessible-teams, invalid input, and auth.projectsGridLogic.test.tsfor the new endpoint shape, plus cases asserting rows reload from the top when the compared projects change and that keys are requested for the visible columns.These should be run in CI before merge.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Authored by the PostHog Slack app at @phillram's request to fix #63155. Key decision: the row set is the union of distinct flag keys across the visible projects (current + picked), which matches the "compare selected projects" mental model. Rather than fetching every project's flag list client-side, I added one backend endpoint that computes the distinct-key union with pagination/search, picking a representative flag per key (preferring the current project so row links stay local). Excluded it from the OpenAPI spec to avoid drifting generated types, consistent with the file's existing handwritten client usage.
Created with PostHog Code from a Slack thread