Conversation
github-actions
bot
added
the
Scope: Frontend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
jaydgoss
left a comment
jaydgoss
left a comment
There was a problem hiding this comment.
Issue cursor brought up seems like the only blocker
static/app/views/detectors/components/forms/cron/previewSection.tsx
Outdated
Show resolved
Hide resolved
| name: T | ||
| ): UptimeDetectorFormData[T] { | ||
| const value = useFormField(name); | ||
| return value || DEFAULT_UPTIME_DETECTOR_FORM_DATA_MAP[name]; |
There was a problem hiding this comment.
l: Did you consider using ?? (nullish coalescing) instead of || here? Using || means falsy values like 0, false, or '' get replaced with defaults. While the current defaults happen to work correctly, ?? would be more precise if you only want to fall back when the value is null/undefined.
Same pattern exists in useCronDetectorFormField in cron/fields.tsx:70.
There was a problem hiding this comment.
Yeah this was intentional to fallback for when the fields are cleared i.e. ''. You are right about the false value.
Pushed a more explicit check for '' and !defined values instead
| const recoveryThreshold = useUptimeDetectorFormField('recoveryThreshold'); | ||
|
|
||
| const intervalSeconds = useUptimeDetectorFormField('intervalSeconds'); | ||
| const intervalMinutes = Math.floor(intervalSeconds / 60); |
There was a problem hiding this comment.
l: Was the truncation with Math.floor(intervalSeconds / 60) intentional? If intervalSeconds is 90, this becomes 1 minute rather than 1.5. Just want to confirm the API expects integer minutes.
There was a problem hiding this comment.
Removed this, the sample endpoint 400s for non integer minutes, but we have a warning setup for it.
Shouldn't be triggered by the current options:
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.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
jaydgoss
left a comment
jaydgoss
left a comment
There was a problem hiding this comment.
m: Not sure if this is intentional or not:
DEFAULT_UPTIME_DETECTOR_FORM_DATA_MAP.timeoutMs is set to 5000, but
uptimeSavedDetectorToFormData returns 10000 as the fallback when a data source
is missing. These should be aligned to avoid inconsistent behavior.
// fields.tsx ~line 49
timeoutMs: 5000, // should be 10000 to match historical default
Consider also using DEFAULT_UPTIME_DETECTOR_FORM_DATA_MAP.timeoutMs in
uptimeSavedDetectorToFormData for consistency, similar to how intervalSeconds,
method, and traceSampling were refactored.
|
@jaydgoss A little weird but the form assigns a default value of 5s, that gets saved upon creating a monitor. I kept 5s as the default and used it in side: url is the only hardcoded fallback in |
2 participants
PreviewScheduleuseUptimeDetectorFormFieldthat assigns defaults when retrieving uptime form fields