Repository navigation
Conversation
Tile alerts can only express a static threshold, and the editor hard-codes that. Add `AlertUIExtension` (components/alerts/ alertUIExtension.tsx), read from a new `@/extensions` module that supplies none here: - useConditionEditor: header, condition fields, labels, interval options and footer for TileAlertEditor's condition row. The threshold controls move, unchanged, to ThresholdConditionFields. - useTimeChartPreview: config override, extra DBTimeChart props, hiding the threshold lines, and a footer for the preview's time chart. - useChartEditor: beforeSave and normalizeAlert for the chart editor. - Provider: wraps EditTimeChartForm (via the folder's index) so the hooks above can share state. With no extension the editor renders and saves exactly as before.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 0346d10 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
🟡 Tier 3 — StandardIntroduces new logic, modifies core functionality, or touches areas with non-trivial risk. Why this tier:
Review process: Full human review — logic, architecture, edge cases. Stats
|
Duplicate code analysis1 new clone introduced by this PR.
If one of these duplicates a helper that already exists, import it instead jscpd compared this branch against |
PR Review
4 finding(s): 🔴 0 critical · 🟠 1 major · 🔵 3 minor 1 posted as inline comment(s) on the changed lines. 3 listed below. Findings outside the changed lines
2 minor
Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
Deep Review✅ No critical issues found. This is a behavior-preserving refactor. With no extension registered ( No P2-level issues surfaced: the no-op fallbacks, reference-line gating, and stable hook identity are all sound, and the new behavior ( 🔵 P3 nitpicks (2)
Reviewers (6): correctness, testing, maintainability, project-standards, kieran-typescript, previous-comments. Testing gaps: No coverage for the No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond |
…n test The escape-hatch ratchet caps eslint-disable comments in the app.
E2E Test Results✅ All tests passed • 463 passed • 1 skipped • 1211s
Tests ran across 4 shards in parallel. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| <NativeSelect | ||
| data={optionsToSelectData(TILE_ALERT_INTERVAL_OPTIONS)} | ||
| data={optionsToSelectData( | ||
| conditionUI.intervalOptions ?? TILE_ALERT_INTERVAL_OPTIONS, |
There was a problem hiding this comment.
🔵 minor — Narrowed intervalOptions are not reconciled with the form's current interval
If the extension passes intervalOptions that leave out the alert's current interval (the default is '5m'; an existing alert might be '1m'), the NativeSelect shows a different option than the one stored, and the hidden value is what gets saved. Fix: when the current alert.interval is not a key of conditionUI.intervalOptions, reset it to the first allowed key with setValue, or document that the extension must do this and enforce it in the hook wrapper.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
|
|
||
| const handleSave = useCallback( | ||
| (form: ChartEditorFormState) => { | ||
| if (!beforeSave(form)) { |
There was a problem hiding this comment.
potentially blocking here, seems handleSaveAlert doesn't call beforeSave
This check only runs in handleSave. The edit-alert modal saves through handleSaveAlert a little further down, so an extension that blocks a save there would still see the alert saved. Would something like this make sense?
const handleSaveAlert = useCallback(
(form: ChartEditorFormState) => {
if (!beforeSave(form)) return;There was a problem hiding this comment.
Good catch, fixed in 9c9f716: handleSaveAlert now runs beforeSave first, same as handleSave. The new test does not save an inline alert when beforeSave returns false covers it.
…ne alert handleSaveAlert, used by the inline-alert editor, skipped the check that handleSave runs, so an extension couldn't stop that save.
…r below the chart - normalizeAlert takes the saved builder alert instead of a generic T. - timeChartProps can't set config or threshold lines, which have their own fields. - The preview footer renders after the 400px chart container, not inside it. - intervalOptions documents that the extension keeps alert.interval within them.
|
|
||
| const handleSave = useCallback( | ||
| (form: ChartEditorFormState) => { | ||
| if (!beforeSave(form)) { |
There was a problem hiding this comment.
🔵 minor — beforeSave runs before validation, so a form can fail in two rounds
beforeSave gets the raw, unvalidated form and is expected to show its own message. If the form is also invalid, the user sees only the extension's message. After they fix it, the next save shows the validateChartForm errors. Call beforeSave after validateAndNormalize succeeds, so the user sees every error in one pass and the extension never sees a malformed alert.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
|
Going to do a bit of refactoring before doing this |
Summary
Tile alerts can only express a static threshold, and the chart editor hard-codes that in three places: the alert's condition row, the threshold lines on the preview chart, and saving. This adds an extension point for other detection strategies, the UI counterpart to the backend's
AlertEvaluator(#3576). With no extension registered, the editor renders and saves exactly as before.AlertUIExtension(components/alerts/alertUIExtension.tsx) is read from a new@/extensionsmodule, which supplies none here. Every part is optional:useConditionEditor(used byTileAlertEditor): a header, the condition fields, the labels, the interval options and a footer for the condition row. The threshold controls move, unchanged, intoThresholdConditionFields, which renders when no condition fields are supplied.useTimeChartPreview(used byChartPreviewPanel): replaces the previewed config, addsDBTimeChartprops, hides the threshold reference lines, and adds content below the chart.useChartEditor(used byEditTimeChartForm):beforeSave, which can stop a save, andnormalizeAlert, which is applied to a builder chart's saved alert. Callbacks must keep their identity across renders.Provider: wrapsEditTimeChartForm, applied in the folder'sindex.ts, so the hooks above can share state.How to test locally or on Vercel
cd packages/app && yarn ci:unit src/components/DBEditTimeChartForm src/components/ChartEditor src/components/alerts. The new*.extension.test.tsxfiles check:beforeSaveblocks a save andnormalizeAlertshapes the saved alertThe existing suites cover the unchanged no-extension path.
Manual: open a dashboard tile, add an alert, and check that the threshold row, interval and preview reference lines look and behave as on
main.Screenshots
No visual change is intended. I haven't captured before/after screenshots, because I had no browser available; please add them if you can.
🤖 Generated with Claude Code