-
Notifications
You must be signed in to change notification settings - Fork 227
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
feat: UI for adding alerts for dashboard tiles #562
Conversation
ernestii
commented
Jan 20, 2025
•
edited
Loading
edited
🦋 Changeset detectedLatest commit: 72c9926 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
a813a05
to
9aa649b
Compare
9aa649b
to
67ee87d
Compare
21dcf30
to
a90737e
Compare
6dda29e
to
052e209
Compare
a90737e
to
a41f979
Compare
052e209
to
e7e9885
Compare
await Alert.find({ | ||
dashboard: { $in: _dashboards.map(d => d._id) }, | ||
source: 'tile', | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
double check we want to select all fields from the alert?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, I think all data is fine, we need full channel
to populate the form
...d, | ||
tiles: d.tiles.map(t => ({ | ||
...t, | ||
config: { ...t.config, alert: alertsByTileId[t.id]?.[0] }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason we want to add alert
field to config
instead of adding it to the top level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the reason what that it's easier to integrate alert form into Edit Chart form, since we can use same react-hook-form controller (it uses SavedChartConfig schema)
a41f979
to
74934e8
Compare
4a7a5d4
to
85002c3
Compare
0d4adc2
to
b54104b
Compare
9880cb2
to
ae2dbdf
Compare
f0c8364
to
6616f30
Compare
packages/common-utils/src/types.ts
Outdated
@@ -346,6 +347,7 @@ export const SavedChartConfigSchema = z.intersection( | |||
z.object({ | |||
name: z.string(), | |||
source: z.string(), | |||
alert: AlertSchemaBase.optional(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't this supposed to be AlertSchema
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not really, since tile alerts are managed via /dashboard
routes, we already know its source (Tile) and dashboardId
and tileId
, so we only need alert configuration here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. But don't we still attach the redundant fields? from getDashboardAlerts
, we fetch all fields in the alert. I guess the same thing applied to
hyperdx/packages/common-utils/src/types.ts
Line 269 in f1228de
alerts: z.array(AlertSchema).optional(), |
6616f30
to
aafd130
Compare
9cf83f4
to
48bdb7e
Compare
48bdb7e
to
1ea8af3
Compare
Object.entries(alertsByTile).map(async ([tileId, alert]) => { | ||
return await Alert.findOneAndUpdate( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this async
and await
seem redundant
The rest of the comments are minor. I'm going to clean this PR up and merge it |