fix(Utils): coerce non-string values before split in validateRule - #8198
fix(Utils): coerce non-string values before split in validateRule#8198bardock-2393 wants to merge 2 commits into
Conversation
The semver and MODULO branches of validateRule guarded against a missing rule value but still called .split() directly on rule.value, so a truthy non-string value (e.g. a boolean) reached .split and threw. Coerce to a string first so validation degrades to false instead of crashing CreateSegment/EditSegment's isValid check.
|
@bardock-2393 is attempting to deploy a commit to the Flagsmith Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
github-actions
Bot
added
the
front-end
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f878f4cf-e85a-4366-88a4-0078f2dc7264
📒 Files selected for processing (2)
frontend/common/utils/__tests__/utils.test.tsfrontend/common/utils/utils.tsx
Strengthens the MODULO and semver regression tests per CodeRabbit review feedback on PR Flagsmith#8198 — they previously only checked for no throw.
1 participant
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Closes #7536
Utils.validateRule(used by both the create-segment and edit-segment forms to decide whether a condition row is valid) crashes withTypeError: Cannot read properties of undefined (reading 'split')for some segment conditions.#7779 previously added a guard so a missing (
null/undefined) condition value returnsfalseinstead of crashing for the semver and Modulo operators. That guard only covers a missing value, though — it still calls.split()directly onrule.valueonce the value is present. If a condition reachesvalidateRulewith a truthy value that isn't a string (for example a boolean, which can happen while a condition row's value is mid-update),.splitdoesn't exist on it and the same class of crash occurs, which is why this issue stayed open after #7779 merged.This PR coerces the value to a string (
`${rule.value}`) before calling.split()in both the semver and Modulo branches, sovalidateRulenever throws regardless of what type the value happens to be, while keeping the existing pass/fail validation behaviour for well-formed input unchanged.How did you test this code?
Added unit tests in
frontend/common/utils/__tests__/utils.test.tscoveringvalidateRulefor the Modulo and semver operators with missing (undefined/null/empty string) and non-string (boolean) values, plus theIS_SET/IS_NOT_SET"hideValue" operator, and existing valid/invalid value cases. The two non-string-value tests fail on the code prior to this fix (confirmed by reverting the fix locally and re-running) and pass afterwards.Ran the full frontend unit test suite (
npm run test:unit): 368 tests passed across 28 suites. Also rannpx eslintandnpm run typecheckagainst the changed files — no new lint or type errors introduced.