Skip to content

fix(repositories): Maven grouped browser pagination, file click-through, and full file listing#460

Closed
brandonrc wants to merge 1 commit into
mainfrom
fix/maven-grouped-browser
Closed

fix(repositories): Maven grouped browser pagination, file click-through, and full file listing#460
brandonrc wants to merge 1 commit into
mainfrom
fix/maven-grouped-browser

Conversation

@brandonrc

Copy link
Copy Markdown
Contributor

Summary

Fixes three related defects in the Maven repository browser's GROUPED view.

  • Pagination not shown during Maven repository browse in Grouped mode #443 Grouped mode rendered no pagination, so only the first 20 GAV components were visible even when many more existed. The grouped branch used MavenComponentList, which had a static "showing N of M" footer but no page controls. Extracted the pagination markup out of DataTable into a shared DataTablePagination component and wired it into MavenComponentList, driven by the same page/pageSize state the flat list already uses.

  • Cannot open artifact details when browsing the grouped mode #444 File rows inside an expanded GAV group were plain text spans that did nothing on click (flat mode opens details fine). They are now buttons that reconstruct the Maven path (groupId/artifactId/version/filename) and open the artifact detail dialog via a new onFileSelect callback.

  • Maven repository browser does not list all files associated with a specific GAV #445 Non-jar files (e.g. .zip deployed alongside the POM, and checksum sidecars) could not be opened in grouped mode. The grouped backend response already lists every filename per GAV; surfacing each as a clickable row exposes them with full metadata (size, downloads, uploader, dates) through the detail dialog. Also fixed artifactsApi.get to encode path segments individually so the backend wildcard route /:key/artifacts/*path matches reconstructed paths instead of 404ing on percent-encoded slashes.

Closes #443
Closes #444
Closes #445

Test Checklist

  • Unit tests added/updated
  • E2E Playwright tests added/updated
  • Manually tested locally
  • No regressions in existing tests

Added unit coverage for grouped pagination, clickable file rows, full file listing (incl. .zip), the new mavenFilePath helper, and segment-wise path encoding in artifactsApi.get. New Playwright spec e2e/suites/interactions/repositories/maven-grouped-browser.spec.ts seeds GAVs via the API and asserts grouped pagination renders and navigates, that clicking a grouped file opens the detail dialog, and that all GAV files (including .zip and checksums) are listed.

UI Changes

  • Playwright E2E spec covers the change
  • Responsive layout verified (mobile + desktop)
  • Dark mode verified
  • Accessibility checked (keyboard navigation, screen reader)

File rows are now native <button> elements with aria-labels, so they are keyboard-focusable and announced. Pagination reuses the existing accessible DataTablePagination controls. The grouped list inherits the existing dark-mode and responsive Tailwind classes unchanged.

…ing in Maven grouped view

Grouped (Maven/Gradle) mode in the repository browser had three defects:

- #443: the grouped view rendered no pagination, so only the first 20 GAV
  components were ever shown. Extracted the DataTable pagination markup into
  a shared DataTablePagination component and wired it into MavenComponentList,
  driven by the same page/pageSize state the flat list uses.

- #444: file rows inside an expanded GAV group were plain text and did
  nothing on click. They are now buttons that reconstruct the Maven path
  (groupId/artifactId/version/filename) and open the artifact detail dialog,
  matching flat-mode behaviour.

- #445: non-jar files such as .zip and checksum sidecars were not openable in
  grouped mode. The grouped response already carries every filename per GAV;
  surfacing each as a clickable row now exposes them with full metadata via
  the detail dialog. Also fixed artifactsApi.get to encode path segments
  individually so the backend wildcard route (/:key/artifacts/*path) matches
  reconstructed paths instead of 404ing on percent-encoded slashes.

Closes #443
Closes #444
Closes #445
@brandonrc brandonrc requested a review from a team as a code owner June 2, 2026 16:28
@brandonrc brandonrc added this to the v1.2.0 milestone Jun 2, 2026
@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

@brandonrc brandonrc left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accessibility review (WCAG 2.2 AA).

Clickable file rows are real controls (the main question): yes.
The new per-file rows in maven-component-list.tsx are real <button type="button"> elements, not clickable divs, with aria-label={Open details for ${filename}} and focus-visible styling. They are keyboard operable and have an accessible name (4.1.2, 2.1.1). Good.

Positive findings

  • The GAV disclosure trigger is a <button aria-expanded>; expand/collapse state is exposed (4.1.2).
  • File and component lists keep role="list"; the FileIcon is aria-hidden. Correct.
  • DataTablePagination reuses the shared control with named prev/next buttons.

Gaps

  1. Moderate (2.4.7 / 3.2 focus visibility): the file-row button uses focus-visible:bg-muted/50 focus-visible:outline-none. Removing the outline and replacing it with only a faint background tint (bg-muted/50) is a weak focus indicator, especially on the muted text color, and may not meet WCAG 2.2 SC 2.4.11 Focus Appearance / 2.4.7 Focus Visible against the row background. Prefer keeping a visible ring (e.g. focus-visible:ring-2 focus-visible:ring-ring) rather than outline-none + background only.

  2. Minor: the <button> accessible name is Open details for {filename}, but the visible text is just the filename. That is fine (name contains visible label, 2.5.3). No change needed.

  3. Verify the new file detail dialog opened via showDetailByPath returns focus to the originating file button on close (2.4.3). The dialog is the shared artifact detail dialog, so this likely already works, but it is worth a manual SR/keyboard pass since the trigger is now a nested button inside a collapsible.

No blocker. Recommend restoring a real focus ring on the file-row button.

@brandonrc brandonrc left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test/E2E quality review (v1.2.0 batch). Verdict: real coverage at both layers; e2e has no lazy skip paths.

#443 pagination unit. Good and tight. renders pagination controls when total is provided asserts both data-table-pagination present AND page 1 of 3 / 1-20 of 42. The negative case (total undefined => no pagination) is covered. onPageChange called with 2 proves the wiring. The prior tests asserted a "showing N of M" footer; the bug was that grouped mode never rendered real pagination, and these tests now assert the actual control, so they distinguish fixed-vs-broken.

#444/#445 unit. onFileSelect is asserted to receive the reconstructed Maven path org/junit/.../...jar plus filename, proving the click-to-detail wiring. The non-jar listing test explicitly checks .zip and .jar.sha1 rows render, which is exactly the #445 bug.

e2e. Notably this is the only repo-browser spec in the batch with no admin-gating skip. It seeds 25 components and asserts page 1 of [2-9]\d* then navigates to page 2 of (real pagination assertion, not just presence). The #444 test asserts clicking the .jar opens a dialog with a Download URL field. The #445 test asserts all 4 files visible AND the 4 files count badge. Selectors (data-testid="maven-component-row", data-gav, toggle-grouped with aria-pressed, data-table-pagination) are all added in the component diff, so these are not testing against non-existent attributes.

Minor: page.getByText(/page 1 of [2-9]\d*/i) assumes seeded set stays >20 components; since groupId is per-run-unique this is deterministic. One waitForTimeout(500) after expand is an arbitrary wait but low risk. Not requesting changes.

@brandonrc brandonrc left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security/API review (path traversal + encoding lens).

Net: safe and a genuine correctness improvement. No new traversal exposure.

Per-segment encoding in artifactsApi.get — CORRECT. Switching from encodeURIComponent(artifactPath) (which would %2F-encode separators and break the Axum *path wildcard) to artifactPath.split('/').map(encodeURIComponent).join('/') preserves slashes while escaping reserved characters within each segment (space -> %20, etc.). This does not widen traversal exposure: .. as a literal segment stays .. (encodeURIComponent does not touch .), so the path sent is no more dangerous than before — and traversal prevention for the wildcard route is the backend's responsibility, unchanged here. Spaces/#/?/% in segments are now correctly escaped, which actually reduces request-smuggling/ambiguous-parse risk versus the old whole-string encode.

One thing to verify outside this PR (not blocking): confirm the backend /:key/artifacts/*path handler canonicalizes/rejects .. and absolute paths before touching storage. The client now faithfully forwards multi-segment paths, so the backend remains the sole traversal guard. If you want, I can spot-check the Rust handler.

No request-changes.

@brandonrc brandonrc left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Frontend/React correctness review. No blockers. Clean extraction and a correct encoding fix.

Correctness notes

  • artifactsApi.get per-segment encoding is the right fix: splitting on / and encodeURIComponent-ing each segment preserves the Axum wildcard (/:key/artifacts/*path) while still escaping reserved chars within a segment. The unit test asserts spaces become %20 and no %2F appears. Good. Since get runs against getActiveInstanceBaseUrl(), the grouped-mode detail lookup also works under a proxied remote instance (a nice contrast with #457's absolute-URL gap).

  • DataTablePagination extraction preserves behavior: the render-nothing-when-no-handlers guard moved inside the component (if (!onPageChange && !onPageSizeChange) return null), and DataTable now always renders it but the guard keeps the old semantics. No regression.

  • Grouped pagination visibility: MavenComponentList renders pagination whenever typeof total === "number", and repo-detail-content always passes onPageChange/onPageSizeChange, so the grouped view now matches the flat list (controls always shown, even on a single page). Consistent with DataTable. Fine.

  • showDetailByPath is correctly memoized on [repoKey] and has user-facing error handling (toast on fetch failure). Clickable file rows are real <button>s with aria-labels. The reconstructed path goes through the fixed get, closing the #444/#445 loop.

Nit

  • maven-component-path.ts mavenFilePath and src/lib/maven.ts mavenFilePath (from #458) are near-duplicate helpers in two locations. Not a defect, but if both PRs land, consider consolidating on the shared src/lib/maven.ts version to avoid drift. Merge-sequencing overlap with #458 on maven-component-list.tsx is known.

Well-tested (unit + e2e). LGTM.

@brandonrc

Copy link
Copy Markdown
Contributor Author

Shipped in v1.2.0. The changes from this PR were folded into the release integration branch #466, which was squash-merged to main and tagged as v1.2.0. The code is already on main, so closing this as superseded by #466 rather than merging it (re-applying would just duplicate what already shipped). No work lost. See #466 for the consolidated record.

@brandonrc brandonrc closed this Jun 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant