Repository navigation
feat(dashboard): move Repositories from the Marketplace to Admin - #4286
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughRepositories now render in the Admin portal at ChangesRepositories Admin Routing
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~12 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant App
participant MarketplacePage
participant AdminPage
participant TapsPage
App->>MarketplacePage: match /marketplace/taps
MarketplacePage->>AdminPage: redirect to /admin/taps
AdminPage->>TapsPage: render Repositories
Merge Risk: ⚪ Minimal · up to Repositories remain accessible under Admin, and the legacy Marketplace URL redirects correctly without an established merge-blocking risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Carry cozystack/cozystack#4286 into the demo as a patch so the demo already shows Repositories in the Admin portal. The patch is temporary: once that change is on main it no longer applies, the build stops with "upstream drifted", and the file goes. Assisted-by: LLM Signed-off-by: Andrei Kvapil <[email protected]>
Build the /demo/ console from the cozystack monorepo instead of the archived cozystack-ui, and bring the mock layer up to the current console. - build.sh clones packages/system/dashboard/images/console sparse and labels the header; the refresh workflow watches static/demo-app. - The smoke check sees un-mocked calls now and crawls 250 screens. - Mock Repositories, Migration, KubeVirt power state and the option sources behind the create forms; populate every instance's tabs. - Resolve inherited module status, drop sign out, and show Repositories under Admin ahead of cozystack/cozystack#4286 through a temporary patch. Assisted-by: LLM Signed-off-by: Andrei Kvapil <[email protected]>
The demo was still built from cozystack/cozystack-ui. That repo was archived in June when the console moved into the monorepo, under `packages/system/dashboard/images/console`. The clone kept working, so nothing ever failed, the weekly refresh just rebuilt a June snapshot every Monday. `demo-src/build.sh` clones the monorepo sparse now and lifts the console tree into a repo of its own, so the overlay and the patches stay rooted where they were. All four patches applied unchanged. The workflow was also diffing `static/demo`, which the build stopped writing to when the bundle moved to `static/demo-app`. It would never have opened a PR. The smoke check could not fail on a screen the mock layer had never heard of. MSW passes unhandled requests to the network, and the test server answered any unknown path with index.html, so an un-mocked call came back 200 text/html and looked fine. It answers 404 on /api and /apis now, and crawls three waves out instead of one, 250 destinations instead of 88. That found three endpoints added upstream since June and never mocked: - `core.cozystack.io` taps, Marketplace -> Repositories - `forklift.cozystack.io` vmimportsources and vmimporttasks, Console -> Migration Both are mocked now, with the CRD schemas their create forms read and the option sources behind their dropdowns. Instance detail tabs were also populated for 5 of the 20 demo instances, the capture only covered a few in depth. Pods, services and volumes are synthesized at read time now for whatever the fixtures don't carry. Checked on the built bundle by hand, not only by exit code: marketplace, repositories, both migration screens, import detail, VM and disk pages, admin. Deployed a Redis from the marketplace and watched it install and turn ready. Picked a connection in the New import form, the dependent machine list loads. VNC can't work, there is no VM behind it. Imported machines report as stopped, so the console says "not running, start it to use the VNC console" instead of failing to connect. One thing I left alone: the marketplace catalog is a snapshot of a live cluster taken on 12 August, 27 app types with create-form schemas of that date. Upstream ships 32 now. Generating it from `packages/system/*-rd/cozyrds/*.yaml` at build time would keep it in step. Admin -> Modules showed four modules as Unknown. The status is looked up at whichever tenant provides the module, `tenant-root` here per the `namespace.cozystack.io/<module>` labels, and the registry only carried workshop00's own info module. Added the four TenantModules in tenant-root, they render as inherited from an ancestor the demo user cannot see. The smoke check cannot catch that one, nothing 404s, the badge just says Unknown. The header carries a version label now, where a release build shows its version. It is pinned to `v1.7.0-rc.2` in the refresh workflow; without `DEMO_VERSION` a branch build reads as `main@<sha>` and a tag build as the tag. The demo also shows Repositories under Admin already. That comes from cozystack/cozystack#4286, carried as a temporary patch (`06-repositories-in-admin.patch`). Once #4286 is merged the patch stops applying and the build stops with "upstream drifted", then the file just gets deleted. Sign out is gone from the demo. It was the only entry in the user menu and sent the page to /oauth2/sign_out, which nothing serves on the site.
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM with non-blocking notes
Checked out at b83e5e2 and ran the console suite: 341 tests and typecheck green. The four tests the PR adds are not decorative — reverting the redirect, the /admin/taps route, the Administration sidebar entry, or the Marketplace removal each turns the suite red, one mutation per change.
Upgrade path holds: the redirect keeps an existing bookmark working instead of handing /marketplace/taps to the :appName catch-all, and grep finds no other reference to the old path in the tree. Access is unchanged, as the description says — the entry was ungated in the Marketplace and the Administration section carries no gate either, so Tap RBAC stays the only thing deciding who gets a list back. The website follow-up (cozystack/website#700) names Admin → Administration → Repositories, which matches where the entry actually renders.
Findings
- [NIT]
packages/system/dashboard/images/console/apps/console/src/routes/MarketplacePage.tsx:10, the comment claims an ordering constraint react-router does not have
Checked and correct
Sidebar highlight, the alsoMatch list on Modules, and the Console sidebar are untouched by the move; /admin/taps resolves for a user with neither operator area (your own test covers it); the Package icon import stays live through the Administration entry.
| <Route path="c/:category" element={<MarketplaceList />} /> | ||
| {/* Static route must precede the :appName catch-all. */} | ||
| <Route path="taps" element={<TapsPage />} /> | ||
| {/* Repositories moved to Admin; must precede the :appName catch-all. */} |
There was a problem hiding this comment.
[NIT] the comment claims an ordering constraint react-router does not have
{/* Repositories moved to Admin; must precede the :appName catch-all. */} says placement matters. It does not: react-router ranks routes by specificity rather than by source order, so a static segment beats a dynamic one wherever it sits.
Moved the redirect below the catch-all and reran the two routing suites:
$ pnpm vitest run src/App.routing.test.tsx src/routes/AdminPage.routing.test.tsx
Tests 9 passed (9)
Same 9 green with the route above the catch-all. The same mutation on AdminPage.tsx, moving taps below :plural, also stays green.
The claim predates this PR (main carries Static route must precede the :appName catch-all. on the same line), so this is inherited rather than introduced, but the PR rewrites the sentence and keeps the false half. Either drop the ordering clause and say why the route exists at all, or keep it and pin it with a test that fails when the order changes — right now nothing holds it, and a reader who believes it will preserve an order that carries no meaning.
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM. Access does not change, and the three new test files fail when you revert what they cover.
Business context: connecting an External-Apps repository is a cluster-wide operator action, so Repositories moves out of the Marketplace catalog into the Admin portal.
Admin is not gated at all. useCanSeeAdmin() returns true with no condition, App.tsx adds the Admin tab on that alone, and the new taps route sits outside both CapacityAdminGuard and BackupClassAdminGuard. "Who can open it doesn't change" is exact.
This is also not the CRD case that hit the backup and migration forms. Those go through useCRDSchema, a cluster-scoped by-name get on apiextensions.k8s.io. useTaps lists core.cozystack.io/taps and reads no CRD object.
UI Test runs on pull_request for packages/system/dashboard/images/console/**, so a red test here fails the job. 17 test blocks in the three files, 17 executed. I reverted each claim separately (route element, route path, redirect target, sidebar entry back under Marketplace) and every one goes red. Renaming the route path is the one that counts, since it proves the :plural catch-all is not quietly answering the heading assertion.
Red E2E is inherited: the mongodb, postgres, rabbitmq and redis backup roundtrips, two kafka suites and opensearch, with overlay drift on backupstrategy-controller/values.yaml reported in Finalize. A console-only change reaches none of it.
Two nits.
MarketplacePage.tsx:10. The ordering claim was already flagged, so here is a check for it: move the taps route below :appName and all 17 tests stay green, because react-router ranks a static segment above a dynamic one whatever the source order. The other half of the line has the reverse problem. "Repositories moved to Admin" describes an edit, and after merge it becomes a note about something that already happened. The part worth keeping is why the redirect exists instead of just dropping the route, and the commit body already covers that.
sidebar-sections.test.tsx:135 pins the entry and its to, not the section holding it. findItem walks every section flat, so moving Repositories out of Administration into any other always-rendered section keeps it green. sectionIndex is already in the file.
b83e5e2 to
bf933c0
Compare
Connecting an External-Apps repository creates a cluster-scoped Tap and runs its charts in the management cluster. That is an operator action, not something a tenant does while browsing the catalog, so the entry belongs next to the other cluster-wide areas in the Admin portal rather than in the Marketplace sidebar. The page moves to /admin/taps under Administration. /marketplace/taps redirects there, since the :appName catch-all would otherwise render an order page for an application called "taps". Assisted-by: LLM Signed-off-by: Andrei Kvapil <[email protected]>
bf933c0 to
5692297
Compare
The dashboard moves the Repositories view from the Marketplace sidebar to Admin, under Administration (cozystack/cozystack#4286). The connect guide only said to open it "from the sidebar", and it sits in the Marketplace section, so readers would look for it in the wrong place. Assisted-by: LLM Signed-off-by: Andrei Kvapil <[email protected]>
Follow-up for cozystack/cozystack#4286, which moved the dashboard Repositories view from the Marketplace sidebar to Admin, under Administration. The connect guide only said to open the view "from the sidebar", and the page sits in the Marketplace docs, so after that change readers would look in the wrong place. Only `next/` is touched, the view is not in a release yet.
What this PR does
Connecting an External-Apps repository creates a cluster-scoped Tap and runs its charts in the management cluster. That is an operator action, so the Repositories view moves from the Marketplace sidebar to Admin, under Administration.
The page is at
/admin/tapsnow./marketplace/tapsredirects there, otherwise the:appNamecatch-all would render an order page for an app called "taps".Who can open it doesn't change. The entry was ungated in the Marketplace and stays ungated in Administration, Tap RBAC decides the rest.
Screenshots
Downstream repositories
content/en/docs/next/marketplace/connecting.mdon the website told readers to open the Repositories view "from the sidebar", inside the Marketplace docs. The follow-up points them to Admin and waits for this PR.Release note
Summary by CodeRabbit
New Features
/admin/taps.Bug Fixes
/admin/taps.