Skip to content

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

Merged
merged 5 commits into from
Jun 5, 2025

Conversation

dmarticus
Copy link
Contributor

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 as decide. Here was the diff before (/flags on the left, /decide on the right)

Before

image

and after

image

Changes

  • serialize autocapture_opt_out correctly
  • serialize a few fields in the SessionRecordingConfig correctly
  • serialize the "analytics" entry correctly
  • exclude "hasFeatureFlags"

Did you write or update any docs for this change?

  • No docs needed for this change

How did you test this code?

  • I wrote a ton of new tests for more complex configs to feel even better about matching to /decide.

@dmarticus dmarticus requested a review from haacked June 5, 2025 05:58
Copy link
Contributor

@greptile-apps greptile-apps bot left a 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 in rust/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

Comment on lines 550 to 551
let mut con = client.get_connection()?;
redis::cmd("FLUSHDB").execute(&mut con);
Copy link
Contributor

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.

Suggested change
let mut con = client.get_connection()?;
redis::cmd("FLUSHDB").execute(&mut con);
let mut con = client.get_connection()?;
redis::cmd("FLUSHDB").execute(&mut con)?;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems legit, no?

Copy link
Contributor Author

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.

Copy link
Contributor

@haacked haacked left a 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?

Comment on lines 550 to 551
let mut con = client.get_connection()?;
redis::cmd("FLUSHDB").execute(&mut con);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems legit, no?

@dmarticus dmarticus merged commit 8627707 into master Jun 5, 2025
87 checks passed
@dmarticus dmarticus deleted the fix/tune-a-few-legacy-decide-fields branch June 5, 2025 16:35
@dmarticus dmarticus mentioned this pull request Jun 5, 2025
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants