-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(flags): make the config fields on /flags
perfectly map to /decide
#33194
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
Makes feature flag configurations in /flags
endpoint match exactly with /decide
endpoint, focusing on field serialization and response structure consistency.
- Modified
autocapture_opt_out
field inrust/feature-flags/src/api/types.rs
to be a non-optional boolean defaulting to false - Added
skip_serializing_if = "Option::is_none"
for session recording canvas fields to match/decide
behavior - Removed
has_feature_flags
field from response as it's not needed in API alignment - Added extensive test coverage using deterministic UUIDs for site app URLs instead of timestamps
- Ensures empty analytics capture endpoints are not exposed in responses
6 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
let mut con = client.get_connection()?; | ||
redis::cmd("FLUSHDB").execute(&mut con); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Missing explicit error handling for Redis FLUSHDB command result. Should capture and return the result.
let mut con = client.get_connection()?; | |
redis::cmd("FLUSHDB").execute(&mut con); | |
let mut con = client.get_connection()?; | |
redis::cmd("FLUSHDB").execute(&mut con)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems legit, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, actually, that was a commit that crept in; i didn't want to commit those files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing the diff on the response is clutch! Is the greptile suggestion legit?
let mut con = client.get_connection()?; | ||
redis::cmd("FLUSHDB").execute(&mut con); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems legit, no?
Important
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Problem
Before this change, there were a few fields that
/flags
didn't serialize the exact same way asdecide
. Here was the diff before (/flags
on the left,/decide
on the right)Before
and after
Changes
autocapture_opt_out
correctlySessionRecordingConfig
correctly"analytics"
entry correctlyDid you write or update any docs for this change?
How did you test this code?
/decide
.