From ac718b922b7c79497279a46d828280a6f6b8fc0c Mon Sep 17 00:00:00 2001 From: Robin Scher Date: Tue, 18 Aug 2026 13:18:24 -0700 Subject: [PATCH 01/20] feat(store): add product readme column for long-form documentation --- internal/store/fake/product_test.go | 20 ++++++ .../migrations/00029_products_readme.sql | 29 ++++++++ internal/store/product.go | 14 ++-- internal/store/sqlite/product.go | 20 +++--- internal/store/sqlite/product_test.go | 67 +++++++++++++++++++ 5 files changed, 135 insertions(+), 15 deletions(-) create mode 100644 internal/store/migrations/00029_products_readme.sql diff --git a/internal/store/fake/product_test.go b/internal/store/fake/product_test.go index a0af25b2..473763d4 100644 --- a/internal/store/fake/product_test.go +++ b/internal/store/fake/product_test.go @@ -83,3 +83,23 @@ func TestFakeProduct_OriginRoundTrip(t *testing.T) { t.Fatalf("fake dropped origin: %+v", got) } } + +// The fake stores the whole store.Product value, so Readme needs no explicit +// handling. This test exists so a future refactor to field-by-field copying +// cannot silently drop it. +func TestFakeProduct_ReadmeRoundTrips(t *testing.T) { + st := fake.New() + ctx := context.Background() + if _, err := st.CreateProduct(ctx, store.Product{ + ID: "p1", Name: "readme-probe", Title: "Readme Probe", Readme: "# Docs\n", + }); err != nil { + t.Fatalf("create: %v", err) + } + got, err := st.GetProductByName(ctx, "readme-probe") + if err != nil { + t.Fatalf("get: %v", err) + } + if got.Readme != "# Docs\n" { + t.Errorf("readme = %q, want %q", got.Readme, "# Docs\n") + } +} diff --git a/internal/store/migrations/00029_products_readme.sql b/internal/store/migrations/00029_products_readme.sql new file mode 100644 index 00000000..db72e2bb --- /dev/null +++ b/internal/store/migrations/00029_products_readme.sql @@ -0,0 +1,29 @@ +-- SPDX-License-Identifier: AGPL-3.0-or-later +-- Long-form markdown documentation for a product. +-- +-- Separate from `description` because the two serve incompatible jobs. +-- `description` must fit a picker card and a native Blender EnumProperty +-- tooltip, which is why commit a1e529e shortened every shipped preset's +-- description by hand -- the longest, 940 characters on +-- ffmpeg-segment-transcode-expr, was documentation wearing a blurb's clothes. +-- Markdown in `description` would not have helped: it adds formatting, not +-- length budget. So the blurb stays short, plain and searchable, and the +-- documentation moves here. +-- +-- `readme` is deliberately NOT searched. That is what keeps the change small: +-- with no search over it, no markdown stripper is needed in either TypeScript +-- or Python, and presetlib.IndexEntry needs no readme field, so the remote +-- preset-index format is unchanged. +-- +-- NOT NULL DEFAULT '' matches `description` and every other late-added string +-- column in this schema (see 00028's note): scanProduct reads it into a plain +-- string, so a NULL would be a scan error rather than an empty value. +-- +-- The Down migration's ALTER TABLE ... DROP COLUMN requires SQLite >= 3.35.0, +-- the same note 00002, 00008, 00013, 00026 and 00028 carry. + +-- +goose Up +ALTER TABLE products ADD COLUMN readme TEXT NOT NULL DEFAULT ''; + +-- +goose Down +ALTER TABLE products DROP COLUMN readme; diff --git a/internal/store/product.go b/internal/store/product.go index 1fab0d7c..1e70f408 100644 --- a/internal/store/product.go +++ b/internal/store/product.go @@ -27,11 +27,15 @@ type Product struct { Name string // stable identity, e.g. "script", "studio/maya-render" Title string Description string - Category string - Version string - Source Source - Template string // verbatim OpenJD template - Format TemplateFormat + // Readme is long-form markdown documentation, rendered on detail pages + // only. Unlike Description it is never searched and never reaches a + // plain-text consumer; see the field table in docs/products.md. + Readme string + Category string + Version string + Source Source + Template string // verbatim OpenJD template + Format TemplateFormat // OriginRef is the preset-library index entry name this product was // installed from; empty for builtin/custom products. OriginRef string diff --git a/internal/store/sqlite/product.go b/internal/store/sqlite/product.go index efdc64d2..c3eaa467 100644 --- a/internal/store/sqlite/product.go +++ b/internal/store/sqlite/product.go @@ -11,23 +11,23 @@ import ( const ( sqlInsertProduct = ` -INSERT INTO products (id, name, title, description, category, version, source, template, format, origin_ref, origin_fingerprint, created_at, updated_at) -VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) -RETURNING id, name, title, description, category, version, source, template, format, origin_ref, origin_fingerprint, created_at, updated_at` +INSERT INTO products (id, name, title, description, readme, category, version, source, template, format, origin_ref, origin_fingerprint, created_at, updated_at) +VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) +RETURNING id, name, title, description, readme, category, version, source, template, format, origin_ref, origin_fingerprint, created_at, updated_at` sqlGetProductByName = ` -SELECT id, name, title, description, category, version, source, template, format, origin_ref, origin_fingerprint, created_at, updated_at +SELECT id, name, title, description, readme, category, version, source, template, format, origin_ref, origin_fingerprint, created_at, updated_at FROM products WHERE name = ?` sqlListProducts = ` -SELECT id, name, title, description, category, version, source, template, format, origin_ref, origin_fingerprint, created_at, updated_at +SELECT id, name, title, description, readme, category, version, source, template, format, origin_ref, origin_fingerprint, created_at, updated_at FROM products ORDER BY name` sqlUpdateProduct = ` UPDATE products -SET title = ?, description = ?, category = ?, version = ?, template = ?, format = ?, origin_ref = ?, origin_fingerprint = ?, updated_at = ? +SET title = ?, description = ?, readme = ?, category = ?, version = ?, template = ?, format = ?, origin_ref = ?, origin_fingerprint = ?, updated_at = ? WHERE name = ? -RETURNING id, name, title, description, category, version, source, template, format, origin_ref, origin_fingerprint, created_at, updated_at` +RETURNING id, name, title, description, readme, category, version, source, template, format, origin_ref, origin_fingerprint, created_at, updated_at` sqlDeleteProduct = `DELETE FROM products WHERE name = ?` ) @@ -36,7 +36,7 @@ func scanProduct(row scanner) (store.Product, error) { var p store.Product var source, format, createdAt, updatedAt string if err := row.Scan( - &p.ID, &p.Name, &p.Title, &p.Description, &p.Category, &p.Version, + &p.ID, &p.Name, &p.Title, &p.Description, &p.Readme, &p.Category, &p.Version, &source, &p.Template, &format, &p.OriginRef, &p.OriginFingerprint, &createdAt, &updatedAt, ); err != nil { return store.Product{}, err @@ -52,7 +52,7 @@ func scanProduct(row scanner) (store.Product, error) { func (s *Store) CreateProduct(ctx context.Context, p store.Product) (store.Product, error) { now := timeToText(time.Now().UTC()) row := s.stmtInsertProduct.QueryRowContext(ctx, - p.ID, p.Name, p.Title, p.Description, p.Category, p.Version, + p.ID, p.Name, p.Title, p.Description, p.Readme, p.Category, p.Version, string(p.Source), p.Template, string(p.Format), p.OriginRef, p.OriginFingerprint, now, now) out, err := scanProduct(row) return out, mapErr(err) @@ -87,7 +87,7 @@ func (s *Store) ListProducts(ctx context.Context) ([]store.Product, error) { func (s *Store) UpdateProduct(ctx context.Context, p store.Product) (store.Product, error) { now := timeToText(time.Now().UTC()) row := s.stmtUpdateProduct.QueryRowContext(ctx, - p.Title, p.Description, p.Category, p.Version, + p.Title, p.Description, p.Readme, p.Category, p.Version, p.Template, string(p.Format), p.OriginRef, p.OriginFingerprint, now, p.Name) out, err := scanProduct(row) return out, mapErr(err) diff --git a/internal/store/sqlite/product_test.go b/internal/store/sqlite/product_test.go index 8bc823cf..4756a02b 100644 --- a/internal/store/sqlite/product_test.go +++ b/internal/store/sqlite/product_test.go @@ -148,3 +148,70 @@ func TestProduct_UpdateOriginRoundTrip(t *testing.T) { t.Fatalf("persisted origin wrong: ref=%q fp=%q", fetched.OriginRef, fetched.OriginFingerprint) } } + +func TestProduct_ReadmeRoundTrips(t *testing.T) { + st := newProductStore(t) + ctx := context.Background() + + const readme = "# Heading\n\nBody with code.\n" + created, err := st.CreateProduct(ctx, store.Product{ + ID: "p1", Name: "readme-probe", Title: "Readme Probe", + Description: "short blurb", Readme: readme, + Source: store.SourceCustom, Template: "specificationVersion: jobtemplate-2023-09\nname: X\nsteps: []", Format: store.TemplateFormatYAML, + }) + if err != nil { + t.Fatalf("create: %v", err) + } + if created.Readme != readme { + t.Errorf("create returned readme %q, want %q", created.Readme, readme) + } + + got, err := st.GetProductByName(ctx, "readme-probe") + if err != nil { + t.Fatalf("get: %v", err) + } + if got.Readme != readme { + t.Errorf("get returned readme %q, want %q", got.Readme, readme) + } + + list, err := st.ListProducts(ctx) + if err != nil { + t.Fatalf("list: %v", err) + } + if len(list) != 1 || list[0].Readme != readme { + t.Errorf("list returned %+v, want one row carrying the readme", list) + } + + updated, err := st.UpdateProduct(ctx, store.Product{ + Name: "readme-probe", Title: "Readme Probe", Description: "short blurb", + Readme: "replaced", Template: "specificationVersion: jobtemplate-2023-09\nname: X\nsteps: []", Format: store.TemplateFormatYAML, + }) + if err != nil { + t.Fatalf("update: %v", err) + } + if updated.Readme != "replaced" { + t.Errorf("update returned readme %q, want %q", updated.Readme, "replaced") + } +} + +// A product created without a readme reads back as "" rather than failing the +// scan. That is what the migration's empty-string default buys for +// pre-existing rows. +func TestProduct_ReadmeDefaultsEmpty(t *testing.T) { + st := newProductStore(t) + ctx := context.Background() + + if _, err := st.CreateProduct(ctx, store.Product{ + ID: "p2", Name: "no-readme", Title: "No Readme", + Source: store.SourceCustom, Template: "specificationVersion: jobtemplate-2023-09\nname: Y\nsteps: []", Format: store.TemplateFormatYAML, + }); err != nil { + t.Fatalf("create: %v", err) + } + got, err := st.GetProductByName(ctx, "no-readme") + if err != nil { + t.Fatalf("get: %v", err) + } + if got.Readme != "" { + t.Errorf("readme = %q, want empty", got.Readme) + } +} From a7e62413e54105948ce5ef88e198d7063da9311b Mon Sep 17 00:00:00 2001 From: Robin Scher Date: Tue, 18 Aug 2026 13:24:39 -0700 Subject: [PATCH 02/20] feat(product): add readme field to product definitions --- internal/product/definition.go | 2 + internal/product/definition_test.go | 62 +++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+) diff --git a/internal/product/definition.go b/internal/product/definition.go index 938f8902..d5ee21e0 100644 --- a/internal/product/definition.go +++ b/internal/product/definition.go @@ -152,6 +152,7 @@ type definitionFile struct { Name string `yaml:"name"` Title string `yaml:"title"` Description string `yaml:"description"` + Readme string `yaml:"readme"` Category string `yaml:"category"` Version string `yaml:"version"` Template yaml.Node `yaml:"template"` @@ -199,6 +200,7 @@ func ParseDefinition(data []byte, opts ValidateOptions) (store.Product, error) { Name: df.Name, Title: df.Title, Description: df.Description, + Readme: df.Readme, Category: df.Category, Version: df.Version, Template: string(rawTemplate), diff --git a/internal/product/definition_test.go b/internal/product/definition_test.go index 35863a57..c1d8fcac 100644 --- a/internal/product/definition_test.go +++ b/internal/product/definition_test.go @@ -80,3 +80,65 @@ func TestParseDefinition_Errors(t *testing.T) { }) } } + +func TestParseDefinition_Readme(t *testing.T) { + t.Parallel() + const def = ` +name: readme-probe +title: Readme Probe +description: A short blurb. +readme: | + # Readme Probe + + Does a thing. +category: General +version: 1.0.0 +template: + specificationVersion: jobtemplate-2023-09 + name: Readme Probe + steps: + - name: Step + script: + actions: + onRun: + command: echo + args: ["hi"] +` + p, err := product.ParseDefinition([]byte(def), product.ValidateOptions{EnforceLimits: true}) + if err != nil { + t.Fatalf("ParseDefinition: %v", err) + } + want := "# Readme Probe\n\nDoes a thing.\n" + if p.Readme != want { + t.Errorf("Readme = %q, want %q", p.Readme, want) + } + if p.Description != "A short blurb." { + t.Errorf("Description = %q, want the blurb unchanged", p.Description) + } +} + +// A definition with no readme yields the empty string, not an error. +func TestParseDefinition_ReadmeOptional(t *testing.T) { + t.Parallel() + const def = ` +name: no-readme +title: No Readme +template: + specificationVersion: jobtemplate-2023-09 + name: No Readme + steps: + - name: Step + script: + actions: + onRun: + command: echo + args: ["hi"] +` + p, err := product.ParseDefinition([]byte(def), product.ValidateOptions{EnforceLimits: true}) + if err != nil { + t.Fatalf("ParseDefinition: %v", err) + } + if p.Readme != "" { + t.Errorf("Readme = %q, want empty", p.Readme) + } +} From 9a2e0272bfab3d32c395fec9fcdb61f3e538f6c8 Mon Sep 17 00:00:00 2001 From: Robin Scher Date: Tue, 18 Aug 2026 13:34:12 -0700 Subject: [PATCH 03/20] feat(product): add metadata length limits and validate before templates --- internal/product/definition.go | 19 ++++--- internal/product/limits.go | 77 ++++++++++++++++++++++++++ internal/product/limits_test.go | 96 +++++++++++++++++++++++++++++++++ 3 files changed, 186 insertions(+), 6 deletions(-) create mode 100644 internal/product/limits.go create mode 100644 internal/product/limits_test.go diff --git a/internal/product/definition.go b/internal/product/definition.go index d5ee21e0..c0d7a1ca 100644 --- a/internal/product/definition.go +++ b/internal/product/definition.go @@ -26,6 +26,9 @@ func validateName(name string) error { if name == "" { return errors.New("product: name is required") } + if err := checkLen("name", name, MaxNameLen); err != nil { + return err + } if !slugPattern.MatchString(name) { return fmt.Errorf("product: name %q is not a valid slug (lowercase, digits, '-', '_', one optional '/')", name) } @@ -193,17 +196,21 @@ func ParseDefinition(data []byte, opts ValidateOptions) (store.Product, error) { if err != nil { return store.Product{}, fmt.Errorf("product: re-serialize template: %w", err) } - if err := ValidateTemplate(string(rawTemplate), store.TemplateFormatYAML, opts); err != nil { - return store.Product{}, err - } - return store.Product{ + p := store.Product{ Name: df.Name, Title: df.Title, Description: df.Description, Readme: df.Readme, Category: df.Category, Version: df.Version, - Template: string(rawTemplate), Format: store.TemplateFormatYAML, - }, nil + } + if err := ValidateMetadata(p); err != nil { + return store.Product{}, err + } + if err := ValidateTemplate(string(rawTemplate), store.TemplateFormatYAML, opts); err != nil { + return store.Product{}, err + } + p.Template = string(rawTemplate) + return p, nil } diff --git a/internal/product/limits.go b/internal/product/limits.go new file mode 100644 index 00000000..59fe443e --- /dev/null +++ b/internal/product/limits.go @@ -0,0 +1,77 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package product + +import ( + "fmt" + "unicode/utf8" + + "github.com/uberware/sqi/internal/store" +) + +// Metadata length limits, in RUNES rather than bytes: 500 bytes of CJK is ~166 +// characters, and a Japanese-language description must not be silently +// third-class. +// +// The two interesting caps differ in kind. MaxDescriptionLen is a design +// constraint -- description is rendered into an unclamped picker card and a +// native Blender EnumProperty tooltip, and 500 is 1.5x the longest description +// that survived commit a1e529e's hand-trim (329). It would have REJECTED the +// 940-, 629- and 617-rune descriptions that forced that commit, which is the +// test a cap should pass; a 1000 cap would have permitted all three. +// MaxReadmeLen is only an abuse guard -- readme is detail-page-only, so nothing +// downstream breaks; it simply should not be a novel. +// +// The rest have far more headroom than the shipped presets need (their maxima +// are name 35, title 37, category 11). They exist because capping description +// while leaving its neighbors unbounded would be incoherent once the helper +// exists. +const ( + MaxNameLen = 128 + MaxTitleLen = 200 + MaxDescriptionLen = 500 + MaxReadmeLen = 8000 + MaxCategoryLen = 64 + MaxVersionLen = 32 +) + +// checkLen returns an error naming the field, the actual rune count and the cap +// when value is longer than maxRunes. +func checkLen(field, value string, maxRunes int) error { + if n := utf8.RuneCountInString(value); n > maxRunes { + return fmt.Errorf("product: %s is %d characters, limit is %d", field, n, maxRunes) + } + return nil +} + +// ValidateMetadata enforces the length limits on a product's metadata fields. +// +// It is exported and deliberately called from BOTH doors into product data -- +// ParseDefinition and the REST create/update handler. Those are separate entry +// points to the same data, and the comment on ValidateOptions records this exact +// trap biting once already: the preset routes silently kept validating on +// DefaultExprLimits() after the create/update route was fixed. +// +// It checks LENGTH only. The slug pattern stays in validateName on the +// definition path, because applying it to the REST route would tighten +// acceptance and strand products already stored under a pattern-invalid name. +func ValidateMetadata(p store.Product) error { + checks := []struct { + field string + value string + max int + }{ + {"name", p.Name, MaxNameLen}, + {"title", p.Title, MaxTitleLen}, + {"description", p.Description, MaxDescriptionLen}, + {"readme", p.Readme, MaxReadmeLen}, + {"category", p.Category, MaxCategoryLen}, + {"version", p.Version, MaxVersionLen}, + } + for _, c := range checks { + if err := checkLen(c.field, c.value, c.max); err != nil { + return err + } + } + return nil +} diff --git a/internal/product/limits_test.go b/internal/product/limits_test.go new file mode 100644 index 00000000..7bdc4e99 --- /dev/null +++ b/internal/product/limits_test.go @@ -0,0 +1,96 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package product + +import ( + "strings" + "testing" + + "github.com/uberware/sqi/internal/store" +) + +func TestValidateMetadata_Boundaries(t *testing.T) { + t.Parallel() + tests := []struct { + name string + build func(s string) store.Product + max int + field string + }{ + {"title", func(s string) store.Product { return store.Product{Title: s} }, MaxTitleLen, "title"}, + {"description", func(s string) store.Product { return store.Product{Description: s} }, MaxDescriptionLen, "description"}, + {"readme", func(s string) store.Product { return store.Product{Readme: s} }, MaxReadmeLen, "readme"}, + {"category", func(s string) store.Product { return store.Product{Category: s} }, MaxCategoryLen, "category"}, + {"version", func(s string) store.Product { return store.Product{Version: s} }, MaxVersionLen, "version"}, + {"name", func(s string) store.Product { return store.Product{Name: s} }, MaxNameLen, "name"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if err := ValidateMetadata(tt.build(strings.Repeat("a", tt.max))); err != nil { + t.Errorf("at the cap: unexpected error %v", err) + } + err := ValidateMetadata(tt.build(strings.Repeat("a", tt.max+1))) + if err == nil { + t.Fatal("at cap+1: want an error, got nil") + } + if !strings.Contains(err.Error(), tt.field) { + t.Errorf("error %q does not name the field %q", err, tt.field) + } + }) + } +} + +// The caps count runes, not bytes. A CJK description of exactly +// MaxDescriptionLen characters is ~3x that many bytes and must still be +// accepted -- otherwise a Japanese-language description is silently +// third-class. +func TestValidateMetadata_CountsRunesNotBytes(t *testing.T) { + t.Parallel() + desc := strings.Repeat("日", MaxDescriptionLen) + if len(desc) <= MaxDescriptionLen { + t.Fatalf("test is not exercising multi-byte input: %d bytes for %d runes", len(desc), MaxDescriptionLen) + } + if err := ValidateMetadata(store.Product{Description: desc}); err != nil { + t.Errorf("CJK description at the rune cap: unexpected error %v", err) + } + if err := ValidateMetadata(store.Product{Description: desc + "日"}); err == nil { + t.Error("CJK description at cap+1: want an error, got nil") + } +} + +// The message carries the actual length as well as the cap, so an author can +// see how much to cut without counting by hand. +func TestValidateMetadata_ErrorNamesCapAndActual(t *testing.T) { + t.Parallel() + err := ValidateMetadata(store.Product{Description: strings.Repeat("a", MaxDescriptionLen+7)}) + if err == nil { + t.Fatal("want an error, got nil") + } + msg := err.Error() + for _, want := range []string{"description", "507", "500"} { + if !strings.Contains(msg, want) { + t.Errorf("error %q is missing %q", msg, want) + } + } +} + +// The length cap applies to name, but the slug PATTERN stays on the definition +// path only -- see the spec's post-approval finding. A pattern-invalid name is +// therefore not ValidateMetadata's business. +func TestValidateMetadata_IgnoresSlugPattern(t *testing.T) { + t.Parallel() + if err := ValidateMetadata(store.Product{Name: "Not A Slug!!"}); err != nil { + t.Errorf("unexpected error %v", err) + } +} + +func TestValidateName_RejectsOverlongSlug(t *testing.T) { + t.Parallel() + if err := validateName(strings.Repeat("a", MaxNameLen+1)); err == nil { + t.Fatal("want an error for a pattern-valid but over-long slug, got nil") + } + if err := validateName(strings.Repeat("a", MaxNameLen)); err != nil { + t.Errorf("at the cap: unexpected error %v", err) + } +} From 559d7a554096f611632e0cd4cc13f391f3959d8e Mon Sep 17 00:00:00 2001 From: Robin Scher Date: Tue, 18 Aug 2026 13:39:07 -0700 Subject: [PATCH 04/20] fix(presets): move segment-transcode description overflow into readme --- .../sqi/ffmpeg-segment-transcode-bash.yaml | 65 +++++++++++++--- .../sqi/ffmpeg-segment-transcode-expr.yaml | 78 +++++++++++++++---- .../ffmpeg-segment-transcode-powershell.yaml | 66 +++++++++++++--- 3 files changed, 178 insertions(+), 31 deletions(-) diff --git a/presets/sqi/ffmpeg-segment-transcode-bash.yaml b/presets/sqi/ffmpeg-segment-transcode-bash.yaml index a8a41df9..715ce765 100644 --- a/presets/sqi/ffmpeg-segment-transcode-bash.yaml +++ b/presets/sqi/ffmpeg-segment-transcode-bash.yaml @@ -2,15 +2,62 @@ name: ffmpeg-segment-transcode-bash title: FFmpeg Segment Transcode (Bash) description: >- - Splits a video into fixed-length slices, converts each on a different worker, - then joins them back into one file. Enter the source's length in seconds - - nothing can measure it before the job is submitted; a value that is too - small silently drops the tail, and a value that is too large wastes a task - transcoding an empty tail slice. The join step runs a bash script, so this - variant needs Linux or macOS workers; use the Portable variant on Windows or - a mixed farm. Slice files are written beside the output - and removed once the join succeeds. Requires ffmpeg on PATH. A starting - point - duplicate to customize for your pipeline. + Splits a video into fixed-length slices, transcodes each on a different + worker, then joins them with a bash script, so it needs Linux or macOS + workers. +readme: | + # FFmpeg Segment Transcode (Bash) + + Splits one source video into fixed-length slices, transcodes each slice on + a different worker, then joins the finished slices back into a single + output file. The join step is a bash script, so this variant needs Linux + or macOS workers - use the Portable variant on Windows or a mixed farm. + + ## Steps + + 1. **Transcode** - fans out into one task per slice. Each task runs: + `ffmpeg -ss -t -i SourceFile ...` and writes + `_seg_<00001>.` beside `OutputFile`. + The number of tasks is `ceil(DurationSeconds / SegmentSeconds)`. + 2. **Join** - depends on every Transcode task, and runs an embedded bash + script on the worker. The script globs `/_seg_*.` next + to `OutputFile` - zero-padded slice numbers make lexical glob order the + same as numeric order - builds a concat file list from the matches, + then runs `ffmpeg -f concat -safe 0` to copy the streams into + `OutputFile` with no re-encode. On success it deletes the slice files + it just joined. + + ## Parameters + + - **SourceFile** - the video to split. + - **OutputFile** - the joined result. Its parent directory and stem also + name the intermediate slice files the join step globs for. + - **DurationSeconds** - the source's length in seconds. Type it in: nothing + in the job template can measure a source file before the job is + submitted. Getting it wrong has two different failure shapes: + - **Too small** silently drops the tail of the source - the last + slice(s) are never scheduled. + - **Too large** schedules an extra task past the real end of the source. + `ffmpeg` transcodes silence or a frozen frame for that slice, wasting a + task; the join step still globs it up, joins it in, and deletes it + same as every other slice. + - **SegmentSeconds** - the length of each slice, and so the number of + tasks. + - **VideoCodec**, **Quality** (CRF, lower is better), **AudioCodec** - + passed straight to `ffmpeg` for every slice. + + ## Cleanup + + The join script removes every slice file it globbed once the concat + succeeds. If the join fails, the slice files are left on disk for you to + inspect or remove by hand. + + ## Requirements + + `ffmpeg` and `bash` must be on `PATH` for every worker that runs this job, + and workers must be tagged `attr.worker.tag.ffmpeg = true` with + `attr.worker.os.family` in `linux` or `macos`. This is a starting point - + duplicate it to customize for your own pipeline. category: Transcoding version: 1.0.0 template: diff --git a/presets/sqi/ffmpeg-segment-transcode-expr.yaml b/presets/sqi/ffmpeg-segment-transcode-expr.yaml index 088ff06b..a2a62ffa 100644 --- a/presets/sqi/ffmpeg-segment-transcode-expr.yaml +++ b/presets/sqi/ffmpeg-segment-transcode-expr.yaml @@ -2,19 +2,71 @@ name: ffmpeg-segment-transcode-expr title: FFmpeg Segment Transcode (Portable) description: >- - Splits a video into fixed-length slices, converts each on a different worker, - then joins them back into one file. Enter the source's length in seconds - - nothing can measure it before the job is submitted; a value that is too - small silently drops the tail, and a value that is too large wastes a task - and leaves an extra empty slice file beside the output for you to clean up, - same as the rest. Needs no shell at all: the join step's file list is - written by the template itself, so this variant runs on Linux, macOS - and Windows workers alike. The cost of writing that list is charged at - submission and grows with the slice count, so it suits jobs of up to 400 - slices - a longer source needs a longer slice to stay under that. Past 400, - use the Bash or PowerShell variant, whose cost does not grow. Slice files are - left beside the output for you to remove. Requires ffmpeg on PATH. A starting - point - duplicate to customize for your pipeline. + Splits a video into fixed-length slices, transcodes each on a different + worker, then joins them with a template-generated file list so it runs on + Linux, macOS and Windows workers alike. +readme: | + # FFmpeg Segment Transcode (Portable) + + Splits one source video into fixed-length slices, transcodes each slice on a + different worker, then joins the finished slices back into a single output + file. This variant needs no shell on the worker: the file list the join step + reads is generated by the job template itself using OpenJD's `EXPR` + extension, so the same template runs unchanged on Linux, macOS and Windows + workers. + + ## Steps + + 1. **Transcode** - fans out into one task per slice. Each task runs: + `ffmpeg -ss -t -i SourceFile ...` and writes + `_seg_<00001>.` beside `OutputFile`. + The number of tasks is `ceil(DurationSeconds / SegmentSeconds)`. + 2. **Join** - depends on every Transcode task. An embedded file lists every + slice path as `file ''`, one per line, built with an `EXPR` + `join()` expression over a `range()` comprehension - no script, no loop + on the worker. `ffmpeg -f concat -safe 0` reads that list and copies the + streams into `OutputFile` with no re-encode. + + ## Parameters + + - **SourceFile** - the video to split. + - **OutputFile** - the joined result. Its parent directory and stem also + name the intermediate slice files. + - **DurationSeconds** - the source's length in seconds. Type it in: nothing + in the job template can measure a source file before the job is + submitted. Getting it wrong has two different failure shapes: + - **Too small** silently drops the tail of the source - the last + slice(s) are never scheduled. + - **Too large** schedules an extra task past the real end of the source. + `ffmpeg` transcodes silence or a frozen frame for that slice, wasting a + task, and its slice file is still concatenated into the join, then + left on disk afterward (see Cleanup). + - **SegmentSeconds** - the length of each slice, and so the number of + tasks. + - **VideoCodec**, **Quality** (CRF, lower is better), **AudioCodec** - + passed straight to `ffmpeg` for every slice. + + ## Scaling + + Building the join step's file list is charged against this template's + `EXPR` evaluation budget at submission time, and that cost grows with the + number of slices. It comfortably fits jobs of up to about 400 slices; a + longer source needs a longer `SegmentSeconds` to stay under that, or use + the Bash or PowerShell variant instead, whose join step costs the same + regardless of slice count (it lists slices with a directory glob at run + time, not at submission time). + + ## Cleanup + + Unlike the Bash and PowerShell variants, this template does not remove the + per-slice files after a successful join - there is no shell step to do it + in. They are left beside `OutputFile` for you to delete. + + ## Requirements + + `ffmpeg` must be on `PATH` for every worker that runs this job, and workers + must be tagged `attr.worker.tag.ffmpeg = true`. This is a starting point - + duplicate it to customize for your own pipeline. category: Transcoding version: 1.0.0 template: diff --git a/presets/sqi/ffmpeg-segment-transcode-powershell.yaml b/presets/sqi/ffmpeg-segment-transcode-powershell.yaml index bfc5bd6a..12cfd280 100644 --- a/presets/sqi/ffmpeg-segment-transcode-powershell.yaml +++ b/presets/sqi/ffmpeg-segment-transcode-powershell.yaml @@ -2,15 +2,63 @@ name: ffmpeg-segment-transcode-powershell title: FFmpeg Segment Transcode (PowerShell) description: >- - Splits a video into fixed-length slices, converts each on a different worker, - then joins them back into one file. Enter the source's length in seconds - - nothing can measure it before the job is submitted; a value that is too - small silently drops the tail, and a value that is too large wastes a task - transcoding an empty tail slice. The join step runs a PowerShell script, so - this variant needs Windows workers; use the Portable variant on a mixed farm. - Slice files are written beside the output and removed once the join succeeds. - Requires ffmpeg on PATH. A starting point - duplicate to customize for your - pipeline. + Splits a video into fixed-length slices, transcodes each on a different + worker, then joins them with a PowerShell script, so it needs Windows + workers. +readme: | + # FFmpeg Segment Transcode (PowerShell) + + Splits one source video into fixed-length slices, transcodes each slice on + a different worker, then joins the finished slices back into a single + output file. The join step is a PowerShell script, so this variant needs + Windows workers - use the Portable variant on a mixed farm. + + ## Steps + + 1. **Transcode** - fans out into one task per slice. Each task runs: + `ffmpeg -ss -t -i SourceFile ...` and writes + `_seg_<00001>.` beside `OutputFile`. + The number of tasks is `ceil(DurationSeconds / SegmentSeconds)`. + 2. **Join** - depends on every Transcode task, and runs an embedded + PowerShell script on the worker. The script lists + `\_seg_*` next to `OutputFile` with `Get-ChildItem`, + sorted by name - zero-padded slice numbers make that the same as + numeric order - writes the matches to a BOM-less UTF-8 concat file list + (a BOM breaks the concat demuxer's parse of the first line), then runs + `ffmpeg -f concat -safe 0` to copy the streams into `OutputFile` with no + re-encode. On success it deletes the slice files it just joined. + + ## Parameters + + - **SourceFile** - the video to split. + - **OutputFile** - the joined result. Its parent directory and stem also + name the intermediate slice files the join step lists. + - **DurationSeconds** - the source's length in seconds. Type it in: nothing + in the job template can measure a source file before the job is + submitted. Getting it wrong has two different failure shapes: + - **Too small** silently drops the tail of the source - the last + slice(s) are never scheduled. + - **Too large** schedules an extra task past the real end of the source. + `ffmpeg` transcodes silence or a frozen frame for that slice, wasting a + task; the join step still lists it, joins it in, and deletes it same + as every other slice. + - **SegmentSeconds** - the length of each slice, and so the number of + tasks. + - **VideoCodec**, **Quality** (CRF, lower is better), **AudioCodec** - + passed straight to `ffmpeg` for every slice. + + ## Cleanup + + The join script removes every slice file it listed once the concat + succeeds. If the join fails, the slice files are left on disk for you to + inspect or remove by hand. + + ## Requirements + + `ffmpeg` and `powershell` must be on `PATH` for every worker that runs this + job, and workers must be tagged `attr.worker.tag.ffmpeg = true` with + `attr.worker.os.family` set to `windows`. This is a starting point - + duplicate it to customize for your own pipeline. category: Transcoding version: 1.0.0 template: From 3337d6eada656ec40a53fc2bd077ef8b925b529f Mon Sep 17 00:00:00 2001 From: Robin Scher Date: Tue, 18 Aug 2026 13:51:02 -0700 Subject: [PATCH 05/20] fix(presets): flatten nested lists in segment-transcode readmes --- presets/sqi/ffmpeg-segment-transcode-bash.yaml | 12 +++++------- presets/sqi/ffmpeg-segment-transcode-expr.yaml | 12 +++++------- presets/sqi/ffmpeg-segment-transcode-powershell.yaml | 12 +++++------- 3 files changed, 15 insertions(+), 21 deletions(-) diff --git a/presets/sqi/ffmpeg-segment-transcode-bash.yaml b/presets/sqi/ffmpeg-segment-transcode-bash.yaml index 715ce765..2bda3173 100644 --- a/presets/sqi/ffmpeg-segment-transcode-bash.yaml +++ b/presets/sqi/ffmpeg-segment-transcode-bash.yaml @@ -34,13 +34,11 @@ readme: | name the intermediate slice files the join step globs for. - **DurationSeconds** - the source's length in seconds. Type it in: nothing in the job template can measure a source file before the job is - submitted. Getting it wrong has two different failure shapes: - - **Too small** silently drops the tail of the source - the last - slice(s) are never scheduled. - - **Too large** schedules an extra task past the real end of the source. - `ffmpeg` transcodes silence or a frozen frame for that slice, wasting a - task; the join step still globs it up, joins it in, and deletes it - same as every other slice. + submitted. Too small silently drops the tail of the source - the last + slice(s) are never scheduled. Too large schedules an extra task past the + real end of the source: `ffmpeg` transcodes silence or a frozen frame for + that slice, wasting a task, but the join step still globs it up, joins + it in, and deletes it same as every other slice. - **SegmentSeconds** - the length of each slice, and so the number of tasks. - **VideoCodec**, **Quality** (CRF, lower is better), **AudioCodec** - diff --git a/presets/sqi/ffmpeg-segment-transcode-expr.yaml b/presets/sqi/ffmpeg-segment-transcode-expr.yaml index a2a62ffa..e1d40352 100644 --- a/presets/sqi/ffmpeg-segment-transcode-expr.yaml +++ b/presets/sqi/ffmpeg-segment-transcode-expr.yaml @@ -34,13 +34,11 @@ readme: | name the intermediate slice files. - **DurationSeconds** - the source's length in seconds. Type it in: nothing in the job template can measure a source file before the job is - submitted. Getting it wrong has two different failure shapes: - - **Too small** silently drops the tail of the source - the last - slice(s) are never scheduled. - - **Too large** schedules an extra task past the real end of the source. - `ffmpeg` transcodes silence or a frozen frame for that slice, wasting a - task, and its slice file is still concatenated into the join, then - left on disk afterward (see Cleanup). + submitted. Too small silently drops the tail of the source - the last + slice(s) are never scheduled. Too large schedules an extra task past the + real end of the source: `ffmpeg` transcodes silence or a frozen frame for + that slice, wasting a task, and its slice file is still concatenated + into the join, then left on disk afterward (see Cleanup). - **SegmentSeconds** - the length of each slice, and so the number of tasks. - **VideoCodec**, **Quality** (CRF, lower is better), **AudioCodec** - diff --git a/presets/sqi/ffmpeg-segment-transcode-powershell.yaml b/presets/sqi/ffmpeg-segment-transcode-powershell.yaml index 12cfd280..da37f30f 100644 --- a/presets/sqi/ffmpeg-segment-transcode-powershell.yaml +++ b/presets/sqi/ffmpeg-segment-transcode-powershell.yaml @@ -35,13 +35,11 @@ readme: | name the intermediate slice files the join step lists. - **DurationSeconds** - the source's length in seconds. Type it in: nothing in the job template can measure a source file before the job is - submitted. Getting it wrong has two different failure shapes: - - **Too small** silently drops the tail of the source - the last - slice(s) are never scheduled. - - **Too large** schedules an extra task past the real end of the source. - `ffmpeg` transcodes silence or a frozen frame for that slice, wasting a - task; the join step still lists it, joins it in, and deletes it same - as every other slice. + submitted. Too small silently drops the tail of the source - the last + slice(s) are never scheduled. Too large schedules an extra task past the + real end of the source: `ffmpeg` transcodes silence or a frozen frame for + that slice, wasting a task, but the join step still lists it, joins it + in, and deletes it same as every other slice. - **SegmentSeconds** - the length of each slice, and so the number of tasks. - **VideoCodec**, **Quality** (CRF, lower is better), **AudioCodec** - From 5765c12e36587c20d17f449d6713f11e1e5bd2ba Mon Sep 17 00:00:00 2001 From: Robin Scher Date: Tue, 18 Aug 2026 13:51:15 -0700 Subject: [PATCH 06/20] feat(api): expose product and preset readme over REST with length caps --- internal/api/openapi.yaml | 51 ++++++++++++++++++++ internal/api/presets.go | 2 + internal/api/products.go | 36 ++++++++++---- internal/api/products_test.go | 70 ++++++++++++++++++++++++++++ internal/presetlib/presetlib_test.go | 17 +++++++ 5 files changed, 168 insertions(+), 8 deletions(-) diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 45e6e614..1e8afac7 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -1280,6 +1280,12 @@ components: - $ref: "#/components/schemas/Preset" - type: object properties: + readme: + type: string + description: >- + Long-form documentation in a restricted Markdown subset + (paragraphs, lists, fenced code, ATX headings, bold, italic, + inline code, links). Rendered on the preset detail page only. template: type: string format: @@ -1297,6 +1303,21 @@ components: type: string description: type: string + maxLength: 500 + description: >- + Short plain-text catalog blurb, shown in cards and in DCC + submitter dropdowns. Plain text, not Markdown -- it reaches + consumers that cannot render markup, such as the Blender addon's + tooltip. + readme: + type: string + maxLength: 8000 + description: >- + Long-form documentation in a restricted Markdown subset + (paragraphs, lists, fenced code, ATX headings, bold, italic, + inline code, links). Rendered on detail pages only. Unlike + `description` it is NOT searched, and it is not carried in the + preset library index. category: type: string version: @@ -1324,6 +1345,21 @@ components: type: string description: type: string + maxLength: 500 + description: >- + Short plain-text catalog blurb, shown in cards and in DCC + submitter dropdowns. Plain text, not Markdown -- it reaches + consumers that cannot render markup, such as the Blender addon's + tooltip. + readme: + type: string + maxLength: 8000 + description: >- + Long-form documentation in a restricted Markdown subset + (paragraphs, lists, fenced code, ATX headings, bold, italic, + inline code, links). Rendered on detail pages only. Unlike + `description` it is NOT searched, and it is not carried in the + preset library index. category: type: string version: @@ -1344,6 +1380,21 @@ components: type: string description: type: string + maxLength: 500 + description: >- + Short plain-text catalog blurb, shown in cards and in DCC + submitter dropdowns. Plain text, not Markdown -- it reaches + consumers that cannot render markup, such as the Blender addon's + tooltip. + readme: + type: string + maxLength: 8000 + description: >- + Long-form documentation in a restricted Markdown subset + (paragraphs, lists, fenced code, ATX headings, bold, italic, + inline code, links). Rendered on detail pages only. Unlike + `description` it is NOT searched, and it is not carried in the + preset library index. category: type: string version: diff --git a/internal/api/presets.go b/internal/api/presets.go index b0ac4341..bf8fc97c 100644 --- a/internal/api/presets.go +++ b/internal/api/presets.go @@ -109,6 +109,7 @@ type presetResponse struct { type presetDetailResponse struct { presetResponse + Readme string `json:"readme"` Template string `json:"template"` Format string `json:"format"` } @@ -217,6 +218,7 @@ func (h *presetHandler) getPreset(w http.ResponseWriter, r *http.Request) { Name: entry.Name, Title: def.Title, Description: def.Description, Category: def.Category, Version: def.Version, Status: installStatus(entry, byRef), }, + Readme: def.Readme, Template: def.Template, Format: string(def.Format), }) diff --git a/internal/api/products.go b/internal/api/products.go index 02bf3d57..414193c1 100644 --- a/internal/api/products.go +++ b/internal/api/products.go @@ -131,6 +131,7 @@ type productResponse struct { Name string `json:"name"` Title string `json:"title"` Description string `json:"description"` + Readme string `json:"readme"` Category string `json:"category"` Version string `json:"version"` Source string `json:"source"` @@ -142,6 +143,7 @@ type createProductRequest struct { Name string `json:"name"` Title string `json:"title"` Description string `json:"description"` + Readme string `json:"readme"` Category string `json:"category"` Version string `json:"version"` Template string `json:"template"` @@ -150,7 +152,7 @@ type createProductRequest struct { func toProductResponse(p store.Product) productResponse { return productResponse{ - Name: p.Name, Title: p.Title, Description: p.Description, + Name: p.Name, Title: p.Title, Description: p.Description, Readme: p.Readme, Category: p.Category, Version: p.Version, Source: string(p.Source), Template: p.Template, Format: string(p.Format), } @@ -494,13 +496,31 @@ func (h *productHandler) decodeProductBody(w http.ResponseWriter, r *http.Reques writeProblem(w, r, http.StatusBadRequest, "template is required") return store.Product{}, false } - if err := product.ValidateTemplate(req.Template, format, h.templateValidateOptions()); err != nil { - h.writeTemplateProblem(w, r, err) - return store.Product{}, false - } - return store.Product{ - Name: name, Title: req.Title, Description: req.Description, + p := store.Product{ + Name: name, Title: req.Title, Description: req.Description, Readme: req.Readme, Category: req.Category, Version: req.Version, Template: req.Template, Format: format, - }, true + } + if !h.validateProductBody(w, r, p, format) { + return store.Product{}, false + } + return p, true +} + +// validateProductBody enforces the metadata length caps (checked first, since +// it is cheap) and then the OpenJD template itself (the expensive check) on a +// product built from a decoded request body. It writes the problem response +// and returns false on either failure. +func (h *productHandler) validateProductBody( + w http.ResponseWriter, r *http.Request, p store.Product, format store.TemplateFormat, +) bool { + if err := product.ValidateMetadata(p); err != nil { + writeProblem(w, r, http.StatusBadRequest, err.Error()) + return false + } + if err := product.ValidateTemplate(p.Template, format, h.templateValidateOptions()); err != nil { + h.writeTemplateProblem(w, r, err) + return false + } + return true } diff --git a/internal/api/products_test.go b/internal/api/products_test.go index 0e1c71d7..875fadf8 100644 --- a/internal/api/products_test.go +++ b/internal/api/products_test.go @@ -577,6 +577,76 @@ func TestProducts_SubmitWithDependsOn_MissingUpstreamIs422(t *testing.T) { // TestProducts_SubmitRejectsInvalidRetryOverrides asserts the product submit // endpoint applies the same retry-override bounds as direct job submission. +// TestCreateProduct_ReadmeRoundTrips verifies readme is accepted on create, +// distinct from description, and comes back on both the create response and +// a subsequent GET. +func TestCreateProduct_ReadmeRoundTrips(t *testing.T) { + srv := newProductRouter(fake.New()) + req := newReq(t, http.MethodPost, "/api/v1/products", jsonBody(t, map[string]any{ + "name": "readme-probe", "title": "Readme Probe", "description": "blurb", + "readme": "# Docs\n\nBody.\n", + "template": validTemplate, "format": "yaml", + })) + rec := httptest.NewRecorder() + srv.ServeHTTP(rec, req) + if rec.Code != http.StatusCreated { + t.Fatalf("status = %d, want 201: %s", rec.Code, rec.Body.String()) + } + var got struct { + Readme string `json:"readme"` + Description string `json:"description"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil { + t.Fatalf("decode: %v", err) + } + if got.Readme != "# Docs\n\nBody.\n" { + t.Errorf("readme = %q, want the markdown body", got.Readme) + } + if got.Description != "blurb" { + t.Errorf("description = %q, want %q", got.Description, "blurb") + } + + rec = httptest.NewRecorder() + srv.ServeHTTP(rec, newReq(t, http.MethodGet, "/api/v1/products/readme-probe", nil)) + if rec.Code != http.StatusOK { + t.Fatalf("get status = %d, want 200", rec.Code) + } + if !strings.Contains(rec.Body.String(), `"readme"`) { + t.Error("GET response has no readme field") + } +} + +// TestCreateProduct_RejectsOverlongMetadata verifies decodeProductBody enforces +// product.ValidateMetadata's length caps with a 400, naming the offending field. +func TestCreateProduct_RejectsOverlongMetadata(t *testing.T) { + tests := []struct { + field string + size int + }{ + {"description", product.MaxDescriptionLen + 1}, + {"readme", product.MaxReadmeLen + 1}, + {"title", product.MaxTitleLen + 1}, + } + for _, tt := range tests { + t.Run(tt.field, func(t *testing.T) { + srv := newProductRouter(fake.New()) + payload := map[string]string{ + "name": "cap-probe", "title": "Cap Probe", + "template": validTemplate, "format": "yaml", + } + payload[tt.field] = strings.Repeat("a", tt.size) + rec := httptest.NewRecorder() + srv.ServeHTTP(rec, newReq(t, http.MethodPost, "/api/v1/products", jsonBody(t, payload))) + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400: %s", rec.Code, rec.Body.String()) + } + if !strings.Contains(rec.Body.String(), tt.field) { + t.Errorf("problem body %q does not name the field %q", rec.Body.String(), tt.field) + } + }) + } +} + func TestProducts_SubmitRejectsInvalidRetryOverrides(t *testing.T) { srv := newProductTestServer(t) farmID, queueID := seedProductSubmitPrereqs(t, srv) diff --git a/internal/presetlib/presetlib_test.go b/internal/presetlib/presetlib_test.go index 9874b169..868f6eda 100644 --- a/internal/presetlib/presetlib_test.go +++ b/internal/presetlib/presetlib_test.go @@ -9,6 +9,7 @@ import ( "errors" "net/http" "net/http/httptest" + "reflect" "sync/atomic" "testing" "time" @@ -163,3 +164,19 @@ func TestFetchIndex_StaleCache_OnFailedRefresh(t *testing.T) { t.Fatalf("stale cache entry mismatch: want %q, got %q", entries[0].Name, got[0].Name) } } + +// IndexEntry deliberately carries NO readme. The readme reaches PresetDetail +// from the fetched DEFINITION, not from the index -- presetDetailResponse is +// built from def, and only entry.Name and the install status come from the +// index. Description is in the index solely because the preset LIST page +// searches it; readme is not searched, so putting it in the index would grow +// every client's cached index for nothing and change the remote index format. +// +// This test exists so that adding it later is a deliberate decision rather than +// drift while implementing "make readme searchable". +func TestIndexEntry_HasNoReadmeField(t *testing.T) { + t.Parallel() + if _, ok := reflect.TypeFor[presetlib.IndexEntry]().FieldByName("Readme"); ok { + t.Fatal("IndexEntry gained a Readme field; read this test's comment before removing it") + } +} From ac856373568fd31e030fdb76a5894bc828302660 Mon Sep 17 00:00:00 2001 From: Robin Scher Date: Tue, 18 Aug 2026 13:57:38 -0700 Subject: [PATCH 07/20] docs(api): align PresetDetail readme cap and wording with Product schemas --- internal/api/openapi.yaml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 1e8afac7..4f81edc6 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -1282,10 +1282,13 @@ components: properties: readme: type: string + maxLength: 8000 description: >- Long-form documentation in a restricted Markdown subset (paragraphs, lists, fenced code, ATX headings, bold, italic, - inline code, links). Rendered on the preset detail page only. + inline code, links). Rendered on detail pages only. Unlike + `description` it is NOT searched, and it is not carried in the + preset library index. template: type: string format: From 376f5f67b94338e3b0dfa1f76b78a1dea2fb5226 Mon Sep 17 00:00:00 2001 From: Robin Scher Date: Tue, 18 Aug 2026 13:59:16 -0700 Subject: [PATCH 08/20] feat(web): add hand-written Markdown renderer for product readmes --- web/src/components/Markdown.module.css | 26 ++++ web/src/components/Markdown.test.tsx | 109 ++++++++++++++++ web/src/components/Markdown.tsx | 166 +++++++++++++++++++++++++ 3 files changed, 301 insertions(+) create mode 100644 web/src/components/Markdown.module.css create mode 100644 web/src/components/Markdown.test.tsx create mode 100644 web/src/components/Markdown.tsx diff --git a/web/src/components/Markdown.module.css b/web/src/components/Markdown.module.css new file mode 100644 index 00000000..ee3c7300 --- /dev/null +++ b/web/src/components/Markdown.module.css @@ -0,0 +1,26 @@ +/* SPDX-License-Identifier: AGPL-3.0-or-later */ +.markdown { + color: var(--color-text-secondary); + font-size: var(--font-size-sm); +} + +.markdown p, +.markdown ul, +.markdown ol, +.markdown pre { + margin: 0 0 var(--space-3) 0; +} + +.markdown h3, +.markdown h4, +.markdown h5, +.markdown h6 { + color: var(--color-text-primary); + margin: var(--space-4) 0 var(--space-2) 0; +} + +.markdown pre { + overflow-x: auto; + padding: var(--space-3); + border-radius: var(--radius-sm); +} diff --git a/web/src/components/Markdown.test.tsx b/web/src/components/Markdown.test.tsx new file mode 100644 index 00000000..b111f3a1 --- /dev/null +++ b/web/src/components/Markdown.test.tsx @@ -0,0 +1,109 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +import { render, screen } from '@testing-library/react' +import { describe, expect, it } from 'vitest' +import Markdown from './Markdown' + +describe('Markdown', () => { + it('renders paragraphs split on blank lines', () => { + render() + expect(screen.getByText('First para.')).toBeInTheDocument() + expect(screen.getByText('Second para.')).toBeInTheDocument() + }) + + it('renders inline bold, italic and code', () => { + render() + expect(screen.getByText('bold').tagName).toBe('STRONG') + expect(screen.getByText('italic').tagName).toBe('EM') + expect(screen.getByText('code').tagName).toBe('CODE') + }) + + it('renders unordered lists', () => { + render() + expect(screen.getAllByRole('listitem')).toHaveLength(2) + }) + + it('renders ordered lists', () => { + const { container } = render() + expect(container.querySelector('ol')).not.toBeNull() + expect(screen.getAllByRole('listitem')).toHaveLength(2) + }) + + it('renders fenced code blocks verbatim, without inline parsing', () => { + render() + expect(screen.getByText(/not \*\*bold\*\* here/)).toBeInTheDocument() + expect(screen.queryByText('bold')).not.toBeInTheDocument() + }) + + // Both detail pages nest h1 (PageHeader) -> h2 (product/preset title) -> + // readme, so readme headings start at h3 to keep the document outline valid. + it('offsets headings so the outline stays correct', () => { + render() + expect(screen.getByText('One').tagName).toBe('H3') + expect(screen.getByText('Two').tagName).toBe('H4') + expect(screen.getByText('Three').tagName).toBe('H5') + expect(screen.getByText('Four').tagName).toBe('H6') + expect(screen.getByText('Five').tagName).toBe('H6') + }) + + it('renders http and mailto links', () => { + render() + expect(screen.getByRole('link', { name: 'docs' })).toHaveAttribute( + 'href', + 'https://example.com/x', + ) + expect(screen.getByRole('link', { name: 'mail' })).toHaveAttribute('href', 'mailto:a@b.c') + }) + + it('gives external links rel="noopener noreferrer"', () => { + render() + expect(screen.getByRole('link', { name: 'docs' })).toHaveAttribute('rel', 'noopener noreferrer') + }) + + it('degrades unsupported syntax to literal text', () => { + render() + expect(screen.getByText(/\| a \| b \|/)).toBeInTheDocument() + expect(screen.queryByRole('table')).not.toBeInTheDocument() + }) + + describe('security', () => { + it.each([ + ['javascript:alert(1)'], + ['JaVaScRiPt:alert(1)'], + [' javascript:alert(1)'], + ['java\tscript:alert(1)'], + ['data:text/html,'], + ['vbscript:msgbox(1)'], + ])('renders %s as text, not a link', (href) => { + render() + expect(screen.queryByRole('link')).not.toBeInTheDocument() + expect(screen.getByText(/click/)).toBeInTheDocument() + }) + + it('renders raw HTML as visible text', () => { + const { container } = render( + alert(1) and '} />, + ) + expect(container.querySelector('script')).toBeNull() + expect(container.querySelector('img')).toBeNull() + expect(screen.getByText(/ and '} />, diff --git a/web/src/components/Markdown.tsx b/web/src/components/Markdown.tsx index eed88ade..19015a55 100644 --- a/web/src/components/Markdown.tsx +++ b/web/src/components/Markdown.tsx @@ -7,19 +7,25 @@ import styles from './Markdown.module.css' * readmes come from a remote index, so this allowlist is the only control. */ const SAFE_SCHEMES = ['http:', 'https:', 'mailto:'] -/** True when href's scheme is allowlisted. Whitespace and control characters - * are stripped before matching: `java\tscript:` is a spelling browsers have +/** Returns href with its control and whitespace characters stripped when the + * scheme is allowlisted, or null when it is not. Lowercasing is applied only + * to the extracted scheme for the allowlist comparison, never to the + * returned string -- the caller renders this return value directly, so the + * string that is checked and the string that reaches the DOM are the same + * value, not merely argued-equivalent. Whitespace and control characters are + * stripped before matching: `java\tscript:` is a spelling browsers have * historically accepted, and a naive prefix test would miss it. */ -function isSafeHref(href: string): boolean { +function safeHref(href: string): string | null { // Strips every character at or below U+0020 -- space, tab, newline and // the C0 controls. A class such as /[ -]/ would strip spaces and hyphens // but leave the tab in `java\tscript:` intact, which is the case the // security test covers. // eslint-disable-next-line no-control-regex -- intentional: strip C0 controls, see comment above - const cleaned = href.replace(/[\u0000-\u0020]/g, '').toLowerCase() - const colon = cleaned.indexOf(':') - if (colon === -1) return false - return SAFE_SCHEMES.includes(cleaned.slice(0, colon + 1)) + const stripped = href.replace(/[\u0000-\u0020]/g, '') + const colon = stripped.indexOf(':') + if (colon === -1) return null + const scheme = stripped.slice(0, colon + 1).toLowerCase() + return SAFE_SCHEMES.includes(scheme) ? stripped : null } const INLINE = /(\*\*[^*]+\*\*|\*[^*]+\*|_[^_]+_|`[^`]+`|\[[^\]]*\]\([^)\s]*\))/g @@ -48,11 +54,10 @@ function inline(text: string, keyPrefix: string): ReactNode[] { // emitted as text, so emitting the token as text too degrades the whole // construct to literal characters rather than to a link labelled `alt`. const isImage = match.index > 0 && text[match.index - 1] === '!' - if (isImage) { - out.push(token) - } else if (isSafeHref(href)) { + const safe = isImage ? null : safeHref(href) + if (safe !== null) { out.push( - + {label} , ) From c67a028a5bc06bd4e2a7f8a301c12cc57eb5bb5b Mon Sep 17 00:00:00 2001 From: Robin Scher Date: Tue, 18 Aug 2026 14:17:19 -0700 Subject: [PATCH 12/20] docs(products): document the readme field, its markdown subset and single-level-list limit --- docs/preset-library.md | 5 ++++ docs/products.md | 51 ++++++++++++++++++++++++++++++++++++++- docs/web-accessibility.md | 14 +++++++++++ 3 files changed, 69 insertions(+), 1 deletion(-) diff --git a/docs/preset-library.md b/docs/preset-library.md index 8c3c2d17..73e115dc 100644 --- a/docs/preset-library.md +++ b/docs/preset-library.md @@ -53,6 +53,11 @@ promised) and **update detection** (if the hash in the index changes, the instal product is shown as having an update available). It is not a cryptographic authorship signature — the trust boundary is the configured index URL itself. +The index carries `description` but **not** `readme`. `description` is there +because the preset list page searches it; `readme` is not searched, so shipping +it in the index would grow every client's cached index for nothing. A preset's +readme arrives with its definition when the detail page is opened. + --- ## Configuration diff --git a/docs/products.md b/docs/products.md index cda0cd91..efcb4c7c 100644 --- a/docs/products.md +++ b/docs/products.md @@ -86,7 +86,8 @@ template: |---|---|---| | `name` | yes | Stable slug identity. Lowercase letters, digits, `_` and `-`, with at most one `/` namespace separator (e.g. `studio/maya-render`). | | `title` | yes | Human-readable display name. | -| `description` | no | Short summary shown in the catalog. | +| `description` | no | Short plain-text catalog blurb, max 500 runes (a Unicode character count, not a byte count). Shown in cards and in DCC submitter dropdowns, and matched by product search. **Plain text, not Markdown** — it reaches consumers that cannot render markup, such as the Blender addon's tooltip. | +| `readme` | no | Long-form Markdown documentation, max 8000 runes. Rendered on the product's detail page in the web UI. **Not searched**, and not carried in the preset library index. | | `category` | no | Free-form group label (e.g. `General`, `Rendering`). | | `version` | no | Semver string used for future update-detection. | | `template` | yes | Inline OpenJD job template (`specificationVersion: jobtemplate-2023-09`). | @@ -96,6 +97,54 @@ The inline template is re-serialized and fully validated (via `openjd.Parse` + `openjd.ValidateWithOptions`) when the definition is parsed — a malformed template is rejected at load time. +### Writing a `readme` + +`readme` must use a YAML **literal** block (`|`), not the folded block (`>-`) +that `description` uses. A folded scalar collapses newlines, which silently +destroys paragraph breaks and list structure — the result is one run-on +paragraph, with no error to tell you: + +```yaml +description: >- + Splits a video into segments and transcodes them in parallel. +readme: | + # FFmpeg Segment Transcode + + Splits the source into fixed-length segments, transcodes each on its own + worker, then concatenates the results. + + ## When to use it + + - Long sources where a single-worker transcode would take hours. + - Codecs that tolerate segment boundaries. + + Set `SegmentSeconds` to trade parallelism against concat overhead. +``` + +`readme` is inline content, not a path — `readme: ./README.md` renders the +literal text `./README.md`. + +`description` is plain text and is what product search matches. `readme` is +Markdown, rendered only on the detail page, and is **not** searched. + +**Supported Markdown:** paragraphs, unordered and ordered lists, fenced code +blocks, ATX headings (`#` renders as `

`, nested under the page's existing +heading structure, clamped at `

`), `**bold**`, `*italic*`, `` `code` `` +and `[links](https://example.com)` using `http:`, `https:` or `mailto:` only. + +**Lists are single-level only — nesting is not supported.** The renderer +(`web/src/components/Markdown.tsx`) matches list items with a regular +expression anchored at column 0. An item indented under another list item +does not become a nested list; it silently renders as an ordinary paragraph +with a stray leading `-` or `1.`, with no error or warning. This has already +caught one author on this branch — write every list flat, with no indented +sub-items. + +**Not supported,** and rendered as literal text: images, tables, blockquotes, +reference links, raw HTML, and nesting of any kind. Images are excluded +deliberately — a remote image in a preset readme is an IP beacon firing for +every viewer. + ### `userInterface` parameter hints The `userInterface` block on each parameter is base-spec OpenJD, not a product diff --git a/docs/web-accessibility.md b/docs/web-accessibility.md index 76d6c09e..4c908441 100644 --- a/docs/web-accessibility.md +++ b/docs/web-accessibility.md @@ -95,3 +95,17 @@ These are not regressions to file against; they are deliberately deferred: See [`web-development.md`](web-development.md) for the development workflow and the `CONTRIBUTING.md` "Web UI contributions" section for the component and testing conventions these checks fit into. + +--- + +## Headings in rendered Markdown + +Product and preset readmes render through `src/components/Markdown.tsx`, which +offsets heading levels rather than emitting them verbatim: `#` becomes `

`, +`##` becomes `

`, and so on, clamped at `

`. Both detail pages already +nest `

` (the product or preset title) under `

` (PageHeader), so an +un-offset `#` would put a second `

` mid-document and break the outline. + +Real headings are used rather than visually-styled paragraphs: a bold paragraph +looks like a heading to sighted users and is invisible as structure to a screen +reader. From e1405519a3347c1984ee70f5ad5c1f3ad15eec99 Mon Sep 17 00:00:00 2001 From: Robin Scher Date: Tue, 18 Aug 2026 14:33:04 -0700 Subject: [PATCH 13/20] fix(web): apply CommonMark lazy continuation to the readme list parser --- web/src/components/Markdown.test.tsx | 29 ++++++++++++++++++++++++++++ web/src/components/Markdown.tsx | 15 ++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/web/src/components/Markdown.test.tsx b/web/src/components/Markdown.test.tsx index f4ffa846..0a74db0a 100644 --- a/web/src/components/Markdown.test.tsx +++ b/web/src/components/Markdown.test.tsx @@ -28,6 +28,35 @@ describe('Markdown', () => { expect(screen.getAllByRole('listitem')).toHaveLength(2) }) + // CommonMark lazy continuation: a plain text line right after a list item, + // with no blank line between them, belongs to that item -- not a new + // paragraph. This is the shape every shipped preset readme uses (bullets + // wrapped across source lines). + it('folds a wrapped bullet continuation into the same list item', () => { + render() + const items = screen.getAllByRole('listitem') + expect(items).toHaveLength(1) + expect(items[0]).toHaveTextContent('one continued text') + }) + + it('keeps a wrapped ordered list as one
    with correctly numbered items', () => { + const { container } = render() + const lists = container.querySelectorAll('ol') + expect(lists).toHaveLength(1) + const items = screen.getAllByRole('listitem') + expect(items).toHaveLength(2) + expect(items[0]).toHaveTextContent('first continued') + expect(items[1]).toHaveTextContent('second') + }) + + it('still ends a list on a blank line, not folding the following text', () => { + render() + const items = screen.getAllByRole('listitem') + expect(items).toHaveLength(1) + expect(items[0]).toHaveTextContent('a') + expect(screen.getByText('plain').tagName).toBe('P') + }) + it('renders fenced code blocks verbatim, without inline parsing', () => { render() expect(screen.getByText(/not \*\*bold\*\* here/)).toBeInTheDocument() diff --git a/web/src/components/Markdown.tsx b/web/src/components/Markdown.tsx index 19015a55..50b7ecc6 100644 --- a/web/src/components/Markdown.tsx +++ b/web/src/components/Markdown.tsx @@ -161,6 +161,21 @@ export default function Markdown({ source }: { source: string }): ReactElement { flushList() continue } + // Lazy continuation (CommonMark): a plain text line while a list is open + // and no paragraph is in progress belongs to the item above it, not to a + // new paragraph. Without this, a bullet wrapped across source lines -- + // which every shipped preset readme does -- silently splits into a + // one-item list plus an orphaned paragraph, and a wrapped ordered item + // splits its list too, so the next numbered line starts a fresh
      and + // renders "1." again instead of continuing the count. + if (list !== null && para.length === 0) { + const lastIndex = list.items.length - 1 + const lastItem = list.items[lastIndex] + if (lastItem !== undefined) { + list.items[lastIndex] = `${lastItem} ${line.trim()}` + continue + } + } flushList() para.push(line) } From b1f39fc77f7b960fb9e1d7fcb1ec1f282df72ff4 Mon Sep 17 00:00:00 2001 From: Robin Scher Date: Tue, 18 Aug 2026 14:33:07 -0700 Subject: [PATCH 14/20] fix(web,api): wire remaining product limits, fix doc comment, add Preset description cap --- internal/api/openapi.yaml | 1 + internal/api/products_test.go | 4 ++-- web/src/pages/ProductForm.tsx | 4 ++++ 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 4f81edc6..f4f7fa1e 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -1267,6 +1267,7 @@ components: type: string description: type: string + maxLength: 500 category: type: string version: diff --git a/internal/api/products_test.go b/internal/api/products_test.go index 875fadf8..2ee38fdd 100644 --- a/internal/api/products_test.go +++ b/internal/api/products_test.go @@ -575,8 +575,6 @@ func TestProducts_SubmitWithDependsOn_MissingUpstreamIs422(t *testing.T) { } } -// TestProducts_SubmitRejectsInvalidRetryOverrides asserts the product submit -// endpoint applies the same retry-override bounds as direct job submission. // TestCreateProduct_ReadmeRoundTrips verifies readme is accepted on create, // distinct from description, and comes back on both the create response and // a subsequent GET. @@ -647,6 +645,8 @@ func TestCreateProduct_RejectsOverlongMetadata(t *testing.T) { } } +// TestProducts_SubmitRejectsInvalidRetryOverrides asserts the product submit +// endpoint applies the same retry-override bounds as direct job submission. func TestProducts_SubmitRejectsInvalidRetryOverrides(t *testing.T) { srv := newProductTestServer(t) farmID, queueID := seedProductSubmitPrereqs(t, srv) diff --git a/web/src/pages/ProductForm.tsx b/web/src/pages/ProductForm.tsx index 960da98b..7c4c59bf 100644 --- a/web/src/pages/ProductForm.tsx +++ b/web/src/pages/ProductForm.tsx @@ -149,6 +149,7 @@ function ProductFormInner({ mode, defaults }: InnerProps) { aria-invalid={nameInvalid || undefined} required aria-required="true" + maxLength={PRODUCT_LIMITS.name} /> {(nameFocused || nameInvalid) && (
      @@ -180,6 +181,7 @@ function ProductFormInner({ mode, defaults }: InnerProps) { onChange={(e) => setTitle(e.target.value)} required aria-required="true" + maxLength={PRODUCT_LIMITS.title} />
      @@ -225,6 +227,7 @@ function ProductFormInner({ mode, defaults }: InnerProps) { className={styles.input} value={category} onChange={(e) => setCategory(e.target.value)} + maxLength={PRODUCT_LIMITS.category} />
      @@ -236,6 +239,7 @@ function ProductFormInner({ mode, defaults }: InnerProps) { className={styles.input} value={version} onChange={(e) => setVersion(e.target.value)} + maxLength={PRODUCT_LIMITS.version} />
      From 45606496cc1c381f1d4c04da4dbc7a9712c89aac Mon Sep 17 00:00:00 2001 From: Robin Scher Date: Tue, 18 Aug 2026 17:48:46 -0700 Subject: [PATCH 15/20] feat(web): show product and preset readme and template as tabs --- web/src/components/Tabs.module.css | 51 ++++++++ web/src/components/Tabs.test.tsx | 174 +++++++++++++++++++++++++ web/src/components/Tabs.tsx | 107 +++++++++++++++ web/src/hooks/useTabParam.test.tsx | 67 ++++++++++ web/src/hooks/useTabParam.ts | 42 ++++++ web/src/pages/PresetDetail.module.css | 7 - web/src/pages/PresetDetail.test.tsx | 57 ++++++++ web/src/pages/PresetDetail.tsx | 30 ++++- web/src/pages/ProductDetail.module.css | 7 - web/src/pages/ProductDetail.test.tsx | 72 ++++++++++ web/src/pages/ProductDetail.tsx | 30 ++++- 11 files changed, 620 insertions(+), 24 deletions(-) create mode 100644 web/src/components/Tabs.module.css create mode 100644 web/src/components/Tabs.test.tsx create mode 100644 web/src/components/Tabs.tsx create mode 100644 web/src/hooks/useTabParam.test.tsx create mode 100644 web/src/hooks/useTabParam.ts diff --git a/web/src/components/Tabs.module.css b/web/src/components/Tabs.module.css new file mode 100644 index 00000000..76fd52b3 --- /dev/null +++ b/web/src/components/Tabs.module.css @@ -0,0 +1,51 @@ +/* SPDX-License-Identifier: AGPL-3.0-or-later */ + +.tabs { + display: flex; + flex-direction: column; +} + +.tablist { + display: flex; + gap: var(--space-1); + border-bottom: 1px solid var(--color-border); +} + +.tab { + padding: var(--space-2) var(--space-4); + border: none; + border-bottom: 2px solid transparent; + background: none; + color: var(--color-text-secondary); + font-size: var(--font-size-base); + font-weight: var(--font-weight-medium); + cursor: pointer; + /* Keeps the row from reflowing when the selected tab gains weight. */ + margin-bottom: -1px; +} + +.tab:hover:not(:disabled) { + color: var(--color-text-primary); +} + +.tab[aria-selected='true'] { + color: var(--color-text-primary); + border-bottom-color: var(--color-accent); +} + +.tab:disabled { + color: var(--color-text-muted); + cursor: default; +} + +.panel { + padding-top: var(--space-4); +} + +/* The panel is focusable so keyboard users can reach its content directly from + the tab, but it is not an interactive control -- no focus ring styling of its + own beyond the browser default. */ +.panel:focus-visible { + outline: 2px solid var(--color-accent); + outline-offset: 2px; +} diff --git a/web/src/components/Tabs.test.tsx b/web/src/components/Tabs.test.tsx new file mode 100644 index 00000000..52bfb743 --- /dev/null +++ b/web/src/components/Tabs.test.tsx @@ -0,0 +1,174 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +import { describe, it, expect, vi } from 'vitest' +import { render, screen } from '@testing-library/react' +import userEvent from '@testing-library/user-event' +import Tabs from './Tabs' + +const TABS = [ + { id: 'readme', label: 'Readme' }, + { id: 'template', label: 'OpenJD Template' }, +] + +describe('Tabs', () => { + it('exposes a tablist whose tabs are named by their labels', () => { + render( + +

      panel body

      +       , + ) + expect(screen.getByRole('tablist', { name: 'Product sections' })).toBeInTheDocument() + expect(screen.getByRole('tab', { name: 'Readme' })).toBeInTheDocument() + expect(screen.getByRole('tab', { name: 'OpenJD Template' })).toBeInTheDocument() + }) + + it('marks only the active tab as selected', () => { + render( + +

      panel body

      +       , + ) + expect(screen.getByRole('tab', { name: 'Readme' })).toHaveAttribute('aria-selected', 'false') + expect(screen.getByRole('tab', { name: 'OpenJD Template' })).toHaveAttribute( + 'aria-selected', + 'true', + ) + }) + + it('renders children in a tabpanel labelled by the active tab', () => { + render( + +

      panel body

      +       , + ) + const panel = screen.getByRole('tabpanel', { name: 'Readme' }) + expect(panel).toHaveTextContent('panel body') + }) + + it('calls onChange with the tab id when a tab is clicked', async () => { + const user = userEvent.setup() + const onChange = vi.fn() + render( + +

      panel body

      +       , + ) + await user.click(screen.getByRole('tab', { name: 'OpenJD Template' })) + expect(onChange).toHaveBeenCalledWith('template') + }) + + // Roving tabindex: only the active tab is reachable by Tab, and the arrow + // keys move between tabs. Without this a keyboard user has to step through + // every tab to reach the panel. + it('keeps only the active tab in the tab order', () => { + render( + +

      panel body

      +       , + ) + expect(screen.getByRole('tab', { name: 'Readme' })).toHaveAttribute('tabindex', '0') + expect(screen.getByRole('tab', { name: 'OpenJD Template' })).toHaveAttribute('tabindex', '-1') + }) + + it('moves to the next tab on ArrowRight', async () => { + const user = userEvent.setup() + const onChange = vi.fn() + render( + +

      panel body

      +       , + ) + screen.getByRole('tab', { name: 'Readme' }).focus() + await user.keyboard('{ArrowRight}') + expect(onChange).toHaveBeenCalledWith('template') + }) + + it('wraps from the last tab to the first on ArrowRight', async () => { + const user = userEvent.setup() + const onChange = vi.fn() + render( + +

      panel body

      +       , + ) + screen.getByRole('tab', { name: 'OpenJD Template' }).focus() + await user.keyboard('{ArrowRight}') + expect(onChange).toHaveBeenCalledWith('readme') + }) + + it('moves to the previous tab on ArrowLeft', async () => { + const user = userEvent.setup() + const onChange = vi.fn() + render( + +

      panel body

      +       , + ) + screen.getByRole('tab', { name: 'OpenJD Template' }).focus() + await user.keyboard('{ArrowLeft}') + expect(onChange).toHaveBeenCalledWith('readme') + }) + + it('jumps to the first tab on Home and the last on End', async () => { + const user = userEvent.setup() + const onChange = vi.fn() + render( + +

      panel body

      +       , + ) + screen.getByRole('tab', { name: 'OpenJD Template' }).focus() + await user.keyboard('{Home}') + expect(onChange).toHaveBeenLastCalledWith('readme') + await user.keyboard('{End}') + expect(onChange).toHaveBeenLastCalledWith('template') + }) + + describe('disabled tabs', () => { + const WITH_DISABLED = [ + { id: 'readme', label: 'Readme', disabled: true }, + { id: 'template', label: 'OpenJD Template' }, + ] + + it('marks a disabled tab as disabled', () => { + render( + +

      panel body

      +       , + ) + expect(screen.getByRole('tab', { name: 'Readme' })).toBeDisabled() + }) + + it('does not call onChange when a disabled tab is clicked', async () => { + const user = userEvent.setup() + const onChange = vi.fn() + render( + +

      panel body

      +       , + ) + await user.click(screen.getByRole('tab', { name: 'Readme' })) + expect(onChange).not.toHaveBeenCalled() + }) + + // Arrow keys must step OVER a disabled tab rather than landing on it, + // otherwise the keyboard user gets stuck on a tab that cannot activate. + it('skips a disabled tab when arrowing', async () => { + const user = userEvent.setup() + const onChange = vi.fn() + const three = [ + { id: 'a', label: 'A' }, + { id: 'b', label: 'B', disabled: true }, + { id: 'c', label: 'C' }, + ] + render( + +

      panel body

      +       , + ) + screen.getByRole('tab', { name: 'A' }).focus() + await user.keyboard('{ArrowRight}') + expect(onChange).toHaveBeenCalledWith('c') + }) + }) +}) diff --git a/web/src/components/Tabs.tsx b/web/src/components/Tabs.tsx new file mode 100644 index 00000000..d80c548b --- /dev/null +++ b/web/src/components/Tabs.tsx @@ -0,0 +1,107 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +import { useCallback, useId, type KeyboardEvent, type ReactNode } from 'react' +import styles from './Tabs.module.css' + +export interface TabDef { + /** Stable identifier, also used as the URL `?tab=` value. */ + id: string + /** Visible, accessible name for the tab. */ + label: string + /** When true the tab cannot be selected and is skipped by the arrow keys. */ + disabled?: boolean +} + +interface TabsProps { + tabs: TabDef[] + /** Currently selected tab id. */ + active: string + onChange: (id: string) => void + /** Accessible name for the tablist itself. */ + label: string + /** The active tab's panel content — the caller renders one panel at a time. */ + children: ReactNode +} + +/** + * Controlled tabs following the WAI-ARIA tabs pattern: a roving tabindex so + * only the selected tab is in the tab order, arrow keys to move between tabs + * (wrapping, and skipping disabled ones), and Home/End to jump to the ends. + * + * Only the active panel is rendered. Callers own the panel content, so a tab + * whose data is absent should be passed `disabled` rather than given an empty + * panel. + */ +export default function Tabs({ tabs, active, onChange, label, children }: TabsProps) { + const baseId = useId() + const tabId = (id: string) => `${baseId}-tab-${id}` + const panelId = (id: string) => `${baseId}-panel-${id}` + + const activeTab = tabs.find((t) => t.id === active) + + const onKeyDown = useCallback( + (e: KeyboardEvent) => { + const selectable = tabs.filter((t) => t.disabled !== true) + if (selectable.length === 0) return + const current = selectable.findIndex((t) => t.id === active) + let next: TabDef | undefined + switch (e.key) { + case 'ArrowRight': + next = selectable[(current + 1) % selectable.length] + break + case 'ArrowLeft': + next = selectable[(current - 1 + selectable.length) % selectable.length] + break + case 'Home': + next = selectable[0] + break + case 'End': + next = selectable[selectable.length - 1] + break + default: + return + } + e.preventDefault() + if (next !== undefined) onChange(next.id) + }, + [tabs, active, onChange], + ) + + return ( +
      +
      + {tabs.map((t) => { + const selected = t.id === active + return ( + + ) + })} +
      + {activeTab !== undefined && ( +
      + {children} +
      + )} +
      + ) +} diff --git a/web/src/hooks/useTabParam.test.tsx b/web/src/hooks/useTabParam.test.tsx new file mode 100644 index 00000000..336f61dd --- /dev/null +++ b/web/src/hooks/useTabParam.test.tsx @@ -0,0 +1,67 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +import { describe, it, expect } from 'vitest' +import { renderHook, act } from '@testing-library/react' +import { MemoryRouter, useLocation } from 'react-router' +import type { ReactNode } from 'react' +import { useTabParam } from './useTabParam' + +function wrapper(initial: string) { + return ({ children }: { children: ReactNode }) => ( + {children} + ) +} + +const VALID = ['readme', 'template'] + +describe('useTabParam', () => { + it('reads an existing ?tab= value', () => { + const { result } = renderHook(() => useTabParam(VALID, 'template'), { + wrapper: wrapper('/x?tab=readme'), + }) + expect(result.current.tab).toBe('readme') + }) + + it('falls back to the default when the param is absent', () => { + const { result } = renderHook(() => useTabParam(VALID, 'template'), { wrapper: wrapper('/x') }) + expect(result.current.tab).toBe('template') + }) + + // A ?tab= value from a stale link or a hand-edited URL must not select a tab + // that does not exist — the page would render no panel at all. + it('falls back to the default when the param names an unknown tab', () => { + const { result } = renderHook(() => useTabParam(VALID, 'template'), { + wrapper: wrapper('/x?tab=nonsense'), + }) + expect(result.current.tab).toBe('template') + }) + + it('setTab writes the param', () => { + const { result } = renderHook(() => useTabParam(VALID, 'template'), { wrapper: wrapper('/x') }) + act(() => result.current.setTab('readme')) + expect(result.current.tab).toBe('readme') + }) + + it('setTab preserves other query params', () => { + const { result } = renderHook( + () => ({ tabs: useTabParam(VALID, 'template'), loc: useLocation() }), + { wrapper: wrapper('/x?search=nuke') }, + ) + act(() => result.current.tabs.setTab('readme')) + expect(result.current.loc.search).toContain('search=nuke') + expect(result.current.loc.search).toContain('tab=readme') + }) + + // Selecting the default tab drops the param rather than pinning it, so a + // shared URL stays clean and the page keeps following its own default if + // that default later changes. + it('setTab to the default removes the param', () => { + const { result } = renderHook( + () => ({ tabs: useTabParam(VALID, 'template'), loc: useLocation() }), + { wrapper: wrapper('/x?tab=readme') }, + ) + act(() => result.current.tabs.setTab('template')) + expect(result.current.loc.search).not.toContain('tab=') + expect(result.current.tabs.tab).toBe('template') + }) +}) diff --git a/web/src/hooks/useTabParam.ts b/web/src/hooks/useTabParam.ts new file mode 100644 index 00000000..684341c4 --- /dev/null +++ b/web/src/hooks/useTabParam.ts @@ -0,0 +1,42 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +import { useCallback } from 'react' +import { useSearchParams } from 'react-router' + +const TAB_PARAM = 'tab' + +export interface UseTabParamResult { + tab: string + setTab: (value: string) => void +} + +/** + * URL-persisted `?tab=` selection for pages with a tabbed region — the cousin + * of useSearchParam for tab state. + * + * Living in the URL is what lets a list row deep-link straight to a specific + * tab, and keeps the browser Back button stepping through tab changes. + * + * A param naming a tab outside `valid` falls back to `fallback` rather than + * selecting nothing, so a stale link cannot render a page with no panel. + * Selecting `fallback` removes the param instead of pinning it, keeping shared + * URLs clean. + */ +export function useTabParam(valid: readonly string[], fallback: string): UseTabParamResult { + const [searchParams, setSearchParams] = useSearchParams() + const raw = searchParams.get(TAB_PARAM) + const tab = raw !== null && valid.includes(raw) ? raw : fallback + + const setTab = useCallback( + (value: string) => + setSearchParams((prev) => { + const next = new URLSearchParams(prev) + if (value === fallback) next.delete(TAB_PARAM) + else next.set(TAB_PARAM, value) + return next + }), + [setSearchParams, fallback], + ) + + return { tab, setTab } +} diff --git a/web/src/pages/PresetDetail.module.css b/web/src/pages/PresetDetail.module.css index 828f36cd..3e9160e1 100644 --- a/web/src/pages/PresetDetail.module.css +++ b/web/src/pages/PresetDetail.module.css @@ -62,13 +62,6 @@ color: var(--color-text-primary); } -.sectionTitle { - font-size: var(--font-size-sm); - font-weight: var(--font-weight-semibold); - color: var(--color-text-secondary); - margin: 0; -} - .template { margin: 0; padding: var(--space-4); diff --git a/web/src/pages/PresetDetail.test.tsx b/web/src/pages/PresetDetail.test.tsx index 5417b0d3..47731993 100644 --- a/web/src/pages/PresetDetail.test.tsx +++ b/web/src/pages/PresetDetail.test.tsx @@ -206,3 +206,60 @@ describe('PresetDetail', () => { }) }) }) + +describe('PresetDetail readme/template tabs', () => { + it('presents the readme and template as tabs', async () => { + fetchMock.mockResolvedValueOnce(ok(makePreset({ readme: '# Usage' }))) + renderDetail('/presets/nuke-comp') + await screen.findByRole('tablist', { name: /preset sections/i }) + expect(screen.getByRole('tab', { name: 'Readme' })).toBeInTheDocument() + expect(screen.getByRole('tab', { name: 'OpenJD Template' })).toBeInTheDocument() + }) + + it('opens on the readme tab when the preset has one', async () => { + fetchMock.mockResolvedValueOnce(ok(makePreset({ readme: '# Usage' }))) + renderDetail('/presets/nuke-comp') + expect(await screen.findByRole('tab', { name: 'Readme' })).toHaveAttribute( + 'aria-selected', + 'true', + ) + expect(screen.getByText('Usage')).toBeInTheDocument() + expect(screen.queryByText('name: nuke-template')).not.toBeInTheDocument() + }) + + it('opens on the template tab when the preset has no readme', async () => { + fetchMock.mockResolvedValueOnce(ok(makePreset({ readme: '' }))) + renderDetail('/presets/nuke-comp') + expect(await screen.findByRole('tab', { name: 'OpenJD Template' })).toHaveAttribute( + 'aria-selected', + 'true', + ) + expect(screen.getByText('name: nuke-template')).toBeInTheDocument() + }) + + it('disables the readme tab when the preset has no readme', async () => { + fetchMock.mockResolvedValueOnce(ok(makePreset({ readme: '' }))) + renderDetail('/presets/nuke-comp') + expect(await screen.findByRole('tab', { name: 'Readme' })).toBeDisabled() + }) + + it('honours ?tab=template over the readme default', async () => { + fetchMock.mockResolvedValueOnce(ok(makePreset({ readme: '# Usage' }))) + renderDetail('/presets/nuke-comp?tab=template') + expect(await screen.findByRole('tab', { name: 'OpenJD Template' })).toHaveAttribute( + 'aria-selected', + 'true', + ) + expect(screen.queryByText('Usage')).not.toBeInTheDocument() + }) + + it('swaps the panel when another tab is clicked', async () => { + const user = userEvent.setup() + fetchMock.mockResolvedValueOnce(ok(makePreset({ readme: '# Usage' }))) + renderDetail('/presets/nuke-comp') + await screen.findByText('Usage') + await user.click(screen.getByRole('tab', { name: 'OpenJD Template' })) + expect(screen.getByText('name: nuke-template')).toBeInTheDocument() + expect(screen.queryByText('Usage')).not.toBeInTheDocument() + }) +}) diff --git a/web/src/pages/PresetDetail.tsx b/web/src/pages/PresetDetail.tsx index 2d25ae14..f3845735 100644 --- a/web/src/pages/PresetDetail.tsx +++ b/web/src/pages/PresetDetail.tsx @@ -4,6 +4,8 @@ import { useCallback } from 'react' import { useNavigate, useParams } from 'react-router' import PageHeader from '@/components/PageHeader' import Markdown from '@/components/Markdown' +import Tabs from '@/components/Tabs' +import { useTabParam } from '@/hooks/useTabParam' import { useToast } from '@/components/Toast' import { usePreset } from '@/api/queries' import { useInstallPreset } from '@/api/mutations' @@ -11,6 +13,8 @@ import { useAuth } from '@/auth/context' import { can } from '@/auth/policy' import styles from './PresetDetail.module.css' +const TAB_IDS = ['readme', 'template'] as const + export default function PresetDetail() { const params = useParams<{ name: string }>() const name = params.name ?? '' @@ -19,6 +23,10 @@ export default function PresetDetail() { const { principal } = useAuth() const canManage = can(principal, 'products.manage') const { data: preset, isLoading, isError } = usePreset(name) + // Default to the readme when there is one, else the template. Computed from + // the loaded preset, so the hook runs unconditionally and re-derives once + // the fetch resolves. + const { tab, setTab } = useTabParam(TAB_IDS, preset?.readme ? 'readme' : 'template') const install = useInstallPreset() const handleInstall = useCallback(async () => { @@ -67,7 +75,6 @@ export default function PresetDetail() {

      {preset.title}

      {preset.description &&

      {preset.description}

      } - {preset.readme && }
      @@ -81,10 +88,23 @@ export default function PresetDetail() {
      {preset.status}
      -

      OpenJD Template

      - 
      -        {preset.template}
-
      + + {tab === 'readme' ? ( + + ) : ( + 
      +            {preset.template}
+
      + )} +       ) } diff --git a/web/src/pages/ProductDetail.module.css b/web/src/pages/ProductDetail.module.css index 853d6430..8e9df231 100644 --- a/web/src/pages/ProductDetail.module.css +++ b/web/src/pages/ProductDetail.module.css @@ -95,13 +95,6 @@ border-color: var(--color-success); } -.sectionTitle { - font-size: var(--font-size-sm); - font-weight: var(--font-weight-semibold); - color: var(--color-text-secondary); - margin: 0; -} - .template { margin: 0; padding: var(--space-4); diff --git a/web/src/pages/ProductDetail.test.tsx b/web/src/pages/ProductDetail.test.tsx index 159b7a80..d2e62ca1 100644 --- a/web/src/pages/ProductDetail.test.tsx +++ b/web/src/pages/ProductDetail.test.tsx @@ -242,3 +242,75 @@ describe('ProductDetail', () => { }) }) }) + +describe('ProductDetail readme/template tabs', () => { + it('presents the readme and template as tabs', async () => { + fetchMock.mockResolvedValueOnce(ok(makeProduct({ readme: '# Usage' }))) + renderDetail('/products/my-render') + await screen.findByRole('tablist', { name: /product sections/i }) + expect(screen.getByRole('tab', { name: 'Readme' })).toBeInTheDocument() + expect(screen.getByRole('tab', { name: 'OpenJD Template' })).toBeInTheDocument() + }) + + it('opens on the readme tab when the product has one', async () => { + fetchMock.mockResolvedValueOnce(ok(makeProduct({ readme: '# Usage' }))) + renderDetail('/products/my-render') + expect(await screen.findByRole('tab', { name: 'Readme' })).toHaveAttribute( + 'aria-selected', + 'true', + ) + expect(screen.getByText('Usage')).toBeInTheDocument() + // Only one panel is mounted, so the template must not be on the page. + expect(screen.queryByText('name: my-template')).not.toBeInTheDocument() + }) + + it('opens on the template tab when the product has no readme', async () => { + fetchMock.mockResolvedValueOnce(ok(makeProduct({ readme: '' }))) + renderDetail('/products/my-render') + expect(await screen.findByRole('tab', { name: 'OpenJD Template' })).toHaveAttribute( + 'aria-selected', + 'true', + ) + expect(screen.getByText('name: my-template')).toBeInTheDocument() + }) + + it('disables the readme tab when the product has no readme', async () => { + fetchMock.mockResolvedValueOnce(ok(makeProduct({ readme: '' }))) + renderDetail('/products/my-render') + expect(await screen.findByRole('tab', { name: 'Readme' })).toBeDisabled() + }) + + // The list-row readme button deep-links to ?tab=readme, so the param has to + // win over the page's own default. + it('honours ?tab=template over the readme default', async () => { + fetchMock.mockResolvedValueOnce(ok(makeProduct({ readme: '# Usage' }))) + renderDetail('/products/my-render?tab=template') + expect(await screen.findByRole('tab', { name: 'OpenJD Template' })).toHaveAttribute( + 'aria-selected', + 'true', + ) + expect(screen.getByText('name: my-template')).toBeInTheDocument() + expect(screen.queryByText('Usage')).not.toBeInTheDocument() + }) + + it('swaps the panel when another tab is clicked', async () => { + const user = userEvent.setup() + fetchMock.mockResolvedValueOnce(ok(makeProduct({ readme: '# Usage' }))) + renderDetail('/products/my-render') + await screen.findByText('Usage') + await user.click(screen.getByRole('tab', { name: 'OpenJD Template' })) + expect(screen.getByText('name: my-template')).toBeInTheDocument() + expect(screen.queryByText('Usage')).not.toBeInTheDocument() + }) + + // The description is page-level context, not tab content — it stays put so a + // reader sees what the product is regardless of which tab is open. + it('keeps the description visible on both tabs', async () => { + const user = userEvent.setup() + fetchMock.mockResolvedValueOnce(ok(makeProduct({ readme: '# Usage' }))) + renderDetail('/products/my-render') + expect(await screen.findByText('desc')).toBeInTheDocument() + await user.click(screen.getByRole('tab', { name: 'OpenJD Template' })) + expect(screen.getByText('desc')).toBeInTheDocument() + }) +}) diff --git a/web/src/pages/ProductDetail.tsx b/web/src/pages/ProductDetail.tsx index ef0c17ff..93b8f03e 100644 --- a/web/src/pages/ProductDetail.tsx +++ b/web/src/pages/ProductDetail.tsx @@ -4,6 +4,8 @@ import { useCallback } from 'react' import { Link, useNavigate, useParams } from 'react-router' import PageHeader from '@/components/PageHeader' import Markdown from '@/components/Markdown' +import Tabs from '@/components/Tabs' +import { useTabParam } from '@/hooks/useTabParam' import { useToast } from '@/components/Toast' import { useProduct } from '@/api/queries' import { useDeleteProduct } from '@/api/mutations' @@ -12,6 +14,8 @@ import { can } from '@/auth/policy' import type { ProductDuplicateState } from './ProductForm' import styles from './ProductDetail.module.css' +const TAB_IDS = ['readme', 'template'] as const + export default function ProductDetail() { const params = useParams<{ name: string }>() const name = params.name ?? '' @@ -21,6 +25,10 @@ export default function ProductDetail() { const canManage = can(principal, 'products.manage') const { data: product, isLoading, isError } = useProduct(name) const deleteProduct = useDeleteProduct() + // Default to the readme when there is one, else the template. Computed from + // the loaded product, so the hook runs unconditionally and simply re-derives + // once the fetch resolves. + const { tab, setTab } = useTabParam(TAB_IDS, product?.readme ? 'readme' : 'template') const handleDelete = useCallback(async () => { if (!product) return @@ -100,7 +108,6 @@ export default function ProductDetail() {

      {product.title}

      {product.description &&

      {product.description}

      } - {product.readme && }
      @@ -118,10 +125,23 @@ export default function ProductDetail() {
      -

      OpenJD Template

      - 
      -        {product.template}
-
      + + {tab === 'readme' ? ( + + ) : ( + 
      +            {product.template}
+
      + )} +       ) } From c6d36a3410e393f51767c639671d05e69c3591a5 Mon Sep 17 00:00:00 2001 From: Robin Scher Date: Tue, 18 Aug 2026 17:52:25 -0700 Subject: [PATCH 16/20] feat(web): add readme buttons to product and preset lists and the picker --- web/src/components/ReadmeButton.tsx | 66 ++++++++++++++++++++++++++ web/src/pages/PresetLibrary.module.css | 10 ++++ web/src/pages/PresetLibrary.test.tsx | 21 ++++++++ web/src/pages/PresetLibrary.tsx | 10 ++++ web/src/pages/ProductList.module.css | 10 ++++ web/src/pages/ProductList.test.tsx | 28 +++++++++++ web/src/pages/ProductList.tsx | 9 +++- web/src/pages/ProductPicker.module.css | 34 ++++++++++++- web/src/pages/ProductPicker.test.tsx | 26 ++++++++++ web/src/pages/ProductPicker.tsx | 18 +++++-- web/src/pages/entityList.module.css | 35 ++++++++++++++ 11 files changed, 261 insertions(+), 6 deletions(-) create mode 100644 web/src/components/ReadmeButton.tsx diff --git a/web/src/components/ReadmeButton.tsx b/web/src/components/ReadmeButton.tsx new file mode 100644 index 00000000..4f52cb55 --- /dev/null +++ b/web/src/components/ReadmeButton.tsx @@ -0,0 +1,66 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +import { Link } from 'react-router' +import IconButton from '@/components/IconButton' +import { Document } from '@/components/icons' + +interface ReadmeButtonProps { + /** Destination, normally a detail route carrying `?tab=readme`. */ + to: string + /** Accessible name, e.g. "View readme for maya-render". */ + label: string + /** + * Whether the item is known to have a readme. False renders a disabled + * control titled "No readme". + * + * The preset list passes true unconditionally: the remote library index + * deliberately carries no readme, so a list row cannot know. The detail page + * it links to then shows whatever is actually there. + */ + hasReadme: boolean + /** + * Open in a new tab. Used from the submit flow's product picker, where a + * same-tab navigation would abandon the job the user is in the middle of + * starting. + */ + newTab?: boolean + className?: string | undefined +} + +/** + * Row/card affordance for jumping to an item's rendered readme. + * + * Renders a real link when a readme exists — so cmd/middle-click open it in a + * new tab as with any link — and a disabled button when it does not, since an + * anchor has no disabled state. + */ +export default function ReadmeButton({ + to, + label, + hasReadme, + newTab = false, + className, +}: ReadmeButtonProps) { + if (!hasReadme) { + return ( + } + label={label} + title="No readme" + disabled + className={className} + /> + ) + } + return ( + + + + ) +} diff --git a/web/src/pages/PresetLibrary.module.css b/web/src/pages/PresetLibrary.module.css index f2a42b67..81550c7f 100644 --- a/web/src/pages/PresetLibrary.module.css +++ b/web/src/pages/PresetLibrary.module.css @@ -127,3 +127,13 @@ .refreshBtn:hover { background: var(--color-surface); } + +.readmeBtn { + composes: rowAction from './entityList.module.css'; +} + +.actions { + display: flex; + gap: var(--space-1); + justify-content: flex-end; +} diff --git a/web/src/pages/PresetLibrary.test.tsx b/web/src/pages/PresetLibrary.test.tsx index f4054ba9..221a3e28 100644 --- a/web/src/pages/PresetLibrary.test.tsx +++ b/web/src/pages/PresetLibrary.test.tsx @@ -159,3 +159,24 @@ describe('PresetLibrary', () => { expect(screen.queryByRole('searchbox')).not.toBeInTheDocument() }) }) + +describe('PresetLibrary readme button', () => { + it('links to the preset detail page with the readme tab open', async () => { + fetchMock.mockResolvedValue(ok([makePreset({ name: 'nuke-comp' })])) + renderPage() + const link = await screen.findByRole('link', { name: /view readme for nuke-comp/i }) + expect(link).toHaveAttribute('href', '/presets/nuke-comp?tab=readme') + }) + + // The remote library index deliberately carries no readme field, so a list + // row cannot know whether one exists. The button is therefore always + // enabled and the detail page shows whatever is actually there. If this ever + // becomes a disabled-state test, the index format changed — read the spec. + it('is enabled even though the index carries no readme', async () => { + fetchMock.mockResolvedValue(ok([makePreset({ name: 'nuke-comp' })])) + renderPage() + const link = await screen.findByRole('link', { name: /view readme for nuke-comp/i }) + expect(link).not.toHaveAttribute('aria-disabled', 'true') + expect(screen.queryByRole('button', { name: /view readme for nuke-comp/i })).toBeNull() + }) +}) diff --git a/web/src/pages/PresetLibrary.tsx b/web/src/pages/PresetLibrary.tsx index c7bd6297..67f3155a 100644 --- a/web/src/pages/PresetLibrary.tsx +++ b/web/src/pages/PresetLibrary.tsx @@ -6,6 +6,7 @@ import PageHeader from '@/components/PageHeader' import { useToast } from '@/components/Toast' import DebouncedSearchInput from '@/components/DebouncedSearchInput' import ErrorBanner from '@/components/ErrorBanner' +import ReadmeButton from '@/components/ReadmeButton' import { useSearchParam } from '@/hooks/useSearchParam' import { filterBySearch } from '@/utils/filterBySearch' import { usePresets, queryKeys, fetchPresets } from '@/api/queries' @@ -113,6 +114,7 @@ export default function PresetLibrary() { Title Version Status + @@ -132,6 +134,14 @@ export default function PresetLibrary() { {STATUS_LABEL[p.status] ?? p.status} + + + ))} diff --git a/web/src/pages/ProductList.module.css b/web/src/pages/ProductList.module.css index e13475ec..bf353e46 100644 --- a/web/src/pages/ProductList.module.css +++ b/web/src/pages/ProductList.module.css @@ -26,6 +26,10 @@ composes: deleteBtn from './entityList.module.css'; } +.readmeBtn { + composes: rowAction from './entityList.module.css'; +} + .newBtn { composes: newBtn from './entityList.module.css'; } @@ -61,3 +65,9 @@ color: var(--color-success); border-color: var(--color-success); } + +.actions { + display: flex; + gap: var(--space-1); + justify-content: flex-end; +} diff --git a/web/src/pages/ProductList.test.tsx b/web/src/pages/ProductList.test.tsx index a3b569c5..837a1ad0 100644 --- a/web/src/pages/ProductList.test.tsx +++ b/web/src/pages/ProductList.test.tsx @@ -214,3 +214,31 @@ describe('ProductList', () => { }) }) }) + +describe('ProductList readme button', () => { + it('links to the product detail page with the readme tab open', async () => { + fetchMock.mockResolvedValue(ok([makeProduct({ name: 'alpha', readme: '# Docs' })])) + renderList() + const link = await screen.findByRole('link', { name: /view readme for alpha/i }) + expect(link).toHaveAttribute('href', '/products/alpha?tab=readme') + }) + + it('is disabled with an explanatory title when the product has no readme', async () => { + fetchMock.mockResolvedValue(ok([makeProduct({ name: 'alpha', readme: '' })])) + renderList() + const btn = await screen.findByRole('button', { name: /view readme for alpha/i }) + expect(btn).toBeDisabled() + expect(btn).toHaveAttribute('title', 'No readme') + }) + + // Reading documentation is a read action, so the control is not gated on + // products.manage and appears for built-ins, which have no delete button. + it('shows the readme button for a builtin product', async () => { + fetchMock.mockResolvedValue( + ok([makeProduct({ name: 'script', source: 'builtin', readme: '# D' })]), + ) + renderList() + expect(await screen.findByRole('link', { name: /view readme for script/i })).toBeInTheDocument() + expect(screen.queryByRole('button', { name: /delete product script/i })).not.toBeInTheDocument() + }) +}) diff --git a/web/src/pages/ProductList.tsx b/web/src/pages/ProductList.tsx index 6ba2dd96..ecab55f0 100644 --- a/web/src/pages/ProductList.tsx +++ b/web/src/pages/ProductList.tsx @@ -4,6 +4,7 @@ import { useCallback, useState } from 'react' import { Link } from 'react-router' import PageHeader from '@/components/PageHeader' import IconButton from '@/components/IconButton' +import ReadmeButton from '@/components/ReadmeButton' import { Trash } from '@/components/icons' import { useToast } from '@/components/Toast' import { useProducts } from '@/api/queries' @@ -145,7 +146,13 @@ export default function ProductList() { {product.source} - + + {canManage && product.source !== 'builtin' && ( } diff --git a/web/src/pages/ProductPicker.module.css b/web/src/pages/ProductPicker.module.css index d7712b6a..4afcc608 100644 --- a/web/src/pages/ProductPicker.module.css +++ b/web/src/pages/ProductPicker.module.css @@ -45,14 +45,46 @@ } .card { - display: block; + display: flex; + align-items: flex-start; + gap: var(--space-2); padding: var(--space-3); border: 1px solid var(--color-border); border-radius: var(--radius-md); +} + +/* The card's clickable body. It takes the remaining width so the whole card + area still selects the product, with the readme control beside it. */ +.cardLink { + flex: 1; + min-width: 0; text-decoration: none; color: inherit; } +.readmeBtn { + display: inline-flex; + align-items: center; + justify-content: center; + flex-shrink: 0; + padding: var(--space-1); + border-radius: var(--radius-sm); + border: 1px solid var(--color-border); + background: transparent; + color: var(--color-text-secondary); + cursor: pointer; +} + +.readmeBtn:hover:not(:disabled) { + background: var(--color-surface-raised); + color: var(--color-text-primary); +} + +.readmeBtn:disabled { + opacity: 0.4; + cursor: not-allowed; +} + .advanced { margin-top: var(--space-4); } diff --git a/web/src/pages/ProductPicker.test.tsx b/web/src/pages/ProductPicker.test.tsx index 69a5ef32..7c4b8739 100644 --- a/web/src/pages/ProductPicker.test.tsx +++ b/web/src/pages/ProductPicker.test.tsx @@ -15,6 +15,7 @@ vi.mock('@/api/queries', async (orig) => ({ title: 'My Tool', category: 'misc', description: 'An internal utility', + readme: '# My Tool\n\nDocs.', version: '1', source: 'custom', template: '', @@ -25,6 +26,7 @@ vi.mock('@/api/queries', async (orig) => ({ title: 'Blender', category: 'render', description: '', + readme: '', version: '1', source: 'builtin', template: '', @@ -113,3 +115,27 @@ describe('ProductPicker', () => { expect(screen.getByRole('link', { name: /raw OpenJD/i })).toHaveAttribute('href', '/submit/raw') }) }) + +describe('ProductPicker readme button', () => { + it('links to the product readme in a new tab', () => { + renderPage() + const link = screen.getByRole('link', { name: /view readme for my-tool/i }) + expect(link).toHaveAttribute('href', '/products/my-tool?tab=readme') + // A same-tab navigation here would abandon the job the user is starting. + expect(link).toHaveAttribute('target', '_blank') + expect(link).toHaveAttribute('rel', 'noopener noreferrer') + }) + + it('keeps the card itself pointing at the submit step', () => { + renderPage() + expect(screen.getByRole('link', { name: /my tool/i })).toHaveAttribute( + 'href', + '/submit/product/my-tool', + ) + }) + + it('disables the readme button when the product has no readme', () => { + renderPage() + expect(screen.getByRole('button', { name: /view readme for blender/i })).toBeDisabled() + }) +}) diff --git a/web/src/pages/ProductPicker.tsx b/web/src/pages/ProductPicker.tsx index 8d38fbe6..55d71c5c 100644 --- a/web/src/pages/ProductPicker.tsx +++ b/web/src/pages/ProductPicker.tsx @@ -5,6 +5,7 @@ import DebouncedSearchInput from '@/components/DebouncedSearchInput' import { useSearchParam } from '@/hooks/useSearchParam' import { filterBySearch } from '@/utils/filterBySearch' import { useProducts } from '@/api/queries' +import ReadmeButton from '@/components/ReadmeButton' import type { Product, ProductSource } from '@/api/types' import styles from './ProductPicker.module.css' @@ -18,10 +19,19 @@ const GROUPS: { source: ProductSource; label: string }[] = [ function ProductCard({ product }: { product: Product }) { return ( - - {product.title || product.name} - {product.description &&

      {product.description}

      } - +
      + + {product.title || product.name} + {product.description &&

      {product.description}

      } + + +
      ) } diff --git a/web/src/pages/entityList.module.css b/web/src/pages/entityList.module.css index eb93cb5e..902e41c7 100644 --- a/web/src/pages/entityList.module.css +++ b/web/src/pages/entityList.module.css @@ -98,6 +98,41 @@ tr:hover .deleteBtn { cursor: not-allowed; } +.rowAction { + display: inline-flex; + align-items: center; + justify-content: center; + padding: var(--space-1); + border-radius: var(--radius-sm); + border: 1px solid var(--color-border); + background: transparent; + color: var(--color-text-secondary); + cursor: pointer; + opacity: 0; + transition: + opacity var(--transition-fast), + background var(--transition-fast); +} + +tr:hover .rowAction, +.rowAction:focus-visible { + opacity: 1; +} + +.rowAction:hover:not(:disabled) { + background: var(--color-surface-raised); + color: var(--color-text-primary); +} + +.rowAction:disabled { + opacity: 0; + cursor: not-allowed; +} + +tr:hover .rowAction:disabled { + opacity: 0.4; +} + /* ── Header action ───────────────────────────────────────────────────────────── */ .newBtn { From 8470d464faaa1141b3a09b4d930e25acb44ac31c Mon Sep 17 00:00:00 2001 From: Robin Scher Date: Tue, 18 Aug 2026 17:56:19 -0700 Subject: [PATCH 17/20] docs(products): give the built-in products readmes demonstrating the markdown subset --- internal/product/builtins/container.yaml | 29 ++++++++++ internal/product/builtins/python.yaml | 29 ++++++++++ internal/product/builtins/script.yaml | 28 ++++++++++ internal/product/builtins_test.go | 69 ++++++++++++++++++++++++ 4 files changed, 155 insertions(+) diff --git a/internal/product/builtins/container.yaml b/internal/product/builtins/container.yaml index c694c7c1..dd912b48 100644 --- a/internal/product/builtins/container.yaml +++ b/internal/product/builtins/container.yaml @@ -2,6 +2,35 @@ name: container title: Run a Docker Image description: Run a command inside a Docker image. +readme: | + # Run a Docker Image + + Runs a container image to completion on a worker, with `docker run --rm`. Use it + when the work already ships as an image and you want the farm to schedule it + rather than reproduce its dependencies on every host. + + ## Parameters + + - **Image** — the image reference to run, for example `alpine:3.20` or + `registry.example.com/team/tool:1.4.2`. + + ## Example + + ```sh + docker run --rm registry.example.com/team/nuke:15.1 + ``` + + ## Notes + + The step declares `attr.worker.tag.docker`, so it will only ever be scheduled + onto workers tagged as having Docker. A worker without that tag is not a + candidate, and a job whose queue has no such worker stays unscheduled rather + than failing — check the queue's unschedulable reason if a job sits idle. + + The image is run with **no arguments and no volumes**. Anything the container + needs from the host has to be baked into the image or fetched by its own + entrypoint. Copy this product to a custom one if you need to mount storage or + pass a command. category: General version: 1.0.0 template: diff --git a/internal/product/builtins/python.yaml b/internal/product/builtins/python.yaml index eec44b4e..5db30362 100644 --- a/internal/product/builtins/python.yaml +++ b/internal/product/builtins/python.yaml @@ -2,6 +2,35 @@ name: python title: Run a Python Script description: Run a Python script with a chosen interpreter. +readme: | + # Run a Python Script + + Runs a Python script on a worker. The script is written to a file next to the + task and executed with the interpreter you name, so it needs no quoting + gymnastics and can be as long as you like. + + ## Parameters + + - **Interpreter** — the Python to run, `python3` by default. Give an absolute + path to pin a specific install, for example `/opt/rez/python/3.11/bin/python`. + - **Python Script** — the script body itself. + + ## Example + + ```python + import sys + print("running on", sys.platform) + ``` + + ## Notes + + The script is delivered as an OpenJD embedded file, so it reaches the worker as + a real `script.py` on disk rather than as a command-line argument. That is why + multi-line scripts, quotes and backslashes all survive intact. + + Nothing installs the interpreter for you. If a worker has no `python3` on its + `PATH` the task fails at launch, so pin the path or gate the step with a + `hostRequirements` tag when your fleet is mixed. category: General version: 1.0.0 template: diff --git a/internal/product/builtins/script.yaml b/internal/product/builtins/script.yaml index efa8f488..0764614c 100644 --- a/internal/product/builtins/script.yaml +++ b/internal/product/builtins/script.yaml @@ -2,6 +2,34 @@ name: script title: Run a Shell Command description: Run an arbitrary shell command on a worker. +readme: | + # Run a Shell Command + + Runs one shell command on a worker, as a single task. The smallest useful + product in sqi, and the quickest way to check that a queue and its workers are + actually working. + + ## Parameters + + - **Command** — the shell text to run. It is passed to `/bin/sh -c`, so pipes, + redirection and `&&` all work. + + ## Example + + ```sh + ffmpeg -i input.mov -c:v libx264 output.mp4 + ``` + + ## Notes + + The command runs on whichever worker picks up the task, as the account that + worker runs under, in a session directory the worker creates. Nothing is + staged for you: any path in the command must already be reachable from the + worker. + + There are no host requirements, so this product will run *anywhere*. Add a + `hostRequirements` block to the template if the command needs a particular + platform or tool — see the [OpenJD specification](https://github.com/OpenJobDescription/openjd-specifications). category: General version: 1.0.0 template: diff --git a/internal/product/builtins_test.go b/internal/product/builtins_test.go index 0562b56a..e015742f 100644 --- a/internal/product/builtins_test.go +++ b/internal/product/builtins_test.go @@ -43,3 +43,72 @@ func TestBuiltins_ContainerDeclaresDockerRequirement(t *testing.T) { } t.Fatal("container built-in not found") } + +// Every built-in ships a readme. These three are the first products a new +// operator opens, so they double as the worked example of what the readme +// field is for and which Markdown the renderer actually supports. +func TestBuiltins_AllHaveAReadme(t *testing.T) { + builtins := product.Builtins() + if len(builtins) == 0 { + t.Fatal("no builtins loaded") + } + for _, p := range builtins { + if strings.TrimSpace(p.Readme) == "" { + t.Errorf("builtin %q has no readme", p.Name) + } + if err := product.ValidateMetadata(p); err != nil { + t.Errorf("builtin %q: %v", p.Name, err) + } + } +} + +// The built-in readmes are the reference for authors, so between them they +// must exercise the whole supported subset -- and nothing outside it. A +// construct the renderer does not support would render as literal text in the +// very examples people copy. +func TestBuiltins_ReadmesExerciseTheSupportedSubset(t *testing.T) { + builtins := product.Builtins() + var all strings.Builder + for _, p := range builtins { + all.WriteString(p.Readme) + all.WriteString("\n") + } + corpus := all.String() + + supported := map[string]string{ + "ATX heading": "\n# ", + "bullet list": "\n- ", + "fenced code": "```", + "bold": "**", + "inline code": "`", + "a link": "](http", + } + for name, marker := range supported { + if !strings.Contains(corpus, marker) { + t.Errorf("no builtin readme demonstrates %s (looked for %q)", name, marker) + } + } + + // Unsupported constructs render as literal text; none may appear. + for _, p := range builtins { + for _, bad := range []struct{ name, marker string }{ + {"an image", "!["}, + {"a blockquote", "\n> "}, + {"a table", "\n|"}, + {"raw HTML", "<"}, + } { + if strings.Contains(p.Readme, bad.marker) { + t.Errorf("builtin %q readme contains %s (%q), which the renderer does not support", + p.Name, bad.name, bad.marker) + } + } + // Nested list items silently lose their structure -- the renderer's + // list regexes are anchored at column 0. + for line := range strings.SplitSeq(p.Readme, "\n") { + trimmed := strings.TrimLeft(line, " ") + if len(line) > len(trimmed) && (strings.HasPrefix(trimmed, "- ") || strings.HasPrefix(trimmed, "* ")) { + t.Errorf("builtin %q readme has an indented list item %q; nesting is not supported", p.Name, line) + } + } + } +} From 5e0c307385340e40713a16dea93b01ab1ec08b63 Mon Sep 17 00:00:00 2001 From: Robin Scher Date: Tue, 18 Aug 2026 18:21:15 -0700 Subject: [PATCH 18/20] fix(web): show the server's reason when a preset fails to load --- web/src/pages/PresetDetail.test.tsx | 41 +++++++++++++++++++++++++++++ web/src/pages/PresetDetail.tsx | 14 ++++++++-- 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/web/src/pages/PresetDetail.test.tsx b/web/src/pages/PresetDetail.test.tsx index 47731993..ac5bbe8a 100644 --- a/web/src/pages/PresetDetail.test.tsx +++ b/web/src/pages/PresetDetail.test.tsx @@ -263,3 +263,44 @@ describe('PresetDetail readme/template tabs', () => { expect(screen.queryByText('Usage')).not.toBeInTheDocument() }) }) + +describe('PresetDetail error reporting', () => { + function problem(status: number, detail: string): Response { + return new Response( + JSON.stringify({ type: 'about:blank', title: 'Unprocessable Entity', status, detail }), + { status, headers: { 'Content-Type': 'application/problem+json' } }, + ) + } + + // A stale published library fails validation with a precise, actionable + // message naming the offending field. Swallowing it behind "Failed to load + // preset" turns a five-second diagnosis into a debugging session. + it('shows the server’s explanation when the definition fails validation', async () => { + fetchMock.mockResolvedValueOnce( + problem( + 422, + 'failed to load preset definition: control "LINE_EDIT" is not valid on a PATH parameter', + ), + ) + renderDetail('/presets/nuke-comp') + expect(await screen.findByRole('alert')).toHaveTextContent( + /control "LINE_EDIT" is not valid on a PATH parameter/, + ) + }) + + it('still explains itself when the library is not configured', async () => { + fetchMock.mockResolvedValueOnce(problem(503, 'preset library not configured')) + renderDetail('/presets/nuke-comp') + expect(await screen.findByRole('alert')).toHaveTextContent(/preset library not configured/) + }) + + // A transport failure has no problem document, so the generic line remains + // the honest fallback rather than rendering "undefined". + it('falls back to a generic message when there is no problem detail', async () => { + fetchMock.mockRejectedValueOnce(new Error('network down')) + renderDetail('/presets/nuke-comp') + const alert = await screen.findByRole('alert') + expect(alert).toHaveTextContent(/failed to load preset/i) + expect(alert).not.toHaveTextContent(/undefined/) + }) +}) diff --git a/web/src/pages/PresetDetail.tsx b/web/src/pages/PresetDetail.tsx index f3845735..70c179b3 100644 --- a/web/src/pages/PresetDetail.tsx +++ b/web/src/pages/PresetDetail.tsx @@ -8,6 +8,8 @@ import Tabs from '@/components/Tabs' import { useTabParam } from '@/hooks/useTabParam' import { useToast } from '@/components/Toast' import { usePreset } from '@/api/queries' +import { ApiError } from '@/api/client' +import ErrorBanner from '@/components/ErrorBanner' import { useInstallPreset } from '@/api/mutations' import { useAuth } from '@/auth/context' import { can } from '@/auth/policy' @@ -22,7 +24,7 @@ export default function PresetDetail() { const { showToast } = useToast() const { principal } = useAuth() const canManage = can(principal, 'products.manage') - const { data: preset, isLoading, isError } = usePreset(name) + const { data: preset, isLoading, isError, error } = usePreset(name) // Default to the readme when there is one, else the template. Computed from // the loaded preset, so the hook runs unconditionally and re-derives once // the fetch resolves. @@ -41,7 +43,15 @@ export default function PresetDetail() { if (isLoading || !preset) { return ( -
      {isError ?

      Failed to load preset.

      :

      Loading…

      }
      +
      + {isError ? ( + + {error instanceof ApiError && error.detail ? error.detail : 'Failed to load preset.'} + + ) : ( +

      Loading…

      + )} +
      ) } From bfb0a7edb78d98fa63dc3923a75de453e1c0556d Mon Sep 17 00:00:00 2001 From: Robin Scher Date: Tue, 18 Aug 2026 18:28:45 -0700 Subject: [PATCH 19/20] fix(web): distinguish product and parameter load failures on the submit form --- web/src/pages/ProductSubmit.test.tsx | 53 +++++++++++++++++++++++++--- web/src/pages/ProductSubmit.tsx | 22 ++++++++++-- 2 files changed, 69 insertions(+), 6 deletions(-) diff --git a/web/src/pages/ProductSubmit.test.tsx b/web/src/pages/ProductSubmit.test.tsx index a65b224a..e08a1914 100644 --- a/web/src/pages/ProductSubmit.test.tsx +++ b/web/src/pages/ProductSubmit.test.tsx @@ -8,6 +8,7 @@ import type { ReactElement } from 'react' import { ToastProvider } from '@/components/Toast' import type { Principal } from '@/api/types' import ProductSubmit from './ProductSubmit' +import { ApiError } from '@/api/client' // ── Auth mock ───────────────────────────────────────────────────────────────── // ProductSubmit reads useAuth() to gate the Owner field behind 'jobs.submit_as'. @@ -95,7 +96,12 @@ const h = vi.hoisted(() => { internalParam, defaultParams, makeJob, - state: { params: defaultParams(), jobs: [makeJob()] }, + state: { + params: defaultParams(), + jobs: [makeJob()], + productError: null as unknown, + paramsError: null as unknown, + }, } }) @@ -118,12 +124,12 @@ vi.mock('@/api/queries', async (orig) => ({ format: 'yaml' as const, }, isLoading: false, - error: null, + error: h.state.productError, }), useProductParameters: () => ({ - data: h.state.params, + data: h.state.paramsError === null ? h.state.params : undefined, isLoading: false, - error: null, + error: h.state.paramsError, }), // Bug #2 corrected: use real FarmWithQueues shape { farm, queues } not { id, name, queues } useFarmsWithQueues: () => ({ @@ -166,6 +172,8 @@ vi.mock('@/api/mutations', async (orig) => ({ })) beforeEach(() => { + h.state.productError = null + h.state.paramsError = null submitMock.mockClear() navigateMock.mockClear() // Default every test to an operator principal (holds jobs.submit_as) so @@ -349,3 +357,40 @@ describe('owner field permission gating', () => { expect(arg).toMatchObject({ maySubmitAs: false }) }) }) + +describe('ProductSubmit load errors', () => { + function apiError(status: number, detail: string): ApiError { + return new ApiError({ type: 'about:blank', title: 'Error', status, detail }) + } + + it("shows the server's reason when the product cannot be loaded", () => { + h.state.productError = apiError(404, 'product not found') + renderPage() + expect(screen.getByRole('alert')).toHaveTextContent(/product not found/) + }) + + // The two requests fail for different reasons and send you to different + // places, so the message must say which one broke. + it('names the parameters request when that is what failed', () => { + h.state.paramsError = apiError(422, 'product template is invalid: yaml: line 3: bad indent') + renderPage() + const alert = screen.getByRole('alert') + expect(alert).toHaveTextContent(/parameters/i) + expect(alert).toHaveTextContent(/product template is invalid: yaml: line 3: bad indent/) + }) + + it('names the product request when that is what failed', () => { + h.state.productError = apiError(500, 'failed to get product') + renderPage() + expect(screen.getByRole('alert')).toHaveTextContent(/product/i) + expect(screen.getByRole('alert')).toHaveTextContent(/failed to get product/) + }) + + it('falls back to a generic message when the failure carries no detail', () => { + h.state.productError = new Error('network down') + renderPage() + const alert = screen.getByRole('alert') + expect(alert).toHaveTextContent(/failed to load/i) + expect(alert).not.toHaveTextContent(/undefined/) + }) +}) diff --git a/web/src/pages/ProductSubmit.tsx b/web/src/pages/ProductSubmit.tsx index d8350cb7..01d96a58 100644 --- a/web/src/pages/ProductSubmit.tsx +++ b/web/src/pages/ProductSubmit.tsx @@ -19,6 +19,13 @@ import styles from './ProductSubmit.module.css' const QUEUE_STORAGE_KEY = 'sqi:submit:last-queue-id' +/** Message for a failed load: the server's explanation when it sent one, + * otherwise the caller's generic line. A transport failure carries no problem + * document, so it must not render as "undefined". */ +function loadFailureMessage(generic: string, err: unknown): string { + return err instanceof ApiError && err.detail ? `${generic} ${err.detail}` : generic +} + export default function ProductSubmit() { const { name = '' } = useParams() const navigate = useNavigate() @@ -78,8 +85,19 @@ export default function ProductSubmit() { const productData = product.data const paramList = params.data - if (product.error || params.error || !productData || !paramList) { - return

      Failed to load this product.

      + if (product.error || !productData) { + return ( +

      + {loadFailureMessage('Failed to load this product.', product.error)} +

      + ) + } + if (params.error || !paramList) { + return ( +

      + {loadFailureMessage("Failed to load this product's parameters.", params.error)} +

      + ) } // Fall back to the first available queue when none is stored (mirrors Submit.tsx). From 448804b73319f733812150efa2a638e3084a2db6 Mon Sep 17 00:00:00 2001 From: Robin Scher Date: Tue, 18 Aug 2026 19:37:39 -0700 Subject: [PATCH 20/20] ci: validate the published preset library against the current validator --- .github/workflows/preset-library.yml | 81 ++++++++++++++++++++ .golangci.yml | 1 + Makefile | 17 +++++ docs/development.md | 1 + test/presetlib/published_test.go | 110 +++++++++++++++++++++++++++ 5 files changed, 210 insertions(+) create mode 100644 .github/workflows/preset-library.yml create mode 100644 test/presetlib/published_test.go diff --git a/.github/workflows/preset-library.yml b/.github/workflows/preset-library.yml new file mode 100644 index 00000000..322c6b5e --- /dev/null +++ b/.github/workflows/preset-library.yml @@ -0,0 +1,81 @@ +# SPDX-FileCopyrightText: 2026 Uberware Inc. +# SPDX-License-Identifier: AGPL-3.0-or-later + +name: Preset library + +# Validates the PUBLISHED preset library against the validator on this branch. +# +# Why this exists: a validator change can silently invalidate content that is +# already published. Commit 2cdef4f tightened parameter-control validation to +# match the base spec and corrected every preset in this repo in the same +# change -- but the copy at uberware.github.io/sqi-presets is only regenerated +# by the release workflow, so from that commit until the next release every +# published preset failed to load. Nothing reported it: the list page renders +# from the index, which needs no validation, and only the detail page parses. +# The first signal was a user clicking a preset and getting an error. +# +# Deliberately NOT part of ci.yml. It reaches the network, so putting it on +# every pull request would make unrelated work hostage to an outage. Instead it +# runs where it is actually informative: +# +# - on a schedule, so drift is found within a day rather than by a user; +# - when the presets or the validator change, which is exactly when +# already-published content can become invalid; +# - on demand. +on: + schedule: + # 07:00 UTC daily. Drift here is never urgent-to-the-minute; it just must + # not wait for someone to click a preset. + - cron: "0 7 * * *" + push: + branches: [main] + paths: + - "presets/**" + - "internal/openjd/**" + - "internal/product/**" + - "internal/presetlib/**" + - "test/presetlib/**" + - ".github/workflows/preset-library.yml" + pull_request: + paths: + - "presets/**" + - "internal/openjd/**" + - "internal/product/**" + - "internal/presetlib/**" + - "test/presetlib/**" + - ".github/workflows/preset-library.yml" + workflow_dispatch: + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +permissions: + contents: read + +jobs: + published-presets: + name: Published presets validate against this tree + runs-on: ubuntu-latest + + steps: + - name: Checkout + uses: actions/checkout@v7 + + - name: Set up Go + uses: actions/setup-go@v7 + with: + go-version-file: go.mod + + - name: Validate the published preset library + shell: bash + run: make test-preset-library 2>&1 | tee /tmp/preset-library.log + + # The target exits 0 when the library is unreachable, because an offline + # runner is not evidence that the content is bad. That means a green step + # alone proves nothing -- assert the test actually ran and passed. A SKIP + # fails here on purpose: it means this check verified nothing, and the + # cause (network, or a repointed URL) needs a human. + - name: Assert the check actually ran + run: | + grep -q -- '--- PASS: TestPublishedPresets_ValidateAgainstThisTree' /tmp/preset-library.log diff --git a/.golangci.yml b/.golangci.yml index 61185316..f8630676 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -21,6 +21,7 @@ run: - integration - conformance - oracle + - presetlib # ── Linters ─────────────────────────────────────────────────────────────────── linters: diff --git a/Makefile b/Makefile index fd6138b7..8d5495d8 100644 --- a/Makefile +++ b/Makefile @@ -239,6 +239,23 @@ test-expr-oracle: ## Differential-test the EXPR evaluator against the OpenJD ref fi go test $(TEST_FLAGS) -tags oracle -run 'TestExprOracle' -v -timeout 5m ./test/oracle/ +# Validates the PUBLISHED preset library against the validator in this working +# tree. It exists because a validator change can silently invalidate content +# already published: 2cdef4f tightened parameter-control validation and fixed +# every preset in this repo, but the copy at uberware.github.io/sqi-presets is +# only refreshed on release, so every preset there failed to load in between -- +# with no signal until a user clicked one. +# +# Needs the network. SKIPS when the library is unreachable and FAILS when it is +# reachable but invalid, so an offline runner never masks a real breakage. A +# SKIP VERIFIES NOTHING -- look for the "--- PASS: TestPublishedPresets" line. +# CI asserts it by name for that reason. +# +# SQI_TEST_PRESET_LIBRARY_URL points it at a staging index instead. +.PHONY: test-preset-library +test-preset-library: ## Validate the published preset library against this tree (needs network) + go test $(TEST_FLAGS) -tags presetlib -run 'TestPublishedPresets' -v -timeout 5m ./test/presetlib/ + .PHONY: test-ldap test-ldap: ## Run the LDAP tests against a real directory in a container (needs Docker) go test $(TEST_FLAGS) -tags integration -run 'TestLDAP_' -v -timeout 15m ./test/integration/ diff --git a/docs/development.md b/docs/development.md index 38cade52..c6e908bc 100644 --- a/docs/development.md +++ b/docs/development.md @@ -69,6 +69,7 @@ Run `make` (no arguments) to see all available targets with descriptions. | `make test-oidc` | Run the SSO tests against a real Keycloak in a container (needs Docker; **skips** without it) | | `make test-isolation` | Run run-as-user task-isolation tests as real root against real OS accounts in a container (needs Docker; **skips** without it) | | `make test-expr-oracle` | Differential-test the EXPR evaluator against the OpenJD reference implementation (needs `python3`; **skips** without it) | +| `make test-preset-library` | Validate the **published** preset library against the validator in your tree (needs network; **skips** when the library is unreachable, **fails** when it is reachable but invalid) | | `make expr-oracle-venv` | Create `.venv-oracle/` with the pinned reference implementation (`make test-expr-oracle` does this on demand) | | `make smoke` | End-to-end smoke test against the real binaries (REST + WebSocket) | | `make bench` | Run benchmarks | diff --git a/test/presetlib/published_test.go b/test/presetlib/published_test.go new file mode 100644 index 00000000..eb82dfdb --- /dev/null +++ b/test/presetlib/published_test.go @@ -0,0 +1,110 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +//go:build presetlib + +// Package presetlib_test validates the PUBLISHED preset library against the +// validator in this working tree. +// +// It exists because of a real, silent breakage. Commit 2cdef4f ("improved +// openJD conformance") tightened parameter-control validation to match the base +// spec -- a PATH parameter may not use LINE_EDIT -- and corrected every preset +// in this repo in the same change. What it could not correct was the copy +// already published at uberware.github.io/sqi-presets, which is only refreshed +// on release. From that commit until the next release, every preset in the +// library failed to load, and nothing anywhere reported it: the list page +// renders from the index (which needs no validation) and only the detail page +// parses, so the first signal was a user clicking a preset and getting an +// error. +// +// This test closes that gap by running the published bytes through the same +// path the server uses -- presetlib.FetchDefinition, which pins the sha256 and +// then calls product.ParseDefinition -- so a validator change that invalidates +// published content fails here rather than in someone's browser. +// +// Build tag `presetlib` keeps it out of `make ci`: it needs the network, and a +// unit-test suite that reaches the internet is a flake generator. Run it with +// `make test-preset-library`. +package presetlib_test + +import ( + "context" + "errors" + "net" + "os" + "testing" + "time" + + "github.com/uberware/sqi/internal/presetlib" + "github.com/uberware/sqi/internal/product" +) + +// defaultIndexURL mirrors config.Defaults()' preset_library.url. Duplicated +// rather than imported so this test validates what operators actually get by +// default, and fails loudly if that default is ever repointed without thought. +const defaultIndexURL = "https://uberware.github.io/sqi-presets/index.json" + +// indexURL is the library under test. SQI_TEST_PRESET_LIBRARY_URL points it at +// a staging index or a local file server. +func indexURL() string { + if u := os.Getenv("SQI_TEST_PRESET_LIBRARY_URL"); u != "" { + return u + } + return defaultIndexURL +} + +// unreachable reports whether err is a transport failure rather than a verdict +// about the content. An offline runner must SKIP; invalid content must FAIL. +// Collapsing the two would let a real breakage hide behind a network blip. +func unreachable(err error) bool { + var dnsErr *net.DNSError + var opErr *net.OpError + return errors.As(err, &dnsErr) || errors.As(err, &opErr) || errors.Is(err, context.DeadlineExceeded) +} + +// validateOptions mirrors what the preset routes pass in production: limits +// enforced, EXPR budget left at its defaults (this test has no operator +// configuration to offer), and a generous deadline since we are validating a +// whole library rather than serving one request. +func validateOptions() product.ValidateOptions { + return product.ValidateOptions{EnforceLimits: true} +} + +func TestPublishedPresets_ValidateAgainstThisTree(t *testing.T) { + url := indexURL() + svc := presetlib.New(url, time.Minute) + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() + + entries, err := svc.FetchIndex(ctx, true) + if err != nil { + if unreachable(err) { + t.Skipf("preset library %s unreachable, skipping: %v", url, err) + } + t.Fatalf("fetch index %s: %v", url, err) + } + if len(entries) == 0 { + t.Fatalf("preset library %s lists no presets", url) + } + t.Logf("validating %d published presets from %s", len(entries), url) + + for _, entry := range entries { + t.Run(entry.Name, func(t *testing.T) { + // FetchDefinition is the production path: it verifies the index's + // sha256 before parsing, so this also catches a definition that + // drifted from the fingerprint the index vouches for. + if _, err := svc.FetchDefinition(ctx, entry, validateOptions()); err != nil { + if unreachable(err) { + t.Skipf("definition %s unreachable: %v", entry.Definition, err) + } + t.Errorf("published preset %q no longer validates against this tree.\n"+ + " definition: %s\n"+ + " error: %v\n"+ + "This means the published library is stale relative to the validator in\n"+ + "this working tree. Publish the corrected presets (the release workflow\n"+ + "regenerates the library from presets/), or, if the validator change was\n"+ + "unintended, revert it.", entry.Name, entry.Definition, err) + } + }) + } +}