Skip to content

Catalog: fix package-name prefix link not filtering the package list#5035

Merged
sir-sigurd merged 6 commits into
masterfrom
fix-package-prefix-filter
Jun 26, 2026
Merged

Catalog: fix package-name prefix link not filtering the package list#5035
sir-sigurd merged 6 commits into
masterfrom
fix-package-prefix-filter

Conversation

@sir-sigurd

@sir-sigurd sir-sigurd commented Jun 26, 2026

Copy link
Copy Markdown
Member

Description

On a package page, the package handle is rendered with its prefix (the part before /) as a link to the bucket's package list, meant to filter the list to that prefix. Clicking it produced a URL with a filter query param that the list page ignored, so the list rendered unfiltered.

Regression. The feature (#2214) worked from 2021 until the unified-search migration (#4413, 2025-07-18) replaced the bespoke package list with the SearchUIModel-based page. The new page reads filters from the URL by predicate key (name, size, …) and silently drops unrecognized params; the prefix link in PackageLink.tsx was never updated and kept emitting the now-dead filter param.

Fix: emit the name keyword-wildcard filter instead — name=<prefix>/, which addMagicWildcardsKW turns into the <prefix>/* wildcard matching the package handle. Also dropped the now-unused filter/sort/p opts from bucketPackageList in routes.ts (PackageLink was their only caller).

Verified end-to-end with a local dev build against the open.quiltdata.com backend: on quilt-example (80 packages), clicking the examples/ prefix narrows the list to the 24 examples/* packages.

A regression test (PackageLink.spec.tsx) locks the contract that drifted in #4413: it round-trips the link's URL through parseSearchParams and asserts the name filter is applied — and that the old filter param stays inert — so a future search-params change can't silently re-break it.

TODO

  • Unit tests
  • Security: Confirm that this change meets security best practices and does not violate the security model
  • Open and Embed: Confirm that this change doesn't break Open variant and Embed widget
  • Documentation
  • Changelog entry

Greptile Summary

This PR restores package-prefix links on package pages. The main changes are:

  • Uses the name search query parameter for prefix links.
  • Removes unused legacy package-list route options.
  • Adds tests for prefix-link parsing and legacy filter behavior.
  • Adds a changelog entry for the regression fix.

Confidence Score: 5/5

This looks safe to merge.

  • No blocking issues found in the changed code.

Important Files Changed

Filename Overview
catalog/app/containers/Bucket/PackageTree/PackageLink.tsx Updates the prefix link to send the package-name search filter.
catalog/app/constants/routes.ts Updates the package-list route helper to accept the name search parameter.
catalog/app/containers/Bucket/PackageTree/PackageLink.spec.tsx Adds tests for the prefix-link search parameter contract.
catalog/CHANGELOG.md Documents the package-prefix filtering fix.

Reviews (3): Last reviewed commit: "catalog: tighten prefix-filter test comm..." | Re-trigger Greptile

sir-sigurd and others added 2 commits June 26, 2026 12:26
The package detail page renders the package handle with its prefix (the
part before `/`) as a link to the bucket package list. Since the package
list migrated to the unified search UI (SearchUIModel), it reads filters
from the URL by predicate key (`name`, `size`, …) and silently ignores
unrecognized params. The link still emitted the legacy `filter=<prefix>/`
param, so the list rendered unfiltered.

Emit the `name` keyword-wildcard filter instead (`name=<prefix>/`, which
becomes the `<prefix>/*` wildcard matching the package handle), and drop
the now-dead `filter`/`sort`/`p` opts from `bucketPackageList` —
PackageLink was their only caller.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@codecov

codecov Bot commented Jun 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 48.22%. Comparing base (9d8759c) to head (bb706e2).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5035      +/-   ##
==========================================
+ Coverage   48.02%   48.22%   +0.20%     
==========================================
  Files         854      854              
  Lines       34992    34992              
  Branches     5990     5990              
==========================================
+ Hits        16804    16876      +72     
+ Misses      16198    16144      -54     
+ Partials     1990     1972      -18     
Flag Coverage Δ
api-python 93.14% <ø> (ø)
catalog 23.81% <100.00%> (+0.31%) ⬆️
lambda 97.09% <ø> (ø)
py-shared 98.02% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread catalog/app/containers/Bucket/PackageTree/PackageLink.tsx
sir-sigurd and others added 2 commits June 26, 2026 13:15
…ame` filter

Guards the producer/consumer contract that regressed in #4413: the link's
URL is parsed by the real `parseSearchParams` and asserted to activate the
`name` filter (→ `team/*` wildcard). A dropped/renamed param fails the test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- drop the redundant `order` assertion (fromURLSearchParams sets the
  predicate and order together, so the predicate check implies it)
- document why the config mock is needed and what the bare `name` route
  opt means (a KeywordWildcard glob kept in sync with PackagesSearchFilterIO)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@sir-sigurd sir-sigurd requested a review from Copilot June 26, 2026 08:23
@sir-sigurd

Copy link
Copy Markdown
Member Author

@greptileai please review

Comment thread catalog/app/containers/Bucket/PackageTree/PackageLink.tsx

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Fixes a regression where clicking the package-name prefix on a package page generated a URL query param (filter) that the unified-search package list ignores, causing the list to render unfiltered. The prefix link is updated to emit the unified-search predicate key (name) so the list page correctly applies the prefix filter again.

Changes:

  • Update package-prefix link generation to use name=<prefix>/ (unified-search predicate key) instead of the legacy filter param.
  • Simplify bucketPackageList route helper options to only support the name filter.
  • Add a unit test that verifies the prefix-link URL round-trips through parseSearchParams into the name filter and becomes a prefix/* wildcard at the GraphQL layer.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
catalog/CHANGELOG.md Adds a changelog entry describing the regression and the fix.
catalog/app/containers/Bucket/PackageTree/PackageLink.tsx Switches prefix link query param from legacy filter to unified-search name.
catalog/app/containers/Bucket/PackageTree/PackageLink.spec.tsx Adds coverage to prevent producer/consumer drift for the prefix-link query param.
catalog/app/constants/routes.ts Updates bucketPackageList route helper to serialize name and drops unused opts.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@sir-sigurd sir-sigurd left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Notes from an AI-assisted self-review pass:

The fix is correct and verified end-to-end. The full producer→consumer round-trip holds: mkSearch({ name }) writes ?name=, and fromURLSearchParams reads that exact key (children['name'], no alias indirection); resultType defaults to QuiltPackage on the package-list page, so the param is parsed as the packages name filter; and a filter arriving only via the URL is both applied to the query and surfaced as an active/removable chip. No correctness, security, API-contract, or edge-case issues surfaced.

Greptile's P1 ("escape wildcard prefixes") doesn't apply — package handles are constrained to ^[\w-]+/[\w-]+$ on every write path, so a prefix can never contain */?; details in that thread.

Everything below is optional polish — none of it blocks merge.

1. Producer/consumer coupling (P3, design). routes.ts:127-135 — the URL builder hard-codes the search predicate key name and its keyword-wildcard semantics; only the new comment and the test keep it in sync with PackagesSearchFilterIO. This is the same producer/consumer drift that caused the original bug (#4413), now guarded by a test rather than the type system. The comment + test are a reasonable mitigation for a single call site, so this is a note, not a request.
Alternative: have the search model export a typed prefix-filter URL-state builder that bucketPackageList consumes, so the predicate key and wildcard rules stay owned by the module that defines them?

2. Redundant test cleanup (nit). PackageLink.spec.tsx:4,20 — the per-file afterEach(cleanup) is now redundant: setup-vitest.ts:11 registers it globally (from #5026). It's harmless and still matches most existing specs, so leaving it is fine; flagging only because the global registration is recent.

3. Stale TODO checkbox (nit). The "Unit tests" box is unchecked, but PackageLink.spec.tsx was added — tick it, or leave it if more coverage was intended (see #4).

4. Negative assertion for the inert param (P3, optional). The test pins the new name key end-to-end but doesn't assert that the old filter param is inert. A one-liner — expect(parseSearchParams('?filter=team/').filter.predicates.name).toBeNull() — would lock the "unrecognized params are dropped" invariant that is the crux of this bug. The positive test already prevents the fix from regressing, so this documents the invariant rather than closing a coverage hole.

5. Stale commented-out scaffold (nit, pre-existing). containers/Bucket/Routes.ts:115-126 — the commented-out bucketPackageList migration scaffold still carries the old { filter, sort, p } opts shape this PR removes, so it's now doubly stale. Pre-existing Effect-Schema scaffold, untouched here; noting only because it's adjacent to the opts change.

🤖 Drafted with Claude; findings vetted by the reviewer — sanity-check rather than take as authoritative.

Reviewed against 157d83b

sir-sigurd and others added 2 commits June 26, 2026 14:00
…inertness)

- drop per-file `afterEach(cleanup)`: redundant with the global registration
  in setup-vitest.ts (#5026), and a no-op in a single-test file
- add a negative test pinning that the unrecognized legacy `filter` param
  produces no `name` filter — the exact contract that regressed in #4413

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
State the regression rationale once (drop the duplicated "ignores unrecognized
params" prose), and lead the changelog with the user-visible effect instead of
the search-model internals.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@sir-sigurd sir-sigurd requested a review from fiskus June 26, 2026 09:13
@fiskus

fiskus commented Jun 26, 2026

Copy link
Copy Markdown
Member

Confirmed and intentional. Handles are [\w-]+/[\w-]+ (/package_name_valid), so a prefix can't hold */? and addMagicWildcardsKW always hits the append-* branch — the raw-passthrough is unreachable for real packages. If naming ever relaxed, worst case is an over-broad name filter on the package list: read-only, no crash, no data/security impact.

Considered and rejected:

  • Escape */? — fights addMagicWildcardsKW's own */? detection, breaking the trailing-wildcard append.
  • invariant/render guard — throws in render, turning a cosmetic, unreachable inaccuracy into a hard page crash. Strictly worse.
  • Code comment — the behavior lives in the search layer, not this call site; it'd document a global filter contract at one arbitrary caller.

Leaving it unhandled on purpose.

@greptileai please re-review and update the confidence score.

@sir-sigurd sir-sigurd added this pull request to the merge queue Jun 26, 2026
Merged via the queue into master with commit db2a67c Jun 26, 2026
45 checks passed
@sir-sigurd sir-sigurd deleted the fix-package-prefix-filter branch June 26, 2026 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants