Slice 1/5: feat(calm-hub-ui): loading states and link-based navigation by jpgough-ms · Pull Request #3061 · finos/architecture-as-code

jpgough-ms · 2026-09-08T09:56:35Z

Description

Extracted from feat(calmhub): OIDC + SCM (Git) backend support: #3001 ("OIDC + SCM (Git) backend support") — that PR bundled these UI changes in with the GitHub storage backend work, with no connection to auth or GitHub
Layered PR 1 of 5 splitting feat(calmhub): OIDC + SCM (Git) backend support: #3001 into reviewable slices — see the stacking order and full plan in the tracking comment on feat(calmhub): OIDC + SCM (Git) backend support: #3001
This PR targets main directly — it has no dependency on the other slices
Loading states for the explore rail and mobile nav while counts resolve
Namespace/type in the section header now render as links
Sparkline overflow fix (Timeline bar: long moment names block the expand control and clip the moment cards (no truncation) #2728)
Commit authorship preserved: all commits are attributed to @byrash (original PR author), committed by @jpgough-ms as part of this split

Type of Change

♻️ Refactoring (no functional changes)
🐛 Bug fix (non-breaking change which fixes an issue)
🎨 Code style/formatting changes

Affected Components

CALM Hub UI (calm-hub-ui/)

Testing

I have tested my changes locally
All existing tests pass (lint clean, no test regressions)

Checklist

My commits follow the conventional commit format
I have added tests for my changes (if applicable) — n/a, existing tests carried over unchanged
My changes follow the project's coding standards

Split from #3001.

Extracted from #3001 (OIDC + SCM backend support), which bundled these unrelated UI changes with the GitHub storage backend work. - Explore rail and mobile nav show loading state while counts resolve - Namespace/type in the section header render as links - Sparkline overflow fix (#2728) Original-PR: #3001

…arkline changes Addresses the 4 findings posted on this slice's PR review: - Split ExploreRail/MobileNavMenu's combined loading flag into namespacesLoading/domainsLoading so a faster-resolving section no longer waits on the slower one, and the static root-level rows in MobileNavMenu render immediately instead of hiding behind a spinner tied to counts that don't affect them - Anchor the first/last sparkline labels to their column instead of centering them, so they can no longer reach DiagramSection's overflow-hidden card boundary (#2728) — the previous fix removed the track's own overflow but the actual clip was from an ancestor - Removed a dead ternary in ExploreRail.test.tsx that always took its else branch (screen.getAllByClassName isn't a testing-library API); spinners now carry role=status/aria-label, closing a real accessibility gap along the way Also, while in these files: added a muted empty state to both ExploreRail sections (MobileNavMenu already had one), renamed a local loading flag that collided in name with the new counts-loading props, and encodeURIComponent'd a query param for consistency with its neighbour in SectionHeader. Claude-Session: https://claude.ai/code/session_0199XmacMNTrxWL4x5CSXyp1

jpgough-ms · 2026-09-08T10:34:46Z

One item noticed while fixing the loading-state findings, not addressed here: namespaceCountsFailed (Hub.tsx) never reaches ExploreRail or MobileNavMenu, so a failed counts fetch is indistinguishable from "no data" in the UI. Bigger than a loading-state fix and touches Hub's fetch-failure handling more broadly — flagging as a follow-up rather than folding it into this slice.

…ine slice Addresses the 3 findings from a second review pass on this slice's PR: - ExploreRail/MobileNavMenu now render a distinct 'could not load' message when a counts fetch fails, instead of the same 'Nothing here' text used for a genuinely empty list. Threads Hub's existing namespaceCountsFailed through both surfaces, and adds the equivalent domainCountsFailed (which didn't exist before) for symmetry. - Sparkline's #2728 label-clipping fix only anchored the literal first and last dot; with enough versions in a narrow track, an interior dot close to the edge (e.g. the 2nd of many) could still overflow. Replaced the per-index special case with a CSS clamp() applied to every dot, so no label can leave the track regardless of dot count or track width — the true first/last still keep their edge-hugging text alignment, since they always hit the clamp's bound. - Added link role/href coverage for SectionHeader's namespace/type links (including a case with reserved URL characters), which had gained real navigation behaviour with no assertions on it. Claude-Session: https://claude.ai/code/session_0199XmacMNTrxWL4x5CSXyp1

…ap and spinner duplication - domainControlCount/controlDomainCount in Hub.tsx only gated on domainCountsLoaded, not the domainCountsFailed added in the previous round, so a failed domain-counts fetch rendered a confirmed '0 controls' instead of unknown — inconsistent with the comment right above claiming it mirrors activeNamespaceCounts, which does check the failed flag. Both now gate on both flags; added a Hub-level test simulating the failure via a spy on the mocked CountsService. - Extracted the identical spinner span duplicated across ExploreRail and MobileNavMenu (both touched by this same PR) into a shared LoadingSpinner, so a future style change can't drift between them. Each component keeps its own wrapper/spacing, which differ by context (rail div vs mobile list item). Claude-Session: https://claude.ai/code/session_0199XmacMNTrxWL4x5CSXyp1

jpgough-ms · 2026-09-08T11:05:08Z

One more finding from a third review pass (couldn't anchor inline — the diff position isn't resolvable across the intermediate commits):

calm-hub-ui/src/hub/Hub.tsx — domainControlCount/controlDomainCount only gated on domainCountsLoaded, not the domainCountsFailed added in the previous round, so a failed domain-counts fetch rendered a confirmed "0 controls" via DomainPage instead of unknown. The comment directly above claimed this mirrors activeNamespaceCounts, which does check the failed flag — this didn't. Fixed in 84827d3, with a Hub-level test simulating the failure via a spy on the mocked CountsService.

…mbiguous filter-empty state - MobileNavMenu's failure message said 'try again' but there's no retry path (Hub fetches counts once on mount) — replaced with level-specific wording matching ExploreRail's equivalent copy. - ExploreRail's 'Nothing here' fired identically whether the filter matched nothing or there were genuinely no namespaces; the two are now distinguished. Not addressed, noted for the record: - The same unknown-vs-zero distinction doesn't reach MobileNavMenu's leaf-level fetches (resource types, controls) — those catch blocks predate this PR and are unrelated to the counts-loading fix this slice makes; flagged as a separate follow-up on the PR. - Sparkline's label clamp() inverts if the track ever drops under 120px; not reachable at any current desktop or mobile viewport in this codebase per review verification, so left as a known theoretical edge rather than adding a speculative width guard. Claude-Session: https://claude.ai/code/session_0199XmacMNTrxWL4x5CSXyp1

jpgough-ms · 2026-09-08T11:19:06Z

Two more items from a fourth review pass:

Not addressed — pre-existing, out of scope: the unknown-vs-zero distinction this slice adds for the namespace/domain counts doesn't reach MobileNavMenu's leaf-level fetches (resource types within a namespace, controls within a domain). Those .catch(() => finish([]))/.catch(() => setLeafItems([])) blocks predate this PR unchanged — adding *Failed tracking for four more service calls is a separate, larger piece of work unrelated to the counts-loading fix this slice makes. Flagging as a follow-up rather than folding it in here.
Not addressed — currently unreachable: Sparkline's label clamp() inverts if the inner track ever drops under 120px (all labels would pin to the left edge). Review verified this isn't reachable at any current desktop (≥1024px) or mobile viewport in this codebase. Left as a known theoretical edge rather than adding a speculative width guard for a layout constraint that doesn't exist yet.

…rflow and generic spinner label - Removing the sparkline track's overflow:hidden (for the #2728 clamp() fix) also dropped vertical containment on the validFrom date row, which had no whiteSpace/overflow bound of its own — an unusually long value could now bleed past the track instead of being clipped. Gave it the same single-line ellipsis treatment the label row already has. - MobileNavMenu's drill-down spinner used a bare 'Loading' label regardless of which section was loading, unlike ExploreRail's section-specific labels on the desktop equivalent. Now says 'Loading namespaces'/'Loading control domains' to match. Declined as architecture/DRY preferences rather than defects (noted on the PR): generalizing the namespace/domain loading-failed state into a shared async-resource abstraction, and deduplicating the loading/failed/empty branch between ExploreRail and MobileNavMenu. Both are legitimate refactors but out of scope for a review-fix pass — the current duplication is small (a few lines) and each component's render differs enough (div vs li, different spacing) that a shared abstraction would add its own indirection cost. Claude-Session: https://claude.ai/code/session_0199XmacMNTrxWL4x5CSXyp1

jpgough-ms · 2026-09-08T11:27:56Z

A fifth review pass found two more real, fixed in 1dba077:

Removing the track's overflow: hidden (for the Timeline bar: long moment names block the expand control and clip the moment cards (no truncation) #2728 clamp fix) also dropped vertical containment on the validFrom date row, which had no bound of its own — an unusually long value could bleed past the track. Gave it the same single-line ellipsis treatment the label row already has.
MobileNavMenu's drill-down spinner used a bare "Loading" regardless of section, unlike ExploreRail's specific labels. Now matches.

Five findings from this same pass were architecture/DRY preferences, not defects — declining rather than churning the diff further:

Generalizing namespace/domain loading+failed state into a shared async-resource abstraction (Hub.tsx)
A shared hook/component for the loading→failed→empty→list branch, currently hand-written once in each of ExploreRail and MobileNavMenu
Calling encodeURIComponent(namespace) three times in SectionHeader instead of memoizing it once
RailEmpty (ExploreRail) vs MobileNavMenu's inline empty-state <li> being separately maintained
ExploreRail.tsx crossing this repo's ~150-line component-file guideline (158 lines) by defining RailSpinner/RailEmpty inline rather than as separate files — the reviewer itself flagged this as low-confidence/a judgment call

All are legitimate refactors, but they generalize working code rather than fix something broken, and each component's actual render differs enough (div vs li wrapper, different spacing, different sections) that a shared abstraction adds its own indirection cost. Open to revisiting if a maintainer wants them as a separate follow-up.

rocketstack-matt

Reviewed independently (own critical + idiom pass, full local build/lint/test run, and a live visual check of the #2728 sparkline fix against a seeded long-name timeline). No defects found beyond what your own five self-review rounds already caught and fixed — all 10 threads check out against the code. Nit: the PR description's Type of Change / Testing checkboxes are stale (still say "refactoring, no functional changes" / tests n/a) relative to what actually shipped across the review rounds — worth a quick update before merge, non-blocking.

jpgough-ms requested a review from a team as a code owner September 8, 2026 09:56

github-actions Bot added the calm-hub-ui Affects `calm-hub-ui` label Sep 8, 2026

This was referenced Sep 8, 2026

Slice 2/5: build(calm-hub): add observability and GitHub-backend dependencies #3063

Merged

feat(calmhub): OIDC + SCM (Git) backend support: #3001

Open

jpgough-ms changed the title ~~feat(calm-hub-ui): loading states and link-based navigation~~ Slice 1/5: feat(calm-hub-ui): loading states and link-based navigation Sep 8, 2026