Skip to content

feat(spaces): add shared-space actions to workflows - #981

Open
Deeds67 wants to merge 30 commits into
mainfrom
feat/spaces-in-workflows
Open

feat(spaces): add shared-space actions to workflows#981
Deeds67 wants to merge 30 commits into
mainfrom
feat/spaces-in-workflows

Conversation

@Deeds67

@Deeds67 Deeds67 commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

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 2026 album 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:

AssetCreate → execute()                    [upstream, untouched]
  └─ step "gallery-core#addToSpaceAlbum"
      └─ plugin-gallery wasm shim          [fork] forwards config, no logic
          └─ host fn gallery(method, args) [the seam]
              └─ GalleryWorkflowHostService [fork] validates, dispatches, never throws
                  ├─ SharedSpaceService
                  └─ AlbumService

Three properties are deliberate:

  • The dispatcher calls services, never repositories, so space membership and contribution rights are enforced by exactly the code paths the HTTP API uses. The AuthDto is minted for the asset owner and forwarded untouched.
  • The wasm shim carries no logic — one host call per method. Logic in a sandbox can only be tested through Docker; in a NestJS service it gets 38 unit tests.
  • One generic 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 --filter and one union widening. server/src/services/index.ts is deliberately untouched: the dispatcher is built with BaseService.create and 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. runGuarded splits on instanceof HttpException, and it's tested as an invariant rather than as a detail.

The same rule governs the orphan-album compensation: album.create succeeds for anyone, but linkAlbum can 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

  • 38 server unit tests covering both handlers: name matching (trimmed, case-insensitive, oldest-wins with an id tie-break), per-space failure isolation, retry idempotency, compensation scoping, and the never-throws invariant across four exception types plus the propagation case.
  • 7 web tests for the space picker, asserting the bindable prop through a test-wrapper.
  • 5 e2e tests, which exist for one specific reason: if a future rebase silently drops the gallery registration from onPluginLoad, the wasm fails to instantiate and every space step stops working — while unit tests still pass, fork-patches-check only covers pnpm patches, and ci-invariants only matches patterns under .github/workflows. Four of the five go red in that scenario.

Notes for review

  • Manifest title/description strings render untranslated — upstream has no i18n layer for plugin metadata. The picker's own strings are in all ten locales.
  • No database migration and no OpenAPI regeneration: plugin and method rows are written by the boot-time importer, and method schemas travel as opaque jsonb.
  • A failed space action is logged and skipped, with no user-facing signal. Upstream has no workflow run history at all; building one was out of scope. Follow-up territory.

Design and implementation plan are in docs/superpowers/specs/2026-08-12-spaces-in-workflows-design.md and docs/superpowers/plans/2026-08-12-spaces-in-workflows.md.

Deeds67 added 30 commits August 12, 2026 22:12
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.
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.
@Deeds67 Deeds67 added the changelog:feat Feature change for changelog label Aug 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant