diff --git a/.planning/NOTES.md b/.planning/NOTES.md index 28076da..5230157 100644 --- a/.planning/NOTES.md +++ b/.planning/NOTES.md @@ -3664,3 +3664,35 @@ knowing before someone "fixes" it as broken: sampled from screenshots at 900×600, the main panel's background goes 33,37,41 → 18,20,23 and a row's text 242 → 133. It covers the content area only — not the sidebar or the transport — because the queue is not modal. + +## No test tier can see a `hover:` media query (measured 2026-08-19) + +Gating an affordance on `(hover: hover) and (pointer: fine)` — #68's fix +for the play button that flashed on a long-press — is invisible to both +browser tiers, in *different* ways, and neither of them fails. + +- **`make ui-test`**: CDP's `Emulation.setEmulatedMedia` with a `hover` + feature does not reach the tier's iframe. The call succeeds and + `matchMedia('(hover: hover)')` still answers `true` afterwards. So + there is no way to render a component as a phone would and read the + computed style. +- **`make e2e`**: both projects are desktop (`Desktop Chrome`, + `Desktop Safari`), and the phone specs reach phone *width* with + `setViewportSize`, which changes no media feature but `width`. So the + phone specs run with `hover: hover` and the gate is never exercised. + +What does work, and what the fix was verified with, is a second browser +context under a device descriptor: `chromium.newContext(devices['Pixel +5'])` reports `hover=false pointer:fine=false` and the button computes +`display: none`, against `flex` at 1440px. That is a one-off script, not +a spec — `isMobile` is Chromium-only, so it cannot become an e2e project +without losing the WebKit half. + +`hover-affordance.test.ts` therefore asserts the *parsed stylesheet* — +that the reveal rule sits inside the media query — which catches the +regression that actually threatens it: someone hoisting the rule back out +as a tidy-up, a change nothing on a desktop renders differently. + +Related: a width-gated decision **is** testable at both tiers, which is +why #61's phone mini player is a `matchMedia` stub in the component test +and needs nothing special. diff --git a/backend/explore/prune_test.go b/backend/explore/prune_test.go index 53f4c72..2c6460c 100644 --- a/backend/explore/prune_test.go +++ b/backend/explore/prune_test.go @@ -82,6 +82,100 @@ func TestPruneStaleLocalCrossReferences(t *testing.T) { } } +// TestPruneClearsInLibraryWithNoLocalID covers the fixed point: a row +// carrying in_library with a NULL local_*_id. The upsert's conflict +// clause is `in_library = MAX(in_library, excluded.in_library)`, so it +// can only ever raise the flag, and this pass used to be gated on the id +// being present — which meant nothing in the app could clear such a row, +// ever. It is asserted for all three entity types because the gate was +// written once and used three times, so a fix applied to one is a fix +// that looks complete. +// +// The rows are seeded with raw SQL rather than through seedIndexResult +// deliberately: upsertBatch writes a zero LocalArtistID as literal 0, +// not NULL, and 0 satisfies `IS NOT NULL` — so the old gate already +// caught that shape and a fixture built through the upsert cannot +// reproduce this at all. NULL is what the artifact importer and any +// older writer leave behind, the column being nullable with no default. +func TestPruneClearsInLibraryWithNoLocalID(t *testing.T) { + t.Parallel() + + db := database.NewTestDB(t) + si := NewSearchIndex(db, nil, nil, slog.Default()) + + // A genuinely owned artist, to prove the wider gate does not simply + // clear everything it now looks at. + database.InsertTestTrack(t, db, database.TestTrack{ + FilePath: "/music/owned.mp3", + Artist: "Owned", + }) + + artist, err := db.Queries.GetArtistByName(t.Context(), "Owned") + if err != nil { + t.Fatalf("read seeded artist: %v", err) + } + + seedIndexResult(t, db, SearchIndexResult{ + EntityType: EntityArtist, + MBID: testMBID("owned"), + Title: "Owned", + ArtistName: "Owned", + ArtistMBID: testMBID("owned"), + InLibrary: true, + LocalArtistID: artist.ID, + }) + + orphans := []struct { + name string + entityType string + mbid string + }{ + {"artist", EntityArtist, "orphan-artist"}, + {"release group", EntityReleaseGroup, "orphan-release-group"}, + {"recording", EntityRecording, "orphan-recording"}, + } + + for _, o := range orphans { + if _, err := db.ExecContext( + `INSERT INTO explore_index + (entity_type, mbid, title, artist_name, artist_mbid, + in_library, + local_artist_id, local_release_group_id, local_recording_id) + VALUES (?, ?, ?, ?, ?, 1, ?, ?, ?)`, + dbEntityType(o.entityType), dbMBID(testMBID(o.mbid)), o.name, o.name, + dbMBID(testMBID(o.mbid)), + nil, nil, nil, + ); err != nil { + t.Fatalf("seed %s orphan: %v", o.name, err) + } + } + + si.pruneStaleLocalCrossReferences() + + inLibrary := func(t *testing.T, mbid string) int { + t.Helper() + + var flag int + if err := db.QueryRowWriter( + "SELECT in_library FROM explore_index WHERE mbid = ?", dbMBID(mbid), + ).Scan(&flag); err != nil { + t.Fatalf("read in_library for %q: %v", mbid, err) + } + + return flag + } + + for _, o := range orphans { + if got := inLibrary(t, testMBID(o.mbid)); got != 0 { + t.Errorf("%s with a NULL local id: in_library = %d, want 0", o.name, got) + } + } + + if got := inLibrary(t, testMBID("owned")); got != 1 { + t.Errorf("owned artist: in_library = %d, want 1 (it still has a file)", got) + } +} + // TestUnenrichedLibraryArtistMBIDs_OrdersByOwnedTrackCount verifies the // backfill queue prioritizes artists by how many tracks the user actually // owns, not by how many duplicate-mbid artist rows happen to exist (the diff --git a/backend/explore/searchindex.go b/backend/explore/searchindex.go index d7a0448..4e4c7c5 100644 --- a/backend/explore/searchindex.go +++ b/backend/explore/searchindex.go @@ -2562,6 +2562,19 @@ func (si *SearchIndex) PopulateLocalCrossReferences() { // The row itself is left in place (it may still be part of the shipped // catalog, just no longer owned) — only the "this is mine" bookkeeping // is cleared. +// +// It is gated on the flag *or* the id, not on the id alone. Gated on +// the id, `in_library = 1 AND local_*_id IS NULL` is a fixed point: the +// upsert can only ever raise the flag and this pass skipped such a row +// by construction, so nothing in the app could clear it — a row claiming +// to be owned, permanently, with no local row to check the claim +// against. Nothing in the tree writes that shape today +// (collectLibraryEntities sets both together), which is exactly why it +// is worth closing now: the exposure is a database written by an older +// version, and the next writer that sets the flag without an id, which +// nothing structurally prevents. A NULL id fails the existence test on +// its own, so the wider gate needs no second clause to say what "not +// owned" means. func (si *SearchIndex) pruneStaleLocalCrossReferences() { type prune struct { entityType string @@ -2594,7 +2607,8 @@ func (si *SearchIndex) pruneStaleLocalCrossReferences() { result, err := si.db.ExecContext( `UPDATE explore_index SET in_library = 0, `+p.column+` = NULL - WHERE entity_type = ? AND `+p.column+` IS NOT NULL + WHERE entity_type = ? + AND (`+p.column+` IS NOT NULL OR in_library = 1) AND NOT EXISTS (`+p.exists+`)`, dbEntityType(p.entityType), ) diff --git a/frontend/src/components/home-view/home-view.ts b/frontend/src/components/home-view/home-view.ts index fbef07b..4d8ca74 100644 --- a/frontend/src/components/home-view/home-view.ts +++ b/frontend/src/components/home-view/home-view.ts @@ -160,29 +160,52 @@ export class HomeView extends ViewLifecycleMixin(LitElement) { user-select: none; } + /* + * The hover play button is a *hover* affordance, so it is + * gated on the device having hover rather than on width. A + * touch long-press synthesises a hover state in the WebView, + * so on a phone it flashed into view during the 500ms hold + * that utils/long-press.ts is measuring for a context menu — + * a control appearing because you were reaching for a + * different one. A phone user taps the album and plays from + * the detail view, so there is nothing to replace it with. + * + * display:none outside the query rather than opacity:0 on + * its own: an opacity-0 button still takes taps and is + * still in the accessibility tree, so the invisible control + * would keep the hit area it was never meant to have on + * touch. Everything else stays inside, so the desktop + * animation is unchanged. + */ .play { - position: absolute; - right: 8px; - bottom: 8px; - width: 38px; - height: 38px; - border: none; - border-radius: 50%; - background: var(--yj-accent, #ffd43b); - color: var(--yj-accent-fg, #000); - display: flex; - align-items: center; - justify-content: center; - cursor: pointer; - opacity: 0; - transform: translateY(6px); - transition: opacity 0.12s ease, transform 0.12s ease; + display: none; } - .card:hover .play, - .card:focus-within .play { - opacity: 1; - transform: translateY(0); + @media (hover: hover) and (pointer: fine) { + .play { + position: absolute; + right: 8px; + bottom: 8px; + width: 38px; + height: 38px; + border: none; + border-radius: 50%; + background: var(--yj-accent, #ffd43b); + color: var(--yj-accent-fg, #000); + display: flex; + align-items: center; + justify-content: center; + cursor: pointer; + opacity: 0; + transform: translateY(6px); + transition: opacity 0.12s ease, transform 0.12s ease; + } + + .card:hover .play, + .card:focus-within .play { + opacity: 1; + transform: translateY(0); + } } .name { diff --git a/frontend/src/components/now-playing/now-playing.ts b/frontend/src/components/now-playing/now-playing.ts index 4405695..0057f1d 100644 --- a/frontend/src/components/now-playing/now-playing.ts +++ b/frontend/src/components/now-playing/now-playing.ts @@ -13,6 +13,7 @@ import { isQueueSourceNavigable, navigateToQueueSource, } from '@utils/queue-source-link'; +import { PHONE_QUERY } from '@utils/breakpoints'; import { PlayerController } from '@store/controllers/player-controller'; import { creditStore } from '@store/credit-store'; import { QueueController } from '@store/controllers/queue-controller'; @@ -80,6 +81,19 @@ export class NowPlaying extends LitElement { private reduceMotionQuery?: MediaQueryList; + /** + * Phone width, from the shell's own breakpoint. + * + * This is in JS rather than in the stylesheet because what changes + * is the *content*, not its appearance: the title, artist and + * source render as plain text instead of as links, and no CSS rule + * can take a click handler off an element. + */ + @state() + private phone = false; + + private phoneQuery?: MediaQueryList; + /** Whether each field is actively mid-scroll (class toggle). */ @state() private titleScrolling = false; @@ -341,6 +355,12 @@ export class NowPlaying extends LitElement { this.reduceMotion = this.reduceMotionQuery?.matches ?? false; this.reduceMotionQuery?.addEventListener('change', this.handleReduceMotionChange); + // Same reasoning as above: looked up here, not at module load, + // so a test can install its own matchMedia first. + this.phoneQuery = window.matchMedia?.(PHONE_QUERY); + this.phone = this.phoneQuery?.matches ?? false; + this.phoneQuery?.addEventListener('change', this.handlePhoneChange); + this.resizeObserver = new ResizeObserver(() => { this.geometryDirty = true; this.requestUpdate(); @@ -364,6 +384,7 @@ export class NowPlaying extends LitElement { this.attachDragListeners(false); window.removeEventListener(SCROLL_CHANGE_EVENT, this.handleScrollModeEvent); this.reduceMotionQuery?.removeEventListener('change', this.handleReduceMotionChange); + this.phoneQuery?.removeEventListener('change', this.handlePhoneChange); this.resizeObserver?.disconnect(); this.stopScrollCycle('title'); this.stopScrollCycle('artist'); @@ -488,7 +509,7 @@ export class NowPlaying extends LitElement { @mouseleave=${this.handleTitleMouseLeave} @transitionend=${() => this.onScrollCycleEnd('title')} > - ${trackLink(track.title, track.album, track.releaseGroupMbid, track.recordingMbid) || track.title} + ${this.phone ? track.title : trackLink(track.title, track.album, track.releaseGroupMbid, track.recordingMbid) || track.title} this.onScrollCycleEnd('artist')} > - ${creditLink(creditStore.credits(track.recordingMbid), track.artist, track.artistMbid) || 'Unknown Artist'} + ${this.phone ? track.artist || 'Unknown Artist' : creditLink(creditStore.credits(track.recordingMbid), track.artist, track.artistMbid) || 'Unknown Artist'} ${describeQueueSource(this.queue.source) ? html` { + if (this.phone) return; if (!isQueueSourceNavigable(this.queue.source)) return; navigateToQueueSource( e.currentTarget as EventTarget, @@ -571,6 +593,10 @@ export class NowPlaying extends LitElement { this.reduceMotion = e.matches; }; + private handlePhoneChange = (e: MediaQueryListEvent): void => { + this.phone = e.matches; + }; + private shouldScroll(field: 'title' | 'artist'): boolean { const overflows = field === 'title' ? this.titleOverflows : this.artistOverflows; @@ -606,6 +632,12 @@ export class NowPlaying extends LitElement { track?.artist ?? '', this.shouldScroll('title') ? '1' : '0', this.shouldScroll('artist') ? '1' : '0', + // Crossing the breakpoint swaps a link for a bare string, + // and a link is not guaranteed to measure the same as the + // text inside it. The marquee travels a distance read from + // that measurement, so this belongs in the key even though + // the words are identical either side. + this.phone ? '1' : '0', ].join('\u0000'); } diff --git a/frontend/src/components/track-list/track-list.ts b/frontend/src/components/track-list/track-list.ts index df61e31..d278389 100644 --- a/frontend/src/components/track-list/track-list.ts +++ b/frontend/src/components/track-list/track-list.ts @@ -11,6 +11,7 @@ import { import { SelectionController } from '@utils/selection-controller'; import type { SelectionHost } from '@utils/selection-controller'; import { ViewLifecycleMixin } from '@utils/view-lifecycle'; +import { PHONE_QUERY } from '@utils/breakpoints'; import { ContextMenuController, contextMenuStyles, @@ -105,9 +106,6 @@ const ROW_CHROME_WIDTH = const ROW_HEIGHT = 33; const PHONE_ROW_HEIGHT = 52; -/** The shell's phone breakpoint, as `index.css` and every component - * stylesheet spells it. */ -const PHONE_QUERY = '(max-width: 599px)'; // Inline SVG paths for favorite icons — eliminates wa-icon shadow DOM // overhead (30-50 shadow roots during scroll). Font Awesome 6 paths. diff --git a/frontend/src/utils/breakpoints.ts b/frontend/src/utils/breakpoints.ts new file mode 100644 index 0000000..2c5b026 --- /dev/null +++ b/frontend/src/utils/breakpoints.ts @@ -0,0 +1,23 @@ +/** + * The shell's breakpoints, where JavaScript has to agree with CSS. + * + * A media query inside a shadow root is answered by the viewport, so a + * component normally states what it drops at phone width in its own + * stylesheet and needs nothing from here. This exists for the cases + * where the decision is not a style: `track-list` computes its grid in + * JS from the host width, and `now-playing` renders *different content* + * on a phone — a plain string instead of a link — which no stylesheet + * can express. + * + * One breakpoint, several expressions of it. It was a private const in + * track-list.ts when there was one; a second reader is where a copy + * would start drifting from index.css. + */ + +/** + * Phone width. 600px rather than the sidebar's 900px because 900 is a + * laptop: the answer there is a narrower sidebar, which is still a + * sidebar. Below this the shell drops the sidebar column entirely and + * bottom-nav takes over. + */ +export const PHONE_QUERY = '(max-width: 599px)'; diff --git a/frontend/test/components/hover-affordance.test.ts b/frontend/test/components/hover-affordance.test.ts new file mode 100644 index 0000000..78d671c --- /dev/null +++ b/frontend/test/components/hover-affordance.test.ts @@ -0,0 +1,85 @@ +/** + * A hover affordance is gated on the device having hover. + * + * The home page's cover cards reveal a play button on :hover. A touch + * long-press synthesises a hover state in the WebView, so on a phone + * that button flashed into view during the 500ms hold that + * utils/long-press.ts is measuring for a context menu — a control + * appearing because the user was reaching for a different one. + * + * This is asserted against the *parsed stylesheet* rather than by + * emulating a touch device, and that is a limitation worth stating + * rather than hiding. CDP's Emulation.setEmulatedMedia does not reach + * this tier's iframe — matchMedia still answers `hover: hover` after it + * is set — so there is no way here to render the component as a phone + * would and read the computed style. What can be checked is the shape + * the browser actually built from the css`` literal: that the reveal + * lives inside a hover media query and that the default is display:none. + * + * Which is the regression worth catching anyway. The failure mode is + * someone hoisting the rule back out of the query for a one-line tidy — + * a change nothing renders differently on a desktop, so every other + * assertion in this repo passes and the phone silently regresses. + */ +import { describe, expect, it } from 'vitest'; + +import '@components/home-view/home-view'; +import { fixture } from '@test/support/render'; + +/** Every rule in the element's own adopted stylesheets, flattened. */ +function rulesOf(host: Element): { text: string; condition: string | null }[] { + const sheets = host.shadowRoot?.adoptedStyleSheets ?? []; + const out: { text: string; condition: string | null }[] = []; + + for (const sheet of sheets) { + for (const rule of Array.from(sheet.cssRules)) { + if (rule instanceof CSSMediaRule) { + for (const inner of Array.from(rule.cssRules)) { + out.push({ text: inner.cssText, condition: rule.conditionText }); + } + + continue; + } + + out.push({ text: rule.cssText, condition: null }); + } + } + + return out; +} + +describe('the home card play button', () => { + it('reveals itself only where the device has hover', async () => { + const el = await fixture('home-view', {}); + const rules = rulesOf(el); + + // The sweep is worth nothing if it read no rules at all — the same + // first assertion icon-language.test.ts makes for the same reason. + expect(rules.length).toBeGreaterThan(0); + + const reveals = rules.filter( + (r) => r.text.includes('.play') && /opacity:\s*1/.test(r.text), + ); + + expect(reveals.length).toBeGreaterThan(0); + + for (const rule of reveals) { + expect(rule.condition).toMatch(/hover:\s*hover/); + expect(rule.condition).toMatch(/pointer:\s*fine/); + } + }); + + it('is display:none rather than transparent where it is absent', async () => { + const el = await fixture('home-view', {}); + + // opacity:0 alone would leave a button that still takes taps and is + // still in the accessibility tree, so a phone would keep the hit + // area for a control it can never see. + const unconditional = rulesOf(el).filter( + (r) => r.condition === null && r.text.startsWith('.play'), + ); + + expect(unconditional.length).toBeGreaterThan(0); + expect(unconditional.some((r) => /display:\s*none/.test(r.text))).toBe(true); + }); +}); diff --git a/frontend/test/components/now-playing-phone.test.ts b/frontend/test/components/now-playing-phone.test.ts new file mode 100644 index 0000000..027e5c5 --- /dev/null +++ b/frontend/test/components/now-playing-phone.test.ts @@ -0,0 +1,166 @@ +/** + * The mini player's links are a desktop affordance. + * + * `utils/explore-link.ts` makes every track and artist name navigate, + * and `utils/queue-source-link.ts` makes "Playing from X" navigate — in + * the bottom bar those are a few characters of text at a font size + * chosen for a bar, which is not a touch target. Worse, explore-link + * holds the navigation for one double-click interval and drops it if a + * second click arrives: a gesture that exists so double-clicking a row + * can play it, and which means nothing at all on touch. + * + * So below the shell's phone breakpoint the three render as plain text + * and the whole bar's cover art opens the full-screen Now Playing view, + * which is where the links live. + * + * The breakpoint is stubbed rather than emulated for the reason + * track-list-phone.test.ts states: this tier's viewport is fixed at + * 1280x800 by the runner, and the component reads matchMedia in + * connectedCallback precisely so a test can answer it first. + */ +import { describe, expect, it, beforeEach } from 'vitest'; + +import '@components/now-playing/now-playing'; +import { Events } from '../../src/events'; +import { emit, flush } from '@test/support/harness'; +import { fixture, shadow, shadowAll, text } from '@test/support/render'; +import type { TrackInfo } from '@store/player-store'; +import type { QueueTrack } from '@store/queue-store'; + +const TRACK: TrackInfo = { + fileName: 'ashes.mp3', + filePath: '/music/ashes.mp3', + trackLength: 215, + seekPosition: 0, + state: 'playing', + title: 'Ashes to Ashes', + artist: 'David Bowie', + album: 'Scary Monsters', + coverArt: '', + coverArtSmall: '', + coverArtMedium: '', + coverArtLarge: '', + trackChangeId: 1, + artistMbid: '', + releaseGroupMbid: '', + recordingMbid: '', +}; + +function queueTrack(n: number, title: string): QueueTrack { + return { + id: n, + audioFileId: n, + filePath: `/music/${n}.mp3`, + position: n, + title, + artist: 'David Bowie', + album: 'Scary Monsters', + coverArtPath: '', + artistMbid: '', + releaseGroupMbid: '', + recordingMbid: '', + }; +} + +/** Mount the bar with the phone breakpoint answering `matches`. */ +async function mountAt(phone: boolean) { + const real = window.matchMedia.bind(window); + + window.matchMedia = ((q: string) => + q.includes('max-width: 599px') + ? { + matches: phone, + media: q, + addEventListener() {}, + removeEventListener() {}, + } + : real(q)) as typeof window.matchMedia; + + try { + const el = await fixture('now-playing'); + + emit(Events.TrackChanged, { ...TRACK, trackChangeId: 20 }); + emit(Events.QueueChanged, { + tracks: [queueTrack(1, 'Ashes to Ashes')], + currentIndex: 0, + source: { type: 'album', id: 7, label: 'Scary Monsters' }, + }); + await flush(); + await el.updateComplete; + + return el; + } finally { + window.matchMedia = real; + } +} + +describe('the mini player on a phone', () => { + beforeEach(() => { + emit(Events.TrackChanged, { ...TRACK, trackChangeId: 1 }); + }); + + it('renders the title and artist as plain text', async () => { + const el = await mountAt(true); + + expect(shadowAll(el, '.explore-link').length).toBe(0); + + // The words are unchanged — this is about what they are, not about + // hiding them. A fix that dropped the text would pass an assertion + // about links alone. + expect(text(el, '[data-testid="now-playing-title"]')).toContain( + 'Ashes to Ashes', + ); + expect(text(el, '[data-testid="now-playing-artist"]')).toContain( + 'David Bowie', + ); + }); + + it('does not navigate from the source line', async () => { + const el = await mountAt(true); + const source = shadow(el, '[data-testid="now-playing-source"]'); + + expect(source?.classList.contains('navigable')).toBe(false); + + let navigated = false; + el.addEventListener('navigate', () => { + navigated = true; + }); + + source?.click(); + + expect(navigated).toBe(false); + }); + + it('still says where the queue came from', async () => { + const el = await mountAt(true); + + // Dropping the *link* is the change; dropping the information would + // be a different and worse one. + expect(text(el, '[data-testid="now-playing-source"]')).toBe( + 'Playing from Scary Monsters', + ); + }); + + it('leaves the desktop bar exactly as it was', async () => { + const el = await mountAt(false); + + expect(shadowAll(el, '.explore-link').length).toBeGreaterThan(0); + + const source = shadow(el, '[data-testid="now-playing-source"]'); + + expect(source?.classList.contains('navigable')).toBe(true); + + let detail: unknown; + el.addEventListener('navigate', (e) => { + detail = (e as CustomEvent).detail; + }); + + source?.click(); + + expect(detail).toEqual({ + view: 'explore-album-details', + localAlbumId: 7, + albumName: 'Scary Monsters', + }); + }); +}); diff --git a/lefthook.yml b/lefthook.yml index 7c56e83..70a03ba 100644 --- a/lefthook.yml +++ b/lefthook.yml @@ -20,19 +20,14 @@ pre-commit: glob: "*.go" run: go tool golangci-lint run --timeout 5m ./... + # Snapshots the tree either side of the generators and reports only + # what moved across them. This used to be `go generate` plus a bare + # `git diff --name-only`, which is the *whole unstaged worktree* — so + # any unrelated edit sitting there was reported as stale generated + # code, and `make generate` then fixed nothing. See the script. codegen-check: glob: "*.{go,sql,templ}" - run: | - go generate ./... - if [ -n "$(git diff --name-only)" ]; then - echo "Generated code is out of date. Run 'make generate' and stage the changes." - # --no-pager, or this blocks forever on `less` waiting for a - # keypress that a hook run without a tty will never get: the - # commit hangs at exactly the moment it is trying to tell you - # why it failed. - git --no-pager diff --stat - exit 1 - fi + run: ./scripts/codegen-check.sh # frontend/bindings is generated by `wails3`, not `go generate`, so # the check above does not cover it. ~3.5s warm, ~20s on a cold diff --git a/scripts/codegen-check.sh b/scripts/codegen-check.sh new file mode 100755 index 0000000..846b1a1 --- /dev/null +++ b/scripts/codegen-check.sh @@ -0,0 +1,81 @@ +#!/usr/bin/env bash +# +# Fails when `go generate ./...` would change something that is not staged. +# +# The obvious spelling of this is `go generate && git diff --name-only`, +# which is what the hook used to be, and it answers the wrong question: +# that diff is the *whole unstaged worktree*, so any unrelated edit — a +# note, a plan document, the next commit's files sitting there while this +# one lands — was reported as +# +# Generated code is out of date. Run 'make generate' and stage the changes. +# +# Running `make generate` then does nothing, because nothing generated is +# stale, and the message sends you looking for a codegen problem that does +# not exist. Splitting one piece of work into several commits is exactly +# the shape that triggers it, so the workaround was a constraint on commit +# order for no real reason. +# +# So the tree is snapshotted either side of the generators and only what +# *moved across them* is reported. That is deliberately not a list of +# generated paths: sqlcgen, `*_templ.go` and `frontend/src/events.ts` are +# today's answer, a fourth generator is one `//go:generate` line away, and +# a path list is a second place to remember it — the same reasoning that +# keeps staleshape.go parsing sql/schemas/ rather than restating it. +# +# Content, not names: a generated file that is *already* dirty and is then +# rewritten further keeps its name in both snapshots and would otherwise +# slip through. + +set -euo pipefail + +cd "$(dirname "$0")/.." + +# name + worktree blob hash for every file that differs from the index. +# A file listed but absent (a deletion) hashes as "gone" rather than +# aborting the pipeline. +snapshot() { + git diff --name-only | while IFS= read -r f; do + if [ -f "$f" ]; then + printf '%s %s\n' "$f" "$(git hash-object -- "$f")" + else + printf '%s gone\n' "$f" + fi + done +} + +# A brand-new generated file is not in either diff, because it is not +# tracked at all — the same blind spot bindings-check.sh names. Both +# snapshots are taken before the generators run. +before="$(snapshot)" +before_untracked="$(git ls-files --others --exclude-standard)" + +go generate ./... + +after="$(snapshot)" +after_untracked="$(git ls-files --others --exclude-standard)" + +# Symmetric difference, and the symmetry is the whole point. Generation +# can push a file *into* the unstaged set (it was current, now it is not) +# or *out* of it (someone hand-edited generated output and the generator +# put it back) — and the second is stale generated code just as much as +# the first. Comparing one direction only reports "current" for it, +# which is the failure this script was written to stop. +moved="$(comm -3 <(printf '%s\n' "$before" | sort) <(printf '%s\n' "$after" | sort) | + cut -d' ' -f1 | tr -d '\t' | sort -u | grep -v '^$' || true)" + +if [ -n "$moved" ]; then + echo "codegen-check: generated code is out of date." >&2 + echo "Run 'make generate' and stage:" >&2 + printf ' %s\n' $moved >&2 + exit 1 +fi + +if [ "$after_untracked" != "$before_untracked" ]; then + echo "codegen-check: generation produced new files. Stage them:" >&2 + comm -13 <(printf '%s\n' "$before_untracked" | sort) \ + <(printf '%s\n' "$after_untracked" | sort) >&2 + exit 1 +fi + +echo "codegen-check: generated code is current" diff --git a/scripts/dev-headless.sh b/scripts/dev-headless.sh index 22ff732..c8a9de0 100755 --- a/scripts/dev-headless.sh +++ b/scripts/dev-headless.sh @@ -97,6 +97,55 @@ if [ -f "$PID_FILE" ] && kill -0 "$(cat "$PID_FILE")" 2>/dev/null; then fi rm -f "$PID_FILE" +# ── Refuse to inherit somebody else's port ─────────────────────────── +# The PID check above only knows about *this* worktree: `make dev-stop` +# kills the pid in this .dev/app.pid and nothing else. Several worktrees +# of this repo share the default port, so an app orphaned by a deleted +# worktree goes on listening with nothing left to stop it. +# +# Without this check the new app starts, fails to bind, exits — and every +# curl and playwright-cli call afterwards goes to the *other* process, so +# the harness reports facts about an app nobody asked for. That is not a +# quiet wrongness either: it presented as +# "no such table: libraries" against a freshly created YJ_HOME, which +# reads exactly like applySchema or staleshape.go having gone wrong and +# is a frightening place to start looking. +# +# The startup wait below cannot catch it, because the health check is +# satisfied by *any* app on the port — which is precisely the failure. +# So it is refused here, before anything is launched, rather than warned +# about. --port already exists for the legitimate second-app case. +port_holder() { + command -v ss >/dev/null || return 0 + ss -lptn "sport = :$PORT" 2>/dev/null | grep -oP 'pid=\K[0-9]+' | head -n 1 +} + +if curl -sf -o /dev/null --max-time 2 "http://localhost:$PORT/" || + [ -n "$(port_holder)" ]; then + holder="$(port_holder)" + echo "dev-headless: :$PORT is already in use; refusing to start" >&2 + if [ -n "$holder" ]; then + # /proc//cwd names the checkout it belongs to, and says + # "(deleted)" for the orphaned-worktree case that is the whole + # reason this is worth a check. + cwd="$(readlink "/proc/$holder/cwd" 2>/dev/null || echo unknown)" + cmd="$(tr '\0' ' ' <"/proc/$holder/cmdline" 2>/dev/null || echo unknown)" + echo " pid $holder ($cmd)" >&2 + echo " cwd $cwd" >&2 + # The PID-file check above has already passed, so whatever this + # is, `make dev-stop` does not know about it — saying otherwise + # sends you to a command that will report success and change + # nothing. Never `pkill -f` here either: the pattern would + # match this script's own command line. + echo " 'make dev-stop' will not touch it (it is not in" >&2 + echo " ${PID_FILE#"$REPO_ROOT"/}): kill $holder, or pass --port." >&2 + else + echo " The holder could not be identified (no ss, or it belongs" >&2 + echo " to another user). Try: ss -lptn 'sport = :$PORT'" >&2 + fi + exit 1 +fi + # ── Choose the YJ_HOME ─────────────────────────────────────────────── # A seed is a YJ_HOME that a previous run of the app produced, tarred # up (see scripts/seed-sandbox.sh). Restoring it means starting *in* @@ -200,6 +249,20 @@ until curl -sf -o /dev/null "http://localhost:$PORT/"; do sleep 0.25 done +# The loop above exits on the first answer from the port, and "something +# answered" is not "the app we started answered". The pre-launch guard +# makes that unlikely rather than impossible — a race, or a listener +# started in between — and the check is one signal, so it is worth making +# here too. An empty log beside a dead pid is the "it exited immediately +# and nothing said so" case that the original report spent its time on. +if ! kill -0 "$APP_PID" 2>/dev/null; then + echo "dev-headless: :$PORT answered, but the app we started (pid" >&2 + echo " $APP_PID) is gone — something else holds the port." >&2 + tail -n 30 "$LOG_FILE" >&2 + rm -f "$PID_FILE" + exit 1 +fi + cat <&2 + echo "issue.sh: re-issue GITEA_TOKEN with read:user." >&2 + exit 1 + } +} label_id() { call GET "/labels?limit=100" | python3 "$py" label-id "$1"; }