feat(uptime-json-path-assertion): Splitting path string to value, operator and operand#107512
feat(uptime-json-path-assertion): Splitting path string to value, operator and operand#107512
Conversation
github-actions
bot
added
the
Scope: Frontend
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
static/app/views/alerts/rules/uptime/assertions/assertionFailure/rows/jsonPathOpRow.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| if (nextValue) { | ||
| onChange(nextValue); | ||
| } | ||
| }, [isNumeric, onChange, operandValue, normalizedOp]); |
There was a problem hiding this comment.
useEffect runs on every render due to object dependency
Medium Severity
The normalizedOp variable is created fresh on every render via normalizeJsonPathOp(value), and then included in the useEffect dependency array at line 77. Since normalizedOp is always a new object reference, the effect runs on every render regardless of whether the actual values changed. This is inefficient and a common React anti-pattern. The fix is to memoize normalizedOp with useMemo(() => normalizeJsonPathOp(value), [value]) so it only changes when value actually changes.
There was a problem hiding this comment.
We don't really care much about performance here, I think this is okay.
We do spam useMemo all over the codebase
jaydgoss
left a comment
jaydgoss
left a comment
There was a problem hiding this comment.
LGTM, I think it would be good to do some manual testing with existing monitors that have json assertions to make sure this is fully backwards compatible.
klochek
left a comment
klochek
left a comment
There was a problem hiding this comment.
Path/operand stuff looks good to me
3 participants
Before:
After:
