Design around Issue milestones

crueber commented

2026-09-05 14:31:48 +00:00

Owner

It says "anon added this to the “000001” milestone · 2026-09-05 14:29Z"

It should be the name of the milestone, not the id number

There is no way to remove it from the milestone once added. There needs to be a way to remove it.

![image](/attachments/b5faca80-46f2-40ee-91fa-f287c540ae14) It says "anon added this to the “000001” milestone · 2026-09-05 14:29Z" It should be the name of the milestone, not the id number There is no way to remove it from the milestone once added. There needs to be a way to remove it.

image.png

81 KiB

crueber referenced this issue from a commit

2026-09-05 15:04:55 +00:00

Fix #132: milestone titles in timeline events, removal verified (02_issues.md)

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

2026-09-05 15:05:01 +00:00

Fix #132: milestone titles in events + removal #136

crueber commented

2026-09-05 15:05:15 +00:00

Author

Owner

Fixed by #136 (not merged — needs review): system lines now show the milestone title ("v1.1") instead of the id, resolved via the cached milestones list with bare-id fallback for deleted milestones. Removal verified working end to end: the + dropdown's "No milestone" row clears via PATCH null — no picker change needed. Details + test results in the PR.

crueber commented

2026-09-05 15:09:17 +00:00

Author

Owner

PR #136 review (fix/issue-132 @ 86c76ae) — reviewed the diff + full verify in scratch worktree /tmp/opencode/wt132 (pre-existing, left in place).

BEFORE shot confirms the bug: timeline reads anon added this to the "000001" milestone while the sidebar shows V1.1. This PR resolves exactly that.

Title resolution — CORRECT (web/src/lib/issue-events.js:53-60). from/to both go through milestoneTitle(); unknown id falls back to bare id via milestones.js:18 (found?.title ?? id); undefined/empty list falls back the same way via (milestones ?? []) (milestones.js:17). Tests cover all three (issue-events.test.js:63-98). Transient bare-id first paint (cache not yet loaded → [] → bare id, then reactive re-render with titles): ACCEPTABLE — suspending timeline rows on the milestones fetch would hold the whole timeline hostage for one fragment kind, and the sidebar has the identical transient (Issue.jsx:392 same helper, same source). Consistent, self-heals, no extra complexity justified.
Call site — CORRECT, no N+1 (Issue.jsx:291 textFor={(ev) => eventText(ev, allMilestones())}). allMilestones() is a signal read of the single page-owned milestones:{o}/{r} useData cache (Issue.jsx:53-54); the fetch happens once per page, the closure just re-reads it per row. ThreadTimeline calls textFor(ev) once per row (ThreadTimeline.jsx:26,31) — signature compatible.
Removal — VERIFIED by code reading, no change needed. Picker 'No milestone' row → pick(null) (MilestonePicker.jsx:82) → selectMilestone(null) → milestonePatch returns {milestone: null} explicit null (milestones.js:27-30) → PATCH (Issue.jsx:227-240); no-op selects skip the trip. Explicit-null (not absent) matches the server contract per the header comment (milestones.js:6-7). Author's browser-drive set+clear claim is credible; not independently re-driven (no browser per review instructions — noted).
Laws: 1 (no new deps — only first-party ./milestones.js import; package files untouched), 7 (N/A — sync render), 8 (lib→lib + page→lib only; no new routes/registries), 12 (02_issues.md decision entry in the same commit, accurate: wire shape unchanged, render-time resolution, bare-id self-heal, explicit-null removal). Dark+light unaffected (text-only, no classes touched).
Nits (not blocking, no push): missing blank line between test blocks (issue-events.test.js:98-99); eventText() in Issue.jsx:31 is a pure pass-through that could inline — harmless and documented, leave it.

Verify: node --test web/test/unit/*.test.js → 297 pass / 0 fail; vite build clean (SPA 345.85 kB + repos.js SDK bundle). No fixes pushed (nothing functional to fix).

MERGE RECOMMENDATION: ready to merge.

PR #136 review (fix/issue-132 @ 86c76ae) — reviewed the diff + full verify in scratch worktree /tmp/opencode/wt132 (pre-existing, left in place). BEFORE shot confirms the bug: timeline reads `anon added this to the "000001" milestone` while the sidebar shows V1.1. This PR resolves exactly that. 1. Title resolution — CORRECT (web/src/lib/issue-events.js:53-60). from/to both go through milestoneTitle(); unknown id falls back to bare id via milestones.js:18 (`found?.title ?? id`); undefined/empty list falls back the same way via `(milestones ?? [])` (milestones.js:17). Tests cover all three (issue-events.test.js:63-98). Transient bare-id first paint (cache not yet loaded → [] → bare id, then reactive re-render with titles): ACCEPTABLE — suspending timeline rows on the milestones fetch would hold the whole timeline hostage for one fragment kind, and the sidebar has the identical transient (Issue.jsx:392 same helper, same source). Consistent, self-heals, no extra complexity justified. 2. Call site — CORRECT, no N+1 (Issue.jsx:291 `textFor={(ev) => eventText(ev, allMilestones())}`). allMilestones() is a signal read of the single page-owned `milestones:{o}/{r}` useData cache (Issue.jsx:53-54); the fetch happens once per page, the closure just re-reads it per row. ThreadTimeline calls textFor(ev) once per row (ThreadTimeline.jsx:26,31) — signature compatible. 3. Removal — VERIFIED by code reading, no change needed. Picker 'No milestone' row → pick(null) (MilestonePicker.jsx:82) → selectMilestone(null) → milestonePatch returns {milestone: null} explicit null (milestones.js:27-30) → PATCH (Issue.jsx:227-240); no-op selects skip the trip. Explicit-null (not absent) matches the server contract per the header comment (milestones.js:6-7). Author's browser-drive set+clear claim is credible; not independently re-driven (no browser per review instructions — noted). 4. Laws: 1 (no new deps — only first-party ./milestones.js import; package files untouched), 7 (N/A — sync render), 8 (lib→lib + page→lib only; no new routes/registries), 12 (02_issues.md decision entry in the same commit, accurate: wire shape unchanged, render-time resolution, bare-id self-heal, explicit-null removal). Dark+light unaffected (text-only, no classes touched). 5. Nits (not blocking, no push): missing blank line between test blocks (issue-events.test.js:98-99); eventText() in Issue.jsx:31 is a pure pass-through that could inline — harmless and documented, leave it. Verify: node --test web/test/unit/*.test.js → 297 pass / 0 fail; vite build clean (SPA 345.85 kB + repos.js SDK bundle). No fixes pushed (nothing functional to fix). MERGE RECOMMENDATION: ready to merge.

crueber closed this issue

2026-09-05 15:09:52 +00:00

crueber referenced this issue from a commit

2026-09-05 15:09:52 +00:00

Merge pull request 'Fix #132: milestone titles in events + removal' (#136) from fix/issue-132 into main

crueber commented

2026-09-05 15:09:52 +00:00

Author

Owner

Fixed by PR #136 (review clean; titles resolved with fallbacks, removal verified working; 297/297 node tests), merged. Closing.

crueber referenced this issue

2026-09-05 16:17:10 +00:00