Fix monitor and node error banners - #436
Conversation
Signed-off-by: Argus Li <argus@argusli.dev>
Signed-off-by: ravjotb <ravjot.brar@improving.com>
📝 WalkthroughWalkthroughCluster monitoring now supports partial updates. Successful nodes return applied settings, failed nodes remain visible as partial data, and total failures preserve prior settings. Cluster selectors and activity views distinguish running nodes from node-level errors. ChangesCluster monitoring behavior
Sequence Diagram(s)sequenceDiagram
participant ClusterNodes
participant sendConfigReply
participant configSlice
participant ActivityView
ClusterNodes->>sendConfigReply: Report node update outcomes
sendConfigReply->>configSlice: Return appliedConfig for partial success
configSlice->>ActivityView: Apply settings and partial state
ActivityView->>ActivityView: Show node-level partial errors
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@apps/frontend/src/components/activity-view/ActivityView.tsx`:
- Around line 362-364: Update the HotKeys props in ActivityView to include
selectClusterMonitorNodeErrors(clusterId) in nodeErrors for cluster routes,
while retaining the existing hotKeysNodeErrors behavior as appropriate. Keep
monitorError cleared for clusterId so MonitorNotRunningBanner continues showing
the start button.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0f6e1e32-0072-4763-91f1-198866f182d1
📒 Files selected for processing (8)
apps/frontend/src/components/activity-view/ActivityView.tsxapps/frontend/src/state/valkey-features/config/configSlice.test.tsapps/frontend/src/state/valkey-features/config/configSlice.tsapps/frontend/src/state/valkey-features/monitor/monitorSlice.test.tsapps/frontend/src/state/valkey-features/monitor/monitorSlice.tsapps/server/src/__tests__/config.test.tsapps/server/src/actions/config.tsdocs-site/src/content/docs/features/activity.md
| // Cluster errors are surfaced by NodeErrorsBanner; passing monitorError | ||
| // here would hide the start button in MonitorNotRunningBanner. | ||
| monitorError={clusterId ? null : monitorError} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find every production consumer of the new selector and of monitor errors.
rg -n -C 4 --glob '*.{ts,tsx}' \
'\bselectClusterMonitorNodeErrors\b|\bselectMonitorError\b|monitorError=|nodeErrors=' \
apps/frontend/srcRepository: valkey-io/valkey-admin
Length of output: 13471
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ActivityView relevant lines =="
sed -n '80,115p' apps/frontend/src/components/activity-view/ActivityView.tsx
sed -n '330,372p' apps/frontend/src/components/activity-view/ActivityView.tsx
echo
echo "== HotKeys outline and relevant implementation =="
ast-grep outline apps/frontend/src/components/activity-view/hotkeys/hot-keys.tsx --view expanded || true
sed -n '1,90p' apps/frontend/src/components/activity-view/hotkeys/hot-keys.tsx
echo
echo "== Monitor slice exports =="
sed -n '1,50p' apps/frontend/src/state/valkey-features/monitor/monitorSlice.ts
echo
echo "== NodeErrorsBanner definitions/usages =="
rg -n -C 4 --glob '*.{ts,tsx}' '<NodeErrorsBanner|NodeErrorsBanner\s+|function NodeErrorsBanner|const NodeErrorsBanner|interface NodeErrorsBanner|type NodeErrorsBanner' apps/frontend/srcRepository: valkey-io/valkey-admin
Length of output: 18600
Surface cluster MONITOR failures in HotKeys.
For cluster routes, monitorError is cleared and nodeErrors={hotKeysNodeErrors} does not include selectClusterMonitorNodeErrors. Add selectClusterMonitorNodeErrors(clusterId) and pass it as nodeErrors, while preserving the start button in MonitorNotRunningBanner.
🤖 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 `@apps/frontend/src/components/activity-view/ActivityView.tsx` around lines 362
- 364, Update the HotKeys props in ActivityView to include
selectClusterMonitorNodeErrors(clusterId) in nodeErrors for cluster routes,
while retaining the existing hotKeysNodeErrors behavior as appropriate. Keep
monitorError cleared for clusterId so MonitorNotRunningBanner continues showing
the start button.
* Fix monitor and node error banners. Signed-off-by: Argus Li <argus@argusli.dev> * Remove redundant error banner for clusters, keep for standalone Signed-off-by: ravjotb <ravjot.brar@improving.com> --------- Signed-off-by: Argus Li <argus@argusli.dev> Signed-off-by: ravjotb <ravjot.brar@improving.com> Co-authored-by: ravjotb <ravjot.brar@improving.com>
Description
Addresses #425.
Instead of reading the current node's entry, which made the error invisible if a different node failed, monitorError is derived from the first cluster node whose MONITOR failed to start.
For the Hot Keys config changes not being reflected, the sendConfigReply now includes appliedConfig in the partial reply, so that the frontend will update config even on partial successes.
Change Visualization