Milestone/label tab-strip icons from #484/#485 mount on the Issues tab instead of the Milestones/Labels links

crueber commented

2026-09-13 21:18:24 +00:00

Owner

What's requested

The #484/#485 tab-strip icon implementations landed in the wrong places. Both issues asked for icons on the labels and milestones surfaces; the landed code instead mounts both icons on the Issues tab of the repo tab strip, conditionally on the current path.

Requested placement (per the parent issues):

The milestone icon (milestone-open) should appear on the Milestones tab — i.e. the Milestones link/btn on the issues page (/${full}/milestones), matching how the label icon should appear on its link.
The label icon should NOT appear on the Issues tab. It belongs on the labels-page link — the Labels button on the issues page toolbar (Issues.jsx line ~240: <A class="btn" href={/${ctx.full}/labels}>Labels</A>).

Evidence (current tree, main @ `2e64fed`)

web/src/pages/Repo.jsx, tab strip render (~lines 803–816):

{/* Forgejo #484, option (a): the milestone-open icon leads
    the Issues label while under /milestones ... */}
<Show when={t.id === "issues" && isMilestonesPath(location.pathname)}>
  <Icon name="milestone-open" />{" "}
</Show>
{/* Forgejo #485, option (a): the label icon leads
    the Issues label while under /labels ... */}
<Show when={t.id === "issues" && isLabelsPath(location.pathname)}>
  <Icon name="label" />{" "}
</Show>
{t.label}

isMilestonesPath (Repo.jsx:188) / isLabelsPath (Repo.jsx:206) key off the path segment after /:owner/:name, and both <Show> guards require t.id === "issues". Result: navigating to /milestones paints a milestone icon on the Issues tab (which is highlighted because lib/tabs.js maps milestones: "issues"), and /labels paints a label icon on the Issues tab. The tab you land on shows an icon for the other page's concept while you're actually on Milestones/Labels — the icon follows the highlight mapping, not the destination.

The pinned unit tests encode this same placement (web/test/unit/milestone-icons-484.test.js: "the icon shows only on the Issues tab, only under /milestones"; web/test/unit/label-icon-485.test.js likewise), so the tests will need updating together with the code.

Tab-id mapping (checked)

web/src/lib/tabs.js SECTION_TABS maps both labels: "issues" and milestones: "issues" — there is no labels/milestones tab id and TABS (Repo.jsx:162) has no such entries; the active-tab highlight for both routes is the Issues tab, by design. This is why the implementers mounted the icons on the t.id === "issues" tab: it's the only tab that highlights on those routes. Any fix must respect that mapping (do not add a new tab id that activeTab never returns — #484 explicitly rejected that as option (b), since it would split highlight from navigation and disturb the #319 badge and #274 scroll contracts).

The resolution therefore is not "put the icon on a Milestones tab in the strip" (no such tab exists), but:

Remove both <Show> icon blocks from the tab strip in Repo.jsx (lines ~803–816) and the now-unused isMilestonesPath/isLabelsPath helpers.
Milestone icon → Milestones link: the Milestones button on the issues page toolbar (web/src/pages/Issues.jsx ~line 243, <A class="btn" href={/${ctx.full}/milestones}>Milestones</A>) leads with <Icon name="milestone-open" />, composing into the btn flow (no per-icon class, per the decorative contract).
Label icon → Labels link: same toolbar, the Labels button (~line 240) leads with <Icon name="label" />.
The Labels page heading already carries the icon (Labels.jsx:83); for consistency the Milestones page heading (Milestones.jsx:119) may gain <Icon name="milestone-open" /> in the same pass — planner's call, note the decision.

Acceptance criteria

No icon renders on any repo tab-strip tab, under any path (the <Show> blocks and both path helpers are gone from Repo.jsx).
The Labels button on the issues page toolbar shows the label icon before its text.
The Milestones button on the issues page toolbar shows the milestone-open icon before its text.
lib/tabs.js mapping (labels/milestones → issues) is unchanged; Issues tab still highlights on both routes; #319 badge and #274 scroll behavior untouched.
Icons stay decorative (aria-hidden via the shared svg mechanism, no per-icon classes, accessible names unchanged).
milestone-icons-484.test.js and label-icon-485.test.js updated to pin the new placement; node --test web/test/unit green.

## What's requested The #484/#485 tab-strip icon implementations landed in the wrong places. Both issues asked for icons on the *labels* and *milestones* surfaces; the landed code instead mounts both icons on the **Issues tab** of the repo tab strip, conditionally on the current path. Requested placement (per the parent issues): - The **milestone icon** (`milestone-open`) should appear on the **Milestones tab** — i.e. the Milestones link/btn on the issues page (`/${full}/milestones`), matching how the label icon should appear on its link. - The **label icon** should NOT appear on the Issues tab. It belongs on the **labels-page link** — the `Labels` button on the issues page toolbar (`Issues.jsx` line ~240: `<A class="btn" href={`/${ctx.full}/labels`}>Labels</A>`). ## Evidence (current tree, main @ 2e64fed) `web/src/pages/Repo.jsx`, tab strip render (~lines 803–816): ```jsx {/* Forgejo #484, option (a): the milestone-open icon leads the Issues label while under /milestones ... */} <Show when={t.id === "issues" && isMilestonesPath(location.pathname)}> <Icon name="milestone-open" />{" "} </Show> {/* Forgejo #485, option (a): the label icon leads the Issues label while under /labels ... */} <Show when={t.id === "issues" && isLabelsPath(location.pathname)}> <Icon name="label" />{" "} </Show> {t.label} ``` - `isMilestonesPath` (Repo.jsx:188) / `isLabelsPath` (Repo.jsx:206) key off the path segment after /:owner/:name, and both `<Show>` guards require `t.id === "issues"`. Result: navigating to /milestones paints a milestone icon on the **Issues** tab (which is highlighted because `lib/tabs.js` maps `milestones: "issues"`), and /labels paints a label icon on the Issues tab. The tab you land on shows an icon for the *other* page's concept while you're actually on Milestones/Labels — the icon follows the highlight mapping, not the destination. The pinned unit tests encode this same placement (`web/test/unit/milestone-icons-484.test.js`: "the icon shows only on the Issues tab, only under /milestones"; `web/test/unit/label-icon-485.test.js` likewise), so the tests will need updating together with the code. ## Tab-id mapping (checked) `web/src/lib/tabs.js` `SECTION_TABS` maps both `labels: "issues"` and `milestones: "issues"` — there is no `labels`/`milestones` tab id and `TABS` (Repo.jsx:162) has no such entries; the active-tab highlight for both routes is the Issues tab, by design. **This is why the implementers mounted the icons on the `t.id === "issues"` tab**: it's the only tab that highlights on those routes. Any fix must respect that mapping (do not add a new tab id that `activeTab` never returns — #484 explicitly rejected that as option (b), since it would split highlight from navigation and disturb the #319 badge and #274 scroll contracts). The resolution therefore is not "put the icon on a Milestones tab in the strip" (no such tab exists), but: 1. **Remove both `<Show>` icon blocks from the tab strip** in Repo.jsx (lines ~803–816) and the now-unused `isMilestonesPath`/`isLabelsPath` helpers. 2. **Milestone icon → Milestones link**: the `Milestones` button on the issues page toolbar (`web/src/pages/Issues.jsx` ~line 243, `<A class="btn" href={`/${ctx.full}/milestones`}>Milestones</A>`) leads with `<Icon name="milestone-open" />`, composing into the btn flow (no per-icon class, per the decorative contract). 3. **Label icon → Labels link**: same toolbar, the `Labels` button (~line 240) leads with `<Icon name="label" />`. 4. The Labels page heading already carries the icon (`Labels.jsx:83`); for consistency the Milestones page heading (`Milestones.jsx:119`) may gain `<Icon name="milestone-open" />` in the same pass — planner's call, note the decision. ## Acceptance criteria - [ ] No icon renders on any repo tab-strip tab, under any path (the `<Show>` blocks and both path helpers are gone from Repo.jsx). - [ ] The `Labels` button on the issues page toolbar shows the label icon before its text. - [ ] The `Milestones` button on the issues page toolbar shows the milestone-open icon before its text. - [ ] `lib/tabs.js` mapping (`labels`/`milestones` → `issues`) is unchanged; Issues tab still highlights on both routes; #319 badge and #274 scroll behavior untouched. - [ ] Icons stay decorative (`aria-hidden` via the shared svg mechanism, no per-icon classes, accessible names unchanged). - [ ] `milestone-icons-484.test.js` and `label-icon-485.test.js` updated to pin the new placement; `node --test web/test/unit` green.

crueber added the

bug

label

2026-09-13 21:18:36 +00:00

crueber added this to the v1 milestone

2026-09-13 21:18:36 +00:00

crueber commented

2026-09-13 21:18:36 +00:00

Author

Owner

Implementation of #484/#485 landed the strip icons on the wrong surface — see body.

crueber added the

issues

label

2026-09-13 21:19:21 +00:00

crueber referenced this issue from a commit

2026-09-13 21:24:16 +00:00

Fix #495: tab-strip icon placement

crueber referenced this issue from a pull request that will close it,

2026-09-13 21:24:18 +00:00

Fix #495: tab-strip icon placement #496

crueber commented

2026-09-13 21:24:21 +00:00

Author

Owner

Fixed by #496 (branch fix/issue-495): strip blocks + both path helpers deleted from Repo.jsx; milestone-open → Milestones toolbar button, label → Labels toolbar button; Milestones heading gains the open icon (planner's call, noted in docs/go/12_web_ui.md). Tests: 1088 pass / 2 pre-existing live-server smoke fails; vite build green.

Fixed by #496 (branch fix/issue-495): strip <Show> blocks + both path helpers deleted from Repo.jsx; milestone-open → Milestones toolbar button, label → Labels toolbar button; Milestones heading gains the open icon (planner's call, noted in docs/go/12_web_ui.md). Tests: 1088 pass / 2 pre-existing live-server smoke fails; vite build green.

crueber commented

2026-09-13 21:26:12 +00:00

Author

Owner

Review of PR #496 (fix/issue-495 @ 438177d) — all 6 acceptance criteria verified in a scratch worktree (removed afterward; main worktree left clean).

(1) Strip clean — PASS. web/src/pages/Repo.jsx: both icon blocks deleted and both helpers (isMilestonesPath/isLabelsPath) deleted. Grep confirms zero Icon label/milestone references remain in Repo.jsx (only pre-existing clone/watch/star/fork Icons). No icon on any tab under any path.
(2) Toolbar buttons — PASS. web/src/pages/Issues.jsx:243-244 Labels link leads with , :249-250 Milestones link leads with . class="btn" and hrefs (${ctx.full}/labels, ${ctx.full}/milestones) byte-identical; link text unchanged.
(3) Milestones heading — PASS. web/src/pages/Milestones.jsx:123 mirrors Labels.jsx:83 exactly (same mb-3 flex items-center gap-2 text-lg font-semibold shell, icon leads, no-space composition like Labels). Decision noted in code comment + icons.jsx header + 12_web_ui.md entry.
(4) tabs.js/badge/scroll — PASS. Empty diff on web/src/lib/tabs.js; TABS still 7 entries, no JSX; labels/milestones→issues mapping pinned by tests; #319 badge + #274 scroll pins intact.
(5) Decorative — PASS. Name-only usage, no per-icon classes; aria-hidden via the shared per-entry svg factories (icons.jsx, fifteen entries intact, twelve-surface count pin in icons-per-row-491.test.js:105 still green); accessible names unchanged.
(6) Tests faithful — PASS. Old strip-placement pins replaced with no-strip/no-helper pins (not dropped); new toolbar-order + heading pins added; dropdown-rows-untouched pin re-scoped with explicit slice so the toolbar link can't trip it. Targeted files 21/21 pass; full suite with live server excluded (WALHUB_TEST_WEB_BASE_URL=http://127.0.0.1:9): 1087 pass / 0 fail / 3 skipped (smoke skips), exit 0.
(7) Scope/hygiene — PASS. No backend change (internal/, cmd/, go.mod/go.sum untouched), no new deps (package.json/pnpm-workspace untouched), docs accurate (12_web_ui.md entry + icons.jsx header comment updated in the same change, law 12).

Test notes: default run showed 1088 pass / 2 fail, both in smoke.test.js hitting this environment's live instance on :8080 (/ returned 401) — environmental, not PR-caused; proven by the clean no-server run above. vite build green (2.11s). No browser drive per review instructions (node tests + reasoning); the docs entry records browser proof open per this file's convention.

No fixes needed — nothing pushed. MERGE RECOMMENDATION: ready to merge.

Review of PR #496 (fix/issue-495 @ 438177d) — all 6 acceptance criteria verified in a scratch worktree (removed afterward; main worktree left clean). (1) Strip clean — PASS. web/src/pages/Repo.jsx: both <Show> icon blocks deleted and both helpers (isMilestonesPath/isLabelsPath) deleted. Grep confirms zero Icon label/milestone references remain in Repo.jsx (only pre-existing clone/watch/star/fork Icons). No icon on any tab under any path. (2) Toolbar buttons — PASS. web/src/pages/Issues.jsx:243-244 Labels link leads with <Icon name="label" />, :249-250 Milestones link leads with <Icon name="milestone-open" />. class="btn" and hrefs (${ctx.full}/labels, ${ctx.full}/milestones) byte-identical; link text unchanged. (3) Milestones heading — PASS. web/src/pages/Milestones.jsx:123 mirrors Labels.jsx:83 exactly (same mb-3 flex items-center gap-2 text-lg font-semibold shell, icon leads, no-space composition like Labels). Decision noted in code comment + icons.jsx header + 12_web_ui.md entry. (4) tabs.js/badge/scroll — PASS. Empty diff on web/src/lib/tabs.js; TABS still 7 entries, no JSX; labels/milestones→issues mapping pinned by tests; #319 badge + #274 scroll pins intact. (5) Decorative — PASS. Name-only <Icon> usage, no per-icon classes; aria-hidden via the shared per-entry svg factories (icons.jsx, fifteen entries intact, twelve-surface count pin in icons-per-row-491.test.js:105 still green); accessible names unchanged. (6) Tests faithful — PASS. Old strip-placement pins replaced with no-strip/no-helper pins (not dropped); new toolbar-order + heading pins added; dropdown-rows-untouched pin re-scoped with explicit slice so the toolbar link can't trip it. Targeted files 21/21 pass; full suite with live server excluded (WALHUB_TEST_WEB_BASE_URL=http://127.0.0.1:9): 1087 pass / 0 fail / 3 skipped (smoke skips), exit 0. (7) Scope/hygiene — PASS. No backend change (internal/, cmd/, go.mod/go.sum untouched), no new deps (package.json/pnpm-workspace untouched), docs accurate (12_web_ui.md entry + icons.jsx header comment updated in the same change, law 12). Test notes: default run showed 1088 pass / 2 fail, both in smoke.test.js hitting this environment's live instance on :8080 (/ returned 401) — environmental, not PR-caused; proven by the clean no-server run above. vite build green (2.11s). No browser drive per review instructions (node tests + reasoning); the docs entry records browser proof open per this file's convention. No fixes needed — nothing pushed. MERGE RECOMMENDATION: ready to merge.

crueber closed this issue

2026-09-13 21:26:29 +00:00

crueber referenced this issue from a commit

2026-09-13 21:26:29 +00:00

Merge pull request 'Fix #495: tab-strip icon placement' (#496) from fix/issue-495 into main

crueber commented

2026-09-13 21:26:30 +00:00

Author

Owner

Fixed by PR #496 (review clean — all 6 criteria pass, strip clean, toolbar + heading verified), merged. Closing.

crueber referenced this issue

2026-09-14 21:07:55 +00:00