feat: alert overview mode with column filters and multi-section workspace - #37
Merged
Conversation
…on workspace - OverviewTemplateTree: checkbox tree with search, group/leaf toggle, indeterminate state - AlertOverviewWorkspace: multi-section workspace with workspace-level and per-section filters - AlertTable: lift filter state to props (filters/onFiltersChange), attach __realIndex for correct edit/delete on filtered rows - AlertUserView: Single/Overview mode toggle via Segmented control, overview checked state persisted via useSessionState Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rsists on no-match - matchesFilter: case-insensitive string compare, fix null/undefined guard - FilterHeader: string ops ['contains','='], enum ops depend on value type (numeric enum → numeric ops, string enum → ['='] only) - Table: replace display:none with locale.emptyText so filter inputs stay visible when no rows match - AlertUserView single mode: add Clear filters button to save bar - SectionPanel: add Clear filters button in section header (clears both section and workspace filters) - e2e: alert-filter.spec.js covers contains/exact string filter, numeric filter, filter-input persistence on no-match, Clear filters button in single and overview modes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
matchesFilter was reading row[varName] for common vars which is always undefined — their value lives in commonValues. Now falls back to commonValues[varName] when the field is a common var. Added e2e test: common var column filter keeps/removes rows correctly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Fix race condition: allValues added to useEffect deps so rows load
correctly when alert type is clicked before deployment fetch completes
- Correct e2e test fixture to use mariadb_latency_slow_queries key and
actual schema fields (instance_name, warn_threshold, critical_threshold)
- Fix overview mode tests: use { exact: true } to avoid strict mode
violations from substring matches on folder names containing "overview"
- Add waitForResponse after clicking deployment node to ensure data is
loaded before clicking alert type templates
- Fix WorkspaceFilterBar conflict: target column headers by th text
selector instead of generic getByPlaceholder('value').first()
- Use .first() on row count locators to handle strict mode when multiple
sections render the same pattern
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
Preview environment torn down. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds overview mode for alerts, with selectable alert types, shared and section filters, filter-aware tables, save support, a schema update for alert instances, and expanded unit and e2e coverage. ChangesAlert overview filtering and save flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Contributor
Author
|
此 PR 涵蓋以下兩個 issue 的內容，請確認是否一次關閉：
如果確認，PR description 加上 |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/AlertUserView.jsx (1)
106-115: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSplit the reset logic from the row-sync effect. Keep
allValuesin the deps forsetRows(allValues[activeAlert] || []), but movesetFilters({})andsetDirty(false)behind theactiveAlert/schemachange only. In overview mode,onAllValuesChangere-triggers this effect whileactiveAlertis still set from single mode, which clears the unsaved state and leaves “Save all” disabled.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/AlertUserView.jsx` around lines 106 - 115, The current AlertUserView useEffect mixes row synchronization with reset behavior, so updates from onAllValuesChange can clear unsaved state in overview mode. Keep the effect that syncs rows via setRows(allValues[activeAlert] || []) dependent on allValues, activeAlert, and schema, but move the reset calls for setFilters({}) and setDirty(false) into a separate effect or logic that only runs when activeAlert or schema changes. Use the existing useEffect in AlertUserView and the activeAlert/schema/allValues dependencies to split these concerns cleanly.
🧹 Nitpick comments (2)
src/components/AlertOverviewWorkspace.jsx (2)
81-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
matchCountduplicatesAlertTable'smatchesFilterand omitscommonValuesfrom deps.This block is a near-verbatim copy of
matchesFilterinsrc/components/AlertTable.jsx(lines 5-27). Extracting a shared helper would prevent the two from drifting. Additionally, the memo deps[rows, filters, vars]omitcommonValues, so the count goes stale for common-var filters (and triggersexhaustive-deps).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/AlertOverviewWorkspace.jsx` around lines 81 - 107, `matchCount` in `AlertOverviewWorkspace` is duplicating the `matchesFilter` logic from `AlertTable`, and its memo can go stale because `commonValues` is missing from the dependency list. Extract the filtering logic into a shared helper used by both `AlertOverviewWorkspace` and `AlertTable`, then update the `useMemo` dependencies for `matchCount` to include `commonValues` so common-var filters recalculate correctly.Source: Coding guidelines
7-7: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winWorkspace
OPERATORSlacks acontainsoption for string columns.
OPERATORS = ['>=', '<=', '>', '<', '=']exposes only numeric comparators plus=. For a string column,matchesFiltertreats any op other than=as a substring match, so a user wanting "contains" must counter-intuitively pick>or<. This diverges fromAlertTable's per-columnFilterHeader, which offers an explicitcontains. Consider deriving operators per selected column (asgetOperatorsdoes) instead of a single static list.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/AlertOverviewWorkspace.jsx` at line 7, The workspace filter operator list in AlertOverviewWorkspace is too generic and omits an explicit contains option for string columns. Update the filtering UI logic around OPERATORS and matchesFilter to derive operators from the selected column type, similar to getOperators and AlertTable’s FilterHeader, so string fields expose contains instead of forcing users to misuse numeric comparators. Keep the operator list dynamic per column and ensure the selected operator still maps correctly in the existing filter handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/AlertOverviewWorkspace.jsx`:
- Around line 176-187: The section filter state in AlertOverviewWorkspace is
being polluted by workspace-level filters because mergedFilters(alertName) is
passed into SectionPanel and then written back through
handleSectionFiltersChange. Update the AlertOverviewWorkspace flow so only
section-specific changes are stored in sectionFilters, not the full merged
object coming from AlertTable/FilterHeader. Use wsFilters as the base for
display, but strip out any workspace-derived entries before saving the delta for
a section, so clearing wsFilters really clears that section too.
In `@src/components/AlertTable.jsx`:
- Around line 84-90: The filteredRows useMemo is missing commonValues in its
dependency list, so changes to common-var filter data can leave AlertTable
stale. Update the useMemo in AlertTable.jsx so the memo depends on commonValues
as well as rows, filters, and vars, keeping matchesFilter behavior in sync and
satisfying the react-hooks/exhaustive-deps rule.
---
Outside diff comments:
In `@src/pages/AlertUserView.jsx`:
- Around line 106-115: The current AlertUserView useEffect mixes row
synchronization with reset behavior, so updates from onAllValuesChange can clear
unsaved state in overview mode. Keep the effect that syncs rows via
setRows(allValues[activeAlert] || []) dependent on allValues, activeAlert, and
schema, but move the reset calls for setFilters({}) and setDirty(false) into a
separate effect or logic that only runs when activeAlert or schema changes. Use
the existing useEffect in AlertUserView and the activeAlert/schema/allValues
dependencies to split these concerns cleanly.
---
Nitpick comments:
In `@src/components/AlertOverviewWorkspace.jsx`:
- Around line 81-107: `matchCount` in `AlertOverviewWorkspace` is duplicating
the `matchesFilter` logic from `AlertTable`, and its memo can go stale because
`commonValues` is missing from the dependency list. Extract the filtering logic
into a shared helper used by both `AlertOverviewWorkspace` and `AlertTable`,
then update the `useMemo` dependencies for `matchCount` to include
`commonValues` so common-var filters recalculate correctly.
- Line 7: The workspace filter operator list in AlertOverviewWorkspace is too
generic and omits an explicit contains option for string columns. Update the
filtering UI logic around OPERATORS and matchesFilter to derive operators from
the selected column type, similar to getOperators and AlertTable’s FilterHeader,
so string fields expose contains instead of forcing users to misuse numeric
comparators. Keep the operator list dynamic per column and ensure the selected
operator still maps correctly in the existing filter handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 85e7a925-70b1-4ed9-bddc-a8f99a1052b7
📒 Files selected for processing (6)
src/components/AlertOverviewWorkspace.jsxsrc/components/AlertTable.jsxsrc/components/OverviewTemplateTree.jsxsrc/pages/AlertUserView.jsxtests/e2e/alert-filter.spec.jstests/e2e/alert-overview-mode.spec.js
Adds alert-overview-save.spec.js and expands alert-filter.spec.js and alert-overview-mode.spec.js with new describe blocks: - alert-filter.spec.js: Overview mode section filters, Filter state management - alert-overview-mode.spec.js: Workspace filter bar, Session persistence - alert-overview-save.spec.js: Save all persistence, Add/delete rows in sections Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…er, fix workspace filter ops - AlertUserView: split useEffect into reset effect ([activeAlert, schema]) and row-sync effect ([activeAlert, allValues]) so overview saves don't accidentally clear filters/dirty state - Extract matchesFilter to src/utils/filterUtils.js shared by AlertTable and AlertOverviewWorkspace (removes duplication) - AlertTable: add commonValues to filteredRows useMemo deps - AlertOverviewWorkspace: use shared matchesFilter, add commonValues to matchCount useMemo deps, derive workspace filter operators per column type (adds 'contains' for string columns) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…mented toggle
- Add instance_name string field to mariadb_latency_slow_queries schema so
CI (fresh cluster from sample chart) has the column that filter tests require
- Fix alert-overview-save: make dirty via Add instance instead of cell edit
(td.nth(1) landed on a common-var span with no input)
- Fix nested-deployment: use getByRole('textbox') to skip hidden Segmented
radio inputs that became first in DOM after mode toggle was added; add
10s timeout to modal visibility checks
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Save all test now edits instance_name text (available since schema was extended) rather than adding a row, so subsequent tests still see the seeded 2-row count. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/AlertOverviewWorkspace.jsx`:
- Around line 23-32: Scope the workspace filter state by the full alert-specific
column identity instead of just `name`, because `AlertOverviewWorkspace`
currently dedupes `allVars` by name and `handleColChange`/`getWsOperators`
always use the first matching definition. Update the column lookup logic in
`AlertOverviewWorkspace.jsx` so operator selection and `pendingVarDef` resolve
the correct schema entry for the active alert section, matching the same
identity used by `matchesFilter`.
In `@tests/e2e/alert-filter.spec.js`:
- Around line 252-262: This alert-filter spec is relying on a second deployment
that is never created here, so the tree selection can become order-dependent.
Update the setup around the deployment switcher logic in alert-filter.spec.js to
seed/create the e2e-overview-test/dev folder before using the tree locators,
instead of depending on alert-overview-mode.spec.js. Keep the existing
tree/overviewTestSwitcher/otherDeploy flow, but ensure the second dev node
exists deterministically before the .nth(1) click.
In `@tests/e2e/alert-overview-mode.spec.js`:
- Around line 146-163: The Workspace filter bar tests are depending on
pre-seeded data from another spec instead of preparing their own state, which
makes `openOverviewWithLatencySection()` and the `2 / 2 rows` check flaky in
isolation. Update this `describe` in `alert-overview-mode.spec.js` to seed or
initialize `SEEDED_FOLDER` within the test setup itself (or through a local
setup helper/fixture) before calling `expandToDeployment` and
`clickDeploymentAndWait`, so the latency rows are guaranteed to exist regardless
of test execution order.
In `@tests/e2e/alert-overview-save.spec.js`:
- Around line 59-68: The alert overview save tests are sharing mutated
deployment state because the seeding only happens once in test.beforeAll, which
makes later assertions depend on prior test runs. Move the deployment reset/seed
logic into a per-test setup for the affected cases in
alert-overview-save.spec.js, and reuse the same initialization flow around the
mutating tests so each one starts from a clean deployment state before calling
the deployment save API and asserting row counts.
- Line 6: The test file contains an unused top-level constant, ALERT_TYPE_LABEL,
which will trigger ESLint. Remove the unused declaration from
tests/e2e/alert-overview-save.spec.js and keep the rest of the spec unchanged;
if a label is needed later, reference it only where it is actually used.
In `@tests/e2e/nested-deployment.spec.js`:
- Around line 84-85: The test is incorrectly targeting the first textbox, which
can now be a header filter instead of an editable row cell. Update the nested
deployment save-path test to target a row editor within the table/form context
rather than using page.getByRole('textbox').first(), so the interaction dirties
deployment data reliably. Use the existing test flow around the input selection
in nested-deployment.spec.js and identify the editable cell by its row/editor
role or scoped container instead of the generic first textbox.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9ae0f04e-f9b5-4baa-b00d-d02ed24873d5
📒 Files selected for processing (9)
sample/charts/mariadb-alerts/values.schema.jsonsrc/components/AlertOverviewWorkspace.jsxsrc/components/AlertTable.jsxsrc/pages/AlertUserView.jsxsrc/utils/filterUtils.jstests/e2e/alert-filter.spec.jstests/e2e/alert-overview-mode.spec.jstests/e2e/alert-overview-save.spec.jstests/e2e/nested-deployment.spec.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/AlertUserView.jsx
…eeding, stable selectors - AlertTable: add effectiveFilters prop so workspace filters drive row filtering without leaking into section filter state via FilterHeader spread - AlertOverviewWorkspace: pass sectionFilters (display only) and effectiveFilters (ws + section merged) to SectionPanel/AlertTable separately - alert-filter.spec.js: self-seed e2e-overview-test/dev in Filter state management beforeAll to remove cross-spec dependency - alert-overview-mode.spec.js: self-seed e2e-filter-test/dev in Workspace filter bar beforeAll to remove cross-spec dependency - alert-overview-save.spec.js: use dynamic getCurrentRowCount() instead of hardcoded '3 / 3'; remove unused ALERT_TYPE_LABEL constant - nested-deployment.spec.js: use 'input.ant-input:visible' to skip hidden Segmented radio buttons and potential FilterHeader inputs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
OK to cover both issues in this PR. But the PR lacks unit tests. Add sufficient unit tests to cover new code. |
Move NUM_OPERATORS, STR_OPERATORS, and getWsOperators from AlertOverviewWorkspace into filterUtils so they can be tested in isolation. Add 38 unit tests covering: - getWsOperators: all var types (string, number, integer, numeric enum, string enum, unknown) - matchesFilter: empty/null guards, string contains/=, all numeric ops (>=/<=/>/</=), NaN cell and filter value, numeric enum, string enum, commonValues lookup, multi-filter AND semantics, unknown var fallback Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/filterUtils.js (1)
17-19: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse strict numeric parsing for numeric filters.
parseFloataccepts numeric prefixes, so values like"100ms"or"12abc"are treated as valid numbers here. BecausematchesFilterdrives both row rendering and overview match counts, malformed numeric input can still match rows instead of being rejected.Suggested fix
- const num = parseFloat(cellVal) - const fnum = parseFloat(filter.value) - if (isNaN(num) || isNaN(fnum)) return false + const num = cellVal == null || String(cellVal).trim() === '' ? NaN : Number(cellVal) + const fnum = filter.value == null || String(filter.value).trim() === '' ? NaN : Number(filter.value) + if (Number.isNaN(num) || Number.isNaN(fnum)) return false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/filterUtils.js` around lines 17 - 19, The numeric branch in matchesFilter is too permissive because parseFloat accepts numeric prefixes, so malformed values like "100ms" can still match. Update the numeric parsing in filterUtils to use strict validation for both cellVal and filter.value, rejecting any non-pure numeric input before comparison. Keep the fix scoped to the matchesFilter logic so row rendering and overview match counts only accept valid numeric strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/utils/filterUtils.js`:
- Around line 17-19: The numeric branch in matchesFilter is too permissive
because parseFloat accepts numeric prefixes, so malformed values like "100ms"
can still match. Update the numeric parsing in filterUtils to use strict
validation for both cellVal and filter.value, rejecting any non-pure numeric
input before comparison. Keep the fix scoped to the matchesFilter logic so row
rendering and overview match counts only accept valid numeric strings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8bc2d03f-94fd-457b-ad3b-07688dea1ec3
📒 Files selected for processing (8)
src/components/AlertOverviewWorkspace.jsxsrc/components/AlertTable.jsxsrc/utils/filterUtils.jstests/e2e/alert-filter.spec.jstests/e2e/alert-overview-mode.spec.jstests/e2e/alert-overview-save.spec.jstests/e2e/nested-deployment.spec.jstests/unit/filterUtils.test.js
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/e2e/nested-deployment.spec.js
- tests/e2e/alert-overview-save.spec.js
- src/components/AlertTable.jsx
- src/components/AlertOverviewWorkspace.jsx
- tests/e2e/alert-filter.spec.js
- tests/e2e/alert-overview-mode.spec.js
filterUtils.js: - Rename getWsOperators → getFilterOperators (shared by FilterHeader and WorkspaceFilterBar; update all callers) - Extract mergeFilters() from AlertOverviewWorkspace closure so it is independently testable; update component to call the imported helper filterUtils.test.js (45 tests): - getFilterOperators: undefined / string / number / integer / numeric enum / string enum / unknown type - mergeFilters: pass-through, non-overlapping merge, section override of ws, skip empty-string value, skip null value, empty inputs, immutability guard - matchesFilter: unchanged from previous commit (31 tests) schemaUtils.test.js (30 tests — new file): - schemaAlertNames: happy path, $-prefix exclusion, null/missing schema - getCommonVars: shape, required flags, enum type, missing section, null schema - schemaToVars: common-first ordering, no duplication, default/required/enum propagation, unknown alert, null schema, no items.properties - setCommonVars: set, clear on empty/null, preserve other props, enum prop - varsMapToSchema: full build, omit required when none, multiple alerts - updateSchemaAlert: add new, overwrite existing, immutability, required array Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…guation and strict numeric parsing #10: WorkspaceFilterBar now dedupes columns by name+type. Same-named columns with different types appear as separate options (e.g. threshold (number) / threshold (string)); unique names show without suffix. Filter key remains the column name so matchesFilter per-section type lookup is unaffected. #11: parseFloat → Number() for strict numeric parsing so prefix-numeric strings like "100ms" are rejected (NaN) instead of silently truncated to 100. Null cell values explicitly coerced to NaN to preserve existing null-rejection behaviour. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/filterUtils.test.js (1)
103-104: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThis case doesn't exercise
value: undefined.The current input sets the whole filter entry to
null, so thefilter.value === undefinedguard is still untested.Suggested change
it('returns true when filter value is undefined', () => { - expect(matchesFilter({ instance_name: 'prod' }, { instance_name: null }, [STR_VAR])).toBe(true) + expect(matchesFilter({ instance_name: 'prod' }, { instance_name: { op: 'contains', value: undefined } }, [STR_VAR])).toBe(true) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/filterUtils.test.js` around lines 103 - 104, The test for matchesFilter is not actually covering the filter.value === undefined branch because the filter entry is null instead of an object with an undefined value. Update the unit test in filterUtils.test.js so the case passed into matchesFilter uses a filter object whose value property is undefined, and keep the assertion on the matchesFilter behavior to exercise that specific guard.
♻️ Duplicate comments (1)
src/components/AlertOverviewWorkspace.jsx (1)
47-49: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep workspace filters keyed by the full
name::typeidentity.This still writes
foo::stringandfoo::numberback as the samefooentry, so the second filter overwrites the first and the workspace filter becomes ambiguous again. Persist the composite key (or bothnameandtype) throughwsFiltersinstead of collapsing it here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/AlertOverviewWorkspace.jsx` around lines 47 - 49, The workspace filter update in addFilter is collapsing the selected column identity down to pendingColName, which causes name collisions between different types. Update AlertOverviewWorkspace’s addFilter/onWsFiltersChange flow to persist the full composite identity from the selected field (name::type, or separate name and type fields) in wsFilters instead of using only the name key, so distinct filters remain unambiguous.
🧹 Nitpick comments (1)
tests/unit/filterUtils.test.js (1)
74-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a deep copy for the immutability assertion.
{ ...ws }still sharesws.threshold, so an in-place nested mutation would pass this test.Suggested change
- const wsCopy = { ...ws } + const wsCopy = JSON.parse(JSON.stringify(ws))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/filterUtils.test.js` around lines 74 - 78, The immutability test for mergeFilters is using a shallow copy, so nested mutations on ws.threshold would not be detected. Update the assertion in the does not mutate wsFilters input test to use a deep copy of ws before calling mergeFilters, then compare against that deep snapshot so changes inside nested objects are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/unit/filterUtils.test.js`:
- Around line 103-104: The test for matchesFilter is not actually covering the
filter.value === undefined branch because the filter entry is null instead of an
object with an undefined value. Update the unit test in filterUtils.test.js so
the case passed into matchesFilter uses a filter object whose value property is
undefined, and keep the assertion on the matchesFilter behavior to exercise that
specific guard.
---
Duplicate comments:
In `@src/components/AlertOverviewWorkspace.jsx`:
- Around line 47-49: The workspace filter update in addFilter is collapsing the
selected column identity down to pendingColName, which causes name collisions
between different types. Update AlertOverviewWorkspace’s
addFilter/onWsFiltersChange flow to persist the full composite identity from the
selected field (name::type, or separate name and type fields) in wsFilters
instead of using only the name key, so distinct filters remain unambiguous.
---
Nitpick comments:
In `@tests/unit/filterUtils.test.js`:
- Around line 74-78: The immutability test for mergeFilters is using a shallow
copy, so nested mutations on ws.threshold would not be detected. Update the
assertion in the does not mutate wsFilters input test to use a deep copy of ws
before calling mergeFilters, then compare against that deep snapshot so changes
inside nested objects are caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c2631db4-f7b6-43bc-8849-daa246c456b5
📒 Files selected for processing (5)
src/components/AlertOverviewWorkspace.jsxsrc/components/AlertTable.jsxsrc/utils/filterUtils.jstests/unit/filterUtils.test.jstests/unit/schemaUtils.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/AlertTable.jsx
Contributor
@HahaSula What is an "Alert Type"? Please avoid inventing new terms
After clicking overview, I cannot see these in my mobile browswer. i use android and chrome |
- Fix filter.value=undefined test case (was passing null entry, not {value:undefined})
- Use deep copy (JSON.parse/stringify) for mergeFilters immutability assertion
- Document wsFilters same-name/different-type overwrite as intended behaviour
with two tests covering key collision semantics and per-section type evaluation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Sidebar (alerts panel) was fixed at 300px with no way to hide it on narrow screens, leaving the workspace invisible on mobile. Move the resize handle outside the sidebar div so it remains clickable when collapsed, and add a click-to-toggle behaviour (‹/›) alongside the existing drag-to-resize. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The toggle button was position:absolute inside a flex child, causing it to scroll off-screen when the sidebar tree expanded beyond the viewport height. Switch to position:fixed so it always stays at the bottom-left of the viewport, with left transitioning alongside the sidebar open/close animation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rophy
approved these changes
Jun 29, 2026
This was referenced Jun 29, 2026
4 tasks
This was referenced Jul 5, 2026
HahaSula
added a commit
that referenced
this pull request
Jul 6, 2026
Bump version from 1.3.0 to 1.4.0 to trigger a new image build and GitHub Release via CI. Since 1.3.0: deployment sync (#44), alert overview mode with column filters (#37), promtool validation (#41, #43), plus several fixes — enough new features to warrant a minor bump per this project's convention (1.2.0 -> 1.3.0 similarly bumped minor for the Gitea migration).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
allValuesadded touseEffectdeps so rows load correctly when an alert type is clicked before the deployment API response arrivesTest plan
npm test(unit) passesnpm run test:e2e— 36/38 pass (2 pre-existingnested-deploymentfailures unrelated to this PR)alert-filter.spec.js(10 tests) andalert-overview-mode.spec.js(6 tests) — all 16 pass🤖 Generated with Claude Code
Summary by CodeRabbit
instance_nameper alert instance.