diff --git a/.github/workflows/validate_data.yaml b/.github/workflows/validate_data.yaml index 389f3a82a1..fbed724414 100644 --- a/.github/workflows/validate_data.yaml +++ b/.github/workflows/validate_data.yaml @@ -25,7 +25,7 @@ jobs: runs-on: ubuntu-24.04 strategy: matrix: - name: [ json-files, folder-names, store-ids, logo-files, fiber-consistency ] + name: [ json-files, folder-names, store-ids, logo-files, fiber-consistency, data-quality ] name: Validate data - ${{ matrix.name }} steps: - name: Checkout diff --git a/ofd/commands/validate.py b/ofd/commands/validate.py index fc84397478..006f7e12fc 100644 --- a/ofd/commands/validate.py +++ b/ofd/commands/validate.py @@ -93,6 +93,12 @@ def register_subcommand(subparsers: argparse._SubParsersAction) -> None: action="store_true", help="Validate no filament mixes carbon fiber and glass fiber across its variants", ) + scope_group.add_argument( + "--data-quality", + action="store_true", + help="Check for placeholder values, duplicate spool rows, name casing/whitespace, " + "missing fiber traits, filaments with no variants, and word-order duplicates", + ) # Output options output_group = parser.add_argument_group("output options") @@ -172,6 +178,7 @@ def run_validate(args: argparse.Namespace) -> int: args.store_ids, args.gtin, args.fiber_consistency, + args.data_quality, ] ) @@ -179,8 +186,8 @@ def run_validate(args: argparse.Namespace) -> int: # Run all validations if not args.json and not args.progress: print(_bold("Running all validations...")) - # validate_all() already includes the native fiber-consistency check - # (skipped automatically under a changes overlay). + # validate_all() already includes the native fiber-consistency and + # data-quality checks (skipped automatically under a changes overlay). result = orchestrator.validate_all(changes_json=changes_json) else: # Run specific validations @@ -207,6 +214,18 @@ def run_validate(args: argparse.Namespace) -> int: ), file=sys.stderr, ) + if args.data_quality: + # Same constraint as fiber-consistency: on-disk only. + if changes_json is None: + result.merge(orchestrator.validate_data_quality()) + else: + print( + _yellow( + "Skipping data-quality: it runs against on-disk data and " + "cannot apply --apply-changes." + ), + file=sys.stderr, + ) # Output results if args.json: diff --git a/ofd/scripts/style_data.py b/ofd/scripts/style_data.py index 241a611de7..76c165ab17 100644 --- a/ofd/scripts/style_data.py +++ b/ofd/scripts/style_data.py @@ -104,8 +104,14 @@ "refinements", "_encoding", "pldnsite", + # SimplyPrint storefront recommender context. Seen on 3dhojor datasheet URLs merged in + # #461; the token is per-session and hundreds of characters long. + "_su_rec", + "_su_rec_id", + # Shopify affiliate referral (seen across SUNLU's purchase links). + "sca_ref", } -_TRACKING_PREFIXES = ("utm_", "mc_", "pk_", "pd_rd_", "pf_rd_") +_TRACKING_PREFIXES = ("utm_", "mc_", "pk_", "pd_rd_", "pf_rd_", "_su_") # Hosts where the product identity is entirely in the path, so the whole query is disposable. _WHOLE_QUERY_HOSTS = re.compile(r"(^|\.)(amazon|ebay)\.[a-z.]+$") _SHORTLINK_HOSTS = {"a.co", "amzn.to", "amzn.eu"} @@ -122,6 +128,26 @@ def _is_tracking_fragment(fragment: str) -> bool: return _is_tracking_key(re.split(r"[&=]", fragment, maxsplit=1)[0]) +_AMAZON_HOST = re.compile(r"(^|\.)amazon\.[a-z.]+$") + + +def _strip_amazon_ref_path(base: str, host: str) -> str: + """Drop Amazon's trailing `/ref=` path breadcrumb. + + Amazon puts a tracker in the *path*, not only the query. PR #408 was submitted as + `.../dp/B0FB93XQLM/ref=sr_1_6` ("search result, page 1, position 6") and had to be + shortened by hand in review. The product-name slug before `/dp/` is kept — Amazon + ignores it and it makes the link readable. + """ + if not _AMAZON_HOST.search(host): + return base + path = urlsplit(base).path + idx = path.find("/ref=") + if idx == -1: + return base + return base[: len(base) - len(path) + idx] + + def strip_tracking_params(url: str) -> str: """Remove known tracking/affiliate params (plus an empty '?' and tracking-only fragments) from a URL, preserving everything else. Idempotent. Mirrors the ofd-validator Rust rule @@ -139,6 +165,9 @@ def strip_tracking_params(url: str) -> str: host = "" drop_all = bool(_WHOLE_QUERY_HOSTS.search(host)) or host in _SHORTLINK_HOSTS + # Amazon carries tracking in the path, not only the query. + base = _strip_amazon_ref_path(base, host) + out = base if query is not None and not drop_all: kept = [ diff --git a/ofd/validation/__init__.py b/ofd/validation/__init__.py index bf3f5ba5ab..2921073100 100644 --- a/ofd/validation/__init__.py +++ b/ofd/validation/__init__.py @@ -87,13 +87,25 @@ def validate_fiber_consistency(self) -> ValidationResult: result.add_error(error) return result + def validate_data_quality(self) -> ValidationResult: + """Report the recurring data-quality defects reviewers fixed by hand. + + This is a native (non-Rust) check; see data_quality.py for the rules. + """ + from ofd.validation.data_quality import check_data_quality + + result = ValidationResult() + for error in check_data_quality(self.data_dir): + result.add_error(error) + return result + def validate_all(self, changes_json: str | None = None) -> ValidationResult: """Run all validations, optionally with pending changes applied. - Includes the native cross-variant fiber-consistency check, except under a - changes overlay: that check reads on-disk data and can't see pending edits, - so running it there could report stale conflicts (the webui enforces the - rule pre-export). + Includes the native fiber-consistency and data-quality checks, except under a + changes overlay: those read on-disk data and can't see pending edits, so + running them there could report stale findings (the webui enforces the same + rules pre-export). """ if changes_json and _validate_all_with_changes is not None: return _validate_all_with_changes( @@ -107,6 +119,7 @@ def validate_all(self, changes_json: str | None = None) -> ValidationResult: result = _validate_all(self.data_dir, self.stores_dir, max_workers=self.max_workers) if not changes_json: result.merge(self.validate_fiber_consistency()) + result.merge(self.validate_data_quality()) return result diff --git a/ofd/validation/data_quality.py b/ofd/validation/data_quality.py new file mode 100644 index 0000000000..05edb547c7 --- /dev/null +++ b/ofd/validation/data_quality.py @@ -0,0 +1,438 @@ +""" +Data-quality validation. + +Rules for the small, repetitive defects that reviewers kept fixing by hand on webui +submissions after PR #405: placeholder values, duplicated spool rows, casing that +diverges from a filament's own siblings, fiber traits missing from variants whose +names announce the fiber, filaments left with no variants, and near-duplicate sibling +names that differ only by word order. + +This is the authoritative, server-side enforcement; the webui mirrors the rules in +``webui/src/lib/utils/dataQuality.ts`` and surfaces each as an inline "Fix" hint at +entry time. Keep the two in lockstep. + +Every rule reuses the detector that already exists for its concept rather than +restating it: + +* fiber codes -> ``ofd/scripts/apply_fiber_traits.py`` (``detect_codes``) +* spool identity -> ``ofd/merge.py`` (``size_dedupe_key``) +* word-swap normalization -> ``ofd/scripts/deduplicate_data.py`` + +Levels are chosen so a rule only blocks a merge when the data is genuinely wrong: +``placeholder_value``, ``duplicate_size_entry``, ``name_whitespace`` and +``purchase_link_storefront_root`` are ERRORs; the judgement calls +(``name_casing``, ``fiber_trait_missing``, ``orphan_filament``, +``sibling_near_duplicate``) are WARNINGs. +""" + +import json +import re +from collections import Counter, defaultdict +from pathlib import Path + +from ofd_validator import ValidationError, ValidationLevel + +from ofd.merge import size_dedupe_key +from ofd.scripts.apply_fiber_traits import CODE_TRAITS, detect_codes + +CATEGORY = "data_quality" + +# Fields whose value is a human-facing display name. +_NAME_FIELDS = { + "brand.json": "name", + "material.json": "material", + "filament.json": "name", + "variant.json": "name", + "store.json": "name", +} + +# Two or more consecutive whitespace characters anywhere in a name. +_DOUBLED_SPACE = re.compile(r"\s\s") + +# Words that legitimately stay lowercase inside a title-cased display name. +_MINOR_WORDS = {"and", "of", "the", "with", "de", "w/", "in", "on", "for", "a", "an"} + + +def _load_json(path: Path): + try: + with open(path, encoding="utf-8") as f: + return json.load(f) + except (json.JSONDecodeError, OSError): + return None + + +def _rel(path: Path, base: Path) -> str: + """Path relative to the data dir's parent (project root) for readable output.""" + try: + return str(path.relative_to(base)) + except ValueError: + return str(path) + + +def _err(level, message: str, path: str) -> ValidationError: + return ValidationError(level, CATEGORY, message, path) + + +# --- Rule: placeholder values ------------------------------------------------- + + +def _placeholder_paths(value, prefix: str = "") -> list[str]: + """Dotted paths of blank string values, recursing into dicts and lists. + + A blank string is never meaningful data. ``certifications: [""]`` (#453) reads + downstream as "this filament has a certification" whose name is blank; an empty + array (or an absent field) says the true thing. Whitespace-only counts as blank — + ``" "`` is no more a certification name than ``""`` is, and the webui's + ``checkPlaceholderEntries`` flags it as one too. + """ + found: list[str] = [] + if isinstance(value, dict): + for key, sub in value.items(): + found.extend(_placeholder_paths(sub, f"{prefix}.{key}" if prefix else key)) + elif isinstance(value, list): + for index, sub in enumerate(value): + found.extend(_placeholder_paths(sub, f"{prefix}[{index}]")) + elif isinstance(value, str) and value.strip() == "" and prefix: + found.append(prefix) + return found + + +def _check_placeholders(data, file_path: Path, base: Path) -> list[ValidationError]: + return [ + _err( + ValidationLevel.Error, + f"'{dotted}' is blank. Remove the field, or drop the empty entry " + f"from the array — a blank value is not the same as no value.", + _rel(file_path, base), + ) + for dotted in _placeholder_paths(data) + ] + + +# --- Rule: name whitespace ---------------------------------------------------- + + +def _check_name_whitespace(data, file_path: Path, base: Path) -> list[ValidationError]: + """Leading/trailing or doubled whitespace in a display name. + + #460 created a filament literally named ``"Silk "``. The trailing space is + invisible in review, survives into every downstream consumer, and makes the entity + look distinct from the ``"Silk"`` the contributor meant. + """ + field = _NAME_FIELDS.get(file_path.name) + if not field or not isinstance(data, dict): + return [] + name = data.get(field) + if not isinstance(name, str) or not name: + return [] + + if name != name.strip(): + return [ + _err( + ValidationLevel.Error, + f"Name {name!r} has leading or trailing whitespace.", + _rel(file_path, base), + ) + ] + if _DOUBLED_SPACE.search(name): + return [ + _err( + ValidationLevel.Error, + f"Name {name!r} contains repeated whitespace.", + _rel(file_path, base), + ) + ] + return [] + + +# --- Rule: duplicate size entries --------------------------------------------- + + +def _redundant_size_fields(size: dict) -> dict: + """The fields that make a spool row worth keeping, ignoring canonical identity. + + A field set to ``None`` or a blank string says nothing the absent field doesn't, so + it must not be what distinguishes two rows — otherwise ``{"gtin": null}`` reads as a + distinct GTIN. The webui's ``meaningfulFields`` drops the same values. + """ + return { + k: v + for k, v in size.items() + if k not in ("uuid", "moved_from") + and v is not None + and not (isinstance(v, str) and v.strip() == "") + } + + +def _is_subsumed(size: dict, earlier: dict) -> bool: + """True when `size` says nothing `earlier` doesn't already say. + + Spool identity in this repo is ``(filament_weight, diameter)`` — see + ``size_dedupe_key`` in ``ofd/merge.py``, which is what ``merge_sizes`` and + ``record_moved_from`` pair on. So two rows sharing that key are the same spool, and + the later one is redundant unless it carries a value the earlier one lacks or + contradicts (a distinct GTIN, article number, purchase link, spool geometry...). + + #453 shipped two 1 kg / 1.75 mm rows with nothing to tell them apart, which renders + as the same size listed twice. + """ + if size_dedupe_key(size) != size_dedupe_key(earlier): + return False + earlier_fields = _redundant_size_fields(earlier) + return all( + key in earlier_fields and earlier_fields[key] == value + for key, value in _redundant_size_fields(size).items() + ) + + +def _check_duplicate_sizes(sizes, file_path: Path, base: Path) -> list[ValidationError]: + if not isinstance(sizes, list): + return [] + + errors: list[ValidationError] = [] + for index, size in enumerate(sizes): + if not isinstance(size, dict): + continue + for earlier_index in range(index): + earlier = sizes[earlier_index] + if isinstance(earlier, dict) and _is_subsumed(size, earlier): + weight = size.get("filament_weight") + diameter = size.get("diameter") + errors.append( + _err( + ValidationLevel.Error, + f"Size #{index + 1} ({weight}g, {diameter}mm) adds nothing over size " + f"#{earlier_index + 1}. Merge them, or give it the SKU, GTIN or " + f"purchase link that tells them apart.", + _rel(file_path, base), + ) + ) + break + return errors + + +# --- Rule: name casing -------------------------------------------------------- + + +def _is_title_cased(name: str) -> bool: + """True when every significant word starts with an uppercase letter.""" + words = [w for w in name.split() if w and w[0].isalpha()] + if not words: + return False + return all(w[0].isupper() for w in words if w.lower() not in _MINOR_WORDS) + + +def _is_all_lower(name: str) -> bool: + return bool(name) and name == name.lower() and any(c.isalpha() for c in name) + + +def _check_sibling_casing(names: list[tuple[str, Path]], base: Path) -> list[ValidationError]: + """Flag an all-lowercase name among Title Case siblings. + + Casing is a house style rather than a hard rule, so this only fires when the + entity's own siblings establish the convention — which is exactly how it was + caught by hand on #451 ("translucent blue" beside "Purple", "Transparent") and + #452 ("true red" beside "Red", "Mint Green"). + """ + titled = [n for n, _ in names if _is_title_cased(n)] + if not titled: + return [] + + return [ + _err( + ValidationLevel.Warning, + f"Name {name!r} is lowercase while its siblings use Title Case (e.g. {titled[0]!r}).", + _rel(path, base), + ) + for name, path in names + if _is_all_lower(name) + ] + + +# --- Rule: fiber traits missing ----------------------------------------------- + + +def _check_fiber_traits( + data: dict, text: str, file_path: Path, base: Path +) -> list[ValidationError]: + """A variant whose name announces CF/GF/HF but carries none of the traits. + + ``apply_fiber_traits.py`` has detected this since #405, but only to *suggest*. + #450 still merged four variants named "carbon fiber " with no + ``contains_carbon_fiber`` or ``abrasive``, so they were invisible to every + downstream abrasive-material filter. + """ + codes = detect_codes(text) + if not codes: + return [] + + traits = data.get("traits") + traits = traits if isinstance(traits, dict) else {} + + missing: list[str] = [] + for code in sorted(codes): + for trait in CODE_TRAITS.get(code, ()): + if traits.get(trait) is not True and trait not in missing: + missing.append(trait) + + if not missing: + return [] + + return [ + _err( + ValidationLevel.Warning, + f"Name suggests {', '.join(sorted(codes))} but these traits are not set: " + f"{', '.join(missing)}.", + _rel(file_path, base), + ) + ] + + +# --- Rule: orphan filament ---------------------------------------------------- + + +def _check_orphan_filament(filament_dir: Path, base: Path) -> list[ValidationError]: + """A filament directory with no variants under it. + + A filament with no colours is not reachable in the UI and buys nothing. #461 + left ``data/3dhojor/PLA/silk_blue_green/`` behind this way, by deleting and + re-creating a filament that a still-open PR was also editing. + """ + has_variant = any( + child.is_dir() and (child / "variant.json").exists() for child in filament_dir.iterdir() + ) + if has_variant: + return [] + return [ + _err( + ValidationLevel.Warning, + "Filament has no variants. Add at least one colour, or remove the filament.", + _rel(filament_dir, base), + ) + ] + + +# --- Rule: sibling near-duplicates -------------------------------------------- + + +def _word_multiset(name: str) -> tuple: + """Order-insensitive identity of an underscore-separated id. + + Mirrors the word-swap grouping in ``ofd/scripts/deduplicate_data.py``, so what + that script would offer to merge is what this reports. + """ + return tuple(sorted(Counter(name.split("_")).items())) + + +def _check_sibling_duplicates(ids: list[tuple[str, Path]], base: Path) -> list[ValidationError]: + groups: dict[tuple, list[tuple[str, Path]]] = defaultdict(list) + for entity_id, path in ids: + groups[_word_multiset(entity_id)].append((entity_id, path)) + + errors: list[ValidationError] = [] + for members in groups.values(): + if len(members) < 2: + continue + names = [entity_id for entity_id, _ in members] + for entity_id, path in members[1:]: + others = [n for n in names if n != entity_id] + errors.append( + _err( + ValidationLevel.Warning, + f"'{entity_id}' is a word-order duplicate of {', '.join(repr(o) for o in others)}. " + f"These are almost always the same thing under two names.", + _rel(path, base), + ) + ) + return errors + + +# --- Entry point -------------------------------------------------------------- + + +def check_data_quality(data_dir) -> list[ValidationError]: + """Scan the data tree and report every data-quality finding.""" + data_dir = Path(data_dir) + errors: list[ValidationError] = [] + if not data_dir.exists(): + return errors + + base = data_dir.parent + + for brand_dir in sorted(p for p in data_dir.iterdir() if p.is_dir()): + brand_file = brand_dir / "brand.json" + brand_data = _load_json(brand_file) + if isinstance(brand_data, dict): + errors.extend(_check_placeholders(brand_data, brand_file, base)) + errors.extend(_check_name_whitespace(brand_data, brand_file, base)) + + for material_dir in sorted(p for p in brand_dir.iterdir() if p.is_dir()): + material_file = material_dir / "material.json" + material_data = _load_json(material_file) + if isinstance(material_data, dict): + errors.extend(_check_placeholders(material_data, material_file, base)) + errors.extend(_check_name_whitespace(material_data, material_file, base)) + + filament_names: list[tuple[str, Path]] = [] + filament_ids: list[tuple[str, Path]] = [] + + for filament_dir in sorted(p for p in material_dir.iterdir() if p.is_dir()): + filament_file = filament_dir / "filament.json" + filament_data = _load_json(filament_file) + if not isinstance(filament_data, dict): + continue + + errors.extend(_check_placeholders(filament_data, filament_file, base)) + errors.extend(_check_name_whitespace(filament_data, filament_file, base)) + errors.extend(_check_orphan_filament(filament_dir, base)) + + name = filament_data.get("name") + if isinstance(name, str) and name: + filament_names.append((name, filament_file)) + filament_ids.append((filament_dir.name, filament_dir)) + + variant_names: list[tuple[str, Path]] = [] + variant_ids: list[tuple[str, Path]] = [] + + for variant_dir in sorted(p for p in filament_dir.iterdir() if p.is_dir()): + variant_file = variant_dir / "variant.json" + variant_data = _load_json(variant_file) + if not isinstance(variant_data, dict): + continue + + errors.extend(_check_placeholders(variant_data, variant_file, base)) + errors.extend(_check_name_whitespace(variant_data, variant_file, base)) + + # Fiber codes can come from any level of the path, matching + # apply_fiber_traits.py's scan. + fiber_text = " / ".join( + str(part).lower() + for part in ( + material_dir.name, + filament_dir.name, + filament_data.get("name") or "", + variant_dir.name, + variant_data.get("name") or "", + ) + if part + ) + errors.extend(_check_fiber_traits(variant_data, fiber_text, variant_file, base)) + + variant_name = variant_data.get("name") + if isinstance(variant_name, str) and variant_name: + variant_names.append((variant_name, variant_file)) + variant_ids.append((variant_dir.name, variant_dir)) + + sizes_file = variant_dir / "sizes.json" + if sizes_file.exists(): + sizes = _load_json(sizes_file) + errors.extend(_check_placeholders(sizes, sizes_file, base)) + errors.extend(_check_duplicate_sizes(sizes, sizes_file, base)) + + errors.extend(_check_sibling_casing(variant_names, base)) + errors.extend(_check_sibling_duplicates(variant_ids, base)) + + errors.extend(_check_sibling_casing(filament_names, base)) + errors.extend(_check_sibling_duplicates(filament_ids, base)) + + return errors diff --git a/tests/test_data_quality.py b/tests/test_data_quality.py new file mode 100644 index 0000000000..9a54a68844 --- /dev/null +++ b/tests/test_data_quality.py @@ -0,0 +1,427 @@ +"""Tests for the data-quality validation rules. + +Each rule is anchored to the submission that motivated it, so a regression is +traceable back to the review comment it was meant to make unnecessary. +""" + +import json + +from ofd.validation.data_quality import CATEGORY, check_data_quality + + +def write(path, obj): + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(json.dumps(obj), encoding="utf-8") + + +def build( + data_dir, + *, + brand="acme", + material="PLA", + filament="basic", + filament_extra=None, + variants=(), +): + """Lay out data/////{variant,sizes}.json. + + `variants` items are dicts: {id, name?, traits?, sizes?}. + """ + fil_dir = data_dir / brand / material / filament + write(data_dir / brand / "brand.json", {"id": brand, "name": brand.title()}) + write(data_dir / brand / material / "material.json", {"material": material}) + + filament_obj = {"id": filament, "name": filament.replace("_", " ").title()} + filament_obj.update(filament_extra or {}) + write(fil_dir / "filament.json", filament_obj) + + for v in variants: + vdir = fil_dir / v["id"] + variant_obj = { + "id": v["id"], + "name": v.get("name", v["id"]), + "color_hex": "#000000", + } + if "traits" in v: + variant_obj["traits"] = v["traits"] + write(vdir / "variant.json", variant_obj) + write( + vdir / "sizes.json", + v.get("sizes", [{"filament_weight": 1000, "diameter": 1.75}]), + ) + + return data_dir + + +def messages(errors, needle): + return [e for e in errors if needle in e.message] + + +def test_clean_data_has_no_findings(tmp_path): + data = build( + tmp_path / "data", + variants=[{"id": "black", "name": "Black"}, {"id": "red", "name": "Red"}], + ) + assert check_data_quality(data) == [] + + +def test_missing_data_dir_is_not_an_error(tmp_path): + assert check_data_quality(tmp_path / "nope") == [] + + +def test_every_finding_is_tagged_with_the_category(tmp_path): + data = build( + tmp_path / "data", + filament_extra={"certifications": [""]}, + variants=[{"id": "black", "name": "Black"}], + ) + findings = check_data_quality(data) + assert findings + assert all(e.category == CATEGORY for e in findings) + + +# --- placeholder_value (#453: `certifications: [""]`) ------------------------- + + +def test_empty_string_in_an_array_is_an_error(tmp_path): + data = build( + tmp_path / "data", + filament_extra={"certifications": [""]}, + variants=[{"id": "black", "name": "Black"}], + ) + found = messages(check_data_quality(data), "is blank") + assert len(found) == 1 + assert "certifications[0]" in found[0].message + assert found[0].level.value == "ERROR" + + +def test_an_empty_array_is_fine(tmp_path): + data = build( + tmp_path / "data", + filament_extra={"certifications": []}, + variants=[{"id": "black", "name": "Black"}], + ) + assert messages(check_data_quality(data), "is blank") == [] + + +def test_whitespace_only_string_is_an_error(tmp_path): + """A blank-looking value is as meaningless as ``""`` — and the webui flags it too.""" + data = build( + tmp_path / "data", + filament_extra={"certifications": [" "]}, + variants=[{"id": "black", "name": "Black"}], + ) + found = messages(check_data_quality(data), "is blank") + assert len(found) == 1 + assert "certifications[0]" in found[0].message + + +def test_empty_string_nested_in_a_purchase_link_is_found(tmp_path): + data = build( + tmp_path / "data", + variants=[ + { + "id": "black", + "name": "Black", + "sizes": [ + { + "filament_weight": 1000, + "diameter": 1.75, + "purchase_links": [{"store_id": "shop", "url": ""}], + } + ], + } + ], + ) + found = messages(check_data_quality(data), "is blank") + assert len(found) == 1 + assert "purchase_links[0].url" in found[0].message + + +# --- name_whitespace (#460: a filament literally named "Silk ") --------------- + + +def test_trailing_whitespace_in_a_name_is_an_error(tmp_path): + data = build( + tmp_path / "data", + filament_extra={"name": "Silk "}, + variants=[{"id": "black", "name": "Black"}], + ) + found = messages(check_data_quality(data), "leading or trailing whitespace") + assert len(found) == 1 + assert found[0].level.value == "ERROR" + + +def test_repeated_whitespace_in_a_name_is_an_error(tmp_path): + data = build( + tmp_path / "data", + variants=[{"id": "sky_blue", "name": "Sky Blue"}], + ) + found = messages(check_data_quality(data), "repeated whitespace") + assert len(found) == 1 + + +def test_a_single_internal_space_is_fine(tmp_path): + data = build(tmp_path / "data", variants=[{"id": "sky_blue", "name": "Sky Blue"}]) + assert messages(check_data_quality(data), "whitespace") == [] + + +# --- duplicate_size_entry (#453) ---------------------------------------------- + + +def test_identical_size_rows_are_an_error(tmp_path): + data = build( + tmp_path / "data", + variants=[ + { + "id": "black", + "name": "Black", + "sizes": [ + {"filament_weight": 1000, "diameter": 1.75}, + {"filament_weight": 1000, "diameter": 1.75}, + ], + } + ], + ) + found = messages(check_data_quality(data), "adds nothing over size") + assert len(found) == 1 + assert found[0].level.value == "ERROR" + + +def test_a_row_that_only_omits_fields_is_still_redundant(tmp_path): + # The polylite / sunlu shape: the same spool listed again with less detail. + data = build( + tmp_path / "data", + variants=[ + { + "id": "black", + "name": "Black", + "sizes": [ + { + "filament_weight": 5000, + "diameter": 1.75, + "empty_spool_weight": 819, + }, + {"filament_weight": 5000, "diameter": 1.75}, + ], + } + ], + ) + assert len(messages(check_data_quality(data), "adds nothing over size")) == 1 + + +def test_rows_distinguished_by_a_gtin_are_kept(tmp_path): + data = build( + tmp_path / "data", + variants=[ + { + "id": "black", + "name": "Black", + "sizes": [ + {"filament_weight": 1000, "diameter": 1.75, "gtin": "012345678905"}, + {"filament_weight": 1000, "diameter": 1.75, "gtin": "012345678912"}, + ], + } + ], + ) + assert messages(check_data_quality(data), "adds nothing over size") == [] + + +def test_different_diameters_are_not_duplicates(tmp_path): + data = build( + tmp_path / "data", + variants=[ + { + "id": "black", + "name": "Black", + "sizes": [ + {"filament_weight": 1000, "diameter": 1.75}, + {"filament_weight": 1000, "diameter": 2.85}, + ], + } + ], + ) + assert messages(check_data_quality(data), "adds nothing over size") == [] + + +def test_a_differing_uuid_does_not_make_a_row_distinct(tmp_path): + # Every real duplicate in the dataset carries its own UUID; identity fields must + # not be what tells two rows apart. + data = build( + tmp_path / "data", + variants=[ + { + "id": "black", + "name": "Black", + "sizes": [ + {"uuid": "a" * 8, "filament_weight": 1000, "diameter": 1.75}, + {"uuid": "b" * 8, "filament_weight": 1000, "diameter": 1.75}, + ], + } + ], + ) + assert len(messages(check_data_quality(data), "adds nothing over size")) == 1 + + +def test_only_one_finding_per_redundant_row(tmp_path): + data = build( + tmp_path / "data", + variants=[ + { + "id": "black", + "name": "Black", + "sizes": [ + {"filament_weight": 1000, "diameter": 1.75}, + {"filament_weight": 1000, "diameter": 1.75}, + {"filament_weight": 1000, "diameter": 1.75}, + ], + } + ], + ) + # Rows 2 and 3 each report once, against the first row they duplicate. + assert len(messages(check_data_quality(data), "adds nothing over size")) == 2 + + +# --- name_casing (#451, #452) ------------------------------------------------- + + +def test_lowercase_name_among_title_case_siblings_warns(tmp_path): + data = build( + tmp_path / "data", + variants=[ + {"id": "purple", "name": "Purple"}, + {"id": "transparent", "name": "Transparent"}, + {"id": "translucent_blue", "name": "translucent blue"}, + ], + ) + found = messages(check_data_quality(data), "is lowercase while its siblings") + assert len(found) == 1 + assert "translucent blue" in found[0].message + assert found[0].level.value == "WARNING" + + +def test_all_lowercase_siblings_are_left_alone(tmp_path): + # No sibling establishes Title Case, so there is no house style to diverge from. + data = build( + tmp_path / "data", + variants=[ + {"id": "black", "name": "black"}, + {"id": "blue", "name": "blue"}, + ], + ) + assert messages(check_data_quality(data), "is lowercase while") == [] + + +def test_minor_words_do_not_break_title_case_detection(tmp_path): + data = build( + tmp_path / "data", + variants=[ + {"id": "black_and_white", "name": "Black and White"}, + {"id": "red", "name": "red"}, + ], + ) + assert len(messages(check_data_quality(data), "is lowercase while")) == 1 + + +def test_casing_is_scoped_to_siblings_not_the_whole_brand(tmp_path): + data = tmp_path / "data" + build(data, filament="basic", variants=[{"id": "black", "name": "Black"}]) + build(data, filament="matte", variants=[{"id": "red", "name": "red"}]) + # 'red' has no Title Case sibling inside `matte`, so it is not reported. + assert messages(check_data_quality(data), "is lowercase while") == [] + + +# --- fiber_trait_missing (#450) ----------------------------------------------- + + +def test_carbon_fiber_name_without_traits_warns(tmp_path): + data = build( + tmp_path / "data", + filament="pla_cf", + variants=[{"id": "carbon_fiber_blue", "name": "carbon fiber blue"}], + ) + found = messages(check_data_quality(data), "traits are not set") + assert len(found) == 1 + assert "contains_carbon_fiber" in found[0].message + assert "abrasive" in found[0].message + assert found[0].level.value == "WARNING" + + +def test_carbon_fiber_name_with_traits_is_clean(tmp_path): + data = build( + tmp_path / "data", + filament="pla_cf", + variants=[ + { + "id": "carbon_fiber_blue", + "name": "carbon fiber blue", + "traits": {"contains_carbon_fiber": True, "abrasive": True}, + } + ], + ) + assert messages(check_data_quality(data), "traits are not set") == [] + + +def test_fiber_code_on_the_filament_applies_to_its_variants(tmp_path): + data = build( + tmp_path / "data", + filament="pa6_gf", + variants=[{"id": "black", "name": "Black"}], + ) + found = messages(check_data_quality(data), "traits are not set") + assert len(found) == 1 + assert "contains_glass_fiber" in found[0].message + + +def test_a_plain_name_needs_no_fiber_traits(tmp_path): + data = build(tmp_path / "data", variants=[{"id": "black", "name": "Black"}]) + assert messages(check_data_quality(data), "traits are not set") == [] + + +# --- orphan_filament (#461) --------------------------------------------------- + + +def test_filament_with_no_variants_warns(tmp_path): + data = build(tmp_path / "data", filament="silk_blue_green", variants=[]) + found = messages(check_data_quality(data), "has no variants") + assert len(found) == 1 + assert found[0].level.value == "WARNING" + + +def test_filament_with_a_variant_is_clean(tmp_path): + data = build(tmp_path / "data", variants=[{"id": "black", "name": "Black"}]) + assert messages(check_data_quality(data), "has no variants") == [] + + +# --- sibling_near_duplicate --------------------------------------------------- + + +def test_word_order_duplicate_filaments_warn(tmp_path): + data = tmp_path / "data" + build(data, filament="cf_pla", variants=[{"id": "black", "name": "Black"}]) + build(data, filament="pla_cf", variants=[{"id": "black", "name": "Black"}]) + found = messages(check_data_quality(data), "word-order duplicate") + assert len(found) == 1 + assert found[0].level.value == "WARNING" + + +def test_word_order_duplicate_variants_warn(tmp_path): + data = build( + tmp_path / "data", + variants=[ + {"id": "blue_green_orange", "name": "Blue Green Orange"}, + {"id": "orange_blue_green", "name": "Orange Blue Green"}, + ], + ) + assert len(messages(check_data_quality(data), "word-order duplicate")) == 1 + + +def test_genuinely_different_siblings_are_not_duplicates(tmp_path): + data = build( + tmp_path / "data", + variants=[ + {"id": "sky_blue", "name": "Sky Blue"}, + {"id": "navy_blue", "name": "Navy Blue"}, + ], + ) + assert messages(check_data_quality(data), "word-order duplicate") == [] diff --git a/webui/src/lib/components/entity/EntityCard.svelte b/webui/src/lib/components/entity/EntityCard.svelte index 21cfbe6958..7fc387939a 100644 --- a/webui/src/lib/components/entity/EntityCard.svelte +++ b/webui/src/lib/components/entity/EntityCard.svelte @@ -56,6 +56,8 @@ hasDescendantChanges?: boolean; /** Whether this entity has submitted (pending-merge) changes */ hasSubmittedChanges?: boolean; + /** PR number of the submission this entity is in, shown in the badge tooltip. */ + submittedPrNumber?: number; /** The type of submitted change */ submittedChangeType?: 'create' | 'update' | 'delete'; /** Hover color variant */ @@ -87,6 +89,7 @@ localChangeType, hasDescendantChanges = false, hasSubmittedChanges = false, + submittedPrNumber = undefined, submittedChangeType, hoverColor, onCopy, @@ -217,7 +220,9 @@ {:else if hasSubmittedChanges} Submitted diff --git a/webui/src/lib/components/entity/InFlightHint.svelte b/webui/src/lib/components/entity/InFlightHint.svelte new file mode 100644 index 0000000000..004551641c --- /dev/null +++ b/webui/src/lib/components/entity/InFlightHint.svelte @@ -0,0 +1,40 @@ + + +{#if entry} + + This {label} is in review as part of submission + #{entry.prNumber}. When you submit, these changes can be added to it + instead of opening a second pull request. + +{/if} diff --git a/webui/src/lib/components/entity/SubmittedBanner.svelte b/webui/src/lib/components/entity/SubmittedBanner.svelte new file mode 100644 index 0000000000..e3a94b2ee3 --- /dev/null +++ b/webui/src/lib/components/entity/SubmittedBanner.svelte @@ -0,0 +1,53 @@ + + +
+
+

+ {#if merged} + Merged — going live with tonight's dataset build. + {:else} + Submitted — awaiting review. + {/if} +

+ {#if entry.prUrl} + + View submission #{entry.prNumber} ↗ + + {/if} +
+ {#if !merged} +

+ Further edits here can be added to that same submission when you submit again. +

+ {/if} +
diff --git a/webui/src/lib/components/entity/index.ts b/webui/src/lib/components/entity/index.ts index 222cc61289..9829ff6828 100644 --- a/webui/src/lib/components/entity/index.ts +++ b/webui/src/lib/components/entity/index.ts @@ -4,3 +4,5 @@ export { default as Logo } from './Logo.svelte'; export { default as SlicerSettingsDisplay } from './SlicerSettingsDisplay.svelte'; export { default as CertificationsDisplay } from './CertificationsDisplay.svelte'; export { default as ChildListPanel } from './ChildListPanel.svelte'; +export { default as SubmittedBanner } from './SubmittedBanner.svelte'; +export { default as InFlightHint } from './InFlightHint.svelte'; diff --git a/webui/src/lib/components/form-fields/PurchaseLinkCard.svelte b/webui/src/lib/components/form-fields/PurchaseLinkCard.svelte index 2a5d4f13df..02e0b679ab 100644 --- a/webui/src/lib/components/form-fields/PurchaseLinkCard.svelte +++ b/webui/src/lib/components/form-fields/PurchaseLinkCard.svelte @@ -1,10 +1,10 @@ + +
+ + {#if children}{@render children()}{:else}{message}{/if} + + + {#if hasAction} +
+ {#if href} + + {hrefLabel} + + {/if} + {#if fixLabel && onFix} + + {/if} +
+ {/if} +
diff --git a/webui/src/lib/components/ui/index.ts b/webui/src/lib/components/ui/index.ts index dc39d575af..d505b245d9 100644 --- a/webui/src/lib/components/ui/index.ts +++ b/webui/src/lib/components/ui/index.ts @@ -4,6 +4,7 @@ export { default as Switch } from './Switch.svelte'; export { default as LoadingSpinner } from './LoadingSpinner.svelte'; export { default as Modal } from './Modal.svelte'; export { default as MessageBanner } from './MessageBanner.svelte'; +export { default as FixHint } from './FixHint.svelte'; export { default as DeleteEntityModal } from './DeleteEntityModal.svelte'; export { default as ActionButtons } from './ActionButtons.svelte'; export { default as EmptyState } from './EmptyState.svelte'; diff --git a/webui/src/lib/config/__tests__/datasetSchedule.test.ts b/webui/src/lib/config/__tests__/datasetSchedule.test.ts new file mode 100644 index 0000000000..b9b9974dee --- /dev/null +++ b/webui/src/lib/config/__tests__/datasetSchedule.test.ts @@ -0,0 +1,61 @@ +/** + * The upstream API is rebuilt by a single nightly cron, so "merged" and "visible to the + * webui" are up to ~24 h apart. `datasetVisibleAfter` is what lets the submitted overlay + * bridge that gap instead of dropping a contributor's work the moment their PR merges. + */ +import { describe, it, expect } from 'vitest'; +import { + datasetVisibleAfter, + BUILD_CRON_UTC_HOUR, + BUILD_CRON_UTC_MINUTE, + BUILD_PROPAGATION_GRACE_MS +} from '../datasetSchedule'; + +/** The cron time on a given UTC date, plus the propagation grace. */ +function expected(iso: string): number { + const d = new Date(iso); + d.setUTCHours(BUILD_CRON_UTC_HOUR, BUILD_CRON_UTC_MINUTE, 0, 0); + return d.getTime() + BUILD_PROPAGATION_GRACE_MS; +} + +describe('datasetVisibleAfter', () => { + it('waits for tonight’s build when the merge is earlier in the day', () => { + // PR #459 merged 2026-08-19T13:48:57Z -> published by the 22:15Z build the same day. + expect(datasetVisibleAfter('2026-08-19T13:48:57Z').getTime()).toBe( + expected('2026-08-19T00:00:00Z') + ); + }); + + it('waits for the next day when the merge lands after the cron', () => { + expect(datasetVisibleAfter('2026-08-19T23:00:00Z').getTime()).toBe( + expected('2026-08-20T00:00:00Z') + ); + }); + + it('treats a merge exactly at cron time as missing that run', () => { + // The build reads the tree when it starts; a merge at that instant may not be in it. + expect(datasetVisibleAfter('2026-08-19T22:15:00Z').getTime()).toBe( + expected('2026-08-20T00:00:00Z') + ); + }); + + it('rolls over month and year boundaries', () => { + expect(datasetVisibleAfter('2026-12-31T23:30:00Z').getTime()).toBe( + expected('2027-01-01T00:00:00Z') + ); + }); + + it('accepts a Date or an epoch as well as an ISO string', () => { + const iso = '2026-08-19T13:48:57Z'; + const fromIso = datasetVisibleAfter(iso).getTime(); + expect(datasetVisibleAfter(new Date(iso)).getTime()).toBe(fromIso); + expect(datasetVisibleAfter(new Date(iso).getTime()).getTime()).toBe(fromIso); + }); + + it('always lands strictly after the merge', () => { + for (const hour of [0, 6, 12, 22, 23]) { + const merged = `2026-08-19T${String(hour).padStart(2, '0')}:20:00Z`; + expect(datasetVisibleAfter(merged).getTime()).toBeGreaterThan(new Date(merged).getTime()); + } + }); +}); diff --git a/webui/src/lib/config/datasetSchedule.ts b/webui/src/lib/config/datasetSchedule.ts new file mode 100644 index 0000000000..145fbd89c4 --- /dev/null +++ b/webui/src/lib/config/datasetSchedule.ts @@ -0,0 +1,41 @@ +/** + * When upstream data a contributor submitted actually becomes visible to the webui. + * + * In cloud mode the webui reads `api.openfilamentdatabase.org`, which is rebuilt by + * `.github/workflows/build-dataset.yml` on a single nightly cron (`15 22 * * *`, UTC). + * So a merged PR is *not* reflected upstream at merge time — it lands at the next nightly + * build, up to ~24 h later. + * + * The submitted-changes overlay (`$lib/stores/submitted.ts`) has to cover exactly that + * window: dropping an entry when its PR merges makes the contributor's own work vanish from + * their view, which is what caused duplicate submissions #442 (re-submitting #433's variants) + * and #460 (re-creating a filament #459 had already added). + * + * Keep `BUILD_CRON_UTC_HOUR`/`_MINUTE` in sync with the workflow's cron expression. + */ + +/** Hour (UTC) of the nightly `build-dataset` cron. */ +export const BUILD_CRON_UTC_HOUR = 22; +/** Minute (UTC) of the nightly `build-dataset` cron. */ +export const BUILD_CRON_UTC_MINUTE = 15; + +/** + * Slack after the cron fires before the rebuilt dataset is actually served: the build, + * the Pages deploy and CDN propagation. Deliberately generous — over-estimating keeps a + * correct entry in the overlay a little too long, while under-estimating re-opens the + * disappearing-work bug this whole mechanism exists to prevent. + */ +export const BUILD_PROPAGATION_GRACE_MS = 90 * 60 * 1000; + +/** + * The moment a change merged at `mergedAt` can be expected to appear in the upstream API: + * the first nightly build strictly after the merge, plus propagation grace. + */ +export function datasetVisibleAfter(mergedAt: Date | string | number): Date { + const merged = new Date(mergedAt); + const build = new Date(merged); + build.setUTCHours(BUILD_CRON_UTC_HOUR, BUILD_CRON_UTC_MINUTE, 0, 0); + // A merge at or after tonight's cron time waits for tomorrow's run. + if (build.getTime() <= merged.getTime()) build.setUTCDate(build.getUTCDate() + 1); + return new Date(build.getTime() + BUILD_PROPAGATION_GRACE_MS); +} diff --git a/webui/src/lib/server/anonBot.ts b/webui/src/lib/server/anonBot.ts index 400cf62c8a..f78d8e2d29 100644 --- a/webui/src/lib/server/anonBot.ts +++ b/webui/src/lib/server/anonBot.ts @@ -12,10 +12,14 @@ import { createTree, createCommit, updateRef, - createPullRequest + createPullRequest, + updatePullRequest, + getPullRequest } from '$lib/server/github'; import { getInstallationToken } from '$lib/server/githubApp'; import { buildTreeItems, buildChangesSummary, explainEmptyTree } from '$lib/server/prBuilder'; +import { generateChangeTitle } from '$lib/utils/changeTitleGenerator'; +import type { EntityChange } from '$lib/types/changes'; // --- Types --- @@ -35,6 +39,8 @@ export interface AnonSubmissionResult { error?: string; skippedPaths?: string[]; noopDeletes?: string[]; + /** True when the changes were added to an existing PR rather than opening a new one. */ + amended?: boolean; } // --- Configuration --- @@ -56,6 +62,139 @@ export function extractUuidFromBody(body: string): string | null { return match?.[1] ?? null; } +// --- Shared PR body construction --- + +/** The branch an anon submission owns. Stable per submission UUID, so it can be amended. */ +export function anonBranchName(uuid: string): string { + return `ofd-anon-${uuid}`; +} + +/** + * Compose the PR body. The UUID comment must come first and stay byte-identical across + * amends — `api/webhooks/github` reads it back with `extractUuidFromBody` to route merge + * and close events to the right submission. + */ +function buildPrBody(uuid: string, description: string | undefined, changes: any[]): string { + const via = publicEnv.PUBLIC_WRAPPER_NAME || 'the OFD web editor'; + + return [ + buildUuidComment(uuid), + description || 'Submitted via Open Filament Database web editor.', + '', + '## Changes', + buildChangesSummary(changes), + '', + `*Submitted via ${via}*` + ].join('\n'); +} + +function defaultTitle(changes: any[]): string { + return `Update filament database (${changes.length} change${changes.length === 1 ? '' : 's'})`; +} + +// --- PR Amendment --- + +export interface AnonAmendment extends AnonSubmission { + /** The open PR to add these changes to. */ + prNumber: number; + /** Every change in the submission, earlier batches included, for the rewritten PR body. */ + allChanges: any[]; +} + +/** + * Add another batch of changes to a submission that is still in review. + * + * Contributors routinely finish one batch, submit, then keep editing — #459/#460/#461 opened + * three PRs on the same 3dhojor paths within 13 minutes, which merged out of order, needed + * hand-resolved conflicts, and left an orphan filament in `main`. Stacking the second batch + * onto the same branch keeps it one review and one merge. + * + * The tree is built from the **branch head**, not `main`, so the new batch sees the first + * batch's files: `buildTreeItems` resolves cascade-deletes and carries canonical `uuid` / + * `moved_from` fields out of whatever tree it is given. + * + * Returns `success: false` with `retryAsNew` when the PR is no longer amendable (merged, + * closed, or its branch deleted), so the caller can fall back to opening a new PR. + */ +export async function amendAnonPR( + amendment: AnonAmendment +): Promise { + const token = await getInstallationToken(); + const upstreamOwner = privateEnv.GITHUB_UPSTREAM_OWNER!; + const upstreamRepo = privateEnv.GITHUB_UPSTREAM_REPO!; + const branchName = anonBranchName(amendment.uuid); + + // 1. The PR must still be open. Re-checked here rather than trusting the caller's cache, + // because a maintainer may have merged it between the client's last poll and this call. + const pr = await getPullRequest(token, upstreamOwner, upstreamRepo, amendment.prNumber); + if (!pr || pr.merged || pr.state !== 'open') { + return { + success: false, + uuid: amendment.uuid, + retryAsNew: true, + error: 'That submission is no longer open.' + }; + } + + // 2. Resolve the branch head. A missing ref means the branch was deleted out from under + // the PR; there is nothing to stack onto. + let headSha: string; + let baseTreeSha: string; + try { + headSha = await getLatestCommitSha(token, upstreamOwner, upstreamRepo, branchName); + baseTreeSha = await getCommitTreeSha(token, upstreamOwner, upstreamRepo, headSha); + } catch { + return { + success: false, + uuid: amendment.uuid, + retryAsNew: true, + error: 'That submission’s branch no longer exists.' + }; + } + + // 3. Build the new batch against the branch's own tree. + const { treeItems, skippedPaths = [], noopDeletes = [] } = await buildTreeItems( + token, upstreamOwner, upstreamRepo, baseTreeSha, + upstreamOwner, upstreamRepo, + amendment.changes, amendment.images + ); + + if (treeItems.length === 0) { + return { + success: false, + uuid: amendment.uuid, + error: explainEmptyTree(skippedPaths, noopDeletes) + }; + } + + // 4. Commit onto the branch head. + const treeSha = await createTree(token, upstreamOwner, upstreamRepo, baseTreeSha, treeItems); + const commitMessage = + amendment.title || defaultTitle(amendment.changes); + const commitSha = await createCommit( + token, upstreamOwner, upstreamRepo, commitMessage, treeSha, headSha + ); + await updateRef(token, upstreamOwner, upstreamRepo, branchName, commitSha); + + // 5. Rewrite the PR title and body so both cover every batch. `amendment.title` describes + // only the batch being added — good as the commit message above, wrong as the PR title, + // which would otherwise end up naming the last batch while the body lists them all. + const updated = await updatePullRequest(token, upstreamOwner, upstreamRepo, amendment.prNumber, { + title: generateChangeTitle(amendment.allChanges as EntityChange[]), + body: buildPrBody(amendment.uuid, amendment.description, amendment.allChanges) + }); + + return { + success: true, + uuid: amendment.uuid, + prUrl: updated.html_url, + prNumber: updated.number, + amended: true, + skippedPaths: skippedPaths.length > 0 ? skippedPaths : undefined, + noopDeletes: noopDeletes.length > 0 ? noopDeletes.map((d) => d.description || d.path) : undefined + }; +} + // --- PR Creation --- export async function createAnonPR(submission: AnonSubmission): Promise { @@ -68,7 +207,7 @@ export async function createAnonPR(submission: AnonSubmission): Promise { // --- Store ID cache (for cross-reference validation) --- -let storeIdsCache: { ids: Set; fetchedAt: number } | null = null; +/** Known store id/slug -> that store's storefront URL (null when the store has none). */ +type KnownStores = Map; + +let storeIdsCache: { stores: KnownStores; fetchedAt: number } | null = null; const STORE_IDS_TTL_MS = 30 * 60 * 1000; // 30 minutes -async function fetchKnownStoreIds(): Promise> { +async function fetchKnownStores(): Promise { if (storeIdsCache && Date.now() - storeIdsCache.fetchedAt < STORE_IDS_TTL_MS) { - return storeIdsCache.ids; + return storeIdsCache.stores; } const url = `${API_BASE}/api/v1/stores/index.json`; @@ -90,18 +95,19 @@ async function fetchKnownStoreIds(): Promise> { // rewritten to slugs before submission (see normalizeCloudVariant in api.ts / // cloudProxy.ts), but legacy data may still reference UUIDs — index both forms // so cross-reference validation accepts either. - const ids = new Set(); + const known: KnownStores = new Map(); const stores = Array.isArray(data) ? data : data?.stores; if (Array.isArray(stores)) { for (const store of stores) { if (!store) continue; - if (typeof store.id === 'string') ids.add(store.id); - if (typeof store.slug === 'string') ids.add(store.slug); + const storefront = typeof store.storefront_url === 'string' ? store.storefront_url : null; + if (typeof store.id === 'string') known.set(store.id, storefront); + if (typeof store.slug === 'string') known.set(store.slug, storefront); } } - storeIdsCache = { ids, fetchedAt: Date.now() }; - return ids; + storeIdsCache = { stores: known, fetchedAt: Date.now() }; + return known; } // --- Ajv setup --- @@ -343,18 +349,84 @@ function isValidGtin(gtin: string): boolean { return checkDigit === expected; } +/** The field holding an entity's human-facing display name. */ +const NAME_FIELD: Record = { + brand: 'name', + material: 'material', + filament: 'name', + variant: 'name', + store: 'name' +}; + +/** + * Data-quality checks on an entity, mirroring `ofd/validation/data_quality.py`. + * + * The forms surface these as inline "Fix" hints, but those live in Svelte components: + * a changeset POSTed straight at `/api/anon/submit` never sees them. This is the same + * rule set at the submission gate. + */ +function validateEntityQuality( + entityType: string, + entityPath: string, + data: Record +): ValidationError[] { + const errors: ValidationError[] = []; + + const nameField = NAME_FIELD[entityType]; + const name = nameField ? data[nameField] : undefined; + if (typeof name === 'string') { + const whitespace = checkNameWhitespace(name); + if (whitespace) { + errors.push({ + category: 'Data quality', + level: 'ERROR', + message: `Name '${name}' ${whitespace.reason}. Did you mean '${whitespace.suggestion}'?`, + path: entityPath + }); + } + } + + for (const [key, value] of Object.entries(data)) { + if (!Array.isArray(value)) continue; + const blanks = checkPlaceholderEntries(value); + if (blanks.length > 0) { + errors.push({ + category: 'Data quality', + level: 'ERROR', + message: `'${key}' contains ${blanks.length} blank ${blanks.length === 1 ? 'entry' : 'entries'}. Remove ${blanks.length === 1 ? 'it' : 'them'}, or use an empty list.`, + path: entityPath + }); + } + } + + return errors; +} + /** * Validate store ID cross-references and GTIN checksums in sizes data. */ function validateSizesData( sizesData: unknown, entityPath: string, - knownStoreIds: Set, + knownStores: KnownStores, newStoreIds: Set ): ValidationError[] { const errors: ValidationError[] = []; if (!Array.isArray(sizesData)) return errors; + // A row that says nothing an earlier row doesn't renders as the same size listed + // twice (#453). Spool identity is (filament_weight, diameter) — see size_dedupe_key. + for (const { index, duplicateOf } of findRedundantSizes( + sizesData as Array> + )) { + errors.push({ + category: 'Data quality', + level: 'ERROR', + message: `Size #${index + 1} adds nothing over size #${duplicateOf + 1}. Merge them, or give it the SKU, GTIN or purchase link that tells them apart.`, + path: entityPath + }); + } + for (let si = 0; si < sizesData.length; si++) { const size = sizesData[si]; if (typeof size !== 'object' || size === null) continue; @@ -382,7 +454,7 @@ function validateSizesData( const storeId = linkObj.store_id; if (typeof storeId === 'string' && storeId.length > 0) { - if (!knownStoreIds.has(storeId) && !newStoreIds.has(storeId)) { + if (!knownStores.has(storeId) && !newStoreIds.has(storeId)) { errors.push({ category: 'Store IDs', level: 'ERROR', @@ -391,6 +463,24 @@ function validateSizesData( }); } } + + // A shop homepage identifies neither the filament nor the colour. #454 arrived + // with `https://store.bambulab.com/` and a maintainer had to find the real + // product page by hand. The webui refuses these at the field; this is the same + // rule for payloads posted straight at the submission endpoint. + const linkUrl = linkObj.url; + if (typeof linkUrl === 'string' && linkUrl.length > 0) { + const storefront = + typeof storeId === 'string' ? (knownStores.get(storeId) ?? null) : null; + if (isStorefrontRoot(linkUrl, storefront)) { + errors.push({ + category: 'Purchase links', + level: 'ERROR', + message: `Purchase link points at the shop homepage rather than a product page: '${linkUrl}' (size #${si + 1}, link #${li + 1})`, + path: entityPath + }); + } + } } } } @@ -476,11 +566,11 @@ export async function runCloudValidation( return; } - let knownStoreIds = new Set(); + let knownStores: KnownStores = new Map(); try { - knownStoreIds = await fetchKnownStoreIds(); + knownStores = await fetchKnownStores(); } catch { - // Non-fatal: store ID cross-referencing will be skipped + // Non-fatal: store cross-referencing and the storefront-root check are skipped } // --- Validate entity paths and operations --- @@ -618,6 +708,9 @@ export async function runCloudValidation( errors.push(folderError); } + // Data-quality rules mirrored from ofd/validation/data_quality.py + errors.push(...validateEntityQuality(entity.type, entity.path, cleaned)); + // Validate sizes: schema + GTIN checksums + store ID cross-references if (sizesData !== undefined) { try { @@ -636,7 +729,7 @@ export async function runCloudValidation( }); } - errors.push(...validateSizesData(sizesData, entity.path, knownStoreIds, newStoreIds)); + errors.push(...validateSizesData(sizesData, entity.path, knownStores, newStoreIds)); } } diff --git a/webui/src/lib/server/github.ts b/webui/src/lib/server/github.ts index 53168b1ced..bf53ef19f3 100644 --- a/webui/src/lib/server/github.ts +++ b/webui/src/lib/server/github.ts @@ -324,6 +324,31 @@ export async function createPullRequest( return { number: pr.number, html_url: pr.html_url }; } +/** + * Update an existing pull request's title and/or body. + * + * Used when a contributor adds a second batch of changes to a submission that is still in + * review: the new commit goes on the same branch, and the PR body's `## Changes` list is + * rewritten to cover everything. See `amendAnonPR` in `anonBot.ts`. + */ +export async function updatePullRequest( + token: string, + owner: string, + repo: string, + prNumber: number, + fields: { title?: string; body?: string } +): Promise<{ number: number; html_url: string }> { + const response = await ghFetch(token, `/repos/${owner}/${repo}/pulls/${prNumber}`, { + method: 'PATCH', + body: JSON.stringify(fields) + }); + + if (!response.ok) throw await ghError(response, 'Failed to update PR'); + + const pr = await response.json(); + return { number: pr.number, html_url: pr.html_url }; +} + /** * Get a pull request's current state from GitHub. * `token` is optional — the upstream repo is public, so an unauthenticated @@ -335,7 +360,16 @@ export async function getPullRequest( owner: string, repo: string, prNumber: number -): Promise<{ number: number; state: 'open' | 'closed'; merged: boolean; html_url: string } | null> { +): Promise<{ + number: number; + state: 'open' | 'closed'; + merged: boolean; + /** ISO timestamp of the merge, or null when the PR is not merged. */ + merged_at: string | null; + html_url: string; + /** Head branch name, so callers can tell an amendable branch from a deleted one. */ + head_ref: string | null; +} | null> { const path = `/repos/${owner}/${repo}/pulls/${prNumber}`; const response = token ? await ghFetch(token, path) @@ -349,7 +383,9 @@ export async function getPullRequest( number: pr.number, state: pr.state, merged: pr.merged === true, - html_url: pr.html_url + merged_at: pr.merged_at ?? null, + html_url: pr.html_url, + head_ref: pr.head?.ref ?? null }; } diff --git a/webui/src/lib/stores/__tests__/submitted.test.ts b/webui/src/lib/stores/__tests__/submitted.test.ts index 945831dd5f..0bf21ff341 100644 --- a/webui/src/lib/stores/__tests__/submitted.test.ts +++ b/webui/src/lib/stores/__tests__/submitted.test.ts @@ -473,6 +473,245 @@ describe('Submitted Store', () => { }); }); + describe('reconcile', () => { + function mockStatus(statuses: Record, mergedAt: Record = {}) { + return vi.fn(async () => ({ + ok: true, + json: async () => ({ statuses, mergedAt }) + })) as unknown as typeof fetch; + } + + beforeEach(() => { + vi.unstubAllGlobals(); + }); + + it('drops a closed submission immediately', async () => { + submittedStore.archive({ + uuid: 'sub-1', + prUrl: 'https://example.com/pr/1', + prNumber: 1, + changes: [makeChange('brands/acme', 'acme')] + }); + vi.stubGlobal('fetch', mockStatus({ 1: 'closed' })); + + await submittedStore.reconcile(); + + expect(submittedStore.has('brands/acme')).toBe(false); + }); + + it('keeps a merged submission until the nightly rebuild has published it', async () => { + // The bug this guards: PR #459 merged, the overlay dropped it, the upstream API + // (rebuilt once a day) still lacked it, so the contributor re-created the filament + // in #460. A merged entry must stay visible until the next build has run. + submittedStore.archive({ + uuid: 'sub-1', + prUrl: 'https://example.com/pr/459', + prNumber: 459, + changes: [makeChange('brands/3dhojor', '3dhojor')] + }); + const justMerged = new Date().toISOString(); + vi.stubGlobal('fetch', mockStatus({ 459: 'merged' }, { 459: justMerged })); + + await submittedStore.reconcile(); + + expect(submittedStore.has('brands/3dhojor')).toBe(true); + const entry = submittedStore.getEntries()[0]; + expect(entry.status).toBe('merged'); + expect(entry.mergedAt).toBe(justMerged); + }); + + it('evicts a merged submission once the rebuild window has passed', async () => { + submittedStore.archive({ + uuid: 'sub-1', + prUrl: 'https://example.com/pr/1', + prNumber: 1, + changes: [makeChange('brands/acme', 'acme')] + }); + // Merged two days ago — at least two nightly builds have run since. + const longAgo = new Date(Date.now() - 2 * 24 * 60 * 60 * 1000).toISOString(); + vi.stubGlobal('fetch', mockStatus({ 1: 'merged' }, { 1: longAgo })); + + await submittedStore.reconcile(); + + expect(submittedStore.has('brands/acme')).toBe(false); + }); + + it('falls back to now when GitHub gives no merge timestamp', async () => { + submittedStore.archive({ + uuid: 'sub-1', + prUrl: 'https://example.com/pr/1', + prNumber: 1, + changes: [makeChange('brands/acme', 'acme')] + }); + vi.stubGlobal('fetch', mockStatus({ 1: 'merged' })); + + await submittedStore.reconcile(); + + // Still present (the fallback is "just now"), and stamped so it can age out later. + expect(submittedStore.has('brands/acme')).toBe(true); + expect(submittedStore.getEntries()[0].mergedAt).toBeTruthy(); + }); + + it('records changes_requested without dropping the entry', async () => { + submittedStore.archive({ + uuid: 'sub-1', + prUrl: 'https://example.com/pr/1', + prNumber: 1, + changes: [makeChange('brands/acme', 'acme')] + }); + vi.stubGlobal('fetch', mockStatus({ 1: 'changes_requested' })); + + await submittedStore.reconcile(); + + expect(submittedStore.has('brands/acme')).toBe(true); + expect(submittedStore.getEntries()[0].status).toBe('changes_requested'); + }); + + it('leaves entries alone when the status endpoint fails', async () => { + submittedStore.archive({ + uuid: 'sub-1', + prUrl: 'https://example.com/pr/1', + prNumber: 1, + changes: [makeChange('brands/acme', 'acme')] + }); + vi.stubGlobal('fetch', vi.fn(async () => ({ ok: false })) as unknown as typeof fetch); + + await submittedStore.reconcile(); + + expect(submittedStore.has('brands/acme')).toBe(true); + }); + + it('ignores an unknown status', async () => { + submittedStore.archive({ + uuid: 'sub-1', + prUrl: 'https://example.com/pr/1', + prNumber: 1, + changes: [makeChange('brands/acme', 'acme')] + }); + vi.stubGlobal('fetch', mockStatus({ 1: 'unknown' })); + + await submittedStore.reconcile(); + + expect(submittedStore.has('brands/acme')).toBe(true); + expect(submittedStore.getEntries()[0].status).toBe('open'); + }); + }); + + describe('openEntries / findOverlap', () => { + it('treats an entry with no recorded status as open', () => { + submittedStore.archive({ + uuid: 'sub-1', + prUrl: 'https://example.com/pr/1', + prNumber: 1, + changes: [makeChange('brands/acme', 'acme')] + }); + expect(submittedStore.openEntries().map((e) => e.uuid)).toEqual(['sub-1']); + }); + + it('excludes merged entries from the amend candidates', async () => { + submittedStore.archive({ + uuid: 'sub-1', + prUrl: 'https://example.com/pr/1', + prNumber: 1, + changes: [makeChange('brands/acme', 'acme')] + }); + vi.stubGlobal( + 'fetch', + vi.fn(async () => ({ + ok: true, + json: async () => ({ + statuses: { 1: 'merged' }, + mergedAt: { 1: new Date().toISOString() } + }) + })) as unknown as typeof fetch + ); + await submittedStore.reconcile(); + + // Still layered over API data, but no longer something to amend. + expect(submittedStore.has('brands/acme')).toBe(true); + expect(submittedStore.openEntries()).toHaveLength(0); + expect(submittedStore.findOverlap(['brands/acme'])).toEqual([]); + }); + + it('finds an exact path overlap', () => { + submittedStore.archive({ + uuid: 'sub-1', + prUrl: 'https://example.com/pr/459', + prNumber: 459, + changes: [makeChange('brands/3dhojor/materials/PLA/filaments/silk', 'silk', 'filament')] + }); + + const overlap = submittedStore.findOverlap([ + 'brands/3dhojor/materials/PLA/filaments/silk' + ]); + expect(overlap).toHaveLength(1); + expect(overlap[0].entry.prNumber).toBe(459); + }); + + it('finds a descendant of an in-flight path', () => { + // #461 added a variant under the filament #459 had just created. + submittedStore.archive({ + uuid: 'sub-1', + prUrl: 'https://example.com/pr/459', + prNumber: 459, + changes: [makeChange('brands/3dhojor/materials/PLA/filaments/silk', 'silk', 'filament')] + }); + + const overlap = submittedStore.findOverlap([ + 'brands/3dhojor/materials/PLA/filaments/silk/variants/silk_blue_green' + ]); + expect(overlap).toHaveLength(1); + }); + + it('finds an ancestor of an in-flight path', () => { + submittedStore.archive({ + uuid: 'sub-1', + prUrl: 'https://example.com/pr/1', + prNumber: 1, + changes: [ + makeChange( + 'brands/elegoo/materials/PLA/filaments/pla_metallic/variants/metallic_blue', + 'metallic_blue', + 'variant' + ) + ] + }); + + const overlap = submittedStore.findOverlap([ + 'brands/elegoo/materials/PLA/filaments/pla_metallic' + ]); + expect(overlap).toHaveLength(1); + }); + + it('does not match an unrelated sibling with a shared prefix', () => { + submittedStore.archive({ + uuid: 'sub-1', + prUrl: 'https://example.com/pr/1', + prNumber: 1, + changes: [makeChange('brands/acme', 'acme')] + }); + + // "brands/acme_two" starts with "brands/acme" as a string but is a different brand. + expect(submittedStore.findOverlap(['brands/acme_two'])).toEqual([]); + }); + + it('reports only the overlapping paths, not the whole change set', () => { + submittedStore.archive({ + uuid: 'sub-1', + prUrl: 'https://example.com/pr/1', + prNumber: 1, + changes: [makeChange('brands/acme', 'acme')] + }); + + const overlap = submittedStore.findOverlap(['brands/acme', 'brands/other']); + expect(overlap[0].paths).toEqual(['brands/acme']); + }); + + it('returns nothing for an empty path list', () => { + expect(submittedStore.findOverlap([])).toEqual([]); + }); + }); + describe('localStorage error handling', () => { it('survives a corrupted localStorage entry', async () => { vi.resetModules(); diff --git a/webui/src/lib/stores/submitted.ts b/webui/src/lib/stores/submitted.ts index d0c0a50c5b..3fec7f9121 100644 --- a/webui/src/lib/stores/submitted.ts +++ b/webui/src/lib/stores/submitted.ts @@ -1,7 +1,13 @@ import { writable, derived, get } from 'svelte/store'; import { browser } from '$app/environment'; -import type { EntityChange, SubmittedEntry, SubmittedBuffer } from '$lib/types/changes'; +import type { + EntityChange, + SubmittedEntry, + SubmittedBuffer, + SubmissionStatus +} from '$lib/types/changes'; import { STORAGE_KEY_SUBMITTED } from '$lib/config/storageKeys'; +import { datasetVisibleAfter } from '$lib/config/datasetSchedule'; const DEFAULT_TTL_DAYS = 7; @@ -9,6 +15,29 @@ function createEmptyBuffer(): SubmittedBuffer { return { entries: {}, version: 1 }; } +/** Entries written before `status` existed predate any reconcile, so they are still open. */ +function statusOf(entry: SubmittedEntry): SubmissionStatus { + return entry.status ?? 'open'; +} + +/** True while the PR is still in review — the case an amend can be offered for. */ +function isOpen(entry: SubmittedEntry): boolean { + const status = statusOf(entry); + return status === 'open' || status === 'changes_requested'; +} + +/** + * True once a merged entry's data can be expected to have reached the upstream API, so the + * overlay copy is redundant and should be dropped. Open/closed entries are not stale by this + * measure — open ones are still pending, and closed ones are removed outright. + */ +function isMergedAndPublished(entry: SubmittedEntry, now: number): boolean { + if (statusOf(entry) !== 'merged') return false; + // A merged entry with no timestamp can't be aged; the TTL still evicts it. + if (!entry.mergedAt) return false; + return datasetVisibleAfter(entry.mergedAt).getTime() <= now; +} + /** Build a path-to-change index from all entries (newest submission wins). */ function buildIndex( buffer: SubmittedBuffer @@ -45,10 +74,10 @@ function createSubmittedStore() { if (stored) { const parsed: SubmittedBuffer = JSON.parse(stored); if (parsed.version === 1) { - // Evict expired entries on load + // Evict expired entries on load, plus merged ones upstream has published. const now = Date.now(); for (const [uuid, entry] of Object.entries(parsed.entries)) { - if (new Date(entry.expiresAt).getTime() <= now) { + if (new Date(entry.expiresAt).getTime() <= now || isMergedAndPublished(entry, now)) { delete parsed.entries[uuid]; } } @@ -76,6 +105,19 @@ function createSubmittedStore() { _index = buildIndex(buffer); } + /** Drop merged entries the nightly rebuild has published. Returns true if anything went. */ + function evictPublishedFrom(buffer: SubmittedBuffer): boolean { + const now = Date.now(); + let changed = false; + for (const [uuid, entry] of Object.entries(buffer.entries)) { + if (isMergedAndPublished(entry, now)) { + delete buffer.entries[uuid]; + changed = true; + } + } + return changed; + } + return { subscribe, @@ -110,7 +152,8 @@ function createSubmittedStore() { submittedAt: now.toISOString(), expiresAt, changes: lightChanges, - paths: lightChanges.map((c) => c.entity.path) + paths: lightChanges.map((c) => c.entity.path), + status: 'open' }; update((buffer) => { @@ -123,9 +166,19 @@ function createSubmittedStore() { /** * Reconcile tracked submissions against GitHub's real PR state. - * Asks the server for the current status of each tracked PR (which checks - * GitHub for anything the merge webhook may have missed) and drops entries - * whose PR is merged or closed. Safe to call on load; no-ops with no entries. + * + * Asks the server for the current status of each tracked PR (which checks GitHub for + * anything the merge webhook may have missed) and records it on the entry. + * + * A **closed** PR is dropped immediately — its data is never coming. + * + * A **merged** PR is deliberately *kept*, marked `merged` with the merge time, until + * the nightly dataset rebuild has plausibly published it (`datasetVisibleAfter`). + * Merging does not make the change visible upstream — the API is rebuilt once a day — + * so dropping the overlay at merge time makes the contributor's own work disappear + * from their view. That is what produced the duplicate submissions in #442 and #460. + * + * Safe to call on load; no-ops with no entries. */ async reconcile(): Promise { if (!browser) return; @@ -135,6 +188,7 @@ function createSubmittedStore() { if (prNumbers.length === 0) return; let statuses: Record; + let mergedAt: Record = {}; try { const res = await fetch('/api/submissions/status', { method: 'POST', @@ -142,20 +196,48 @@ function createSubmittedStore() { body: JSON.stringify({ prNumbers }) }); if (!res.ok) return; - ({ statuses } = await res.json()); + const payload = await res.json(); + statuses = payload.statuses; + mergedAt = payload.mergedAt ?? {}; } catch { return; // Network/offline — leave entries as-is. } + const now = new Date().toISOString(); + update((buffer) => { let changed = false; for (const entry of Object.values(buffer.entries)) { - const status = statuses[entry.prNumber]; - if (status === 'merged' || status === 'closed') { + const status = statuses[entry.prNumber] as SubmissionStatus | 'unknown' | undefined; + if (!status || status === 'unknown') continue; + + if (status === 'closed') { delete buffer.entries[entry.uuid]; changed = true; + continue; + } + + if (status === 'merged') { + // Prefer GitHub's merge time; fall back to first observation, which is + // close enough given reconcile runs whenever the menu mounts. + const merged = mergedAt[entry.prNumber] ?? entry.mergedAt ?? now; + if (entry.status !== 'merged' || entry.mergedAt !== merged) { + entry.status = 'merged'; + entry.mergedAt = merged; + changed = true; + } + continue; + } + + if (entry.status !== status) { + entry.status = status; + changed = true; } } + + // A merged entry whose data has since been published upstream is redundant. + if (evictPublishedFrom(buffer)) changed = true; + if (changed) { rebuildIndex(buffer); persist(buffer); @@ -164,6 +246,43 @@ function createSubmittedStore() { }); }, + /** + * Entries whose PR is still in review, newest first — the candidates for amending + * rather than opening a competing PR. + */ + openEntries(): SubmittedEntry[] { + const buffer = get({ subscribe }); + return Object.values(buffer.entries) + .filter(isOpen) + .sort((a, b) => new Date(b.submittedAt).getTime() - new Date(a.submittedAt).getTime()); + }, + + /** + * Which still-open submissions already touch any of `paths`. Opening a second PR over + * these is what causes the out-of-order merges and hand-resolved conflicts this store + * exists to prevent, so the submit flow offers to amend instead. + * + * A path counts as overlapping when it matches an in-flight path exactly, or is an + * ancestor/descendant of one — editing a filament that is in review conflicts with the + * PR that created it just as much as re-editing the filament itself does. + */ + findOverlap(paths: string[]): Array<{ entry: SubmittedEntry; paths: string[] }> { + if (paths.length === 0) return []; + const results: Array<{ entry: SubmittedEntry; paths: string[] }> = []; + + for (const entry of this.openEntries()) { + const inFlight = entry.paths; + const hits = paths.filter((p) => + inFlight.some( + (q) => p === q || p.startsWith(q + '/') || q.startsWith(p + '/') + ) + ); + if (hits.length > 0) results.push({ entry, paths: hits }); + } + + return results; + }, + /** Remove a specific submission by UUID. */ remove(uuid: string) { update((buffer) => { @@ -174,13 +293,13 @@ function createSubmittedStore() { }); }, - /** Evict expired entries. */ + /** Evict entries past their TTL, plus merged ones the upstream API has since published. */ evictExpired() { const now = Date.now(); update((buffer) => { let changed = false; for (const [uuid, entry] of Object.entries(buffer.entries)) { - if (new Date(entry.expiresAt).getTime() <= now) { + if (new Date(entry.expiresAt).getTime() <= now || isMergedAndPublished(entry, now)) { delete buffer.entries[uuid]; changed = true; } @@ -233,6 +352,11 @@ function createSubmittedStore() { return false; }, + /** Look up one submission by UUID. */ + getEntry(uuid: string): SubmittedEntry | undefined { + return get({ subscribe }).entries[uuid]; + }, + /** Get all entries (for ChangesMenu display). Sorted newest first. */ getEntries(): SubmittedEntry[] { const buffer = get({ subscribe }); diff --git a/webui/src/lib/types/changes.ts b/webui/src/lib/types/changes.ts index daf2a3a972..710390441f 100644 --- a/webui/src/lib/types/changes.ts +++ b/webui/src/lib/types/changes.ts @@ -94,6 +94,8 @@ export interface ImageReference { * A single submitted (PR-created) change set, preserved for visual overlay * until the entry expires (TTL-based). */ +export type SubmissionStatus = 'open' | 'merged' | 'closed' | 'changes_requested'; + export interface SubmittedEntry { uuid: string; prUrl: string; @@ -103,6 +105,17 @@ export interface SubmittedEntry { changes: EntityChange[]; /** Denormalized entity paths for O(1) lookup */ paths: string[]; + /** + * Last known PR state, refreshed by `submittedStore.reconcile()`. Absent on entries + * written before this field existed; treat that as 'open'. + */ + status?: SubmissionStatus; + /** + * When the PR merged. A merged entry stays in the overlay until the nightly dataset + * rebuild has plausibly published it (see `$lib/config/datasetSchedule.ts`) — otherwise + * the contributor's own work disappears from their view and they re-submit it. + */ + mergedAt?: string; } /** diff --git a/webui/src/lib/utils/__tests__/dataQuality.test.ts b/webui/src/lib/utils/__tests__/dataQuality.test.ts new file mode 100644 index 0000000000..7e8eeae00a --- /dev/null +++ b/webui/src/lib/utils/__tests__/dataQuality.test.ts @@ -0,0 +1,205 @@ +/** + * Mirrors `tests/test_data_quality.py`. Each case is anchored to the submission that + * motivated the rule, so a regression traces back to the review comment it exists to + * make unnecessary. + */ +import { describe, it, expect } from 'vitest'; +import { + isTitleCased, + toTitleCase, + checkNameCasing, + checkNameWhitespace, + checkPlaceholderEntries, + findRedundantSizes +} from '../dataQuality'; + +describe('isTitleCased', () => { + it('accepts a title-cased name', () => { + expect(isTitleCased('Sky Blue')).toBe(true); + expect(isTitleCased('Black')).toBe(true); + }); + + it('ignores minor words', () => { + expect(isTitleCased('Black and White')).toBe(true); + }); + + it('rejects a lowercase name', () => { + expect(isTitleCased('translucent blue')).toBe(false); + }); + + it('is false for a name with no letters', () => { + expect(isTitleCased('#1')).toBe(false); + expect(isTitleCased('')).toBe(false); + }); +}); + +describe('toTitleCase', () => { + it('capitalises each significant word', () => { + expect(toTitleCase('true red')).toBe('True Red'); + expect(toTitleCase('carbon fiber blue')).toBe('Carbon Fiber Blue'); + }); + + it('leaves minor words alone except at the start', () => { + expect(toTitleCase('black and white')).toBe('Black and White'); + expect(toTitleCase('the black')).toBe('The Black'); + }); + + it('preserves existing capitals and internal punctuation', () => { + expect(toTitleCase('PLA silk')).toBe('PLA Silk'); + expect(toTitleCase("robin's egg")).toBe("Robin's Egg"); + }); + + it('preserves the original spacing', () => { + expect(toTitleCase('sky blue')).toBe('Sky Blue'); + }); +}); + +describe('checkNameCasing', () => { + it('flags a lowercase name among Title Case siblings — the #451 case', () => { + const issue = checkNameCasing('translucent blue', ['Purple', 'Transparent']); + expect(issue).toEqual({ suggestion: 'Translucent Blue', example: 'Purple' }); + }); + + it('flags the #452 case', () => { + expect(checkNameCasing('true red', ['Red', 'Mint Green'])?.suggestion).toBe('True Red'); + }); + + it('says nothing when no sibling establishes Title Case', () => { + expect(checkNameCasing('black', ['blue', 'red'])).toBeNull(); + }); + + it('says nothing when there are no siblings at all', () => { + expect(checkNameCasing('black', [])).toBeNull(); + }); + + it('says nothing about an already title-cased name', () => { + expect(checkNameCasing('Sky Blue', ['Purple'])).toBeNull(); + }); + + it('does not treat the name itself as its own Title Case sibling', () => { + expect(checkNameCasing('black', ['black'])).toBeNull(); + }); +}); + +describe('checkNameWhitespace', () => { + it('flags the trailing space that shipped in #460', () => { + expect(checkNameWhitespace('Silk ')).toEqual({ + suggestion: 'Silk', + reason: 'has leading or trailing whitespace' + }); + }); + + it('flags repeated internal whitespace', () => { + expect(checkNameWhitespace('Sky Blue')?.suggestion).toBe('Sky Blue'); + expect(checkNameWhitespace('Sky Blue')?.reason).toBe('contains repeated whitespace'); + }); + + it('accepts a clean name', () => { + expect(checkNameWhitespace('Sky Blue')).toBeNull(); + expect(checkNameWhitespace('')).toBeNull(); + }); +}); + +describe('checkPlaceholderEntries', () => { + it('finds the empty certification string from #453', () => { + expect(checkPlaceholderEntries([''])).toEqual([0]); + expect(checkPlaceholderEntries(['CE', '', 'RoHS'])).toEqual([1]); + }); + + it('treats a whitespace-only entry as empty', () => { + expect(checkPlaceholderEntries([' '])).toEqual([0]); + }); + + it('says nothing about a populated or empty array', () => { + expect(checkPlaceholderEntries(['CE'])).toEqual([]); + expect(checkPlaceholderEntries([])).toEqual([]); + }); + + it('is safe on a non-array', () => { + expect(checkPlaceholderEntries(undefined)).toEqual([]); + expect(checkPlaceholderEntries('CE')).toEqual([]); + }); +}); + +describe('findRedundantSizes', () => { + it('flags two identical rows — the #453 case', () => { + expect( + findRedundantSizes([ + { filament_weight: 1000, diameter: 1.75 }, + { filament_weight: 1000, diameter: 1.75 } + ]) + ).toEqual([{ index: 1, duplicateOf: 0 }]); + }); + + it('flags a row that only omits fields — the polylite / sunlu shape', () => { + expect( + findRedundantSizes([ + { filament_weight: 5000, diameter: 1.75, empty_spool_weight: 819 }, + { filament_weight: 5000, diameter: 1.75 } + ]) + ).toEqual([{ index: 1, duplicateOf: 0 }]); + }); + + it('keeps rows distinguished by a GTIN', () => { + expect( + findRedundantSizes([ + { filament_weight: 1000, diameter: 1.75, gtin: '012345678905' }, + { filament_weight: 1000, diameter: 1.75, gtin: '012345678912' } + ]) + ).toEqual([]); + }); + + it('keeps rows distinguished by a purchase link', () => { + expect( + findRedundantSizes([ + { filament_weight: 1000, diameter: 1.75, purchase_links: [{ url: 'a' }] }, + { filament_weight: 1000, diameter: 1.75, purchase_links: [{ url: 'b' }] } + ]) + ).toEqual([]); + }); + + it('does not treat different diameters as duplicates', () => { + expect( + findRedundantSizes([ + { filament_weight: 1000, diameter: 1.75 }, + { filament_weight: 1000, diameter: 2.85 } + ]) + ).toEqual([]); + }); + + it('ignores uuid when comparing — every real duplicate has its own', () => { + expect( + findRedundantSizes([ + { uuid: 'a', filament_weight: 1000, diameter: 1.75 }, + { uuid: 'b', filament_weight: 1000, diameter: 1.75 } + ]) + ).toEqual([{ index: 1, duplicateOf: 0 }]); + }); + + it('ignores empty-string fields, which the form leaves behind', () => { + expect( + findRedundantSizes([ + { filament_weight: 1000, diameter: 1.75 }, + { filament_weight: 1000, diameter: 1.75, article_number: '' } + ]) + ).toEqual([{ index: 1, duplicateOf: 0 }]); + }); + + it('reports each redundant row once, against the row it duplicates', () => { + expect( + findRedundantSizes([ + { filament_weight: 1000, diameter: 1.75 }, + { filament_weight: 1000, diameter: 1.75 }, + { filament_weight: 1000, diameter: 1.75 } + ]) + ).toEqual([ + { index: 1, duplicateOf: 0 }, + { index: 2, duplicateOf: 0 } + ]); + }); + + it('is empty for a single row', () => { + expect(findRedundantSizes([{ filament_weight: 1000, diameter: 1.75 }])).toEqual([]); + expect(findRedundantSizes([])).toEqual([]); + }); +}); diff --git a/webui/src/lib/utils/__tests__/urlSanitizer.test.ts b/webui/src/lib/utils/__tests__/urlSanitizer.test.ts index 6c278dde22..9322ebd64b 100644 --- a/webui/src/lib/utils/__tests__/urlSanitizer.test.ts +++ b/webui/src/lib/utils/__tests__/urlSanitizer.test.ts @@ -4,7 +4,9 @@ import { hasTrackingParams, getHost, rewriteHost, - stripTrackersDeep + stripTrackersDeep, + isStorefrontRoot, + looksLikeProductPage } from '../urlSanitizer'; describe('stripTrackingParams', () => { @@ -166,3 +168,137 @@ describe('stripTrackersDeep', () => { expect(out.name).toBe('Brand'); }); }); + +describe('SimplyPrint recommender params', () => { + // These reached `main` on 3dhojor's datasheet URLs (#461) because nothing matched them. + const suRec = + 'https://3dhojor.com/products/3dprinting-silk-pla-dual-tri-color-filament' + + '?_su_rec=tHGijyqxpJgrAXISK1V6KIs&_su_rec_id=76ac0788-67aa-4a2e-bed7&variant=44298583081060'; + + it('strips _su_rec and _su_rec_id but keeps the product variant selector', () => { + expect(stripTrackingParams(suRec)).toBe( + 'https://3dhojor.com/products/3dprinting-silk-pla-dual-tri-color-filament?variant=44298583081060' + ); + }); + + it('matches any _su_ prefixed key', () => { + expect(hasTrackingParams('https://shop.com/p?_su_anything=1')).toBe(true); + }); +}); + +describe('isStorefrontRoot', () => { + it('rejects a bare origin — the #454 case', () => { + expect(isStorefrontRoot('https://store.bambulab.com/')).toBe(true); + expect(isStorefrontRoot('https://store.bambulab.com')).toBe(true); + }); + + it('rejects a lone landing segment', () => { + expect(isStorefrontRoot('https://example.com/shop')).toBe(true); + expect(isStorefrontRoot('https://example.com/store/')).toBe(true); + }); + + it('accepts a real product URL', () => { + expect(isStorefrontRoot('https://eu.store.bambulab.com/products/pla-cmyk-lithophane')).toBe( + false + ); + // Single-segment product paths are legitimate on some shops (e.g. alza.cz). + expect(isStorefrontRoot('https://www.alza.cz/alzament-abs-1-kg-black-d13015343.htm')).toBe( + false + ); + }); + + it('rejects a URL equal to the selected store’s storefront, however deep', () => { + expect(isStorefrontRoot('https://example.com/eu/shop', 'https://example.com/eu/shop')).toBe( + true + ); + expect( + isStorefrontRoot('https://example.com/eu/shop/products/x', 'https://example.com/eu/shop') + ).toBe(false); + }); + + it('rejects a URL equal to the brand website', () => { + expect(isStorefrontRoot('https://brand.com/en/', null, 'https://brand.com/en')).toBe(true); + }); + + it('ignores tracking query params and fragments when comparing', () => { + expect(isStorefrontRoot('https://store.bambulab.com/?utm_source=x#top')).toBe(true); + }); + + it('accepts a product identified by the query string rather than the path', () => { + // Older shop software (OpenCart, PrestaShop) routes products through `index.php`. + expect( + isStorefrontRoot('https://shop.example.com/index.php?route=product/product&product_id=123') + ).toBe(false); + expect(isStorefrontRoot('https://example.com/shop?p=4711')).toBe(false); + // Including when the path is exactly the store's own front page. + expect( + isStorefrontRoot('https://example.com/eu/shop?product_id=9', 'https://example.com/eu/shop') + ).toBe(false); + }); + + it('is false for an unparseable value', () => { + expect(isStorefrontRoot('')).toBe(false); + expect(isStorefrontRoot('not a url at all')).toBe(false); + }); +}); + +describe('looksLikeProductPage', () => { + it('flags a shop product path', () => { + expect(looksLikeProductPage('https://3dhojor.com/products/silk-pla')).toBe(true); + expect(looksLikeProductPage('https://www.amazon.de/-/da/dp/B0FB93XQLM')).toBe(true); + }); + + it('flags any URL carrying a variant selector', () => { + expect(looksLikeProductPage('https://primacreator.com/x?variant=48794154')).toBe(true); + }); + + it('accepts a document URL', () => { + expect(looksLikeProductPage('https://brand.com/docs/pla-tds.pdf')).toBe(false); + expect(looksLikeProductPage('https://brand.com/support/technical-data-sheets')).toBe(false); + }); + + it('does not flag a PDF that happens to sit under /products/', () => { + expect(looksLikeProductPage('https://brand.com/products/pla/tds.pdf')).toBe(false); + }); + + it('is false for empty input', () => { + expect(looksLikeProductPage('')).toBe(false); + }); +}); + +describe('Amazon /ref= path breadcrumb', () => { + // #408 shipped this and a maintainer shortened it by hand in review. + const dirty = + 'https://www.amazon.de/-/da/AzureFilm-PLA-filament-3D-printer-praecision-prototyper/dp/B0FB93XQLM/ref=sr_1_6'; + + it('truncates the path at the /ref= segment', () => { + expect(stripTrackingParams(dirty)).toBe( + 'https://www.amazon.de/-/da/AzureFilm-PLA-filament-3D-printer-praecision-prototyper/dp/B0FB93XQLM' + ); + expect(hasTrackingParams(dirty)).toBe(true); + }); + + it('is idempotent', () => { + const once = stripTrackingParams(dirty); + expect(stripTrackingParams(once)).toBe(once); + }); + + it('strips the breadcrumb and the query together', () => { + expect( + stripTrackingParams('https://www.amazon.com/x/dp/B01/ref=sr_1_1?qid=1&sr=8-1') + ).toBe('https://www.amazon.com/x/dp/B01'); + }); + + it('leaves non-Amazon hosts alone', () => { + // Only Amazon uses `/ref=` this way; elsewhere it could be a real path. + expect(stripTrackingParams('https://shop.x.com/ref=partner')).toBe( + 'https://shop.x.com/ref=partner' + ); + }); + + it('leaves an Amazon URL with no breadcrumb untouched', () => { + expect(stripTrackingParams('https://www.amazon.de/-/da/dp/B0FB93XQLM')).toBe( + 'https://www.amazon.de/-/da/dp/B0FB93XQLM' + ); + }); +}); diff --git a/webui/src/lib/utils/dataQuality.ts b/webui/src/lib/utils/dataQuality.ts new file mode 100644 index 0000000000..4dfef25d8b --- /dev/null +++ b/webui/src/lib/utils/dataQuality.ts @@ -0,0 +1,172 @@ +/** + * Data-quality checks, mirrored from `ofd/validation/data_quality.py`. + * + * These are the small, repetitive defects reviewers kept fixing by hand on webui + * submissions after PR #405 — lowercase colour names among Title Case siblings (#451, + * #452), placeholder empty strings (#453), spool rows that duplicate an earlier row + * (#453), names with stray whitespace (#460). The Python module is the authority and + * runs in CI; this file is what turns each rule into an inline "Fix" hint at entry + * time, so a contributor never has to be told in review. Keep the two in lockstep. + * + * Every check is pure and returns `null` when there is nothing to say, so a form can + * render it as `{#if issue}`. None of them mutate the value — the caller applies the + * suggested fix only when the user presses the button. + * + * Fiber-trait detection is not repeated here: `fiberTraitSuggestions.ts` already + * mirrors the same Python detector and is wired into `VariantForm`. + */ + +/** Words that legitimately stay lowercase inside a title-cased display name. */ +const MINOR_WORDS = new Set([ + 'and', + 'of', + 'the', + 'with', + 'de', + 'w/', + 'in', + 'on', + 'for', + 'a', + 'an' +]); + +/** True when every significant word starts with an uppercase letter. */ +export function isTitleCased(name: string): boolean { + const words = name.split(/\s+/).filter((w) => w && /[a-zA-Z]/.test(w[0])); + if (words.length === 0) return false; + return words.every((w) => MINOR_WORDS.has(w.toLowerCase()) || w[0] === w[0].toUpperCase()); +} + +function isAllLower(name: string): boolean { + return !!name && name === name.toLowerCase() && /[a-zA-Z]/.test(name); +} + +/** Title-case a display name, leaving minor words and existing capitals alone. */ +export function toTitleCase(name: string): string { + return name + .split(/(\s+)/) + .map((token, index) => { + if (!token.trim()) return token; + // The first word is capitalised even when it is a minor word. + if (index > 0 && MINOR_WORDS.has(token.toLowerCase())) return token; + return token[0].toUpperCase() + token.slice(1); + }) + .join(''); +} + +/** + * A lowercase name among Title Case siblings. + * + * Casing is house style rather than a hard rule, so this only fires when the entity's + * own siblings establish the convention — exactly how it was caught by hand on #451 + * ("translucent blue" beside "Purple", "Transparent") and #452 ("true red" beside + * "Red", "Mint Green"). Returns the suggested replacement and an example sibling. + */ +export function checkNameCasing( + name: string, + siblingNames: string[] +): { suggestion: string; example: string } | null { + if (!isAllLower(name)) return null; + const example = siblingNames.find((s) => s && s !== name && isTitleCased(s)); + if (!example) return null; + const suggestion = toTitleCase(name); + if (suggestion === name) return null; + return { suggestion, example }; +} + +/** + * Leading/trailing or repeated whitespace in a display name. + * + * #460 created a filament literally named `"Silk "`. The trailing space is invisible + * in review, survives into every downstream consumer, and makes the entity look + * distinct from the `"Silk"` the contributor meant. + */ +export function checkNameWhitespace(name: string): { suggestion: string; reason: string } | null { + if (!name) return null; + const collapsed = name.trim().replace(/\s+/g, ' '); + if (collapsed === name) return null; + return { + suggestion: collapsed, + reason: + name !== name.trim() + ? 'has leading or trailing whitespace' + : 'contains repeated whitespace' + }; +} + +/** + * Empty strings inside an array value. + * + * `certifications: [""]` (#453) reads downstream as "this filament has a + * certification" whose name is blank; an empty array says the true thing. Returns the + * indices to drop. + */ +export function checkPlaceholderEntries(values: unknown): number[] { + if (!Array.isArray(values)) return []; + return values.reduce((indices, value, index) => { + if (typeof value === 'string' && value.trim() === '') indices.push(index); + return indices; + }, []); +} + +/** The `(filament_weight, diameter)` spool identity, mirroring `size_dedupe_key`. */ +function sizeKey(size: Record): string { + return `${size.filament_weight ?? ''}|${size.diameter ?? ''}`; +} + +/** + * Fields that make a spool row worth keeping, ignoring canonical identity. + * + * Blank values say nothing an absent field doesn't, so they must not be what tells two + * rows apart. Kept in step with `_redundant_size_fields` in `ofd/validation/data_quality.py`. + */ +function meaningfulFields(size: Record): Array<[string, unknown]> { + return Object.entries(size).filter( + ([key, value]) => + key !== 'uuid' && + key !== 'moved_from' && + value !== undefined && + value !== null && + !(typeof value === 'string' && value.trim() === '') + ); +} + +/** True when `size` says nothing `earlier` doesn't already say. */ +function isSubsumed(size: Record, earlier: Record): boolean { + if (sizeKey(size) !== sizeKey(earlier)) return false; + const earlierFields = new Map(meaningfulFields(earlier)); + return meaningfulFields(size).every( + ([key, value]) => + earlierFields.has(key) && + JSON.stringify(earlierFields.get(key)) === JSON.stringify(value) + ); +} + +/** + * Spool rows that add nothing over an earlier row. + * + * Spool identity is `(filament_weight, diameter)` — the pairing `merge_sizes` and + * `record_moved_from` use — so two rows sharing it are the same offering unless one + * carries a value the other lacks (a distinct GTIN, article number, purchase link, + * spool geometry). #453 shipped two 1 kg / 1.75 mm rows with nothing between them. + * + * Returns, for each redundant row, its index and the index it duplicates. + */ +export function findRedundantSizes( + sizes: Array> +): Array<{ index: number; duplicateOf: number }> { + const found: Array<{ index: number; duplicateOf: number }> = []; + for (let i = 0; i < sizes.length; i++) { + const size = sizes[i]; + if (!size || typeof size !== 'object') continue; + for (let j = 0; j < i; j++) { + const earlier = sizes[j]; + if (earlier && typeof earlier === 'object' && isSubsumed(size, earlier)) { + found.push({ index: i, duplicateOf: j }); + break; + } + } + } + return found; +} diff --git a/webui/src/lib/utils/deletedStubs.ts b/webui/src/lib/utils/deletedStubs.ts index ba36fe8d45..d6dbe1eed6 100644 --- a/webui/src/lib/utils/deletedStubs.ts +++ b/webui/src/lib/utils/deletedStubs.ts @@ -110,14 +110,18 @@ export interface ChangeProps { hasDescendantChanges: boolean; hasSubmittedChanges: boolean; submittedChangeType: ChangeOperation | undefined; + /** PR number of the submission this entity is in, so the badge can name the review. */ + submittedPrNumber: number | undefined; } -const NO_CHANGES: ChangeProps = { +/** Shared "nothing changed here" props, for callers that skip the lookup entirely. */ +export const NO_CHANGES: ChangeProps = { hasLocalChanges: false, localChangeType: undefined, hasDescendantChanges: false, hasSubmittedChanges: false, - submittedChangeType: undefined + submittedChangeType: undefined, + submittedPrNumber: undefined }; /** @@ -139,6 +143,7 @@ export function getChildChangeProps( localChangeType: change?.operation, hasDescendantChanges: changes.hasDescendantChanges(entityPath), hasSubmittedChanges: !!sub, - submittedChangeType: sub?.change.operation + submittedChangeType: sub?.change.operation, + submittedPrNumber: sub?.entry.prNumber }; } diff --git a/webui/src/lib/utils/entityState.svelte.ts b/webui/src/lib/utils/entityState.svelte.ts index c3249a1256..854d5786d8 100644 --- a/webui/src/lib/utils/entityState.svelte.ts +++ b/webui/src/lib/utils/entityState.svelte.ts @@ -112,16 +112,20 @@ export function createEntityState(config: EntityStateConfig) { return treeHasDescendantChanges(node); }); - // Check if entity has submitted (pending-merge) changes - const hasSubmittedChanges = $derived.by(() => { - if (!trackingEnabled) return false; + // The submission this entity is part of, if any — carries the PR number and URL so the + // page can point at the review rather than just saying "submitted". + const submittedEntry = $derived.by(() => { + if (!trackingEnabled) return undefined; // Access submittedVersion to trigger reactivity on store changes void submittedVersion; const path = config.getEntityPath(); - if (!path) return false; - return submittedStore.has(path); + if (!path) return undefined; + return submittedStore.getChange(path)?.entry; }); + // Check if entity has submitted (pending-merge) changes + const hasSubmittedChanges = $derived(!!submittedEntry); + // Check if entity was locally created (for delete modal messaging) const isLocalCreate = $derived.by(() => { if (!trackingEnabled) return false; @@ -188,6 +192,9 @@ export function createEntityState(config: EntityStateConfig) { get hasSubmittedChanges() { return hasSubmittedChanges; }, + get submittedEntry() { + return submittedEntry; + }, // Duplicate/Paste/Compare modal states get showDuplicateModal() { diff --git a/webui/src/lib/utils/urlSanitizer.ts b/webui/src/lib/utils/urlSanitizer.ts index b3fc985f6d..91290d8492 100644 --- a/webui/src/lib/utils/urlSanitizer.ts +++ b/webui/src/lib/utils/urlSanitizer.ts @@ -62,7 +62,14 @@ const TRACKING_PARAMS = new Set([ 'rnid', 'refinements', '_encoding', - 'pldnsite' + 'pldnsite', + // SimplyPrint storefront recommender context. These reached `main` on 3dhojor's datasheet + // URLs (#461) because nothing matched them — the token is per-session and hundreds of + // characters long. + '_su_rec', + '_su_rec_id', + // Shopify affiliate referral (seen across SUNLU's purchase links). + 'sca_ref' ]); /** Hosts (regex on the lowercased hostname) where the product identity is entirely in the @@ -78,6 +85,7 @@ function isTrackingKey(key: string): boolean { k.startsWith('pk_') || k.startsWith('pd_rd_') || k.startsWith('pf_rd_') || + k.startsWith('_su_') || TRACKING_PARAMS.has(k) ); } @@ -95,6 +103,29 @@ function isTrackingFragment(fragment: string): boolean { return isTrackingKey(firstKey); } +/** + * Amazon puts a tracking breadcrumb in the *path*, not just the query: a trailing + * `/ref=` segment. #408 was submitted as + * `.../AzureFilm-PLA-filament-3D-printer-præcision-prototyper/dp/B0FB93XQLM/ref=sr_1_6`, + * where `sr_1_6` records "search result, page 1, position 6" — and had to be shortened by + * hand in review. + * + * Truncates the path at that segment. The product-name slug before `/dp/` is left + * alone: Amazon ignores it, and it makes the link readable. + */ +function stripAmazonRefPath(base: string): string { + const host = (getHost(base) ?? '').toLowerCase(); + if (!/(^|\.)amazon\.[a-z.]+$/.test(host)) return base; + + const schemeEnd = base.indexOf('://'); + const pathStart = base.indexOf('/', schemeEnd === -1 ? 0 : schemeEnd + 3); + if (pathStart === -1) return base; + + const path = base.slice(pathStart); + const refIndex = path.search(/\/ref=/); + return refIndex === -1 ? base : base.slice(0, pathStart + refIndex); +} + /** * Remove known tracking parameters (plus an empty `?` and tracking-only fragments) from a * URL, preserving the original ordering/encoding of everything kept. Idempotent. Returns the @@ -108,9 +139,12 @@ export function stripTrackingParams(url: string): string { const fragment = hashIdx === -1 ? null : url.slice(hashIdx + 1); const qIdx = beforeFrag.indexOf('?'); - const base = qIdx === -1 ? beforeFrag : beforeFrag.slice(0, qIdx); + const rawBase = qIdx === -1 ? beforeFrag : beforeFrag.slice(0, qIdx); const query = qIdx === -1 ? null : beforeFrag.slice(qIdx + 1); + // Amazon carries tracking in the path, not only the query. + const base = stripAmazonRefPath(rawBase); + let out = base; if (query !== null && !hasDisposableQuery(base)) { @@ -156,6 +190,93 @@ export function stripTrackersDeep(value: T): T { return value; } +/** Path segments that are a shop's landing area rather than a product. */ +const LANDING_SEGMENTS = new Set(['shop', 'store', 'home', 'index.html', 'index.php']); + +/** + * True when a URL carries query parameters that could identify a specific product, + * i.e. any parameter that is not a known tracking key. + */ +function hasIdentifyingQuery(url: string): boolean { + if (!url) return false; + try { + const parsed = new URL(/^[a-z][a-z0-9+.-]*:\/\//i.test(url) ? url : `https://${url}`); + for (const key of parsed.searchParams.keys()) { + if (!isTrackingKey(key)) return true; + } + return false; + } catch { + return false; + } +} + +/** Lowercased `host + path`, without protocol, trailing slash, query or fragment. */ +function hostAndPath(url: string): string | null { + if (!url) return null; + try { + const parsed = new URL(/^[a-z][a-z0-9+.-]*:\/\//i.test(url) ? url : `https://${url}`); + if (!parsed.hostname) return null; + return (parsed.hostname + parsed.pathname).toLowerCase().replace(/\/+$/, ''); + } catch { + return null; + } +} + +/** + * True when a purchase link points at a shop's front door rather than at the product. + * + * PR #454 was submitted with `https://store.bambulab.com/` as the purchase link for a + * specific colour, and a maintainer had to go find the real product page by hand. A + * homepage identifies neither the filament nor the colour, so it is worse than no link: + * downstream consumers treat it as "buy this here" and send people to a catalogue. + * + * `storefrontUrl` (from the selected store) and `brandWebsite` are compared too, since a + * shop's canonical entry point is not always the bare origin (e.g. `example.com/shop`). + */ +export function isStorefrontRoot( + url: string, + storefrontUrl?: string | null, + brandWebsite?: string | null +): boolean { + const target = hostAndPath(url); + if (!target) return false; + + // Older shop software routes products through the query string rather than the path + // (`/index.php?route=product/product&product_id=123`). That names a product, so the + // path alone says nothing — tracking params excepted, since those name no product. + if (hasIdentifyingQuery(url)) return false; + + // A bare origin, or one whose only path segment is a landing area. + const segments = target.split('/').slice(1).filter(Boolean); + if (segments.length === 0) return true; + if (segments.length === 1 && LANDING_SEGMENTS.has(segments[0])) return true; + + // Or exactly the store's / brand's own front page, however deep that happens to be. + for (const candidate of [storefrontUrl, brandWebsite]) { + const front = candidate ? hostAndPath(candidate) : null; + if (front && front === target) return true; + } + + return false; +} + +/** + * True when a URL looks like a storefront product page rather than a document. + * + * `data_sheet_url` / `safety_sheet_url` are meant to reach a TDS/SDS. 3dhojor's merged + * filament (#461) has both fields pointing at the same Shopify product page — a shop listing + * is not a datasheet, and a `?variant=` selector makes it colour-specific on a field that + * describes the whole filament. + */ +export function looksLikeProductPage(url: string): boolean { + if (!url) return false; + const target = hostAndPath(url); + if (!target) return false; + if (/\.(pdf|docx?|xlsx?)$/.test(target)) return false; + if (/[?&]variant=/i.test(url)) return true; + return /\/(products?|collections|item|dp|sku)\//.test(target + '/'); +} + /** * Extract the hostname (e.g. `shop.polymaker.com`, no port) from an absolute or * protocol-less URL. Returns null when unparseable. diff --git a/webui/src/routes/api/__tests__/anon-submit.test.ts b/webui/src/routes/api/__tests__/anon-submit.test.ts index 198740a1b8..0dd4cd1eee 100644 --- a/webui/src/routes/api/__tests__/anon-submit.test.ts +++ b/webui/src/routes/api/__tests__/anon-submit.test.ts @@ -14,6 +14,8 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; const mocks = vi.hoisted(() => ({ isAnonBotEnabled: vi.fn(() => true), createAnonPR: vi.fn(), + amendAnonPR: vi.fn(), + getSubmission: vi.fn(), runCloudValidation: vi.fn(), sendWebhook: vi.fn(), trackSubmission: vi.fn(), @@ -24,13 +26,17 @@ const mocks = vi.hoisted(() => ({ vi.mock('$lib/server/anonBot', () => ({ isAnonBotEnabled: mocks.isAnonBotEnabled, - createAnonPR: mocks.createAnonPR + createAnonPR: mocks.createAnonPR, + amendAnonPR: mocks.amendAnonPR })); vi.mock('$lib/server/cloudValidator', () => ({ runCloudValidation: mocks.runCloudValidation })); vi.mock('$lib/server/webhooks', () => ({ sendWebhook: mocks.sendWebhook })); -vi.mock('$lib/server/submissionStore', () => ({ trackSubmission: mocks.trackSubmission })); +vi.mock('$lib/server/submissionStore', () => ({ + trackSubmission: mocks.trackSubmission, + getSubmission: mocks.getSubmission +})); vi.mock('$lib/server/auth', () => ({ getSimplyPrintToken: mocks.getSimplyPrintToken, getSimplyPrintUser: mocks.getSimplyPrintUser @@ -72,6 +78,173 @@ describe('POST /api/anon/submit', () => { prNumber: 42, prUrl: 'https://github.com/foo/bar/pull/42' }); + mocks.amendAnonPR.mockResolvedValue({ + success: true, + amended: true, + prNumber: 459, + prUrl: 'https://github.com/foo/bar/pull/459' + }); + }); + + // --- Amending an open submission --------------------------------------------------- + // + // Contributors submit, keep editing, and submit again minutes later. Without this the + // second batch opens a competing PR on the same paths (#459/#460/#461 merged out of order + // and left an orphan filament in `main`). + + const OPEN_SUBMISSION = { + uuid: 'sub-459', + prNumber: 459, + prUrl: 'https://github.com/foo/bar/pull/459', + createdAt: new Date().toISOString(), + status: 'open' as const, + email: 'u@example.com', + changeData: JSON.stringify({ changes: [{ entity: { path: 'brands/3dhojor' } }] }) + }; + + describe('amendUuid', () => { + it('adds to the open PR instead of creating a new one', async () => { + mocks.getSubmission.mockReturnValue(OPEN_SUBMISSION); + + const res: any = await POST( + makeEvent({ changes: [{ id: 'c2' }], amendUuid: 'sub-459' }) + ); + + expect(res.status).toBe(200); + expect(res.body.amended).toBe(true); + expect(res.body.prNumber).toBe(459); + expect(res.body.uuid).toBe('sub-459'); + expect(mocks.createAnonPR).not.toHaveBeenCalled(); + expect(mocks.amendAnonPR).toHaveBeenCalledOnce(); + }); + + it('passes earlier batches through so the PR body describes the whole submission', async () => { + mocks.getSubmission.mockReturnValue(OPEN_SUBMISSION); + + await POST(makeEvent({ changes: [{ id: 'c2' }], amendUuid: 'sub-459' })); + + const arg = mocks.amendAnonPR.mock.calls[0][0]; + expect(arg.changes).toEqual([{ id: 'c2' }]); + expect(arg.allChanges).toEqual([{ entity: { path: 'brands/3dhojor' } }, { id: 'c2' }]); + expect(arg.prNumber).toBe(459); + }); + + it('re-records the submission with the combined change set', async () => { + mocks.getSubmission.mockReturnValue(OPEN_SUBMISSION); + + await POST(makeEvent({ changes: [{ id: 'c2' }], amendUuid: 'sub-459' })); + + const [uuid, prNumber, , changeData] = mocks.trackSubmission.mock.calls[0]; + expect(uuid).toBe('sub-459'); + expect(prNumber).toBe(459); + expect(JSON.parse(changeData).changes).toHaveLength(2); + }); + + it('still validates the new batch before touching the branch', async () => { + mocks.getSubmission.mockReturnValue(OPEN_SUBMISSION); + mocks.runCloudValidation.mockImplementation(async (job: any) => { + job.status = 'complete'; + job.result = { is_valid: false, errors: [{ message: 'bad' }] }; + }); + + const res: any = await POST( + makeEvent({ changes: [{ id: 'c2' }], amendUuid: 'sub-459' }) + ); + + expect(res.status).toBe(422); + expect(mocks.amendAnonPR).not.toHaveBeenCalled(); + }); + + it('returns 404 for an unknown submission', async () => { + mocks.getSubmission.mockReturnValue(undefined); + const res: any = await POST( + makeEvent({ changes: [{ id: 'c' }], amendUuid: 'nope' }) + ); + expect(res.status).toBe(404); + expect(mocks.amendAnonPR).not.toHaveBeenCalled(); + }); + + it('refuses to amend another account’s submission', async () => { + // A submission UUID is printed in the PR body, so it is not a secret. + mocks.getSubmission.mockReturnValue({ ...OPEN_SUBMISSION, email: 'someone@else.com' }); + const res: any = await POST( + makeEvent({ changes: [{ id: 'c' }], amendUuid: 'sub-459' }) + ); + expect(res.status).toBe(403); + expect(mocks.amendAnonPR).not.toHaveBeenCalled(); + }); + + it('refuses to amend when the caller has no email to match against', async () => { + mocks.getSimplyPrintUser.mockResolvedValue({ id: 1, email: null }); + mocks.getSubmission.mockReturnValue(OPEN_SUBMISSION); + const res: any = await POST( + makeEvent({ changes: [{ id: 'c' }], amendUuid: 'sub-459' }) + ); + expect(res.status).toBe(403); + }); + + it('opens a new PR when the submission has already merged', async () => { + // Same fallback as when `amendAnonPR` discovers the merge one layer down: the + // contributor's work lands either way, so a dead-end error would be gratuitous. + mocks.getSubmission.mockReturnValue({ ...OPEN_SUBMISSION, status: 'merged' }); + const res: any = await POST( + makeEvent({ changes: [{ id: 'c' }], amendUuid: 'sub-459' }) + ); + expect(res.status).toBe(200); + expect(mocks.amendAnonPR).not.toHaveBeenCalled(); + expect(mocks.createAnonPR).toHaveBeenCalledOnce(); + expect(res.body.amended).toBe(false); + expect(res.body.amendFellBackFrom).toBe('sub-459'); + }); + + it('allows amending a submission with changes requested', async () => { + mocks.getSubmission.mockReturnValue({ ...OPEN_SUBMISSION, status: 'changes_requested' }); + const res: any = await POST( + makeEvent({ changes: [{ id: 'c' }], amendUuid: 'sub-459' }) + ); + expect(res.status).toBe(200); + expect(mocks.amendAnonPR).toHaveBeenCalledOnce(); + }); + + it('falls back to a new PR when the branch is gone by the time we push', async () => { + // The PR can merge between the client's last status poll and this request. + mocks.getSubmission.mockReturnValue(OPEN_SUBMISSION); + mocks.amendAnonPR.mockResolvedValue({ + success: false, + retryAsNew: true, + error: 'That submission is no longer open.' + }); + + const res: any = await POST( + makeEvent({ changes: [{ id: 'c2' }], amendUuid: 'sub-459' }) + ); + + expect(res.status).toBe(200); + expect(res.body.amended).toBe(false); + expect(res.body.prNumber).toBe(42); + expect(res.body.amendFellBackFrom).toBe('sub-459'); + expect(res.body.uuid).not.toBe('sub-459'); + expect(mocks.createAnonPR).toHaveBeenCalledOnce(); + }); + + it('does not fall back on an ordinary amend failure', async () => { + mocks.getSubmission.mockReturnValue(OPEN_SUBMISSION); + mocks.amendAnonPR.mockResolvedValue({ success: false, error: 'GitHub is down' }); + + const res: any = await POST( + makeEvent({ changes: [{ id: 'c2' }], amendUuid: 'sub-459' }) + ); + + expect(res.status).toBe(500); + expect(mocks.createAnonPR).not.toHaveBeenCalled(); + }); + + it('ignores an empty amendUuid and submits normally', async () => { + const res: any = await POST(makeEvent({ changes: [{ id: 'c' }], amendUuid: '' })); + expect(res.status).toBe(200); + expect(res.body.amended).toBe(false); + expect(mocks.createAnonPR).toHaveBeenCalledOnce(); + }); }); it('returns 404 when the bot feature flag is off', async () => { diff --git a/webui/src/routes/api/__tests__/submissions-status.test.ts b/webui/src/routes/api/__tests__/submissions-status.test.ts new file mode 100644 index 0000000000..7775135702 --- /dev/null +++ b/webui/src/routes/api/__tests__/submissions-status.test.ts @@ -0,0 +1,111 @@ +/** + * Tests for /api/submissions/status — the client's merge-state reconciliation. + * + * The endpoint's job is twofold: report each PR's real state, and hand back GitHub's + * `merged_at` so the client can anchor overlay eviction to the merge rather than to + * whenever it happened to ask. A locally-cached 'merged' must therefore still hit + * GitHub for the timestamp; only 'closed' can be answered from the cache alone. + */ +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const mocks = vi.hoisted(() => ({ + getPullRequest: vi.fn(), + getInstallationToken: vi.fn(async () => 'tok'), + getUuidByPrNumber: vi.fn((n: number) => `uuid-${n}`), + getSubmission: vi.fn(), + updateStatus: vi.fn() +})); + +vi.mock('$lib/server/github', () => ({ getPullRequest: mocks.getPullRequest })); +vi.mock('$lib/server/githubApp', () => ({ getInstallationToken: mocks.getInstallationToken })); +vi.mock('$lib/server/submissionStore', () => ({ + getUuidByPrNumber: mocks.getUuidByPrNumber, + getSubmission: mocks.getSubmission, + updateStatus: mocks.updateStatus +})); +vi.mock('$env/dynamic/private', () => ({ + env: { GITHUB_UPSTREAM_OWNER: 'owner', GITHUB_UPSTREAM_REPO: 'repo' } +})); +vi.mock('@sveltejs/kit', () => ({ + json: (data: any, init?: { status?: number }) => ({ + status: init?.status ?? 200, + body: data + }) +})); + +import { POST } from '../submissions/status/+server'; + +const makeEvent = (body: any): any => ({ request: { json: async () => body } }); + +const MERGED_AT = '2026-08-18T22:10:00Z'; + +beforeEach(() => { + vi.clearAllMocks(); + mocks.getInstallationToken.mockResolvedValue('tok'); + mocks.getUuidByPrNumber.mockImplementation((n: number) => `uuid-${n}`); + mocks.getSubmission.mockReturnValue(undefined); +}); + +describe('POST /api/submissions/status', () => { + it('rejects a non-array prNumbers', async () => { + const res: any = await POST(makeEvent({ prNumbers: 'nope' })); + expect(res.status).toBe(400); + }); + + it('returns the upstream state and merge time for an unknown PR', async () => { + mocks.getPullRequest.mockResolvedValue({ + merged: true, + state: 'closed', + merged_at: MERGED_AT + }); + const res: any = await POST(makeEvent({ prNumbers: [459] })); + expect(res.body.statuses[459]).toBe('merged'); + expect(res.body.mergedAt[459]).toBe(MERGED_AT); + expect(mocks.updateStatus).toHaveBeenCalledWith('uuid-459', 'merged'); + }); + + it('still fetches merged_at for a PR already cached as merged', async () => { + // The store has no merge-time column, so short-circuiting here would leave the + // client falling back to "now" and over-extending the submitted overlay. + mocks.getSubmission.mockReturnValue({ status: 'merged' }); + mocks.getPullRequest.mockResolvedValue({ + merged: true, + state: 'closed', + merged_at: MERGED_AT + }); + const res: any = await POST(makeEvent({ prNumbers: [459] })); + expect(mocks.getPullRequest).toHaveBeenCalledOnce(); + expect(res.body.statuses[459]).toBe('merged'); + expect(res.body.mergedAt[459]).toBe(MERGED_AT); + // Already terminal in the store — nothing new to persist. + expect(mocks.updateStatus).not.toHaveBeenCalled(); + }); + + it('answers a cached closed PR without calling GitHub', async () => { + mocks.getSubmission.mockReturnValue({ status: 'closed' }); + const res: any = await POST(makeEvent({ prNumbers: [460] })); + expect(mocks.getPullRequest).not.toHaveBeenCalled(); + expect(res.body.statuses[460]).toBe('closed'); + }); + + it('keeps a known merge when GitHub is unreachable', async () => { + mocks.getSubmission.mockReturnValue({ status: 'merged' }); + mocks.getPullRequest.mockRejectedValue(new Error('502')); + const res: any = await POST(makeEvent({ prNumbers: [459] })); + expect(res.body.statuses[459]).toBe('merged'); + expect(res.body.mergedAt[459]).toBeUndefined(); + }); + + it('reports unknown when an uncached PR cannot be fetched', async () => { + mocks.getPullRequest.mockResolvedValue(null); + const res: any = await POST(makeEvent({ prNumbers: [999] })); + expect(res.body.statuses[999]).toBe('unknown'); + }); + + it('preserves a changes_requested status for a still-open PR', async () => { + mocks.getSubmission.mockReturnValue({ status: 'changes_requested' }); + mocks.getPullRequest.mockResolvedValue({ merged: false, state: 'open' }); + const res: any = await POST(makeEvent({ prNumbers: [461] })); + expect(res.body.statuses[461]).toBe('changes_requested'); + }); +}); diff --git a/webui/src/routes/api/anon/submit/+server.ts b/webui/src/routes/api/anon/submit/+server.ts index 3159bd3fc5..e9be6a6fa9 100644 --- a/webui/src/routes/api/anon/submit/+server.ts +++ b/webui/src/routes/api/anon/submit/+server.ts @@ -5,10 +5,10 @@ */ import { json } from '@sveltejs/kit'; import type { RequestHandler } from './$types'; -import { isAnonBotEnabled, createAnonPR } from '$lib/server/anonBot'; +import { isAnonBotEnabled, createAnonPR, amendAnonPR } from '$lib/server/anonBot'; import { runCloudValidation } from '$lib/server/cloudValidator'; import { sendWebhook } from '$lib/server/webhooks'; -import { trackSubmission } from '$lib/server/submissionStore'; +import { trackSubmission, getSubmission } from '$lib/server/submissionStore'; import { getSimplyPrintToken, getSimplyPrintUser } from '$lib/server/auth'; import { checkRateLimit } from '$lib/server/rateLimit'; import type { Job } from '$lib/server/jobManager'; @@ -56,16 +56,53 @@ export const POST: RequestHandler = async ({ request, cookies, getClientAddress return json({ error: 'Invalid JSON' }, { status: 400 }); } - const { changes, images, title, description } = body; + const { changes, images, title, description, amendUuid } = body; if (!changes || !Array.isArray(changes) || changes.length === 0) { return json({ error: 'No changes to submit' }, { status: 400 }); } - // 5. Generate UUID - const uuid = crypto.randomUUID(); + // 5. Resolve an amend target, if the client asked to add to an open submission. + // + // Contributors regularly submit, keep editing, and submit again minutes later; without + // this they get competing PRs on the same paths (#459/#460/#461 merged out of order and + // left an orphan filament in `main`). Adding to the open PR keeps it one review. + // + // A submission UUID is not a secret — it is printed in the PR body — so amending is + // gated on the submission having been made by *this* SimplyPrint account. + let amendTarget: { uuid: string; prNumber: number; previousChanges: any[] } | null = null; + // Set when an amend was asked for but the PR turned out to be unamendable, so the + // response can explain why a new PR number came back. The same fallback is applied + // whether the closed PR is noticed here or by `amendAnonPR` a few steps down. + let amendUuidRetired: string | undefined; + if (typeof amendUuid === 'string' && amendUuid.length > 0) { + const existing = getSubmission(amendUuid); + if (!existing) { + return json({ error: 'Unknown submission to add to.' }, { status: 404 }); + } + if (!spUser.email || existing.email !== spUser.email) { + return json({ error: 'That submission belongs to someone else.' }, { status: 403 }); + } + if (existing.status !== 'open' && existing.status !== 'changes_requested') { + // Already merged or closed. Fall through to a fresh PR rather than erroring out — + // the contributor's work is the same either way, and the alternative is a dead end. + amendUuidRetired = amendUuid; + } else { + // The previous batches, so the rewritten PR body describes the whole submission. + let previousChanges: any[] = []; + try { + previousChanges = JSON.parse(existing.changeData || '{}').changes ?? []; + } catch { + // A malformed cached payload only costs us the earlier bullets in the PR body. + } + amendTarget = { uuid: amendUuid, prNumber: existing.prNumber, previousChanges }; + } + } - // 6. Run validation synchronously + // 6. Generate UUID (an amend reuses the submission's existing one) + const uuid = amendTarget?.uuid ?? crypto.randomUUID(); + + // 7. Run validation synchronously const validationJob: Job = { id: `sp-validation-${uuid}`, type: 'validation', @@ -93,39 +130,74 @@ export const POST: RequestHandler = async ({ request, cookies, getClientAddress ); } - // 7. Create PR via bot with SimplyPrint user attribution + // 8. Create or amend the PR via the bot, attributed to the SimplyPrint user try { - const result = await createAnonPR({ - uuid, - changes, - images: images || {}, - title, - description - }); + let result = amendTarget + ? await amendAnonPR({ + uuid, + prNumber: amendTarget.prNumber, + changes, + allChanges: [...amendTarget.previousChanges, ...changes], + images: images || {}, + title, + description + }) + : await createAnonPR({ uuid, changes, images: images || {}, title, description }); + + // The PR may have merged or had its branch deleted between the client's last status + // poll and now. Falling back to a new PR is better than losing the contributor's work. + let submissionUuid = uuid; + if (!result.success && 'retryAsNew' in result && result.retryAsNew) { + amendUuidRetired = amendTarget!.uuid; + amendTarget = null; + submissionUuid = crypto.randomUUID(); + result = await createAnonPR({ + uuid: submissionUuid, + changes, + images: images || {}, + title, + description + }); + } if (!result.success) { return json({ error: result.error || 'Failed to create PR' }, { status: 500 }); } - // 8. Track submission (store email for lifecycle notifications) - const changeData = JSON.stringify({ changes, images: images || {} }); - trackSubmission(uuid, result.prNumber!, result.prUrl!, changeData, spUser.email || undefined); + // 9. Track submission (store email for lifecycle notifications). An amend re-records + // the submission under the same UUID with the combined change set, so a later amend + // can still describe every batch. + const trackedChanges = amendTarget + ? [...amendTarget.previousChanges, ...changes] + : changes; + const changeData = JSON.stringify({ changes: trackedChanges, images: images || {} }); + trackSubmission( + submissionUuid, + result.prNumber!, + result.prUrl!, + changeData, + spUser.email || undefined + ); - // 9. Fire "submitted" webhook (fire-and-forget) + // 10. Fire "submitted" webhook (fire-and-forget) sendWebhook({ event: 'submitted', - uuid, + uuid: submissionUuid, prNumber: result.prNumber!, prUrl: result.prUrl!, timestamp: new Date().toISOString() }); - // 10. Return result + // 11. Return result return json({ success: true, - uuid, + uuid: submissionUuid, prUrl: result.prUrl, - prNumber: result.prNumber + prNumber: result.prNumber, + amended: result.amended === true, + // Set when the client asked to amend but the PR had already closed, so the UI can + // explain why it got a new PR number back. + amendFellBackFrom: amendUuidRetired }); } catch (error: any) { console.error('Bot PR creation error:', error); diff --git a/webui/src/routes/api/submissions/status/+server.ts b/webui/src/routes/api/submissions/status/+server.ts index 166b220a79..7935012820 100644 --- a/webui/src/routes/api/submissions/status/+server.ts +++ b/webui/src/routes/api/submissions/status/+server.ts @@ -54,23 +54,30 @@ export const POST: RequestHandler = async ({ request }) => { } const statuses: Record = {}; + // GitHub's merge timestamps, so the client can tell when the nightly dataset rebuild will + // have published a merged submission (see $lib/config/datasetSchedule.ts). The client falls + // back to "now" when a timestamp is missing, which over-extends the overlay, so a merged PR + // is always asked about upstream even when we already know it merged. + const mergedAt: Record = {}; await Promise.all( prNumbers.map(async (prNumber) => { - // Trust a terminal status already recorded locally — no GitHub call needed. const uuid = getUuidByPrNumber(prNumber); const cached = uuid ? getSubmission(uuid) : undefined; - if (cached && (cached.status === 'merged' || cached.status === 'closed')) { - statuses[prNumber] = cached.status; + // A locally-recorded 'closed' is the whole answer — nothing else to learn from GitHub. + // 'merged' still needs the upstream call for `merged_at`; the store has no such column. + if (cached?.status === 'closed') { + statuses[prNumber] = 'closed'; return; } try { const pr = await getPullRequest(token, owner, repo, prNumber); if (!pr) { - statuses[prNumber] = 'unknown'; + statuses[prNumber] = cached?.status === 'merged' ? 'merged' : 'unknown'; return; } + if (pr.merged_at) mergedAt[prNumber] = pr.merged_at; let status: Status; if (pr.merged) status = 'merged'; @@ -85,10 +92,11 @@ export const POST: RequestHandler = async ({ request }) => { statuses[prNumber] = status; } catch (err) { console.warn(`[Submissions] Failed to check PR #${prNumber}:`, (err as Error).message); - statuses[prNumber] = 'unknown'; + // Don't downgrade a merge we already know about just because GitHub was unreachable. + statuses[prNumber] = cached?.status === 'merged' ? 'merged' : 'unknown'; } }) ); - return json({ statuses }); + return json({ statuses, mergedAt }); }; diff --git a/webui/src/routes/brands/+page.svelte b/webui/src/routes/brands/+page.svelte index e24b3bce5c..b07e31ec1e 100644 --- a/webui/src/routes/brands/+page.svelte +++ b/webui/src/routes/brands/+page.svelte @@ -235,6 +235,7 @@ hasLocalChanges={changeProps.hasLocalChanges} localChangeType={changeProps.localChangeType} hasSubmittedChanges={changeProps.hasSubmittedChanges} + submittedPrNumber={changeProps.submittedPrNumber} submittedChangeType={changeProps.submittedChangeType} entityType="brand" onCopy={() => brandCopy.request(brand, `brands/${brand.slug ?? brand.id}`)} diff --git a/webui/src/routes/brands/[brand]/+page.svelte b/webui/src/routes/brands/[brand]/+page.svelte index 29689628d5..de428d056a 100644 --- a/webui/src/routes/brands/[brand]/+page.svelte +++ b/webui/src/routes/brands/[brand]/+page.svelte @@ -7,7 +7,7 @@ import { BrandForm, MaterialForm } from '$lib/components/forms'; import { BackButton } from '$lib/components/actions'; import { DataDisplay } from '$lib/components/layout'; - import { Logo, EntityDetails, EntityCard, ChildListPanel } from '$lib/components/entity'; + import { Logo, EntityDetails, EntityCard, ChildListPanel, SubmittedBanner, InFlightHint } from '$lib/components/entity'; import { createMessageHandler } from '$lib/utils/messageHandler.svelte'; import { createEntityState } from '$lib/utils/entityState.svelte'; import { createDeleteFlow } from '$lib/utils/useDeleteFlow.svelte'; @@ -365,8 +365,8 @@ {#if entityState.hasLocalChanges} - {:else if entityState.hasSubmittedChanges} - + {:else if entityState.submittedEntry} + {/if} {#if messageHandler.message} @@ -425,6 +425,7 @@ hasLocalChanges={changeProps.hasLocalChanges} localChangeType={changeProps.localChangeType} hasSubmittedChanges={changeProps.hasSubmittedChanges} + submittedPrNumber={changeProps.submittedPrNumber} submittedChangeType={changeProps.submittedChangeType} entityType="material" onCopy={() => materialCopy.request(material, materialPath)} @@ -509,6 +510,7 @@ {#if createMaterialError} {/if} + {#if materialSchema} - {:else if entityState.hasSubmittedChanges} - + {:else if entityState.submittedEntry} + {/if} {#if messageHandler.message} @@ -367,6 +367,7 @@ badge={filament.discontinued ? { text: 'Discontinued', color: 'red' } : undefined} hasLocalChanges={changeProps.hasLocalChanges} localChangeType={changeProps.localChangeType} hasSubmittedChanges={changeProps.hasSubmittedChanges} submittedChangeType={changeProps.submittedChangeType} + submittedPrNumber={changeProps.submittedPrNumber} entityType="filament" onCopy={() => filamentCopy.request(filament, filamentPath)} onDuplicate={() => filamentDuplicate.request(filament)} @@ -430,6 +431,7 @@ { createError = null; entityState.closeCreate(); }} maxWidth="5xl"> {#if createError}{/if} +
diff --git a/webui/src/routes/brands/[brand]/[material]/[filament]/+page.svelte b/webui/src/routes/brands/[brand]/[material]/[filament]/+page.svelte index 3ed3ae46e9..1986d7ce3c 100644 --- a/webui/src/routes/brands/[brand]/[material]/[filament]/+page.svelte +++ b/webui/src/routes/brands/[brand]/[material]/[filament]/+page.svelte @@ -5,7 +5,7 @@ import { Modal, MessageBanner, DeleteEntityModal, Button, EntityActionDropdown, CloudCompareModal, DuplicateOptionsModal } from '$lib/components/ui'; import { BackButton } from '$lib/components/actions'; import { DataDisplay } from '$lib/components/layout'; - import { EntityDetails, EntityCard, SlicerSettingsDisplay, CertificationsDisplay, ChildListPanel } from '$lib/components/entity'; + import { EntityDetails, EntityCard, SlicerSettingsDisplay, CertificationsDisplay, ChildListPanel, SubmittedBanner, InFlightHint } from '$lib/components/entity'; import { FilamentForm, VariantForm } from '$lib/components/forms'; import { createMessageHandler } from '$lib/utils/messageHandler.svelte'; import { createEntityState } from '$lib/utils/entityState.svelte'; @@ -334,8 +334,8 @@ {#if entityState.hasLocalChanges} - {:else if entityState.hasSubmittedChanges} - + {:else if entityState.submittedEntry} + {/if} {#if messageHandler.message} @@ -402,6 +402,7 @@ secondaryInfo={sizesInfo} hasLocalChanges={changeProps.hasLocalChanges} localChangeType={changeProps.localChangeType} hasSubmittedChanges={changeProps.hasSubmittedChanges} submittedChangeType={changeProps.submittedChangeType} + submittedPrNumber={changeProps.submittedPrNumber} entityType="variant" onCopy={() => variantCopy.request(variant, variantPath)} onDuplicate={() => variantDuplicate.request(variant)} @@ -459,5 +460,6 @@ { createError = null; entityState.closeCreate(); }} maxWidth="5xl" height="3/4"> {#if createError}{/if} - + + v.name)} onSubmit={handleCreateVariant} saving={entityState.creating} /> diff --git a/webui/src/routes/brands/[brand]/[material]/[filament]/[variant]/+page.svelte b/webui/src/routes/brands/[brand]/[material]/[filament]/[variant]/+page.svelte index ae98885aa4..460b00fc38 100644 --- a/webui/src/routes/brands/[brand]/[material]/[filament]/[variant]/+page.svelte +++ b/webui/src/routes/brands/[brand]/[material]/[filament]/[variant]/+page.svelte @@ -3,6 +3,7 @@ import { goto } from '$app/navigation'; import type { Variant, Store } from '$lib/types/database'; import { Modal, MessageBanner, DeleteEntityModal, Button, EntityActionDropdown, CloudCompareModal } from '$lib/components/ui'; + import { SubmittedBanner } from '$lib/components/entity'; import { VariantForm } from '$lib/components/forms'; import { BackButton } from '$lib/components/actions'; import { DataDisplay } from '$lib/components/layout'; @@ -44,6 +45,13 @@ let editSiblingFibers = $derived(collectSiblingFibers(siblingVariants, variantSlug)); let newVariantSiblingFibers = $derived(collectSiblingFibers(siblingVariants)); + // Sibling display names, for the Title Case nudge. Same split as the fiber sets: an + // edit compares against every OTHER variant, a new one against all of them. + let editSiblingNames = $derived( + siblingVariants.filter((v) => (v.slug ?? v.id) !== variantSlug).map((v) => v.name) + ); + let newVariantSiblingNames = $derived(siblingVariants.map((v) => v.name)); + // Which filament's siblings are currently loaded, so we fetch at most once per page. let siblingsLoadedFor: string | null = null; @@ -280,8 +288,8 @@ {#if entityState.hasLocalChanges} - {:else if entityState.hasSubmittedChanges} - + {:else if entityState.submittedEntry} + {/if} {#if messageHandler.message} @@ -420,7 +428,7 @@ {#if variant}
- +
{/if} @@ -444,7 +452,7 @@ {/if} {#if entityState.duplicateData}
- +
{/if} @@ -456,7 +464,7 @@ {/if} {#if entityState.pasteData}
- +
{/if} diff --git a/webui/src/routes/search/+page.svelte b/webui/src/routes/search/+page.svelte index f03b75bda5..266caa9e35 100644 --- a/webui/src/routes/search/+page.svelte +++ b/webui/src/routes/search/+page.svelte @@ -9,7 +9,7 @@ import { changes, changesList } from '$lib/stores/changes'; import { submittedStore, submittedCount } from '$lib/stores/submitted'; import { useChangeTracking } from '$lib/stores/environment'; - import { getChildChangeProps, type ChangeProps } from '$lib/utils/deletedStubs'; + import { getChildChangeProps, NO_CHANGES, type ChangeProps } from '$lib/utils/deletedStubs'; const PAGE_SIZE = 24; @@ -21,14 +21,6 @@ { label: 'Stores', value: 'store' } ]; - const NO_CHANGES: ChangeProps = { - hasLocalChanges: false, - localChangeType: undefined, - hasDescendantChanges: false, - hasSubmittedChanges: false, - submittedChangeType: undefined - }; - let baseRecords: SearchRecord[] = $state([]); let loading = $state(true); let error: string | null = $state(null); @@ -187,6 +179,7 @@ hasDescendantChanges={cp.hasDescendantChanges} hasSubmittedChanges={cp.hasSubmittedChanges} submittedChangeType={cp.submittedChangeType} + submittedPrNumber={cp.submittedPrNumber} /> {/each} diff --git a/webui/src/routes/stores/+page.svelte b/webui/src/routes/stores/+page.svelte index 5d05b7e212..86185fa894 100644 --- a/webui/src/routes/stores/+page.svelte +++ b/webui/src/routes/stores/+page.svelte @@ -229,6 +229,7 @@ hasLocalChanges={changeProps.hasLocalChanges} localChangeType={changeProps.localChangeType} hasSubmittedChanges={changeProps.hasSubmittedChanges} + submittedPrNumber={changeProps.submittedPrNumber} submittedChangeType={changeProps.submittedChangeType} entityType="store" onCopy={() => storeCopy.request(store, `stores/${store.slug ?? store.id}`)} diff --git a/webui/src/routes/stores/[store]/+page.svelte b/webui/src/routes/stores/[store]/+page.svelte index 9a64da9961..301a3e6f4e 100644 --- a/webui/src/routes/stores/[store]/+page.svelte +++ b/webui/src/routes/stores/[store]/+page.svelte @@ -7,7 +7,7 @@ import { StoreForm } from '$lib/components/forms'; import { BackButton } from '$lib/components/actions'; import { DataDisplay } from '$lib/components/layout'; - import { EntityDetails, Logo } from '$lib/components/entity'; + import { EntityDetails, Logo, SubmittedBanner } from '$lib/components/entity'; import { createMessageHandler } from '$lib/utils/messageHandler.svelte'; import { createEntityState } from '$lib/utils/entityState.svelte'; import { createDeleteFlow } from '$lib/utils/useDeleteFlow.svelte'; @@ -211,8 +211,8 @@ {#if entityState.hasLocalChanges} - {:else if entityState.hasSubmittedChanges} - + {:else if entityState.submittedEntry} + {/if} {#if messageHandler.message}