Hypershell-112 UI with adjustments - #214
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Amber reviewStatus: Complete VerdictCOMMENT — the operational-dashboard feature is well-structured (clean hexagonal boundary, domain probes, correlation IDs, thorough component/CI registration, additive tests) and I found no blocker. However, this PR overlaps almost entirely with PR #209 and takes a dashboard-plumbing design that conflicts with PR #211, so maintainers must decide which PR owns this feature before merge. A stray unrelated planning file and an authorization-claim behavior change also warrant attention. Hi team — Amber here. This is a large, mostly-new web-console feature adding an operational dashboard behind a |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
COMMENT — the operational-dashboard feature is well-structured (clean hexagonal boundary, domain probes, correlation IDs, thorough component/CI registration, additive tests) and I found no blocker. However, this PR overlaps almost entirely with PR #209 and takes a dashboard-plumbing design that conflicts with PR #211, so maintainers must decide which PR owns this feature before merge. A stray unrelated planning file and an authorization-claim behavior change also warrant attention.
Hi team — Amber here. This is a large, mostly-new web-console feature adding an operational dashboard behind a packages/operational-dashboard-ui package, an admin gate (client + BFF), and full CI/registration wiring. Overall quality is good; my concerns are concentrated in cross-PR coordination, one accidental file, and authorization-claim semantics.
Cross-PR coordination
I compared this PR against all other open PRs in openshift-online/hypershell. Open PRs at review time: #73, #75, #109, #135, #148, #150, #151, #179, #182, #185, #188, #189, #194, #200, #201, #206, #207, #208, #209, #210, #211, #212, #216.
Two material conflicts need a maintainer decision:
-
PR #209 "HYPERSHELL-112 Dashboard UI" — duplicate solution. #209 and this PR (#214 "Hypershell-112 UI with adjustments") implement the same HYPERSHELL-112 feature. The changed-file lists are near-identical (same
packages/operational-dashboard-uipackage, same web-console shell/route/BFF/role wiring, same i18n and CI registration). #214 is a superset — it addsgateway-status-chart.tsx,gateway-exception-status-counts.ts,metric-trend-change.ts, and an error-state Storybook story. Decision needed: these cannot both merge. Maintainers should pick #214 as the successor and close #209 (or explicitly rebase one onto the other), otherwise the second to merge will collide on every shared file. -
PR #211 "fix(kind): expose gateway metrics and fix control plane connectivity" — incompatible dashboard data-plumbing design. Both PRs create
components/web-console/app/composition/dashboard-composition.tsas a new file with divergent, incompatible contents:- This PR:
dashboardOperations = createDashboardOperations({ controlPlane: createMockDashboardControlPlane() }), importingcreateDashboardOperationsfrom the new@openshift-online/hypershell-operational-dashboard-uipackage and a mock adapter underapp/adapters/mock/. - PR #211: defines its own local
DashboardOperationsinterface and wires a live adaptercreateDashboardControlPlaneAdapter()fromapp/adapters/api/dashboard-control-plane.ts, backed by Prometheus + agateway-management-uimetrics dashboard — it does not use the newoperational-dashboard-uipackage at all.
This is not a mere text merge conflict: the two PRs assume different owning package, different composition interface, and different data source (mock vs live Prometheus). This PR's body says "live API integration can follow in a later PR," which suggests #211 is meant to supply the live data — but as written they disagree on the composition contract and adapter location. Decision needed: agree on the canonical dashboard composition/adapter contract and a merge order (which package owns the dashboard; whether #211 layers its live adapter onto #214'screateDashboardOperationsport rather than replacing the composition).
- This PR:
No material conflict was found with the other open PRs (dependency bumps #73/#75/#135/#188/#189; spec/docs #148/#151/#185/#200; control-plane/API #179/#182/#194/#201/#207/#216; CLI #206; e2e #212; security tooling #109). PR #208/#210 touch gateway-management-ui gateway-connection files but do not overlap this PR's changes.
Findings
Major
-
Stray unrelated file
packages/gateway-management-ui/plan.md(187 lines). Its content is an implementation plan for "Update Daily Note - Jira" (branch004-daily-jira-items,.agents/skills/, Jira REST API) — it has nothing to do with HyperShell or this feature and appears to be an accidental commit. It lands inside the sharedgateway-management-uipackage (included in that component's lint/path scope). Please delete it. Confidence: High. -
extractRealmRolesnow falls back to thegroupsclaim for authorization (bff/src/auth.ts:69-79). The admin gate matches exact literalshypershell-admins/platform:admin. Keycloak group memberships are commonly emitted as paths with a leading slash (e.g./hypershell-admins), which would silently fail to match, and conflating group names with realm-role names for a privilege decision can also over-grant if group and role namespaces differ. Since this feeds the dashboard admin gate, please confirm the exact Keycloak claim/format this targets and normalize (or scope the fallback) so the authorization decision is deterministic. Confidence: Medium.
Minor
-
Client-side admin gate falls through for unauthenticated/undefined sessions (
app/features/dashboard/require-dashboard-admin.tsx:32). The deny branch only triggers whensession?.authenticatedis true; an unauthenticated or failed-load session returnschildren. Server-side BFF enforcement covers the real boundary, so this is defense-in-depth only, but consider an explicit not-authorized/redirect for the non-authenticated case rather than rendering the dashboard shell. Confidence: High. -
domain-probespackage publictypesentry repointed fromdist/*.d.tstosrc/*.ts(components/web-console/domain-probes/package.json:8,11,16). This changes type resolution for every consumer of a shared package and is bundled into a feature PR. It's likely intentional (so the new package type-checks against source without a prior build), but call it out explicitly and confirm it doesn't break published-artifact consumers or build ordering. Confidence: Medium. -
Admin gate is only enforced when
config.oidcIssueris set (bff/src/app.ts:379). In no-auth dev mode the dashboard is open to everyone — acceptable for local dev, but worth a comment so it isn't mistaken for production behavior. Confidence: High.
What looks good
- New package respects the narrow hexagonal boundary: control-plane access is behind a
DashboardControlPlaneport, workflow effects go through aDashboardProbePublisherfan-out, correlation IDs are generated per invocation, and there are no rawconsole.*/telemetry calls in the package source. - Component registration is complete and consistent:
component-paths.json,lint.yml(job + aggregate gate),pnpm-workspace.yaml, rootpackage.jsonbuild/check scripts,swap-component.sh, andCLAUDE.mdall updated together. - Test changes are additive — no pre-existing assertion was weakened or flipped. New BFF tests cover non-admin redirect and admin allow for both roles; the existing unauthenticated-redirect test was extended (not rewritten) to include
/dashboard.
Findings Summary (ordered by severity, highest first):
- [Major] Cross-PR: duplicate of #209 and incompatible dashboard composition vs #211 — needs maintainer decision — Cross-PR Coordination
- [Major] Stray unrelated
packages/gateway-management-ui/plan.mdaccidentally committed — Repo Hygiene - [Major]
extractRealmRolesgroups-claim fallback may misfire authorization (Keycloak group path format) — Security / AuthZ (auth.ts L69) - [Minor] Client admin gate falls through for unauthenticated sessions — Defense in Depth (require-dashboard-admin.tsx L32)
- [Minor]
domain-probestypesrepointed to source.ts— shared-package contract change — API Surface (package.json L8) - [Minor] Dashboard admin gate bypassed when OIDC disabled (dev mode) — Observability/Docs (app.ts L379)
Convention Checklist (only evaluated rows):
| Convention | Result |
|---|---|
No panic() / no raw console.* in production code |
Pass |
| No secrets in logs or responses | Pass |
| Input validated (auth roles/claims) | Fail |
| Narrow hexagonal UI boundary (ports for external effects) | Pass |
| Domain probes for UI observability | Pass |
| Reuse PatternFly / no duplicate UI components | Pass |
| Register every component in CI | Pass |
| Image references consistent across manifests | N/A |
| Test Diff Scrutiny (no weakened assertions) | Pass |
a4ecf2d to
8393fd2
Compare
Amber reviewStatus: Complete VerdictCOMMENT — This is a well-structured, hexagonally-boundaried operational dashboard package with solid BFF+client admin gating and good test coverage; the concerns are around shipping mock data as if it were live, a dev-only hardcoded hostname, and a few observability/brand nits. Nothing here rises to a blocker, but the mock-data and hostname items warrant a maintainer decision before this becomes visible to admins. The new Findings[Major] Mock metrics are wired as the production dependency. [Major] Dev-only hardcoded hostname in [Minor] Raw adapter error message rendered in the UI. [Minor] Declared [Minor] Hardcoded hex color literals in the status chart. Cross-PR coordinationNo material cross-PR coordination issue requires maintainer action. Findings Summary (ordered by severity, highest first)
Convention Checklist
|
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
COMMENT — This is a well-structured, hexagonally-boundaried operational dashboard package with solid BFF+client admin gating and good test coverage; the concerns are around shipping mock data as if it were live, a dev-only hardcoded hostname, and a few observability/brand nits. Nothing here rises to a blocker, but the mock-data and hostname items warrant a maintainer decision before this becomes visible to admins.
The new @openshift-online/hypershell-operational-dashboard-ui package is cleanly layered (application ports, probes, presentation), CI registration is complete (component-paths, lint.yml, aggregate gate, Dockerfile, .dockerignore, workspace), and the admin gate is enforced in both the BFF (/dashboard + dashboard. host) and the client (RequireDashboardAdmin) with additive, well-targeted tests. Test diff scrutiny passed: the modified assertions in session-adapter.test.ts and bff/test/auth.test.ts are additive (new authEnabled field, new admin-gate cases) and do not flip or delete any prior guarantee.
Findings
[Major] Mock metrics are wired as the production dependency.
components/web-console/app/composition/dashboard-composition.ts injects createMockDashboardControlPlane() as the real controlPlane, so authenticated admins are shown fabricated numbers (with an artificial 2s delay) presented as live operational metrics. The PR body acknowledges this is interim, but there is no visible "sample data" indicator or feature flag. Admins could make operational judgments on fake data. Recommend gating the route behind a feature flag or rendering an unmistakable "sample data" banner until the live adapter lands.
[Major] Dev-only hardcoded hostname in home.tsx diverges from the BFF host model.
components/web-console/app/routes/home.tsx:22 decides whether / renders the dashboard via globalThis.location.hostname === "dashboard.hypershell.localhost", while the BFF (bff/src/app.ts) uses a generic dashboard. prefix (isDashboardHost). In any non-Kind environment (e.g. dashboard.<prod-domain>) the SPA at / on the dashboard host will render the Gateways page instead of the dashboard, so client behavior silently disagrees with the BFF gate. This violates "Separate configuration from code." Derive the check from the same dashboard. prefix logic or from runtime config rather than a literal localhost.
[Minor] Raw adapter error message rendered in the UI.
packages/operational-dashboard-ui/src/pages/operational-dashboard-page.tsx:472 and :482 render metricsQuery.error.message directly into the Alert. Once a live control-plane adapter exists, this can surface internal/backend detail to the browser and is unlocalized. Prefer the localized loadErrorBody/refreshErrorBody for display and route the raw message to a probe/log.
[Minor] Declared dashboard.layout.template.invalid probe is never emitted.
operational-dashboard-page.tsx:322 silently falls back to the default template when isValidSavedTemplate fails (and the surrounding catch also swallows parse errors), yet the probe contract declares dashboard.layout.template.invalid. Emit that probe on the invalid/parse-failure paths so the declared observability signal is actually produced, or drop the unused probe name.
[Minor] Hardcoded hex color literals in the status chart.
packages/operational-dashboard-ui/src/dashboard/gateway-status-chart.tsx:20-25 hardcodes hex values. They do map to approved palette swatches and correctly avoid using red for failure (failure uses danger-orange), which is good — but UI-BRAND-02 asks for named design tokens and flags hard-coded duplicates. Prefer PatternFly chart theme tokens over literal hex.
Cross-PR coordination
No material cross-PR coordination issue requires maintainer action.
Findings Summary (ordered by severity, highest first)
- [Major] Mock control-plane adapter wired as the production dependency; admins see fabricated metrics without a "sample data" indicator - Trust / Product (dashboard-composition.ts L6, mock/dashboard-control-plane.ts L12)
- [Major] Hardcoded
dashboard.hypershell.localhostinhome.tsxdiverges from the BFF's genericdashboard.host model; dashboard won't render on/in non-Kind envs - Config-from-code (home.tsx L22) - [Minor] Raw, unlocalized
error.messagerendered in dashboard Alerts - Observability / i18n (operational-dashboard-page.tsx L472, L482) - [Minor] Declared
dashboard.layout.template.invalidprobe never emitted on the invalid/parse-failure path - Observability (operational-dashboard-page.tsx L322) - [Minor] Hardcoded hex chart colors instead of named design tokens - Brand color (gateway-status-chart.tsx L20-25)
Convention Checklist
| Convention | Result |
|---|---|
No panic() / no raw console in production code |
Pass |
| Errors handled, not silently swallowed | Pass (probe-backed) |
| No secrets in logs or responses | Pass |
| Restricted admin access enforced server-side (BFF) and client-side | Pass |
| Test Diff Scrutiny (no flipped/removed guarantees) | Pass |
| Component registered in CI (paths, lint job, aggregate, Docker) | Pass |
| Separate configuration from code | Fail (hardcoded dashboard hostname) |
| Narrow hexagonal UI boundary (ports for effects) | Pass |
| Domain probes for observability (no raw telemetry) | Pass |
| PatternFly reuse / named brand tokens | Fail (hardcoded hex) |
| i18n for user-facing strings | Partial (raw error.message unlocalized) |
| import { createMockDashboardControlPlane } from "../adapters/mock/dashboard-control-plane"; | ||
|
|
||
| export const dashboardOperations = createDashboardOperations({ | ||
| controlPlane: createMockDashboardControlPlane(), |
There was a problem hiding this comment.
[Major] The mock control-plane adapter is wired here as the production dependency, so authenticated admins are shown fabricated metrics (see the artificial 2s delay in mock/dashboard-control-plane.ts) presented as live data. The PR body notes this is interim, but there's no visible "sample data" indicator or feature flag. Consider gating the route behind a flag or rendering an unmistakable sample-data banner until the live adapter lands, so admins don't act on fake numbers.
| const navigate = useNavigate(); | ||
| const [searchParameters, setSearchParameters] = useSearchParams(); | ||
| const hostname = globalThis.location.hostname; | ||
| if (hostname === "dashboard.hypershell.localhost") { |
There was a problem hiding this comment.
[Major] This hardcodes the Kind-only hostname dashboard.hypershell.localhost, while the BFF (bff/src/app.ts isDashboardHost) uses a generic dashboard. prefix. In any non-Kind environment (e.g. dashboard.<prod-domain>) the SPA at / on the dashboard host will render the Gateways page instead of the dashboard, so client behavior silently disagrees with the BFF gate. This violates "Separate configuration from code" - derive the check from the same dashboard. prefix logic or runtime config.
| variant="danger" | ||
| > | ||
| {metricsQuery.error instanceof Error | ||
| ? metricsQuery.error.message |
There was a problem hiding this comment.
[Minor] Rendering metricsQuery.error.message verbatim (here and at ~L482) can surface internal/backend detail to the browser once a live adapter exists, and it's unlocalized. Prefer the localized loadErrorBody/refreshErrorBody for display and route the raw message to a probe/log.
| } | ||
|
|
||
| const parsed = JSON.parse(rawTemplate) as ExtendedTemplateConfig; | ||
| if (!isValidSavedTemplate(parsed)) { |
There was a problem hiding this comment.
[Minor] When a saved template is invalid this falls back to default silently (and the surrounding catch also swallows parse errors), yet the probe contract declares dashboard.layout.template.invalid. Emit that probe on the invalid/parse-failure paths so the declared observability signal is actually produced, or drop the unused probe name.
| * | ||
| * @see https://www.patternfly.org/components/alert | ||
| * @see https://www.patternfly.org/components/label | ||
| */ |
There was a problem hiding this comment.
[Minor] These hex literals map to approved palette swatches and correctly avoid red for failure (uses danger-orange), which is good - but UI-BRAND-02 asks for named design tokens and flags hard-coded duplicates. Prefer PatternFly chart theme tokens over literal hex.
2 participants
Summary
@openshift-online/hypershell-operational-dashboard-uipackage (widgetized layout, utilization charts, summary metrics).hypershell-adminsorplatform:adminrealm roles.Changes
packages/operational-dashboard-uipackage with dashboard page, charts, and mock metrics fixture.RequireDashboardAdmin).Metrics are served from a mock control plane adapter for now; live API integration can follow in a later PR.
Test plan
pnpm --filter @openshift-online/hypershell-operational-dashboard-ui checkpnpm --filter @openshift-online/hypershell-web-console test:run(or the web-console unit test command you normally use)make kind-web-console-up— and go to: https://console.hypershell.localhost/dashboard OR run story book:cd components/web-console && pnpm run storybook