Conversation
emyller
requested review from
gagantrivedi and
kyle-ssg
and removed request for
a team
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
github-actions
bot
added
front-end
Docker builds report
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on March 10
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6722 +/- ##
=======================================
Coverage 98.23% 98.23%
=======================================
Files 1311 1311
Lines 48474 48513 +39
=======================================
+ Hits 47617 47656 +39
Misses 857 857 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
I have a non-blocking question and the e2e are failing so it's gonna stale the approval anyways, i'll run them to see what's going on
✖ Segment-part-3
1) AssertionError: waitForElementVisible([data-test="user-feature-switch-1-off"]): expected false to be truthy
Browser: Firefox 143.0 / Debian 13.3
48 |
49 |export const waitForElementVisible = async (selector: string) => {
50 | logUsingLastSection(`Waiting element visible ${selector}`)
51 | return t
52 | .expect(Selector(selector).visible)
> 53 | .ok(`waitForElementVisible(${selector})`, { timeout: LONG_TIMEOUT })
54 |}
55 |
56 |export const waitForElementNotClickable = async (selector: string) => {
57 | logUsingLastSection(`Waiting element visible ${selector}`)
58 | await t
at <anonymous> (/srv/flagsmith/e2e/helpers.cafe.ts:53:6)
at step (/srv/flagsmith/e2e/helpers.cafe.ts:33:23)
at Object.next (/srv/flagsmith/e2e/helpers.cafe.ts:14:53)
at <anonymous> (/srv/flagsmith/e2e/helpers.cafe.ts:8:71)
at __awaiter (/srv/flagsmith/e2e/helpers.cafe.ts:4:12)
at waitForElementVisible (/srv/flagsmith/e2e/helpers.cafe.ts:49:61)
at <anonymous> (/srv/flagsmith/e2e/tests/segment-test.ts:261:30)
at step (/srv/flagsmith/e2e/tests/segment-test.ts:33:23)
at Object.next (/srv/flagsmith/e2e/tests/segment-test.ts:14:53)
at fulfilled (/srv/flagsmith/e2e/tests/segment-test.ts:5:58)
| override_ordering.append("-has_segment_override") | ||
| if identity_id := query_data.get("identity"): | ||
| queryset = queryset.annotate( | ||
| has_identity_override=Exists( |
There was a problem hiding this comment.
So here, this would make a subquery for each feature row right? I'm comparing to the segment query just above that have a unique_constraint on (feature, segment_id, environment_id), there might be a slight performance difference?
I personally think it's negligeable given that feature limit is 400 per project (~max overrides we can expect) but do you have an idea of the perf loss, how we could measaure it and at what point it could become problematic?
|
Looks like legit as persistent and reproducible:
|
| required=False, | ||
| help_text="Integer ID of the segment to retrieve segment overrides for.", | ||
| ) | ||
| identity = serializers.IntegerField( |
There was a problem hiding this comment.
Yes ok, that's working when identities are stored in postgres, but when using edge identities, it passes an UUID.
I'll post a full comment with my findings as we'll need to have 2 implementations
There was a problem hiding this comment.
Can you confirm you were having integer identity.id in the screenshot you shared ?
Zaimwa9
left a comment
Zaimwa9
left a comment
There was a problem hiding this comment.
So my finding after tracking the E2E failure is that we need 2 distinct approaches depending on using edge identities or storing them in postgres.
The current implementation is all good for postgres, even though the frontend should not pass the identity if using edge (we have an utils that is already used to call the edge endpoints).
For edge:
It's actually simpler, as the information already exists. It's coming from
/api/v1/environments/{env_key}/edge-identities/{identity_as_uuid}/edge-featurestates/all/
and the response is as an array as follows:
[
{
"feature": {
"id": 174685,
"name": "my_go_flag",
"type": "STANDARD"
},
"enabled": true,
"feature_state_value": null,
"overridden_by": null,
"segment": null,
"multivariate_feature_state_values": []
},
{
"feature": {
"id": 174687,
"name": "welcome_page",
"type": "MULTIVARIATE"
},
"enabled": false,
"feature_state_value": null,
"overridden_by": null,
"segment": null,
"multivariate_feature_state_values": [
{
"multivariate_feature_option": {
"value": "plop"
},
"percentage_allocation": 0
}
]
}
]
We already have "overridden_by": null, that can be IDENTITY | SEGMENT. So we could even do it from the frontend (i'd be more at peace to add a sort query param and have it done in the backend as it is the case with postgres identities). Don't hesitate to confirm my finding and I let you chose the best path to take
| search, | ||
| sort, | ||
| getServerFilter(filter), | ||
| { ...getServerFilter(filter), identity: id }, |
There was a problem hiding this comment.
Here we will need to use Utils.getIsEdge() to do something like ...(!isEdge ? { identity: id } : {}),, also for the next useEffect
| required=False, | ||
| help_text="Integer ID of the segment to retrieve segment overrides for.", | ||
| ) | ||
| identity = serializers.IntegerField( |
There was a problem hiding this comment.
Can you confirm you were having integer identity.id in the screenshot you shared ?
2 participants
Closes #6188.
Prioritise displaying overriden features in the dashboard.
For segment overrides:
For identity overrides:
Review effort: 2/5