From d0a4ffc2c03cc1312ef5f478459b27d231408776 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 3 Nov 2026 20:28:25 +0000 Subject: [PATCH 01/13] :books: Make the no-wrap rule cover comments and review replies The writing principles of the PR memory carried the no-wrap rule inside the PR description section, so it read as description-only and comments kept getting hard-wrapped. Say that it applies to PR and issue comments and to review replies too. AI-assisted-by: deepseek-v4.1-flash --- .serena/memories/workflow/creating-prs.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.serena/memories/workflow/creating-prs.md b/.serena/memories/workflow/creating-prs.md index 4d9d0fe8d4..4f2d2bcfa7 100644 --- a/.serena/memories/workflow/creating-prs.md +++ b/.serena/memories/workflow/creating-prs.md @@ -66,7 +66,7 @@ The "Note:" line is required at the top. Adjust if this is a manual (non-AI) PR. - **Write for humans.** The diff shows what changed. The description explains why. - **Be concise.** Focus on reasoning: What was the problem? Why did it happen? How did you solve it? - **Prefer bullets over paragraphs.** Short bullet items, grouped by area with bold lead-ins where helpful, are far easier to digest than prose; keep any remaining paragraph to a few sentences. -- **No manual line wraps.** Markdown renders adapting to the viewport; hard-wrapped lines degrade rendering. One line per paragraph or bullet, however long. +- **No manual line wraps.** Markdown renders adapting to the viewport; hard-wrapped lines degrade rendering. One line per paragraph or bullet, however long. This applies to PR descriptions and to PR/issue comments and review replies alike. - **Skip the obvious.** Don't explain what `git diff` already shows. ### What NOT to Include From 68c85c3c692d8dfb85fa8cd91cfca350b220b54e Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 3 Nov 2026 22:31:27 +0000 Subject: [PATCH 02/13] :sparkles: Add changelog flow memory and breaking-change generator Add a `boom` subcommand that generates the `:boom:` section of CHANGES.md from the closed, non-excluded milestone issues labelled `breaking change`, using the last `## Breaking change note` comment of each issue. The whole comment is copied (headings demoted to `#####` outside code fences), and each issue becomes a `#### ` block with the issue and merged closing PR references on their own line. `--write` replaces only that subsection and is idempotent. Add a `comments` subcommand to gh.py (REST, paginated) so the generator can read issue comments, and report anomaly type G (`breaking change` issue missing from `:boom:`), mirroring the `release highlight` check. Document the flow in `mem:workflow/updating-changelog` (also readable by release managers, like `mem:audit-log`), turn the `update-changelog` skill into a router to it, and update the references in `critical-info`, `creating-issues` and the scripts memories. AI-assisted-by: deepseek-v4.1-flash --- .agents/skills/update-changelog/SKILL.md | 307 ++------------ .serena/memories/critical-info.md | 5 +- .serena/memories/scripts/changelog.md | 8 +- .serena/memories/scripts/gh.md | 11 + .serena/memories/workflow/creating-issues.md | 31 +- .../memories/workflow/updating-changelog.md | 187 +++++++++ AGENTS.md | 3 +- scripts/changelog.py | 324 +++++++++++++-- scripts/gh.py | 66 +++ scripts/test_changelog.py | 383 ++++++++++++++++++ scripts/test_gh.py | 46 +++ 11 files changed, 1053 insertions(+), 318 deletions(-) create mode 100644 .serena/memories/workflow/updating-changelog.md create mode 100644 scripts/test_changelog.py diff --git a/.agents/skills/update-changelog/SKILL.md b/.agents/skills/update-changelog/SKILL.md index 9d0276ef53..b6803bce46 100644 --- a/.agents/skills/update-changelog/SKILL.md +++ b/.agents/skills/update-changelog/SKILL.md @@ -5,16 +5,18 @@ description: Update the project CHANGES.md with issues from a given GitHub miles # Skill: update-changelog -Update `CHANGES.md` with entries for all issues and PRs in a given GitHub -milestone. Each entry references the user-facing issue (not the PR) as the -primary link, with the fix PR inline on the same line. +Entry point for all changelog work. All rules — sections, exclusions, entry +format, the `:boom:` and `:rocket:` label contracts, pre-flight, and the +anomaly report — live in `mem:workflow/updating-changelog` (file: +`.serena/memories/workflow/updating-changelog.md`). This skill routes to that +memory; it does not repeat it. ## When to Use - Before a new release, to populate the changelog with all fixed issues - When new issues are added to an existing milestone and the changelog needs to be refreshed -- To ensure every entry follows the correct format for the changelog +- To audit or repair an existing version section ## Prerequisites @@ -23,289 +25,28 @@ primary link, with the fix PR inline on the same line. - `scripts/gh.py` for GitHub queries, `scripts/changelog.py` for changelog checks (run `python3 scripts/changelog.py <command> --help` for usage) -## Workflow +## What to Read -### 1. Determine the target version +1. `mem:workflow/updating-changelog` — the canonical flow: sources of truth, + layout, entry rules, the `## Breaking change note` comment contract, + pre-flight checks, anomaly types, and pitfalls. +2. `mem:scripts/changelog` — `boom`, `check-merged`, `cross-ref`, `report`. +3. `mem:scripts/gh` — `issues`, `issue`, `comments`, `prs`. -The version is typically a semver string like `2.15.3`. Confirm with the user -if not specified. - -### 2. Fetch all issues in the milestone +## Commands ```bash -# All closed issues (default) -python3 scripts/gh.py issues "2.16.0" - -# Include open issues too -python3 scripts/gh.py issues "2.16.0" --state all - -# Exclude entries that should not go in the changelog -python3 scripts/gh.py issues "2.16.0" --exclude "release blocker,no changelog" +python3 scripts/gh.py issues "<VERSION>" --exclude "release blocker,no changelog" +python3 scripts/changelog.py check-merged <PR numbers...> +python3 scripts/changelog.py boom "<VERSION>" # print the :boom: section +python3 scripts/changelog.py boom "<VERSION>" --write # replace it in CHANGES.md +python3 scripts/changelog.py cross-ref "<VERSION>" +python3 scripts/changelog.py report "<VERSION>" ``` -**Exclusion rules (issue-level):** -- `no changelog` label — chore/refactor work, no entry needed -- `release blocker` label — blocked issues not yet ready for changelog -- `Task` issue type — internal chores, not user-facing; excluded by `gh.py` - (use `--include-tasks` to override) -- **Rejected project status** — issues with "Rejected" status on the "Main" - project board are excluded by `gh.py` (use `--include-rejected` to override). - This status is independent of the GitHub issue `state`. +## Hard Rules -**Exclusion rules (PR-level):** PRs with these labels stay out regardless of -their linked issue's labels: `release blocker`, `no issue required`. - -Each entry carries `number`, `title`, `state`, `issue_type`, `labels`, -`closing_prs`, and `project_status`. - -### 3. Identify missing entries (optional) - -```bash -python3 scripts/gh.py issues "2.16.0" --exclude "release blocker,no changelog" --compare CHANGES.md -``` - -Returns only milestone issues not yet referenced in the changelog. Note: it -compares **issues** only. For unreferenced merged **PRs**, use the -cross-reference in step 8. - -### 4. Fetch additional PR details when needed - -```bash -# One or more PR numbers (also: --file prs.txt, or --stdin) -python3 scripts/gh.py prs 9179 9204 9311 - -# All merged PRs in a milestone (default); --state all/open/closed for others -python3 scripts/gh.py prs --milestone "2.16.0" -``` - -Returns `number`, `title`, `body`, `state`, `merged_at`, `author`, `labels`, -and `closing_issues`. Milestone mode uses paginated GraphQL (100 per page). - -### 5. Categorize entries — strictly by issue type, never by labels or emoji - -Use the **Issue Type** field (`issue_type` in the `gh.py` output). No separate -query is needed. - -> **⚠️ CRITICAL: Never use labels or title emoji prefixes for categorization.** -> Labels like `bug`/`enhancement` and prefixes like `:bug:`/`:sparkles:` are -> often wrong or missing. `issue_type` is the single source of truth. - -| `issue_type` value | Changelog section | -|--------------------|-------------------| -| `Bug` | `### :bug: Bugs fixed` | -| `Feature` or `Enhancement` | `### :sparkles: New features & Enhancements` | -| `Task` | **Exclude** — internal chores, not user-facing | -| `null` (not set) | Fallback to labels: `bug` label → bugs, otherwise enhancements | - -**Breaking changes override everything:** an issue with the `breaking change` -label goes under `### :boom: Breaking changes & Deprecations`, no matter its -issue type. This and `release highlight` below are the only label-based -rules — explicit exceptions to the "never by labels" principle above. The -`:boom:` subsection goes first in the version, before `:rocket:` (matching -the existing precedent). - -**Highlights come from the `release highlight` label:** an issue with that -label also gets an entry under `### :rocket: Epics and highlights`, on top of -its regular entry (usually `:sparkles:`). In the section being refreshed, -`:rocket:` lists exactly the labelled issues: never pick highlights yourself -and never keep an unlabelled entry there. Product decides the highlights by -labelling issues on GitHub, so a regeneration cannot undo that choice. Older -version sections are left as they are. - -**Community attribution:** if the issue or its fix PR has the -`community contribution` label, add `(by @<github_username>)` on the entry -line, **before** the issue/PR references. Use the **PR author** (the `author` -field from step 4), not the issue author: - -```markdown -- Fix description of the bug (by @username) [#<ISSUE>](...) (PR: [#<PR>](...)) -``` - -**Only closed issues are included**, even if open ones are tracked in the -milestone. - -**Pairing rules:** - -| Pattern | Changelog format | -|---------|-----------------| -| Closed issue + one or more fix PRs | Primary link = issue, PRs inline comma-separated | -| PR with no linked issue | Link the issue if a matching closed one exists in the milestone; otherwise skip (the issue is the changelog unit) | -| Closed issue with no fix PR in milestone | Link the issue directly, no PR reference | - -> **False-positive associations:** a PR may wrongly claim to close an issue -> from another context (ancient PR, cross-project reference). If titles are -> clearly unrelated or the PR predates the issue by years, treat it as a data -> glitch and skip it. - -### 5a. Verify PR merge status before writing - -A closed issue may list closing PRs that were **closed without merging** -(e.g. a superseded community PR). Only **merged** PRs go in the changelog: - -```bash -python3 scripts/changelog.py check-merged <ALL_PR_NUMBERS> -# also accepts: --file prs.txt, or numbers via --stdin -``` - -If a closing PR is closed-unmerged, find the merged PR that superseded it -(other PRs in the issue's closing list, similar titles, or pointers in the -closed PR's timeline) and reference that one instead. - -### 5b. Security advisory (GHSA) entries - -Advisories fixed in a release go in the changelog even though they are -**neither milestone issues nor PRs**. The GHSA ID and description come from -the user or the release notes — never from the milestone fetch. - -```markdown -- Fix <user-facing description> (https://github.com/penpot/penpot/security/advisories/GHSA-XXXX-XXXX-XXXX) -``` - -Rules: place under `### :bug: Bugs fixed` with **no issue or PR link**; do -**not** fetch or verify the URL (it may be draft/unpublished and 404); write -the description in imperative mood from the advisory title. These entries are -invisible to the automation — add them by hand, and in step 9 apply only the -backport/duplicate check to them. - -### 6. Read the current CHANGES.md and run pre-flight checks - -Newest version goes at the top, right after the `# CHANGELOG` header: - -```markdown -## <VERSION> - -### :boom: Breaking changes & Deprecations - -- <breaking change or deprecation> [#<ISSUE>](https://github.com/penpot/penpot/issues/<ISSUE>) (PR: [#<PR>](https://github.com/penpot/penpot/pull/<PR>)) - -### :bug: Bugs fixed - -- Fix description of the bug [#<ISSUE>](https://github.com/penpot/penpot/issues/<ISSUE>) (PR: [#<PR>](https://github.com/penpot/penpot/pull/<PR>)) -- Fix another bug (by @contributor) [#<ISSUE>](https://github.com/penpot/penpot/issues/<ISSUE>) (PR: [#<PR>](https://github.com/penpot/penpot/pull/<PR>)) - -### :sparkles: New features & Enhancements - -- Add new feature description [#<ISSUE>](https://github.com/penpot/penpot/issues/<ISSUE>) (PR: [#<PR>](https://github.com/penpot/penpot/pull/<PR>)) -``` - -Format details: entries start with `- ` plus a short imperative description; -PR refs stay inline (`(PR: [#<N>](<url>))`, comma-separated for several); -`(by @<username>)` goes before the issue link; only include non-empty -sections; blank line between a section's last entry and the next title; never -duplicate an entry from an earlier version section (the earlier version wins -for backports). - -**Pre-flight checks — fix violations directly in `CHANGES.md` before writing -the new section.** Reconcile every existing entry (any version section) and -every milestone candidate against the current milestone state (re-fetch, do -not trust cached data): - -1. **Duplicate across versions** → remove from the current section. -2. **Stale milestone assignment** (issue moved out of this milestone) → - remove the entry (or drop it if the section does not exist yet). -3. **Newly applied exclusion labels** (`no changelog`, `release blocker`) → - remove the entry. -4. **Issue no longer closed/deleted/Rejected** → remove the entry. -5. **Unmerged or moved PR reference** → fix the reference or remove the - entry (a PR merged in a *different* milestone is a step-9 anomaly, do not - silently remove it). -6. **Issue type changed to `Task`** → remove the entry. -7. **Breaking change misplaced** (issue has the `breaking change` label but - sits in another section) → move the entry to `:boom:`. -8. **Highlight out of sync** (`:rocket:` entry without the `release - highlight` label, or a labelled issue missing from `:rocket:`) → remove - the `:rocket:` entry or add it (step 7b). The regular entry stays. -9. **Missing valid issues** (closed, non-excluded, unreferenced anywhere) → - add them to the current section per step 5. - -### 7. Build the description text - -Derive it from the **issue title**, not the PR title. Strip leading emoji -prefixes (`:bug:`, `:sparkles:`, `:tada:`) and describe the user-facing -behavior: - -| Issue title | Changelog description | -|-------------|----------------------| -| `Plugin API token methods fail with schema validation error on PRO` | `Fix Plugin API token methods failing with schema validation error on PRO` | -| `Comment content is not sanitized before rendering, enabling stored XSS` | `Sanitize comment content on rendering` | -| `Custom uploaded font family names are not sanitized` | `Sanitize font family names on custom uploaded fonts` | - -Insert the new version section right after the `# CHANGELOG` header with the -`edit` tool and enough context for a unique match. - -### 7b. Populate `:rocket: Epics and highlights` from the label - -List the milestone issues that pass step 2 exclusions and carry the -`release highlight` label: - -```bash -python3 scripts/gh.py issues "2.16.0" --exclude "release blocker,no changelog" \ - | python3 -c "import sys,json; [print(i['number'], i['title']) for i in json.load(sys.stdin) if 'release highlight' in i['labels']]" -``` - -Create the subsection if missing (place it before `### :sparkles:`) with one -entry per labelled issue, same text and references as its regular entry. -Every `:rocket:` entry MUST carry issue AND PR references. If no issue is -labelled, leave `:rocket:` out and tell the user: the highlights are -product's call, not the agent's. To change them, add or remove the label on -GitHub and re-run; never edit `:rocket:` by hand. - -### 8. Cross-reference milestone PRs against the changelog - -```bash -python3 scripts/changelog.py cross-ref "<MILESTONE>" [--changes CHANGES.md] -``` - -Lists merged milestone PRs missing from the changelog section (decide per PR: -add it or confirm its exclusion labels) and warns about CLOSED (unmerged) PRs -in the milestone. GHSA entries never appear here — that absence is expected. - -### 9. Generate the anomaly report - -```bash -python3 scripts/changelog.py report "<MILESTONE>" [--changes CHANGES.md --output CHANGES-ISSUES.md] -``` - -Overwrites `CHANGES-ISSUES.md` with the current state. Every number renders as -a full `[#N](https://github.com/penpot/penpot/issues/N)` or -`[#N](https://github.com/penpot/penpot/pull/N)` link. - -**An anomaly is a milestone mismatch or a `:boom:` mislabel** (the changelog -pairing is misleading and a human must judge intent): - -1. Issue in this milestone, referenced PR in another milestone (or none). -2. PR in this milestone, closed issue in another milestone — except an issue - with *no* milestone, which belongs to another (probably private) project - and is neither anomaly nor changelog candidate. -3. `:boom:` entry whose issue lacks the `breaking change` label. These stay - listed in the report **and** in the changelog: either label the issue or - move the entry to its regular section. -4. `:rocket:` out of sync with the `release highlight` label. Step 6 - pre-flight should have fixed it: if it shows up, re-run the workflow. - -**Highlight gaps are warnings, not anomalies:** missing `:rocket:` on a -released X.Y.0 (patches never carry highlights); `:rocket:` entry without -issue AND PR references. Gaps never count toward the anomaly total. - -**Anything else is a rule violation**, not a report item: fix it in step 6 -pre-flight. If one shows up in the report, re-run the workflow. - -## Key Principles - -- **Issue = changelog unit**, PR = implementation detail inline. -- **Latest version first**, below the `# CHANGELOG` header. -- **Issue Type decides the section — exclusively**, except `breaking change` - label → `:boom:` first, and `release highlight` label → also `:rocket:`. -- **User-facing descriptions** in imperative mood. -- **Community attribution** uses the PR author, placed before the issue link. -- **Only closed issues**; **Rejected** project status excludes. -- **`no changelog` and `Task`** stay out. -- **Multiple fix PRs** go comma-separated inline. -- **Duplicates**: the earlier version section wins. -- **Taiga references**: resolve to the GitHub issue via description text or - PRs mentioning the Taiga URL, then link issue + PR. -- **GHSA entries** live under `:bug:` with only the advisory URL (see 5b). -- **Re-fetch before editing**; prefer `scripts/gh.py` over raw `gh api`. -- **Verify PR merge status** (`check-merged`); PR-level exclusions apply. -- **Cross-reference PRs, not just issues** (`cross-ref`); watch for - false-positive PR-to-issue links. +- `CHANGES.md` is edited only on explicit request or during the release process. +- Only closed, non-excluded milestone issues are included; the issue is the changelog unit and the PR is inline. +- `:boom:` and `:rocket:` are label-generated: never hand-edit them; edit the label or the `## Breaking change note` comment and re-run the command. +- Verify PR merge status before writing; only merged PRs are referenced. diff --git a/.serena/memories/critical-info.md b/.serena/memories/critical-info.md index 867eecdbd9..43f678a643 100644 --- a/.serena/memories/critical-info.md +++ b/.serena/memories/critical-info.md @@ -15,11 +15,12 @@ You are working on the GitHub project `penpot/penpot`, a monorepo. - Before `git commit` → `mem:workflow/creating-commits` (subject/body format, 80-char body wrapping enforced by `scripts/check-commit`, `AI-assisted-by: model-name` trailer) - Before `gh issue create` → `mem:workflow/creating-issues` (title derivation, body template, labels, Issue Type) - Before `gh pr create` / `gh pr edit` → `mem:workflow/creating-prs` (title format, body structure, "Note:" line) +- Before updating `CHANGES.md` → `mem:workflow/updating-changelog` (sources of truth, entry rules, the `:boom:`/`:rocket:` label contracts, pre-flight checks, anomaly report) - Before a repo-wide pnpm version update → `mem:workflow/updating-pnpm` (workspace layout, `scripts/sync-pnpm-version` flow, the stamp-missing-field and ignored-builds gotchas, verification steps) - Before a Node version update → `mem:workflow/updating-node` (the six pin sites in `.nvmrc`, the devenv and image Dockerfiles and the plugins-deploy workflows, the DHI tag gotcha, what to verify) - Before bumping a non-JS dependency pinned in `docker/images/` (JDK, ImageMagick) → `mem:workflow/docker-images` - **Never `git push`, force-push, or modify `git origin`** (or any other remote). The user pushes from their own shell; if a push is required, say so and wait. Never amend a commit that the user has already pushed unless explicitly asked. -- **Never edit `CHANGES.md` by hand.** The changelog is generated from GitHub milestones during the release process; update it only via the `update-changelog` skill flow or on explicit user request. +- **Never edit `CHANGES.md` by hand.** The changelog is generated from GitHub milestones during the release process; update it only via `mem:workflow/updating-changelog` (the `update-changelog` skill flow) or on explicit user request. - You have access to the GitHub CLI `gh` or corresponding MCP tools. - Issues are also managed on Taiga. Read issues using the `read_taiga_issue` tool. - Before writing code, analyze the task in depth and describe your plan. If the task is complex, break it down into atomic steps. After making changes, run the applicable lint and format checks for the affected module before considering the work done (per example `mem:backend/core` or `mem:frontend/core`). @@ -59,7 +60,7 @@ The memory is structured in a way that you can get the critical information abou - `scripts/taiga.py` — Fetch public issues, user stories, and tasks from the Penpot Taiga project without authentication. See `mem:scripts/taiga`. - `scripts/gh.py` — GitHub operations helper: list milestone issues, fetch PR details, compare against CHANGES.md. Requires `gh` CLI. See `mem:scripts/gh`. - `scripts/project-anomalies.py` — Main-board anomaly check for a milestone (`check` writes `tmp/<MILESTONE>-ANOMALIES.md`). Bulk resolution via `scripts/gh.py`. See `mem:scripts/project-anomalies`. -- `scripts/changelog.py` — Changelog checks for the `update-changelog` skill: `check-merged` (PR merge status), `cross-ref` (milestone PRs vs changelog section), `report` (anomaly report to CHANGES-ISSUES.md). Calls `scripts/gh.py` via subprocess. See `mem:scripts/changelog`. +- `scripts/changelog.py` — Changelog checks for `mem:workflow/updating-changelog`: `boom` (generate the `:boom:` section from `breaking change` issues), `check-merged` (PR merge status), `cross-ref` (milestone PRs vs changelog section), `report` (anomaly report to CHANGES-ISSUES.md). Calls `scripts/gh.py` via subprocess. See `mem:scripts/changelog`. - `scripts/error-reports.mjs` — Query error reports via RPC API with token authentication. Supports list/get operations with filtering and pagination. See `mem:scripts/error-reports`. - `scripts/clean-node-modules` — Remove stale `node_modules` from all pnpm workspaces (root, modules, member packages). Keeps the shared pnpm store at `<repo>/.pnpm-store` unless `--store`; ignores `external/` and `.opencode/`. Usage and reinstall steps: `mem:workflow/updating-pnpm`. - `scripts/setup-opencode` — Install the opencode CLI in-container as a global pnpm package in `$PNPM_HOME`. Not part of the devenv image; see `mem:devenv/core`. diff --git a/.serena/memories/scripts/changelog.md b/.serena/memories/scripts/changelog.md index 6dc0dd5c94..e04036ea9a 100644 --- a/.serena/memories/scripts/changelog.md +++ b/.serena/memories/scripts/changelog.md @@ -1,12 +1,13 @@ # Changelog helper -`scripts/changelog.py` holds the checks used by the `update-changelog` skill (extracted from the skill file so the skill stays workflow-only). Calls `scripts/gh.py` via subprocess; never call the GitHub API directly from this script. +`scripts/changelog.py` holds the checks used by the changelog flow. The rules and the step-by-step live in `mem:workflow/updating-changelog`; this memory documents the subcommands. Calls `scripts/gh.py` via subprocess; never call the GitHub API directly from this script. ## Subcommands - `check-merged <PR...>` (`--file`, `--stdin` accepted) — warn on any non-merged PR, exit 1 when found; run before writing changelog entries. +- `boom <MILESTONE> [--changes CHANGES.md --write]` — generate `### :boom: Breaking changes & Deprecations` from the closed, non-excluded milestone issues labelled `breaking change`, sorted by issue number. Each issue becomes a `#### <issue title>` block: content = body of the issue's last `## Breaking change note` comment (whole markdown, headings demoted to `#####` outside code fences; fallback = heading plus refs with a stderr warning), refs = issue link plus merged closing PRs on their own line, with `(by @author)` for `community contribution`. `--write` replaces only that subsection of the version (inserts it after the version header when missing, keeps the heading when there are no entries; idempotent). Print-only by default. Uses `gh.py comments`. - `cross-ref <MILESTONE> [--changes CHANGES.md]` — fetch milestone PRs with `--state all` once, report merged PRs missing from the version section plus CLOSED (unmerged) warnings; GHSA entries never appear here by design. -- `report <MILESTONE> [--changes CHANGES.md --output CHANGES-ISSUES.md]` — overwrite the anomaly report (milestone mismatches type A/B plus `:rocket:` gaps C/D); rule violations never belong in the report, they are fixed in CHANGES.md first. +- `report <MILESTONE> [--changes CHANGES.md --output CHANGES-ISSUES.md]` — overwrite the anomaly report: A/B milestone mismatches, E `:boom:` entry without the label, F `:rocket:` sync, G `:boom:` sync, plus the C/D `:rocket:` gaps (warnings). `:boom:`/`:rocket:` rule violations are fixed by regenerating or relabelling, not by editing entries in place. ## Conventions @@ -15,4 +16,5 @@ - Version-section lookup accepts an optional `(suffix)` after the version header (e.g. `(Unreleased)`); a missing section is an error for `cross-ref` and an empty section for `report`. - PR/issue reference parsing covers standard `[#N](.../pull/N)` / `[#N](.../issues/N)` links plus legacy `PR:[N]` / `[Github #N]` forms. - Shared parsing helpers (`extract_version_section`, `extract_subsection`, `collect_changelog_prs/issues`) live at module top so future subcommands reuse them instead of duplicating regexes. -- Report anomaly types: A/B milestone mismatches, E `:boom:` entry whose issue lacks the `breaking change` label (counted, entries preserved — human labels the issue or moves the entry); C/D are `:rocket:` gaps (warnings, never counted). +- Report anomaly types: A/B milestone mismatches, E `:boom:` entry whose issue lacks the `breaking change` label (counted, entries preserved — human labels the issue or moves the entry), G `breaking change` issue missing from `:boom:` (counted, same contract as F); C/D are `:rocket:` gaps (warnings, never counted). +- `issue_excluded` (closed, non-Task, non-Rejected, no `release blocker`/`no changelog` label) is module-level and shared by `report` and `boom` so the exclusions cannot drift. diff --git a/.serena/memories/scripts/gh.md b/.serena/memories/scripts/gh.md index 9c53875672..93fd4e37b8 100644 --- a/.serena/memories/scripts/gh.md +++ b/.serena/memories/scripts/gh.md @@ -11,6 +11,7 @@ repository via GraphQL and REST APIs through the authenticated `gh` CLI. - Comparing milestone issues against CHANGES.md to find missing entries. - Explicitly linking a GitHub issue to a pull request. - Listing or inspecting GitHub Security Advisories (GHSA). +- Fetching issue comments (the `## Breaking change note` markdown for the changelog). ## Prerequisites @@ -82,6 +83,16 @@ Fetch issue details by number in batches of 50 (same input styles as `prs`: numb **Output**: JSON array to stdout; progress to stderr. +### `comments` + +Fetch every comment of one or more issues via REST (paginated, 100 per page), oldest first. Same input styles as `issue`: numbers, `--file`, `--stdin`. + +```bash +python3 scripts/gh.py comments 11235 11236 +``` + +**Output fields per comment**: `issue`, `id`, `author`, `created_at`, `body`. JSON array to stdout; progress to stderr. `changelog.py boom` reads the `## Breaking change note` comment from this output. + ### `link-issue` Explicitly assign a GitHub issue to a pull request: diff --git a/.serena/memories/workflow/creating-issues.md b/.serena/memories/workflow/creating-issues.md index dbe55c9241..537e3e73ea 100644 --- a/.serena/memories/workflow/creating-issues.md +++ b/.serena/memories/workflow/creating-issues.md @@ -35,11 +35,35 @@ Command what should be built. Format: `[Imperative verb] [what] in/on [where]`. | Field | Rule | |-------|------| -| **Labels** | `community contribution` (PRs from non-core) · skip workflow labels (`backport candidate`, `team-qa`) · do **not** add `bug` or `enhancement` labels (use Issue Type instead) | +| **Labels** | `community contribution` (PRs from non-core) · `breaking change` when the change breaks compatibility (see below) · skip workflow labels (`backport candidate`, `team-qa`) · do **not** add `bug` or `enhancement` labels (use Issue Type instead) | | **Milestone** | Use the current or next planned milestone. Fetch available milestones: `gh api repos/penpot/penpot/milestones --jq '.[].title'`. If unsure, omit. | | **Project** | Always `Main` (project number 8). Use `--project "Main"` flag. | | **Issue Type** | See Issue Type section below. Cannot be set via `gh issue create` — use GraphQL after creation. | +## Breaking changes + +When the issue introduces a breaking change or a deprecation, it is the unit +`mem:workflow/updating-changelog` turns into a block under +`### :boom: Breaking changes & Deprecations`. The section is generated from +the issue (`scripts/changelog.py boom`), so the issue must carry both: + +1. The `breaking change` label. +2. A `## Breaking change note` **comment** (not a body section). Everything + after that marker line is copied to the changelog: paragraphs, bullet + lists, code blocks and headings are kept (headings are demoted to level 5 + in `CHANGES.md`). Write it for self-hosters: what changes, what they must + do, and what breaks if they do nothing. Keep it short: explain the + implications and link the runbook or docs for the detail, never paste the + runbook itself. Links must point to a long-lived branch (`staging` or + `main`), never to a feature branch. + +Post the note as a new comment. Do not add a breaking-change section to the +issue body and do not hand-write the changelog block. When the issue comes +from a PR, reuse the text of the PR's breaking-change section. + +The full changelog rules (sections, exclusions, `:rocket:`, pre-flight and +anomaly report) live in `mem:workflow/updating-changelog`. + ## Issue Body Template Write the body to a temp file to avoid shell quoting issues: @@ -237,6 +261,9 @@ Identify: description). Omit internal details. Use the templates above. - **Issue Type:** use the mapping table above (also handles `:bug:` / `:sparkles:` / `:tada:` title prefixes). +- **Breaking changes:** when the PR body has a breaking-change section, post + its user-facing text as a `## Breaking change note` comment on the issue and + add the `breaking change` label (see **Breaking changes** above). ### Create the issue @@ -303,6 +330,8 @@ rm -f /tmp/issue-body.md /tmp/pr-body.md label or the author is not a core team member, add the label to the issue. - **Don't put implementation details in the issue body** — the issue is for users, QA, and changelog readers. +- **Drop the PR's `no issue required` label** once the issue exists: it only + marks PRs that need no changelog unit, and it is stale from then on. ## Creating Issues from Draft Body diff --git a/.serena/memories/workflow/updating-changelog.md b/.serena/memories/workflow/updating-changelog.md new file mode 100644 index 0000000000..26ac7624af --- /dev/null +++ b/.serena/memories/workflow/updating-changelog.md @@ -0,0 +1,187 @@ +# Updating the changelog + +> Note about this memory: unlike other Serena memories (dense notes written for LLM agents, see `mem:memory-maintenance`), this document is also written for human release managers who need full context. That is why it uses plain language and examples. It is the canonical home of the changelog flow; the `update-changelog` skill is only an entry point that routes here. + +`CHANGES.md` (repository root) is the release notes file. Every entry comes from GitHub: a milestone issue, its merged PR, or a security advisory. It is never written from scratch by hand; the only manual parts are the security advisory entries and the wording of a description when the title is not enough. + +The commands live in `scripts/gh.py` (GitHub queries) and `scripts/changelog.py` (changelog checks). Their flags and internals are documented in `mem:scripts/gh` and `mem:scripts/changelog`. + +## 1. When to run it + +- Before a release, to fill the section of the version being released. +- When new issues are added to an existing milestone. +- When asked to audit or refresh `CHANGES.md`. + +Editing `CHANGES.md` happens only on explicit request or as part of the release process; see `mem:critical-info`. + +## 2. Sources of truth + +| Question | Source | +|---|---| +| Which issues belong to the version? | GitHub milestone | +| Which section? | Issue Type | +| Is it a breaking change? | `breaking change` label | +| Is it a highlight? | `release highlight` label | + +- **Issue Type decides the section**: `Bug` → `:bug:`, `Feature`/`Enhancement` → `:sparkles:`, `Task` is excluded (internal chores). Labels `bug`/`enhancement` and title emoji prefixes are often wrong or missing; do not use them. +- **Two labels override the Issue Type**: `breaking change` → an entry only under `:boom:`, never in its regular section; `release highlight` → an extra entry under `:rocket:` on top of its regular entry. +- **Only closed issues**. An open issue in the milestone is not generated, even if someone added a provisional entry by hand before the merge. +- **Excluded issues**: `no changelog`, `release blocker`, `Task` issue type, `Rejected` project status. +- **Excluded PRs**: a PR with `release blocker` or `no issue required` does not bring an entry of its own. +- **An issue with no milestone** belongs to another (probably private) project: it is neither changelog candidate nor anomaly. + +## 3. Layout of `CHANGES.md` + +The newest version goes at the top, right after the `# CHANGELOG` header. Inside a version, the section order is fixed: + +```markdown +# CHANGELOG + +## 2.19.0 (Unreleased) + +### :boom: Breaking changes & Deprecations + +#### <Breaking change title> + +<content> + +[#ISSUE](https://github.com/penpot/penpot/issues/ISSUE) (PR: [#PR](https://github.com/penpot/penpot/pull/PR)) + +### :rocket: Epics and highlights + +- Add configurable keyboard shortcuts [#9924](https://github.com/penpot/penpot/issues/9924) (PR: [#10237](https://github.com/penpot/penpot/pull/10237)) + +### :bug: Bugs fixed + +- Fix <user-facing description of the bug> [#ISSUE](https://github.com/penpot/penpot/issues/ISSUE) (PR: [#PR](https://github.com/penpot/penpot/pull/PR)) + +### :sparkles: New features & Enhancements + +- Add <user-facing description of the feature> [#ISSUE](https://github.com/penpot/penpot/issues/ISSUE) (PR: [#PR](https://github.com/penpot/penpot/pull/PR)) +``` + +- `:boom:` first, then `:rocket:`, `:bug:`, `:sparkles:`. Other headings exist in old versions (`:heart:` community contributions); do not add them to new versions. +- Include a section only when it has entries. Historical versions may have an empty `:boom:` or `:rocket:` heading; `boom --write` keeps the `:boom:` heading even when there are no entries. +- One blank line between the last entry of a section and the next heading. +- No manual line wrapping: one line per bullet or paragraph, however long. + +## 4. Regular entries (`:bug:`, `:sparkles:`) + +The entry shape is: + +```markdown +- <description> (by @github_username) [#ISSUE](https://github.com/penpot/penpot/issues/ISSUE) (PR: [#PR](https://github.com/penpot/penpot/pull/PR), [#PR2](https://github.com/penpot/penpot/pull/PR2)) +``` + +- **Description**: derived from the issue title, not the PR title. Strip emoji prefixes (`:bug:`, `:sparkles:`, `:tada:`) and write the user-facing behaviour in imperative mood. Examples: `Plugin API token methods fail with schema validation error` → `Fix Plugin API token methods failing with schema validation error`; `Custom uploaded font family names are not sanitized` → `Sanitize font family names on custom uploaded fonts`. +- **Issue is the primary link**: the issue is the changelog unit, the PR is the implementation. Multiple fix PRs go comma-separated inline. +- **Community attribution**: when the issue or its PR carries `community contribution`, add `(by @username)` before the issue link. Use the PR author, not the issue author. +- **Closed issue with no PR** in the milestone: link the issue only, no PR reference. +- **PR with no linked issue**: link a matching closed issue in the milestone when one exists; otherwise skip it, because the issue is the changelog unit. +- **False-positive PR links**: a PR may claim to close an unrelated issue (ancient PR, cross-project reference). If the titles are clearly unrelated or the PR predates the issue by years, treat it as a data glitch and ignore it. + +## 5. `:boom:`: breaking changes and deprecations + +Each breaking change is a `####` block, not a bullet. The title is the issue title without its emoji prefix; the content comes from a comment on the issue; the references go on their own line at the end. + +The **issue** must carry: + +1. The `breaking change` label. +2. A comment whose first line is `## Breaking change note`. Everything after that marker line is the changelog content: paragraphs, bullet lists, code blocks, tables, headings (they are demoted to `#####` on write), and so on. The marker itself is case-insensitive and accepts `#`, `##` or `###`; use `##`. + +Example comment: + +```markdown +## Breaking change note + +The Admin Console is now enabled by default and the `enable-admin-console` flag is no longer needed. + +Self-hosted instances must deploy the `penpot-admin-console` service and set `PENPOT_ADMIN_CONSOLE_URI` in frontend and backend (check the [upgrade guide](https://help.penpot.app/technical-guide/getting-started/docker/#upgrade-to-2.19)). +``` + +Generated block: + +```markdown +#### Enable the Admin Console by default + +The Admin Console is now enabled by default and the `enable-admin-console` flag is no longer needed. + +Self-hosted instances must deploy the `penpot-admin-console` service and set `PENPOT_ADMIN_CONSOLE_URI` in frontend and backend (check the [upgrade guide](https://help.penpot.app/technical-guide/getting-started/docker/#upgrade-to-2.19)). + +[#12807](https://github.com/penpot/penpot/issues/12807) (PR: [#12475](https://github.com/penpot/penpot/pull/12475)) +``` + +Rules for the note: + +- **Write it for admins**: what changes, what they must do, and what breaks if they do nothing. Do not paste the full runbook; explain the implications and link the runbook or docs. +- **Link a long-lived branch**, `staging` or `main`, never a feature branch (`issue-1234`). The link may 404 until the PR merges; that is expected. +- **Do not add issue or PR links**: the generator appends them. +- **One note per issue**. If several comments carry the marker, the generator uses the last one and warns. +- **No note**: the generator warns on stderr and emits only the heading and references; fix the issue, not `CHANGES.md`. +- **`####`, not `###`**: `###` is the level of the changelog sections. An entry at `###` would render as a sibling section and the scripts that read and rewrite `:boom:` would cut the section there. The note author never writes the `####`: the generator adds it. +- **Only merged closing PRs** are referenced; unmerged or superseded PRs are skipped. +- **Never hand-edit the section**. Generate it with `boom` (section 7). The note lives on GitHub; edit it there. + +## 6. `:rocket:`: epics and highlights + +- Lists exactly the milestone issues with the `release highlight` label, and nothing else. Product decides highlights by labelling issues; a regeneration cannot undo that choice. +- Every entry copies the text and references of the issue's regular entry and MUST carry both an issue link and a PR reference. +- If no issue is labelled, leave the section out; do not pick highlights yourself. +- Never edit `:rocket:` by hand: add or remove the label and regenerate. + +## 7. Flow + +1. **Confirm the version** (a semver string such as `2.19.0`) with the user when it is not obvious. +2. **Fetch the milestone issues**: `python3 scripts/gh.py issues "<VERSION>" --exclude "release blocker,no changelog"` (closed by default; add `--state all` only to inspect open issues). `gh.py` already drops `Task` and `Rejected`; pass `--include-tasks` / `--include-rejected` only to inspect them. To find what is missing from an existing section, add `--compare CHANGES.md`. +3. **Fetch PR details when needed**: `python3 scripts/gh.py prs <numbers...>` (also `--milestone`). Use it for the PR author, labels and merge state. +4. **Categorize** by Issue Type and the two labels (section 2), and **build the descriptions** (section 4). +5. **Verify PR merge status** before writing: `python3 scripts/changelog.py check-merged <PR numbers...>`. Only merged PRs go in the changelog; if a closing PR was closed unmerged, look for the PR that superseded it. +6. **Reconcile the existing text** (section 8). +7. **Generate `:boom:`**: `python3 scripts/changelog.py boom "<VERSION>"` prints the section; add `--write` to replace it in `CHANGES.md`. The write replaces only that subsection, inserts it after the version header when missing, keeps the heading when there are no entries, and is idempotent. +8. **Generate `:rocket:`** from the `release highlight` label (section 6). +9. **Cross-check PRs**: `python3 scripts/changelog.py cross-ref "<VERSION>"`. It lists merged milestone PRs missing from the section and warns about closed (unmerged) PRs. GHSA entries never appear; that absence is expected. +10. **Generate the anomaly report**: `python3 scripts/changelog.py report "<VERSION>"`. It overwrites `CHANGES-ISSUES.md` with the current state (section 9). +11. **Security advisories (GHSA)**: add them by hand under `:bug:` as `- Fix <user-facing description> (<advisory URL>)`, with no issue or PR link. Do not fetch or verify the URL: it may be draft and return 404. Write the description in imperative mood from the advisory title. These entries are invisible to the automation; apply only the duplicate/backport check to them. + +## 8. Pre-flight: reconcile before writing + +Re-fetch the milestone (do not trust cached data) and fix these in `CHANGES.md` before adding the new section: + +1. **Duplicate across versions** → remove it from the current section; the earlier version wins for backports. +2. **Stale milestone assignment** (the issue moved out of this milestone) → remove the entry. +3. **Newly applied exclusion labels** (`no changelog`, `release blocker`) → remove the entry. +4. **Issue no longer closed, deleted, or now `Rejected`** → remove the entry. +5. **Unmerged or moved PR reference** → fix the reference or remove the entry. A PR merged in a different milestone is an anomaly (section 9); do not remove it silently. +6. **Issue Type changed to `Task`** → remove the entry. +7. **Breaking change out of sync** (a `breaking change` issue outside `:boom:`, or a `:boom:` entry without the label) → regenerate the section; a labelled issue has no regular entry. +8. **Highlight out of sync** (`:rocket:` entry without the label, or a labelled issue missing) → remove it or add it. +9. **Missing valid issues** (closed, non-excluded, referenced nowhere) → add them to their section. + +## 9. Anomaly report + +`report` classifies problems it cannot decide alone. The total counts A, B, E, F and G; C and D are warnings. + +- **A — issue in this milestone, referenced PR in another milestone (or none).** The claim "this issue is fixed by this PR in this milestone" is inconsistent. Fix the milestone on the issue or the PR, or remove the entry. +- **B — PR in this milestone whose closing issue is in another milestone.** Same fix. An issue with no milestone is not an anomaly (another, probably private, project). +- **E — `:boom:` entry whose issue lacks the `breaking change` label.** The entry stays in the changelog: either label the issue or move the entry to its regular section. +- **F — `:rocket:` out of sync with the `release highlight` label.** A rule violation: `:rocket:` must list exactly the labelled issues. Fix the label or re-run the flow. +- **G — `breaking change` issue missing from `:boom:`.** A rule violation: `:boom:` must list exactly the labelled issues. Regenerate the section or relabel. +- **C — released X.Y.0 version without a `:rocket:` section** (gap; patches never carry highlights). +- **D — `:rocket:` entry without issue AND PR references** (gap). + +Anything else is a rule violation, not a report item: fix it in the pre-flight and re-run. + +## 10. Pitfalls + +- **Open issues with provisional entries**: a PR author may add the entry before the merge. `boom` will not generate it until the issue closes; keep the hand-written block identical to what the generator would produce. +- **Labels, not emoji**: title prefixes are cosmetic and often missing; the section comes from Issue Type and the two labels. +- **Taiga references**: old entries link to Taiga. Resolve them to the GitHub issue via the description text or the PRs that mention the Taiga URL, then link issue and PR. +- **Long-lived links only**: runbooks and docs referenced from a note must point to `staging` or `main`, never to a feature branch that will be deleted. +- **Generated sections**: `:boom:` and `:rocket:` are the only label-generated parts. Never patch them by hand; edit the label or the note comment and re-run. + +## 11. Where the code is + +- `scripts/gh.py`: `issues`, `issue`, `comments`, `prs`, `link-issue`, `advisories`. Details: `mem:scripts/gh`. +- `scripts/changelog.py`: `check-merged`, `boom`, `cross-ref`, `report`. Details: `mem:scripts/changelog`. +- Tests: `scripts/test_gh.py`, `scripts/test_changelog.py`. +- The note contract for issue authors: `mem:workflow/creating-issues`, section **Breaking changes**. diff --git a/AGENTS.md b/AGENTS.md index c95fb06920..c39c02dcaf 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -10,7 +10,8 @@ If the user pushes, treat that commit as final from the agent's side. - **Never edit `CHANGES.md` by hand** in commits or PRs. The changelog is generated from GitHub milestones during the release process; update it only - via the `update-changelog` skill flow or on explicit user request. + via `mem:workflow/updating-changelog` (the `update-changelog` skill flow) or + on explicit user request. - **Never pipe test output directly to filters** (`| head`, `| tail`, `| grep`, etc.). Always redirect to a file first: `command > /tmp/output.txt 2>&1`, then read/grep the file. This prevents hiding test failures. See `mem:testing` for details. diff --git a/scripts/changelog.py b/scripts/changelog.py index fbba459fbd..e964bacc18 100644 --- a/scripts/changelog.py +++ b/scripts/changelog.py @@ -1,18 +1,21 @@ #!/usr/bin/env python3 """ -changelog.py — Helper commands for the ``update-changelog`` skill. +changelog.py — Helper commands for the changelog flow. Uses ``scripts/gh.py`` (via subprocess) and the local ``CHANGES.md`` so the -skill file itself stays free of inline programs. +flow memory (`mem:workflow/updating-changelog`) stays free of inline programs. Subcommands: check-merged Verify that every given PR is merged (warns otherwise) + boom Generate the :boom: section from `breaking change` issues cross-ref Compare merged milestone PRs against the changelog section report Generate the anomaly report (CHANGES-ISSUES.md) Usage: python3 scripts/changelog.py check-merged 9179 9204 9311 cat prs.txt | python3 scripts/changelog.py check-merged --stdin + python3 scripts/changelog.py boom "2.19.0" + python3 scripts/changelog.py boom "2.19.0" --write python3 scripts/changelog.py cross-ref "2.16.0" python3 scripts/changelog.py report "2.16.0" python3 scripts/changelog.py report "2.16.0" --changes CHANGES.md --output CHANGES-ISSUES.md @@ -34,6 +37,21 @@ from pathlib import Path REPO = "penpot/penpot" GH_PY = Path(__file__).resolve().parent / "gh.py" +BOOM_HEADING = "### :boom: Breaking changes & Deprecations" +BREAKING_LABEL = "breaking change" +EXCLUDED_LABELS = {"release blocker", "no changelog"} + +# Marker line of the issue comment that carries the changelog text of a +# breaking change. Everything after the marker line is the entry content. +BREAKING_NOTE_RE = re.compile(r"(?im)^#{1,3}[ \t]+Breaking change note[ \t]*$") + +# Level used for the headings found inside a breaking change note. Entries +# themselves are level-4 headings, so note headings must live below them. +NOTE_HEADING = "#####" + +# Emoji prefixes used at the start of issue titles (`:bug:`, `:sparkles:`). +TITLE_PREFIX_RE = re.compile(r"^(?::[A-Za-z0-9_+-]+:\s*)+") + # ───────────────────────────────────────────── # Shared helpers @@ -125,6 +143,84 @@ def collect_changelog_issues(section: str) -> set[int]: return numbers +def issue_excluded(issue: dict | None) -> bool: + """Return True when an issue must stay out of the changelog. + + Shared by the ``report`` and ``boom`` subcommands so the exclusions + cannot drift apart. + """ + if not issue: + return True + if issue.get("state") != "CLOSED": + return True + if issue.get("issue_type") in {"Task"}: + return True + if issue.get("project_status") in {"Rejected"}: + return True + if EXCLUDED_LABELS & set(issue.get("labels", [])): + return True + return False + + +def extract_breaking_note(comment_body: str | None) -> str | None: + """Return the content that follows a ``## Breaking change note`` marker. + + The whole comment is used (paragraphs, lists, code blocks), not just its + first paragraph, so a breaking change can be as detailed as needed. + """ + if not comment_body: + return None + match = BREAKING_NOTE_RE.search(comment_body) + if not match: + return None + content = demote_headings(comment_body[match.end():].strip("\n")) + return content.strip() or None + + +def demote_headings(markdown: str) -> str: + """Push note headings below the entry heading, leaving code fences alone.""" + lines: list[str] = [] + in_fence = False + for line in markdown.splitlines(): + if line.lstrip().startswith(("```", "~~~")): + in_fence = not in_fence + lines.append(line) + continue + if not in_fence: + match = re.match(r"^#{1,4}([ \t]+.*)$", line) + if match: + line = NOTE_HEADING + match.group(1) + lines.append(line) + return "\n".join(lines) + + +def strip_title_prefix(title: str) -> str: + """Remove leading emoji shortcodes from a title.""" + return TITLE_PREFIX_RE.sub("", title).strip() + + +def closing_pr_numbers(issue: dict) -> list[int]: + """Return the sorted closing PR numbers of an issue.""" + return sorted(set(issue.get("closing_prs", []))) + + +def render_boom_entry( + issue: dict, + note: str | None, + pr_numbers: list[int], + author: str | None = None, +) -> str: + """Render one ``:boom:`` entry: title, note content and references.""" + title = strip_title_prefix(issue["title"]) + attribution = f"(by @{author}) " if author else "" + refs = attribution + issue_link(issue["number"]) + if pr_numbers: + refs += " (PR: " + ", ".join(pr_link(n) for n in pr_numbers) + ")" + if note: + return f"#### {title}\n\n{note}\n\n{refs}" + return f"#### {title}\n\n{refs}" + + # ───────────────────────────────────────────── # Subcommand: check-merged # ───────────────────────────────────────────── @@ -155,6 +251,153 @@ def cmd_check_merged(args: argparse.Namespace) -> None: print(f"OK: all {len(prs)} PRs are merged", file=sys.stderr) +# ───────────────────────────────────────────── +# Subcommand: boom +# ───────────────────────────────────────────── + + +def version_section_bounds(content: str, milestone: str) -> tuple[int, int] | None: + """Return the ``[start, end)`` offsets of a ``## <VERSION>`` section.""" + match = re.search( + rf"(?m)^## {re.escape(milestone)}(?:[ \t]*\([^)]*\))?[ \t]*$", content + ) + if not match: + return None + start = match.start() + next_heading = re.search(r"(?m)^## ", content[match.end():]) + end = match.end() + next_heading.start() if next_heading else len(content) + return start, end + + +def boom_body(entries: list[str]) -> str: + """Return the body text that follows the ``:boom:`` heading line.""" + if entries: + return "\n\n" + "\n\n".join(entries) + "\n\n" + return "\n\n" + + +def replace_boom_section(content: str, milestone: str, entries: list[str]) -> str: + """Return ``content`` with its ``:boom:`` subsection replaced. + + The subsection is inserted right after the version header when missing, + which keeps ``:boom:`` as the first section. + """ + bounds = version_section_bounds(content, milestone) + if bounds is None: + raise ValueError(f'no "## {milestone}" section found') + start, end = bounds + section = content[start:end] + heading = re.search(rf"(?m)^{re.escape(BOOM_HEADING)}[ \t]*$", section) + if heading: + body_start = start + heading.end() + next_heading = re.search(r"(?m)^### ", content[body_start:end]) + body_end = body_start + next_heading.start() if next_heading else end + return content[:body_start] + boom_body(entries) + content[body_end:] + header = re.match(r"(?m)^## [^\n]*\n", section) + if header is None: + raise ValueError(f'version "## {milestone}" header has no trailing newline') + insert_at = start + header.end() + block = "\n" + BOOM_HEADING + boom_body(entries)[:-1] + return content[:insert_at] + block + content[insert_at:] + + +def fetch_prs_by_issue(issues: list[dict]) -> dict[int, dict]: + """Fetch every closing PR of ``issues``, keyed by PR number.""" + numbers = sorted({n for issue in issues for n in issue.get("closing_prs", [])}) + if not numbers: + return {} + return {pr["number"]: pr for pr in fetch_prs_by_numbers(numbers)} + + +def fetch_breaking_notes(issues: list[dict]) -> dict[int, str]: + """Return the last ``## Breaking change note`` comment of each issue.""" + if not issues: + return {} + comments = json.loads( + run_gh_py("comments", *[str(issue["number"]) for issue in issues]) + ) + notes: dict[int, str] = {} + for comment in comments: + note = extract_breaking_note(comment.get("body")) + if note is None: + continue + number = comment["issue"] + if number in notes: + print( + f'WARNING: issue #{number} has several "## Breaking change note" ' + "comments; using the last one", + file=sys.stderr, + ) + notes[number] = note + return notes + + +def cmd_boom(args: argparse.Namespace) -> None: + """Generate the ``:boom:`` section from ``breaking change`` issues.""" + all_issues = json.loads(run_gh_py("issues", args.milestone, "--state", "all")) + labelled = sorted( + ( + issue + for issue in all_issues + if BREAKING_LABEL in issue.get("labels", []) + and not issue_excluded(issue) + ), + key=lambda issue: issue["number"], + ) + + notes: dict[int, str] = {} + prs: dict[int, dict] = {} + if labelled: + notes = fetch_breaking_notes(labelled) + prs = fetch_prs_by_issue(labelled) + + entries: list[str] = [] + for issue in labelled: + note = notes.get(issue["number"]) + if not note: + print( + f'WARNING: issue #{issue["number"]} has no "## Breaking change note" ' + "comment; the entry will have no description", + file=sys.stderr, + ) + merged = [ + prs[num] + for num in closing_pr_numbers(issue) + if num in prs and prs[num].get("state") == "MERGED" + ] + author = None + if "community contribution" in issue.get("labels", []): + authors = [pr["author"] for pr in merged if pr.get("author")] + author = authors[0] if authors else None + entries.append( + render_boom_entry(issue, note, [pr["number"] for pr in merged], author) + ) + + block = BOOM_HEADING + boom_body(entries) + print(block.rstrip("\n")) + + if not labelled: + print( + f"No closed issues labelled `{BREAKING_LABEL}` in {args.milestone}", + file=sys.stderr, + ) + + if args.write: + with open(args.changes) as f: + content = f.read() + try: + updated = replace_boom_section(content, args.milestone, entries) + except ValueError as err: + print(f"ERROR: {err}", file=sys.stderr) + sys.exit(1) + if updated == content: + print(f"{args.changes}: :boom: section already up to date", file=sys.stderr) + else: + with open(args.changes, "w") as f: + f.write(updated) + print(f"{args.changes}: :boom: section updated", file=sys.stderr) + + # ───────────────────────────────────────────── # Subcommand: cross-ref # ───────────────────────────────────────────── @@ -298,21 +541,6 @@ def cmd_report(args: argparse.Namespace) -> None: """Return the milestone title for an issue, or None if unassigned.""" return issue_milestone_cache.get(issue_num) - excluded_labels = {"release blocker", "no changelog"} - - def issue_excluded(issue: dict | None) -> bool: - if not issue: - return True - if issue.get("state") != "CLOSED": - return True - if issue.get("issue_type") in {"Task"}: - return True - if issue.get("project_status") in {"Rejected"}: - return True - if excluded_labels & set(issue.get("labels", [])): - return True - return False - # Type A: issue in milestone, referenced PR in different milestone or none anomalies_a = [] for issue_num in sorted(changelog_issues): @@ -361,13 +589,14 @@ def cmd_report(args: argparse.Namespace) -> None: # These stay in the changelog (they are preserved, not removed) — the # human decides whether to label the issue or move the entry elsewhere. anomalies_e = [] - boom_body = extract_subsection(section, r"### :boom:") - if boom_body: - for issue_num in sorted(collect_changelog_issues(boom_body)): + boom_section_body = extract_subsection(section, r"### :boom:") + boom_issues = collect_changelog_issues(boom_section_body) + if boom_section_body: + for issue_num in sorted(boom_issues): labels = get_issue_labels(issue_num) if labels is None: continue - if "breaking change" not in labels: + if BREAKING_LABEL not in labels: issue = issue_by_num.get(issue_num) or external_issues.get(issue_num) or {} anomalies_e.append( { @@ -406,6 +635,20 @@ def cmd_report(args: argparse.Namespace) -> None: } ) + # Type G: `breaking change` issue missing from :boom:. Same contract as + # type F for highlights: the label is the only source of the section. + anomalies_g = [] + for issue_num, issue in sorted(issue_by_num.items()): + if issue_excluded(issue) or issue_num in boom_issues: + continue + if BREAKING_LABEL in issue.get("labels", []): + anomalies_g.append( + { + "issue": issue_num, + "issue_title": issue.get("title", ""), + } + ) + # Type C: released X.Y.0 sections without a :rocket: subsection. # Patches (X.Y.Z with Z != 0) never carry :rocket: — only minors/majors. anomalies_c = [] @@ -459,13 +702,14 @@ def cmd_report(args: argparse.Namespace) -> None: ) f.write("---\n\n") - n_a, n_b, n_c, n_d, n_e, n_f = ( + n_a, n_b, n_c, n_d, n_e, n_f, n_g = ( len(anomalies_a), len(anomalies_b), len(anomalies_c), len(anomalies_d), len(anomalies_e), len(anomalies_f), + len(anomalies_g), ) f.write("## Summary\n\n") @@ -476,14 +720,15 @@ def cmd_report(args: argparse.Namespace) -> None: f"- **PR in {milestone}, closing issue in a different milestone:** {n_b}\n" ) f.write(f"- **:boom: entry without breaking change label:** {n_e}\n") + f.write(f"- **:boom: out of sync with breaking change label:** {n_g}\n") f.write(f"- **:rocket: out of sync with release highlight label:** {n_f}\n") - f.write(f"- **Total anomalies:** {n_a + n_b + n_e + n_f}\n") + f.write(f"- **Total anomalies:** {n_a + n_b + n_e + n_f + n_g}\n") f.write( f"- **Released X.Y.0 version missing :rocket: section (gap):** {n_c}\n" ) f.write(f"- **:rocket: entry without issue AND PR references (gap):** {n_d}\n\n") - if n_a or n_b or n_e or n_f: + if n_a or n_b or n_e or n_f or n_g: f.write("## Anomalies\n\n") f.write( "Types A and B are milestone mismatches between an issue in the changelog " @@ -494,8 +739,9 @@ def cmd_report(args: argparse.Namespace) -> None: "removing the misleading entry from the changelog.\n\n" "Type E entries stay in the changelog: either label the issue " "as `breaking change` or move the entry out of `:boom:`.\n\n" - "Type F is a rule violation: `:rocket:` must list exactly the " - "issues labelled `release highlight`. Fix the label on GitHub " + "Types F and G are rule violations: `:rocket:` must list exactly " + "the issues labelled `release highlight`, and `:boom:` exactly " + "the issues labelled `breaking change`. Fix the label on GitHub " "or re-run the workflow.\n\n" ) @@ -556,6 +802,18 @@ def cmd_report(args: argparse.Namespace) -> None: f"- ⚠️ {issue_link_title(e['issue'], e['issue_title'][:80])}: {e['reason']}\n" ) f.write("\n") + + if n_g: + f.write("\n### :boom: out of sync with breaking change label\n\n") + f.write( + "These milestone issues carry the `breaking change` label but " + "have no entry under `### :boom: Breaking changes & Deprecations`. " + "Regenerate the section (`changelog.py boom --write`) or add " + "the entry.\n\n" + ) + for e in anomalies_g: + f.write(f"- ⚠️ {issue_link_title(e['issue'], e['issue_title'][:80])}\n") + f.write("\n") else: f.write( "✅ No anomalies found. All (issue, PR) pairs in the changelog have aligned milestone assignments.\n\n" @@ -590,7 +848,7 @@ def cmd_report(args: argparse.Namespace) -> None: for d in anomalies_d: f.write(f"- **{d['version']}**: `{d['line']}`\n") f.write("\n") - elif not (n_a or n_b or n_e or n_f): + elif not (n_a or n_b or n_e or n_f or n_g): f.write( "✅ No highlight gaps found. All released X.Y.0 versions have properly referenced :rocket: entries.\n\n" ) @@ -618,7 +876,7 @@ def cmd_report(args: argparse.Namespace) -> None: def main() -> None: parser = argparse.ArgumentParser( - description="Helper commands for the update-changelog skill" + description="Helper commands for the changelog flow" ) sub = parser.add_subparsers(dest="command", required=True, title="subcommands") @@ -633,6 +891,16 @@ def main() -> None: help="Read PR numbers from stdin (one per line)") p_check.set_defaults(func=cmd_check_merged) + p_boom = sub.add_parser( + "boom", help="Generate the :boom: section from `breaking change` issues" + ) + p_boom.add_argument("milestone", help="Milestone title (e.g. '2.16.0')") + p_boom.add_argument("--changes", default="CHANGES.md", + help="Path to the changelog file (default: CHANGES.md)") + p_boom.add_argument("--write", action="store_true", + help="Replace the :boom: section in CHANGES.md in place") + p_boom.set_defaults(func=cmd_boom) + p_cross = sub.add_parser( "cross-ref", help="Compare merged milestone PRs against the changelog section", diff --git a/scripts/gh.py b/scripts/gh.py index c430fbe843..2ec3eb8dc3 100755 --- a/scripts/gh.py +++ b/scripts/gh.py @@ -6,6 +6,8 @@ Uses GitHub GraphQL and REST APIs via the authenticated ``gh`` CLI. Subcommands: issues List issues in a milestone (or unassigned with milestone=none) + issue Fetch issue details by number (batched) + comments Fetch every comment of one or more issues prs Fetch details for one or more PRs (by number or milestone) advisories List or inspect GitHub security advisories link-issue Explicitly link a GitHub issue to a pull request @@ -26,6 +28,7 @@ Usage: python3 scripts/gh.py prs --milestone "2.16.0" (default: state=merged) python3 scripts/gh.py prs --milestone "2.16.0" --state all python3 scripts/gh.py issue 11235 11236 + python3 scripts/gh.py comments 11235 11236 python3 scripts/gh.py advisories (list all advisories) python3 scripts/gh.py advisories --severity critical (filter by severity) python3 scripts/gh.py advisories GHSA-xvj6-fh9w-gjw7 (single advisory detail) @@ -888,6 +891,51 @@ def cmd_issue(args: argparse.Namespace) -> None: print(json.dumps(all_results, indent=2)) +# ───────────────────────────────────────────── +# Subcommand: comments +# ───────────────────────────────────────────── + +COMMENTS_PER_PAGE = 100 + + +def fetch_issue_comments(issue_number: int) -> list[dict]: + """Fetch every comment of an issue via REST, oldest first.""" + comments: list[dict] = [] + page = 1 + while True: + batch = run_gh_rest( + f"repos/{REPO}/issues/{issue_number}/comments" + f"?per_page={COMMENTS_PER_PAGE}&page={page}" + ) + for comment in batch: + comments.append({ + "issue": issue_number, + "id": comment["id"], + "author": (comment.get("user") or {}).get("login"), + "created_at": comment.get("created_at"), + "body": comment.get("body"), + }) + if len(batch) < COMMENTS_PER_PAGE: + return comments + page += 1 + + +def cmd_comments(args: argparse.Namespace) -> None: + """Handle the ``comments`` subcommand (REST, paginated per issue).""" + issue_numbers = read_numbers_from_args(args) + if not issue_numbers: + print("ERROR: no issue numbers provided (pass numbers, --file, or --stdin)", + file=sys.stderr) + sys.exit(1) + + all_comments: list[dict] = [] + for num in issue_numbers: + print(f"Fetching comments for issue #{num}...", file=sys.stderr) + all_comments.extend(fetch_issue_comments(num)) + + print(json.dumps(all_comments, indent=2)) + + # ───────────────────────────────────────────── # Subcommand: advisories # ───────────────────────────────────────────── @@ -1080,6 +1128,24 @@ def main() -> None: ) p_issue.set_defaults(func=cmd_issue) + # --- comments --- + p_comments = sub.add_parser( + "comments", help="Fetch every comment of one or more issues" + ) + p_comments.add_argument( + "numbers", type=int, nargs="*", + help="Issue numbers to fetch (space-separated)" + ) + p_comments.add_argument( + "--file", type=str, + help="File with one issue number per line" + ) + p_comments.add_argument( + "--stdin", action="store_true", + help="Read issue numbers from stdin (one per line)" + ) + p_comments.set_defaults(func=cmd_comments) + # --- advisories --- p_adv = sub.add_parser("advisories", help="List or inspect GitHub security advisories") p_adv.add_argument( diff --git a/scripts/test_changelog.py b/scripts/test_changelog.py new file mode 100644 index 0000000000..5d6a19946e --- /dev/null +++ b/scripts/test_changelog.py @@ -0,0 +1,383 @@ +#!/usr/bin/env python3 +"""Tests for scripts/changelog.py. + +Run with: + + python3 scripts/test_changelog.py +""" + +import argparse +import contextlib +import importlib.machinery +import importlib.util +import io +import json +import pathlib +import sys +import tempfile +import unittest +from unittest.mock import patch + + +# Loading scripts/changelog.py should not emit scripts/__pycache__/. +sys.dont_write_bytecode = True + +SCRIPT_PATH = pathlib.Path(__file__).resolve().parent / "changelog.py" + + +def load_changelog(): + """Load scripts/changelog.py as a module without running its CLI.""" + loader = importlib.machinery.SourceFileLoader("changelog_helper", str(SCRIPT_PATH)) + spec = importlib.util.spec_from_loader("changelog_helper", loader) + module = importlib.util.module_from_spec(spec) + loader.exec_module(module) + return module + + +ch = load_changelog() + +CHANGES = """# CHANGELOG + +## 2.19.0 (Unreleased) + +### :boom: Breaking changes & Deprecations + +- Old entry. + +### :rocket: Epics and highlights + +- Add highlights [#1](https://github.com/penpot/penpot/issues/1) +""" + + +class ExtractBreakingNoteTests(unittest.TestCase): + def test_uses_the_whole_comment_after_the_marker(self): + body = ( + "## Breaking change note\n\n" + "First paragraph.\n\n" + "- bullet one\n" + "- bullet two\n\n" + "```sql\nSELECT 1;\n```\n" + ) + self.assertEqual( + ch.extract_breaking_note(body), + "First paragraph.\n\n- bullet one\n- bullet two\n\n" + "```sql\nSELECT 1;\n```", + ) + + def test_accepts_any_heading_level_for_the_marker(self): + self.assertEqual( + ch.extract_breaking_note("# breaking change note\n\nText."), "Text." + ) + self.assertEqual( + ch.extract_breaking_note("### Breaking Change Note\n\nText."), "Text." + ) + + def test_returns_none_without_marker(self): + self.assertIsNone(ch.extract_breaking_note("### Description\n\ntext\n")) + self.assertIsNone(ch.extract_breaking_note("## Breaking change notes\n\ntext")) + self.assertIsNone(ch.extract_breaking_note(None)) + self.assertIsNone(ch.extract_breaking_note("")) + + def test_returns_none_for_a_marker_without_content(self): + self.assertIsNone(ch.extract_breaking_note("## Breaking change note\n")) + + +class DemoteHeadingsTests(unittest.TestCase): + def test_demotes_headings_below_the_entry_level(self): + markdown = "# One\n## Two\n### Three\n#### Four\n##### Five\n" + self.assertEqual( + ch.demote_headings(markdown), + "##### One\n##### Two\n##### Three\n##### Four\n##### Five", + ) + + def test_keeps_headings_inside_code_fences(self): + markdown = ( + "```bash\n# not a heading\n```\n" + "## Real heading\n" + "~~~\n### not a heading\n~~~\n" + ) + self.assertEqual( + ch.demote_headings(markdown), + "```bash\n# not a heading\n```\n" + "##### Real heading\n" + "~~~\n### not a heading\n~~~", + ) + + +class StripTitlePrefixTests(unittest.TestCase): + def test_strips_shortcodes(self): + self.assertEqual(ch.strip_title_prefix(":boom: Enable it"), "Enable it") + self.assertEqual(ch.strip_title_prefix(":bug: :tada: Fix it"), "Fix it") + self.assertEqual(ch.strip_title_prefix("Plain title"), "Plain title") + + +class IssueExcludedTests(unittest.TestCase): + def issue(self, **overrides): + base = { + "state": "CLOSED", + "issue_type": "Enhancement", + "project_status": "In progress", + "labels": [], + } + base.update(overrides) + return base + + def test_keeps_a_valid_closed_issue(self): + self.assertFalse(ch.issue_excluded(self.issue())) + + def test_excludes_open_task_rejected_and_labelled(self): + self.assertTrue(ch.issue_excluded(None)) + self.assertTrue(ch.issue_excluded(self.issue(state="OPEN"))) + self.assertTrue(ch.issue_excluded(self.issue(issue_type="Task"))) + self.assertTrue(ch.issue_excluded(self.issue(project_status="Rejected"))) + self.assertTrue(ch.issue_excluded(self.issue(labels=["release blocker"]))) + self.assertTrue(ch.issue_excluded(self.issue(labels=["no changelog"]))) + + +class RenderBoomEntryTests(unittest.TestCase): + def test_entry_is_a_heading_plus_note_plus_references(self): + issue = {"number": 7, "title": ":boom: Drop the flag", "labels": []} + self.assertEqual( + ch.render_boom_entry(issue, "One.\n\nTwo.", [8, 9]), + "#### Drop the flag\n\nOne.\n\nTwo.\n\n" + "[#7](https://github.com/penpot/penpot/issues/7)" + " (PR: [#8](https://github.com/penpot/penpot/pull/8)," + " [#9](https://github.com/penpot/penpot/pull/9))", + ) + + def test_entry_without_a_note_keeps_the_heading(self): + issue = {"number": 7, "title": "Drop the flag", "labels": []} + self.assertEqual( + ch.render_boom_entry(issue, None, []), + "#### Drop the flag\n\n[#7](https://github.com/penpot/penpot/issues/7)", + ) + + def test_community_attribution_goes_before_the_issue_link(self): + issue = {"number": 7, "title": "Drop the flag", "labels": []} + entry = ch.render_boom_entry(issue, "Text.", [8], "someone") + self.assertEqual( + entry, + "#### Drop the flag\n\nText.\n\n(by @someone)" + " [#7](https://github.com/penpot/penpot/issues/7)" + " (PR: [#8](https://github.com/penpot/penpot/pull/8))", + ) + + +class ReplaceBoomSectionTests(unittest.TestCase): + def test_replaces_the_existing_subsection(self): + entry = "#### New entry\n\nSome text.\n\n[#9](https://example.test/9)" + updated = ch.replace_boom_section(CHANGES, "2.19.0", [entry]) + self.assertIn( + "### :boom: Breaking changes & Deprecations\n\n" + f"{entry}\n\n### :rocket:", + updated, + ) + self.assertNotIn("Old entry.", updated) + + def test_replacement_is_idempotent(self): + entry = "#### New entry\n\nSome text.\n\n[#9](https://example.test/9)" + once = ch.replace_boom_section(CHANGES, "2.19.0", [entry]) + self.assertEqual(once, ch.replace_boom_section(once, "2.19.0", [entry])) + + def test_empty_section_keeps_the_heading(self): + updated = ch.replace_boom_section(CHANGES, "2.19.0", []) + self.assertIn( + "### :boom: Breaking changes & Deprecations\n\n### :rocket:", updated + ) + + def test_inserts_the_subsection_when_missing(self): + without = CHANGES.replace( + "### :boom: Breaking changes & Deprecations\n\n- Old entry.\n\n", "" + ) + updated = ch.replace_boom_section(without, "2.19.0", ["#### New entry"]) + self.assertIn( + "## 2.19.0 (Unreleased)\n\n" + "### :boom: Breaking changes & Deprecations\n\n#### New entry\n\n" + "### :rocket:", + updated, + ) + + def test_unknown_version_raises(self): + with self.assertRaises(ValueError): + ch.replace_boom_section(CHANGES, "9.9.9", ["#### Entry"]) + + +ISSUES = [ + { + "number": 21, + "title": ":boom: New default", + "state": "CLOSED", + "issue_type": "Enhancement", + "labels": ["breaking change"], + "closing_prs": [31], + "project_status": "Done", + }, + { + "number": 22, + "title": "No note", + "state": "CLOSED", + "issue_type": "Enhancement", + "labels": ["breaking change"], + "closing_prs": [], + "project_status": "Done", + }, + { + "number": 23, + "title": "Still open", + "state": "OPEN", + "issue_type": "Enhancement", + "labels": ["breaking change"], + "closing_prs": [], + "project_status": "In progress", + }, + { + "number": 24, + "title": "Not breaking", + "state": "CLOSED", + "issue_type": "Bug", + "labels": [], + "closing_prs": [], + "project_status": "Done", + }, +] + +COMMENTS = [ + {"issue": 21, "body": "## Breaking change note\n\nDeploy the new service.\n"}, + {"issue": 22, "body": "## Description\n\nNo marker here.\n"}, + {"issue": 21, "body": "## Breaking change note\n\nDeploy the new service.\n"}, +] + +PRS = [{"number": 31, "author": "someone", "labels": [], "state": "MERGED"}] + + +class CmdBoomTests(unittest.TestCase): + def setUp(self): + self.calls = [] + self.issues = ISSUES + self.comments = COMMENTS + self.prs = PRS + + def fake_run_gh_py(*args, input_text=None): + self.calls.append(args) + if args[0] == "issues": + return json.dumps(self.issues) + if args[0] == "comments": + return json.dumps(self.comments) + if args[0] == "prs": + return json.dumps(self.prs) + raise AssertionError(f"unexpected gh.py call: {args}") + + patcher = patch.object(ch, "run_gh_py", side_effect=fake_run_gh_py) + patcher.start() + self.addCleanup(patcher.stop) + + def run_boom(self, **kwargs): + args = argparse.Namespace( + milestone="2.19.0", + changes=CHANGES if "changes" not in kwargs else kwargs["changes"], + write=kwargs.get("write", False), + ) + out, err = io.StringIO(), io.StringIO() + with contextlib.redirect_stdout(out), contextlib.redirect_stderr(err): + ch.cmd_boom(args) + return out.getvalue(), err.getvalue() + + def test_renders_only_closed_breaking_issues(self): + out, err = self.run_boom() + expected = ( + "### :boom: Breaking changes & Deprecations\n\n" + "#### New default\n\n" + "Deploy the new service.\n\n" + "[#21](https://github.com/penpot/penpot/issues/21)" + " (PR: [#31](https://github.com/penpot/penpot/pull/31))\n\n" + "#### No note\n\n" + "[#22](https://github.com/penpot/penpot/issues/22)\n" + ) + self.assertEqual(out, expected) + self.assertIn("issue #22 has no", err) + self.assertIn("using the last one", err) + + def test_unmerged_closing_pr_is_not_referenced(self): + self.prs = [{"number": 31, "author": "someone", "state": "CLOSED"}] + out, _ = self.run_boom() + self.assertNotIn("pull/31", out) + self.assertIn("[#21](https://github.com/penpot/penpot/issues/21)", out) + + def test_no_labelled_issue_clears_the_section_on_write(self): + self.issues = [ISSUES[3]] + with tempfile.TemporaryDirectory() as tmp: + path = pathlib.Path(tmp) / "CHANGES.md" + path.write_text(CHANGES) + out, err = self.run_boom(changes=str(path), write=True) + updated = path.read_text() + self.assertEqual(out, "### :boom: Breaking changes & Deprecations\n") + self.assertNotIn("Old entry.", updated) + self.assertIn(":boom: section updated", err) + + def test_writes_the_section_in_place(self): + with tempfile.TemporaryDirectory() as tmp: + path = pathlib.Path(tmp) / "CHANGES.md" + path.write_text(CHANGES) + out, err = self.run_boom(changes=str(path), write=True) + updated = path.read_text() + self.assertIn("#### New default", updated) + self.assertNotIn("Old entry.", updated) + self.assertIn(":boom: section updated", err) + self.assertIn("Deploy the new service.", out) + + +class ReportTests(unittest.TestCase): + """Focused check of the `:boom:` sync anomaly (type G) in the report.""" + + def test_reports_breaking_issue_missing_from_boom(self): + issues = [ + { + "number": 1, + "title": "Unlabelled entry", + "state": "CLOSED", + "issue_type": "Enhancement", + "labels": [], + "closing_prs": [], + "project_status": "Done", + }, + { + "number": 2, + "title": "Labelled, not listed", + "state": "CLOSED", + "issue_type": "Enhancement", + "labels": ["breaking change"], + "closing_prs": [], + "project_status": "Done", + }, + ] + changes = ( + "# CHANGELOG\n\n## 2.19.0 (Unreleased)\n\n" + "### :boom: Breaking changes & Deprecations\n\n" + "#### Something changed\n\n" + "[#1](https://github.com/penpot/penpot/issues/1)\n" + ) + with tempfile.TemporaryDirectory() as tmp: + changes_path = pathlib.Path(tmp) / "CHANGES.md" + output_path = pathlib.Path(tmp) / "ISSUES.md" + changes_path.write_text(changes) + args = argparse.Namespace( + milestone="2.19.0", + changes=str(changes_path), + output=str(output_path), + ) + with patch.object(ch, "run_gh_py", return_value=json.dumps(issues)), \ + patch.object(ch, "fetch_milestone_prs", return_value=[]), \ + contextlib.redirect_stdout(io.StringIO()), \ + contextlib.redirect_stderr(io.StringIO()): + ch.cmd_report(args) + report = output_path.read_text() + + self.assertIn("**:boom: out of sync with breaking change label:** 1", report) + self.assertIn("### :boom: out of sync with breaking change label", report) + self.assertIn("issues/2", report) + self.assertIn("**:boom: entry without breaking change label:** 1", report) + + +if __name__ == "__main__": + unittest.main(verbosity=2) diff --git a/scripts/test_gh.py b/scripts/test_gh.py index 5fb6840a22..3255e8b495 100644 --- a/scripts/test_gh.py +++ b/scripts/test_gh.py @@ -110,5 +110,51 @@ class LinkIssueTests(unittest.TestCase): self.assertIn("link missing", stderr.getvalue()) +class CommentsTests(unittest.TestCase): + def test_paginates_until_a_short_page(self): + page_one = [ + {"id": i, "user": {"login": "u"}, "created_at": "t", "body": f"b{i}"} + for i in range(100) + ] + page_two = [{"id": 100, "user": None, "created_at": "t", "body": "last"}] + calls = [] + + def fake_rest(path): + calls.append(path) + return page_one if len(calls) == 1 else page_two + + with patch.object(gh, "run_gh_rest", side_effect=fake_rest): + comments = gh.fetch_issue_comments(7) + + self.assertEqual(len(comments), 101) + self.assertEqual( + comments[0], + {"issue": 7, "id": 0, "author": "u", "created_at": "t", "body": "b0"}, + ) + self.assertIsNone(comments[-1]["author"]) + self.assertIn("page=1", calls[0]) + self.assertIn("page=2", calls[1]) + + @patch.object(gh, "fetch_issue_comments") + def test_cmd_comments_outputs_every_issue(self, fetch_comments): + fetch_comments.side_effect = lambda num: [ + {"issue": num, "id": num, "author": "u", "created_at": "t", "body": "x"} + ] + args = types.SimpleNamespace(numbers=[1, 2], file=None, stdin=False) + stdout = io.StringIO() + + with contextlib.redirect_stdout(stdout), contextlib.redirect_stderr(io.StringIO()): + gh.cmd_comments(args) + + self.assertEqual([c["issue"] for c in json.loads(stdout.getvalue())], [1, 2]) + + def test_cmd_comments_requires_numbers(self): + args = types.SimpleNamespace(numbers=[], file=None, stdin=False) + with contextlib.redirect_stderr(io.StringIO()): + with self.assertRaises(SystemExit) as error: + gh.cmd_comments(args) + self.assertEqual(error.exception.code, 1) + + if __name__ == "__main__": unittest.main(verbosity=2) From 0e9a9a8950fcefbdd56a4e174a78839d2540710c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Barrag=C3=A1n=20Merino?= <bameda@dbarragan.com> Date: Tue, 3 Nov 2026 23:34:16 +0100 Subject: [PATCH 03/13] :boom: Enable admin console by default (#12475) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: David Barragán Merino <david.barragan@kaleidos.net> --- backend/test/backend_tests/helpers.clj | 3 ++- backend/test/backend_tests/rpc_auth_test.clj | 12 ++++++------ common/src/app/common/flags.cljc | 5 +++-- docker/images/docker-compose.yaml | 2 +- docker/images/files/nginx-entrypoint.sh | 2 +- docs/technical-guide/getting-started/docker.md | 14 ++++++++++---- frontend/playwright/ui/pages/BasePage.js | 6 +++++- plugins/apps/composable-test-suite/ci/run-ci.ts | 2 +- plugins/apps/plugin-api-test-suite/ci/run-ci.ts | 2 +- 9 files changed, 30 insertions(+), 18 deletions(-) diff --git a/backend/test/backend_tests/helpers.clj b/backend/test/backend_tests/helpers.clj index 58b36244ea..5148916464 100644 --- a/backend/test/backend_tests/helpers.clj +++ b/backend/test/backend_tests/helpers.clj @@ -86,7 +86,8 @@ :enable-quotes :enable-rpc-climit :enable-auto-file-snapshot - :disable-file-validation]) + :disable-file-validation + :disable-admin-console]) (defn init-config ([next] diff --git a/backend/test/backend_tests/rpc_auth_test.clj b/backend/test/backend_tests/rpc_auth_test.clj index 802c70ac08..dcc8ffdcbf 100644 --- a/backend/test/backend_tests/rpc_auth_test.clj +++ b/backend/test/backend_tests/rpc_auth_test.clj @@ -122,7 +122,7 @@ (.close ^AutoCloseable conn))))) (t/deftest login-with-password-locks-after-max-attempts - (with-redefs [app.config/flags (flags/parse flags/default [:enable-account-lockout])] + (with-redefs [app.config/flags (flags/parse flags/default [:enable-account-lockout :disable-admin-console])] (let [profile (th/create-profile* 200 {:is-active true})] (cleanup-redis (:id profile)) ;; 4 failed attempts — not yet locked @@ -141,7 +141,7 @@ (t/is (th/ex-of-code? (:error out) :account-locked)))))) (t/deftest login-with-password-returns-rate-limit-when-locked - (with-redefs [app.config/flags (flags/parse flags/default [:enable-account-lockout])] + (with-redefs [app.config/flags (flags/parse flags/default [:enable-account-lockout :disable-admin-console])] (let [profile (th/create-profile* 201 {:is-active true})] (cleanup-redis (:id profile)) ;; Trigger lockout @@ -161,7 +161,7 @@ (t/is (pos? ttl) "ttl should be a positive number")))))) (t/deftest successful-login-clears-failed-attempts - (with-redefs [app.config/flags (flags/parse flags/default [:enable-account-lockout])] + (with-redefs [app.config/flags (flags/parse flags/default [:enable-account-lockout :disable-admin-console])] (let [profile (th/create-profile* 202 {:is-active true})] (cleanup-redis (:id profile)) ;; 3 failed attempts @@ -184,7 +184,7 @@ (t/is (th/ex-of-code? (:error out) :wrong-credentials))))))) (t/deftest lockout-does-not-apply-when-flag-disabled - (with-redefs [app.config/flags (flags/parse flags/default [])] + (with-redefs [app.config/flags (flags/parse flags/default [:disable-admin-console])] (let [profile (th/create-profile* 203 {:is-active true})] (cleanup-redis (:id profile)) ;; 10 failed attempts — no lockout without the flag @@ -199,7 +199,7 @@ "Verify that login-with-ldap short-circuits when account is locked, without attempting LDAP authentication. Calls the actual command function with a mock provider to test the full flow." - (with-redefs [app.config/flags (flags/parse flags/default [:enable-account-lockout :login-with-ldap]) + (with-redefs [app.config/flags (flags/parse flags/default [:enable-account-lockout :login-with-ldap :disable-admin-console]) ldap/authenticate (constantly nil)] (let [profile (th/create-profile* 204 {:is-active true}) cfg (assoc th/*system* ::ldap/provider {})] @@ -217,7 +217,7 @@ (t/deftest recover-profile-clears-lockout "Verify that clearing attempts (e.g. after password reset) unlocks the account." - (with-redefs [app.config/flags (flags/parse flags/default [:enable-account-lockout])] + (with-redefs [app.config/flags (flags/parse flags/default [:enable-account-lockout :disable-admin-console])] (let [profile (th/create-profile* 205 {:is-active true}) cfg th/*system*] (cleanup-redis (:id profile)) diff --git a/common/src/app/common/flags.cljc b/common/src/app/common/flags.cljc index 164d8c78a9..26f1d54390 100644 --- a/common/src/app/common/flags.cljc +++ b/common/src/app/common/flags.cljc @@ -174,7 +174,7 @@ ;; redis for caching data :redis-cache - ;; Activates the admin-console module + ;; enabled by default. Activates the admin-console module :admin-console ;; disabled by default. When enabled, allows the admin-console @@ -235,7 +235,8 @@ :enable-stroke-per-side :enable-token-combobox :enable-custom-shortcuts - :enable-token-lib-sync]) + :enable-token-lib-sync + :enable-admin-console]) (defn parse [& flags] diff --git a/docker/images/docker-compose.yaml b/docker/images/docker-compose.yaml index e667bc3049..932fda6279 100644 --- a/docker/images/docker-compose.yaml +++ b/docker/images/docker-compose.yaml @@ -24,7 +24,7 @@ # WARNING: if you're exposing Penpot to the internet, you should remove the flags # 'disable-secure-session-cookies' and 'disable-email-verification' x-flags: &penpot-flags - PENPOT_FLAGS: disable-email-verification enable-smtp enable-prepl-server disable-secure-session-cookies enable-mcp enable-admin-console + PENPOT_FLAGS: disable-email-verification enable-smtp enable-prepl-server disable-secure-session-cookies enable-mcp x-uri: &penpot-public-uri PENPOT_PUBLIC_URI: http://localhost:9001 diff --git a/docker/images/files/nginx-entrypoint.sh b/docker/images/files/nginx-entrypoint.sh index 3ec97403a8..e76e581f1c 100644 --- a/docker/images/files/nginx-entrypoint.sh +++ b/docker/images/files/nginx-entrypoint.sh @@ -61,7 +61,7 @@ fi envsubst "\$PENPOT_BACKEND_URI,\$PENPOT_EXPORTER_URI,\$PENPOT_HTTP_SERVER_MAX_BODY_SIZE,\$PENPOT_IPV6_LISTEN_DIRECTIVE" \ < /tmp/nginx.conf.template > /etc/nginx/nginx.conf -if [[ $PENPOT_FLAGS == *"enable-admin-console"* ]]; then +if [[ " $PENPOT_FLAGS " != *" disable-admin-console "* ]]; then export PENPOT_ADMIN_CONSOLE_URI=${PENPOT_ADMIN_CONSOLE_URI:-http://penpot-admin-console:3000} envsubst "\$PENPOT_ADMIN_CONSOLE_URI" \ < /tmp/nginx-admin-console-locations.conf.template > /etc/nginx/overrides/server.d/admin-console-locations.conf diff --git a/docs/technical-guide/getting-started/docker.md b/docs/technical-guide/getting-started/docker.md index 4d192aee11..3d204f6a60 100644 --- a/docs/technical-guide/getting-started/docker.md +++ b/docs/technical-guide/getting-started/docker.md @@ -122,6 +122,13 @@ This will fetch the latest images. When you do <code class="language-bash">docke It is strongly recommended to update the Penpot version in small increments, rather than updating between two distant versions. </p> +#### Upgrade to 2.19 + +The Admin Console is now enabled by default, so the `enable-admin-console` flag is no longer +needed and can be removed from `PENPOT_FLAGS`. The `penpot-admin-console` service and the +`PENPOT_ADMIN_CONSOLE_URI` variable are now required: if you haven't set them up yet, follow +steps 2–4 of the [2.18 upgrade](#upgrade-to-2.18). + #### Upgrade to 2.18 This version deploys a new service, **`penpot-admin-console`**, in the official `docker-compose.yaml` @@ -132,10 +139,9 @@ example. If you maintain your own compose file, you need to replicate the follow **NOTE:** Enabling the Admin Console is not required in this version, but it will be in a future release. We recommend setting it up now to avoid a more complex upgrade later. -This step (the `enable-admin-console` flag) only applies to this version 2.18. In a future release, the -Admin Console will be enabled by default and this flag will be removed. If you're upgrading directly to a -version where the flag has already been removed, skip step 1 and go straight to steps 2–4, which remain -required. +This step (the `enable-admin-console` flag) only applies to this version 2.18. Since 2.19 the Admin +Console is enabled by default: if you're upgrading directly to 2.19 or later, skip step 1 and go straight +to steps 2–4, which remain required. Wherever you set `PENPOT_FLAGS` (frontend and backend): diff --git a/frontend/playwright/ui/pages/BasePage.js b/frontend/playwright/ui/pages/BasePage.js index 670e8a2766..f8f246058d 100644 --- a/frontend/playwright/ui/pages/BasePage.js +++ b/frontend/playwright/ui/pages/BasePage.js @@ -101,7 +101,11 @@ export class BasePage { static async mockConfigFlags(page, flags, extras = {}) { const url = "**/js/config.js*"; - const config = { penpotFlags: flags.join(" "), ...extras }; + // The e2e suite mocks the backend and has no admin console behind it. + const config = { + penpotFlags: ["disable-admin-console", ...flags].join(" "), + ...extras, + }; const body = Object.entries(config) .map(([key, value]) => `var ${key} = ${JSON.stringify(value)};`) .join("\n"); diff --git a/plugins/apps/composable-test-suite/ci/run-ci.ts b/plugins/apps/composable-test-suite/ci/run-ci.ts index d9403c6793..69186e9436 100644 --- a/plugins/apps/composable-test-suite/ci/run-ci.ts +++ b/plugins/apps/composable-test-suite/ci/run-ci.ts @@ -146,7 +146,7 @@ async function setupMockedRoutes(page: Page): Promise<void> { route.fulfill({ status: 200, contentType: "application/javascript", - body: 'var penpotFlags = "";\n', + body: 'var penpotFlags = "disable-admin-console";\n', }) ); diff --git a/plugins/apps/plugin-api-test-suite/ci/run-ci.ts b/plugins/apps/plugin-api-test-suite/ci/run-ci.ts index 559e6f0aea..8b97cf088a 100644 --- a/plugins/apps/plugin-api-test-suite/ci/run-ci.ts +++ b/plugins/apps/plugin-api-test-suite/ci/run-ci.ts @@ -242,7 +242,7 @@ async function setupMockedRoutes(page: Page): Promise<void> { route.fulfill({ status: 200, contentType: 'application/javascript', - body: 'var penpotFlags = "";\n', + body: 'var penpotFlags = "disable-admin-console";\n', }), ); From 35baa06606a5752e18e37c91f2a8fdc6c867e611 Mon Sep 17 00:00:00 2001 From: Andrey Antukh <niwi@niwi.nz> Date: Wed, 4 Nov 2026 12:25:01 +0100 Subject: [PATCH 04/13] :sparkles: Migrate legacy file payloads to file_data and delete orphans (#12785) * :sparkles: Migrate legacy file payloads to file_data and delete orphans Copy file.data and labelled file_change.data to file_data on boot (type main/snapshot, backend db, deleted_at preserved), then delete file_change rows with data but no label, which no read path reaches. Large instances pre-migrate with batched statements and fake this step. Closes #12784. AI-assisted-by: muse-spark-1.3-contributor * :books: Add 2.19.0 operations runbook for legacy file_data migration Manual pre-migration procedure for big instances, same statements as migration 0157 in batched transactions, following the convention of the file-change partition runbook. Closes #12784. AI-assisted-by: muse-spark-1.3-contributor * :bug: Address review feedback on the legacy file data migration Fix the issues raised in the review of the migration that moves the legacy file payloads to file_data. The migration deletes the unlabelled file_change rows that carry data, but restore-files still cleared their date, and the retention test asserted they came back. A row without a date never reaches objects-gc, which is the leak fixed in #12419. Restore now unmarks only snapshots, the rows that carry a label, and the retention test checks that the legacy payload keeps its date instead of coming back. Cover the conflict path that had no test: a pre-existing snapshot that collides with a labelled change row, and a second run that must leave the legacy columns null and the orphan row absent. The runbook analyses file_data after the inserts, so the planner does not wait for autovacuum. Drop app.srepl.procs.fdata-storage: the copy it performed is what the migration replaces and nothing requires it. Closes #12784 AI-assisted-by: deepseek-v4.1-flash --- .serena/memories/backend/subtleties.md | 4 +- backend/src/app/migrations.clj | 5 +- .../sql/0157-migrate-legacy-file-data.sql | 55 +++ backend/src/app/rpc/commands/files.clj | 16 +- backend/src/app/srepl/procs/fdata_storage.clj | 141 -------- .../file_change_retention_test.clj | 4 +- .../migrations_legacy_file_data_test.clj | 180 ++++++++++ operations/2.19.0-migrate-legacy-file-data.md | 323 ++++++++++++++++++ 8 files changed, 574 insertions(+), 154 deletions(-) create mode 100644 backend/src/app/migrations/sql/0157-migrate-legacy-file-data.sql delete mode 100644 backend/src/app/srepl/procs/fdata_storage.clj create mode 100644 backend/test/backend_tests/migrations_legacy_file_data_test.clj create mode 100644 operations/2.19.0-migrate-legacy-file-data.md diff --git a/.serena/memories/backend/subtleties.md b/.serena/memories/backend/subtleties.md index 1b4fa4ddec..2282c6d792 100644 --- a/.serena/memories/backend/subtleties.md +++ b/.serena/memories/backend/subtleties.md @@ -85,8 +85,8 @@ - A `file_data` row is one of three classes, told apart by `type`: `main` is the file's current state and carries `id = file_id`; `snapshot` is a point-in-time copy whose `id` is the one its `file_change` row shares; `fragment` is a piece of the pointer-map, referenced from the file's data. - Their lifecycles differ: `main` and `snapshot` live with the file (dated when it is deleted, undated when it is restored), while a `fragment` carries no date and is collected by `file_gc` once the file stops referencing it. Neither is a session buffer, so do not read a `fragment` as an ephemeral revision. - File snapshots (versions) keep their metadata row in `file_change` and their payload in `file_data` (`type = 'snapshot'`). With the `db` backend `file_change.data` stays NULL: that column is only written by the `legacy-db` backend, plus the rows written before the 2025-07 file storage refactor. -- Tell a snapshot apart by `label IS NOT NULL` (with `created_by` / `deleted_at` as needed), never by `file_change.data IS NOT NULL`: the label holds with every file data backend, and the payload column stopped meaning "this is a snapshot" when the payload moved out. Queries that identify versions must use the label. +- Tell a snapshot apart by `label IS NOT NULL` (with `created_by` / `deleted_at` as needed), never by `file_change.data IS NOT NULL`: the label holds with every file data backend, and the payload column stopped meaning "this is a snapshot" when the payload moved out. Queries that identify versions must use the label. `0157-migrate-legacy-file-data` moved the labelled leftovers to `file_data` and deleted the unlabelled payload rows, so from 2.19 on the label is the only marker there is. - The version quota counts of `app.rpc.quotes` (`sql:get-snapshots-per-file`, `sql:get-snapshots-per-team`) are served by the partial index `file_change__user_snapshots__idx`, whose predicate mirrors their where clause; changing one without the other drops the index only scan and the count walks the whole table again. - Revisions live only in `file_change` and have no `file_data` counterpart. They are born already scheduled for deletion one hour later (`app.rpc.commands.files-update`) and the snapshot paths cannot reach one, because they resolve their row through a `label IS NOT NULL` filter. -- A deletion date is assigned once and never moved: `restore-files` (`app.rpc.commands.files`) unmarks only durable history, the `:file` method of `app.tasks.delete-object` dates only rows that have no date yet, and `objects-gc` collects only rows that carry one. Unmarking a revision leaves it invisible to `objects-gc` for ever, because nothing sets that date again. +- A deletion date is assigned once and never moved: `restore-files` (`app.rpc.commands.files`) unmarks only durable history, which is `label IS NOT NULL` (the unlabelled rows that still carry a legacy payload are removed by `0157-migrate-legacy-file-data`, so restore leaves them dated), the `:file` method of `app.tasks.delete-object` dates only rows that have no date yet, and `objects-gc` collects only rows that carry one. Unmarking a revision leaves it invisible to `objects-gc` for ever, because nothing sets that date again. - Binfile v1/v3 import and the v3 export `get-file` step run inside one long transaction. They raise `idle_in_transaction_session_timeout` to 30 min (`app.binfile.common/transaction-idle-timeout-ms`) instead of disabling it, so a connection left idle while the ZIP is decoded or the file data is realized is bounded; `statement_timeout` stays at 300 s. diff --git a/backend/src/app/migrations.clj b/backend/src/app/migrations.clj index 34751179b7..42d1fab650 100644 --- a/backend/src/app/migrations.clj +++ b/backend/src/app/migrations.clj @@ -526,7 +526,10 @@ :fn (mg/resource "app/migrations/sql/0155-add-idx-to-file-change.sql")} {:name "0156-reset-pg-storage-defaults" - :fn (mg/resource "app/migrations/sql/0156-reset-pg-storage-defaults.sql")}]) + :fn (mg/resource "app/migrations/sql/0156-reset-pg-storage-defaults.sql")} + + {:name "0157-migrate-legacy-file-data" + :fn (mg/resource "app/migrations/sql/0157-migrate-legacy-file-data.sql")}]) (defn apply-migrations! [pool name migrations] diff --git a/backend/src/app/migrations/sql/0157-migrate-legacy-file-data.sql b/backend/src/app/migrations/sql/0157-migrate-legacy-file-data.sql new file mode 100644 index 0000000000..c7cd6d1fe4 --- /dev/null +++ b/backend/src/app/migrations/sql/0157-migrate-legacy-file-data.sql @@ -0,0 +1,55 @@ +-- Migrate legacy payloads to file_data: labelled file_change.data as +-- snapshots, file.data as mains (backend 'db', deleted_at copied so the +-- GC keeps collecting them), then delete orphan file_change rows with +-- data but no label (no read path reaches them). +-- +-- Idempotent: INSERTs ignore conflicts, UPDATEs only null the legacy +-- column where the file_data row exists. Big instances pre-migrate by +-- hand (operations/2.19.0-migrate-legacy-file-data.md) and fake this +-- step instead: +-- +-- INSERT INTO migrations (module, step) +-- VALUES ('main', '0157-migrate-legacy-file-data'); + +INSERT INTO file_data (file_id, id, created_at, modified_at, deleted_at, + type, backend, metadata, data) +SELECT file_id, id, created_at, updated_at, deleted_at, + 'snapshot', 'db', NULL, data + FROM file_change + WHERE data IS NOT NULL + AND label IS NOT NULL +ON CONFLICT (file_id, id) DO NOTHING; + +UPDATE file_change AS fc + SET data = NULL + WHERE fc.data IS NOT NULL + AND fc.label IS NOT NULL + AND EXISTS (SELECT 1 + FROM file_data AS fd + WHERE fd.file_id = fc.file_id + AND fd.id = fc.id + AND fd.type = 'snapshot'); + +INSERT INTO file_data (file_id, id, created_at, modified_at, deleted_at, + type, backend, metadata, data) +SELECT id, id, created_at, modified_at, deleted_at, + 'main', 'db', NULL, data + FROM file + WHERE data IS NOT NULL +ON CONFLICT (file_id, id) DO NOTHING; + +UPDATE file AS f + SET data = NULL + WHERE f.data IS NOT NULL + AND EXISTS (SELECT 1 + FROM file_data AS fd + WHERE fd.file_id = f.id + AND fd.id = f.id + AND fd.type = 'main'); + +-- Orphan revisions: legacy payloads without label. No file_data row +-- corresponds to them (snapshots are told apart by label), so there is +-- nothing to copy; the row itself is garbage. Delete it. +DELETE FROM file_change + WHERE data IS NOT NULL + AND label IS NULL; diff --git a/backend/src/app/rpc/commands/files.clj b/backend/src/app/rpc/commands/files.clj index ad5ebc3e30..d07a9c1280 100644 --- a/backend/src/app/rpc/commands/files.clj +++ b/backend/src/app/rpc/commands/files.clj @@ -1347,16 +1347,16 @@ WHERE file_id = ANY(?::uuid[])") (def ^:private sql:restore-file-changes - ;; Only durable history comes back: a snapshot (carries `label`) or a legacy - ;; payload row (carries `data`, the column the new model replaces with - ;; `file_data`). Revisions have neither and are deliberately left alone: they - ;; are born already scheduled for deletion one hour later, and the collector - ;; only looks at rows that carry a date, so unmarking one here would keep it - ;; in the table for ever. The `data` half is a guard for the rows still - ;; pending migration, not a dependency: drop it with the last one. + ;; Only durable history comes back: a snapshot, the rows that carry `label`. + ;; Everything else is deliberately left alone. Revisions are born already + ;; scheduled for deletion one hour later, and the collector only looks at + ;; rows that carry a date, so unmarking one here would keep it in the table + ;; for ever. Rows that still carry a legacy payload without a label are + ;; garbage that no read path reaches; 0157-migrate-legacy-file-data deletes + ;; them, so they must keep their date and go with the collector. "UPDATE file_change SET deleted_at = null WHERE file_id = ANY(?::uuid[]) - AND (label IS NOT NULL OR data IS NOT NULL)") + AND label IS NOT NULL") (def ^:private sql:restore-file-data "UPDATE file_data SET deleted_at = null diff --git a/backend/src/app/srepl/procs/fdata_storage.clj b/backend/src/app/srepl/procs/fdata_storage.clj deleted file mode 100644 index 5759aa43b9..0000000000 --- a/backend/src/app/srepl/procs/fdata_storage.clj +++ /dev/null @@ -1,141 +0,0 @@ -;; This Source Code Form is subject to the terms of the Mozilla Public -;; License, v. 2.0. If a copy of the MPL was not distributed with this -;; file, You can obtain one at http://mozilla.org/MPL/2.0/. -;; -;; Copyright (c) KALEIDOS SUBSIDIARY SL - -(ns app.srepl.procs.fdata-storage - (:require - [app.common.logging :as l] - [app.db :as db])) - -;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; -;; SNAPSHOTS -;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; - -(def sql:get-unmigrated-snapshots - "SELECT fc.id, fc.file_id - FROM file_change AS fc - WHERE fc.data IS NOT NULL - AND fc.label IS NOT NULL - ORDER BY fc.id ASC") - -(def sql:get-migrated-snapshots - "SELECT f.id, f.file_id - FROM file_data AS f - WHERE f.data IS NOT NULL - AND f.type = 'snapshot' - AND f.id != f.file_id - ORDER BY f.id ASC") - -(defn migrate-snapshot-to-storage - "Migrate the current existing files to store data in new storage - tables." - {:query sql:get-unmigrated-snapshots} - [{:keys [::db/conn]} {:keys [id file-id]} & {:as options}] - (let [{:keys [id file-id data created-at updated-at]} - (db/get* conn :file-change {:id id :file-id file-id} - ::db/for-update true - ::db/remove-deleted false)] - (when data - (l/inf :hint "migrating snapshot" :file-id (str file-id) :id (str id)) - (db/update! conn :file-change - {:data nil} - {:id id :file-id file-id} - {::db/return-keys false}) - (db/insert! conn :file-data - {:backend "db" - :metadata nil - :type "snapshot" - :data data - :created-at created-at - :modified-at updated-at - :file-id file-id - :id id} - {::db/return-keys false})))) - -(defn rollback-snapshot-from-storage - "Migrate back to the file table storage." - {:query sql:get-unmigrated-snapshots} - [{:keys [::db/conn]} {:keys [id file-id]} & {:as opts}] - (when-let [{:keys [id file-id data]} - (db/get* conn :file-data {:id id :file-id file-id :type "snapshot"} - ::db/for-update true - ::db/remove-deleted false)] - (l/inf :hint "rollback snapshot" :file-id (str file-id) :id (str id)) - (db/update! conn :file-change - {:data data} - {:id id :file-id file-id} - {::db/return-keys false}) - (db/delete! conn :file-data - {:id id :file-id file-id :type "snapshot"} - {::db/return-keys false}))) - -;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; -;; FILES -;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; - -(def sql:get-unmigrated-files - "SELECT f.id - FROM file AS f - WHERE f.data IS NOT NULL - ORDER BY f.modified_at ASC") - -(def sql:get-migrated-files - "SELECT f.id, f.file_id - FROM file_data AS f - WHERE f.data IS NOT NULL - AND f.id = f.file_id - ORDER BY f.id ASC") - -(defn migrate-file-to-storage - "Migrate the current existing files to store data in new storage - tables." - {:query sql:get-unmigrated-files} - [{:keys [::db/conn] :as cfg} {:keys [id]} & {:as opts}] - (let [{:keys [id data created-at modified-at]} - (db/get* conn :file {:id id} - ::db/for-update true - ::db/remove-deleted false)] - - (when data - (l/inf :hint "migrating file" :file-id (str id)) - - (db/update! conn :file {:data nil} {:id id} ::db/return-keys false) - (db/insert! conn :file-data - {:backend "db" - :metadata nil - :type "main" - :data data - :created-at created-at - :modified-at modified-at - :file-id id - :id id} - {::db/return-keys false})) - - (let [snapshots-sql - (str "WITH snapshots AS (" sql:get-unmigrated-snapshots ") " - "SELECT s.* FROM snapshots AS s WHERE s.file_id = ?")] - (run! (fn [params] - (migrate-snapshot-to-storage cfg params opts)) - (db/plan cfg [snapshots-sql id]))))) - - -(defn rollback-file-from-storage - "Migrate back to the file table storage." - {:query sql:get-migrated-files} - [{:keys [::db/conn] :as cfg} {:keys [id]} & {:as opts}] - (when-let [{:keys [id data]} - (db/get* conn :file-data {:id id :file-id id :type "main"} - ::db/for-update true - ::db/remove-deleted false)] - (l/inf :hint "rollback file" :file-id (str id)) - (db/update! conn :file {:data data} {:id id} ::db/return-keys false) - (db/delete! conn :file-data {:file-id id :id id :type "main"} ::db/return-keys false) - - (let [snapshots-sql - (str "WITH snapshots AS (" sql:get-migrated-snapshots ") " - "SELECT s.* FROM snapshots AS s WHERE s.file_id = ?")] - (run! (fn [params] - (rollback-snapshot-from-storage cfg params opts)) - (db/plan cfg [snapshots-sql id]))))) diff --git a/backend/test/backend_tests/file_change_retention_test.clj b/backend/test/backend_tests/file_change_retention_test.clj index 7cede5a164..3929e0b129 100644 --- a/backend/test/backend_tests/file_change_retention_test.clj +++ b/backend/test/backend_tests/file_change_retention_test.clj @@ -118,8 +118,8 @@ (t/testing "the snapshot comes back" (t/is (nil? (:deleted-at (find-change snapshot-id))))) - (t/testing "the legacy payload row comes back" - (t/is (nil? (:deleted-at (find-change legacy-id)))))))) + (t/testing "the legacy payload row keeps its date" + (t/is (= trash-at (:deleted-at (find-change legacy-id)))))))) (t/deftest deleting-a-file-does-not-push-the-revision-schedule-out (binding [ct/*clock* (ct/fixed-clock test-now)] diff --git a/backend/test/backend_tests/migrations_legacy_file_data_test.clj b/backend/test/backend_tests/migrations_legacy_file_data_test.clj new file mode 100644 index 0000000000..e2a31ce2bf --- /dev/null +++ b/backend/test/backend_tests/migrations_legacy_file_data_test.clj @@ -0,0 +1,180 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS SUBSIDIARY SL + +(ns backend-tests.migrations-legacy-file-data-test + "Covers the 0157 data migration: legacy file/file_change payloads move + to file_data, keeping timestamps and deleted_at, nulling the legacy + column; unlabelled file_change payloads (orphan revisions) are + deleted." + (:require + [app.common.time :as ct] + [app.common.uuid :as uuid] + [app.db :as db] + [app.util.blob :as blob] + [backend-tests.helpers :as th] + [clojure.java.io :as io] + [clojure.test :as t] + [next.jdbc :as jdbc])) + +(t/use-fixtures :once th/state-init) +(t/use-fixtures :each th/database-reset) + +(defn- run-migration + [] + (let [sql (slurp (io/resource "app/migrations/sql/0157-migrate-legacy-file-data.sql"))] + (jdbc/execute! th/*pool* [sql]))) + +(defn- get-file-data + [file-id id] + (th/db-get :file-data {:file-id file-id :id id} {::db/remove-deleted false})) + +(t/deftest migrates-legacy-file-and-snapshot + (let [profile (th/create-profile* 1) + file (th/create-file* 1 {:profile-id (:id profile) + :project-id (:default-project-id profile)}) + fid (:id file) + + ;; Simulate an unmigrated main: move the payload back to + ;; file.data and drop the file_data row. + main-data (:data (get-file-data fid fid)) + _ (t/is (some? main-data) "expected file_data main row from create-file*") + file-row (th/db-get :file {:id fid} {::db/remove-deleted false}) + _ (th/db-delete! :file-data {:file-id fid :id fid}) + _ (th/db-update! :file {:data main-data} {:id fid}) + + ;; Simulate an unmigrated snapshot (labelled) with its own + ;; timestamps and a deletion date. + snap-id (uuid/next) + snap-data (blob/encode {:snapshot 1}) + created-at (ct/in-past "2h") + updated-at (ct/in-past "1h") + deleted-at (ct/in-past "30m") + _ (th/db-insert! :file-change + {:id snap-id + :file-id fid + :revn 1 + :data snap-data + :label "v1" + :created-at created-at + :updated-at updated-at + :deleted-at deleted-at}) + + ;; Orphan revision (data, no label): deleted by the migration. + rev-id (uuid/next) + rev-data (blob/encode {:rev 1}) + _ (th/db-insert! :file-change + {:id rev-id + :file-id fid + :revn 2 + :data rev-data}) + + ;; Live revision (changes, no data, no label): untouched. + live-id (uuid/next) + _ (th/db-insert! :file-change + {:id live-id + :file-id fid + :revn 3 + :changes (blob/encode [{:type :dummy}])})] + + (run-migration) + + (t/testing "main payload moves to file_data as type main" + (let [fd (get-file-data fid fid)] + (t/is (some? fd)) + (t/is (= "main" (:type fd))) + (t/is (= "db" (:backend fd))) + (t/is (nil? (:metadata fd))) + (t/is (= (seq ^bytes main-data) (seq ^bytes (:data fd)))) + (t/is (= (:created-at file-row) (:created-at fd))) + (t/is (= (:modified-at file-row) (:modified-at fd))) + (t/is (= (:deleted-at file-row) (:deleted-at fd))))) + + (t/testing "snapshot payload moves to file_data as type snapshot" + (let [fd (get-file-data fid snap-id)] + (t/is (some? fd)) + (t/is (= "snapshot" (:type fd))) + (t/is (= "db" (:backend fd))) + (t/is (nil? (:metadata fd))) + (t/is (= (seq ^bytes snap-data) (seq ^bytes (:data fd)))) + ;; NOTE: timestamptz stores microsecond precision while the + ;; test instants carry nanoseconds, so compare at millis. + (t/is (= (inst-ms created-at) (inst-ms (:created-at fd)))) + (t/is (= (inst-ms updated-at) (inst-ms (:modified-at fd)))) + (t/is (= (inst-ms deleted-at) (inst-ms (:deleted-at fd)))))) + + (t/testing "legacy columns are nulled" + (t/is (nil? (:data (th/db-get :file {:id fid} {::db/remove-deleted false})))) + (t/is (nil? (:data (th/db-get :file-change {:id snap-id} {::db/remove-deleted false}))))) + + (t/testing "unlabelled payload row is deleted, live revision stays" + (t/is (nil? (th/db-get :file-change {:id rev-id} {::db/remove-deleted false}))) + (let [row (th/db-get :file-change {:id live-id} {::db/remove-deleted false})] + (t/is (some? row)) + (t/is (some? (:changes row))))) + + (t/testing "second run is a no-op" + (run-migration) + (t/is (some? (get-file-data fid fid))) + (t/is (some? (get-file-data fid snap-id))) + (t/is (nil? (:data (th/db-get :file {:id fid} {::db/remove-deleted false})))) + (t/is (nil? (:data (th/db-get :file-change {:id snap-id} {::db/remove-deleted false})))) + (t/is (nil? (th/db-get :file-change {:id rev-id} {::db/remove-deleted false})))))) + +(t/deftest keeps-preexisting-file-data-row + (let [profile (th/create-profile* 1) + file (th/create-file* 1 {:profile-id (:id profile) + :project-id (:default-project-id profile)}) + fid (:id file) + + ;; file_data already holds the current payload; the legacy + ;; column keeps a stale copy that must not overwrite it. + current (:data (get-file-data fid fid)) + stale (blob/encode {:stale true}) + _ (t/is (some? current)) + _ (t/is (not= (seq ^bytes current) (seq ^bytes stale))) + _ (th/db-update! :file {:data stale} {:id fid})] + + (run-migration) + + (t/testing "file_data keeps its data and legacy is nulled" + (let [fd (get-file-data fid fid)] + (t/is (= (seq ^bytes current) (seq ^bytes (:data fd))))) + (t/is (nil? (:data (th/db-get :file {:id fid} {::db/remove-deleted false}))))))) + +(t/deftest keeps-preexisting-snapshot-row + (let [profile (th/create-profile* 1) + file (th/create-file* 1 {:profile-id (:id profile) + :project-id (:default-project-id profile)}) + fid (:id file) + + ;; file_data already holds the snapshot payload; the legacy column + ;; keeps a stale copy that must not overwrite it. + snap-id (uuid/next) + current (blob/encode {:snapshot :current}) + stale (blob/encode {:snapshot :stale}) + _ (t/is (not= (seq ^bytes current) (seq ^bytes stale))) + _ (th/db-insert! :file-data + {:file-id fid + :id snap-id + :type "snapshot" + :backend "db" + :metadata nil + :data current}) + _ (th/db-insert! :file-change + {:id snap-id + :file-id fid + :revn 1 + :label "v1" + :data stale})] + + (run-migration) + + (t/testing "file_data keeps its data and legacy is nulled" + (let [fd (get-file-data fid snap-id)] + (t/is (= (seq ^bytes current) (seq ^bytes (:data fd))))) + (t/is (nil? (:data (th/db-get :file-change {:id snap-id} + {::db/remove-deleted false}))))))) + diff --git a/operations/2.19.0-migrate-legacy-file-data.md b/operations/2.19.0-migrate-legacy-file-data.md new file mode 100644 index 0000000000..df3c82d917 --- /dev/null +++ b/operations/2.19.0-migrate-legacy-file-data.md @@ -0,0 +1,323 @@ +# Migrate legacy file payloads to `file_data` + +Online procedure to move the legacy file payloads (`file.data` and labelled +`file_change.data`) to `file_data` and to delete the orphan `file_change` +rows that carry `data` without a `label`. + +- Migration: `backend/src/app/migrations/sql/0157-migrate-legacy-file-data.sql` +- Issue: <https://github.com/penpot/penpot/issues/12784> + +The result has to be **exactly** the end state of the migration, because on a +big instance the migration is marked as applied and never runs. This procedure +runs the same statements by hand, in small batched transactions. + +Use this only if the automatic migration would take too long on the instance +(millions of snapshots, hundreds of GB in TOAST). Small and medium instances +run the migration automatically on boot and need nothing from here. + +Run it before upgrading to 2.19.0. + +## What changes + +- Every `file_change` row with `data IS NOT NULL AND label IS NOT NULL` + (a snapshot) gets a `file_data` row with `type='snapshot'`, + `backend='db'`, `metadata=NULL`, `created_at` from `created_at`, + `modified_at` from `updated_at`, and `deleted_at` copied as is. Then the + legacy `file_change.data` is set to `NULL`. +- Every `file` row with `data IS NOT NULL` gets a `file_data` row with + `type='main'` (and `file_id = id`), the same `backend`/`metadata` mapping + and the timestamps copied one to one. Then the legacy `file.data` is set + to `NULL`. +- Every `file_change` row with `data IS NOT NULL AND label IS NULL` (an + orphan revision) is deleted outright. No read path reaches these rows: + snapshots resolve through `label IS NOT NULL`, lagged changes read the + `changes` column (not `data`), and nothing references `file_change` rows + from other tables. Rows carrying `deleted_at` would be collected by + `objects-gc` anyway; undated ones would sit in the table and its TOAST + for ever. + +`deleted_at` is copied on purpose: the write path always writes it to +`file_data`, the GC only collects `file_data` rows carrying it, and reads +never filter on `file_data.deleted_at`. Without the copy, migrated rows of +deleted files and snapshots would be orphaned: nobody reads them and nobody +collects them. + +## Why this mechanism + +A single-statement copy inside the transaction of the startup does not fit a +big instance, so the copy runs here in small batched transactions: + +- One statement is one transaction. Never wrap several batches in one + transaction. Run a single stream of batches at a time. +- `FOR UPDATE SKIP LOCKED`: the batches never block the application and are + never blocked by it. A row skipped by a lock stays in the index and the + next execution picks it up. +- Partial temp indexes hold only the pending rows, so every execution finds + its next batch without rescanning what is done, and migrated rows + (`data = NULL`) and deleted orphans drop out of the index on their own. + No cursor is needed. Do not `VACUUM` mid-run to clean them: it would walk + the whole TOAST. Drop them at the end. +- No `ON CONFLICT DO NOTHING` here on purpose: a duplicate key must abort + the batch (atomically: it migrates nothing) so the conflict is + investigated instead of silently skipped. The automatic migration is + lenient so a small instance never fails to boot; here strictness is what + keeps the data auditable. + +--- + +## Phase 0 — Before anything + +### 0.1 Mark the migration as applied + +Do this first: it registers the automatic migration as done, so a backend +restart mid-procedure never runs it on boot (it would copy the whole legacy +payload inside the startup transaction). + +```sql +INSERT INTO migrations (module, step) +VALUES ('main', '0157-migrate-legacy-file-data'); +``` + +The module is `main` and the step has no `.sql` (see +`backend/src/app/util/migrations.clj`). + +### 0.2 Counters + +Repeat the counters any time to follow the progress. Each batch loop ends +when it returns 0; the check in phase 3 confirms nothing is pending. + +```sql +-- Remaining legacy rows. +SELECT (SELECT count(*) FROM file_change + WHERE data IS NOT NULL AND label IS NOT NULL) AS snapshots_remaining, + (SELECT count(*) FROM file_change + WHERE data IS NOT NULL AND label IS NULL) AS orphans_remaining, + (SELECT count(*) FROM file + WHERE data IS NOT NULL) AS files_remaining; +``` + +Optional, reference only: how much is already migrated. Skip it unless +curious: it is a full count over the partitioned table and can take a +while on a big instance. For progress, the remaining counters above and +the per-batch counts are enough. + +```sql +SELECT (SELECT count(*) FROM file_data + WHERE type = 'snapshot' AND backend = 'db') AS snapshots_in_file_data, + (SELECT count(*) FROM file_data + WHERE type = 'main' AND backend = 'db') AS mains_in_file_data; +``` + +## Phase 1 — Temp indexes + +Required by the batch loops, created before them. `CONCURRENTLY` so writes +are not blocked; each statement is its own transaction. + +```sql +CREATE INDEX CONCURRENTLY tmp_file_change_mig ON file_change (id) + WHERE data IS NOT NULL AND label IS NOT NULL; +CREATE INDEX CONCURRENTLY tmp_file_change_orphan_mig ON file_change (id) + WHERE data IS NOT NULL AND label IS NULL; +CREATE INDEX CONCURRENTLY tmp_file_mig ON file (id) + WHERE data IS NOT NULL; +``` + +A failed `CONCURRENTLY` build stays `INVALID` and the planner ignores it +silently, so verify all three indexes exist and are valid: + +```sql +SELECT indexrelid::regclass AS index, indisvalid AS valid + FROM pg_index + WHERE indexrelid::regclass::text LIKE 'tmp_%mig'; +``` + +## Phase 2 — Batch loops + +Repeat each statement until it returns 0. Lower `LIMIT` if a batch hits +`statement_timeout` or memory pressure (`SET statement_timeout = '15min'` +per batch is the usual guard). + +### 2.1 Delete orphans + +Orphans first (or interleaved with the other two; the row sets are +disjoint, and deleting TOAST-heavy rows early frees space for the rest): + +```sql +WITH doomed AS ( + SELECT id + FROM file_change + WHERE data IS NOT NULL + AND label IS NULL + ORDER BY id + LIMIT 10000 + FOR UPDATE SKIP LOCKED +), +del AS ( + DELETE FROM file_change fc + USING doomed + WHERE fc.id = doomed.id + RETURNING fc.id +) +SELECT count(*) AS deleted + FROM del; +``` + +### 2.2 Migrate snapshots + +Repeat until it returns 0 (`~120 MB` per transaction at `LIMIT 10000`): + +```sql +WITH batch AS ( + SELECT id, file_id, data, created_at, updated_at, deleted_at + FROM file_change + WHERE data IS NOT NULL + AND label IS NOT NULL + ORDER BY id + LIMIT 10000 + FOR UPDATE SKIP LOCKED +), +-- NO ON CONFLICT on purpose: duplicates must abort (see above). +-- upd reads from ins (RETURNING), not from batch. +ins AS ( + INSERT INTO file_data (file_id, id, created_at, modified_at, deleted_at, + type, backend, metadata, data) + SELECT file_id, id, created_at, updated_at, deleted_at, + 'snapshot', 'db', NULL, data + FROM batch + RETURNING file_id, id +), +-- Only the origin of what was really inserted is nulled. On conflict +-- the whole transaction aborts, so ins == batch whenever this runs. +upd AS ( + UPDATE file_change fc + SET data = NULL + FROM ins + WHERE ins.file_id = fc.file_id + AND ins.id = fc.id + RETURNING fc.id +) +SELECT count(*) AS migrated + FROM upd; +``` + +### 2.3 Migrate files + +Repeat until it returns 0 (tune `LIMIT` to the average `file.data` size; +start at 5000): + +```sql +WITH batch AS ( + SELECT id, data, created_at, modified_at, deleted_at + FROM file + WHERE data IS NOT NULL + ORDER BY id + LIMIT 5000 + FOR UPDATE SKIP LOCKED +), +ins AS ( + INSERT INTO file_data (file_id, id, created_at, modified_at, deleted_at, + type, backend, metadata, data) + SELECT id, id, created_at, modified_at, deleted_at, + 'main', 'db', NULL, data + FROM batch + RETURNING file_id, id +), +upd AS ( + UPDATE file f + SET data = NULL + FROM ins + WHERE ins.file_id = f.id + AND ins.id = f.id + RETURNING f.id +) +SELECT count(*) AS migrated + FROM upd; +``` + +### 2.4 Duplicate keys + +If a batch aborts on duplicate key, stop and do not blind-retry it: it +means `file_data` already holds that key. Locate the overlap: + +```sql +-- Snapshots with legacy data AND a file_data key already present. +SELECT c.file_id, c.id, c.created_by, c.deleted_at, + pg_size_pretty(length(c.data)) AS legacy_size, + pg_size_pretty(length(fd.data)) AS file_data_size, + fd.modified_at AS file_data_modified_at + FROM file_change c + JOIN file_data fd ON fd.file_id = c.file_id + AND fd.id = c.id + AND fd.type = 'snapshot' + WHERE c.data IS NOT NULL + ORDER BY c.id + LIMIT 20; + +-- Files with legacy data AND a file_data key already present. +SELECT f.id, + pg_size_pretty(length(f.data)) AS legacy_size, + pg_size_pretty(length(fd.data)) AS file_data_size, + fd.modified_at AS file_data_modified_at + FROM file f + JOIN file_data fd ON fd.file_id = f.id + AND fd.id = f.id + AND fd.type = 'main' + WHERE f.data IS NOT NULL + ORDER BY f.id + LIMIT 20; +``` + +Resolve manually, one row at a time, only when `file_data` holds equal or +newer data (compare `modified_at` and sizes above): the legacy copy is +redundant, null the origin, leave `file_data` untouched. + +```sql +-- UPDATE file_change SET data = NULL WHERE file_id = '...' AND id = '...'; +-- UPDATE file SET data = NULL WHERE id = '...'; +``` + +## Phase 3 — Verification and cleanup + +Each loop above already ended returning 0; confirm nothing is pending (if +anything shows up, repeat that loop): + +```sql +-- Remaining legacy rows: all three must read 0. +SELECT (SELECT count(*) FROM file_change + WHERE data IS NOT NULL AND label IS NOT NULL) AS snapshots_remaining, + (SELECT count(*) FROM file_change + WHERE data IS NOT NULL AND label IS NULL) AS orphans_remaining, + (SELECT count(*) FROM file + WHERE data IS NOT NULL) AS files_remaining; +``` + +```sql +-- Spot check: migrated rows exist in file_data with backend 'db'. +SELECT fd.file_id, fd.id, fd.type, fd.backend, + pg_size_pretty(length(fd.data)) AS size_in_file_data + FROM file_data fd + ORDER BY fd.file_id, fd.id + LIMIT 5; +``` + +Drop the temp indexes first, so the vacuum below skips them (each batch +left its dead entries behind), then reclaim the dead tuples and refresh +planner stats. Plain `VACUUM` does not block writes; `VERBOSE` prints +progress, which matters on big tables: + +```sql +DROP INDEX CONCURRENTLY tmp_file_change_mig; +DROP INDEX CONCURRENTLY tmp_file_change_orphan_mig; +DROP INDEX CONCURRENTLY tmp_file_mig; +VACUUM (ANALYZE ON, VERBOSE ON) file_change; +VACUUM (ANALYZE ON, VERBOSE ON) file; +-- file_data is partitioned into 16 tables: the statement walks all of +-- them, so it can take a while. Without it the planner stats of the fresh +-- inserts stay stale until autovacuum catches up. +VACUUM (ANALYZE ON, VERBOSE ON) file_data; +``` + +## Phase 4 — Upgrade + +The migration was marked as applied in phase 0, so the new version skips +it on boot. Deploy normally. From 030509c6fda1708e54312ac9ab2fb365222c8328 Mon Sep 17 00:00:00 2001 From: Francis Santiago <francis.santiago@kaleidos.net> Date: Wed, 4 Nov 2026 12:46:37 +0100 Subject: [PATCH 05/13] :arrow_up: Update DHI base image digests Refresh the pinned DHI digests used by Penpot's container builds. Stale Debian bases conflict with current APT package versions. AI-assisted-by: gpt-6 Signed-off-by: Francis Santiago <francis.santiago@kaleidos.net> --- docker/images/Dockerfile.backend | 4 ++-- docker/images/Dockerfile.exporter | 2 +- docker/images/Dockerfile.frontend | 2 +- docker/images/Dockerfile.mcp | 4 ++-- docker/images/Dockerfile.media-processor | 2 +- docker/images/Dockerfile.storybook | 2 +- 6 files changed, 8 insertions(+), 8 deletions(-) diff --git a/docker/images/Dockerfile.backend b/docker/images/Dockerfile.backend index 4618c1ba91..3edb969257 100644 --- a/docker/images/Dockerfile.backend +++ b/docker/images/Dockerfile.backend @@ -1,4 +1,4 @@ -FROM dhi.io/debian-base:trixie-debian13-dev@sha256:cc4e05ff415ee2d83918f2af45c737983209924ef7362c53f00cfcdaa1c94326 AS build +FROM dhi.io/debian-base:trixie-debian13-dev@sha256:af61a3296b0c92f16969f11ce8c2a93722e84c9e5c1bb42ed6afec796be71b04 AS build LABEL maintainer="Penpot <docker@penpot.app>" ENV LANG='C.UTF-8' \ @@ -53,7 +53,7 @@ RUN set -eux; \ --output /opt/jre; -FROM dhi.io/debian-base:trixie-debian13-dev@sha256:cc4e05ff415ee2d83918f2af45c737983209924ef7362c53f00cfcdaa1c94326 AS image +FROM dhi.io/debian-base:trixie-debian13-dev@sha256:af61a3296b0c92f16969f11ce8c2a93722e84c9e5c1bb42ed6afec796be71b04 AS image LABEL maintainer="Penpot <docker@penpot.app>" ENV LANG='C.UTF-8' \ diff --git a/docker/images/Dockerfile.exporter b/docker/images/Dockerfile.exporter index b427e0bc80..bb6fa216da 100644 --- a/docker/images/Dockerfile.exporter +++ b/docker/images/Dockerfile.exporter @@ -1,4 +1,4 @@ -FROM dhi.io/node:24.22.2-debian13-dev@sha256:53af56f14166cffcf9c4b9ef6f7025d1b0534edddfa1f333c53bd54766f39e5c +FROM dhi.io/node:24.22.2-debian13-dev@sha256:df819cfcb0de8a9e461bb8437d96d6fcfac341e565129e50597338756a88db5b LABEL maintainer="Penpot <docker@penpot.app>" ENV LANG=en_US.UTF-8 \ diff --git a/docker/images/Dockerfile.frontend b/docker/images/Dockerfile.frontend index 05939a1982..aba52d5bcc 100644 --- a/docker/images/Dockerfile.frontend +++ b/docker/images/Dockerfile.frontend @@ -1,4 +1,4 @@ -FROM dhi.io/nginx:1.31.8-debian13-dev@sha256:fbff190f9162256975ed225adcc9a294e333897de543fd18be630de1d6c8de17 +FROM dhi.io/nginx:1.31.8-debian13-dev@sha256:f69a0ea1762c51080fe96ce5f12103fb2977231308ae1784acd915bf63a0a1a2 LABEL maintainer="Penpot <docker@penpot.app>" ENV DEBIAN_FRONTEND=noninteractive diff --git a/docker/images/Dockerfile.mcp b/docker/images/Dockerfile.mcp index aac0ff2a38..4054a613b0 100644 --- a/docker/images/Dockerfile.mcp +++ b/docker/images/Dockerfile.mcp @@ -1,4 +1,4 @@ -FROM dhi.io/node:24.22.2-debian13-dev@sha256:53af56f14166cffcf9c4b9ef6f7025d1b0534edddfa1f333c53bd54766f39e5c AS build +FROM dhi.io/node:24.22.2-debian13-dev@sha256:df819cfcb0de8a9e461bb8437d96d6fcfac341e565129e50597338756a88db5b AS build LABEL maintainer="Penpot <docker@penpot.app>" ENV DEBIAN_FRONTEND=noninteractive @@ -42,7 +42,7 @@ RUN set -eux; \ RUN ./setup -FROM dhi.io/node:24.22.2-debian13@sha256:24fa6258b891ef66e6c3b7ed82daad550b81cdfcd76f6cc64eee4d2478366747 AS image +FROM dhi.io/node:24.22.2-debian13@sha256:ce582f85254ceba6c572a854b8fb4008ced2f5d365c929f4f580360829037184 AS image LABEL maintainer="Penpot <docker@penpot.app>" ENV LANG=en_US.UTF-8 \ diff --git a/docker/images/Dockerfile.media-processor b/docker/images/Dockerfile.media-processor index b2e9eb4af2..218fd84ecd 100644 --- a/docker/images/Dockerfile.media-processor +++ b/docker/images/Dockerfile.media-processor @@ -1,5 +1,5 @@ # syntax=docker/dockerfile:1 -FROM dhi.io/node:24.22.2-debian13-dev@sha256:53af56f14166cffcf9c4b9ef6f7025d1b0534edddfa1f333c53bd54766f39e5c +FROM dhi.io/node:24.22.2-debian13-dev@sha256:df819cfcb0de8a9e461bb8437d96d6fcfac341e565129e50597338756a88db5b LABEL maintainer="Penpot <docker@penpot.app>" ENV LANG=en_US.UTF-8 \ diff --git a/docker/images/Dockerfile.storybook b/docker/images/Dockerfile.storybook index 4832c60054..bf67fabf88 100644 --- a/docker/images/Dockerfile.storybook +++ b/docker/images/Dockerfile.storybook @@ -1,4 +1,4 @@ -FROM dhi.io/nginx:1.31.8-alpine3.24@sha256:83489ce440fa56cb5149c088dbf75422362e45c532c814100253e5ab71871366 +FROM dhi.io/nginx:1.31.8-alpine3.24@sha256:7a4e9021737f414ad0f851efb939600690d9d092f51712ce6512bfd0a767ceae LABEL maintainer="Penpot <docker@penpot.app>" ARG BUNDLE_PATH="./bundle-storybook/" From b2c9ca1d231270b508a1ec4fc650b972254e24b3 Mon Sep 17 00:00:00 2001 From: Pablo Alba <pablo.alba@kaleidos.net> Date: Fri, 30 Oct 2026 13:11:06 +0100 Subject: [PATCH 06/13] :bug: Fix mask shortcut works on variant main --- .../src/app/main/data/workspace/groups.cljs | 3 +- .../frontend_tests/logic/groups_test.cljs | 165 +++++++++++++++++- 2 files changed, 165 insertions(+), 3 deletions(-) diff --git a/frontend/src/app/main/data/workspace/groups.cljs b/frontend/src/app/main/data/workspace/groups.cljs index f60ae89ba8..46a8737774 100644 --- a/frontend/src/app/main/data/workspace/groups.cljs +++ b/frontend/src/app/main/data/workspace/groups.cljs @@ -290,7 +290,8 @@ selected (->> (or ids (dsh/lookup-selected state)) (cfh/clean-loops objects) (remove #(ctn/has-any-copy-parent? objects (get objects %)))) - shapes (shapes-for-grouping objects selected) + shapes (->> (shapes-for-grouping objects selected) + (remove ctk/is-variant?)) first-shape (first shapes)] (when-not (empty? shapes) (let [;; If the selected shape is a group, we can use it. If not, diff --git a/frontend/test/frontend_tests/logic/groups_test.cljs b/frontend/test/frontend_tests/logic/groups_test.cljs index 600e076096..709285ac74 100644 --- a/frontend/test/frontend_tests/logic/groups_test.cljs +++ b/frontend/test/frontend_tests/logic/groups_test.cljs @@ -6,19 +6,72 @@ (ns frontend-tests.logic.groups-test (:require [app.common.data :as d] + [app.common.test-helpers.components :as cthc] [app.common.test-helpers.compositions :as ctho] [app.common.test-helpers.files :as cthf] + [app.common.test-helpers.ids-map :as cthi] [app.common.test-helpers.shapes :as cths] + [app.common.test-helpers.variants :as thv] + [app.common.types.component :as ctk] [app.common.uuid :as uuid] + [app.main.data.changes :as dch] [app.main.data.workspace :as dw] [app.main.data.workspace.groups :as dwgr] [app.main.data.workspace.selection :as dws] + [beicon.v2.core :as rx] [cljs.test :as t :include-macros true] [frontend-tests.helpers.pages :as thp] - [frontend-tests.helpers.state :as ths])) + [frontend-tests.helpers.state :as ths] + [potok.v2.core :as ptk])) (t/use-fixtures :each - {:before thp/reset-idmap!}) + {:before (fn [] + (thp/reset-idmap!) + (cthi/reset-idmap!))}) + +(defn- variant-file + [] + (-> (cthf/sample-file :file1) + (thv/add-variant :variant :comp1 :root1 :comp2 :root2 + :variant1-params {:name "Button"} + :variant2-params {:name "Button"}) + (cths/update-shape :variant :name "Button"))) + +(defn- variant-file-with-rect + [] + (cths/add-sample-shape (variant-file) :rect)) + +(defn- masked-groups + [file] + (->> (cthf/current-page file) + :objects + vals + (filter :masked-group))) + +(defn- groups + [file] + (->> (cthf/current-page file) + :objects + vals + (filter #(= :group (:type %))))) + +(defn- run + "Run workspace events, then validate the file before assertions. + + Pass `:commits` as an atom to collect each `dch/commit?` event." + [file events done assert-fn & {:keys [commits]}] + (let [store (ths/setup-store file)] + (when commits + (->> (ptk/input-stream store) + (rx/filter dch/commit?) + (rx/subs! #(swap! commits conj %)))) + (ths/run-store + store + done + events + (fn [state] + (cthf/validate-file! (ths/get-file-from-state state)) + (assert-fn state))))) (t/deftest test-create-group @@ -49,3 +102,111 @@ ;; ==== Check ;; Group has been created and is selected (t/is (= (get-in new-state [:workspace-local :selected]) #{group-id})))))))) + +(t/deftest mask-group-leaves-variant-main-in-the-set + (t/async + done + (let [file (variant-file-with-rect) + rect (cths/get-shape file :rect) + main (cths/get-shape file :root1) + comp (cthc/get-component file :comp1)] + (run file + [(dwgr/mask-group [(cthi/id :root1) (cthi/id :rect)])] + done + (fn [state] + (let [file' (ths/get-file-from-state state) + main' (cths/get-shape file' :root1) + rect' (cths/get-shape file' :rect) + group (first (masked-groups file')) + comp' (cthc/get-component file' :comp1)] + (t/is (= (:parent-id main) (:parent-id main'))) + (t/is (= (:variant-id main) (:variant-id main'))) + (t/is (= (:variant-name main) (:variant-name main'))) + (t/is (= (:variant-id comp) (:variant-id comp'))) + (t/is (= (:id group) (:parent-id rect'))) + (t/is (= (:parent-id rect) (:parent-id group))) + (t/is (= (:selrect rect) (:selrect group))))))))) + +(t/deftest mask-group-of-only-variant-mains-does-not-commit + (t/async + done + (let [file (variant-file) + commits (atom [])] + (run file + [(dwgr/mask-group [(cthi/id :root1) (cthi/id :root2)])] + done + (fn [state] + (let [file' (ths/get-file-from-state state) + board (cths/get-shape file' :variant)] + (t/is (= 0 (count @commits))) + (t/is (empty? (masked-groups file'))) + (t/is (= (:id board) (:parent-id (cths/get-shape file' :root1)))) + (t/is (= (:id board) (:parent-id (cths/get-shape file' :root2)))))) + :commits commits)))) + +(t/deftest mask-group-of-ordinary-shapes-creates-a-mask + (t/async + done + (let [file (-> (cthf/sample-file :file1) + (cths/add-sample-shape :a) + (cths/add-sample-shape :b))] + (run file + [(dwgr/mask-group [(cthi/id :a) (cthi/id :b)])] + done + (fn [state] + (let [file' (ths/get-file-from-state state) + group (first (masked-groups file'))] + (t/is (= 1 (count (masked-groups file')))) + (t/is (:masked-group group)) + (t/is (= (:id group) (:parent-id (cths/get-shape file' :a)))) + (t/is (= (:id group) (:parent-id (cths/get-shape file' :b)))))))))) + +(t/deftest mask-group-marks-a-single-plain-group + (t/async + done + (let [file (-> (cthf/sample-file :file1) + (cths/add-sample-shape :g :type :group) + (cths/add-sample-shape :child :parent-label :g)) + group-id (cthi/id :g)] + (run file + [(dwgr/mask-group [group-id])] + done + (fn [state] + (let [file' (ths/get-file-from-state state) + group' (cths/get-shape file' :g)] + (t/is (:masked-group group')) + (t/is (= [group-id] (map :id (groups file')))) + (t/is (= group-id (:parent-id (cths/get-shape file' :child)))))))))) + +(t/deftest group-shapes-leaves-variant-main-in-the-set + (t/async + done + (let [file (variant-file-with-rect) + rect (cths/get-shape file :rect)] + (run file + [(dwgr/group-shapes nil [(cthi/id :root1) (cthi/id :rect)])] + done + (fn [state] + (let [file' (ths/get-file-from-state state) + board (cths/get-shape file' :variant) + main (cths/get-shape file' :root1) + rect' (cths/get-shape file' :rect) + group (first (groups file'))] + (t/is (= (:id board) (:parent-id main))) + (t/is (some? group)) + (t/is (= (:id group) (:parent-id rect'))) + (t/is (= (:parent-id rect) (:parent-id group))))))))) + +(t/deftest ungroup-shapes-leaves-a-variant-container + (t/async + done + (let [file (variant-file)] + (run file + [(dwgr/ungroup-shapes [(cthi/id :variant)])] + done + (fn [state] + (let [file' (ths/get-file-from-state state) + board (cths/get-shape file' :variant)] + (t/is (ctk/is-variant-container? board)) + (t/is (= (:id board) (:parent-id (cths/get-shape file' :root1)))) + (t/is (= (:id board) (:parent-id (cths/get-shape file' :root2)))))))))) From 69fa6822174ec3453b03a89744a6d69bb3e7a412 Mon Sep 17 00:00:00 2001 From: Pablo Alba <pablo.alba@kaleidos.net> Date: Fri, 30 Oct 2026 17:50:30 +0100 Subject: [PATCH 07/13] :bug: Keep variant sets intact under group, mask, board, and layout --- common/src/app/common/types/container.cljc | 46 ++ .../common_tests/types/container_test.cljc | 137 +++++ .../src/app/main/data/workspace/groups.cljs | 37 +- .../app/main/data/workspace/shape_layout.cljs | 22 +- .../src/app/main/data/workspace/shapes.cljs | 16 +- .../app/main/ui/workspace/context_menu.cljs | 12 +- .../frontend_tests/logic/groups_test.cljs | 555 +++++++++++++++++- 7 files changed, 757 insertions(+), 68 deletions(-) diff --git a/common/src/app/common/types/container.cljc b/common/src/app/common/types/container.cljc index cc7ea9bd07..d790999db0 100644 --- a/common/src/app/common/types/container.cljc +++ b/common/src/app/common/types/container.cljc @@ -426,6 +426,52 @@ true (has-any-copy-parent? objects (:parent-id shape)))))) +(defn get-variant-boundary-id + "Id of the nearest variant main or variant container for this shape, or nil + when the shape is outside any variant set. + + Nested children resolve to their enclosing main (not the container), so two + shapes under different variants are treated as different boundaries even + when they share a set." + [objects shape] + (some (fn [s] + (cond + (ctk/is-variant? s) (:id s) + (ctk/is-variant-container? s) (:id s) + :else nil)) + (cfh/get-parents-with-self objects (:id shape)))) + +(defn crosses-variant-set-boundary? + "True when the selection mixes shapes that belong to different variant + boundaries — including outside vs in-set, two different variant mains + (even in the same set), or two different variant containers. + + Structure commands (group, mask, board-from-selection, layout) must not + reparent across that boundary." + [objects shapes] + (let [ids (into #{} (map #(get-variant-boundary-id objects %)) shapes)] + (> (count ids) 1))) + +(defn ids-for-structure-op + "Ids allowed for Group, Mask, and Create board from selection. + + Drops copy-children and variant mains, refuses a cross-boundary or + container selection, then removes descendants whose ancestor is + still selected. Mains are dropped before loop-cleaning so a + selected main does not swallow nested children. Layout does not + use this helper." + [objects ids] + (let [shapes (->> ids + (remove #(has-any-copy-parent? objects (get objects %))) + (keep (d/getf objects)))] + (if (or (crosses-variant-set-boundary? objects shapes) + (some ctk/is-variant-container? shapes)) + [] + (->> shapes + (remove ctk/is-variant?) + (map :id) + (cfh/clean-loops objects))))) + (defn has-any-main? "Check if the shape is a main component or has any children or parent that is a main component." [objects shape] diff --git a/common/test/common_tests/types/container_test.cljc b/common/test/common_tests/types/container_test.cljc index e00b1fb9c6..9f7a308c8a 100644 --- a/common/test/common_tests/types/container_test.cljc +++ b/common/test/common_tests/types/container_test.cljc @@ -154,3 +154,140 @@ shape-b (make-shape id-b :parent-id id-a) objects (objects-map shape-a shape-b)] (t/is (nil? (ctc/find-component-main objects shape-a false)))))) + +(t/deftest get-variant-boundary-id-recognizes-container-main-and-nested + (let [page-id uuid/zero + container-id (uuid/next) + main-id (uuid/next) + nested-id (uuid/next) + outside-id (uuid/next) + container (make-shape container-id + :parent-id page-id + :type :frame + :is-variant-container true) + main (make-shape main-id + :parent-id container-id + :type :frame + :variant-id container-id) + nested (make-shape nested-id :parent-id main-id) + outside (make-shape outside-id :parent-id page-id) + objects (objects-map container main nested outside)] + (t/is (= main-id (ctc/get-variant-boundary-id objects nested))) + (t/is (= main-id (ctc/get-variant-boundary-id objects main))) + (t/is (= container-id (ctc/get-variant-boundary-id objects container))) + (t/is (nil? (ctc/get-variant-boundary-id objects outside))))) + +(t/deftest crosses-variant-set-boundary? + (let [page-id uuid/zero + container-id (uuid/next) + main-id (uuid/next) + main-2-id (uuid/next) + nested-id (uuid/next) + nested-2-id (uuid/next) + outside-id (uuid/next) + other-set-id (uuid/next) + other-main-id (uuid/next) + other-nest-id (uuid/next) + container (make-shape container-id + :parent-id page-id + :type :frame + :is-variant-container true) + main (make-shape main-id + :parent-id container-id + :type :frame + :variant-id container-id) + main-2 (make-shape main-2-id + :parent-id container-id + :type :frame + :variant-id container-id) + nested (make-shape nested-id :parent-id main-id) + nested-2 (make-shape nested-2-id :parent-id main-2-id) + outside (make-shape outside-id :parent-id page-id) + other-set (make-shape other-set-id + :parent-id page-id + :type :frame + :is-variant-container true) + other-main (make-shape other-main-id + :parent-id other-set-id + :type :frame + :variant-id other-set-id) + other-nested (make-shape other-nest-id :parent-id other-main-id) + objects (objects-map container main main-2 nested nested-2 + outside other-set other-main other-nested)] + (t/testing "nested child + outside crosses" + (t/is (true? (ctc/crosses-variant-set-boundary? + objects [nested outside])))) + (t/testing "main + outside crosses" + (t/is (true? (ctc/crosses-variant-set-boundary? + objects [main outside])))) + (t/testing "container + outside crosses" + (t/is (true? (ctc/crosses-variant-set-boundary? + objects [container outside])))) + (t/testing "nested children of two variants in the same set cross" + (t/is (true? (ctc/crosses-variant-set-boundary? + objects [nested nested-2])))) + (t/testing "nested children from two different sets cross" + (t/is (true? (ctc/crosses-variant-set-boundary? + objects [nested other-nested])))) + (t/testing "two outside shapes do not cross" + (let [outside-2 (make-shape (uuid/next) :parent-id page-id)] + (t/is (false? (ctc/crosses-variant-set-boundary? + (assoc objects (:id outside-2) outside-2) + [outside outside-2]))))) + (t/testing "nested only does not cross" + (t/is (false? (ctc/crosses-variant-set-boundary? objects [nested])))) + (t/testing "main + nested under that main do not cross" + (t/is (false? (ctc/crosses-variant-set-boundary? + objects [main nested])))) + (t/testing "two nested under the same main do not cross" + (let [nested-b (make-shape (uuid/next) :parent-id main-id)] + (t/is (false? (ctc/crosses-variant-set-boundary? + (assoc objects (:id nested-b) nested-b) + [nested nested-b]))))))) + +(t/deftest ids-for-structure-op + (let [page-id uuid/zero + container-id (uuid/next) + main-id (uuid/next) + main-2-id (uuid/next) + nested-id (uuid/next) + nested-2-id (uuid/next) + outside-id (uuid/next) + outside-2-id (uuid/next) + container (make-shape container-id + :parent-id page-id + :type :frame + :is-variant-container true) + main (make-shape main-id + :parent-id container-id + :type :frame + :variant-id container-id) + main-2 (make-shape main-2-id + :parent-id container-id + :type :frame + :variant-id container-id) + nested (make-shape nested-id :parent-id main-id) + nested-2 (make-shape nested-2-id :parent-id main-id) + outside (make-shape outside-id :parent-id page-id) + outside-2 (make-shape outside-2-id :parent-id page-id) + objects (objects-map container main main-2 nested nested-2 + outside outside-2)] + (t/testing "container-only is refused" + (t/is (= [] (vec (ctc/ids-for-structure-op objects [container-id]))))) + (t/testing "nested + outside is refused" + (t/is (= [] (vec (ctc/ids-for-structure-op objects [nested-id outside-id]))))) + (t/testing "main + outside is refused" + (t/is (= [] (vec (ctc/ids-for-structure-op objects [main-id outside-id]))))) + (t/testing "two mains cross and are refused" + (t/is (= [] (vec (ctc/ids-for-structure-op objects [main-id main-2-id]))))) + (t/testing "single main is dropped to empty" + (t/is (= [] (vec (ctc/ids-for-structure-op objects [main-id]))))) + (t/testing "main + nested under that main keeps only the nested" + (t/is (= [nested-id] + (vec (ctc/ids-for-structure-op objects [main-id nested-id]))))) + (t/testing "two nested under one main are both kept" + (t/is (= [nested-id nested-2-id] + (vec (ctc/ids-for-structure-op objects [nested-id nested-2-id]))))) + (t/testing "two ordinary outside shapes are both kept" + (t/is (= [outside-id outside-2-id] + (vec (ctc/ids-for-structure-op objects [outside-id outside-2-id]))))))) diff --git a/frontend/src/app/main/data/workspace/groups.cljs b/frontend/src/app/main/data/workspace/groups.cljs index 46a8737774..53175a61f2 100644 --- a/frontend/src/app/main/data/workspace/groups.cljs +++ b/frontend/src/app/main/data/workspace/groups.cljs @@ -30,6 +30,14 @@ reverse (map #(get objects %)))) +(defn- shapes-for-group-or-mask + "Shapes allowed for Group and Mask after the shared variant gates. + + `ids-for-structure-op` drops mains before `clean-loops` so a selected + variant main does not swallow nested children." + [objects ids] + (shapes-for-grouping objects (ctn/ids-for-structure-op objects ids))) + (defn- get-empty-groups-after-group-creation "An auxiliary function that finds and returns a set of ids that corresponds to groups that should be deleted after a group creation. @@ -188,18 +196,11 @@ (ptk/reify ::group-shapes ptk/WatchEvent (watch [it state _] - (let [id (d/nilv id (uuid/next)) - page-id (:current-page-id state) - objects (dsh/lookup-page-objects state page-id) - - shapes - (->> ids - (cfh/clean-loops objects) - (remove #(ctn/has-any-copy-parent? objects (get objects %))) - (shapes-for-grouping objects) - (remove ctk/is-variant?)) - - parents (into #{} (map :parent-id) shapes)] + (let [id (d/nilv id (uuid/next)) + page-id (:current-page-id state) + objects (dsh/lookup-page-objects state page-id) + shapes (shapes-for-group-or-mask objects ids) + parents (into #{} (map :parent-id) shapes)] (when-not (empty? shapes) (let [[group changes] (prepare-create-group (pcb/empty-changes it) id objects page-id shapes "Group" false)] @@ -213,7 +214,9 @@ (ptk/reify ::group-selected ptk/WatchEvent (watch [_ state _] - (let [selected (dsh/lookup-selected state)] + ;; Raw ids: lookup-selected already clean-loops, which would drop + ;; nested children of a selected variant main before we can. + (let [selected (dsh/get-selected-ids state)] (rx/of (group-shapes nil selected :change-selection? true)))))) (defn ungroup-shapes @@ -287,11 +290,9 @@ (watch [it state _] (let [page-id (:current-page-id state) objects (dsh/lookup-page-objects state page-id) - selected (->> (or ids (dsh/lookup-selected state)) - (cfh/clean-loops objects) - (remove #(ctn/has-any-copy-parent? objects (get objects %)))) - shapes (->> (shapes-for-grouping objects selected) - (remove ctk/is-variant?)) + shapes (shapes-for-group-or-mask + objects + (or ids (dsh/get-selected-ids state))) first-shape (first shapes)] (when-not (empty? shapes) (let [;; If the selected shape is a group, we can use it. If not, diff --git a/frontend/src/app/main/data/workspace/shape_layout.cljs b/frontend/src/app/main/data/workspace/shape_layout.cljs index df614dfe7a..784036857f 100644 --- a/frontend/src/app/main/data/workspace/shape_layout.cljs +++ b/frontend/src/app/main/data/workspace/shape_layout.cljs @@ -17,6 +17,7 @@ [app.common.logic.libraries :as cll] [app.common.types.color :as clr] [app.common.types.component :as ctc] + [app.common.types.container :as ctn] [app.common.types.modifiers :as ctm] [app.common.types.shape.layout :as ctl] [app.common.uuid :as uuid] @@ -216,13 +217,15 @@ has-component? (some true? (map ctc/instance-root? selected-shapes)) is-component? (and single? has-component?) has-variant? (some ctc/is-variant? selected-shapes) + has-container? (some ctc/is-variant-container? selected-shapes) + crosses-set? (ctn/crosses-variant-set-boundary? objects selected-shapes) has-layout? (and single? (ctl/any-layout? (first selected-shapes))) new-shape-id (uuid/next) undo-id (js/Symbol)] - (if has-variant? + (if (or has-variant? has-container? crosses-set?) (rx/empty) (rx/concat (rx/of (dwu/start-undo-transaction undo-id)) @@ -294,12 +297,17 @@ undo-id (js/Symbol)] - (rx/of - (dwu/start-undo-transaction undo-id) - (if (and single? is-frame? (not has-layout?)) - (create-layout-from-id (first selected) type :from-frame? true) - (create-layout-from-selection type)) - (dwu/commit-undo-transaction undo-id)))))) + ;; Refuse before undo: container or cross-boundary. Leave single + ;; mains on the from-id branch (selection refuses has-variant?). + (if (or (some ctc/is-variant-container? selected-shapes) + (ctn/crosses-variant-set-boundary? objects selected-shapes)) + (rx/empty) + (rx/of + (dwu/start-undo-transaction undo-id) + (if (and single? is-frame? (not has-layout?)) + (create-layout-from-id (first selected) type :from-frame? true) + (create-layout-from-selection type)) + (dwu/commit-undo-transaction undo-id))))))) (defn toggle-layout [type] diff --git a/frontend/src/app/main/data/workspace/shapes.cljs b/frontend/src/app/main/data/workspace/shapes.cljs index 6ddf66ddc8..491f7a2c2d 100644 --- a/frontend/src/app/main/data/workspace/shapes.cljs +++ b/frontend/src/app/main/data/workspace/shapes.cljs @@ -476,16 +476,14 @@ (ptk/reify ::create-artboard-from-selection ptk/WatchEvent (watch [_ state _] - (let [page-id (:current-page-id state) - objects (dsh/lookup-page-objects state page-id) - selected (->> (dsh/lookup-selected state) - (cfh/clean-loops objects) - (remove #(ctn/has-any-copy-parent? objects (get objects %))) - (remove #(->> % - (get objects) - (ctc/is-variant?))))] + (let [page-id (:current-page-id state) + objects (dsh/lookup-page-objects state page-id) + selected (vec (ctn/ids-for-structure-op + objects + (dsh/get-selected-ids state)))] - (rx/of (create-artboard-from-shapes selected id parent-id index name delta))))))) + (when (seq selected) + (rx/of (create-artboard-from-shapes selected id parent-id index name delta)))))))) ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; Shape Flags diff --git a/frontend/src/app/main/ui/workspace/context_menu.cljs b/frontend/src/app/main/ui/workspace/context_menu.cljs index 7cb59b75e5..6a11dfd988 100644 --- a/frontend/src/app/main/ui/workspace/context_menu.cljs +++ b/frontend/src/app/main/ui/workspace/context_menu.cljs @@ -398,6 +398,8 @@ objects (deref refs/workspace-page-objects) any-in-copy? (some #(ctn/has-any-copy-parent? objects %) shapes) any-is-variant? (some ctk/is-variant? shapes) + any-is-container? (some ctk/is-variant-container? shapes) + crosses-set? (ctn/crosses-variant-set-boundary? objects shapes) ;; components can't be ungrouped has-frame? (->> shapes (d/seek #(and (cfh/frame-shape? %) (not (ctk/instance-head? %)) (not (ctk/is-variant-container? %))))) @@ -416,7 +418,7 @@ #(st/emit! (dwsh/create-artboard-from-selection))] [:* - (when (not (or any-in-copy? any-is-variant?)) + (when (not (or any-in-copy? any-is-variant? any-is-container? crosses-set?)) [:* (when (or has-bool? has-group? has-mask? has-frame?) [:> menu-entry* {:title (tr "workspace.shape.menu.ungroup") @@ -621,6 +623,10 @@ (and single? (every? ctl/grid-layout? shapes)) any-is-variant? (some ctk/is-variant? shapes) + ;; shape-context-menu* already omits this menu for container-only; + ;; gate here too so a future caller cannot show Add flex/grid. + any-is-container? (some ctk/is-variant-container? shapes) + crosses-set? (ctn/crosses-variant-set-boundary? objects shapes) on-add-layout (mf/use-fn @@ -653,7 +659,9 @@ :shortcut-key :toggle-layout-grid :on-click on-remove-layout}])] - (when (or single? (not any-is-variant?)) + (when (and (not crosses-set?) + (not any-is-container?) + (or single? (not any-is-variant?))) [:div [:> menu-separator* {}] [:> menu-entry* {:title (tr "workspace.shape.menu.add-flex") diff --git a/frontend/test/frontend_tests/logic/groups_test.cljs b/frontend/test/frontend_tests/logic/groups_test.cljs index 709285ac74..33590b3bc0 100644 --- a/frontend/test/frontend_tests/logic/groups_test.cljs +++ b/frontend/test/frontend_tests/logic/groups_test.cljs @@ -6,18 +6,18 @@ (ns frontend-tests.logic.groups-test (:require [app.common.data :as d] - [app.common.test-helpers.components :as cthc] - [app.common.test-helpers.compositions :as ctho] [app.common.test-helpers.files :as cthf] [app.common.test-helpers.ids-map :as cthi] [app.common.test-helpers.shapes :as cths] [app.common.test-helpers.variants :as thv] [app.common.types.component :as ctk] - [app.common.uuid :as uuid] + [app.common.types.container :as ctn] + [app.common.types.shape.layout :as ctl] [app.main.data.changes :as dch] - [app.main.data.workspace :as dw] [app.main.data.workspace.groups :as dwgr] [app.main.data.workspace.selection :as dws] + [app.main.data.workspace.shape-layout :as dwsl] + [app.main.data.workspace.shapes :as dwsh] [beicon.v2.core :as rx] [cljs.test :as t :include-macros true] [frontend-tests.helpers.pages :as thp] @@ -41,6 +41,24 @@ [] (cths/add-sample-shape (variant-file) :rect)) +(defn- variant-file-with-nested-and-outside + [] + (-> (variant-file) + (cths/add-sample-shape :nested :parent-label :root1) + (cths/add-sample-shape :outside))) + +(defn- variant-file-with-two-nested + [] + (-> (variant-file) + (cths/add-sample-shape :nested-a :parent-label :root1) + (cths/add-sample-shape :nested-b :parent-label :root1))) + +(defn- variant-file-with-nested-in-each-main + [] + (-> (variant-file) + (cths/add-sample-shape :nested-a :parent-label :root1) + (cths/add-sample-shape :nested-b :parent-label :root2))) + (defn- masked-groups [file] (->> (cthf/current-page file) @@ -103,29 +121,46 @@ ;; Group has been created and is selected (t/is (= (get-in new-state [:workspace-local :selected]) #{group-id})))))))) -(t/deftest mask-group-leaves-variant-main-in-the-set +(t/deftest mask-group-of-main-and-outside-does-not-commit (t/async done - (let [file (variant-file-with-rect) - rect (cths/get-shape file :rect) - main (cths/get-shape file :root1) - comp (cthc/get-component file :comp1)] + (let [file (variant-file-with-rect) + rect (cths/get-shape file :rect) + main (cths/get-shape file :root1) + commits (atom [])] (run file [(dwgr/mask-group [(cthi/id :root1) (cthi/id :rect)])] done (fn [state] - (let [file' (ths/get-file-from-state state) - main' (cths/get-shape file' :root1) - rect' (cths/get-shape file' :rect) - group (first (masked-groups file')) - comp' (cthc/get-component file' :comp1)] - (t/is (= (:parent-id main) (:parent-id main'))) - (t/is (= (:variant-id main) (:variant-id main'))) - (t/is (= (:variant-name main) (:variant-name main'))) - (t/is (= (:variant-id comp) (:variant-id comp'))) - (t/is (= (:id group) (:parent-id rect'))) - (t/is (= (:parent-id rect) (:parent-id group))) - (t/is (= (:selrect rect) (:selrect group))))))))) + (let [file' (ths/get-file-from-state state)] + (t/is (= 0 (count @commits))) + (t/is (empty? (masked-groups file'))) + (t/is (= (:parent-id main) (:parent-id (cths/get-shape file' :root1)))) + (t/is (= (:parent-id rect) (:parent-id (cths/get-shape file' :rect)))))) + :commits commits)))) + +(t/deftest mask-group-of-nested-and-outside-does-not-commit + (t/async + done + (let [file (variant-file-with-nested-and-outside) + nested (cths/get-shape file :nested) + outside (cths/get-shape file :outside) + commits (atom [])] + (t/is (ctn/crosses-variant-set-boundary? + (:objects (cthf/current-page file)) + [nested outside])) + (run file + [(dwgr/mask-group [(cthi/id :nested) (cthi/id :outside)])] + done + (fn [state] + (let [file' (ths/get-file-from-state state)] + (t/is (= 0 (count @commits))) + (t/is (empty? (masked-groups file'))) + (t/is (= (:parent-id nested) + (:parent-id (cths/get-shape file' :nested)))) + (t/is (= (:parent-id outside) + (:parent-id (cths/get-shape file' :outside)))))) + :commits commits)))) (t/deftest mask-group-of-only-variant-mains-does-not-commit (t/async @@ -178,24 +213,480 @@ (t/is (= [group-id] (map :id (groups file')))) (t/is (= group-id (:parent-id (cths/get-shape file' :child)))))))))) -(t/deftest group-shapes-leaves-variant-main-in-the-set +(t/deftest group-shapes-of-main-and-outside-does-not-commit (t/async done - (let [file (variant-file-with-rect) - rect (cths/get-shape file :rect)] + (let [file (variant-file-with-rect) + rect (cths/get-shape file :rect) + main (cths/get-shape file :root1) + commits (atom [])] (run file [(dwgr/group-shapes nil [(cthi/id :root1) (cthi/id :rect)])] done (fn [state] - (let [file' (ths/get-file-from-state state) - board (cths/get-shape file' :variant) - main (cths/get-shape file' :root1) - rect' (cths/get-shape file' :rect) - group (first (groups file'))] - (t/is (= (:id board) (:parent-id main))) + (let [file' (ths/get-file-from-state state)] + (t/is (= 0 (count @commits))) + (t/is (empty? (groups file'))) + (t/is (= (:parent-id main) (:parent-id (cths/get-shape file' :root1)))) + (t/is (= (:parent-id rect) (:parent-id (cths/get-shape file' :rect)))))) + :commits commits)))) + +(t/deftest group-shapes-of-nested-and-outside-does-not-commit + (t/async + done + (let [file (variant-file-with-nested-and-outside) + nested (cths/get-shape file :nested) + outside (cths/get-shape file :outside) + commits (atom [])] + (run file + [(dwgr/group-shapes nil [(cthi/id :nested) (cthi/id :outside)])] + done + (fn [state] + (let [file' (ths/get-file-from-state state)] + (t/is (= 0 (count @commits))) + (t/is (empty? (groups file'))) + (t/is (= (:parent-id nested) + (:parent-id (cths/get-shape file' :nested)))) + (t/is (= (:parent-id outside) + (:parent-id (cths/get-shape file' :outside)))))) + :commits commits)))) + +(t/deftest group-shapes-of-two-nested-children-creates-a-group + (t/async + done + (let [file (variant-file-with-two-nested) + main (cths/get-shape file :root1)] + (run file + [(dwgr/group-shapes nil [(cthi/id :nested-a) (cthi/id :nested-b)])] + done + (fn [state] + (let [file' (ths/get-file-from-state state) + group (first (groups file'))] (t/is (some? group)) - (t/is (= (:id group) (:parent-id rect'))) - (t/is (= (:parent-id rect) (:parent-id group))))))))) + (t/is (= (:id main) (:parent-id group))) + (t/is (= (:id group) (:parent-id (cths/get-shape file' :nested-a)))) + (t/is (= (:id group) (:parent-id (cths/get-shape file' :nested-b)))))))))) + +(t/deftest group-shapes-of-main-and-nested-keeps-main-groups-nested + (t/async + done + (let [file (-> (variant-file) + (cths/add-sample-shape :nested :parent-label :root1)) + main (cths/get-shape file :root1) + nested (cths/get-shape file :nested) + commits (atom [])] + (run file + [(dwgr/group-shapes nil [(cthi/id :root1) (cthi/id :nested)])] + done + (fn [state] + (let [file' (ths/get-file-from-state state) + main' (cths/get-shape file' :root1) + nested' (cths/get-shape file' :nested) + group (first (groups file'))] + (t/is (pos? (count @commits))) + (t/is (some? group)) + (t/is (= (:parent-id main) (:parent-id main'))) + (t/is (= (:variant-id main) (:variant-id main'))) + (t/is (= (:id main') (:parent-id group))) + (t/is (= (:id group) (:parent-id nested'))) + (t/is (= (:parent-id nested) (:id main))))) + :commits commits)))) + +(t/deftest mask-group-of-main-and-nested-keeps-main-masks-nested + (t/async + done + (let [file (-> (variant-file) + (cths/add-sample-shape :nested :parent-label :root1)) + main (cths/get-shape file :root1) + nested (cths/get-shape file :nested) + commits (atom [])] + (run file + [(dwgr/mask-group [(cthi/id :root1) (cthi/id :nested)])] + done + (fn [state] + (let [file' (ths/get-file-from-state state) + main' (cths/get-shape file' :root1) + nested' (cths/get-shape file' :nested) + group (first (masked-groups file'))] + (t/is (pos? (count @commits))) + (t/is (some? group)) + (t/is (:masked-group group)) + (t/is (= (:parent-id main) (:parent-id main'))) + (t/is (= (:variant-id main) (:variant-id main'))) + (t/is (= (:id main') (:parent-id group))) + (t/is (= (:id group) (:parent-id nested'))) + (t/is (= (:parent-id nested) (:id main))))) + :commits commits)))) + +(t/deftest group-shapes-of-nested-in-different-variants-does-not-commit + (t/async + done + (let [file (variant-file-with-nested-in-each-main) + nested-a (cths/get-shape file :nested-a) + nested-b (cths/get-shape file :nested-b) + commits (atom [])] + (t/is (ctn/crosses-variant-set-boundary? + (:objects (cthf/current-page file)) + [nested-a nested-b])) + (run file + [(dwgr/group-shapes nil [(cthi/id :nested-a) (cthi/id :nested-b)])] + done + (fn [state] + (let [file' (ths/get-file-from-state state)] + (t/is (= 0 (count @commits))) + (t/is (empty? (groups file'))) + (t/is (= (:parent-id nested-a) + (:parent-id (cths/get-shape file' :nested-a)))) + (t/is (= (:parent-id nested-b) + (:parent-id (cths/get-shape file' :nested-b)))))) + :commits commits)))) + +(t/deftest mask-group-of-nested-in-different-variants-does-not-commit + (t/async + done + (let [file (variant-file-with-nested-in-each-main) + nested-a (cths/get-shape file :nested-a) + nested-b (cths/get-shape file :nested-b) + commits (atom [])] + (run file + [(dwgr/mask-group [(cthi/id :nested-a) (cthi/id :nested-b)])] + done + (fn [state] + (let [file' (ths/get-file-from-state state)] + (t/is (= 0 (count @commits))) + (t/is (empty? (masked-groups file'))) + (t/is (= (:parent-id nested-a) + (:parent-id (cths/get-shape file' :nested-a)))) + (t/is (= (:parent-id nested-b) + (:parent-id (cths/get-shape file' :nested-b)))))) + :commits commits)))) + +(t/deftest create-artboard-of-nested-and-outside-does-not-commit + (t/async + done + (let [file (variant-file-with-nested-and-outside) + nested (cths/get-shape file :nested) + outside (cths/get-shape file :outside) + commits (atom [])] + (run file + [(dws/select-shapes (d/ordered-set (cthi/id :nested) (cthi/id :outside))) + (dwsh/create-artboard-from-selection)] + done + (fn [state] + (let [file' (ths/get-file-from-state state)] + (t/is (= 0 (count @commits))) + (t/is (= (:parent-id nested) + (:parent-id (cths/get-shape file' :nested)))) + (t/is (= (:parent-id outside) + (:parent-id (cths/get-shape file' :outside)))))) + :commits commits)))) + +(t/deftest create-artboard-of-main-and-outside-does-not-commit + (t/async + done + (let [file (variant-file-with-rect) + rect (cths/get-shape file :rect) + main (cths/get-shape file :root1) + commits (atom [])] + (run file + [(dws/select-shapes (d/ordered-set (cthi/id :root1) (cthi/id :rect))) + (dwsh/create-artboard-from-selection)] + done + (fn [state] + (let [file' (ths/get-file-from-state state)] + (t/is (= 0 (count @commits))) + (t/is (= (:parent-id main) (:parent-id (cths/get-shape file' :root1)))) + (t/is (= (:parent-id rect) (:parent-id (cths/get-shape file' :rect)))))) + :commits commits)))) + +(t/deftest create-artboard-of-two-nested-children-commits + (t/async + done + (let [file (variant-file-with-two-nested) + main (cths/get-shape file :root1) + commits (atom [])] + (run file + [(dws/select-shapes (d/ordered-set (cthi/id :nested-a) (cthi/id :nested-b))) + (dwsh/create-artboard-from-selection)] + done + (fn [state] + (let [file' (ths/get-file-from-state state) + nested-a' (cths/get-shape file' :nested-a) + nested-b' (cths/get-shape file' :nested-b) + board (get (:objects (cthf/current-page file')) + (:parent-id nested-a'))] + (t/is (pos? (count @commits))) + (t/is (= :frame (:type board))) + (t/is (not (ctk/is-variant-container? board))) + (t/is (= (:id main) (:parent-id board))) + (t/is (= (:id board) (:parent-id nested-a'))) + (t/is (= (:id board) (:parent-id nested-b'))))) + :commits commits)))) + +(t/deftest create-artboard-of-main-and-nested-keeps-main-boards-nested + (t/async + done + (let [file (-> (variant-file) + (cths/add-sample-shape :nested :parent-label :root1)) + main (cths/get-shape file :root1) + nested (cths/get-shape file :nested) + commits (atom [])] + (run file + [(dws/select-shapes (d/ordered-set (cthi/id :root1) (cthi/id :nested))) + (dwsh/create-artboard-from-selection)] + done + (fn [state] + (let [file' (ths/get-file-from-state state) + main' (cths/get-shape file' :root1) + nested' (cths/get-shape file' :nested) + board (get (:objects (cthf/current-page file')) + (:parent-id nested'))] + (t/is (pos? (count @commits))) + (t/is (= :frame (:type board))) + (t/is (not (ctk/is-variant-container? board))) + (t/is (= (:parent-id main) (:parent-id main'))) + (t/is (= (:variant-id main) (:variant-id main'))) + (t/is (= (:id main') (:parent-id board))) + (t/is (= (:id board) (:parent-id nested'))) + (t/is (= (:parent-id nested) (:id main))))) + :commits commits)))) + +(t/deftest group-selected-of-main-and-nested-keeps-main-groups-nested + (t/async + done + (let [file (-> (variant-file) + (cths/add-sample-shape :nested :parent-label :root1)) + main (cths/get-shape file :root1) + nested (cths/get-shape file :nested) + commits (atom [])] + (run file + [(dws/select-shapes (d/ordered-set (cthi/id :root1) (cthi/id :nested))) + (dwgr/group-selected)] + done + (fn [state] + (let [file' (ths/get-file-from-state state) + main' (cths/get-shape file' :root1) + nested' (cths/get-shape file' :nested) + group (first (groups file'))] + (t/is (pos? (count @commits))) + (t/is (some? group)) + (t/is (= (:parent-id main) (:parent-id main'))) + (t/is (= (:variant-id main) (:variant-id main'))) + (t/is (= (:id main') (:parent-id group))) + (t/is (= (:id group) (:parent-id nested'))) + (t/is (= (:parent-id nested) (:id main))))) + :commits commits)))) + +(t/deftest mask-group-from-selection-of-main-and-nested-keeps-main-masks-nested + (t/async + done + (let [file (-> (variant-file) + (cths/add-sample-shape :nested :parent-label :root1)) + main (cths/get-shape file :root1) + nested (cths/get-shape file :nested) + commits (atom [])] + (run file + [(dws/select-shapes (d/ordered-set (cthi/id :root1) (cthi/id :nested))) + (dwgr/mask-group)] + done + (fn [state] + (let [file' (ths/get-file-from-state state) + main' (cths/get-shape file' :root1) + nested' (cths/get-shape file' :nested) + group (first (masked-groups file'))] + (t/is (pos? (count @commits))) + (t/is (some? group)) + (t/is (:masked-group group)) + (t/is (= (:parent-id main) (:parent-id main'))) + (t/is (= (:variant-id main) (:variant-id main'))) + (t/is (= (:id main') (:parent-id group))) + (t/is (= (:id group) (:parent-id nested'))) + (t/is (= (:parent-id nested) (:id main))))) + :commits commits)))) + +(t/deftest create-layout-of-nested-and-outside-does-not-commit + (t/async + done + (let [file (variant-file-with-nested-and-outside) + nested (cths/get-shape file :nested) + outside (cths/get-shape file :outside) + commits (atom [])] + (run file + [(dws/select-shapes (d/ordered-set (cthi/id :nested) (cthi/id :outside))) + (dwsl/create-layout-from-selection :flex)] + done + (fn [state] + (let [file' (ths/get-file-from-state state)] + (t/is (= 0 (count @commits))) + (t/is (= (:parent-id nested) + (:parent-id (cths/get-shape file' :nested)))) + (t/is (= (:parent-id outside) + (:parent-id (cths/get-shape file' :outside)))))) + :commits commits)))) + +(t/deftest create-layout-of-nested-in-different-variants-does-not-commit + (t/async + done + (let [file (variant-file-with-nested-in-each-main) + nested-a (cths/get-shape file :nested-a) + nested-b (cths/get-shape file :nested-b) + commits (atom [])] + (run file + [(dws/select-shapes (d/ordered-set (cthi/id :nested-a) (cthi/id :nested-b))) + (dwsl/create-layout :flex)] + done + (fn [state] + (let [file' (ths/get-file-from-state state)] + (t/is (= 0 (count @commits))) + (t/is (= (:parent-id nested-a) + (:parent-id (cths/get-shape file' :nested-a)))) + (t/is (= (:parent-id nested-b) + (:parent-id (cths/get-shape file' :nested-b)))))) + :commits commits)))) + +(t/deftest group-shapes-of-variant-container-only-does-not-commit + (t/async + done + (let [file (variant-file) + board (cths/get-shape file :variant) + commits (atom [])] + (run file + [(dwgr/group-shapes nil [(cthi/id :variant)])] + done + (fn [state] + (let [file' (ths/get-file-from-state state)] + (t/is (= 0 (count @commits))) + (t/is (empty? (groups file'))) + (t/is (= (:parent-id board) + (:parent-id (cths/get-shape file' :variant)))) + (t/is (ctk/is-variant-container? + (cths/get-shape file' :variant))))) + :commits commits)))) + +(t/deftest mask-group-of-variant-container-only-does-not-commit + (t/async + done + (let [file (variant-file) + board (cths/get-shape file :variant) + commits (atom [])] + (run file + [(dwgr/mask-group [(cthi/id :variant)])] + done + (fn [state] + (let [file' (ths/get-file-from-state state)] + (t/is (= 0 (count @commits))) + (t/is (empty? (masked-groups file'))) + (t/is (= (:parent-id board) + (:parent-id (cths/get-shape file' :variant)))) + (t/is (ctk/is-variant-container? + (cths/get-shape file' :variant))))) + :commits commits)))) + +(t/deftest create-artboard-of-variant-container-only-does-not-commit + (t/async + done + (let [file (variant-file) + board (cths/get-shape file :variant) + commits (atom [])] + (run file + [(dws/select-shapes (d/ordered-set (cthi/id :variant))) + (dwsh/create-artboard-from-selection)] + done + (fn [state] + (let [file' (ths/get-file-from-state state)] + (t/is (= 0 (count @commits))) + (t/is (= (:parent-id board) + (:parent-id (cths/get-shape file' :variant)))) + (t/is (ctk/is-variant-container? + (cths/get-shape file' :variant))))) + :commits commits)))) + +(t/deftest mask-group-of-two-nested-children-creates-a-mask + (t/async + done + (let [file (variant-file-with-two-nested) + main (cths/get-shape file :root1)] + (run file + [(dwgr/mask-group [(cthi/id :nested-a) (cthi/id :nested-b)])] + done + (fn [state] + (let [file' (ths/get-file-from-state state) + group (first (masked-groups file'))] + (t/is (= 1 (count (masked-groups file')))) + (t/is (:masked-group group)) + (t/is (= (:id main) (:parent-id group))) + (t/is (= (:id group) (:parent-id (cths/get-shape file' :nested-a)))) + (t/is (= (:id group) (:parent-id (cths/get-shape file' :nested-b)))))))))) + +(t/deftest create-layout-of-variant-container-only-does-not-commit + (t/async + done + (let [file (variant-file) + board (cths/get-shape file :variant) + commits (atom [])] + (t/is (not (ctl/any-layout? board))) + (run file + [(dws/select-shapes (d/ordered-set (cthi/id :variant))) + ;; Pin the create-layout entry (from-id hole), not only + ;; create-layout-from-selection which already no-ops. + (dwsl/create-layout :flex)] + done + (fn [state] + (let [file' (ths/get-file-from-state state) + board' (cths/get-shape file' :variant)] + (t/is (= 0 (count @commits))) + (t/is (not (ctl/any-layout? board'))) + (t/is (ctk/is-variant-container? board')) + (t/is (= (:parent-id board) (:parent-id board'))))) + :commits commits)))) + +(t/deftest create-layout-of-variant-main-commits + (t/async + done + (let [file (variant-file) + main (cths/get-shape file :root1) + board (cths/get-shape file :variant) + commits (atom [])] + (t/is (not (ctl/any-layout? main))) + (run file + [(dws/select-shapes (d/ordered-set (cthi/id :root1))) + ;; Locks create-layout → create-layout-from-id for a single main. + (dwsl/create-layout :flex)] + done + (fn [state] + (let [file' (ths/get-file-from-state state) + main' (cths/get-shape file' :root1)] + (t/is (pos? (count @commits))) + (t/is (ctl/flex-layout? main')) + (t/is (ctk/is-variant? main')) + (t/is (= (:variant-id main) (:variant-id main'))) + (t/is (= (:id board) (:parent-id main'))))) + :commits commits)))) + +(t/deftest create-layout-of-one-nested-child-commits + (t/async + done + (let [file (-> (variant-file) + (cths/add-sample-shape :nested :parent-label :root1)) + nested (cths/get-shape file :nested) + main (cths/get-shape file :root1) + commits (atom [])] + (run file + [(dws/select-shapes (d/ordered-set (cthi/id :nested))) + ;; Same Shift+A / menu entry as the container refuse and main allow. + (dwsl/create-layout :flex)] + done + (fn [state] + (let [file' (ths/get-file-from-state state) + nested' (cths/get-shape file' :nested) + ;; Layout wraps the child in a new board under the main. + parent' (get (:objects (cthf/current-page file')) + (:parent-id nested'))] + (t/is (pos? (count @commits))) + (t/is (not= (:parent-id nested) (:parent-id nested'))) + (t/is (= (:id main) (:parent-id parent'))) + (t/is (ctl/any-layout? parent')))) + :commits commits)))) (t/deftest ungroup-shapes-leaves-a-variant-container (t/async From d47f366c709629c061190fcb0b48975a7693b337 Mon Sep 17 00:00:00 2001 From: Pablo Alba <pablo.alba@kaleidos.net> Date: Thu, 29 Oct 2026 22:46:49 +0100 Subject: [PATCH 08/13] :bug: Fix Creating or combining a variant set saves half-finished steps --- .../src/app/main/data/workspace/variants.cljs | 224 +++++++------- .../data/workspace_variants_test.cljs | 274 +++++++++++++++++- 2 files changed, 392 insertions(+), 106 deletions(-) diff --git a/frontend/src/app/main/data/workspace/variants.cljs b/frontend/src/app/main/data/workspace/variants.cljs index 149a5edeff..f5f82a45b3 100644 --- a/frontend/src/app/main/data/workspace/variants.cljs +++ b/frontend/src/app/main/data/workspace/variants.cljs @@ -304,22 +304,36 @@ (dch/commit-changes changes) (dwu/commit-undo-transaction undo-id))))))))) -(defn- set-variant-id - "Sets the variant-id on a component" - [component-id variant-id] - (ptk/reify ::set-variant-id +(defn generate-define-variant-container + "One commit that turns an existing frame into a valid variant set: + container flag and name, the main's variant-id, the component's + variant-id (refreshing library data), then the first property." + [changes variant-id main-id component-id name] + (-> changes + (pcb/update-shapes [variant-id] + #(assoc % :is-variant-container true :name name)) + (pcb/update-shapes [main-id] + #(assoc % :name name :variant-id variant-id)) + (pcb/update-component component-id + #(assoc % :variant-id variant-id) + {:apply-changes-local-library? true}) + (clvp/generate-add-new-property variant-id {:fill-values? true}))) + +(defn- define-variant-container + "Read page objects after the artboard exists, then commit the defining + variant fields in one change." + [variant-id main-id component-id name] + (ptk/reify ::define-variant-container ptk/WatchEvent (watch [it state _] (let [page-id (:current-page-id state) data (dsh/lookup-file-data state) + objects (dsh/lookup-page-objects state page-id) changes (-> (pcb/empty-changes it page-id) + (pcb/with-objects objects) (pcb/with-library-data data) - (pcb/update-component component-id #(assoc % :variant-id variant-id))) - undo-id (js/Symbol)] - (rx/of - (dwu/start-undo-transaction undo-id) - (dch/commit-changes changes) - (dwu/commit-undo-transaction undo-id)))))) + (generate-define-variant-container variant-id main-id component-id name))] + (rx/of (dch/commit-changes changes)))))) (defn- focus-property [variant-id] @@ -397,6 +411,95 @@ (->> (rx/of (focus-property (:id variant-container))) (rx/delay 250)))))))))) +(defn- transform-in-variant-stream + "Event stream that turns `main-instance-id` into a variant set. + The defining fields share one commit (`define-variant-container`). + Combine concatenates this stream before relocating the other mains. + `page-id` is the page that holds the main; nil means the open page." + [state main-instance-id variant-id delta prefix add-wrapper? duplicate? flex? page-id] + (let [variant-id (or variant-id (uuid/next)) + variant-vec [variant-id] + file-id (:current-file-id state) + page-id (or page-id (:current-page-id state)) + objects (dsh/lookup-page-objects state file-id page-id) + main (get objects main-instance-id) + parent (get objects (:parent-id main)) + component-id (:component-id main) + name (if add-wrapper? + (str "Component/" (:name main)) + (:name main)) + ;; If there is a prefix, set is as first item of path + cpath (-> name + cpn/split-path + (cond-> + (seq prefix) + (->> (drop (count prefix)) + (cons (cpn/join-path prefix)) + vec))) + + name (first cpath) + num-props (max 1 (dec (count cpath))) + style-props (cond-> {:r1 20 + :r2 20 + :r3 20 + :r4 20 + :layout-item-absolute true} + flex? + (into {:layout-item-h-sizing :auto + :layout-item-v-sizing :auto + :layout-padding {:p1 30 :p2 30 :p3 30 :p4 30} + :layout-gap {:row-gap 0 :column-gap 20}})) + + stroke-props {:stroke-alignment :inner + :stroke-style :solid + :stroke-color "#bb97d8" ;; todo use color var? + :stroke-opacity 1 + :stroke-width 2} + + ;; Move the position of the variant container so the main shape doesn't + ;; change its position + delta (or delta + (if (ctsl/any-layout? parent) + (gpt/point 0 0) + (gpt/point -30 -30))) + undo-id (js/Symbol)] + + (rx/concat + (rx/of + (dwu/start-undo-transaction undo-id) + + (when (not= name (:name main)) + (dwl/rename-component component-id name)) + + ;; Create the frame while it is still a normal board + (dwsh/create-artboard-from-shapes [main-instance-id] variant-id nil nil nil delta flex?) + (cl/remove-all-fills variant-vec {:color clr/black :opacity 1}) + (when flex? (dwsl/create-layout-from-id variant-id :flex)) + (dwsh/update-shapes variant-vec #(merge % style-props)) + (cl/add-stroke variant-vec stroke-props)) + + (rx/of (define-variant-container variant-id main-instance-id component-id name)) + + ;; First property is in the defining commit + (when (> num-props 1) + (rx/from + (repeatedly (dec num-props) + #(add-new-property variant-id {:fill-values? true})))) + + ;; When the component has path, set the path items as properties values + (when (> (count cpath) 1) + (rx/from + (map + #(update-property-value component-id % (nth cpath (inc %))) + (range num-props)))) + + (rx/of + (when duplicate? (add-new-variant main-instance-id)) + (dwsh/update-shapes variant-vec #(dissoc % :layout-item-absolute)) + (dwu/commit-undo-transaction undo-id) + (when flex? + (ptk/data-event :layout/update {:ids [variant-id]})))))) + (defn transform-in-variant "Given the id of a main shape of a component, creates a variant structure for that component" @@ -406,98 +509,7 @@ (ptk/reify ::transform-in-variant ptk/WatchEvent (watch [_ state _] - (let [variant-id (or variant-id (uuid/next)) - variant-vec [variant-id] - file-id (:current-file-id state) - page-id (:current-page-id state) - objects (dsh/lookup-page-objects state file-id page-id) - main (get objects main-instance-id) - parent (get objects (:parent-id main)) - component-id (:component-id main) - name (if add-wrapper? - (str "Component/" (:name main)) - (:name main)) - ;; If there is a prefix, set is as first item of path - cpath (-> name - cpn/split-path - (cond-> - (seq prefix) - (->> (drop (count prefix)) - (cons (cpn/join-path prefix)) - vec))) - - name (first cpath) - num-props (max 1 (dec (count cpath))) - base-props {:is-variant-container true - :name name - :r1 20 - :r2 20 - :r3 20 - :r4 20 - :layout-item-absolute true} - flex-props {:layout-item-h-sizing :auto - :layout-item-v-sizing :auto - :layout-padding {:p1 30 :p2 30 :p3 30 :p4 30} - :layout-gap {:row-gap 0 :column-gap 20}} - cont-props (if flex? - (into base-props flex-props) - base-props) - main-props {:name name - :variant-id variant-id} - - stroke-props {:stroke-alignment :inner - :stroke-style :solid - :stroke-color "#bb97d8" ;; todo use color var? - :stroke-opacity 1 - :stroke-width 2} - - ;; Move the position of the variant container so the main shape doesn't - ;; change its position - delta (or delta - (if (ctsl/any-layout? parent) - (gpt/point 0 0) - (gpt/point -30 -30))) - undo-id (js/Symbol)] - - - ;;TODO Refactor all called methods in order to be able to - ;;generate changes instead of call the events - - - (rx/concat - (rx/of - (dwu/start-undo-transaction undo-id) - - (when (not= name (:name main)) - (dwl/rename-component component-id name)) - - ;; Create variant container - (dwsh/create-artboard-from-shapes [main-instance-id] variant-id nil nil nil delta flex?) - (cl/remove-all-fills variant-vec {:color clr/black :opacity 1}) - (when flex? (dwsl/create-layout-from-id variant-id :flex)) - (dwsh/update-shapes variant-vec #(merge % cont-props)) - (dwsh/update-shapes [main-instance-id] #(merge % main-props)) - (cl/add-stroke variant-vec stroke-props) - (set-variant-id component-id variant-id)) - - ;; Add the necessary number of new properties, with default values - (rx/from - (repeatedly num-props - #(add-new-property variant-id {:fill-values? true}))) - - ;; When the component has path, set the path items as properties values - (when (> (count cpath) 1) - (rx/from - (map - #(update-property-value component-id % (nth cpath (inc %))) - (range num-props)))) - - (rx/of - (when duplicate? (add-new-variant main-instance-id)) - (dwsh/update-shapes variant-vec #(dissoc % :layout-item-absolute)) - (dwu/commit-undo-transaction undo-id) - (when flex? - (ptk/data-event :layout/update {:ids [variant-id]}))))))))) + (transform-in-variant-stream state main-instance-id variant-id delta prefix add-wrapper? duplicate? flex? nil))))) (defn add-component-or-variant "Manage the shared shortcut, and do the pertinent action" @@ -696,9 +708,11 @@ (rx/of (dcm/go-to-workspace :page-id page-id)) (rx/empty)) - (rx/of (dwu/start-undo-transaction undo-id) - (transform-in-variant (first ids) variant-id delta prefix add-wrapper? false false) - (dwsh/relocate-shapes (into #{} (rest ids)) variant-id 0) + (rx/of (dwu/start-undo-transaction undo-id)) + + (transform-in-variant-stream state (first ids) variant-id delta prefix add-wrapper? false false current-page) + + (rx/of (dwsh/relocate-shapes (into #{} (rest ids)) variant-id 0) ;; relocate-shapes inserts the shapes in layer-tree ;; order, so restore the intended order afterwards (order-variant-children variant-id ids) diff --git a/frontend/test/frontend_tests/data/workspace_variants_test.cljs b/frontend/test/frontend_tests/data/workspace_variants_test.cljs index d96a7823b8..44e68d6745 100644 --- a/frontend/test/frontend_tests/data/workspace_variants_test.cljs +++ b/frontend/test/frontend_tests/data/workspace_variants_test.cljs @@ -6,16 +6,24 @@ (ns frontend-tests.data.workspace-variants-test (:require + [app.common.files.changes-builder :as pcb] + [app.common.files.variant :as cfv] [app.common.path-names :as cpn] [app.common.test-helpers.components :as cthc] + [app.common.test-helpers.compositions :as tho] [app.common.test-helpers.files :as cthf] [app.common.test-helpers.ids-map :as cthi] [app.common.test-helpers.shapes :as cths] [app.common.test-helpers.variants :as thv] + [app.common.types.component :as ctk] [app.common.types.variant :as ctv] + [app.common.uuid :as uuid] + [app.main.data.workspace :as dw] [app.main.data.workspace.variants :as dwv] + [beicon.v2.core :as rx] [cljs.test :as t :include-macros true] - [frontend-tests.helpers.state :as ths])) + [frontend-tests.helpers.state :as ths] + [potok.v2.core :as ptk])) (t/use-fixtures :each {:before cthi/reset-idmap!}) @@ -152,3 +160,267 @@ (t/is (= clean-name (:name main2))) (t/is (= clean-name (cpn/merge-path-item (:path comp1) (:name comp1)))) (t/is (= clean-name (cpn/merge-path-item (:path comp2) (:name comp2)))))))))) + +;; --------------------------------------------------------------------------- +;; Create-set / combine: every commit must be a valid file +;; --------------------------------------------------------------------------- + +(def ^:private apply-local? + (ptk/type? :app.main.data.changes/apply-changes-localy)) + +(defn- simple-main-file + [] + (-> (cthf/sample-file :file1) + (tho/add-simple-component :c01 :m01 :r01))) + +(defn- two-mains-file + [] + (-> (cthf/sample-file :file1) + (tho/add-simple-component :c01 :m01 :r01 + :root-params {:name "Alpha" :x 0 :y 0 :width 100 :height 80}) + (tho/add-simple-component :c02 :m02 :r02 + :root-params {:name "Beta" :x 150 :y 0 :width 100 :height 80}))) + +(defn- two-mains-on-page1-with-page2 + "Two mains on :page1; the open page is :page2 (Assets combine while elsewhere)." + [] + (-> (cthf/sample-file :file1 :page-label :page1) + (tho/add-simple-component :c01 :m01 :r01 + :root-params {:name "Alpha" :x 0 :y 0 :width 100 :height 80}) + (tho/add-simple-component :c02 :m02 :r02 + :root-params {:name "Beta" :x 150 :y 0 :width 100 :height 80}) + (cthf/add-sample-page :page2) + (cthf/switch-to-page :page2))) + +(defn- post-artboard-file + "A normal frame holding one main — the state after create-artboard-from-shapes + and before the container flag." + [] + (-> (cthf/sample-file :file1) + (tho/add-frame :v01 {:name "Set"}) + (tho/add-simple-component :c01 :m01 :r01 + :root-params {:parent-label :v01 :name "Set"}))) + +(defn- page-objects + [file] + (:objects (cthf/current-page file))) + +(defn- page-objects-on + [file page-label] + (:objects (cthf/get-page file page-label))) + +(defn- commit-error + [file] + (try + (cthf/validate-file! file) + nil + (catch :default cause + (or (ex-message cause) (str cause))))) + +(defn- watch-applied-commits + "Record the file after every local apply. Keep the subscription for the + whole run-store; assertions run in the completed callback." + [store snapshots] + (->> (ptk/input-stream store) + (rx/filter apply-local?) + (rx/subs! (fn [_] + (let [file (ths/get-file-from-state @store)] + (swap! snapshots conj {:file file + :error (commit-error file)})))))) + +(defn- after-commits-idle + "Stop once applied commits have been idle. Combine schedules an async + undo commit after the main stream." + [stream] + (->> stream + (rx/filter apply-local?) + (rx/debounce 100))) + +(defn- assert-valid-snapshots + [snapshots] + (doseq [{:keys [error]} snapshots] + (t/is (nil? error) error))) + +(defn- first-flagged + [snapshots container-id] + (first (filter #(ctk/is-variant-container? + (get (page-objects (:file %)) container-id)) + snapshots))) + +(t/deftest define-variant-container-commit-validates + (let [file (post-artboard-file) + page (cthf/current-page file) + variant-id (cthi/id :v01) + main-id (cthi/id :m01) + component (cthc/get-component file :c01) + component-id (:id component) + name (:name (cths/get-shape file :v01)) + changes (-> (pcb/empty-changes nil (:id page)) + (pcb/with-objects (:objects page)) + (pcb/with-library-data (:data file)) + (dwv/generate-define-variant-container + variant-id main-id component-id name)) + file' (cthf/apply-changes file changes) + container' (cths/get-shape file' :v01) + main' (cths/get-shape file' :m01) + comp' (cthc/get-component file' :c01)] + + (t/is (ctk/is-variant-container? container')) + (t/is (= [main-id] (:shapes container'))) + (t/is (= variant-id (:variant-id main'))) + (t/is (= variant-id (:variant-id comp'))) + (t/is (some? (:variant-id comp'))) + (t/is (= (ctv/properties-to-name (:variant-properties comp')) + (:variant-name main')))))) + +(t/deftest transform-in-variant-validates-after-every-commit + (t/async + done + (let [file (simple-main-file) + main-id (cthi/id :m01) + variant-id (uuid/next) + snapshots (atom []) + store (ths/setup-store file)] + (watch-applied-commits store snapshots) + (ths/run-store + store + done + [(dwv/transform-in-variant main-id variant-id nil [] false true true)] + (fn [state] + (let [snaps @snapshots + flagged (first-flagged snaps variant-id) + file' (ths/get-file-from-state state) + objects' (page-objects file') + container' (get objects' variant-id) + main' (get objects' main-id) + comp' (cthc/get-component file' :c01) + before-flag (take-while + #(not (ctk/is-variant-container? + (get (page-objects (:file %)) variant-id))) + snaps)] + (assert-valid-snapshots snaps) + (t/is (some? flagged) "a commit must set :is-variant-container") + (doseq [snap before-flag] + (t/is (not (ctk/is-variant-container? + (get (page-objects (:file snap)) variant-id))))) + (when flagged + (let [ffile (:file flagged) + fobjs (page-objects ffile) + fcont (get fobjs variant-id) + fmain (get fobjs main-id) + fcomp (cthc/get-component ffile :c01)] + (t/is (= [main-id] (:shapes fcont))) + (t/is (ctk/main-instance? fmain)) + (t/is (= variant-id (:variant-id fmain))) + (t/is (= variant-id (:variant-id fcomp))) + (t/is (= (ctv/properties-to-name (:variant-properties fcomp)) + (:variant-name fmain))))) + (t/is (ctk/is-variant-container? container')) + (t/is (= variant-id (:variant-id main'))) + (t/is (= variant-id (:variant-id comp'))))) + after-commits-idle)))) + +(t/deftest add-new-variant-on-existing-set-still-validates + (t/async + done + (let [file (-> (two-property-file-on-other-page) + (cthf/switch-to-page :page1)) + main-id (cthi/id :m01) + c01-before (cthc/get-component file :c01) + c02-before (cthc/get-component file :c02) + last-c01 (-> c01-before :variant-properties last) + last-c02 (-> c02-before :variant-properties last)] + (run file + [(dwv/add-new-variant main-id)] + done + (fn [state] + (let [file' (ths/get-file-from-state state) + container' (cths/get-shape file' :v01 :page-label :page1) + c01' (cthc/get-component file' :c01) + c02' (cthc/get-component file' :c02) + new-id (first (:shapes container')) + new-shape (cths/get-shape-by-id file' new-id :page-label :page1) + new-comp (cthc/get-component-by-id file' (:component-id new-shape))] + (t/is (= 3 (count (:shapes container')))) + (t/is (= last-c01 (-> c01' :variant-properties last))) + (t/is (= last-c02 (-> c02' :variant-properties last))) + (t/is (not= last-c01 (-> new-comp :variant-properties last))) + (t/is (= (ctv/properties-to-name (:variant-properties new-comp)) + (:variant-name new-shape))))))))) + +(t/deftest combine-as-variants-validates-after-every-commit + (t/async + done + (let [file (two-mains-file) + id1 (cthi/id :m01) + id2 (cthi/id :m02) + variant-id (uuid/next) + snapshots (atom []) + store (ths/setup-store file)] + (watch-applied-commits store snapshots) + (ths/run-store + store + done + [(dwv/combine-as-variants [id1 id2] {:variant-id variant-id :trigger "test"})] + (fn [state] + (let [snaps @snapshots + file' (ths/get-file-from-state state) + objects (page-objects file') + container (get objects variant-id) + m01' (get objects id1) + m02' (get objects id2) + c01' (cthc/get-component file' :c01) + c02' (cthc/get-component file' :c02) + names1 (mapv :name (:variant-properties c01')) + names2 (mapv :name (:variant-properties c02')) + ordered (mapv :main-instance-id + (cfv/find-variant-components (:data file') objects variant-id))] + (assert-valid-snapshots snaps) + (doseq [{:keys [file]} snaps] + (let [objs (page-objects file) + cont (get objs variant-id) + kids (set (:shapes cont))] + (when (contains? kids id2) + (t/is (ctk/is-variant-container? cont) + "other mains must not enter the frame before it is a variant container")))) + (t/is (ctk/is-variant-container? container)) + (t/is (= names1 names2)) + (t/is (= [id1 id2] ordered)) + (t/is (= variant-id (:variant-id m01'))) + (t/is (= variant-id (:variant-id m02'))) + (t/is (= variant-id (:parent-id m01'))) + (t/is (= variant-id (:parent-id m02'))))) + after-commits-idle)))) + +(t/deftest combine-as-variants-from-another-page-validates-after-every-commit + (t/async + done + (let [file (two-mains-on-page1-with-page2) + id1 (cthi/id :m01) + id2 (cthi/id :m02) + page1 (cthi/id :page1) + variant-id (uuid/next) + snapshots (atom []) + store (ths/setup-store file)] + (watch-applied-commits store snapshots) + (ths/run-store + store + done + [(dwv/combine-as-variants [id1 id2] {:page-id page1 :variant-id variant-id :trigger "test"}) + (dw/initialize-page (:id file) page1)] + (fn [state] + (let [snaps @snapshots + file' (ths/get-file-from-state state) + objects (page-objects-on file' :page1) + container (get objects variant-id) + m01' (get objects id1) + m02' (get objects id2) + c01' (cthc/get-component file' :c01) + c02' (cthc/get-component file' :c02)] + (assert-valid-snapshots snaps) + (t/is (ctk/is-variant-container? container)) + (t/is (= variant-id (:variant-id m01'))) + (t/is (= variant-id (:variant-id m02'))) + (t/is (= variant-id (:variant-id c01'))) + (t/is (= variant-id (:variant-id c02'))))) + after-commits-idle)))) From eeda9c1d18f46502c62553c0e8d5c094034e8d1c Mon Sep 17 00:00:00 2001 From: Pablo Alba <pablo.alba@kaleidos.net> Date: Fri, 30 Oct 2026 12:09:42 +0100 Subject: [PATCH 09/13] :bug: Fix renaming a variant set saves half-finished steps --- common/src/app/common/logic/variants.cljc | 23 ++++ .../common_tests/logic/variants_test.cljc | 79 ++++++++++++++ .../src/app/main/data/workspace/variants.cljs | 21 ++-- .../data/workspace_variant_rename_test.cljs | 100 ++++++++++++++++-- 4 files changed, 201 insertions(+), 22 deletions(-) diff --git a/common/src/app/common/logic/variants.cljc b/common/src/app/common/logic/variants.cljc index 79de230b5a..b9c2037788 100644 --- a/common/src/app/common/logic/variants.cljc +++ b/common/src/app/common/logic/variants.cljc @@ -9,12 +9,35 @@ [app.common.logic.libraries :as cll] [app.common.logic.shapes :as cls] [app.common.logic.variant-properties :as clvp] + [app.common.path-names :as cpn] [app.common.types.component :as ctk] [app.common.types.container :as ctn] [app.common.types.file :as ctf] [app.common.types.variant :as ctv] [app.common.uuid :as uuid])) +(defn generate-rename-variant + "Rename a variant set in one change vector. + + The builder already has the mains' page objects and library data. + The stored name is `clean-path` of `name`, or \"(empty)\" when that + path is blank. The board and every direct child take that name. + Each component's path and name come from `split-group-name`." + [changes variant-id name] + (let [objects (pcb/get-objects changes) + data (pcb/get-library-data changes) + clean-name (cpn/clean-path name) + stored-name (if (empty? clean-name) "(empty)" clean-name) + shape-ids (cons variant-id (:shapes (get objects variant-id))) + components (cfv/find-variant-components data objects variant-id)] + (reduce (fn [changes component] + (let [[path item-name] (cpn/split-group-name stored-name)] + (pcb/update-component changes + (:id component) + #(assoc % :path path :name item-name)))) + (pcb/update-shapes changes shape-ids #(assoc % :name stored-name)) + components))) + (defn generate-add-new-variant [changes shape variant-id new-component-id new-shape-id prop-num] (let [data (pcb/get-library-data changes) diff --git a/common/test/common_tests/logic/variants_test.cljc b/common/test/common_tests/logic/variants_test.cljc index d164c0e066..75b292598f 100644 --- a/common/test/common_tests/logic/variants_test.cljc +++ b/common/test/common_tests/logic/variants_test.cljc @@ -11,6 +11,8 @@ [app.common.logic.libraries :as cll] [app.common.logic.shapes :as cls] [app.common.logic.variant-properties :as clvp] + [app.common.logic.variants :as clv] + [app.common.path-names :as cpn] [app.common.test-helpers.components :as thc] [app.common.test-helpers.files :as thf] [app.common.test-helpers.ids-map :as thi] @@ -563,3 +565,80 @@ (t/is (= "crítica" (-> props' second :value))) (t/is (= (ctv/properties-to-name props') (:variant-name main'))) (t/is (= "active, crítica" (:variant-name main'))))) + +(defn- rename-variant-changes + [file name] + (let [page (thf/current-page file)] + (-> (pcb/empty-changes nil) + (pcb/with-page-id (:id page)) + (pcb/with-library-data (:data file)) + (pcb/with-objects (:objects page)) + (clv/generate-rename-variant (thi/id :v01) name)))) + +(defn- redo-ids + [changes change-type] + (into #{} + (comp (filter #(= change-type (:type %))) + (map :id)) + (:redo-changes changes))) + +(t/deftest test-rename-variant-one-vector + (let [file (-> (thf/sample-file :file1) + (thv/add-variant :v01 :c01 :m01 :c02 :m02)) + board (ths/get-shape file :v01) + main1 (ths/get-shape file :m01) + main2 (ths/get-shape file :m02) + comp1 (thc/get-component file :c01) + comp2 (thc/get-component file :c02) + changes (rename-variant-changes file "Group / Button") + file' (thf/apply-changes file changes) + board' (ths/get-shape file' :v01) + main1' (ths/get-shape file' :m01) + main2' (ths/get-shape file' :m02) + comp1' (thc/get-component file' :c01) + comp2' (thc/get-component file' :c02) + restored (thf/apply-undo-changes file' changes)] + + (t/is (= #{(thi/id :v01) (thi/id :m01) (thi/id :m02)} + (redo-ids changes :mod-obj))) + (t/is (= #{(thi/id :c01) (thi/id :c02)} + (redo-ids changes :mod-component))) + (t/is (= "Group / Button" (:name board') (:name main1') (:name main2'))) + (t/is (= "Group" (:path comp1') (:path comp2'))) + (t/is (= "Button" (:name comp1') (:name comp2'))) + (t/is (= "Group / Button" + (cpn/merge-path-item (:path comp1') (:name comp1')) + (cpn/merge-path-item (:path comp2') (:name comp2')))) + (t/is (= (:id board) (:variant-id comp1') (:variant-id comp2'))) + (t/is (= (:variant-name main1) (:variant-name main1'))) + (t/is (= (:variant-name main2) (:variant-name main2'))) + (t/is (= (:variant-properties comp1) (:variant-properties comp1'))) + (t/is (= (:variant-properties comp2) (:variant-properties comp2'))) + (t/is (= (:name board) (:name (ths/get-shape restored :v01)))) + (t/is (= (:name main1) (:name (ths/get-shape restored :m01)))) + (t/is (= (:name main2) (:name (ths/get-shape restored :m02)))) + (t/is (= (:path comp1) (:path (thc/get-component restored :c01)))) + (t/is (= (:name comp1) (:name (thc/get-component restored :c01)))) + (t/is (= (:path comp2) (:path (thc/get-component restored :c02)))) + (t/is (= (:name comp2) (:name (thc/get-component restored :c02)))))) + +(t/deftest test-rename-variant-slash-only-name + (let [file (-> (thf/sample-file :file1) + (thv/add-variant :v01 :c01 :m01 :c02 :m02)) + changes (rename-variant-changes file "///") + file' (thf/apply-changes file changes) + comp1' (thc/get-component file' :c01) + comp2' (thc/get-component file' :c02)] + + (t/is (= "(empty)" + (:name (ths/get-shape file' :v01)) + (:name (ths/get-shape file' :m01)) + (:name (ths/get-shape file' :m02)) + (:name comp1') + (:name comp2'))) + (t/is (= "" (:path comp1') (:path comp2'))) + (t/is (= "(empty)" + (cpn/merge-path-item (:path comp1') (:name comp1')) + (cpn/merge-path-item (:path comp2') (:name comp2')))) + (t/is (some? (:variant-id comp1'))) + (t/is (some? (:variant-id comp2'))))) diff --git a/frontend/src/app/main/data/workspace/variants.cljs b/frontend/src/app/main/data/workspace/variants.cljs index f5f82a45b3..18ac285055 100644 --- a/frontend/src/app/main/data/workspace/variants.cljs +++ b/frontend/src/app/main/data/workspace/variants.cljs @@ -579,20 +579,19 @@ (ptk/reify ::rename-variant ptk/WatchEvent - (watch [_ state _] + (watch [it state _] (let [data (dsh/lookup-file-data state)] (with-main-page data {:variant-id variant-id} (fn [page-id objects] - (let [variant-components (cfv/find-variant-components data objects variant-id) - clean-name (cpn/clean-path name) - undo-id (js/Symbol)] - (rx/concat - (rx/of (dwu/start-undo-transaction undo-id) - (dwsh/update-shapes [variant-id] #(assoc % :name clean-name) {:page-id page-id})) - (rx/from (map - #(dwl/rename-component-and-main-instance (:id %) clean-name) - variant-components)) - (rx/of (dwu/commit-undo-transaction undo-id)))))))))) + (let [changes (-> (pcb/empty-changes it page-id) + (pcb/with-library-data data) + (pcb/with-objects objects) + (clv/generate-rename-variant variant-id name)) + undo-id (js/Symbol)] + (rx/of + (dwu/start-undo-transaction undo-id) + (dch/commit-changes changes) + (dwu/commit-undo-transaction undo-id))))))))) (defn rename-comp-or-variant-and-main "If the component is in a variant, rename the variant. diff --git a/frontend/test/frontend_tests/data/workspace_variant_rename_test.cljs b/frontend/test/frontend_tests/data/workspace_variant_rename_test.cljs index 5e177b1b21..038d0ba112 100644 --- a/frontend/test/frontend_tests/data/workspace_variant_rename_test.cljs +++ b/frontend/test/frontend_tests/data/workspace_variant_rename_test.cljs @@ -6,6 +6,7 @@ (ns frontend-tests.data.workspace-variant-rename-test (:require + [app.common.path-names :as cpn] [app.common.test-helpers.components :as cthc] [app.common.test-helpers.files :as cthf] [app.common.test-helpers.ids-map :as cthi] @@ -13,9 +14,13 @@ [app.common.test-helpers.variants :as thv] [app.common.types.file :as ctf] [app.common.types.pages-list :as ctpl] + [app.main.data.changes :as dch] [app.main.data.workspace :as dw] + [app.main.data.workspace.variants :as dwv] + [beicon.v2.core :as rx] [cljs.test :as t :include-macros true] - [frontend-tests.helpers.state :as ths])) + [frontend-tests.helpers.state :as ths] + [potok.v2.core :as ptk])) (t/use-fixtures :each {:before cthi/reset-idmap!}) @@ -28,6 +33,15 @@ :variant2-params {:name "Button"}) (cths/update-shape :variant :name "Button"))) +(defn- variant-file-on-other-page + [] + (-> (cthf/sample-file :file1 :page-label :page1) + (thv/add-variant :variant :comp1 :root1 :comp2 :root2 + :variant1-params {:name "Button"} + :variant2-params {:name "Button"}) + (cths/update-shape :variant :name "Button") + (cthf/add-sample-page :page2))) + (defn- without-variant-name [file label] (let [page-id (cthf/current-page-id file) @@ -47,16 +61,22 @@ "Run workspace events, then validate the file before assertions. Pass `:validate? false` only when the fixture is intentionally incomplete - (e.g. a variant main without `:variant-name`)." - [file events done assert-fn & {:keys [validate?] :or {validate? true}}] - (ths/run-store - (ths/setup-store file) - done - events - (fn [state] - (when validate? - (cthf/validate-file! (ths/get-file-from-state state))) - (assert-fn state)))) + (e.g. a variant main without `:variant-name`). Pass `:commits` as an atom + to collect each `dch/commit?` event from the store input." + [file events done assert-fn & {:keys [validate? commits] :or {validate? true}}] + (let [store (ths/setup-store file)] + (when commits + (->> (ptk/input-stream store) + (rx/filter dch/commit?) + (rx/subs! #(swap! commits conj %)))) + (ths/run-store + store + done + events + (fn [state] + (when validate? + (cthf/validate-file! (ths/get-file-from-state state))) + (assert-fn state))))) (t/deftest end-rename-shape-does-not-apply-variant-formula (t/async @@ -230,3 +250,61 @@ (t/is (= "Renamed" (:name (cthc/get-component file :comp)))) (t/is (= "Box" (:name (cths/get-shape file :rect)))) (t/is (nil? (:shape-for-rename (:workspace-local state)))))))))) + +(t/deftest end-rename-shape-renames-variant-container-in-one-commit + (t/async + done + (let [commits (atom [])] + (run (variant-file) + [(dw/end-rename-shape (cthi/id :variant) "Renamed set")] + done + (fn [state] + (let [file (ths/get-file-from-state state) + comp1 (cthc/get-component file :comp1) + comp2 (cthc/get-component file :comp2)] + (t/is (= 1 (count @commits))) + (cthf/validate-file! file) + (t/is (= "Renamed set" (:name (cths/get-shape file :variant)))) + (t/is (= "Renamed set" (:name (cths/get-shape file :root1)))) + (t/is (= "Renamed set" (:name (cths/get-shape file :root2)))) + (t/is (= "Renamed set" + (cpn/merge-path-item (:path comp1) (:name comp1)) + (cpn/merge-path-item (:path comp2) (:name comp2)))))) + :commits commits)))) + +(t/deftest rename-variant-from-another-page-keeps-the-open-page + (t/async + done + (let [file (variant-file-on-other-page) + open-page-id (cthf/current-page-id file)] + (run file + [(dwv/rename-variant (cthi/id :variant) "Renamed")] + done + (fn [state] + (let [file (ths/get-file-from-state state) + comp1 (cthc/get-component file :comp1) + comp2 (cthc/get-component file :comp2)] + (t/is (= open-page-id (:current-page-id state))) + (t/is (= "Renamed" (:name (cths/get-shape file :variant :page-label :page1)))) + (t/is (= "Renamed" (:name (cths/get-shape file :root1 :page-label :page1)))) + (t/is (= "Renamed" (:name (cths/get-shape file :root2 :page-label :page1)))) + (t/is (= "Renamed" + (cpn/merge-path-item (:path comp1) (:name comp1)) + (cpn/merge-path-item (:path comp2) (:name comp2)))))))))) + +(t/deftest rename-variant-stores-empty-for-a-slash-only-name + (t/async + done + (run (variant-file) + [(dwv/rename-variant (cthi/id :variant) "///")] + done + (fn [state] + (let [file (ths/get-file-from-state state) + comp1 (cthc/get-component file :comp1) + comp2 (cthc/get-component file :comp2)] + (t/is (= "(empty)" (:name (cths/get-shape file :variant)))) + (t/is (= "(empty)" (:name (cths/get-shape file :root1)))) + (t/is (= "(empty)" (:name (cths/get-shape file :root2)))) + (t/is (= "" (:path comp1) (:path comp2))) + (t/is (= "(empty)" (:name comp1) (:name comp2))) + (t/is (= (cthi/id :variant) (:variant-id comp1) (:variant-id comp2)))))))) From 7666d7d50015f4bf3cd0b28e0470dadf66d37566 Mon Sep 17 00:00:00 2001 From: Pablo Alba <pablo.alba@kaleidos.net> Date: Tue, 3 Nov 2026 10:42:59 +0100 Subject: [PATCH 10/13] :bug: Fix Creating or combining a variant set renames the component first --- common/src/app/common/logic/variants.cljc | 18 ++ .../common_tests/logic/variants_test.cljc | 44 +++++ .../src/app/main/data/workspace/variants.cljs | 26 +-- .../data/workspace_variants_test.cljs | 160 ++++++++++++++++-- 4 files changed, 214 insertions(+), 34 deletions(-) diff --git a/common/src/app/common/logic/variants.cljc b/common/src/app/common/logic/variants.cljc index b9c2037788..9a78d6cc20 100644 --- a/common/src/app/common/logic/variants.cljc +++ b/common/src/app/common/logic/variants.cljc @@ -38,6 +38,24 @@ (pcb/update-shapes changes shape-ids #(assoc % :name stored-name)) components))) +(defn generate-define-variant-container + "One change vector. `name` is the string stored on the board. + The component path and name are `split-group-name` of that string. + Library data is refreshed before the first property is added." + [changes variant-id main-id component-id name] + (let [[path item-name] (cpn/split-group-name name)] + (-> changes + (pcb/update-shapes [variant-id] + #(assoc % :is-variant-container true :name name)) + (pcb/update-shapes [main-id] + #(assoc % :name name :variant-id variant-id)) + (pcb/update-component component-id + #(assoc % :variant-id variant-id + :path path + :name item-name) + {:apply-changes-local-library? true}) + (clvp/generate-add-new-property variant-id {:fill-values? true})))) + (defn generate-add-new-variant [changes shape variant-id new-component-id new-shape-id prop-num] (let [data (pcb/get-library-data changes) diff --git a/common/test/common_tests/logic/variants_test.cljc b/common/test/common_tests/logic/variants_test.cljc index 75b292598f..0323df3afb 100644 --- a/common/test/common_tests/logic/variants_test.cljc +++ b/common/test/common_tests/logic/variants_test.cljc @@ -14,10 +14,12 @@ [app.common.logic.variants :as clv] [app.common.path-names :as cpn] [app.common.test-helpers.components :as thc] + [app.common.test-helpers.compositions :as tho] [app.common.test-helpers.files :as thf] [app.common.test-helpers.ids-map :as thi] [app.common.test-helpers.shapes :as ths] [app.common.test-helpers.variants :as thv] + [app.common.types.component :as ctk] [app.common.types.variant :as ctv] [clojure.test :as t] [cuerdas.core :as str])) @@ -566,6 +568,48 @@ (t/is (= (ctv/properties-to-name props') (:variant-name main'))) (t/is (= "active, crítica" (:variant-name main'))))) +(defn- post-artboard-alpha-file + "A normal frame holding one main named Alpha — before the container flag." + [] + (-> (thf/sample-file :file1) + (tho/add-frame :v01 {:name "Board"}) + (tho/add-simple-component :c01 :m01 :r01 + :root-params {:parent-label :v01 :name "Alpha"}))) + +(t/deftest test-define-variant-container-renames-component-in-same-vector + (let [file (post-artboard-alpha-file) + page (thf/current-page file) + variant-id (thi/id :v01) + main-id (thi/id :m01) + component-id (:id (thc/get-component file :c01)) + changes (-> (pcb/empty-changes nil (:id page)) + (pcb/with-objects (:objects page)) + (pcb/with-library-data (:data file)) + (clv/generate-define-variant-container + variant-id main-id component-id "Component")) + file' (thf/apply-changes file changes) + container' (ths/get-shape file' :v01) + main' (ths/get-shape file' :m01) + comp' (thc/get-component file' :c01) + restored (thf/apply-undo-changes file' changes) + container-rest (ths/get-shape restored :v01) + main-rest (ths/get-shape restored :m01) + comp-rest (thc/get-component restored :c01)] + + (t/is (ctk/is-variant-container? container')) + (t/is (= "Component" + (:name container') + (:name main') + (cpn/merge-path-item (:path comp') (:name comp')))) + (t/is (= variant-id (:variant-id main') (:variant-id comp'))) + (t/is (= (ctv/properties-to-name (:variant-properties comp')) + (:variant-name main'))) + (t/is (not (ctk/is-variant-container? container-rest))) + (t/is (= "Alpha" (:name main-rest))) + (t/is (= "Alpha" (cpn/merge-path-item (:path comp-rest) (:name comp-rest)))) + (t/is (nil? (:variant-id main-rest))) + (t/is (nil? (:variant-id comp-rest))))) + (defn- rename-variant-changes [file name] (let [page (thf/current-page file)] diff --git a/frontend/src/app/main/data/workspace/variants.cljs b/frontend/src/app/main/data/workspace/variants.cljs index 18ac285055..11aa37c85c 100644 --- a/frontend/src/app/main/data/workspace/variants.cljs +++ b/frontend/src/app/main/data/workspace/variants.cljs @@ -304,21 +304,6 @@ (dch/commit-changes changes) (dwu/commit-undo-transaction undo-id))))))))) -(defn generate-define-variant-container - "One commit that turns an existing frame into a valid variant set: - container flag and name, the main's variant-id, the component's - variant-id (refreshing library data), then the first property." - [changes variant-id main-id component-id name] - (-> changes - (pcb/update-shapes [variant-id] - #(assoc % :is-variant-container true :name name)) - (pcb/update-shapes [main-id] - #(assoc % :name name :variant-id variant-id)) - (pcb/update-component component-id - #(assoc % :variant-id variant-id) - {:apply-changes-local-library? true}) - (clvp/generate-add-new-property variant-id {:fill-values? true}))) - (defn- define-variant-container "Read page objects after the artboard exists, then commit the defining variant fields in one change." @@ -332,7 +317,8 @@ changes (-> (pcb/empty-changes it page-id) (pcb/with-objects objects) (pcb/with-library-data data) - (generate-define-variant-container variant-id main-id component-id name))] + (clv/generate-define-variant-container + variant-id main-id component-id name))] (rx/of (dch/commit-changes changes)))))) (defn- focus-property @@ -340,7 +326,10 @@ (ptk/reify ::focus-property ptk/EffectEvent (effect [_ _ _] - (dom/focus! (dom/get-element (str "variant-prop-" variant-id "-0")))))) + ;; Unit tests run without a DOM; skip rather than throw into the store. + (when (exists? js/document) + (some-> (dom/get-element (str "variant-prop-" variant-id "-0")) + (dom/focus!)))))) (defn- resposition-and-resize-variant "Resize the variant container, and move the shape (that is a variant) to the right" @@ -468,9 +457,6 @@ (rx/of (dwu/start-undo-transaction undo-id) - (when (not= name (:name main)) - (dwl/rename-component component-id name)) - ;; Create the frame while it is still a normal board (dwsh/create-artboard-from-shapes [main-instance-id] variant-id nil nil nil delta flex?) (cl/remove-all-fills variant-vec {:color clr/black :opacity 1}) diff --git a/frontend/test/frontend_tests/data/workspace_variants_test.cljs b/frontend/test/frontend_tests/data/workspace_variants_test.cljs index 44e68d6745..20c2fa7acf 100644 --- a/frontend/test/frontend_tests/data/workspace_variants_test.cljs +++ b/frontend/test/frontend_tests/data/workspace_variants_test.cljs @@ -8,6 +8,7 @@ (:require [app.common.files.changes-builder :as pcb] [app.common.files.variant :as cfv] + [app.common.logic.variants :as clv] [app.common.path-names :as cpn] [app.common.test-helpers.components :as cthc] [app.common.test-helpers.compositions :as tho] @@ -181,6 +182,20 @@ (tho/add-simple-component :c02 :m02 :r02 :root-params {:name "Beta" :x 150 :y 0 :width 100 :height 80}))) +(defn- two-mains-shared-prefix-file + [] + (-> (cthf/sample-file :file1) + (tho/add-simple-component :c01 :m01 :r01 + :root-params {:name "UI / Alpha" :x 0 :y 0 :width 100 :height 80}) + (tho/add-simple-component :c02 :m02 :r02 + :root-params {:name "UI / Beta" :x 150 :y 0 :width 100 :height 80}))) + +(defn- path-named-main-file + [] + (-> (cthf/sample-file :file1) + (tho/add-simple-component :c01 :m01 :r01 + :root-params {:name "Group / Button"}))) + (defn- two-mains-on-page1-with-page2 "Two mains on :page1; the open page is :page2 (Assets combine while elsewhere)." [] @@ -201,6 +216,10 @@ (tho/add-simple-component :c01 :m01 :r01 :root-params {:parent-label :v01 :name "Set"}))) +(defn- component-full-name + [component] + (cpn/merge-path-item (:path component) (:name component))) + (defn- page-objects [file] (:objects (cthf/current-page file))) @@ -258,7 +277,7 @@ changes (-> (pcb/empty-changes nil (:id page)) (pcb/with-objects (:objects page)) (pcb/with-library-data (:data file)) - (dwv/generate-define-variant-container + (clv/generate-define-variant-container variant-id main-id component-id name)) file' (cthf/apply-changes file changes) container' (cths/get-shape file' :v01) @@ -271,7 +290,7 @@ (t/is (= variant-id (:variant-id comp'))) (t/is (some? (:variant-id comp'))) (t/is (= (ctv/properties-to-name (:variant-properties comp')) - (:variant-name main')))))) + (:variant-name main'))))) (t/deftest transform-in-variant-validates-after-every-commit (t/async @@ -320,6 +339,54 @@ (t/is (= variant-id (:variant-id comp'))))) after-commits-idle)))) +(t/deftest transform-in-variant-path-name-validates-after-every-commit + (t/async + done + (let [file (path-named-main-file) + main-id (cthi/id :m01) + variant-id (uuid/next) + snapshots (atom []) + store (ths/setup-store file)] + (watch-applied-commits store snapshots) + (ths/run-store + store + done + [(dwv/transform-in-variant main-id variant-id nil [] false true true)] + (fn [state] + (let [snaps @snapshots + flagged (first-flagged snaps variant-id) + file' (ths/get-file-from-state state) + objects' (page-objects file') + container' (get objects' variant-id) + main' (get objects' main-id) + comp' (cthc/get-component file' :c01) + before-flag (take-while + #(not (ctk/is-variant-container? + (get (page-objects (:file %)) variant-id))) + snaps)] + (assert-valid-snapshots snaps) + (t/is (some? flagged) "a commit must set :is-variant-container") + (doseq [snap before-flag] + (let [ffile (:file snap) + fmain (get (page-objects ffile) main-id) + fcomp (cthc/get-component ffile :c01)] + (t/is (= "Group / Button" (:name fmain))) + (t/is (= "Group / Button" (component-full-name fcomp))))) + (when flagged + (let [ffile (:file flagged) + fobjs (page-objects ffile) + fcont (get fobjs variant-id) + fmain (get fobjs main-id) + fcomp (cthc/get-component ffile :c01)] + (t/is (= "Group" (:name fcont) (:name fmain) (component-full-name fcomp))) + (t/is (= variant-id (:variant-id fmain) (:variant-id fcomp))) + (t/is (= (ctv/properties-to-name (:variant-properties fcomp)) + (:variant-name fmain))))) + (t/is (ctk/is-variant-container? container')) + (t/is (= "Group" (:name container') (:name main') (component-full-name comp'))) + (t/is (= variant-id (:variant-id main') (:variant-id comp'))))) + after-commits-idle)))) + (t/deftest add-new-variant-on-existing-set-still-validates (t/async done @@ -363,19 +430,37 @@ done [(dwv/combine-as-variants [id1 id2] {:variant-id variant-id :trigger "test"})] (fn [state] - (let [snaps @snapshots - file' (ths/get-file-from-state state) - objects (page-objects file') - container (get objects variant-id) - m01' (get objects id1) - m02' (get objects id2) - c01' (cthc/get-component file' :c01) - c02' (cthc/get-component file' :c02) - names1 (mapv :name (:variant-properties c01')) - names2 (mapv :name (:variant-properties c02')) - ordered (mapv :main-instance-id - (cfv/find-variant-components (:data file') objects variant-id))] + (let [snaps @snapshots + file' (ths/get-file-from-state state) + objects (page-objects file') + container (get objects variant-id) + m01' (get objects id1) + m02' (get objects id2) + c01' (cthc/get-component file' :c01) + c02' (cthc/get-component file' :c02) + names1 (mapv :name (:variant-properties c01')) + names2 (mapv :name (:variant-properties c02')) + ordered (mapv :main-instance-id + (cfv/find-variant-components (:data file') objects variant-id)) + before-flag (take-while + #(not (ctk/is-variant-container? + (get (page-objects (:file %)) variant-id))) + snaps) + flagged (first-flagged snaps variant-id)] (assert-valid-snapshots snaps) + (doseq [snap before-flag] + (let [ffile (:file snap) + fmain (get (page-objects ffile) id1) + fcomp (cthc/get-component ffile :c01)] + (t/is (= "Alpha" (:name fmain))) + (t/is (= "Alpha" (component-full-name fcomp))))) + (when flagged + (let [ffile (:file flagged) + fobjs (page-objects ffile) + fcont (get fobjs variant-id) + fmain (get fobjs id1) + fcomp (cthc/get-component ffile :c01)] + (t/is (= "Component" (:name fcont) (:name fmain) (component-full-name fcomp))))) (doseq [{:keys [file]} snaps] (let [objs (page-objects file) cont (get objs variant-id) @@ -392,6 +477,53 @@ (t/is (= variant-id (:parent-id m02'))))) after-commits-idle)))) +(t/deftest combine-as-variants-shared-prefix-validates-after-every-commit + (t/async + done + (let [file (two-mains-shared-prefix-file) + id1 (cthi/id :m01) + id2 (cthi/id :m02) + variant-id (uuid/next) + snapshots (atom []) + store (ths/setup-store file)] + (watch-applied-commits store snapshots) + (ths/run-store + store + done + [(dwv/combine-as-variants [id1 id2] {:variant-id variant-id :trigger "test"})] + (fn [state] + (let [snaps @snapshots + file' (ths/get-file-from-state state) + objects (page-objects file') + container (get objects variant-id) + m01' (get objects id1) + m02' (get objects id2) + c01' (cthc/get-component file' :c01) + c02' (cthc/get-component file' :c02) + flagged (first-flagged snaps variant-id)] + (assert-valid-snapshots snaps) + (doseq [{:keys [file]} snaps] + (let [objs (page-objects file) + cont (get objs variant-id) + kids (set (:shapes cont))] + (when (contains? kids id2) + (t/is (ctk/is-variant-container? cont) + "other mains must not enter the frame before it is a variant container")))) + (when flagged + (let [ffile (:file flagged) + fobjs (page-objects ffile) + fcont (get fobjs variant-id) + fmain (get fobjs id1) + fcomp (cthc/get-component ffile :c01)] + (t/is (= "UI" (:name fcont) (:name fmain) (component-full-name fcomp))))) + (t/is (ctk/is-variant-container? container)) + (t/is (= "UI" (:name container) (:name m01') (:name m02') + (component-full-name c01') (component-full-name c02'))) + (t/is (= variant-id (:variant-id m01') (:variant-id m02') + (:variant-id c01') (:variant-id c02'))) + (t/is (= variant-id (:parent-id m01') (:parent-id m02'))))) + after-commits-idle)))) + (t/deftest combine-as-variants-from-another-page-validates-after-every-commit (t/async done From d82b5a4b1e7cb6369b59c639dc952644f091ef24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elena=20Torr=C3=B3?= <elenatorro@gmail.com> Date: Thu, 5 Nov 2026 15:00:46 +0100 Subject: [PATCH 11/13] :bug: Fix pasted text invisible in new text box (#12859) --- frontend/src/app/main/ui/workspace/shapes/text/editor.cljs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/frontend/src/app/main/ui/workspace/shapes/text/editor.cljs b/frontend/src/app/main/ui/workspace/shapes/text/editor.cljs index 770a1ebec7..f7e55cd8be 100644 --- a/frontend/src/app/main/ui/workspace/shapes/text/editor.cljs +++ b/frontend/src/app/main/ui/workspace/shapes/text/editor.cljs @@ -256,7 +256,10 @@ :on-blur on-blur :on-focus on-focus :handle-return handle-return - :strip-pasted-styles true + ;; Do not set strip-pasted-styles. When true, draft-js never calls + ;; handlePastedText, so caret/block styles are not applied on paste. + ;; handle-pasted-text always returns "handled", so pasted HTML/styles + ;; are still stripped. :handle-pasted-text handle-pasted-text :handle-drop handle-drop :custom-style-fn (partial styles-fn shape) From f4a0d42363d28aad5af6223ad275105006152fc2 Mon Sep 17 00:00:00 2001 From: Pablo Alba <pablo.alba@kaleidos.net> Date: Thu, 5 Nov 2026 16:52:52 +0100 Subject: [PATCH 12/13] :bug: Keep multi-select variant switches refreshing every instance (#12857) * :bug: Keep multi-select variant switches refreshing every instance Multi variants-switch ended each swap with select-shape, which replaced the selection, interrupted layout/WASM work, and navigated once per copy so only one instance looked updated under flex. Skip per-swap select and layout in the batch, re-read live geometry, coalesce layout ids via a companion DataEvent, then restore selection once. The same path covers Assets component-multi-swap. Frontend tests lock select-shape count, layout coalesce, and mixed no-op selections. Closes #12829 AI-assisted-by: Composer --- .../app/main/data/workspace/libraries.cljs | 174 ++++++++---- .../src/app/main/data/workspace/variants.cljs | 96 ++++--- .../logic/multi_variant_switch_test.cljs | 254 ++++++++++++++++++ frontend/test/frontend_tests/runner.cljs | 2 + 4 files changed, 427 insertions(+), 99 deletions(-) create mode 100644 frontend/test/frontend_tests/logic/multi_variant_switch_test.cljs diff --git a/frontend/src/app/main/data/workspace/libraries.cljs b/frontend/src/app/main/data/workspace/libraries.cljs index bc52e63736..8d2aec1597 100644 --- a/frontend/src/app/main/data/workspace/libraries.cljs +++ b/frontend/src/app/main/data/workspace/libraries.cljs @@ -1057,67 +1057,123 @@ (cfh/get-children-with-self (:objects page) (:id root)) (cfh/get-parents-with-self objects (:parent-id shape)))))) +(def component-swap-layout-ids-type + "Companion DataEvent type emitted by `component-swap` when `:skip-layout?` + is set. Batch callers filter this exact keyword (libraries ns) — never a + bare `::component-swap-layout-ids` in another namespace." + ::component-swap-layout-ids) + +(def ^:private component-swap-batch-done-type + ::component-swap-batch-done) + +(defn coalesce-skipped-layout + "While `swap-source` runs, collect companion layout-id DataEvents from + `stream`, then emit one `:layout/update` with the union. + + `swap-source` must emit WatchEvents already configured with + `:skip-layout? true`. Uses a done marker (not `(rx/take (count …))`) so + no-op swaps cannot hang the fold. Reads the buffer inside `rx/defer` so + the id set is not frozen empty when `watch` returns." + [stream swap-source] + (let [layout-acc (volatile! [])] + (rx/concat + (rx/merge + (->> stream + (rx/take-until (rx/filter (ptk/type? component-swap-batch-done-type) stream)) + (rx/filter (ptk/type? component-swap-layout-ids-type)) + (rx/tap (fn [ev] + (vswap! layout-acc into (:ids (deref ev))))) + (rx/ignore)) + (rx/concat + swap-source + (rx/of (ptk/data-event component-swap-batch-done-type {})))) + (rx/defer + #(rx/of (ptk/data-event :layout/update {:ids @layout-acc})))))) + (defn component-swap - "Swaps a component with another one" - [shape file-id id-new-component keep-touched?] - (dm/assert! (uuid? id-new-component)) - (dm/assert! (uuid? file-id)) - (ptk/reify ::component-swap - ptk/WatchEvent - (watch [it state _] - ;; First delete shapes so we have space in the layout otherwise we can have problems - ;; in the grid creating new rows/columns to make space - (let [libraries (dsh/lookup-libraries state) - page (dsh/lookup-page state) - objects (:objects page) - parent (get objects (:parent-id shape)) + "Swaps a component with another one. - ldata (dsh/lookup-file-data state file-id) - orig-shapes (when keep-touched? (cfh/get-children-with-self objects (:id shape))) + Optional `opts`: + - `:skip-select?` — do not emit `select-shape` after the swap (batch callers + restore selection once). + - `:skip-layout?` — do not emit `:layout/update`; emit a companion + `component-swap-layout-ids-type` DataEvent instead so the batch can + coalesce one layout update." + ([shape file-id id-new-component keep-touched?] + (component-swap shape file-id id-new-component keep-touched? nil)) + ([shape file-id id-new-component keep-touched? + {:keys [skip-select? skip-layout?] :or {skip-select? false skip-layout? false}}] + (dm/assert! (uuid? id-new-component)) + (dm/assert! (uuid? file-id)) + (ptk/reify ::component-swap + ptk/WatchEvent + (watch [it state _] + ;; First delete shapes so we have space in the layout otherwise we can have problems + ;; in the grid creating new rows/columns to make space + (let [libraries (dsh/lookup-libraries state) + page (dsh/lookup-page state) + objects (:objects page)] + ;; Re-read from live state: sibling swaps / layout can move this head. + ;; Gone from objects → no-op (do not swap a closed-over stale shape). + (if-let [shape (get objects (:id shape))] + (let [parent (get objects (:parent-id shape)) - ;; If the target parent is a grid layout we need to pass the target cell - target-cell (when (ctsl/grid-layout? parent) - (ctsl/get-cell-by-shape-id parent (:id shape))) + ldata (dsh/lookup-file-data state file-id) + orig-shapes (when keep-touched? (cfh/get-children-with-self objects (:id shape))) - index (find-shape-index objects (:parent-id shape) (:id shape)) + ;; If the target parent is a grid layout we need to pass the target cell + target-cell (when (ctsl/grid-layout? parent) + (ctsl/get-cell-by-shape-id parent (:id shape))) - ;; Store the properties that need to be maintained when the component is swapped - keep-props-values (select-keys shape ctk/swap-keep-attrs) + index (find-shape-index objects (:parent-id shape) (:id shape)) - undo-id (js/Symbol) - undo-group (uuid/next) + ;; Store the properties that need to be maintained when the component is swapped + keep-props-values (select-keys shape ctk/swap-keep-attrs) - [new-shape all-parents changes] - (-> (pcb/empty-changes it (:id page)) - (pcb/set-undo-group undo-group) - (cll/generate-component-swap objects shape ldata page libraries id-new-component - index target-cell keep-props-values keep-touched?)) + undo-id (js/Symbol) + undo-group (uuid/next) - updated-objects (pcb/get-objects changes) - new-children-ids (cfh/get-children-ids-with-self updated-objects (:id new-shape)) - new-text-ids (->> new-children-ids - (keep (fn [id] - (when-let [child (get updated-objects id)] - (when (and (cfh/text-shape? child) - (not= :fixed (:grow-type child))) - id)))) - (vec)) + [new-shape all-parents changes] + (-> (pcb/empty-changes it (:id page)) + (pcb/set-undo-group undo-group) + (cll/generate-component-swap objects shape ldata page libraries id-new-component + index target-cell keep-props-values keep-touched?)) - [changes parents-of-swapped] - (if keep-touched? - (clv/generate-keep-touched changes new-shape shape orig-shapes page libraries ldata) - [changes []]) - update-layout-ids (concat all-parents parents-of-swapped new-children-ids)] - (rx/of - (dwu/start-undo-transaction undo-id) - (dch/commit-changes changes) - (dwtp/propagate-workspace-tokens) ;; Make the new instance get the token values from the current file, not from the component's library - (when (and (features/active-feature? state "render-wasm/v1") - (seq new-text-ids)) - (dwwt/resize-wasm-text-all new-text-ids {:skip-component-sync? true})) - (ptk/data-event :layout/update {:ids update-layout-ids :undo-group undo-group}) - (dwu/commit-undo-transaction undo-id) - (dws/select-shape (:id new-shape) false)))))) + updated-objects (pcb/get-objects changes) + new-children-ids (cfh/get-children-ids-with-self updated-objects (:id new-shape)) + new-text-ids (->> new-children-ids + (keep (fn [id] + (when-let [child (get updated-objects id)] + (when (and (cfh/text-shape? child) + (not= :fixed (:grow-type child))) + id)))) + (vec)) + + [changes parents-of-swapped] + (if keep-touched? + (clv/generate-keep-touched changes new-shape shape orig-shapes page libraries ldata) + [changes []]) + update-layout-ids (concat all-parents parents-of-swapped new-children-ids)] + (cond-> (rx/of + (dwu/start-undo-transaction undo-id) + (dch/commit-changes changes) + ;; Make the new instance get the token values from the current file, + ;; not from the component's library + (dwtp/propagate-workspace-tokens) + (when (and (features/active-feature? state "render-wasm/v1") + (seq new-text-ids)) + (dwwt/resize-wasm-text-all new-text-ids {:skip-component-sync? true}))) + (not skip-layout?) + (rx/concat (rx/of (ptk/data-event :layout/update + {:ids update-layout-ids :undo-group undo-group}))) + skip-layout? + (rx/concat (rx/of (ptk/data-event component-swap-layout-ids-type + {:ids update-layout-ids}))) + true + (rx/concat (rx/of (dwu/commit-undo-transaction undo-id))) + (not skip-select?) + (rx/concat (rx/of (dws/select-shape (:id new-shape) false))))) + (rx/empty))))))) (defn component-multi-swap "Swaps several components with another one" @@ -1131,17 +1187,21 @@ {::ev/name "component-swap"}) ptk/WatchEvent - (watch [_ state _] - (let [undo-id (js/Symbol)] + (watch [_ state stream] + (let [undo-id (js/Symbol) + swap-opts {:skip-select? true :skip-layout? true}] (log/info :msg "COMPONENT-SWAP" :file (debug-pretty-file file-id state) :id-new-component id-new-component :undo-id undo-id) (rx/concat (rx/of (dwu/start-undo-transaction undo-id)) - (rx/map #(component-swap % file-id id-new-component false) (rx/from shapes)) - (rx/of (dwu/commit-undo-transaction undo-id)) - (rx/of (dwsp/open-specialized-panel :component-swap))))))) + (coalesce-skipped-layout + stream + (->> (rx/from shapes) + (rx/map #(component-swap % file-id id-new-component false swap-opts)))) + (rx/of (dwu/commit-undo-transaction undo-id) + (dwsp/open-specialized-panel :component-swap))))))) (def valid-asset-types #{:colors :components :typographies}) diff --git a/frontend/src/app/main/data/workspace/variants.cljs b/frontend/src/app/main/data/workspace/variants.cljs index 11aa37c85c..46730ee112 100644 --- a/frontend/src/app/main/data/workspace/variants.cljs +++ b/frontend/src/app/main/data/workspace/variants.cljs @@ -780,60 +780,72 @@ (rx/of (combine-as-variants selected options)))))) (defn- variant-switch - "Switch the shape (that must be a variant copy head) for the closest one with the property value passed as parameter" - [shape {:keys [pos val] :as params}] + "Switch the shape (that must be a variant copy head) for the closest one with the property value passed as parameter. + + `params` may include `:swap-opts` forwarded to `component-swap` (e.g. + `{:skip-select? true :skip-layout? true}` for a multi batch)." + [shape {:keys [pos val swap-opts] :as params}] (ptk/reify ::variant-switch ptk/WatchEvent (watch [it state _] - (let [libraries (dsh/lookup-libraries state) - component-id (:component-id shape) - component (ctf/get-component libraries (:component-file shape) component-id :include-deleted? false)] - ;; If the value is already val, do nothing - (when (not= val (dm/get-in component [:variant-properties pos :value])) - (let [current-page-objects (dsh/lookup-page-objects state) - variant-id (:variant-id component) - component-file-data (dm/get-in libraries [(:component-file shape) :data]) - component-page-objects (-> (dsh/get-page component-file-data (:main-instance-page component)) - (get :objects)) - variant-comps (cfv/find-variant-components component-file-data component-page-objects variant-id) - target-props (-> (:variant-properties component) - (update pos assoc :value val)) - valid-comps (->> variant-comps - (remove #(= (:id %) component-id)) - (filter #(= (dm/get-in % [:variant-properties pos :value]) val)) - (reverse)) - nearest-comp (apply min-key #(ctv/distance target-props (:variant-properties %)) valid-comps) - shape-parents (cfh/get-parents-with-self current-page-objects (:parent-id shape)) - nearest-comp-children (cfh/get-children-with-self component-page-objects (:main-instance-id nearest-comp)) - comps-nesting-loop? (seq? (cfh/components-nesting-loop? nearest-comp-children shape-parents)) + (let [objects (dsh/lookup-page-objects state)] + ;; Sibling swaps / layout can move this head before our turn. + ;; Gone from objects → no-op (do not switch a closed-over stale shape). + (if-let [shape (get objects (:id shape))] + (let [libraries (dsh/lookup-libraries state) + component-id (:component-id shape) + component (ctf/get-component libraries (:component-file shape) component-id :include-deleted? false)] + ;; If the value is already val, do nothing + (if (not= val (dm/get-in component [:variant-properties pos :value])) + (let [variant-id (:variant-id component) + component-file-data (dm/get-in libraries [(:component-file shape) :data]) + component-page-objects (-> (dsh/get-page component-file-data (:main-instance-page component)) + (get :objects)) + variant-comps (cfv/find-variant-components component-file-data component-page-objects variant-id) + target-props (-> (:variant-properties component) + (update pos assoc :value val)) + valid-comps (->> variant-comps + (remove #(= (:id %) component-id)) + (filter #(= (dm/get-in % [:variant-properties pos :value]) val)) + (reverse)) + nearest-comp (apply min-key #(ctv/distance target-props (:variant-properties %)) valid-comps) + shape-parents (cfh/get-parents-with-self objects (:parent-id shape)) + nearest-comp-children (cfh/get-children-with-self component-page-objects (:main-instance-id nearest-comp)) + comps-nesting-loop? (seq? (cfh/components-nesting-loop? nearest-comp-children shape-parents)) - {:keys [on-error] - :or {on-error rx/throw}} (meta params)] + {:keys [on-error] + :or {on-error rx/throw}} (meta params)] - ;; If there is no nearest-comp, do nothing - (when nearest-comp - (if comps-nesting-loop? - (do - (on-error) - (rx/empty)) - (rx/of - (dwl/component-swap shape (:component-file shape) (:id nearest-comp) true) - (ev/event (-> {::ev/name "variant-switch" ::ev/origin "workspace:design-tab"} - (merge (meta it))))))))))))) + ;; If there is no nearest-comp, do nothing + (if nearest-comp + (if comps-nesting-loop? + (do + (on-error) + (rx/empty)) + (rx/of + (dwl/component-swap shape (:component-file shape) (:id nearest-comp) true swap-opts) + (ev/event (-> {::ev/name "variant-switch" ::ev/origin "workspace:design-tab"} + (merge (meta it)))))) + (rx/empty))) + (rx/empty))) + (rx/empty)))))) (defn variants-switch "Switch each shape (that must be a variant copy head) for the closest one with the property value passed as parameter" [{:keys [shapes] :as params}] (ptk/reify ::variants-switch ptk/WatchEvent - (watch [it _ _] - (let [ids (into (d/ordered-set) d/xf:map-id shapes) - undo-id (js/Symbol)] + (watch [it _ stream] + (let [ids (into (d/ordered-set) d/xf:map-id shapes) + undo-id (js/Symbol) + swap-opts {:skip-select? true :skip-layout? true}] (rx/concat (rx/of (dwu/start-undo-transaction undo-id)) - (->> (rx/from shapes) - (rx/map (fn [data] - (-> (variant-switch data params) - (with-meta (meta it)))))) + (dwl/coalesce-skipped-layout + stream + (->> (rx/from shapes) + (rx/map (fn [shape] + (-> (variant-switch shape (assoc params :swap-opts swap-opts)) + (with-meta (meta it))))))) (rx/of (dwu/commit-undo-transaction undo-id) (dws/select-shapes ids))))))) diff --git a/frontend/test/frontend_tests/logic/multi_variant_switch_test.cljs b/frontend/test/frontend_tests/logic/multi_variant_switch_test.cljs new file mode 100644 index 0000000000..3bcd56d3a5 --- /dev/null +++ b/frontend/test/frontend_tests/logic/multi_variant_switch_test.cljs @@ -0,0 +1,254 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS SUBSIDIARY SL + +(ns frontend-tests.logic.multi-variant-switch-test + "Regression tests for #12829: multi-select variants-switch / + component-multi-swap must not emit one select-shape per copy, and must + coalesce skipped per-swap layout into one :layout/update." + (:require + [app.common.data :as d] + [app.common.test-helpers.components :as cthc] + [app.common.test-helpers.compositions :as ctho] + [app.common.test-helpers.files :as cthf] + [app.common.test-helpers.ids-map :as cthi] + [app.common.test-helpers.shapes :as cths] + [app.common.test-helpers.variants :as cthv] + [app.common.uuid :as uuid] + [app.main.data.workspace.libraries :as dwl] + [app.main.data.workspace.selection :as-alias dws] + [app.main.data.workspace.variants :as dwv] + [beicon.v2.core :as rx] + [cljs.test :as t :include-macros true] + [frontend-tests.helpers.pages :as thp] + [frontend-tests.helpers.state :as ths] + [potok.v2.core :as ptk])) + +(t/use-fixtures :each + {:before thp/reset-idmap!}) + +(def ^:private fill-value1 "#2152e5") +(def ^:private fill-value2 "#e52152") + +(defn- count-events + "Atom counting how many events of `type` land on the store input-stream + (nested WatchEvents only appear there — not on a bare ptk/watch of + variants-switch)." + [store type] + (let [counter (atom 0)] + (->> (ptk/input-stream store) + (rx/filter (ptk/type? type)) + (rx/tap (fn [_] (swap! counter inc))) + (rx/subs! (fn [_] nil))) + counter)) + +(defn- collect-layout-updates + "Atom accumulating deref'd `:layout/update` payloads from the store + input-stream (same subscribe-before-emit pattern as `count-events`)." + [store] + (let [acc (atom [])] + (->> (ptk/input-stream store) + (rx/filter (ptk/type? :layout/update)) + (rx/tap #(swap! acc conj (deref %))) + (rx/subs! (fn [_] nil))) + acc)) + +(defn- assert-coalesced-layout + [layout-updates flex-board-id] + (t/is (= 1 (count @layout-updates)) + (str "expected one coalesced :layout/update; got " + (count @layout-updates))) + (let [ids (set (:ids (first @layout-updates)))] + (t/is (seq ids) + "coalesced :layout/update must carry non-empty :ids") + (t/is (contains? ids flex-board-id) + (str "coalesced :ids must include flex board; got " ids)))) + +(defn- fill-color + [shape] + (-> shape :fills first :fill-color)) + +(defn- setup-flex-variant-file + "Two variant members with distinct fills and sizes." + [] + (-> (cthf/sample-file :file1) + (cthv/add-variant :v01 :c01 :m01 :c02 :m02 + :variant1-params {:fills (cths/sample-fills-color + :fill-color fill-value1) + :width 40 + :height 40} + :variant2-params {:fills (cths/sample-fills-color + :fill-color fill-value2) + :width 50 + :height 50}) + (ctho/add-frame :flex-board + :name "Toggle Group" + :layout :flex + :layout-flex-dir :row + :width 400 + :height 80))) + +(defn- setup-flex-multi-copies + "Three Value1 copies as children of a flex board." + [] + (-> (setup-flex-variant-file) + (cthc/instantiate-component :c01 :copy1 :parent-label :flex-board) + (cthc/instantiate-component :c01 :copy2 :parent-label :flex-board) + (cthc/instantiate-component :c01 :copy3 :parent-label :flex-board))) + +(defn- setup-flex-mixed-copies + "Two Value1 copies and one already on Value2 (no-op on switch to Value2)." + [] + (-> (setup-flex-variant-file) + (cthc/instantiate-component :c01 :copy1 :parent-label :flex-board) + (cthc/instantiate-component :c01 :copy2 :parent-label :flex-board) + (cthc/instantiate-component :c02 :copy3 :parent-label :flex-board))) + +(t/deftest multi-variants-switch-emits-no-intermediate-select-shape + (t/async + done + (let [file (setup-flex-multi-copies) + store (ths/setup-store file) + copy1 (cths/get-shape file :copy1) + copy2 (cths/get-shape file :copy2) + copy3 (cths/get-shape file :copy3) + flex-board-id (:id (cths/get-shape file :flex-board)) + target-comp-id (cthi/id :c02) + copy-ids (d/ordered-set (:id copy1) (:id copy2) (:id copy3)) + select-shape-n (count-events store ::dws/select-shape) + layout-updates (collect-layout-updates store) + events + [(dwv/variants-switch {:shapes [copy1 copy2 copy3] + :pos 0 + :val "Value2"})]] + + (ths/run-store + store done events + (fn [new-state] + (let [file' (ths/get-file-from-state new-state) + copy1' (cths/get-shape file' :copy1) + copy2' (cths/get-shape file' :copy2) + copy3' (cths/get-shape file' :copy3) + selected (get-in new-state [:workspace-local :selected])] + + (t/is (zero? @select-shape-n) + (str "multi variants-switch must not emit intermediate " + "select-shape; got " @select-shape-n)) + (assert-coalesced-layout layout-updates flex-board-id) + + (t/is (= target-comp-id (:component-id copy1'))) + (t/is (= target-comp-id (:component-id copy2'))) + (t/is (= target-comp-id (:component-id copy3'))) + (t/is (= fill-value2 (fill-color copy1'))) + (t/is (= fill-value2 (fill-color copy2'))) + (t/is (= fill-value2 (fill-color copy3'))) + (t/is (= (set copy-ids) selected)))))))) + +;; Done-marker collection must not hang when some selected copies already +;; have the target value (no companion from those no-ops). +(t/deftest multi-variants-switch-coalesces-layout-when-some-noop + (t/async + done + (let [file (setup-flex-mixed-copies) + store (ths/setup-store file) + copy1 (cths/get-shape file :copy1) + copy2 (cths/get-shape file :copy2) + copy3 (cths/get-shape file :copy3) + flex-board-id (:id (cths/get-shape file :flex-board)) + target-comp-id (cthi/id :c02) + copy-ids (d/ordered-set (:id copy1) (:id copy2) (:id copy3)) + select-shape-n (count-events store ::dws/select-shape) + layout-updates (collect-layout-updates store) + events + [(dwv/variants-switch {:shapes [copy1 copy2 copy3] + :pos 0 + :val "Value2"})]] + + (ths/run-store + store done events + (fn [new-state] + (let [file' (ths/get-file-from-state new-state) + copy1' (cths/get-shape file' :copy1) + copy2' (cths/get-shape file' :copy2) + copy3' (cths/get-shape file' :copy3) + selected (get-in new-state [:workspace-local :selected])] + + (t/is (zero? @select-shape-n)) + (assert-coalesced-layout layout-updates flex-board-id) + + (t/is (= target-comp-id (:component-id copy1'))) + (t/is (= target-comp-id (:component-id copy2'))) + (t/is (= target-comp-id (:component-id copy3'))) + (t/is (= fill-value2 (fill-color copy1'))) + (t/is (= fill-value2 (fill-color copy2'))) + (t/is (= fill-value2 (fill-color copy3'))) + (t/is (= (set copy-ids) selected)))))))) + +(t/deftest component-multi-swap-skips-select-and-coalesces-layout + (t/async + done + (let [file (setup-flex-multi-copies) + store (ths/setup-store file) + copy1 (cths/get-shape file :copy1) + copy2 (cths/get-shape file :copy2) + copy3 (cths/get-shape file :copy3) + flex-board-id (:id (cths/get-shape file :flex-board)) + target-comp-id (cthi/id :c02) + select-shape-n (count-events store ::dws/select-shape) + layout-updates (collect-layout-updates store) + events + [(dwl/component-multi-swap [copy1 copy2 copy3] + (:id file) + target-comp-id)]] + + (ths/run-store + store done events + (fn [new-state] + (let [file' (ths/get-file-from-state new-state) + copy1' (cths/get-shape file' :copy1) + copy2' (cths/get-shape file' :copy2) + copy3' (cths/get-shape file' :copy3)] + + (t/is (zero? @select-shape-n) + (str "component-multi-swap must not emit intermediate " + "select-shape; got " @select-shape-n)) + (assert-coalesced-layout layout-updates flex-board-id) + + (t/is (= target-comp-id (:component-id copy1'))) + (t/is (= target-comp-id (:component-id copy2'))) + (t/is (= target-comp-id (:component-id copy3'))) + (t/is (= fill-value2 (fill-color copy1'))) + (t/is (= fill-value2 (fill-color copy2'))) + (t/is (= fill-value2 (fill-color copy3'))))))))) + +(t/deftest component-swap-noop-when-shape-missing + (t/async + done + (let [file (setup-flex-multi-copies) + store (ths/setup-store file) + copy1 (cths/get-shape file :copy1) + ghost (assoc copy1 :id (uuid/next)) + before (ths/get-file-from-state @store)] + (ths/run-store + store done + [(dwl/component-swap ghost (:id file) (cthi/id :c02) true)] + (fn [new-state] + (t/is (= before (ths/get-file-from-state new-state)) + "component-swap must no-op when shape id is absent from objects")))))) + +(t/deftest variant-switch-noop-when-shape-missing + (t/async + done + (let [file (setup-flex-multi-copies) + store (ths/setup-store file) + copy1 (cths/get-shape file :copy1) + ghost (assoc copy1 :id (uuid/next)) + before (ths/get-file-from-state @store)] + (ths/run-store + store done + [(#'dwv/variant-switch ghost {:pos 0 :val "Value2"})] + (fn [new-state] + (t/is (= before (ths/get-file-from-state new-state)) + "variant-switch must no-op when shape id is absent from objects")))))) diff --git a/frontend/test/frontend_tests/runner.cljs b/frontend/test/frontend_tests/runner.cljs index 94188ea2b4..df1a89ef60 100644 --- a/frontend/test/frontend_tests/runner.cljs +++ b/frontend/test/frontend_tests/runner.cljs @@ -52,6 +52,7 @@ [frontend-tests.logic.copying-and-duplicating-test] [frontend-tests.logic.frame-guides-test] [frontend-tests.logic.groups-test] + [frontend-tests.logic.multi-variant-switch-test] [frontend-tests.logic.nudge-selected-shapes-test] [frontend-tests.logic.pasting-in-containers-test] [frontend-tests.logic.path-actions-test] @@ -204,6 +205,7 @@ 'frontend-tests.logic.copying-and-duplicating-test 'frontend-tests.logic.frame-guides-test 'frontend-tests.logic.groups-test + 'frontend-tests.logic.multi-variant-switch-test 'frontend-tests.logic.nudge-selected-shapes-test 'frontend-tests.logic.path-actions-test 'frontend-tests.logic.path-clipboard-test From 84e0b33b748658061a2ceee11f11de84f34d45b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?andr=C3=A9s=20gonz=C3=A1lez?= <andres.gonzalez79@gmail.com> Date: Thu, 5 Nov 2026 22:30:20 +0100 Subject: [PATCH 13/13] :books: Explain what to do after losing access to an account (#12866) Add a section to Your account for cloud users who can no longer sign in, for example after their Google account was suspended. It points them to support@penpot.app, lists what to send so support can confirm the account is theirs, and suggests exporting important files. AI-assisted-by: claude-opus-5-5 --- docs/user-guide/account-teams/your-account.njk | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/docs/user-guide/account-teams/your-account.njk b/docs/user-guide/account-teams/your-account.njk index 00233dccc9..f738f82361 100644 --- a/docs/user-guide/account-teams/your-account.njk +++ b/docs/user-guide/account-teams/your-account.njk @@ -30,6 +30,19 @@ desc: Learn how to import and export files in Penpot, the free, open-source desi <p>After several failed login attempts in a short time, your account can be locked for some minutes. Penpot shows how long to wait. Resetting your password unlocks it.</p> <p>On self-hosted installations, the administrator sets these limits. See <a href="/technical-guide/configuration/#session-expiration">Session expiration</a> and <a href="/technical-guide/configuration/#account-lockout">Account lockout</a> in the Technical Guide.</p> +<h3 id="your-account-recovery">If you lose access to your account + <a class="direct-link" href="#your-account-recovery">#</a> +</h3> +<p>If you can no longer sign in to your Penpot cloud account, for example because your Google account was suspended or you lost access to your email, write to <a href="mailto:support@penpot.app">support@penpot.app</a>. Once we confirm that the account is yours, we can give you access to its files from an account you can use.</p> +<p>Include in your message:</p> +<ul> + <li>The email of the account you can no longer access.</li> + <li>The email you can use now, if you already have another Penpot account.</li> + <li>Anything that helps us confirm the account is yours: names of projects, files or boards, file links, or screenshots of your designs.</li> +</ul> +<p>To avoid losing work, <a href="/user-guide/export-import/export-import-files/#files-export">export your important files</a> from time to time and keep the <code>.penpot</code> files somewhere safe.</p> +<p>On self-hosted installations, contact the administrator of your instance.</p> + <h3 id="your-account-notifications">Notifications <a class="direct-link" href="#your-account-notifications">#</a> </h3>