refactor(featureStateToValue): extract into a Flux-free module - #8344
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdded the shared Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🟡 Moderate · up to The refactor centralizes feature-state flattening, but the current head still uses a disallowed relative import and does not model the typed core-trait input without a cast; these issues can break repository checks and weaken compile-time safety, so merge should wait for both fixes. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Docker builds report
|
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19751 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
🗂️ Previous results✅ oss · depot-ubuntu-latest-arm-16 — run #19751 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
✅ private-cloud · depot-ubuntu-latest-16 — run #19751 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-16 — run #19751 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
✅ private-cloud · depot-ubuntu-latest-16 — run #19720 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19720 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19720 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-16 — run #19720 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19715 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
✅ private-cloud · depot-ubuntu-latest-16 — run #19715 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19715 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-16 — run #19715 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
|
Visual Regression19 screenshots compared. See report for details. |
1da53b5 to
edcca61
Compare
Pull Request is not mergeable
edcca61 to
55a1f1c
Compare
55a1f1c to
fcc8bf6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0cccb99f-04e2-4734-8fd0-297e1a2cedc3
📒 Files selected for processing (5)
frontend/common/utils/__tests__/featureStateToValue.test.tsfrontend/common/utils/featureStateToValue.tsfrontend/common/utils/utils.tsxfrontend/web/components/pages/features/hooks/__tests__/deepLinkedFeature.test.tsfrontend/web/components/pages/features/hooks/deepLinkedFeature.ts
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
fcc8bf6 to
3f35fe7
Compare
3f35fe7 to
ac9cdd1
Compare
Zaimwa9
left a comment
There was a problem hiding this comment.
One non blocking comment otherwise good to go
Utils.featureStateToValue is a pure function, but it lives in utils.tsx, which imports the Flux stores (AccountStore/ProjectStore) at module load. That import chain breaks any unit-tested consumer, which is why the deep-link hook had to inline its own copy of the flatten logic. Move it to common/utils/featureStateToValue.ts (types only, no Flux) and have Utils delegate via a shorthand re-export, so the ~22 existing callers are unchanged. The deep-link hook now imports the shared function and drops its duplicate. The function is broadened to also accept an already-flat value (returned untouched) and to normalise a missing int/float to null. First step of decoupling the pure helpers from utils.tsx. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ac9cdd1 to
3c29081
Compare
Zaimwa9
left a comment
There was a problem hiding this comment.
Giving the approval again as it stalled
Changes
Contributes to #8337 (follow-up to #8341)
Utils.featureStateToValueis a pure function, but it lives inutils.tsx, which imports the Flux stores (AccountStore/ProjectStore) at module load. That import chain breaks any unit-tested consumer, which is why #8341's deep-link hook had to inline its own copy of the flatten logic.This extracts it into
common/utils/featureStateToValue.ts(types only, no Flux) and hasUtilsdelegate to it via a shorthand re-export — so the ~22 existingUtils.featureStateToValue(...)callers are unchanged. The deep-link hook now imports the shared function and drops its duplicate.The function is broadened while moving:
int/floattonull.This is the first step of decoupling the pure helpers from
utils.tsx(which follows the existingcommon/utils/<name>.tsconvention, e.g.ensureTrailingSlash,multivariate).How did you test this code?
common/utils/__tests__/featureStateToValue.test.ts: covers every type branch (bool/float/int/unicode),value_typefor core traits, missing int/float →null, null/undefined →null, and already-flat passthrough.deepLinkedFeature.test.ts: keeps thepickEnvironmentFlagbehaviour tests plus one integration case proving the nested value is flattened via the shared function.