monolith/S14: non-blocking follow-ups from the version list stack

Why this issue exists

The monolith/S14 version list ships as a stack of small merge requests. Review on those MRs turns up items that are worth doing but should not hold a step up: nits, test strengthening, seed-data realism, and small polish. Rather than expanding each step's MR or leaving threads open across a stack, they collect here and get fixed together in one MR once the slice is code-complete.

The process, so the table stays trustworthy:

  1. A follow-up item gets a row below, with a link to the thread it came from.
  2. The originating thread is then resolved without a reply, to keep the MRs readable.
  3. Nothing is lost by resolving: the thread link in the row is the record.

Follow-up items

# Item Source
1 versions_table.vue carries a data-testid on the Published cell that no spec reads. Remove it or assert against it. !250074 thread
2 The seeded version ladder is Maven-flavoured (3.3.0-SNAPSHOT) and reads as unrealistic for an npm package. Give npm its own ladder, or pick versions that suit both. !250074 thread
3 The versions-query spec asserts only that the resolver was called. Strengthen it to assert the artifact id it was called with, and that no page window was requested. !250074 thread
4 A version table with no rows renders GlTable's default empty text. Either set show-empty with our own empty-text, or let the S14 empty-state step cover it — the two should not both ship. !250074 thread
5 The Referrer manifests preference renders for every container repository, but it changes nothing on a remote one: the contract makes subject_digest always null there, so include_referrers has no effect. Hiding the switch means selecting kind on the entity query and gating the Preferences section on it. No thread — found while reading api/openapi/v1.yaml for Step 21. !252327 (merged)
6 version_list_empty_state.vue puts data-testid="version-list-empty-state" on the <gl-empty-state> root, and no spec reads it — the spec finds the component via findComponent(GlEmptyState). Remove it or assert against it. !250733 thread
7 Same file, the <gl-button> in the #actions slot carries data-testid="go-to-repository" that no spec reads — the spec finds it via findComponent(GlButton). Remove it or assert against it. !250733 thread
8 In version_list.vue, the <view-options> column-visibility dropdown renders above the versions section whenever the page is populated, including when that section is showing the empty state. There are no columns to hide at that point, so hide the control while the empty state is visible. !250733 thread
9 In version_list_empty_state_spec.js, the describe block is titled 'the link to the setup instructions', but the button it covers routes to the repository detail page, not to setup instructions. Retitle it. !250733 thread
10 Same spec, the test 'routes rather than linking, so following it does not reload the app' asserts props('href') is undefined, which does not prove routing — a GlButton given :to never receives an href prop either way. Assert on the rendered element instead, or drop the test. !250733 thread
11 Same spec, the test named 'names the repository it routes to, so the action reads without its surroundings' asserts the button text is 'Go to repository', which does not name the repository. Put the name in the button label or fix the test name to match what it checks. !250733 thread
12 In version_list_empty_state.vue, the container-format heading (i18n.manifestsTitle) reads 'There are no manifests in this image yet'. A review suggestion proposed 'This image has no manifests yet' instead — shorter, and leads with the subject. Settle on one wording and make the versions-family sibling versionsTitle read consistently with it. !250733 thread
13 The empty state's description strings (versionsDescription, manifestsDescription) are plain sentences — 'Publish your first version to get started.' / 'Push your first manifest to get started.' — with no link, but the Figma design links the description to setup instructions. The !250733 (merged) description argues against that: the action belongs to the repository because the repository owns that surface, so the state deliberately renders no inline instructions — the two positions conflict and need settling. !250529 (merged) (pull command drawer), named in the thread as a precondition, merged 25 Aug, so nothing blocks the change if the design position wins. !250733 thread
14 The empty-state illustration is empty-package-md.svg, imported from @gitlab/svgs, but the Figma design specifies a different image. Confirm which one ships — a non-@gitlab/svgs illustration means adding the asset rather than importing it. !250733 thread
15 Design wants five sortable columns on the version list: Version, Tags, Downloads, Size, and Published (the default). Step 19 ships the two the table has — Version and Published. Tags, Downloads and Size are Phase 2 columns the versions table does not render, and the version sort enum offers only VERSION and CREATED_AT, so sorting on them needs both the columns and a contract change. Nothing to do until those land; the row records the ask. !252093 thread
16 view_options_spec.js, the test 'separates itself from the columns section' reaches the Preferences group by position — findAllComponents(GlDisclosureDropdownGroup).at(1) — so it keeps passing if another group is inserted above it. Target the group with a data-testid instead. !252327 thread
17 Same spec, flipping the referrer switch is asserted to emit nothing on the columns channel, but the mirror case is not covered: flipping a column switch should emit no referrers-changed. Add that assertion. !252327 thread
18 version_list_spec.js, the paging it.each table names one of its columns window, which shadows the global object inside the test body. Rename it. !252327 thread
19 Same spec, 'sends nothing at all on a %s artifact' goes through the referrersFor helper and asserts only that includeReferrers came back undefined. Assert the whole variables object the handler was called with, so the test also proves nothing else drifted. !252327 thread
20 Same spec, renderedIds is defined twice — once in the paging describe and again in the referrer-preference describe. Hoist the one definition. !252327 thread
21 Same spec, the 'drops the active cursor' test spells out mockFirstManifestPage.nodes.map(({ id }) => id) where the paging describe's idsOf helper already does exactly that. Hoist idsOf and reuse it. !252327 thread

Related

Edited by Zack Cuddy