feat(spaces): add shared-space actions to workflows - #981
Open
Deeds67 wants to merge 30 commits into
Open
Conversation
Adds two workflow action steps (add to space, add to space album) via a fork-owned plugin package and a single generic host-function seam, so the permanent cost to upstream-owned files is 7 files / ~21 lines and stays flat as further fork actions and filters are added. Records the verified extension-cost model for future fork capabilities, the never-throws dispatcher contract required to make log-and-skip failure semantics work, and the unit/web/e2e test plan.
Review against the codebase surfaced two findings that invalidated parts of the plan and seven smaller gaps. - The unit test plan was not implementable: newTestService injects repositories, while BaseService.create builds real services from that context, so assertions on collaborator calls had nothing to observe. Adds a protected collaborators() seam (D10, 6.2). - The skip-reason taxonomy is not derivable from exception types, since the access layer rejects non-owners with 400. Reasons are now advisory and log-only, and the affected scenarios assert ok === false (D11). - Specifies orphan-album compensation for a denied link, rather than asserting the outcome with no mechanism (D13). - Adds manifest schema validation (U0), retry idempotency (U27), compensation scoping and failure (U28-U30), a denied linked-album read (U30), and a stale space id in the config form (W7). - Records that the wasm shim is deliberately not unit-tested, ships no templates this cut, and that AssetMetadataExtraction re-fires. - Reorders section 13 so each step names the tests that must be red first.
Twelve TDD tasks covering the fork-owned plugin, the dispatcher and its handlers, the upstream host-function seam, the web picker, the Docker image and end-to-end coverage. Writing the plan corrected three errors in the spec: - services/index.ts is not part of the seam. The dispatcher is built with BaseService.create and declares no events or jobs, so Nest never needs it — the seam is 6 files / ~19 lines, not 7 / ~21. - The chosen-space chip cannot reuse space-card.svelte, which is a full card with a collage, avatars, a pin menu and a route link. - The gate commands did not exist. make check-server / check-web / lint-all are absent from the Makefile despite CLAUDE.md, and 'pnpm test -- --run <path>' silently runs the whole suite.
Reviewing the plan against the codebase found two defects that would have stopped an implementer, and five smaller ones. - The web tests read a bindable prop off the render result, which does not work in Svelte 5 runes mode. Rewritten onto this repo's test-wrapper pattern, following space-albums-controls.test-wrapper. - W2 asserted a space name the component could not render: after picking, the name was looked up in state loaded from getAllSpaces(), so the chip showed the unavailable placeholder — W2 failed and W7 passed for the wrong reason. The picker now merges the picked space into local state, and the specs module-mock @immich/sdk. - Mocking switched from vi.spyOn to the vi.mock convention used across the suite. - Task 3 told the implementer to append an import inside a describe. - U1 was committed red across three tasks; it now lands in the task that makes it pass, so every commit leaves the suite green. - zod is imported as a default import, matching every DTO here. - Dropped the run2 helper in favour of calling dispatch directly.
…tcher, add return type annotation
CI installs with --frozen-lockfile, so a new workspace package must be recorded here or every job fails at install.
import.meta is not legal in the server package: it builds to CommonJS and tsc rejects it with TS1470, even though vitest tolerates it. The plan now specifies the process.cwd()-relative form that was actually implemented.
- The Choose button was gated on `array || spaceIds.length === 0`, which made scenarios W4 and W5 unreachable: in single mode there was no way to reopen the picker to replace an existing selection. - The load effect used a .then().catch() chain, which crashes the tscompat ESLint plugin locally; async/await avoids it.
The design claimed 6 upstream files / ~19 lines. The implemented branch touches 7, +23/-3. The seventh is server/test/repositories/config.repository.mock.ts: making galleryPlugin a required field on resourcePaths breaks that mock's full-object literal, and declaring it optional instead would push a non-null assertion or runtime guard into workflow-execution.service.ts — the one file the design most wants to keep small — to describe an invariant that never actually holds. Also corrects the verification step to diff against the merge base rather than origin/main, which advances during a long branch and otherwise renders unrelated upstream work as if it belonged to this change.
docker-compose.dev.yml already bind-mounts packages/plugin-gallery into the server container, but mise.toml's plugins task never built it — dist/plugin.wasm never existed, so the mount pointed at manifest.json with nothing else in it and the two space actions silently never appeared in the workflow step picker.
Neither path filter listed packages/plugin-gallery/**, so a PR that only edited its manifest.json wouldn't trigger the server or e2e jobs that exist to catch exactly that kind of drift.
The until() helper's own 30s bound could never fire: e2e/vitest.config.ts sets testTimeout to 15s, so every test would die on vitest's timeout first, and E3's two sequential upload-then-poll cycles need more than a single 30s budget regardless. Follow shared-space.e2e-spec.ts's convention of an explicit per-test timeout.
…Picker
toHaveTextContent('space-2') is a substring match, so it also passes
for 'space-1,space-2' -- the append-instead-of-replace bug this test
exists to catch. Anchor with a regex so the test can actually fail.
Every other package under packages/ has its own .prettierrc; without one, plugin-gallery fell back to the repo-root profile (80 columns, prettier-plugin-sort-json), which disagrees with the profile its sibling plugin-core uses. Copied from plugin-core/.prettierrc verbatim.
E3 runs two sequential 30s-bounded polls, so a 60s test timeout exactly equals the worst case and leaves nothing for the uploads and setup that happen outside the polls. The single-poll tests already use bound-plus-margin. Also updates the seam accounting to the measured 9 files / +27-5, and records that the figure is a one-time cost of introducing a package rather than a per-feature cost.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds two shared-space actions to upstream's workflows feature: Add to space and Add to space album.
A workflow can now do things like "every upload taken near home → add it to the Family space", or "auto-curate a
Holidays 2026album inside a space, creating and linking it if it doesn't exist yet". Until now a workflow could add an asset to an album, but spaces were invisible to it.How it fits together
Upstream's workflow engine runs each step as a method inside a WASM plugin (extism), which can only reach server data through a fixed set of host functions. So:
Three properties are deliberate:
AuthDtois minted for the asset owner and forwarded untouched.gallery(method, args)host function, not one per action. This is what keeps the cost flat.Cost to upstream-owned files
9 files, +27/−5. That is a one-time cost of introducing a package, not a per-feature cost — four of the nine exist purely to build and ship
packages/plugin-gallery(Dockerfile, dev compose,mise.toml, CI path filters) and one is a test mock. Every future fork action or filter adds zero upstream lines.The diff is purely additive apart from two pnpm invocations gaining a
--filterand one union widening.server/src/services/index.tsis deliberately untouched: the dispatcher is built withBaseService.createand declares no events or jobs, so Nest never needs to know it exists.The invariant worth reviewing hardest
The dispatcher must never throw for user-fixable conditions. If a host function throws, the SDK rethrows inside the sandbox, that propagates into upstream's
execute()catch, and every remaining step of the workflow is abandoned. So "not a member", "no contribution rights" and "space deleted" resolve to{ ok: false }and the workflow continues; genuine bugs still propagate.runGuardedsplits oninstanceof HttpException, and it's tested as an invariant rather than as a detail.The same rule governs the orphan-album compensation:
album.createsucceeds for anyone, butlinkAlbumcan be denied, so a denied link deletes the album just created — never a pre-existing one — and a failure of that cleanup is swallowed rather than thrown.Tests
galleryregistration fromonPluginLoad, the wasm fails to instantiate and every space step stops working — while unit tests still pass,fork-patches-checkonly covers pnpm patches, andci-invariantsonly matches patterns under.github/workflows. Four of the five go red in that scenario.Notes for review
title/descriptionstrings render untranslated — upstream has no i18n layer for plugin metadata. The picker's own strings are in all ten locales.jsonb.Design and implementation plan are in
docs/superpowers/specs/2026-08-12-spaces-in-workflows-design.mdanddocs/superpowers/plans/2026-08-12-spaces-in-workflows.md.