diff --git a/.agents/README.md b/.agents/README.md index 165a4a7745..e4abf7ae64 100644 --- a/.agents/README.md +++ b/.agents/README.md @@ -1,19 +1,391 @@ -# Agent skills +# Agentic development with opencode inside devenv -This folder is the single home for the skills our coding agents use. -Each skill is a folder with a `SKILL.md` inside — a short instruction -manual that an agent loads only when it needs it. +This doc shows how to run AI-assisted development for Penpot inside the +devenv with [opencode](https://opencode.ai). It covers the setup once, +then points to the skills that drive daily work. -One copy serves every tool: +Full reference lives in the technical guide: -- **opencode** reads this folder directly. -- **Claude Code** reads it through the `.claude/skills` symlink. -- **Codex** reads it directly. +- [Dev environment](../docs/technical-guide/developer/devenv.md) -To change how the agents behave, edit the `SKILL.md` here. There is no -second copy to keep in sync. +This file does not repeat those guides. It gives the short path and +leaves room for notes we add step by step. -## How the skills are organized +## TL;DR + +```bash +./manage.sh pull-devenv +./manage.sh run-devenv --ws 0 --attach +``` + +Then open a shell in the container tmux session, run `opencode` inside +~/penpot directory. + +## 1. Introduction + +The LLM client — opencode, Claude Code, or Codex — runs in a shell +inside the plain devenv container, with the repo mounted and the +skills in this folder driving the work. One client session per +workspace (`ws0` is the live repo, `ws1+` are sibling clones). + +This doc is written around opencode, but Claude Code runs the same +way inside devenv and follows exactly the same flows. This is not +the "agentic devenv" (`--agentic`) from the technical guide, which +runs the client outside devenv and wires it in over MCP — here the +client lives inside the sandboxed devenv docker. + +Unlike the agentic devenv, running the client inside the devenv +docker gives it full access to the live environment: every +dependency already resolved by the image, so the agent can write +and run tests directly, query the running PostgreSQL, and reach +the backend and frontend through nREPL — no proxies, no round +trips outside the container. + +And if you later want vision, it is one MCP entry away — a +headless Playwright server in your `opencode.json`: + +```json +{ + "$schema": "https://opencode.ai/config.json", + "mcp": { + "playwright": { + "type": "local", + "command": ["npx", "-y", "@playwright/mcp", "--headless"], + "enabled": true, + "env": {} + } + } +} +``` + +## 2. Quickstart: bring up devenv and run opencode inside + +Pull once, then bring up the workspace you want (add `--ws 1`, +`--ws 2`, … for more): + +```bash +./manage.sh pull-devenv +./manage.sh run-devenv --ws 0 --attach # ws0 (the live repo) +``` + +This attaches to the container tmux session. Open a new shell there +(`Ctrl+b c`), `cd` to the repo, and run opencode directly: + +```bash +cd penpot; +opencode +``` + +One session drives exactly one workspace — for N parallel workspaces, +open one container shell and one opencode session per workspace. + +Stop with `./manage.sh stop-devenv [--ws N | --all]`. Shared infra +stops when the last workspace stops. + + +## 3. Connecting providers + +### Starting point: Zen free models, no login needed + +The easiest way to try the setup is Zen's free models — they need no +login and no key. Just run opencode, pick a free model, and work. + +That said, creating a Zen account and connecting with your API key is +still worth it from day one: it unlocks the full model list, spend +limits, auto-reload, and the Go-overflow fallback below. Connect +through the TUI: + +- Run `/connect`, pick a provider, paste the key. +- Run `/models` to see what that provider offers. + +### OpenCode Go (subscription, best value for daily use) + +$10/month subscription with generous included usage — up to $60/month +of model consumption at published per-token rates, depending on the +model. Best value if you work mostly with open coding models +(GLM, Kimi, Qwen, DeepSeek, MiniMax, LongCat…). + +The strong part: when you hit a model's monthly limit, Go can fall +back to your Zen balance instead of blocking (enable *Use balance* in +the console). So the setup many of us use is Go + Zen credit on top — +subscription first, pay-as-you-go overflow after. + +### OpenCode Zen (pay per use) + +Zen is the opencode team's gateway: curated models tested for coding +agents, fair prices, no markups, stable latency. You top up credit +and pay per request, with monthly spend limits and auto-reload. + +Free usage is generous: the free models carry limits good enough for +real work, not just a quick taste. Worth knowing: brand-new, +unannounced models often show up on Zen first with a very generous +free quota so people try them — e.g. `0x Alpha`, which later turned +out to be GLM-5.3-Flash. Keep an eye on the free list; the newest +entry is often the best deal. + +Two reasons to have a Zen account even with Go: + +- It absorbs Go overflow (see above). +- Its free models let you try the whole setup before paying. + +### OpenRouter (widest catalog) + +If you already have an account, connect it: the widest model range in +one place. Trade-off is latency and occasional instability versus Zen, +which is tuned for coding agents. + +Beyond code: OpenRouter also serves image, video, and audio models. +opencode itself cannot call those directly — it is built for code — +but a cheap model can quickly build you a small tool or script that +talks to them through the OpenRouter API. So if you also generate +content other than code and text, having OpenRouter connected is +worth it: the agent wires the plumbing for you. + +### OpenAI (subscription or API key) + +If you have an OpenAI subscription or API access, connect it — it +works very well as a daily driver alongside (or instead of) Go/Zen. + +### Suggested combos + +| Profile | Connect | +|---|---| +| Try it out | Nothing (Zen free models, no login) | +| Try it out, properly | Zen account + API key (free models + limits) | +| Daily use, best value | Go + Zen credit (overflow) | +| Widest model choice | Add OpenRouter | +| Already pay OpenAI | Add OpenAI account | + +## 4. Recommended models + +Personal picks from Andrey, current as of September 2026. Models come +and go, so treat this as a snapshot — the shape (one cheap solver, +one reviewer/planner, one explorer) matters more than the names. + +| Model | Role | How often | +|---|---|---| +| Muse Spark 1.3 (`high`) | Main solver: plan, review, develop. Sharp and cheap — covers ~70% of coding tasks. | Daily | +| GLM-5.3-Flash | Reasoning all-rounder, now mostly code/plan reviewer and planner. | Daily | +| DeepSeek V4.1 Flash (`high`) | Explorer: code and idea exploration, sometimes development. Especially good at small bash/node utilities for repo chores and changelog updates. | Daily | +| LongCat 2.0 | Backup solver, occasional stand-in for Muse Spark 1.3. | Weekly | +| GPT-5.6 Luna | Alternative to DeepSeek Flash; pricier, unclear the extra cost pays off. | Rarely | +| Qwen3.8 Flash | As strong as the top three; used in rotation to avoid hammering one model. Less Go subsidy than the top picks, so mostly in overflow mode. | Overflow | +| MiMo-V2.5-Pro | Former main model; slightly pricier now next to Muse Spark / GLM-Flash / LongCat, and less Go subsidy — used in overflow. | Overflow | +| Kimi K3 | Heavy reasoning for hard reviews and plans. Expensive, ~1% of tasks. | Rarely | +| GLM-5.3 | Same slot as Kimi K3: hard reviews and plans only. | Rarely | + +**TL;DR:** the first three (Muse Spark 1.3, GLM-5.3-Flash, DeepSeek +V4.1 Flash) are a good starting point. + +## 5. Customizing your `opencode.json` + +opencode merges config in this order (later wins): + +1. Global: `~/.config/opencode/opencode.json` (on host, or the dir + mounted with `--opencode-config-dir` inside devenv — + see §9 Advanced usage). +2. Project: `opencode.json` at the repo root (gitignored on purpose — + use it to override the global entries for one workspace). + +Below is a full working example of my personal config at the date of +writing this. It is only an example: define whatever subagents you +need, with whatever models you like or work with. + +Copy it to `opencode.json` on the root of the repo: + +```json +{ + "$schema": "https://opencode.ai/config.json", + "disabled_providers": ["amazon-bedrock"], + "subagent_depth": 2, + "agent": { + "compaction": { + "model": "opencode-go/deepseek-flash", + "variant": "high" + }, + "title": { + "model": "opencode-go/deepseek-flash", + "variant": "low" + }, + "explore": { + "model": "opencode-go/deepseek-flash", + "variant": "high" + }, + "build": { + "prompt": "{file:.agents/prompts/engineer-agent-prompt.md}", + "permission": { + "external_directory": { + "/tmp/**": "allow" + } + } + }, + "general": { + "prompt": "{file:.agents/prompts/engineer-agent-prompt.md}", + "permission": { + "external_directory": { + "/tmp/**": "allow" + } + } + }, + "engineer-glm": { + "mode": "subagent", + "model": "opencode-go/glm-5.3-flash", + "variant": "high", + "prompt": "{file:.agents/prompts/engineer-agent-prompt.md}", + "permission": { + "*": "allow", + "task": { + "*": "allow" + } + } + }, + "engineer-kimi": { + "mode": "subagent", + "model": "opencode/kimi-k3", + "variant": "high", + "prompt": "{file:.agents/prompts/engineer-agent-prompt.md}", + "permission": { + "*": "allow", + "task": { + "*": "allow" + } + } + }, + + "engineer-qwen": { + "mode": "subagent", + "model": "opencode-go/qwen3.7-plus", + "variant": "high", + "prompt": "{file:.agents/prompts/engineer-agent-prompt.md}", + "permission": { + "*": "allow", + "task": { + "*": "allow" + } + } + } + } +} +``` + +What the blocks mean: + +- `compaction` / `title` / `explore`: cheap background agents. Keep + them on a fast model; `title` uses the `low` variant on purpose. +- `build` / `general`: the main agents. They load the shared prompt + `{file:.agents/prompts/engineer-agent-prompt.md}` and may only touch + `/tmp/**` outside the repo without asking for explicit permision. +- `engineer-*`: one subagent per model family, all with the same + prompt and full permissions (`"*": "allow"`). They purpose are + specially for delegate work to them because are defined to be used + only as subagents. +- `disabled_providers` / `subagent_depth`: global guards. Keep + `"$schema"` — opencode refuses to start if any field is wrong. + +How the `engineer-*` subagents are actually used — delegating work to +them to keep the main context clean — is covered in §6 Common agentic +flows. + +Note this is opencode-only: other clients have their own way of +defining subagents or helpers — or none at all. + +## 6. Common agentic flows + +Work happens two ways: directly in your session, or delegated to a +subagent. Besides the `engineer-*` subagents from §5 there is a +builtin `general` subagent. Delegating planning and review to a +subagent starts a fresh, clean context with a clean prompt instead +of growing the main session — the main lever for keeping context +small. To delegate without switching models, delegate to `general`. + +### Issue / error report flow + +1. **Frame the problem.** Enter Plan mode (TAB in opencode) and paste the + report with your intent: "investigate this and find the possible cause", + "investigate and tell me where this points", or "does this still apply?". + Explore until you and the agent roughly agree on the problem. +2. **Write the plan.** Run `/make-a-plan` — it executes in Build mode. + If you need to step in and answer something yourself, press TAB to + leave Build mode. Use Plan mode only when you want a hard guarantee + that the agent modifies no file under any circumstance. If you + explored with a weaker model but want a stronger one to write the + plan, switch models first or delegate: + `/make-a-plan delegate to @engineer-glm`. +3. **Iterate on the plan.** The plan is saved to `.agents/plans/`, so you + never depend on LLM memory: read the file directly, or run `/review-plan` + for a second opinion (delegation works here too). Complex plans deserve a + review; simple ones can skip it. +4. **Execute.** Run `/implement-plan`. It first prints the full picture — + whether it will create an issue and a branch, the execution style, and a + task checklist — and waits for your go-ahead. Say "step by step" to stop + after each task (one commit per task) so you can verify as it goes; + the default runs all tasks with one final commit. +5. **Land the work.** When it finishes, either push yourself and run + `/create-pr`, or loop `/review-code` → `/make-a-plan` → + `/implement-plan` until the findings are addressed, then push and + `/create-pr`. Nothing pushes for you — you always push from your shell. + +> Note: `/implement-plan` checks the current branch. On a base branch +> (`main`, `develop`, `staging`) it creates a GitHub issue and a branch +> `issue-NNNN`; on an existing feature branch it continues there and +> creates nothing. The pre-run summary tells you which applies. Read the +> skill at `.agents/skills/implement-plan/SKILL.md` — it is +> self-explanatory. + +### Big feature with multiple plans + +When the work is too large for a single plan, tell `/make-a-plan` +up front: produce a high-level roadmap where each task will get its +own execution plan, and the roadmap doubles as the progress tracker. + +From there the flow mirrors the issue flow above, one level down: +take each roadmap task in turn, write its own plan (`/make-a-plan`, +delegating when it helps), review it when the task is complex +(`/review-plan`), implement it (`/implement-plan`), and mark progress +on the roadmap as you land each piece. + +## 7. Connecting `gh` CLI with a token + +The `create-issue` and `create-pr` flows need an authenticated `gh` +so they can run on their own. Create a fine-grained token with the +minimum scopes: + +1. GitHub → Settings → Developer settings → Personal access tokens → + Fine-grained tokens → Generate new token. +2. Under Organization permissions, grant access to **Projects**. +3. Under Repository permissions, grant at least **Issues** and + **Pull requests**. + +Then authenticate the CLI and follow the prompts: + +```bash +gh auth login +``` + +Verify with `gh auth status` (token lives in +`~/.config/gh/hosts.yml`). You still push from your own shell — the +agents only read and open issues and PRs. + +## 8. Troubleshooting / FAQ + +> TBD — filled in step by step as issues come up. + +## 9. Advanced usage + +### Personal agents and prompts without committing them here + +Bind-mount a host dir over the container's `~/.config/opencode`: + +```bash +./manage.sh run-devenv --ws 0 --opencode-config-dir ../penpot-opencode +``` + +It applies at container creation, so changing it needs a stop + rerun +of that instance. + +## Summary of available skills + +### How the skills are organized **Flows** are the six skills you invoke by name. Each one covers one step in the life of a change: plan it, review the plan, implement it, review @@ -28,7 +400,7 @@ issue, a commit. Flows call them, but they also work on their own. **Utilities** are small helpers for everyday work: search, file lookup, JSON, REPL access, and so on. -## Flows +### Flows | Skill | What it does | When you would say | |---|---|---| @@ -39,14 +411,14 @@ JSON, REPL access, and so on. | [`create-pr`](skills/create-pr/SKILL.md) | Opens a pull request for the current branch — with checks on base branch, commits, issue, and push state — or updates an existing PR's title and description. | "open a PR for this branch" | | [`resolve-git-conflicts`](skills/resolve-git-conflicts/SKILL.md) | Untangles merge or rebase conflicts: explains both sides, proposes a resolution, applies it after you approve. Never runs `git rebase --continue`. | "resolve these conflicts" | -## References +### References | Skill | What it holds | |---|---| | [`plan-review-criteria`](skills/plan-review-criteria/SKILL.md) | The plan review rubric: six axes, severity levels, approval standard, output format. The `review-plan` reviewer loads it. | | [`code-review-criteria`](skills/code-review-criteria/SKILL.md) | The code review rubric: five axes, core principles (DRY, KISS, YAGNI), severity format, verdict. The `review-code` reviewer loads it. | -## Procedures +### Procedures | Skill | What it does | |---|---| @@ -54,7 +426,7 @@ JSON, REPL access, and so on. | [`create-issue`](skills/create-issue/SKILL.md) | Creates a GitHub issue that follows Penpot conventions. Used by `implement-plan`; also works on its own. | | [`create-commit`](skills/create-commit/SKILL.md) | Makes a commit the Penpot way: emoji subject, clear body, `AI-assisted-by` trailer. Used by `implement-plan`; also works alone when you say "commit this". | -## Utilities +### Utilities | Skill | What it does | |---|---| @@ -71,7 +443,7 @@ JSON, REPL access, and so on. | [`refine-prompt`](skills/refine-prompt/SKILL.md) | Rewrites a rough prompt into a clearer one. Never runs the prompt. | | [`update-changelog`](skills/update-changelog/SKILL.md) | Regenerates `CHANGES.md` from a GitHub milestone. | -## A typical round +### A typical round 1. `/make-a-plan` — you get a plan and a saved file in `.agents/plans/`. 2. `/review-plan` — a second opinion; approve or request changes. @@ -82,7 +454,7 @@ JSON, REPL access, and so on. Every step also works on its own, and you can always say what you want in plain words — the agents pick the right skill from what you say. -## Adding or changing a skill +### Adding or changing a skill Create a folder here with a `SKILL.md` inside. The file needs `name` and `description` in its frontmatter, and a clear "When to use" section so diff --git a/.agents/prompts/engineer-agent-prompt.md b/.agents/prompts/engineer-agent-prompt.md new file mode 100644 index 0000000000..a075392fbe --- /dev/null +++ b/.agents/prompts/engineer-agent-prompt.md @@ -0,0 +1,15 @@ +Act as a senior full-stack software engineer for this project. + +## Instructions + +1. Read `AGENTS.md` first and follow its memory-reading rules: read `mem:critical-info`, then the core memory of every module your work touches, plus any deeper memories they reference. +2. Work autonomously: explore the codebase first, follow existing patterns and conventions, apply DRY/KISS. +3. Verify before finishing: run tests, lint and fm, fix anything you broke. Never report done with failing checks. +4. Before finishing, review the affected memories and documentation against the implementation. If the change introduces behavior, contracts, decisions, or constraints that are not documented, or makes existing documentation inaccurate, update the relevant memories and docs in the same change. + +## Strong Rules + +1. All new functionality ships with tests. No exceptions. +2. Do not touch unrelated modules. +3. Never `git push`, force-push, or modify remotes. Only create commits when the + command or the user explicitly instructs it. diff --git a/README.md b/README.md index a419ef7b8e..ffd702aa31 100644 --- a/README.md +++ b/README.md @@ -94,11 +94,11 @@ Penpot is the only design & prototype platform that is deployment agnostic. You Learn how to install it with Docker, Kubernetes, Elestio or other options on [our website](https://penpot.app/self-host). -2 - ## Penpot Enterprise ## -Penpot Enterprise is our paid plan for organizations that need to scale their design work across multiple teams with advanced governance, security, and administration. Manage teams and access from a centralized **Admin Console**, configure advanced permissions, and connect your **identity provider through SSO**. Available for cloud and self-hosted environments, it combines enterprise controls with Penpot’s open-source foundation and open standards. +[Penpot Enterprise](https://help.penpot.app/user-guide/account-teams/enterprise-plan/) is our paid plan for organizations that need to scale their design work across multiple teams with advanced governance, security, and administration. Manage teams and access from a centralized **Admin Console**, configure advanced permissions, and connect your **identity provider through SSO**. Available for cloud and self-hosted environments, it combines enterprise controls with Penpot’s open-source foundation and open standards. + +2 ## Community ## diff --git a/backend/src/app/config.clj b/backend/src/app/config.clj index 57c82a859e..879158119f 100644 --- a/backend/src/app/config.clj +++ b/backend/src/app/config.clj @@ -300,6 +300,18 @@ [:ssrf-allowed-hosts {:optional true} [::sm/set :string]] [:ssrf-extra-blocked-cidrs {:optional true} [::sm/set :string]]])) +(defn telemetry-excluded-host? + "Returns true when the given host belongs to the official SaaS + instances, where telemetry must be fully disabled." + [host] + (let [host (some-> host (str/lower) (str/trim))] + (and (string? host) + (not (str/blank? host)) + (or (= host "penpot.dev") + (= host "penpot.app") + (str/ends-with? host ".penpot.dev") + (str/ends-with? host ".penpot.app"))))) + (defn- parse-flags [config] (let [public-uri (c/get config :public-uri) @@ -388,6 +400,15 @@ ([key default] (c/get config key default))) +(defn telemetry-excluded? + "Returns true when telemetry must be fully disabled because the + public-uri host points to an official instance (penpot.dev or + penpot.app). When true, no telemetry data is collected or sent, + not even the limited newsletter report." + [] + (let [host (some-> (c/get config :public-uri) (u/uri) :host)] + (telemetry-excluded-host? host))) + (defn logging-context [] {:backend/version (:full version)}) diff --git a/backend/src/app/http/management.clj b/backend/src/app/http/management.clj index 50ce8506ad..bccd51cf4b 100644 --- a/backend/src/app/http/management.clj +++ b/backend/src/app/http/management.clj @@ -13,6 +13,7 @@ [app.common.time :as ct] [app.config :as cf] [app.db :as db] + [app.http.middleware :as mw] [app.main :as-alias main] [app.rpc.commands.profile :as cmd.profile] [app.setup :as-alias setup] @@ -57,11 +58,11 @@ (if key (fn [request] (if-let [key' (yreq/get-header request "x-shared-key")] - (if (= key key') + (if (mw/constant-time-eq? key key') (handler request) {::yres/status 403}) {::yres/status 403})) - (fn [_ _] + (fn [_] {::yres/status 403}))))}) (defmethod ig/init-key ::routes diff --git a/backend/src/app/http/middleware.clj b/backend/src/app/http/middleware.clj index 22c1cc29d9..3914cdc3de 100644 --- a/backend/src/app/http/middleware.clj +++ b/backend/src/app/http/middleware.clj @@ -330,7 +330,7 @@ {:name ::auth :compile (constantly wrap-auth)}) -(defn- constant-time-eq? +(defn constant-time-eq? "Compare strings in constant time to prevent timing attacks." [^String a ^String b] (MessageDigest/isEqual (.getBytes a "UTF-8") (.getBytes b "UTF-8"))) @@ -350,7 +350,7 @@ (handler)) {::yres/status 403})) {::yres/status 403})) - (fn [_ _] + (fn [_] {::yres/status 403}))) (def shared-key-auth diff --git a/backend/src/app/rpc/commands/demo.clj b/backend/src/app/rpc/commands/demo.clj index c21ae33838..8a917e548c 100644 --- a/backend/src/app/rpc/commands/demo.clj +++ b/backend/src/app/rpc/commands/demo.clj @@ -10,6 +10,7 @@ [app.auth :refer [derive-password-weak]] [app.common.exceptions :as ex] [app.common.schema :as sm] + [app.common.time :as ct] [app.common.uuid :as uuid] [app.config :as cf] [app.db :as db] @@ -25,7 +26,35 @@ (def ^:private schema:create-demo-profile [:map - [:skip-onboarding {:optional true} ::sm/boolean]]) + [:skip-onboarding {:optional true} ::sm/boolean] + [:expires-in {:optional true} ::ct/duration]]) + +(def ^:private min-expires-in + (ct/duration "5m")) + +(defn- resolve-deletion-delay + "Resolve the effective `:demo-purge` delay for a demo profile. Without + `expires-in` it falls back to the global deletion delay. Otherwise the + value is only allowed to shorten the lifetime: below the 5 minutes + minimum or above the global delay it raises a validation error." + [expires-in] + (let [max-delay (cf/get-deletion-delay)] + (cond + (nil? expires-in) + max-delay + + (ct/is-before? expires-in min-expires-in) + (ex/raise :type :validation + :code :invalid-expires-in + :hint "expires-in is below the 5 minutes minimum.") + + (ct/is-after? expires-in max-delay) + (ex/raise :type :validation + :code :invalid-expires-in + :hint "expires-in exceeds the configured deletion delay.") + + :else + expires-in))) (sv/defmethod ::create-demo-profile "A command that is responsible of creating a demo purpose @@ -34,42 +63,44 @@ {::rpc/auth false ::doc/added "1.15" ::doc/changes [["1.15" "This method is migrated from mutations to commands."] - ["2.18" "Add optional `skip-onboarding` param. When true, the profile is created with `onboarding-viewed` and `release-notes-viewed` (current version) set, skipping the onboarding flow."]] + ["2.18" "Add optional `skip-onboarding` param. When true, the profile is created with `onboarding-viewed` and `release-notes-viewed` (current version) set, skipping the onboarding flow."] + ["2.18" "Add optional `expires-in` param. When set, the demo purge is scheduled that long after creation instead of the global deletion delay. Only values between 5 minutes and the global delay are accepted."]] ::sm/params schema:create-demo-profile} - [cfg {:keys [skip-onboarding]}] + [cfg {:keys [skip-onboarding expires-in]}] (when-not (contains? cf/flags :demo-users) (ex/raise :type :validation :code :demo-users-not-allowed :hint "Demo users are disabled by config.")) - (let [sem (uuid/next) - email (str "demo-" sem "@demo.example.com") - fullname (str "Demo User " sem) + (let [deletion-delay (resolve-deletion-delay expires-in) + sem (uuid/next) + email (str "demo-" sem "@demo.example.com") + fullname (str "Demo User " sem) - password (-> (bn/random-bytes 16) - (bc/bytes->b64 true) - (bc/bytes->str)) + password (-> (bn/random-bytes 16) + (bc/bytes->b64 true) + (bc/bytes->str)) - params {:email email - :fullname fullname - :is-active true - :is-demo true - :password (derive-password-weak password) - :props (cond-> {} - skip-onboarding (assoc :onboarding-viewed true - ;; Redundant today: auth/create-profile - ;; overwrites this with the current - ;; version, kept so the skip does not - ;; depend on that default. - :release-notes-viewed (:main cf/version)))} - profile (db/tx-run! cfg (fn [cfg] - (->> (auth/create-profile cfg params) - (auth/create-profile-rels cfg))))] + params {:email email + :fullname fullname + :is-active true + :is-demo true + :password (derive-password-weak password) + :props (cond-> {} + skip-onboarding (assoc :onboarding-viewed true + ;; Redundant today: auth/create-profile + ;; overwrites this with the current + ;; version, kept so the skip does not + ;; depend on that default. + :release-notes-viewed (:main cf/version)))} + profile (db/tx-run! cfg (fn [cfg] + (->> (auth/create-profile cfg params) + (auth/create-profile-rels cfg))))] (wrk/submit! (-> cfg (assoc ::wrk/task :demo-purge) - (assoc ::wrk/delay (cf/get-deletion-delay)) + (assoc ::wrk/delay deletion-delay) (assoc ::wrk/params {:profile-id (:id profile)}))) (with-meta {:email email diff --git a/backend/src/app/tasks/telemetry.clj b/backend/src/app/tasks/telemetry.clj index 7064c8f40c..254ca5bf35 100644 --- a/backend/src/app/tasks/telemetry.clj +++ b/backend/src/app/tasks/telemetry.clj @@ -26,7 +26,8 @@ (let [sql "SELECT email FROM profile where props->>'~:newsletter-updates' = 'true'"] (db/run! cfg (fn [{:keys [::db/conn]}] (->> (db/exec! conn [sql]) - (mapv :email)))))) + (into [] (map :email)) + (not-empty)))))) ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; LEGACY DATA COLLECTION @@ -312,8 +313,9 @@ send? (get params :send? true) enabled? (or (get params :enabled? false) (contains? cf/flags :telemetry)) - subs (get-subscriptions cfg)] - + ;; Deferred so the query runs only when a report is + ;; actually going to be sent. + subs (delay (get-subscriptions cfg))] ;; If we have telemetry enabled, then proceed the normal ;; operation sending legacy report @@ -327,7 +329,7 @@ (try (let [stats (db/run! cfg get-legacy-stats)] - (send-legacy-data cfg stats subs)) + (send-legacy-data cfg stats @subs)) (catch Exception cause (l/wrn :hint "unable to send legacy report" :cause cause))) @@ -346,7 +348,10 @@ ;; onboarding dialog or the profile section, then proceed to ;; send a limited telemetry data, that consists in the list of ;; subscribed emails and the running penpot version. - (when (and send? (seq subs)) + ;; Official instances (penpot.dev / penpot.app) are excluded: + ;; they must never send subscriber emails to the telemetry + ;; endpoint (which is ourselves). + (when (and (not (cf/telemetry-excluded?)) send? @subs) (px/sleep (rand-int 10000)) (ex/ignoring - (send-legacy-data cfg nil subs))))))) + (send-legacy-data cfg nil @subs))))))) diff --git a/backend/test/backend_tests/helpers.clj b/backend/test/backend_tests/helpers.clj index d4d8935418..37abedc5a8 100644 --- a/backend/test/backend_tests/helpers.clj +++ b/backend/test/backend_tests/helpers.clj @@ -627,3 +627,66 @@ (parse-sse (slurp' input))) (finally (.close input))))) + +;; ---- Dummy Request Helpers + +(defrecord DummyRequest [headers cookies method body-stream + remote-addr server-name server-port + scheme protocol path query ssl-client-cert] + yrq/IRequestCookies + (get-cookie [_ name] + {:value (get cookies name)}) + + yrq/IRequest + (get-header [_ name] + (get headers name)) + (method [_] method) + (body [_] body-stream) + (path [_] path) + (query [_] query) + (server-port [_] server-port) + (server-name [_] server-name) + (remote-addr [_] remote-addr) + (ssl-client-cert [_] ssl-client-cert) + (scheme [_] scheme) + (protocol [_] protocol)) + +(defn make-dummy-request + "Constructs a DummyRequest from an options map. Every key is + optional; missing values fall back to sensible defaults. New + fields added to DummyRequest won't break existing call sites + as long as this constructor keeps its `:or` defaults in sync. + + Recognized keys: + :headers — map of header name → value + :cookies — map of cookie name → value + :method — HTTP method keyword (default :get) + :body-stream — InputStream for the body (used directly) + :body-bytes — bytes or string for the body; wrapped in a + ByteArrayInputStream if :body-stream is not + given + :remote-addr — string (default \"127.0.0.1\") + :server-name — string (default \"test\") + :server-port — long (default 0) + :scheme — keyword (default :http) + :protocol — string (default \"HTTP/1.1\") + :path — string (default \"/test\") + :query — string or nil (default nil) + :ssl-client-cert — X509Certificate or nil (default nil)" + [{:keys [headers cookies method body-stream body-bytes + remote-addr server-name server-port scheme protocol + path query ssl-client-cert] + :or {headers {} cookies {} method :get + body-stream nil + remote-addr "127.0.0.1" server-name "test" server-port 0 + scheme :http protocol "HTTP/1.1" path "/test" query nil + ssl-client-cert nil}}] + (let [body-stream (or body-stream + (when body-bytes + (java.io.ByteArrayInputStream. + (if (string? body-bytes) + (.getBytes ^String body-bytes "UTF-8") + body-bytes))))] + (->DummyRequest headers cookies method body-stream + remote-addr server-name server-port + scheme protocol path query ssl-client-cert))) diff --git a/backend/test/backend_tests/http_management_test.clj b/backend/test/backend_tests/http_management_test.clj index 15413eb5ce..02d2bf5833 100644 --- a/backend/test/backend_tests/http_management_test.clj +++ b/backend/test/backend_tests/http_management_test.clj @@ -78,3 +78,52 @@ (let [subs' (-> response ::yres/body :subscription)] (t/is (= subs' subs)))))) + +;; ---- Shared Key Auth Middleware Tests + +(t/deftest shared-key-auth-middleware + (let [;; The shared-key-auth middleware is private, so we access it via var + middleware-spec @#'mgmt/shared-key-auth + compile-fn (:compile middleware-spec) + make-middleware (compile-fn nil nil) + handler (fn [req] {::yres/status 200}) + configured-key "secret-management-key"] + + ;; Test 1: Request with no x-shared-key header should be rejected (403) + (let [middleware (make-middleware handler configured-key) + response (middleware (th/make-dummy-request {}))] + (t/is (= 403 (::yres/status response)))) + + ;; Test 2: Request with wrong key should be rejected (403) + (let [middleware (make-middleware handler configured-key) + response (middleware (th/make-dummy-request {:headers {"x-shared-key" "wrong-key"}}))] + (t/is (= 403 (::yres/status response)))) + + ;; Test 3: Request with correct key should pass (200) + (let [middleware (make-middleware handler configured-key) + response (middleware (th/make-dummy-request {:headers {"x-shared-key" configured-key}}))] + (t/is (= 200 (::yres/status response)))) + + ;; Test 4: When no key is configured, all requests should be rejected (403) + (let [middleware (make-middleware handler nil) + response (middleware (th/make-dummy-request {:headers {"x-shared-key" "any-key"}}))] + (t/is (= 403 (::yres/status response)))) + + ;; Test 5: Keys differing only in the last character must still be rejected + (let [middleware (make-middleware handler "secret-key-12345") + response1 (middleware (th/make-dummy-request {:headers {"x-shared-key" "secret-key-1234X"}})) + response2 (middleware (th/make-dummy-request {:headers {"x-shared-key" "secret-key-12345"}}))] + (t/is (= 403 (::yres/status response1))) + (t/is (= 200 (::yres/status response2)))) + + ;; Test 6: Empty string in header must be rejected when configured key is non-empty + (let [middleware (make-middleware handler "secret-key") + response (middleware (th/make-dummy-request {:headers {"x-shared-key" ""}}))] + (t/is (= 403 (::yres/status response)))) + + ;; Test 7: Empty string as configured key (truthy but empty) must reject all requests + (let [middleware (make-middleware handler "") + response1 (middleware (th/make-dummy-request {:headers {"x-shared-key" "any-key"}})) + response2 (middleware (th/make-dummy-request {:headers {"x-shared-key" ""}}))] + (t/is (= 403 (::yres/status response1))) + (t/is (= 200 (::yres/status response2)))))) diff --git a/backend/test/backend_tests/http_middleware_test.clj b/backend/test/backend_tests/http_middleware_test.clj index e5f3f48863..c89a3a0369 100644 --- a/backend/test/backend_tests/http_middleware_test.clj +++ b/backend/test/backend_tests/http_middleware_test.clj @@ -30,78 +30,17 @@ (t/use-fixtures :once th/state-init) (t/use-fixtures :each th/database-reset) -(defrecord DummyRequest [headers cookies method body-stream - remote-addr server-name server-port - scheme protocol path query ssl-client-cert] - yreq/IRequestCookies - (get-cookie [_ name] - {:value (get cookies name)}) - - yreq/IRequest - (get-header [_ name] - (get headers name)) - (method [_] method) - (body [_] body-stream) - (path [_] path) - (query [_] query) - (server-port [_] server-port) - (server-name [_] server-name) - (remote-addr [_] remote-addr) - (ssl-client-cert [_] ssl-client-cert) - (scheme [_] scheme) - (protocol [_] protocol)) - -(defn- make-dummy-request - "Constructs a DummyRequest from an options map. Every key is - optional; missing values fall back to sensible defaults. New - fields added to DummyRequest won't break existing call sites - as long as this constructor keeps its `:or` defaults in sync. - - Recognized keys: - :headers — map of header name → value - :cookies — map of cookie name → value - :method — HTTP method keyword (default :get) - :body-stream — InputStream for the body (used directly) - :body-bytes — bytes or string for the body; wrapped in a - ByteArrayInputStream if :body-stream is not - given - :remote-addr — string (default \"127.0.0.1\") - :server-name — string (default \"test\") - :server-port — long (default 0) - :scheme — keyword (default :http) - :protocol — string (default \"HTTP/1.1\") - :path — string (default \"/test\") - :query — string or nil (default nil) - :ssl-client-cert — X509Certificate or nil (default nil)" - [{:keys [headers cookies method body-stream body-bytes - remote-addr server-name server-port scheme protocol - path query ssl-client-cert] - :or {headers {} cookies {} method :get - body-stream nil - remote-addr "127.0.0.1" server-name "test" server-port 0 - scheme :http protocol "HTTP/1.1" path "/test" query nil - ssl-client-cert nil}}] - (let [body-stream (or body-stream - (when body-bytes - (java.io.ByteArrayInputStream. - (if (string? body-bytes) - (.getBytes ^String body-bytes "UTF-8") - body-bytes))))] - (->DummyRequest headers cookies method body-stream - remote-addr server-name server-port - scheme protocol path query ssl-client-cert))) - (t/deftest auth-middleware-1 (let [request (volatile! nil) handler (#'app.http.middleware/wrap-auth (fn [req] (vreset! request req)) {})] - (handler (make-dummy-request {})) + (handler (th/make-dummy-request {})) (t/is (nil? (::http/auth-data @request))) - (handler (make-dummy-request {:headers {"authorization" "Token aaaa"}})) + (handler (th/make-dummy-request {:headers {"authorization" "Token aaaa"}})) (let [{:keys [token claims] token-type :type} (get @request ::http/auth-data)] (t/is (= :token token-type)) @@ -114,10 +53,10 @@ (fn [req] (vreset! request req)) {})] - (handler (make-dummy-request {})) + (handler (th/make-dummy-request {})) (t/is (nil? (::http/auth-data @request))) - (handler (make-dummy-request {:headers {"authorization" "Bearer aaaa"}})) + (handler (th/make-dummy-request {:headers {"authorization" "Bearer aaaa"}})) (let [{:keys [token claims] token-type :type} (get @request ::http/auth-data)] (t/is (= :bearer token-type)) @@ -130,10 +69,10 @@ (fn [req] (vreset! request req)) {})] - (handler (make-dummy-request {})) + (handler (th/make-dummy-request {})) (t/is (nil? (::http/auth-data @request))) - (handler (make-dummy-request {:cookies {"auth-token" "foobar"}})) + (handler (th/make-dummy-request {:cookies {"auth-token" "foobar"}})) (let [{:keys [token claims] token-type :type} (get @request ::http/auth-data)] (t/is (= :cookie token-type)) @@ -145,16 +84,16 @@ (fn [req] {::yres/status 200}) {:test1 "secret-key"})] - (let [response (handler (make-dummy-request {}))] + (let [response (handler (th/make-dummy-request {}))] (t/is (= 403 (::yres/status response)))) - (let [response (handler (make-dummy-request {:headers {"x-shared-key" "secret-key2"}}))] + (let [response (handler (th/make-dummy-request {:headers {"x-shared-key" "secret-key2"}}))] (t/is (= 403 (::yres/status response)))) - (let [response (handler (make-dummy-request {:headers {"x-shared-key" "secret-key"}}))] + (let [response (handler (th/make-dummy-request {:headers {"x-shared-key" "secret-key"}}))] (t/is (= 403 (::yres/status response)))) - (let [response (handler (make-dummy-request {:headers {"x-shared-key" "test1 secret-key"}}))] + (let [response (handler (th/make-dummy-request {:headers {"x-shared-key" "test1 secret-key"}}))] (t/is (= 200 (::yres/status response)))))) (t/deftest access-token-authz @@ -265,7 +204,7 @@ :user-agent "user agent"}) (#'session/assign-token cfg)) - response (handler (make-dummy-request {:cookies {"auth-token" (:token session)}})) + response (handler (th/make-dummy-request {:cookies {"auth-token" (:token session)}})) {:keys [token claims] token-type :type} (get response ::http/auth-data)] @@ -356,7 +295,7 @@ ;; value with a backslash followed by '}', which ;; clojure.data.json v0.5.x cannot handle. body (.getBytes "{\"x\": \"\\}\"}" "UTF-8") - request (make-dummy-request + request (th/make-dummy-request {:method :post :headers {"content-type" "application/json"} :body-bytes body}) @@ -375,7 +314,7 @@ ;; error. (let [handler (#'app.http.middleware/wrap-parse-request (fn [_] (throw (RequestTooBigException. "too large")))) - request (make-dummy-request + request (th/make-dummy-request {:method :post :headers {"content-type" "application/json"} :body-bytes (.getBytes "{}" "UTF-8")}) @@ -393,7 +332,7 @@ ;; should convert it to a 400 :malformed-json validation error. (let [handler (#'app.http.middleware/wrap-parse-request (fn [_] (throw (java.io.EOFException. "stream closed")))) - request (make-dummy-request + request (th/make-dummy-request {:method :post :headers {"content-type" "application/json"} :body-bytes (.getBytes "{}" "UTF-8")}) @@ -416,7 +355,7 @@ (.initCause iae)) handler (#'app.http.middleware/wrap-parse-request (fn [_] (throw wrapped))) - request (make-dummy-request + request (th/make-dummy-request {:method :post :headers {"content-type" "application/json"} :body-bytes (.getBytes "{}" "UTF-8")}) @@ -434,7 +373,7 @@ ;; :unexpected. This is the "true internal error" path. (let [handler (#'app.http.middleware/wrap-parse-request (fn [_] (throw (RuntimeException. "boom")))) - request (make-dummy-request + request (th/make-dummy-request {:method :post :headers {"content-type" "application/json"} :body-bytes (.getBytes "{}" "UTF-8")}) @@ -454,7 +393,7 @@ ;; with :code :io-exception. (let [handler (#'app.http.middleware/wrap-parse-request (fn [_] (throw (java.io.IOException. "network gone")))) - request (make-dummy-request + request (th/make-dummy-request {:method :post :headers {"content-type" "application/json"} :body-bytes (.getBytes "{}" "UTF-8")}) diff --git a/backend/test/backend_tests/rpc_demo_test.clj b/backend/test/backend_tests/rpc_demo_test.clj index b3561ade93..8ca3b417f5 100644 --- a/backend/test/backend_tests/rpc_demo_test.clj +++ b/backend/test/backend_tests/rpc_demo_test.clj @@ -7,8 +7,10 @@ (ns backend-tests.rpc-demo-test (:require [app.auth :as auth] + [app.common.time :as ct] [app.config :as cf] [app.rpc.commands.profile :as profile] + [app.worker :as wrk] [backend-tests.helpers :as th] [clojure.test :as t])) @@ -74,3 +76,48 @@ :skip-onboarding "yes"})] (t/is (th/ex-of-type? error :validation)) (t/is (th/ex-of-code? error :params-validation))))) + +(t/deftest create-demo-profile-uses-global-delay-by-default + (with-redefs [cf/flags (conj cf/flags :demo-users)] + (let [captured (atom nil)] + (with-redefs [wrk/submit! (fn [& {:keys [::wrk/task ::wrk/delay]}] + (reset! captured {:task task :delay delay}))] + (let [{:keys [error result]} (th/command! {::th/type :create-demo-profile})] + (t/is (nil? error)) + (t/is (some? (:email result))) + (t/is (= :demo-purge (:task @captured))) + (t/is (= (cf/get-deletion-delay) (:delay @captured)))))))) + +(t/deftest create-demo-profile-accepts-short-expires-in + (with-redefs [cf/flags (conj cf/flags :demo-users)] + (let [captured (atom nil)] + (with-redefs [wrk/submit! (fn [& {:keys [::wrk/task ::wrk/delay]}] + (reset! captured {:task task :delay delay}))] + (let [{:keys [error result]} (th/command! {::th/type :create-demo-profile + :expires-in "10m"})] + (t/is (nil? error)) + (t/is (some? (:email result))) + (t/is (= :demo-purge (:task @captured))) + (t/is (= (ct/duration "10m") (:delay @captured)))))))) + +(t/deftest create-demo-profile-rejects-expires-in-below-minimum + (with-redefs [cf/flags (conj cf/flags :demo-users)] + (let [{:keys [error]} (th/command! {::th/type :create-demo-profile + :expires-in "1m"})] + (t/is (th/ex-of-type? error :validation)) + (t/is (th/ex-of-code? error :invalid-expires-in))))) + +(t/deftest create-demo-profile-rejects-expires-in-above-global-delay + (with-redefs [cf/flags (conj cf/flags :demo-users) + cf/get-deletion-delay (fn [] (ct/duration {:days 7}))] + (let [{:keys [error]} (th/command! {::th/type :create-demo-profile + :expires-in "200h"})] + (t/is (th/ex-of-type? error :validation)) + (t/is (th/ex-of-code? error :invalid-expires-in))))) + +(t/deftest create-demo-profile-rejects-non-duration-expires-in + (with-redefs [cf/flags (conj cf/flags :demo-users)] + (let [{:keys [error]} (th/command! {::th/type :create-demo-profile + :expires-in "yes"})] + (t/is (th/ex-of-type? error :validation)) + (t/is (th/ex-of-code? error :params-validation))))) diff --git a/backend/test/backend_tests/tasks_telemetry_test.clj b/backend/test/backend_tests/tasks_telemetry_test.clj index a44f6efe74..6c02cd9b45 100644 --- a/backend/test/backend_tests/tasks_telemetry_test.clj +++ b/backend/test/backend_tests/tasks_telemetry_test.clj @@ -135,6 +135,58 @@ (th/run-task! :telemetry {:send? false :enabled? true}) (t/is (not (:called? @mock)))))) +(t/deftest test-telemetry-excluded-host-predicate + (t/is (true? (cf/telemetry-excluded-host? "penpot.app"))) + (t/is (true? (cf/telemetry-excluded-host? "penpot.dev"))) + (t/is (true? (cf/telemetry-excluded-host? "design.penpot.app"))) + (t/is (true? (cf/telemetry-excluded-host? "design.penpot.dev"))) + (t/is (true? (cf/telemetry-excluded-host? "DESIGN.PENPOT.APP"))) + (t/is (false? (cf/telemetry-excluded-host? "localhost"))) + (t/is (false? (cf/telemetry-excluded-host? "example.com"))) + (t/is (false? (cf/telemetry-excluded-host? "mypenpot.app.example.com"))) + (t/is (false? (cf/telemetry-excluded-host? nil))) + (t/is (false? (cf/telemetry-excluded-host? "")))) + +(t/deftest test-telemetry-disabled-on-official-host-newsletter-only + ;; The limited newsletter report must not be sent from official + ;; instances, even when subscriptions exist. + (doseq [[idx public-uri] (map-indexed vector ["https://design.penpot.app" + "https://penpot.app" + "https://design.penpot.dev" + "https://penpot.dev"])] + (with-mocks [mock {:target 'app.tasks.telemetry/make-legacy-request + :return nil}] + (with-redefs [cf/flags #{} + cf/config (assoc cf/config :public-uri public-uri)] + (th/create-profile* (+ 10 idx) {:is-active true + :props {:newsletter-updates true}}) + (th/run-task! :telemetry {:send? true}) + (t/is (not (:called? @mock)) (str "newsletter report must not send for " public-uri)))))) + +(t/deftest test-telemetry-excluded-skips-subscriptions-query + ;; On official hosts the subscriptions query must not even run, + ;; since nothing is going to be sent. + (with-mocks [mock {:target 'app.tasks.telemetry/get-subscriptions + :return []}] + (with-redefs [cf/flags #{} + cf/config (assoc cf/config :public-uri "https://design.penpot.app")] + (th/create-profile* 1 {:is-active true + :props {:newsletter-updates true}}) + (th/run-task! :telemetry {:send? true}) + (t/is (not (:called? @mock)))))) + +(t/deftest test-telemetry-enabled-still-sends-on-official-host + ;; An explicitly enabled telemetry still reports on official hosts; + ;; only the implicit newsletter fallback is excluded. + (with-mocks [mock {:target 'app.tasks.telemetry/make-legacy-request + :return nil}] + (with-redefs [cf/flags #{:telemetry} + cf/config (assoc cf/config :public-uri "https://design.penpot.app")] + (th/create-profile* 1 {:is-active true + :props {:newsletter-updates true}}) + (th/run-task! :telemetry {:send? true :enabled? true}) + (t/is (:called? @mock))))) + ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; AUDIT-EVENT BATCH COLLECTION TESTS ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; diff --git a/common/src/app/common/time.cljc b/common/src/app/common/time.cljc index 080d609761..35ae519809 100644 --- a/common/src/app/common/time.cljc +++ b/common/src/app/common/time.cljc @@ -175,8 +175,14 @@ #?(:clj (defn parse-duration + "Parse a value into a Duration. Total: returns the input unchanged + when it cannot be parsed, so schema decoding never throws and + invalid values fail validation with a clean params error instead." [s] - (duration s))) + (try + (duration s) + (catch Exception _ + s)))) #?(:clj (defn format-duration diff --git a/common/src/app/common/types/shape/interactions.cljc b/common/src/app/common/types/shape/interactions.cljc index f3e7912717..38db81323d 100644 --- a/common/src/app/common/types/shape/interactions.cljc +++ b/common/src/app/common/types/shape/interactions.cljc @@ -717,14 +717,21 @@ (conj (or interactions []) interaction)) (defn remove-interaction + "Interactions without the one at `index`; unchanged when `index` addresses none." [interactions index] (let [interactions (or interactions [])] - (into (subvec interactions 0 index) - (subvec interactions (inc index))))) + (if (and (int? index) (< -1 index (count interactions))) + (into (subvec interactions 0 index) + (subvec interactions (inc index))) + interactions))) (defn update-interaction + "Interactions with `update-fn` applied at `index`; unchanged when `index` + addresses none." [interactions index update-fn] - (update interactions index update-fn)) + (if (and (int? index) (< -1 index (count interactions))) + (update interactions index update-fn) + interactions)) (defn remap-interactions "Update all interactions whose destination points to a shape in the diff --git a/common/test/common_tests/time_test.cljc b/common/test/common_tests/time_test.cljc index a80ed8f16b..9b2c73a82b 100644 --- a/common/test/common_tests/time_test.cljc +++ b/common/test/common_tests/time_test.cljc @@ -14,3 +14,17 @@ dtb (dt/inst 20000)] (t/is (false? (dt/is-after? dta dtb))) (t/is (true? (dt/is-before? dta dtb))))) + +#?(:clj + (t/deftest parse-duration-test + (t/is (dt/duration? (dt/parse-duration "10m"))) + (t/is (= (dt/duration "10m") (dt/parse-duration "10m"))) + (t/is (= (dt/duration "1h") (dt/parse-duration "1h"))) + + ;; Invalid values are returned unchanged instead of throwing, so + ;; they fail the `duration?` schema predicate with a clean + ;; validation error downstream. + (t/is (= "yes" (dt/parse-duration "yes"))) + (t/is (not (dt/duration? (dt/parse-duration "yes")))) + (t/is (= true (dt/parse-duration true))) + (t/is (not (dt/duration? (dt/parse-duration true)))))) diff --git a/common/test/common_tests/types/shape_interactions_test.cljc b/common/test/common_tests/types/shape_interactions_test.cljc index 6dbf7115c0..a310c6bb5a 100644 --- a/common/test/common_tests/types/shape_interactions_test.cljc +++ b/common/test/common_tests/types/shape_interactions_test.cljc @@ -859,7 +859,20 @@ (t/testing "Update interaction" (let [new-interactions (ctsi/update-interaction interactions 1 #(ctsi/set-action-type % :open-url))] (t/is (= (count new-interactions) 2)) - (t/is (= (:action-type (last new-interactions)) :open-url)))))) + (t/is (= (:action-type (last new-interactions)) :open-url)))) + + (t/testing "Remove interaction with an index out of range" + (t/is (= interactions (ctsi/remove-interaction interactions 2))) + (t/is (= interactions (ctsi/remove-interaction interactions -1))) + (t/is (= interactions (ctsi/remove-interaction interactions nil))) + (t/is (= [] (ctsi/remove-interaction nil 0)))) + + (t/testing "Update interaction with an index out of range" + (let [update-fn #(ctsi/set-action-type % :open-url)] + (t/is (= interactions (ctsi/update-interaction interactions 2 update-fn))) + (t/is (= interactions (ctsi/update-interaction interactions -1 update-fn))) + (t/is (= interactions (ctsi/update-interaction interactions nil update-fn))) + (t/is (nil? (ctsi/update-interaction nil 0 update-fn))))))) (t/deftest remap-interactions diff --git a/frontend/playwright/data/workspace/get-file-text-multiple-selection.json b/frontend/playwright/data/workspace/get-file-text-multiple-selection.json new file mode 100644 index 0000000000..74fe4f69d8 --- /dev/null +++ b/frontend/playwright/data/workspace/get-file-text-multiple-selection.json @@ -0,0 +1,484 @@ +{ + "~:features": { + "~#set": [ + "fdata/path-data", + "plugins/runtime", + "design-tokens/v1", + "variants/v1", + "layout/grid", + "styles/v2", + "fdata/pointer-map", + "fdata/objects-map", + "render-wasm/v1", + "components/v2", + "fdata/shape-data-type" + ] + }, + "~:team-id": "~u04868522-3ebf-81e8-8006-306b0c9b5f59", + "~:permissions": { + "~:type": "~:membership", + "~:is-owner": true, + "~:is-admin": true, + "~:can-edit": true, + "~:can-read": true, + "~:is-logged": true + }, + "~:has-media-trimmed": false, + "~:comment-thread-seqn": 0, + "~:name": "Text: Custom Fonts", + "~:revn": 13, + "~:modified-at": "~m1750151641034", + "~:vern": 0, + "~:id": "~u434b0541-fa2f-802f-8006-6a827d964a9b", + "~:is-shared": false, + "~:migrations": { + "~#ordered-set": [ + "legacy-2", + "legacy-3", + "legacy-5", + "legacy-6", + "legacy-7", + "legacy-8", + "legacy-9", + "legacy-10", + "legacy-11", + "legacy-12", + "legacy-13", + "legacy-14", + "legacy-16", + "legacy-17", + "legacy-18", + "legacy-19", + "legacy-25", + "legacy-26", + "legacy-27", + "legacy-28", + "legacy-29", + "legacy-31", + "legacy-32", + "legacy-33", + "legacy-34", + "legacy-36", + "legacy-37", + "legacy-38", + "legacy-39", + "legacy-40", + "legacy-41", + "legacy-42", + "legacy-43", + "legacy-44", + "legacy-45", + "legacy-46", + "legacy-47", + "legacy-48", + "legacy-49", + "legacy-50", + "legacy-51", + "legacy-52", + "legacy-53", + "legacy-54", + "legacy-55", + "legacy-56", + "legacy-57", + "legacy-59", + "legacy-62", + "legacy-65", + "legacy-66", + "legacy-67", + "0001-remove-tokens-from-groups", + "0002-normalize-bool-content", + "0002-clean-shape-interactions", + "0003-fix-root-shape", + "0003-convert-path-content", + "0004-clean-shadow-and-colors", + "0005-deprecate-image-type", + "0006-fix-old-texts-fills", + "0007-clear-invalid-strokes-and-fills-v2", + "0008-fix-library-colors-opacity", + "0009-add-partial-text-touched-flags" + ] + }, + "~:version": 67, + "~:project-id": "~u53a7ff09-2228-81d3-8006-4b5ea964593b", + "~:created-at": "~m1750081311326", + "~:data": { + "~:pages": ["~u434b0541-fa2f-802f-8006-6a827d964a9c"], + "~:pages-index": { + "~u434b0541-fa2f-802f-8006-6a827d964a9c": { + "~:objects": { + "~u00000000-0000-0000-0000-000000000000": { + "~#shape": { + "~:y": 0, + "~:hide-fill-on-export": false, + "~:transform": { + "~#matrix": { + "~:a": 1.0, + "~:b": 0.0, + "~:c": 0.0, + "~:d": 1.0, + "~:e": 0.0, + "~:f": 0.0 + } + }, + "~:rotation": 0, + "~:name": "Root Frame", + "~:width": 0.01, + "~:type": "~:frame", + "~:points": [ + { + "~#point": { + "~:x": 0.0, + "~:y": 0.0 + } + }, + { + "~#point": { + "~:x": 0.01, + "~:y": 0.0 + } + }, + { + "~#point": { + "~:x": 0.01, + "~:y": 0.01 + } + }, + { + "~#point": { + "~:x": 0.0, + "~:y": 0.01 + } + } + ], + "~:r2": 0, + "~:proportion-lock": false, + "~:transform-inverse": { + "~#matrix": { + "~:a": 1.0, + "~:b": 0.0, + "~:c": 0.0, + "~:d": 1.0, + "~:e": 0.0, + "~:f": 0.0 + } + }, + "~:r3": 0, + "~:r1": 0, + "~:id": "~u00000000-0000-0000-0000-000000000000", + "~:parent-id": "~u00000000-0000-0000-0000-000000000000", + "~:frame-id": "~u00000000-0000-0000-0000-000000000000", + "~:strokes": [], + "~:x": 0, + "~:proportion": 1.0, + "~:r4": 0, + "~:selrect": { + "~#rect": { + "~:x": 0, + "~:y": 0, + "~:width": 0.01, + "~:height": 0.01, + "~:x1": 0, + "~:y1": 0, + "~:x2": 0.01, + "~:y2": 0.01 + } + }, + "~:fills": [ + { + "~:fill-color": "#FFFFFF", + "~:fill-opacity": 1 + } + ], + "~:flip-x": null, + "~:height": 0.01, + "~:flip-y": null, + "~:shapes": [ + "~u7d85a63e-18e7-809f-8006-6a827fe8501e", + "~u7d85a63e-18e7-809f-8006-6a833ef5fcef" + ] + } + }, + "~u7d85a63e-18e7-809f-8006-6a827fe8501e": { + "~#shape": { + "~:y": 451.9999962296588, + "~:transform": { + "~#matrix": { + "~:a": 1.0, + "~:b": 0.0, + "~:c": 0.0, + "~:d": 1.0, + "~:e": 0.0, + "~:f": 0.0 + } + }, + "~:rotation": 0, + "~:grow-type": "~:auto-width", + "~:content": { + "~:type": "root", + "~:key": "xgmgu1frox", + "~:children": [ + { + "~:type": "paragraph-set", + "~:children": [ + { + "~:line-height": "1.2", + "~:font-style": "normal", + "~:children": [ + { + "~:line-height": "", + "~:font-style": "normal", + "~:typography-ref-id": null, + "~:text-transform": "none", + "~:font-id": "gfont-rufina", + "~:key": "ee7vl7klqs", + "~:font-size": "72", + "~:font-weight": "400", + "~:typography-ref-file": null, + "~:font-variant-id": "normal-400", + "~:text-decoration": "none", + "~:letter-spacing": "0", + "~:fills": [ + { + "~:fill-color": "#000000", + "~:fill-opacity": 1 + } + ], + "~:font-family": "\"Rufina\"", + "~:text": "Text multiple selection one" + } + ], + "~:typography-ref-id": null, + "~:text-transform": "none", + "~:text-align": "center", + "~:font-id": "gfont-rufina", + "~:key": "17bt2f4evfs", + "~:font-size": "72", + "~:font-weight": "400", + "~:typography-ref-file": null, + "~:text-direction": "ltr", + "~:type": "paragraph", + "~:font-variant-id": "normal-400", + "~:text-decoration": "none", + "~:letter-spacing": "0", + "~:fills": [ + { + "~:fill-color": "#000000", + "~:fill-opacity": 1 + } + ], + "~:font-family": "\"Rufina\"" + } + ] + } + ], + "~:vertical-align": "top" + }, + "~:hide-in-viewer": false, + "~:name": "Text multiple selection one", + "~:width": 403.99995992417394, + "~:type": "~:text", + "~:points": [ + { + "~#point": { + "~:x": 744.0000211580308, + "~:y": 451.9999962296588 + } + }, + { + "~#point": { + "~:x": 1147.9999810822046, + "~:y": 451.9999962296588 + } + }, + { + "~#point": { + "~:x": 1147.9999810822046, + "~:y": 537.9999971833331 + } + }, + { + "~#point": { + "~:x": 744.0000211580308, + "~:y": 537.9999971833331 + } + } + ], + "~:transform-inverse": { + "~#matrix": { + "~:a": 1.0, + "~:b": 0.0, + "~:c": 0.0, + "~:d": 1.0, + "~:e": 0.0, + "~:f": 0.0 + } + }, + "~:id": "~u7d85a63e-18e7-809f-8006-6a827fe8501e", + "~:parent-id": "~u00000000-0000-0000-0000-000000000000", + "~:frame-id": "~u00000000-0000-0000-0000-000000000000", + "~:x": 744.0000211580307, + "~:selrect": { + "~#rect": { + "~:x": 744.0000211580307, + "~:y": 451.9999962296588, + "~:width": 403.99995992417394, + "~:height": 86.00000095367432, + "~:x1": 744.0000211580307, + "~:y1": 451.9999962296588, + "~:x2": 1147.9999810822046, + "~:y2": 537.9999971833331 + } + }, + "~:flip-x": null, + "~:height": 86.00000095367432, + "~:flip-y": null + } + }, + "~u7d85a63e-18e7-809f-8006-6a833ef5fcef": { + "~#shape": { + "~:y": 537.9999971833331, + "~:transform": { + "~#matrix": { + "~:a": 1.0, + "~:b": 0.0, + "~:c": 0.0, + "~:d": 1.0, + "~:e": 0.0, + "~:f": 0.0 + } + }, + "~:rotation": 0, + "~:grow-type": "~:auto-width", + "~:content": { + "~:type": "root", + "~:key": "xgmgu1frox", + "~:children": [ + { + "~:type": "paragraph-set", + "~:children": [ + { + "~:line-height": "1.2", + "~:font-style": "normal", + "~:children": [ + { + "~:line-height": "", + "~:font-style": "normal", + "~:typography-ref-id": null, + "~:text-transform": "none", + "~:font-id": "gfont-rufina", + "~:key": "ee7vl7klqs", + "~:font-size": "36", + "~:font-weight": "500", + "~:typography-ref-file": null, + "~:font-variant-id": "normal-500", + "~:text-decoration": "none", + "~:letter-spacing": "0", + "~:fills": [ + { + "~:fill-color": "#000000", + "~:fill-opacity": 1 + } + ], + "~:font-family": "\"Rufina\"", + "~:text": "Second text, same font" + } + ], + "~:typography-ref-id": null, + "~:text-transform": "none", + "~:text-align": "center", + "~:font-id": "gfont-rufina", + "~:key": "17bt2f4evfs", + "~:font-size": "0", + "~:font-weight": "500", + "~:typography-ref-file": null, + "~:text-direction": "ltr", + "~:type": "paragraph", + "~:font-variant-id": "normal-500", + "~:text-decoration": "none", + "~:letter-spacing": "0", + "~:fills": [ + { + "~:fill-color": "#000000", + "~:fill-opacity": 1 + } + ], + "~:font-family": "\"Rufina\"" + } + ] + } + ], + "~:vertical-align": "top" + }, + "~:hide-in-viewer": false, + "~:name": "Text multiple selection two", + "~:width": 466.0000131576671, + "~:type": "~:text", + "~:points": [ + { + "~#point": { + "~:x": 712.9999941849438, + "~:y": 537.9999971833331 + } + }, + { + "~#point": { + "~:x": 1179.0000073426108, + "~:y": 537.9999971833331 + } + }, + { + "~#point": { + "~:x": 1179.0000073426108, + "~:y": 580.9999976601703 + } + }, + { + "~#point": { + "~:x": 712.9999941849438, + "~:y": 580.9999976601703 + } + } + ], + "~:transform-inverse": { + "~#matrix": { + "~:a": 1.0, + "~:b": 0.0, + "~:c": 0.0, + "~:d": 1.0, + "~:e": 0.0, + "~:f": 0.0 + } + }, + "~:id": "~u7d85a63e-18e7-809f-8006-6a833ef5fcef", + "~:parent-id": "~u00000000-0000-0000-0000-000000000000", + "~:frame-id": "~u00000000-0000-0000-0000-000000000000", + "~:x": 712.9999941849437, + "~:selrect": { + "~#rect": { + "~:x": 712.9999941849437, + "~:y": 537.9999971833331, + "~:width": 466.0000131576671, + "~:height": 43.00000047683716, + "~:x1": 712.9999941849437, + "~:y1": 537.9999971833331, + "~:x2": 1179.0000073426108, + "~:y2": 580.9999976601703 + } + }, + "~:flip-x": null, + "~:height": 43.00000047683716, + "~:flip-y": null + } + } + }, + "~:id": "~u434b0541-fa2f-802f-8006-6a827d964a9c", + "~:name": "Page 1" + } + }, + "~:id": "~u434b0541-fa2f-802f-8006-6a827d964a9b", + "~:options": { + "~:components-v2": true, + "~:base-font-size": "16px" + } + } +} diff --git a/frontend/playwright/ui/specs/text-options-missing-font.spec.js b/frontend/playwright/ui/specs/text-options-missing-font.spec.js new file mode 100644 index 0000000000..dfac536d89 --- /dev/null +++ b/frontend/playwright/ui/specs/text-options-missing-font.spec.js @@ -0,0 +1,201 @@ +import { test, expect } from "@playwright/test"; +import { WorkspacePage } from "../pages/WorkspacePage"; +import { WasmWorkspacePage } from "../pages/WasmWorkspacePage"; + +// --------------------------------------------------------------------------- +// The "Create typography style" button (workspace.options.convert-to-typography) +// in the text options sidebar is only shown when ALL of these hold for the +// selected text shape(s) (src/app/main/ui/workspace/sidebar/options/menus/text.cljs): +// (and (some? font) (not typography) (not multiple?) (not applied-token-name)) +// Each test below isolates one condition that must independently hide it: +// - font missing (font-id not registered in app.main.fonts/fontsdb) +// - a typography asset is applied (typography-ref-id set) +// - multiple shapes are selected with differing attributes +// - a typography design token is applied (applied-tokens :typography) +// --------------------------------------------------------------------------- + +function convertToTypographyButton(workspace) { + return workspace.rightSidebar.getByRole("button", { + name: "Create typography style", + }); +} + +test.describe("font missing", () => { + // Fixture render-wasm/get-file-text-custom-fonts.json has a text shape + // ("Penpot & Dragons") using a custom team font-id and no typography/token + // applied - otherwise exactly the state that reveals the button once its + // font resolves. Toggling the get-font-variants mock between "the team owns + // this font" and "empty" simulates the font being present vs. missing. + const FILE = { + id: "434b0541-fa2f-802f-8006-59827d964a9b", + pageId: "434b0541-fa2f-802f-8006-59827d964a9c", + }; + + test.beforeEach(async ({ page }) => { + await WorkspacePage.init(page); + }); + + test("Create typography style button is hidden when the shape font is missing", async ({ + page, + }) => { + const workspace = new WorkspacePage(page); + await workspace.setupEmptyFile(); + await workspace.mockRPC( + /get\-file\?/, + "render-wasm/get-file-text-custom-fonts.json", + ); + // The team does not own the shape's custom font, so it can't be resolved. + await workspace.mockRPC( + "get-font-variants?team-id=*", + "workspace/get-font-variants-empty.json", + ); + await workspace.goToWorkspace({ fileId: FILE.id, pageId: FILE.pageId }); + + await workspace.clickLeafLayer("Penpot & Dragons"); + + await expect(convertToTypographyButton(workspace)).not.toBeVisible(); + }); + + test("Create typography style button is visible once the shape font resolves", async ({ + page, + }) => { + const workspace = new WorkspacePage(page); + await workspace.setupEmptyFile(); + await workspace.mockRPC( + /get\-file\?/, + "render-wasm/get-file-text-custom-fonts.json", + ); + // The team owns the shape's custom font, so it resolves normally. + await workspace.mockRPC( + "get-font-variants?team-id=*", + "render-wasm/get-font-variants-custom-fonts.json", + ); + await workspace.goToWorkspace({ fileId: FILE.id, pageId: FILE.pageId }); + + await workspace.clickLeafLayer("Penpot & Dragons"); + + await expect(convertToTypographyButton(workspace)).toBeVisible(); + }); +}); + +test.describe("typography asset applied", () => { + // multiselection-typography.json: "Text with typography asset one" has a + // typography-ref-id pointing at an in-file typography asset (font + // gfont-agdasima, a built-in Google font that resolves with no extra + // mocking), and is not multi-selected or token-applied. + const FILE = { + id: "1062e0a0-8fe0-80ae-8007-e70b4993f5ef", + pageId: "1062e0a0-8fe0-80ae-8007-e70b4993f5f0", + }; + + test.beforeEach(async ({ page }) => { + await WorkspacePage.init(page); + }); + + test("Create typography style button is hidden when a typography asset is applied", async ({ + page, + }) => { + const workspace = new WorkspacePage(page); + await workspace.setupEmptyFile(); + await workspace.mockRPC( + /get\-file\?/, + "workspace/multiselection-typography.json", + ); + await workspace.goToWorkspace({ fileId: FILE.id, pageId: FILE.pageId }); + + await workspace.clickLeafLayer("Text with typography asset one"); + + // Sanity check: the text options panel did render for this shape - the + // button is specifically hidden by the applied typography, not because + // the whole panel failed to show up. + await expect( + workspace.rightSidebar.getByRole("region", { name: "Text section" }), + ).toBeVisible(); + await expect(convertToTypographyButton(workspace)).not.toBeVisible(); + }); +}); + +test.describe("multiple selection", () => { + // get-file-text-multiple-selection.json has two text shapes sharing the + // same (resolvable, built-in) font-id but differing font-size, with no + // typography or token applied - so selecting both together isolates + // `multiple?` becoming true without also making the font unresolved. + const FILE = { + id: "434b0541-fa2f-802f-8006-6a827d964a9b", + pageId: "434b0541-fa2f-802f-8006-6a827d964a9c", + }; + + test.beforeEach(async ({ page }) => { + await WorkspacePage.init(page); + }); + + test("Create typography style button is hidden when multiple shapes with different values are selected", async ({ + page, + }) => { + const workspace = new WorkspacePage(page); + await workspace.setupEmptyFile(); + await workspace.mockRPC( + /get\-file\?/, + "workspace/get-file-text-multiple-selection.json", + ); + await workspace.goToWorkspace({ fileId: FILE.id, pageId: FILE.pageId }); + + await workspace.clickLeafLayer("Text multiple selection one"); + await expect(convertToTypographyButton(workspace)).toBeVisible(); + + await workspace.clickLeafLayer("Text multiple selection two", { + modifiers: ["Shift"], + }); + + await expect(convertToTypographyButton(workspace)).not.toBeVisible(); + }); +}); + +test.describe("typography token applied", () => { + // get-file-token-tooltip.json: "Text with token" has a typography design + // token applied (applied-tokens :typography) using font gfont-arizonia (a + // built-in Google font that resolves with no extra mocking). + test.beforeEach(async ({ page }) => { + await WasmWorkspacePage.init(page); + await WasmWorkspacePage.mockRPC(page, "get-teams", "get-teams-tokens.json"); + }); + + test("Create typography style button is hidden when a typography token is applied", async ({ + page, + }) => { + const workspace = new WasmWorkspacePage(page); + await workspace.mockConfigFlags(["enable-feature-token-input"]); + await workspace.setupEmptyFile(); + await workspace.mockRPC("get-team?id=*", "workspace/get-team-tokens.json"); + await workspace.mockRPC( + /get\-file\?/, + "workspace/get-file-token-tooltip.json", + ); + await workspace.mockRPC( + /get\-file\-fragment\?/, + "workspace/get-file-fragment-tokens.json", + ); + await workspace.mockRPC( + "update-file?id=*", + "workspace/update-file-create-rect.json", + ); + await workspace.goToWorkspace({ + fileId: "c7ce0794-0992-8105-8004-38f280443849", + pageId: "4530574a-7a0a-807b-8008-0107b2c4628e", + }); + + await page.getByRole("tab", { name: "Layers" }).click(); + await workspace.layers + .getByTestId("layer-row") + .filter({ hasText: "Text with token" }) + .click(); + + // Sanity check: the text options panel did render for this shape - the + // button is specifically hidden by the applied token, not because the + // whole panel failed to show up. + await expect( + workspace.rightSidebar.getByRole("region", { name: "Text section" }), + ).toBeVisible(); + await expect(convertToTypographyButton(workspace)).not.toBeVisible(); + }); +}); diff --git a/frontend/src/app/main/data/comments.cljs b/frontend/src/app/main/data/comments.cljs index 2b28a07acb..90e83ff4de 100644 --- a/frontend/src/app/main/data/comments.cljs +++ b/frontend/src/app/main/data/comments.cljs @@ -442,7 +442,6 @@ (rx/catch #(rx/throw {:type :comment-error})))))))) -;; FIXME: revisit (defn retrieve-unread-comment-threads "A event used mainly in dashboard for retrieve all unread threads of a team." [team-id] @@ -450,18 +449,9 @@ (ptk/reify ::retrieve-unread-comment-threads ptk/WatchEvent (watch [_ _ _] - (let [fetched-comments #(assoc %2 :comment-threads (d/index-by :id %1)) - fetched-users #(assoc %2 :current-team-comments-users %1)] + (let [fetched-comments #(assoc %2 :comment-threads (d/index-by :id %1))] (->> (rp/cmd! :get-unread-comment-threads {:team-id team-id}) - (rx/merge-map - (fn [comments] - (rx/concat - (rx/of (partial fetched-comments comments)) - - (->> (rx/from (into #{} (map :file-id) comments)) - (rx/merge-map #(rp/cmd! :get-profiles-for-file-comments {:file-id %})) - (rx/reduce #(merge %1 (d/index-by :id %2)) {}) - (rx/map #(partial fetched-users %)))))) + (rx/map #(partial fetched-comments %)) (rx/catch #(rx/throw {:type :comment-error}))))))) (defn mark-all-threads-as-read diff --git a/frontend/src/app/main/data/profile.cljs b/frontend/src/app/main/data/profile.cljs index f0814a2098..f2cae4b2bc 100644 --- a/frontend/src/app/main/data/profile.cljs +++ b/frontend/src/app/main/data/profile.cljs @@ -377,20 +377,6 @@ (js/console.error "delete-photo failed" cause) (rx/of (refresh-profile)))))))) -(defn fetch-file-comments-users - [{:keys [team-id]}] - (assert (uuid? team-id) "expected a valid uuid for `team-id`") - (letfn [(fetched [users state] - (->> users - (d/index-by :id) - (assoc state :file-comments-users)))] - (ptk/reify ::fetch-file-comments-users - ptk/WatchEvent - (watch [_ state _] - (let [share-id (-> state :viewer-local :share-id)] - (->> (rp/cmd! :get-profiles-for-file-comments {:team-id team-id :share-id share-id}) - (rx/map #(partial fetched %)))))))) - ;; --- EVENT: request-account-deletion (def profile-deleted-event? diff --git a/frontend/src/app/main/data/viewer.cljs b/frontend/src/app/main/data/viewer.cljs index 7e6de36796..40b99474ea 100644 --- a/frontend/src/app/main/data/viewer.cljs +++ b/frontend/src/app/main/data/viewer.cljs @@ -44,8 +44,7 @@ :selected #{} :collapsed #{} :hover nil - :share-id "" - :file-comments-users []}) + :share-id ""}) (declare fetch-comment-threads) (declare fetch-bundle) diff --git a/frontend/src/app/main/refs.cljs b/frontend/src/app/main/refs.cljs index 064e7cd4e8..6d202d339a 100644 --- a/frontend/src/app/main/refs.cljs +++ b/frontend/src/app/main/refs.cljs @@ -19,6 +19,7 @@ [app.main.store :as st] [app.main.streams :as ms] [beicon.v2.core :as rx] + [clojure.string :as str] [okulary.core :as l])) ;; ---- Global refs @@ -586,13 +587,21 @@ (dm/get-in state [:viewer-local :zoom-type])) st/state)) +(defn- resolved-uri? + "Returns true if the uri is already a fully resolved URI (blob or data)." + [uri] + (or (str/starts-with? uri "blob:") + (str/starts-with? uri "data:"))) + (defn workspace-thumbnail-by-id [object-id] (l/derived (fn [state] (when-let [entry (dm/get-in state [:thumbnails object-id])] (cond-> entry - (:uri entry) (update :uri cf/resolve-media)))) + (and (:uri entry) + (not (resolved-uri? (:uri entry)))) + (update :uri cf/resolve-media)))) st/state)) (def workspace-text-modifier diff --git a/frontend/src/app/main/ui/workspace/sidebar/options/menus/text.cljs b/frontend/src/app/main/ui/workspace/sidebar/options/menus/text.cljs index 159d082478..4ec36226e7 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/options/menus/text.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/options/menus/text.cljs @@ -20,6 +20,7 @@ [app.main.data.workspace.undo :as dwu] [app.main.data.workspace.wasm-text :as dwwt] [app.main.features :as features] + [app.main.fonts :as fonts] [app.main.refs :as refs] [app.main.store :as st] [app.main.ui.components.title-bar :refer [title-bar*]] @@ -307,6 +308,11 @@ main-menu-open? (:main-menu menu-state) more-options-open? (:more-options menu-state) + font-id (or (:font-id values) (:font-id txt/default-typography)) + + fonts (mf/deref fonts/fontsdb) + font (get fonts font-id) + token-dropdown-open* (mf/use-state false) token-dropdown-open? (deref token-dropdown-open*) @@ -514,7 +520,7 @@ :on-click toggle-token-dropdown :tooltip-placement "top-left" :icon i/tokens}]) - (when (and (not typography) (not multiple?) (not applied-token-name)) + (when (and (some? font) (not typography) (not multiple?) (not applied-token-name)) [:> icon-button* {:variant "ghost" :aria-label (tr "workspace.options.convert-to-typography") :on-click on-convert-to-typography diff --git a/frontend/src/app/plugins/shape.cljs b/frontend/src/app/plugins/shape.cljs index b7a36bdc7d..d42c0529a3 100644 --- a/frontend/src/app/plugins/shape.cljs +++ b/frontend/src/app/plugins/shape.cljs @@ -80,89 +80,102 @@ (obj/type-of? p "InteractionProxy")) (defn interaction-proxy - [plugin-id file-id page-id shape-id index] - (obj/reify {:name "InteractionProxy"} - :$plugin {:enumerable false :get (fn [] plugin-id)} - :$file {:enumerable false :get (fn [] file-id)} - :$page {:enumerable false :get (fn [] page-id)} - :$shape {:enumerable false :get (fn [] shape-id)} - :$index {:enumerable false :get (fn [] index)} + "Proxy over one interaction of a shape. - ;; Not enumerable so we don't have an infinite loop - :shape - {:enumerable false - :get (fn [] (shape-proxy plugin-id file-id page-id shape-id))} + Interactions are addressed by position, which shifts as interactions are added + or removed, so the position is resolved on each access from `interaction`, + kept up to date with the writes made through the proxy." + [plugin-id file-id page-id shape-id interaction index] + (let [current (atom interaction) + locate-index (fn [] (u/locate-interaction-index file-id page-id shape-id @current index))] + (obj/reify {:name "InteractionProxy"} + :$plugin {:enumerable false :get (fn [] plugin-id)} + :$file {:enumerable false :get (fn [] file-id)} + :$page {:enumerable false :get (fn [] page-id)} + :$shape {:enumerable false :get (fn [] shape-id)} + :$index {:enumerable false :get locate-index} - :trigger - {:this true - :get #(-> % u/proxy->interaction :event-type format/format-key) - :set - (fn [_ value] - (let [value (parser/parse-keyword value)] + ;; Not enumerable so we don't have an infinite loop + :shape + {:enumerable false + :get (fn [] (shape-proxy plugin-id file-id page-id shape-id))} + + :trigger + {:this true + :get #(-> % u/proxy->interaction :event-type format/format-key) + :set + (fn [_ value] + (let [value (parser/parse-keyword value)] + (cond + (not (contains? ctsi/event-types value)) + (u/not-valid plugin-id :trigger value) + + (not (r/check-permission plugin-id "content:write")) + (u/not-valid plugin-id :trigger "Plugin doesn't have 'content:write' permission") + + :else + (do + (st/emit! (dwi/update-interaction + (u/locate-shape file-id page-id shape-id) + (locate-index) + #(assoc % :event-type value) + {:page-id page-id})) + (swap! current assoc :event-type value)))))} + + :delay + {:this true + :get #(-> % u/proxy->interaction :delay) + :set + (fn [_ value] (cond - (not (contains? ctsi/event-types value)) - (u/not-valid plugin-id :trigger value) + (or (not (sm/valid-safe-int? value)) (neg? value)) + (u/not-valid plugin-id :delay value) (not (r/check-permission plugin-id "content:write")) - (u/not-valid plugin-id :trigger "Plugin doesn't have 'content:write' permission") + (u/not-valid plugin-id :delay "Plugin doesn't have 'content:write' permission") :else - (st/emit! (dwi/update-interaction - (u/locate-shape file-id page-id shape-id) - index - #(assoc % :event-type value) - {:page-id page-id})))))} + (do + (st/emit! (dwi/update-interaction + (u/locate-shape file-id page-id shape-id) + (locate-index) + #(assoc % :delay value) + {:page-id page-id})) + (swap! current assoc :delay value))))} - :delay - {:this true - :get #(-> % u/proxy->interaction :delay) - :set - (fn [_ value] - (cond - (or (not (sm/valid-safe-int? value)) (neg? value)) - (u/not-valid plugin-id :delay value) + :action + {:this true + :get #(-> % u/proxy->interaction (format/format-action plugin-id file-id page-id)) + :set + (fn [self value] + (let [params (parser/parse-action value) + interaction + (-> (u/proxy->interaction self) + (d/patch-object params))] + (cond + (not (sm/validate ctsi/schema:interaction interaction)) + (u/not-valid plugin-id :action interaction) - (not (r/check-permission plugin-id "content:write")) - (u/not-valid plugin-id :delay "Plugin doesn't have 'content:write' permission") + (not (r/check-permission plugin-id "content:write")) + (u/not-valid plugin-id :action "Plugin doesn't have 'content:write' permission") - :else - (st/emit! (dwi/update-interaction - (u/locate-shape file-id page-id shape-id) - index - #(assoc % :delay value) - {:page-id page-id}))))} + :else + (do + (st/emit! (dwi/update-interaction + (u/locate-shape file-id page-id shape-id) + (locate-index) + #(d/patch-object % params) + {:page-id page-id})) + (reset! current interaction)))))} - :action - {:this true - :get #(-> % u/proxy->interaction (format/format-action plugin-id file-id page-id)) - :set - (fn [self value] - (let [params (parser/parse-action value) - interaction - (-> (u/proxy->interaction self) - (d/patch-object params))] - (cond - (not (sm/validate ctsi/schema:interaction interaction)) - (u/not-valid plugin-id :action interaction) + :remove + (fn [] + (cond + (not (r/check-permission plugin-id "content:write")) + (u/not-valid plugin-id :remove "Plugin doesn't have 'content:write' permission") - (not (r/check-permission plugin-id "content:write")) - (u/not-valid plugin-id :action "Plugin doesn't have 'content:write' permission") - - :else - (st/emit! (dwi/update-interaction - (u/locate-shape file-id page-id shape-id) - index - #(d/patch-object % params) - {:page-id page-id})))))} - - :remove - (fn [] - (cond - (not (r/check-permission plugin-id "content:write")) - (u/not-valid plugin-id :remove "Plugin doesn't have 'content:write' permission") - - :else - (st/emit! (dwi/remove-interaction {:id shape-id} index)))))) + :else + (st/emit! (dwi/remove-interaction {:id shape-id} (locate-index)))))))) (def lib-typography-proxy? nil) (def lib-component-proxy nil) @@ -980,8 +993,9 @@ (fn [self] (let [interactions (-> self u/proxy->shape :interactions)] (format/format-array - #(interaction-proxy plugin-id file-id page-id id %) - (range 0 (count interactions)))))} + (fn [[index interaction]] + (interaction-proxy plugin-id file-id page-id id interaction index)) + (d/enumerate interactions))))} ;; Methods :resize @@ -1626,7 +1640,7 @@ (st/emit! (dwi/add-interaction page-id id interaction) (se/event plugin-id "add-interaction")) - (interaction-proxy plugin-id file-id page-id id index))))) + (interaction-proxy plugin-id file-id page-id id interaction index))))) :removeInteraction (fn [interaction] @@ -1637,6 +1651,9 @@ (not (r/check-permission plugin-id "content:write")) (u/not-valid plugin-id :removeInteraction "Plugin doesn't have 'content:write' permission") + (not= id (obj/get interaction "$shape")) + (u/not-valid plugin-id :removeInteraction "The interaction doesn't belong to this shape") + :else (st/emit! (dwi/remove-interaction {:id id} (obj/get interaction "$index")) diff --git a/frontend/src/app/plugins/utils.cljs b/frontend/src/app/plugins/utils.cljs index 9e4057f1a7..163c77be59 100644 --- a/frontend/src/app/plugins/utils.cljs +++ b/frontend/src/app/plugins/utils.cljs @@ -206,6 +206,15 @@ (when-let [shape (locate-shape file-id page-id shape-id)] (get-in shape [:interactions index]))) +(defn locate-interaction-index + "Position of `interaction` within the shape's current interactions, falling + back to `index` while it addresses an existing interaction." + [file-id page-id shape-id interaction index] + (let [interactions (-> (locate-shape file-id page-id shape-id) :interactions)] + (or (d/index-of interactions interaction) + (when (and (int? index) (< -1 index (count interactions))) + index)))) + (defn proxy->interaction [proxy] (let [file-id (obj/get proxy "$file") diff --git a/plugins/CHANGELOG.md b/plugins/CHANGELOG.md index 96bd5dc421..e4e577f263 100644 --- a/plugins/CHANGELOG.md +++ b/plugins/CHANGELOG.md @@ -8,6 +8,8 @@ ### 🩹 Fixes +- **plugins-runtime**: An interaction obtained from `Shape.interactions` now keeps addressing that interaction instead of the position it held when the array was read. Removing every interaction of a shape from a single read removes all of them rather than leaving some behind, and writing through a held interaction after an earlier one is removed no longer lands on a different interaction. +- **plugins-runtime**: `Shape.removeInteraction()` now rejects an interaction belonging to a different shape with a validation error, instead of removing whichever interaction sat at the same position on the target shape. - **plugins-runtime**: `Library.createComponent()` now rejects invalid input (an empty shape list, or a shape inside a component copy) with a validation error instead of returning a component proxy pointing at nothing. - **plugins-runtime**: Setting an individual padding/margin side (`leftPadding`, `topMargin`, …) now re-derives the padding/margin type, switching to `multiple` when the four sides stop being symmetric (so the value is actually painted) and back to `simple` once top/bottom and left/right are mirrored again. - **plugins-runtime**: Removed the premature deep-hardening of the host plugin context, which froze shared host functions (including `Function.prototype`) before SES override taming, causing `TypeError: Cannot assign to read only property 'toString'` on later host-side function extension. Related to #11001. diff --git a/plugins/apps/plugin-api-test-suite/src/tests/interactions.test.ts b/plugins/apps/plugin-api-test-suite/src/tests/interactions.test.ts index fe18ca4e41..6f01f8ef0f 100644 --- a/plugins/apps/plugin-api-test-suite/src/tests/interactions.test.ts +++ b/plugins/apps/plugin-api-test-suite/src/tests/interactions.test.ts @@ -349,6 +349,70 @@ describe('Interactions', () => { expect(r.interactions.length).toBe(before - 1); }); + // Removing an interaction shifts the ones after it, so draining a shape from + // a single read of the array must reach every interaction it returned. Both + // removal entry points are covered. + test('every interaction can be removed from one read of the array', async (ctx) => { + const r = rect(ctx); + r.addInteraction('click', { type: 'open-url', url: 'https://a.example' }); + await ctx.penpot.waitForLayoutUpdate(); + r.addInteraction('mouse-enter', { + type: 'open-url', + url: 'https://b.example', + }); + await ctx.penpot.waitForLayoutUpdate(); + expect(r.interactions).toHaveLength(2); + + for (const interaction of r.interactions) { + interaction.remove(); + await ctx.penpot.waitForLayoutUpdate(); + } + expect(r.interactions).toHaveLength(0); + }); + + test('removeInteraction can drain a shape from one read of the array', async (ctx) => { + const r = rect(ctx); + r.addInteraction('click', { type: 'open-url', url: 'https://a.example' }); + await ctx.penpot.waitForLayoutUpdate(); + r.addInteraction('mouse-enter', { + type: 'open-url', + url: 'https://b.example', + }); + await ctx.penpot.waitForLayoutUpdate(); + expect(r.interactions).toHaveLength(2); + + for (const interaction of r.interactions) { + r.removeInteraction(interaction); + await ctx.penpot.waitForLayoutUpdate(); + } + expect(r.interactions).toHaveLength(0); + }); + + // A held interaction addresses itself rather than a position, so a write + // reaches it even once an earlier interaction has shifted it. + test('an interaction still writes to itself after an earlier one is removed', async (ctx) => { + const r = rect(ctx); + for (const trigger of ['click', 'mouse-enter', 'mouse-leave'] as const) { + r.addInteraction(trigger, { + type: 'open-url', + url: `https://${trigger}.example`, + }); + await ctx.penpot.waitForLayoutUpdate(); + } + const [first, , last] = r.interactions; + + first.remove(); + await ctx.penpot.waitForLayoutUpdate(); + last.delay = 500; + await ctx.penpot.waitForLayoutUpdate(); + + expect(r.interactions.map((i) => i.trigger)).toEqual([ + 'mouse-enter', + 'mouse-leave', + ]); + expect(r.interactions.map((i) => i.delay)).toEqual([null, 500]); + }); + test('interaction trigger can be changed', (ctx) => { const dest = board(ctx); const r = rect(ctx);