Improved overview & shadowlink pages for users w/o permissions#2540
Conversation
✅ Clean — no registry drift, off-token colours, or ad-hoc classesApp:
Generated by lookout audit-changes. |
malinskibeniamin
left a comment
malinskibeniamin
left a comment
There was a problem hiding this comment.
/review — PR #2540 (Improved overview & shadowlink pages for users w/o permissions)
Multi-hat review (ponytail · resilience · regular · adversarial · visual · test-perf · security/privacy). Inline comments below (P1/P3).
P2 — Overview changes ship without tests. The shadowlink permission flow got two solid test files, but the new overview logic (the unhealthy-alert trigger, the N/A fallbacks, the changed skeleton gating) has no overview.test.tsx. Add one asserting the alert fires only for the intended state and that N/A renders when api.brokers is null — it would pin the alert-trigger condition flagged in the P1 below.
Verified safe (no finding): the ClusterDetails skeleton-gating change is correctly guarded (brokerList = brokers ?? [], hasBrokers), so rendering with null brokers doesn't crash. The shadowlink PermissionDenied handling (component + route loader) is correct and well-tested.
PR value: MEDIUM–HIGH — real UX win (no raw error boundary for no-permission users; honest N/A instead of a perpetual "...").
Needs attention: 1×P1 (alert over-trigger/copy), 1×P2 (tests), 1×P3.
|
|
||
| <PageContent className="overviewGrid"> | ||
| {isClusterUnhealthy && ( | ||
| <Alert className="mb-4" icon={<TriangleAlertIcon />} variant="destructive"> |
There was a problem hiding this comment.
P1 — The new "Cluster metrics unavailable" alert over-triggers, and its copy asserts the wrong cause
What: The alert renders whenever isClusterUnhealthy = clusterStatus.className === 'status-red' (line 143). status-red is the catch-all for any status that isn't HEALTHY/DEGRADED — which includes StatusType.UNSPECIFIED (enum 0), an unset/undefined status, and a genuinely-unhealthy but reachable cluster the user has full permission to view.
Why: On master there was no such banner (status-red only colored the status bar). This net-new destructive banner asserts "the cluster is unreachable, or that your account lacks permission" — wrong for a reachable, permitted, merely-degraded cluster, and it contradicts both the adjacent code comment (which says not to assert a cause) and the real broker/health data shown right below it. Only the optional statusReason line mitigates, and it's frequently empty.
Fix: Gate the banner on a signal that actually means unreachable/forbidden (e.g. api.brokers == null), or reword so it leads with "cluster is reporting an unhealthy status" and presents unreachable/permission as secondary possibilities.
One-shot: Narrow the isClusterUnhealthy alert condition (overview.tsx:143/154) so it doesn't fire for UNSPECIFIED/undefined or reachable-but-unhealthy clusters, and soften the copy accordingly.
There was a problem hiding this comment.
I've removed that banner altogether.
| that your account lacks permission to view it. | ||
| </p> | ||
| {clusterStatusReason ? <p className="font-medium">Reported reason: {clusterStatusReason}</p> : null} | ||
| <UiButton className="mt-1" onClick={() => this.refreshData(true)} size="sm" variant="secondary"> |
There was a problem hiding this comment.
P3 — Retry uses the least-prominent button variant on a destructive surface
What: The Retry control is variant="secondary" inside a variant="destructive" Alert; the sibling shadowlink error state uses a more prominent variant.
Why: The one recovery action in the banner is the least visually prominent option on the red surface, and is inconsistent with the other retry affordances introduced in this PR.
Fix: Use variant="outline" (or primary) for the in-alert Retry.
One-shot: Bump the in-Alert Retry button (overview.tsx:162) from secondary to outline/primary.
The banner was newly introduced and not present before; keep the N/A metric fallbacks but drop the destructive Alert.
2 participants
Before:
After:
