Skip to content

improvement(ci): parallel jobs, Turbo cache, affected-only checks#4

Open
yokoszn wants to merge 24 commits into
mainfrom
improvement/ci-performance
Open

improvement(ci): parallel jobs, Turbo cache, affected-only checks#4
yokoszn wants to merge 24 commits into
mainfrom
improvement/ci-performance

Conversation

@yokoszn

@yokoszn yokoszn commented Apr 10, 2026

Copy link
Copy Markdown

What this changes

A full rewrite of .github/workflows/ci.yml plus supporting changes to turbo.json, scripts/, packages/cli, and .dockerignore. The original workflow was a single serial pipeline taking ~55 minutes. The new one is a parallel job graph that completes in 5–15 minutes depending on what changed.


Original system (baseline on main)

Structure: Two sequential jobs, one after the other.

test → ephemeral-integration
     └→ docker-build

test job did everything in serial:

  1. yarn install (no cache)
  2. yarn npm audit — security gate blocking the build even if nothing changed
  3. yarn build:packages (Turbo cache disabled — cache: false)
  4. yarn generate
  5. yarn build:packages again, post-generate
  6. Dep-version check, i18n sync/usage check
  7. yarn typecheck — all packages, every time
  8. yarn test — all packages, every time
  9. yarn build:app — Next.js build

ephemeral-integration ran after test fully finished and repeated steps 1–5 from scratch (no shared artifacts), then ran all 311+ Playwright specs on a single runner with no sharding.

docker-build also ran after test, building three Dockerfiles sequentially.

Key problems:

  • No caching of any kind — yarn install (~2 min), node_modules extraction (~85s), Turbo outputs, Playwright browsers reinstalled every run
  • turbo.json had "cache": false on build and typecheck, and "globalPassThroughEnv": ["*"] — every environment variable was part of the cache key, making Turbo remote cache effectively useless
  • Audit serialised with the build; a yarn.lock-stable run still waited on a full CVE scan
  • Integration rebuilt the entire app (~95s) in every run, separately from test
  • No affected-only filtering — changing one file typechecked and tested every package in the monorepo
  • All 311+ integration tests ran on a single runner for every PR and every push
  • Stale runs were not cancelled — rapid pushes queued multiple 55-minute runs
  • Action versions targeted Node.js 20, running on Node 24 only via a forced override

New system (this PR)

Structure: Six parallel jobs with a shared build.

prepare ──┬──► test ──────────────────────────────► merge-coverage (push only)
audit   ──┤                                              ▲
lint    ──┘──► ephemeral-integration (parallel) ─────────┘
          └──► docker-build (parallel, skipped for CI-only PRs)

Removed

What Why removed
Serial yarn install with no cache in every job Replaced by .yarn/cache + node_modules GHA cache. On yarn.lock-stable runs, install is skipped entirely.
Repeated full package build in ephemeral-integration Eliminated by prepare uploading compiled packages/*/dist/ + generated files as build-artifacts. All downstream jobs download instead of rebuild.
Repeated yarn build:app in every integration shard Eliminated by uploading the tarred .mercato/next/ as app-build. All 15 shards extract it instead of rebuilding (~95s × 15 = ~24 min saved per full-suite run).
Turbo "cache": false on build and typecheck Replaced with "cache": true + correct inputs and outputs. Unlocks both local .turbo GHA cache and optional remote cache.
"globalPassThroughEnv": ["*"] in turbo.json Replaced with "globalEnv": ["NODE_ENV"]. The wildcard caused every ephemeral runner env var to be part of the turbo cache key, producing a near-zero hit rate.
Audit inline in the test job Extracted to a standalone audit job that runs in parallel and is skipped entirely when yarn.lock has not changed (GHA cache keyed on lockfile hash).
Playwright browsers installed fresh every run Replaced by GHA cache keyed on Playwright version. Install only runs on version bump.
All packages typechecked and tested every PR Replaced by --filter=[origin/$base_ref]... on PRs — only affected packages and their dependents run. Full run on pushes to protected branches.
All 311+ integration tests on a single runner every run CI/docs PRs: skipped. Module PRs: affected-only, single runner. Shared-package PRs and pushes: 15-shard parallel.
docker-build waiting on test to finish Docker now starts as soon as prepare finishes — runs in parallel with test and integration.
docker-build on CI/infra-only PRs Skipped entirely when skip_integration == true. Avoids a 10+ min rebuild from Docker layer cache busting when innocuous files change (e.g. turbo.json).
Node.js 20-targeting action versions Upgraded across all workflow files: checkout@v6, setup-node@v6, cache@v5, upload-artifact@v7, download-artifact@v8.

Added

What Effect
prepare job Single canonical build producing two artifacts (build-artifacts, app-build). Scope computation runs here so integration shards start immediately after prepare without waiting for typecheck.
lint job Parallel ESLint run alongside prepare and audit. Fails fast before heavy work starts.
merge-coverage job Aggregates per-shard Istanbul summaries after all 15 integration shards complete and writes the merged report to the GitHub step summary. Only runs on push (sharded) runs.
Scope computation in prepare Analyses git diff origin/$base_ref --name-only and emits skip_integration (bool), affected_modules (comma list), and shard_matrix (JSON array). Drives all downstream job decisions.
OM_INTEGRATION_MODULES filtering When affected_modules is non-empty, playwright.config.ts filters specs to only the affected modules' tests.
15-shard Playwright matrix on push Full-suite runs distribute 311+ tests across 15 parallel runners. Each shard writes a coverage-shard-N/ subdirectory; merge-coverage recombines them.
--shard N/M CLI flag Added to packages/cli/src/lib/testing/integration.ts — passes through to Playwright's own --shard arg.
scripts/merge-coverage.mjs New script (zero external deps) that sums Istanbul totals across shards and resolves per-file conflicts by picking the shard with higher coverage.
Turbo remote cache wiring TURBO_TOKEN / TURBO_TEAM plumbed into all turbo run steps. If secrets are set, remote cache is active; if not, local .turbo GHA cache is used. No config required.
Concurrency groups ci-${{ github.ref }} group cancels in-flight runs on the same branch when a new push arrives.
.dockerignore improvements Added **/testing/ and CI-only scripts (merge-coverage.mjs, i18n-check-sync.ts, i18n-check-usage.ts) to prevent these files from busting Docker's COPY packages/ layer cache.
Turbo "inputs" on build task "inputs": ["$TURBO_DEFAULT$", "generated/**"] — includes gitignored generated files in the cache key, preventing false Turbo cache hits on the second post-generate build.

Modified (same purpose, different behaviour)

What Before After
ephemeral-integration trigger After test (serial, gated on typecheck + unit tests) After prepare + audit + lint (parallel with test)
Integration test scope Always full suite, 311 tests CI/docs PRs: skipped. Module PRs: affected-only, single shard. Shared-package PRs and pushes: 15-shard full suite.
yarn install Ran unconditionally in every job Skipped when node-modules-${{ runner.os }}-${{ hashFiles('yarn.lock') }} cache hits
Security audit Blocking step in test job Standalone parallel job, result cached on yarn.lock hash
yarn build:app in prepare Unconditional Skipped when skip_integration == true — saves ~95s for CI/docs-only PRs
docker-build dependency needs: test needs: prepare — runs in parallel with test and integration
docker-build scope Always ran on non-fork PRs/pushes Skipped when skip_integration == true
merge-coverage on missing shard data exit 1 (failed the job) exit 0 with warning — graceful degradation

Unchanged

  • All environment variables passed to the ephemeral server (OM_ENABLE_ENTERPRISE_MODULES, JWT_SECRET, etc.)
  • OM_INTEGRATION_APP_READY_TIMEOUT_SECONDS: '180' timeout
  • The Playwright test suite itself — no tests added or removed
  • docker-build steps — same three Dockerfiles, same GHA layer cache config
  • snapshot.yml and release.yml job logic — only action version bumps applied
  • CI must still be green before merge — only the time to get there changes

Hardening

Area Change
Stale run prevention Concurrency group cancels queued runs immediately on new push
Turbo cache key correctness globalPassThroughEnv: ["*"]globalEnv: ["NODE_ENV"] — eliminates phantom cache misses from ephemeral runner metadata in the hash
Turbo input correctness generated/** added to build task inputs — prevents false hits when generated TypeScript files change but are not git-tracked
Audit isolation Audit failure blocks the run independently of build/test; yarn.lock-stable runs skip it entirely
Artifact integrity if-no-files-found: error on both uploaded artifacts — CI fails immediately if a produce step silently produced nothing
Artifact filename safety Next.js chunk files contain colons (e.g. [externals]_node:fs_promises_*.js) which upload-artifact rejects — fixed by tarring .mercato/next/ before upload and extracting after download
Docker layer cache stability .dockerignore additions prevent testing utilities and CI scripts from busting the COPY packages/ layer
Node.js action runtime All actions upgraded to Node.js 24-native versions — removes forced compatibility shim and deprecation warnings
process.env restore in tests Fixed env var teardown in activity-executor.test.ts — previously set the var to the string "undefined" on cleanup when it was originally unset

Measured wall times

Scenario Before After
CI/docs-only PR (this branch) ~18 min ~5.5 min
PR touching 1 module ~55 min ~12–14 min
PR touching shared packages ~55 min ~16–18 min
Push to main/develop (full suite) ~55 min ~16–18 min
Stale run on re-push completes (~55 min) cancelled in <5 sec

The remaining bottleneck for module PRs is ephemeral server startup time inside each integration shard (~120–150s), which is determined by app initialisation speed rather than CI configuration.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added automated coverage merging for distributed integration tests.
    • Implemented module-scoped integration test filtering and Playwright test sharding.
  • Tests

    • Restructured CI/CD pipeline with parallel job execution and artifact reuse across test stages.
  • Chores

    • Optimized build caching configuration and refined Docker image file exclusions.

yokoszn pushed a commit that referenced this pull request Apr 14, 2026
…arry-forward open-mercato#1485) (open-mercato#1488)

* fix: translations

* fix(i18n): sync missing translations + restore BC-critical exports (carry-forward open-mercato#1485)

Carry-forward of open-mercato#1485 (fork PR by @Sawarz) with the blocker findings
from code review applied so it can merge cleanly.

Applied fixes:
- Restored `export const emitCatalogEvent` in `packages/core/src/modules/catalog/events.ts`
  (removed in open-mercato#1485 — breaks BC surface #4 *Import paths: STABLE*).
- Restored `export const emitSalesEvent` in `packages/core/src/modules/sales/events.ts`
  (same reason).
- Restored `packages/core/src/modules/sales/lib/statusHistory.ts` with its
  `StatusChangeLogInput` type and `logStatusChange` helper (part of SPEC-006 / SPEC-059
  public surface).
- Added the two new `auth.acl.*` keys and fourteen new `workflows.checkoutDemo.*`
  keys to `pl.json`, `es.json`, and `de.json` so `i18n-check-sync` passes
  (this was the root cause of the `test` job failure on open-mercato#1485).
- Sorted the touched locale files alphabetically per the check's format rule.
- Kept the original author's AclEditor i18n hookups, workflows checkout-demo
  i18n hookups, and package.json `@types/*` devDeps move.

Credit: original translations + AclEditor wiring by @Sawarz in open-mercato#1485.

CI/verification:
- `yarn build:packages` — OK
- `yarn generate` — OK
- `yarn typecheck` — OK across all 18 packages
- `yarn test` — 2364/2364 pass
- `yarn tsx scripts/i18n-check-sync.ts` — clean

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Sawarz <sawarz22@gmail.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
yokoszn and others added 21 commits April 15, 2026 01:33
…artifacts

- Extract `prepare` job: install → build:packages → generate → rebuild.
  Uploads compiled dist/ + .mercato/generated/ as a reusable artifact so
  downstream jobs skip the repeated build+generate work.

- Extract `audit` job: runs in parallel with `prepare` rather than
  sequentially inside `test`, removing it from the critical path.

- `test` job now downloads the artifact instead of rebuilding packages.

- `ephemeral-integration` downloads the same artifact and skips
  install+build:packages×2+generate (was ~1m31s of duplicate work).

- Add `cache: 'yarn'` to all setup-node@v4 steps (~45s saved per job on
  warm cache hits).

- Add `actions/cache` for pip to avoid re-downloading markitdown on every run.

Measured baseline (run 24178370484): test 7m12s, ephemeral-integration 48m47s,
total wall ~55 min. Integration tests themselves account for 44m59s; sharding
across 3 Playwright workers (--shard=N/3) is the next lever — see comment in
the ephemeral-integration job for what that would require.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ote cache

Concurrency groups:
- ci.yml: cancel-in-progress on same branch — stops stale 55-min runs
  from completing when a new push arrives
- snapshot.yml: cancel-in-progress — prevents double npm publishes from
  two rapid pushes to develop

Turbo cache:
- Remove globalPassThroughEnv: ["*"] which made every env var part of
  the cache key, giving a near-zero hit rate in CI
- Replace with globalEnv: ["NODE_ENV"] — the only env var that
  legitimately affects compiled output
- Enable cache: true for build and typecheck tasks
- Verified locally: second build run hits 18/18 packages from cache
  in 478ms vs 2.7s cold (83% faster, scales to minutes saved in CI)

Remote cache wiring:
- Add TURBO_TOKEN + TURBO_TEAM env vars to build/typecheck steps
  (optional — falls back to local GHA cache when secrets are absent)
- Add actions/cache on .turbo/ with branch-scoped restore-keys so
  cross-commit cache hits work within the same branch, and fallback
  to any previous run serves as a warm start for new branches

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
cancel-in-progress: true would cancel a snapshot run mid-publish if a
new push arrived, leaving some packages at the new version and others
at the previous one — an inconsistent npm registry state.

Queue (cancel-in-progress: false) is the correct behaviour: each
publish completes atomically before the next one starts.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
On pull requests, Turbo now scopes build, typecheck, and test tasks to
only packages that changed relative to the base branch — plus their
transitive dependents. On pushes to main/develop, the full suite runs.

Implementation details:
- Fetch the base branch at depth=1 before running Turbo so git can
  compute the diff (shallow clone cannot reach origin/main otherwise)
- Use --filter=[origin/<base_ref>]... to scope to affected packages
- Add --filter='!./apps/*' to exclude apps from the package build steps:
  Turbo's multiple --filter flags use union semantics, so without the
  negation, apps are included as dependents of changed packages and
  fail because generated files don't exist at that point in the job
- generate and build:app always run fully — generate is global module
  discovery; build:app is required for integration tests downstream

Expected impact on a PR touching 1 module:
  build:packages  2m30s → ~5s  (17 cache hits + 1 rebuild)
  typecheck       2m04s → ~5s  (only affected packages)
  unit tests      1m17s → ~5s  (only affected packages)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Two correctness issues in the previous commit:

1. Filtering build:packages in prepare produces an incomplete artifact.
   Downstream jobs (test, ephemeral-integration) download the artifact
   expecting every package's dist/ to be present. A filtered build skips
   unaffected packages entirely — Turbo only restores cache outputs for
   packages it actually runs, so excluded packages never land on disk.
   Fix: prepare always builds all packages. Turbo cache handles the
   speedup — unchanged packages are cache hits restored in milliseconds,
   so the artifact is always complete and the build is still fast.

2. The --filter='!./apps/*' negation in typecheck/test silently skipped
   the app even when app-level source files changed. Generated files are
   available in the test job (artifact download includes .mercato/generated/),
   so the app typecheck is safe to include when the app is in scope.
   Fix: drop the negation. The git-based filter correctly includes the
   app only when it or something it depends on has changed.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
setup-node@v4's cache: 'yarn' calls `yarn config get cacheFolder` before
corepack enable runs, which invokes Yarn 1 (1.22.22) — not Yarn 4. With
packageManager: "yarn@4.12.0" in package.json, Yarn 1 aborts immediately.

Fix: remove cache: 'yarn' from all four jobs (prepare, audit, test,
ephemeral-integration) and add an explicit actions/cache step on .yarn/cache
after corepack enable. Cache key is yarn.lock hash with runner OS prefix.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
TypeScript resolves #generated/* imports via the packages/core/package.json
imports field, where the 'types' condition points to source .ts files in
packages/core/generated/ (not dist/). The artifact previously only uploaded
packages/*/dist/ and apps/mercato/.mercato/generated/, leaving these source
TypeScript generated files off disk in the test job — causing:

  Cannot find module '#generated/entities.ids.generated'

Affected packages with a generated/ dir: core, onboarding, scheduler,
integration-cozystack.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ementation

- Phase 0: replace incorrect 'cache: yarn' checkbox with the actual fix
  (explicit actions/cache on .yarn/cache after corepack enable) and document
  why setup-node cache: yarn cannot be used with Yarn Berry + Corepack.
- Phase 0: add packages/*/generated/ artifact note — required for #generated/*
  Node.js subpath imports whose types condition points to source .ts files.
- Section 5.5: correct snapshot.yml concurrency to cancel-in-progress: false
  with explanation — cancelling mid-publish leaves partial npm packages.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
packages/*/generated/ is in .gitignore. Turbo only hashes git-tracked
files by default, so after 'yarn generate' creates
packages/core/generated/entities.ids.generated.ts, the second
'yarn build:packages' computes the same hash as the first and returns
a cached result that has no dist/generated/. Jest then fails at runtime:

  Cannot find module '../../../generated/entities.ids.generated.js'
  from '../core/dist/modules/attachments/lib/partitions.js'

Fix: add inputs: ["$TURBO_DEFAULT$", "generated/**"] to the build task.
$TURBO_DEFAULT$ preserves all non-gitignored inputs; generated/** explicitly
includes the gitignored files. After generate, the hash changes, the second
build is a cache miss, esbuild runs and writes dist/generated/.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ywright parallelism

- ephemeral-integration: skip when no module code changed (skip_integration output from test job)
- ephemeral-integration: matrix shard — PR uses single runner with OM_INTEGRATION_MODULES filtering,
  push uses 5 parallel shards (["1/5"…"5/5"]) for full suite
- test job: inline "Compute integration scope" step produces skip/modules outputs from git diff;
  full-suite patterns trigger unfiltered run, non-module changes skip integration entirely
- Add merge-coverage job (push only): downloads integration-test-results-* artifacts with
  merge-multiple, runs scripts/merge-coverage.mjs, writes combined step summary
- playwright.config.ts: filter specs by OM_INTEGRATION_MODULES when set
- packages/cli/src/lib/testing/integration.ts: --shard N/M CLI flag forwarded to Playwright
- scripts/merge-coverage.mjs: sum coverage-shard-*/code/coverage-summary.json into one report
- .ai/specs/2026-04-10-ci-cd-performance.md: mark Phase 3 steps 2-3 and Phase 4 steps 1-5 done

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Cache the audit pass result keyed on yarn.lock hash. If the lockfile
hasn't changed the dependency graph is identical — prior passing audit
still valid, skip yarn install + yarn npm audit (~2m saved per run).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
With nodeLinker:node-modules, yarn extracts zips from .yarn/cache into
node_modules on every install even when the package cache hits. The
extraction alone accounts for ~85s per job (prepare, test, integration).

Cache node_modules keyed on yarn.lock hash and skip install entirely on
hit. First run saves; subsequent runs on the same yarn.lock restore in
~10-15s instead of ~85s. Saves ~4min across the 3 jobs that need it.

The audit job already skips everything on audit-cache hit; node_modules
cache is only checked when audit-cache misses (i.e. yarn.lock changed).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replaces the per-run 'npx playwright install --with-deps chromium' step
with the pre-built mcr.microsoft.com/playwright:v1.50.0-jammy container.
Chromium and all system dependencies are already in the image — saves
~25s per shard (5 shards = ~2min on push runs).

Tag must stay in sync with @playwright/test in .ai/qa/package.json.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…hemeral server

The integration environment uses Docker CLI to start the app server.
Running inside a container (mcr.microsoft.com/playwright) means Docker
is not available in PATH, causing the test runner to fail immediately.

Cache Playwright browser binaries at ~/.cache/ms-playwright instead —
same skip-on-hit effect, no Docker conflict.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Previously PRs always used a single runner regardless of scope. PRs that
touch packages/shared, packages/ui, or other cross-cutting packages still
trigger the full 311-test suite — which takes 20+ min on one runner.

The shard_matrix output from the test job now drives the strategy:
- Full suite (push OR PR touching shared/ui/core-lib): 5 parallel shards
- Affected-only (PR touching specific modules): single runner with filter
- No module changes: skip entirely

merge-coverage triggers on shard_matrix != '["none"]' (replaces push-only).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…g shards

- Add FORCE_JAVASCRIPT_ACTIONS_TO_NODE24=true to ci.yml, qa-deploy.yml,
  and snapshot.yml to silence Node 20 deprecation warnings ahead of the
  June 2 forced migration (actions/checkout@v4, actions/cache@v4,
  actions/setup-node@v4, actions/upload-artifact@v4,
  docker/build-push-action@v6, docker/setup-buildx-action@v3)
- Change merge-coverage.mjs to exit 0 (warn) instead of exit 1 when no
  shard coverage files are found — coverage not being generated is not a
  CI-blocking condition

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add parallel `lint` job (ESLint) that gates `test` alongside audit/prepare,
  implementing "Static Analysis First" principle
- Upload `apps/mercato/.next/` from `test` job as `app-build` artifact;
  `ephemeral-integration` downloads it instead of rebuilding each shard
  (~96s × 5 shards = ~8 min saved per full run)
- Fix `scripts/merge-coverage.mjs` to exit 0 with a warning when no shard
  coverage files are found rather than failing the job

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Key architectural changes to hit the 5-8 min target:

Job graph before:
  prepare -> [test (build:app + typecheck + units)] -> integration (5 shards)

Job graph after:
  prepare (build:app) -> test (typecheck+units)  \
  audit (parallel)    -> integration (15 shards)  -> merge-coverage (final gate)
  lint (parallel)     /

- Move build:app and scope computation from test into prepare
  so integration shards start the moment the build is ready,
  without waiting 3+ min for typecheck/unit tests
- Integration now runs in parallel with test (not after it)
- merge-coverage is the final gate: requires both test AND integration
- 5 shards -> 15 shards for full-suite runs (~6 min vs ~19 min)
- App build uploaded as separate artifact from prepare; each shard
  downloads it instead of rebuilding (~96s x 15 = ~24 min saved)
- Fix distDir: output is .mercato/next/, not .next/
- Fix lint: exclude @open-mercato/app (next lint needs ESLint config)
- Lint runs parallel with prepare/audit; gates test + integration

Projected wall time for full push runs: ~8 min (was 26 min)
Projected wall time for typical module PRs: ~3-5 min

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
actions/upload-artifact rejects filenames containing colons. Next.js
generates chunk files like [externals]_node:fs_promises_*.js in the
.mercato/next/ output directory. Tar the directory into a single archive
before uploading and extract after downloading in each shard.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… in parallel

For branches that only touch CI/scripts/docs files (turbo.json, scripts/,
packages/cli/testing/, .github/), the docker-build job was rebuilding the
entire Next.js app from scratch because these files sit in early Dockerfile
COPY layers and fully bust the GHA Docker layer cache. With docker-build
running sequentially after test, this produced an 18 min wall time for a
branch that never changed any app code.

Three changes:

1. docker-build now needs prepare (not test), running in parallel with test
   and ephemeral-integration. Removes test time from docker critical path.

2. docker-build is skipped when skip_integration == 'true' (CI/docs-only
   PRs). The Docker image is functionally unchanged; there is nothing to
   validate. This eliminates the 10+ min rebuild cost for such PRs.

3. Build app, Archive app build, and Upload app build steps in prepare are
   skipped when skip_integration == 'true'. No integration shard will ever
   download the artifact, so the ~95s build + tar + upload are pure waste.

Also improves .dockerignore to exclude **/testing/ directories and CI-only
scripts so future changes to testing utilities and merge helpers do not bust
Docker layer cache when they change alongside app code.

Expected wall time for CI-only PRs:
  Before: prepare (4 min) → test (4 min) → docker-build (10+ min) = 18 min
  After:  prepare (2.5 min) → test (4 min)                         = 6.5 min

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Resolves "Node.js 20 is deprecated" warnings in all workflow jobs.

  actions/checkout        v4 → v6
  actions/setup-node      v4 → v6
  actions/cache           v4 → v5
  actions/upload-artifact v4 → v7
  actions/download-artifact v4 → v8

Applied to ci.yml (all jobs), qa-deploy.yml, and snapshot.yml.
release.yml and snapshot.yml checkout/setup-node were already on v6.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@yokoszn yokoszn force-pushed the improvement/ci-performance branch from 8c93573 to a29cb2b Compare April 14, 2026 15:43
@TWN-Systems TWN-Systems deleted a comment from coderabbitai Bot Apr 15, 2026
yokoszn and others added 2 commits April 15, 2026 22:51
- Add yarn test:scripts after the Turbo test step — root-level scripts
  are never picked up by turbo run test (M1)
- Restore OM_WEBHOOKS_ALLOW_PRIVATE_URLS=1 to ephemeral-integration env
  so webhook tests delivering to localhost do not fail (M2)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ci.yml: expand affected-module regex from hardcoded packages/core,
packages/enterprise, apps/mercato to all workspace packages and apps
(packages/[^/]+|apps/[^/]+). PRs touching modules in checkout,
gateway-stripe, content, ai-assistant, etc. were falling into the
skip=true branch and bypassing integration tests entirely.

scripts/merge-coverage.mjs: recompute merged totals from the
deduplicated per-file map instead of summing each shard's totals
directly. Summing shard totals double-counts files exercised by more
than one shard, making merged percentages incorrect.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Apr 16, 2026

Copy link
Copy Markdown

Warning

Rate limit exceeded

@yokoszn has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 43 minutes and 37 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 43 minutes and 37 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 86eb216a-7df8-46bb-a289-0fae915a44b6

📥 Commits

Reviewing files that changed from the base of the PR and between 789dd74 and 899c620.

📒 Files selected for processing (1)
  • .github/workflows/ci.yml
📝 Walkthrough

Walkthrough

The changes implement a comprehensive CI/CD performance optimization strategy by enabling Turbo caching with controlled environment pass-through, restructuring the CI workflow with a new prepare job that computes affected modules and shard matrices, implementing Playwright integration test sharding with coverage merging, adding module-scoped test filtering, reorganizing job dependencies to enable parallel execution, and documenting the complete initiative.

Changes

Cohort / File(s) Summary
CI Workflow Restructuring
.github/workflows/ci.yml
Introduced prepare job computing affected modules and shard matrices; refactored test, ephemeral-integration, and docker-build jobs with new dependencies; added merge-coverage job for aggregating shard coverage; added concurrency controls and upgraded action versions (checkout v4→v6, setup-node v4→v6, upload/download-artifact v4→v7).
Workflow Configuration Updates
.github/workflows/qa-deploy.yml, .github/workflows/snapshot.yml
Added FORCE_JAVASCRIPT_ACTIONS_TO_NODE24 environment variable; snapshot workflow added concurrency settings; upgraded checkout and upload-artifact action versions.
Build & Caching Configuration
turbo.json, .dockerignore
Turbo: replaced globalPassThroughEnv: ["*"] with globalEnv: ["NODE_ENV"]; enabled build and typecheck task caching with explicit cache inputs. Dockerignore: added exclusions for **/testing/ directories and CI-only scripts (merge-coverage.mjs, i18n-check-*.ts).
Test Execution & Coverage
.ai/qa/tests/playwright.config.ts, packages/cli/src/lib/testing/integration.ts, scripts/merge-coverage.mjs
Playwright config: replaced discoveredSpecPaths with filteredSpecPaths that respect OM_INTEGRATION_MODULES environment variable. Integration CLI: added --shard N/M argument support. New script: merge-coverage.mjs consolidates per-shard Playwright coverage from coverage-shard-* directories into merged summaries.
Documentation
.ai/specs/2026-04-10-ci-cd-performance.md
New specification documenting current CI/CD topology (55-minute baseline), root-cause analysis (always-run suite, disabled caching, ephemeral setup), proposed improvements (Turbo caching, module filtering, sharding, concurrency groups, prebuilt containers), implementation progress, risk assessment, and phased rollout plan.

Sequence Diagram(s)

sequenceDiagram
    participant GHA as GitHub Actions
    participant Prepare as prepare job
    participant Audit as audit job
    participant Lint as lint job
    participant Test as test job
    participant IntTest as ephemeral-integration job
    participant Merge as merge-coverage job

    GHA->>Prepare: trigger (git diff analysis)
    activate Prepare
    Prepare->>Prepare: compute affected modules<br/>build shard matrix
    Prepare->>Prepare: build packages + generate<br/>rebuild with generated
    alt integration enabled
        Prepare->>Prepare: build Next.js app
    end
    Prepare->>GHA: upload build-artifacts<br/>upload app-build
    deactivate Prepare

    par
        Prepare->>Audit: notify ready
        activate Audit
        Audit->>GHA: run Yarn audit<br/>cache on yarn.lock
        deactivate Audit
    and
        Prepare->>Lint: notify ready
        activate Lint
        Lint->>GHA: run Turbo lint<br/>(filtered packages)
        deactivate Lint
    and
        Prepare->>Test: notify ready
        activate Test
        Test->>GHA: download build-artifacts
        Test->>GHA: run typecheck & tests<br/>(scoped to changes)
        Test->>GHA: run yarn test:scripts
        deactivate Test
    end

    Prepare->>IntTest: send shard matrix<br/>(none or N shards)
    activate IntTest
    IntTest->>GHA: download app-build
    IntTest->>GHA: extract + run sharded tests<br/>OM_INTEGRATION_MODULES set
    par
        IntTest->>GHA: shard 1 (coverage output)
    and
        IntTest->>GHA: shard 2 (coverage output)
    and
        IntTest->>GHA: shard N (coverage output)
    end
    deactivate IntTest

    alt shard matrix not ["none"]
        IntTest->>Merge: notify shards complete
        activate Merge
        Merge->>GHA: download all shard artifacts
        Merge->>Merge: merge coverage-summary.json<br/>from coverage-shard-*
        Merge->>GHA: upload merged coverage
        deactivate Merge
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐰 Hops excitedly with carrot in paw

Shards now scatter like seeds we sow,
Turbo caches make workflows flow,
Modules filtered, coverage merged so bright,
CI runs faster—parallel might!
celebratory binky

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'improvement(ci): parallel jobs, Turbo cache, affected-only checks' directly and accurately summarizes the three main changes: introducing parallel CI jobs, enabling Turbo caching, and implementing affected-only test execution.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch improvement/ci-performance

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (6)
.dockerignore (1)

75-77: Minor: Duplicate ignore entries.

.github (line 4) and .vscode (line 7) already appear earlier in the file. These duplicates are harmless but add maintenance overhead.

🧹 Proposed cleanup
 # CI/CD
-.github
-.vscode
+# (already ignored above)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.dockerignore around lines 75 - 77, Remove the duplicate ignore entries by
deleting the repeated lines ".github" and ".vscode" in the .dockerignore so each
directory appears only once; ensure the earlier occurrences remain and remove
the later ones to avoid redundant entries.
.ai/specs/2026-04-10-ci-cd-performance.md (1)

21-27: Optional: Add language identifiers to fenced code blocks.

The ASCII diagrams and text output blocks could use text or plaintext as a language identifier to satisfy markdown linters, though this is purely cosmetic.

Also applies to: 45-62, 72-74, 195-198

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.ai/specs/2026-04-10-ci-cd-performance.md around lines 21 - 27, The fenced
ASCII diagram blocks (e.g., the block containing "prepare", "test",
"merge-coverage", "audit", "lint", "ephemeral-integration", "docker-build")
should include a language identifier like ```text or ```plaintext to satisfy
linters; update each relevant fenced block (including the other occurrences at
the ranges you noted) by changing the opening fence to ```text (or ```plaintext)
so the diagram and text output are marked as plain text.
.github/workflows/ci.yml (4)

387-394: Consider passing Turbo credentials to the test step.

The typecheck step (lines 383-385) passes TURBO_TOKEN and TURBO_TEAM for remote caching, but the test step (lines 387-394) does not. If yarn test runs through Turbo, it could benefit from remote cache as well.

💡 Optional: Add Turbo credentials to test step
       - name: Test
         # Same scoping as typecheck — affected packages only on PRs.
         run: |
           if [ "${{ github.event_name }}" = "pull_request" ]; then
             yarn turbo run test --filter=[origin/${{ github.base_ref }}]...
           else
             yarn test
           fi
+        env:
+          TURBO_TOKEN: ${{ secrets.TURBO_TOKEN }}
+          TURBO_TEAM: ${{ secrets.TURBO_TEAM }}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/ci.yml around lines 387 - 394, The Test step currently
runs the same turbo-aware commands as typecheck but does not export Turbo remote
cache credentials; update the "Test" step to export the same environment
variables used in the typecheck step by adding TURBO_TOKEN and TURBO_TEAM to the
step's environment so both branches (yarn turbo run test --filter=[origin/${{
github.base_ref }}]... and yarn test) can use remote caching; ensure the env
keys exactly match TURBO_TOKEN and TURBO_TEAM.

491-500: Hardcoded Playwright version in cache key may drift from installed version.

The cache key uses playwright-chromium-${{ runner.os }}-v1.50.0, but this version is hardcoded. If the project upgrades Playwright, the cache key won't update automatically, potentially causing mismatches between cached and required browser versions.

Consider deriving the version dynamically from the lockfile or package.json:

💡 Dynamic Playwright version example
      - name: Get Playwright version
        id: playwright-version
        run: echo "version=$(node -p "require('@playwright/test/package.json').version")" >> "$GITHUB_OUTPUT"

      - name: Cache Playwright browsers
        id: playwright-cache
        uses: actions/cache@v5
        with:
          path: ~/.cache/ms-playwright
          key: playwright-chromium-${{ runner.os }}-${{ steps.playwright-version.outputs.version }}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/ci.yml around lines 491 - 500, Replace the hardcoded cache
key used by the step with id "playwright-cache" so it derives the Playwright
version dynamically; add a preceding step (e.g., id "playwright-version") that
reads the installed Playwright package version from package.json or
`@playwright/test` and writes it to GITHUB_OUTPUT, then use that output in the
cache key (instead of "v1.50.0") so the cache key and the "npx playwright
install --with-deps chromium" install step stay in sync when Playwright is
upgraded.

527-572: Consider extracting coverage display logic to reduce duplication.

The coverage display code (lines 539-572) is nearly identical to the merged coverage display (lines 623-656). This duplication increases maintenance burden.

Consider extracting this to a reusable shell script (e.g., scripts/display-coverage.sh) or a composite action that both steps can invoke with a summary file path argument.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/ci.yml around lines 527 - 572, The two nearly identical
coverage display blocks (the "Display integration coverage summary" step that
reads SUMMARY_FILE and the merged coverage display step) should be extracted
into a single reusable script or composite action (e.g.,
scripts/display-coverage.sh) so both steps invoke the same logic; implement a
script that accepts the summary file path (like the current "$SUMMARY_FILE") and
performs JSON parsing, console output, and appending to GITHUB_STEP_SUMMARY,
then replace the inline node -e blocks in the steps with a call to that script
(or action) passing the summary file argument to eliminate duplication while
keeping behavior identical.

612-613: Consider conditioning display on merge success.

Using if: always() on line 613 will run the display step even if the merge step (line 610) fails. This could show stale or missing data. Consider conditioning on the merge step's outcome:

       - name: Display merged integration coverage
-        if: always()
+        if: success()
         run: |

Alternatively, keep always() if visibility of partial results is preferred.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/ci.yml around lines 612 - 613, The "Display merged
integration coverage" step is currently guarded by if: always(), which runs even
when the earlier merge step failed; change its condition to only run on merge
success by replacing if: always() with a success check against the merge step
(e.g., if: steps.merge.outcome == 'success' or if:
steps["merge-integration"].outcome == 'success' depending on that step's id), or
use if: success() scoped to the job/needs if the merge is in a different job
(e.g., if: needs.merge.outputs.<flag> == 'true'); ensure you reference the
actual merge step id (e.g., merge or merge-integration) when updating the
condition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/ci.yml:
- Around line 668-676: The docker-build job is being skipped when
needs.prepare.outputs.skip_integration == 'true' even if Dockerfiles changed;
update the prepare job (integration-scope step that sets FULL_SUITE_PATTERN /
skip_integration) to include Docker-related paths (e.g., Dockerfile,
apps/docs/Dockerfile, docker/**) in the full-suite detection, or alternatively
add a separate check in the docker-build job's if condition to also run when
Docker paths changed; specifically modify the prepare job's pattern logic or the
docker-build job's if clause so docker-build runs when Dockerfile/docker/
changes despite skip_integration being true.

In @.github/workflows/qa-deploy.yml:
- Around line 3-5: There are duplicate workflow-level env: blocks so the later
env block overwrites the earlier one and FORCE_JAVASCRIPT_ACTIONS_TO_NODE24 is
never set; fix by merging both env: blocks into a single top-level env: mapping
and ensure FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: 'true' is included alongside the
other environment variables (i.e., consolidate the env key so both the existing
variables from the second env block and FORCE_JAVASCRIPT_ACTIONS_TO_NODE24
appear under one env:).

---

Nitpick comments:
In @.ai/specs/2026-04-10-ci-cd-performance.md:
- Around line 21-27: The fenced ASCII diagram blocks (e.g., the block containing
"prepare", "test", "merge-coverage", "audit", "lint", "ephemeral-integration",
"docker-build") should include a language identifier like ```text or
```plaintext to satisfy linters; update each relevant fenced block (including
the other occurrences at the ranges you noted) by changing the opening fence to
```text (or ```plaintext) so the diagram and text output are marked as plain
text.

In @.dockerignore:
- Around line 75-77: Remove the duplicate ignore entries by deleting the
repeated lines ".github" and ".vscode" in the .dockerignore so each directory
appears only once; ensure the earlier occurrences remain and remove the later
ones to avoid redundant entries.

In @.github/workflows/ci.yml:
- Around line 387-394: The Test step currently runs the same turbo-aware
commands as typecheck but does not export Turbo remote cache credentials; update
the "Test" step to export the same environment variables used in the typecheck
step by adding TURBO_TOKEN and TURBO_TEAM to the step's environment so both
branches (yarn turbo run test --filter=[origin/${{ github.base_ref }}]... and
yarn test) can use remote caching; ensure the env keys exactly match TURBO_TOKEN
and TURBO_TEAM.
- Around line 491-500: Replace the hardcoded cache key used by the step with id
"playwright-cache" so it derives the Playwright version dynamically; add a
preceding step (e.g., id "playwright-version") that reads the installed
Playwright package version from package.json or `@playwright/test` and writes it
to GITHUB_OUTPUT, then use that output in the cache key (instead of "v1.50.0")
so the cache key and the "npx playwright install --with-deps chromium" install
step stay in sync when Playwright is upgraded.
- Around line 527-572: The two nearly identical coverage display blocks (the
"Display integration coverage summary" step that reads SUMMARY_FILE and the
merged coverage display step) should be extracted into a single reusable script
or composite action (e.g., scripts/display-coverage.sh) so both steps invoke the
same logic; implement a script that accepts the summary file path (like the
current "$SUMMARY_FILE") and performs JSON parsing, console output, and
appending to GITHUB_STEP_SUMMARY, then replace the inline node -e blocks in the
steps with a call to that script (or action) passing the summary file argument
to eliminate duplication while keeping behavior identical.
- Around line 612-613: The "Display merged integration coverage" step is
currently guarded by if: always(), which runs even when the earlier merge step
failed; change its condition to only run on merge success by replacing if:
always() with a success check against the merge step (e.g., if:
steps.merge.outcome == 'success' or if: steps["merge-integration"].outcome ==
'success' depending on that step's id), or use if: success() scoped to the
job/needs if the merge is in a different job (e.g., if:
needs.merge.outputs.<flag> == 'true'); ensure you reference the actual merge
step id (e.g., merge or merge-integration) when updating the condition.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 81674a52-ea99-4e50-a6da-47b688ab3d80

📥 Commits

Reviewing files that changed from the base of the PR and between f18f549 and 789dd74.

📒 Files selected for processing (9)
  • .ai/qa/tests/playwright.config.ts
  • .ai/specs/2026-04-10-ci-cd-performance.md
  • .dockerignore
  • .github/workflows/ci.yml
  • .github/workflows/qa-deploy.yml
  • .github/workflows/snapshot.yml
  • packages/cli/src/lib/testing/integration.ts
  • scripts/merge-coverage.mjs
  • turbo.json

Comment thread .github/workflows/ci.yml
Comment on lines 668 to +676
docker-build:
runs-on: ubuntu-latest
needs: test
# Only run on non-fork PRs and direct pushes (forks don't have access to GHA cache)
needs: prepare
# Only run on non-fork PRs and direct pushes (forks don't have access to GHA cache).
# Also skip for CI/docs-only PRs — no app code changed, image is unchanged.
if: |
github.event_name == 'push' ||
github.event.pull_request.head.repo.full_name == github.repository
needs.prepare.outputs.skip_integration != 'true' &&
(github.event_name == 'push' ||
github.event.pull_request.head.repo.full_name == github.repository)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Dockerfile-only changes may be incorrectly skipped.

The docker-build job is skipped when skip_integration == 'true', which is determined by whether module-level code changed. However, changes to Dockerfile, apps/docs/Dockerfile, or docker/ directory would not be detected as module changes and would skip Docker validation.

Consider adding Dockerfile paths to the FULL_SUITE_PATTERN in the prepare job, or adding a separate condition for Docker-related file changes.

💡 Option: Add Docker paths to full suite pattern

In the prepare job's integration-scope step, extend the pattern:

-          FULL_SUITE_PATTERN='^packages/shared/|^packages/ui/|^packages/events/|^packages/queue/|^packages/cache/|^packages/search/|^packages/onboarding/|^packages/webhooks/|^packages/core/src/lib/|^packages/enterprise/src/lib/|^apps/mercato/src/(app|lib|components|layout\.|page\.)'
+          FULL_SUITE_PATTERN='^packages/shared/|^packages/ui/|^packages/events/|^packages/queue/|^packages/cache/|^packages/search/|^packages/onboarding/|^packages/webhooks/|^packages/core/src/lib/|^packages/enterprise/src/lib/|^apps/mercato/src/(app|lib|components|layout\.|page\.)|^Dockerfile|^docker/|^apps/docs/Dockerfile'

Or add a separate condition in docker-build to detect Docker file changes independently.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/ci.yml around lines 668 - 676, The docker-build job is
being skipped when needs.prepare.outputs.skip_integration == 'true' even if
Dockerfiles changed; update the prepare job (integration-scope step that sets
FULL_SUITE_PATTERN / skip_integration) to include Docker-related paths (e.g.,
Dockerfile, apps/docs/Dockerfile, docker/**) in the full-suite detection, or
alternatively add a separate check in the docker-build job's if condition to
also run when Docker paths changed; specifically modify the prepare job's
pattern logic or the docker-build job's if clause so docker-build runs when
Dockerfile/docker/ changes despite skip_integration being true.

Comment on lines +3 to +5
env:
FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: 'true'

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Duplicate workflow-level env: blocks — first block is silently overwritten.

YAML does not allow duplicate keys at the same level; the second env: block (lines 38–42) overwrites the first (lines 3–4). As a result, FORCE_JAVASCRIPT_ACTIONS_TO_NODE24 is never set.

🔧 Proposed fix: Merge into a single `env:` block
 name: Deploy to Dokploy QA
 
 env:
   FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: 'true'
-
-on:
-  workflow_dispatch:
-...
-
-env:
   REGISTRY: ghcr.io
   IMAGE_NAME: ${{ github.repository }}
   DOCKERFILE: ./docker/preview/Dockerfile
   BUILD_CONTEXT: .
+
+on:
+  workflow_dispatch:

Also applies to: 38-42

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/qa-deploy.yml around lines 3 - 5, There are duplicate
workflow-level env: blocks so the later env block overwrites the earlier one and
FORCE_JAVASCRIPT_ACTIONS_TO_NODE24 is never set; fix by merging both env: blocks
into a single top-level env: mapping and ensure
FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: 'true' is included alongside the other
environment variables (i.e., consolidate the env key so both the existing
variables from the second env block and FORCE_JAVASCRIPT_ACTIONS_TO_NODE24
appear under one env:).

setup-buildx-action@v3 → @v4 (latest, released 2026-03-05)
build-push-action@v6 → @v7 (latest, released 2026-04-10)

develop already had the correct versions; the previous commit
incorrectly downgraded them.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
yokoszn pushed a commit that referenced this pull request Jun 16, 2026
…ocale (carry-forward of open-mercato#1730) (open-mercato#1781)

* feat(auth,ui): sidebar customization page with variants, DnD, cross-locale

Adds /backend/sidebar-customization with multi-variant CRUD, role apply,
DnD reorder of items inside groups (persisted itemOrder), cascade hide
for parent items, and a friendly add-new dialog. Variants and preferences
are now scoped per (user, tenant) — locale dropped from unique constraints
with a dedupe migration. Search input migrated to DS Input primitive,
section sidebar styling synced with main sidebar (active marker visible
again after removing inner overflow on the section scroll container).
Init effect drops the cancelled flag so React Strict Mode in dev cannot
abort the first init pass and leave the editor stuck on the loading
skeleton.

Adds 5 integration tests TC-AUTH-034..038 covering variant CRUD,
duplicate-name 409 with friendly error, soft-delete + recreate (partial
unique index regression guard), itemOrder round-trip, and add-new
dialog UI flow.

* refactor(ui): sticky sidebar with scroll affordance + settings search + DS layout polish

- Aside is now sticky (lg:sticky lg:top-0 lg:h-svh) with a hidden scrollbar
  (.scrollbar-hide utility) and a gradient-fade chevron at the bottom that
  flips 180 degrees when the user reaches the end of the scroll.
- Settings sidebar gets the DS Input search (mirrors main sidebar) with
  query-based item filtering; the redundant back-to-main link is removed.
- SectionPage moves padding from <aside> to the inner scroll container so
  the absolute active marker stays inside the padding-box (CSS clip happens
  at the padding-box edge, not outside it).
- SidebarCustomizationEditor wraps its content in <Page>/<PageBody> for
  consistency with other backend pages, replaces the role status text with
  DS Tags (info / error + AlertTriangle for "will clear preset"), and
  simplifies the init effect so React Strict Mode in dev no longer aborts
  the first init pass and leaves the editor stuck on the loading skeleton.

* chore(i18n): sync de/es translations for sidebar customization keys

Sidebar customization editor added new appShell.sidebarCustomization*
keys to en.json without backfilling de/es/pl. CI test job runs
i18n-check-sync which fails on missing keys. `yarn tsx
scripts/i18n-check-sync.ts --fix` backfills de/es with EN values as
placeholders (translation TODO), sorts en/pl. No runtime change.

* test(qa): scope getByRole textbox Search to exact match

Sidebar customization adds a settings search input with
aria-label='Search navigation'. Pre-existing tests used
getByRole('textbox', { name: 'Search' }) which matches by substring,
hitting both the sidebar input and the DataTable search input,
producing 'strict mode violation: 2 elements'.

Switch to { name: 'Search', exact: true } in the shared authUi helper
plus 8 spec files (TC-AUTH-010/011/012/013, TC-CAT-012,
TC-ADMIN-001/002/007, TC-INT-004) so the selector stays scoped to the
page-level search input that has aria-label exactly 'Search'.

* test(qa): extend TC-MSG-009 safeFill timeouts for CI shard load

The local safeFill helper used Playwright's default 5s expect timeout
and called keyboard.type immediately after click+focus+clear. Under
CI shard 9 parallel load the inline composer sometimes needs longer
than 5s to finish its mount/state-sync before the textarea is fully
ready; the typed characters then race the React commit and the value
assertion times out with Received "".

Defense in depth: assert toBeVisible + toBeEnabled before interacting
(10s each), extend toHaveValue to 60s, and bump the test-level timeout
to 180s so multiple safeFill chains plus waitForResponse fit. Same
pattern that stabilised TC-CRM-002 in open-mercato#1739.

Local: TC-MSG-009 passes in 7.9s (well under the 180s cap) with the
new timeouts.

* test(qa): use locator.fill + pre-submit reassert in TC-MSG-009

CI shard 9 trace (run 25095248488 retry #1):
- safeFill toHaveValue 60s timeout, 59 polls = textarea empty under
  parallel-shard load.
- Initial run safeFill passed but waitForResponse 180s timeout, page
  snapshot showed the inline reply textarea empty at 180s — controlled
  state had been silently dropped between safeFill (typed body) and the
  submit button click.

keyboard.type races the React state commit when MessageComposer mounts
inside MessageDetailPageClient and runs its own effects in parallel —
characters land but are immediately overwritten before the caller can
proceed. locator.fill is atomic (`element.value = …` + dispatched
input event) and removes the per-keystroke race entirely.

Two fixes:
1. safeFill switched from click + focus + Ctrl+A + Delete + keyboard.type
   to locator.fill, with toHaveValue staying as the commit gate.
2. Pre-submit re-assertion in the inline reply scenario — fail loudly
   on a state drop instead of silently sending an empty-body POST that
   the response filter rejects.

Local: TC-MSG-009 passes in 6.3s.

* fix(auth,ui): address sidebar customization review findings

Resolves the P1/P2 findings from the haxiorz auto-review on PR open-mercato#1730.

- Switch all read paths in sidebarPreferencesService and the preferences /
  variants API routes from raw em.find/em.findOne to findWithDecryption /
  findOneWithDecryption with explicit { tenantId, organizationId } scope so
  tenant data encryption helpers run consistently and the project's
  encryption-aware ORM rule holds (P1 #2).
- Wrap every write in SidebarCustomizationEditor (variant POST/PUT,
  delete, toggleActive, preferences PUT) in useGuardedMutation.runMutation
  with a stable contextId so global mutation injections (record locks,
  conflict UI) run, and surface retryLastMutation in the injection
  context. The PUT preferences sync is now error-checked: a sync failure
  flashes the save error instead of silently flashing success while the
  AppShell sidebar reads the unsynced preference (P1 #3).
- Drop the locale predicate from both nativeDelete sites in the sidebar
  preferences route (PUT clearRoleIds path + DELETE handler). Save and
  load helpers are cross-locale (unique key (role, tenantId)); filtering
  delete by locale orphaned rows created under another locale (P1 #4).
- Add the three missing AppShell search keys (searchNavPlaceholder,
  searchNavAria, searchNavClear) to en/pl/de/es, and remove seven dead
  appShell.sidebar* keys that were never referenced from any source
  file (P2 #5).

* test(qa): extend TC-CRM-007 + TC-INT-002 timeouts for CI shard 6 load

Both deal-creation specs were timing out on CI shard 6/15 with three
deterministic failures across reruns:
  - TC-CRM-007: timedOut at selectByFieldId clicking a still-disabled
    Status combobox (DictionaryEntrySelect.loading > 20s under shard load)
  - TC-INT-002: failed at toHaveURL('/customers/deals$') because Title
    was empty + "This field is required" — a late dictionary load
    re-triggered CrudForm's initialValues merge and clobbered the typed
    value before submit, so validation rejected the request

Both tests pass in ~3-8s locally in the ephemeral Docker environment
with the same code, so the regression is purely CI shard 6 parallel
load competing with 49 other specs for resources. Match the proven
TC-MSG-009 fix pattern (commit ac37d01):
  - test.setTimeout(120_000 / 180_000) per test
  - expect(combobox).toBeEnabled({ timeout: 30_000 }) before every
    selectByFieldId click — gates on dictionary load completion
  - expect(titleInput).toHaveValue(...) immediately after fill —
    atomic confirmation the controlled state has committed
  - defensive title re-fill right before submit so a late initialValues
    merge that clobbers the value still produces a valid POST
  - expect(option).toBeVisible() before option click — gates on
    Radix portal mount

Test-only change; no application code touched. Verified locally with
yarn test:integration:ephemeral on both specs — 2 passed (22.9s).

* fix(auth,ui): address Patryk review findings on sidebar customization

Resolves the High and Medium findings from the @patrykk-com review on PR open-mercato#1730.

High:
- Migration-snapshot drift on sidebar_variants: the snapshot still listed the
  legacy `sidebar_variants_user_id_tenant_id_locale_name_unique` constraint
  even though Migration20260427124900 + 20260427143311 dropped it and replaced
  it with a partial unique index `WHERE deleted_at IS NULL` (which a `@Unique`
  decorator cannot represent). Drop the @unique decorator on `SidebarVariant`
  and remove the stale snapshot entry; partial index is owned by raw SQL in
  the migration. A follow-up `yarn db:generate` now diffs cleanly. (H #1)
- Move inline zod schemas (sidebarSettingsSchema, createVariantInputSchema,
  updateVariantInputSchema, variantRecordSchema) from variants route handlers
  into `data/validators.ts` and import them in both routes. Settings shape is
  shared with `sidebarPreferencesInputSchema` so the constraint definitions
  no longer drift. (H #2)

Medium:
- Replace `as any` / `: any` across the new sidebar code with `EntityManager`
  + typed `FilterQuery`. `parsed.data.settings as any` casts are gone now
  that service signatures accept `Partial<SidebarPreferencesSettings>` (which
  matches the inferred zod type). (M #3)
- Add explicit one-line rationale on every empty-catch block in AppShell
  (localStorage / cookie blocked in private mode — non-critical) and
  SidebarCustomizationEditor (`window.dispatchEvent` with no listener —
  AppShell refreshes on next navigation). (M #4)
- Replace raw `<button>` drag handle in SortableItemRow with `<IconButton
  variant="ghost" size="sm">` and use the existing forwardRef so
  `setActivatorNodeRef` and dnd-kit listeners still wire correctly. (M #5)
- i18n hardcoded strings in SidebarPreview (`Search...`, `No groups to
  preview.`, `Drag to reorder`) — wrapped in `t(...)` and added 3 new keys
  to en/pl/de/es. (M #6)
- Switch primitive: replace inline `shadow-[0_1px_2px_rgba(10,13,20,...)`
  arbitrary-value shadow with the new `--shadow-switch-thumb` CSS custom
  property in light + dark themes (and synced into the standalone template
  globals.css). Switch now uses `shadow-switch-thumb` Tailwind utility. (M #7)
- Behavior regression for non-admin users: `requireFeatures:
  ['auth.sidebar.manage']` on the sidebar-customization page meta locked
  every non-admin user out of personal-scope customization, even though the
  variants/preferences APIs only gate role-application via that feature.
  Drop the page-level requireFeatures so any authenticated user can reach
  the page; the editor already conditionally hides "Apply to roles" via
  `canApplyToRoles` (server-checked against `auth.sidebar.manage`). (M open-mercato#8)

New tests:
- 6 unit tests in `sidebarPreferencesService.scope.test.ts` lock down the
  cross-tenant + cross-user scope guards on `loadSidebarVariant`,
  `updateSidebarVariant`, `deleteSidebarVariant`. Each test stubs
  `findOneWithDecryption` and asserts the exact `{ id, user, tenantId,
  deletedAt: null }` filter shape so a future refactor can't silently
  drop the user or tenant filter. (M open-mercato#9)

All 405 core test suites (3,329 tests) and 71 UI test suites (363 tests)
pass; build:packages clean across 18 packages.

* fix(auth): align sidebar preferences snapshot with partial unique indexes

UserSidebarPreference and RoleSidebarPreference still carried @unique
decorators that included locale, so the MikroORM snapshot kept the old
locale-scoped unique constraints even though Migration20260427143311
replaced them with partial unique indexes scoped to live rows. The next
yarn db:generate would have emitted a fixup migration trying to drop a
constraint already gone and add one colliding with the partial index.

Mirror the SidebarVariant approach: drop the @unique decorators
(partial indexes can't be expressed via the decorator), document the
ownership in raw SQL, and remove the stale unique entries from the
snapshot so it reflects the post-143311 state. yarn db:generate now
reports auth: no changes.

* test(auth,ui): add SidebarCustomizationEditor unit smoke test

The spec at .ai/specs/2026-04-27-ds-sidebar-customization-page.md
required SidebarCustomizationEditor.test.tsx covering load/save/cancel
flows, error states, role-apply target rendering, and drag-handle DOM
presence. Service-layer scope guards and Playwright integration tests
already shipped, but the editor's React state transitions had no unit
coverage. Adds a 5-test smoke suite that mocks apiCall/flash/injection
and asserts: skeleton-before-data, drag handles after load, load-error
surfacing on 500, role-apply targets when canApplyToRoles=true, and
that the role list is hidden when canApplyToRoles=false.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

---------

Co-authored-by: zielivia <zielivia@gmail.com>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
yokoszn pushed a commit that referenced this pull request Jun 16, 2026
…09, bulk validation, i18n (open-mercato#2303, open-mercato#2304, open-mercato#2305) (open-mercato#2309)

* feat(staff): add timesheets backend — SPEC-069 Phase 1 (Steps 1-5)

Implements the backend foundation for timesheets inside the staff module:

- ACL: 7 new feature flags (view, manage_own, manage_all, projects.view/manage, approve, lock)
- Data: 4 entities (TimeEntry, TimeEntrySegment, TimeProject, TimeProjectMember) with indexes
- Validators: 9 Zod schemas including bulk save (max 200 entries)
- Commands: 8 undoable commands (CRUD entries, CRUD projects, assign/unassign members)
- Events: 8 new events (CRUD + timer_started/stopped)
- API: 8 route files (entries CRUD, bulk save, timer start/stop, segments, projects CRUD, employee assignments)
- Search: TimeProject indexed (name, code, description, type, cost_center)
- Interceptor: self-scope enforcement on dashboard widget data for non-admin users
- Utility: shared staffMemberResolver (userId → staffMemberId)

Note: TimeEntry.date uses native date type (not text) per pre-implementation analysis finding.
UI pages, dashboard widgets, and i18n keys to follow in subsequent commits.

* feat(staff): add timesheets UI — SPEC-069 Phase 1 (Steps 6-7)

- My Timesheets monthly grid with 3-query N+1 mitigation (my-projects endpoint)
- Projects admin: list, create, detail with collapsible employee cards + Add Employee modal
- Feature gating: employee sees read-only views, admin/superadmin gets full management
- Page metadata for RBAC: detail requires projects.view, create requires projects.manage
- Bug fixes: bulk route auth.sub, segment route imports, employees route URL parsing
- Fix sticky column transparency in grid (bg-muted/50 → bg-muted)

* fix(staff): fix timer widget and route params — SPEC-069 Phase 1 (Step 8)

Fix timer detection (check startedAt/endedAt instead of non-existent
timer_running field), fix URL param extraction for segments routes,
fix create response field name, add dashboard widgets (Hours by Project
+ Time Reporting), analytics config, self-scope enforcement, i18n keys,
and project page metadata.

* test(staff): add timesheets integration tests — SPEC-069 Phase 1 (Step 9)

8 Playwright tests covering time entry CRUD, timer start/stop,
segments, projects, bulk save, dashboard widget data with self-scope
enforcement, and UI smoke tests for grid, projects list, and widgets.

* refactor(staff): update analytics date type and improve segment creation

- Changed the date field type in analytics configuration from 'date' to 'timestamp'.
- Refactored segment creation in time entries and timer start routes to use a segmentData object for better readability and maintainability.
- Updated error handling in the timesheets page to correctly reference the response object.
- Enhanced time entry and project commands to ensure proper type handling for source and status fields.
- Added optional fields for startedAt and endedAt in the time entry validation schema.
- Expanded i18n files with new keys for timesheet functionality in multiple languages.

* feat(staff): enhance time entry ownership validation and improve timesheet routes

- Added ownership validation for time entries in POST and PATCH routes to ensure users can only manage their own entries.
- Integrated `getStaffMemberByUserId` function to retrieve staff member details based on user ID.
- Updated error handling to return appropriate responses for unauthorized access and entry not found scenarios.
- Modified bulk route to include `staffMemberId` in the query for better data integrity.
- Refactored timesheets page state management to improve handling of entry data and dirty state tracking.

* fix(tapestry): update response field names and enhance timesheet integration tests

- Changed response field names in time project and employee assignment routes for consistency.
- Enhanced timesheet integration tests to include setup and teardown for project and employee assignments.
- Added assignedStartDate to employee assignment fixture for better tracking of assignment dates.

* test(staff): skip dashboard widget visibility test due to missing DB entries

- Updated the TC-STAFF-022 integration test to skip the widget visibility check for Time Reporting and Hours by Project due to the absence of required dashboard_role_widgets DB entries in setup.ts.
- Noted that the test has been manually verified and will be re-enabled once the setup includes the necessary widget IDs in the default role configuration.

* fix(staff): address PR review — CrudForm, DataTable filters, UX polish

- Use CrudForm for project create/edit with shared projectFormConfig
- Add separate edit page with version history and delete support
- Replace inline editing with link to edit page
- Status filter via DataTable filters (not custom buttons)
- Empty state with "+ Add first project" action button
- Profile link points to self-service /staff/profile/create
- Fix response field names (timeProjectId, timeProjectMemberId)

* feat(staff): enhance MyTimesheetsPage with project management features

- Added state management for project management access.
- Implemented API call to check if the user can manage projects.
- Updated empty state messaging to provide context for users without assigned projects, differentiating between admin and employee roles.
- Enhanced UI with buttons for creating and viewing projects based on user permissions.

* feat(i18n): update translations for timesheet project management

- Added new keys for project management features in German, English, Spanish, and Polish.
- Enhanced messaging for users without assigned projects, including admin-specific instructions.
- Updated UI labels and error messages to improve clarity and user experience.

* feat(staff): add timesheets UX enhancements spec

* feat(staff): add weekly view, calendar picker, list view and UX improvements

- Weekly grid as default view with Mon-Sun columns
- Toggle between Weekly/Monthly view modes
- Calendar week picker dropdown with "This week"/"Last week" shortcuts
- List view showing entries grouped by day
- View switcher component (Timesheet/List view)
- Compact grid cells with numeric-only input validation
- Fix cross-month week/monthly toggle bug
- Auto-assign project creator on creation
- Fix 403 redirect for employee on project detail
- No full page reload on navigation (opacity fade instead)

* feat(staff): timesheets UX fixes for hackathon

- Timer: add missing staffMemberId to create payload (fixes "Failed to start timer")
- AddRowDropdown: rewrite with createPortal so dropdown overlays without layout shift or inner scroll
- Move Add row into table tbody (above Daily Total row)
- CreateProjectDialog: add embedded prop to CrudForm to hide duplicate FormHeader (single Create button now)
- i18n: remove leading "+" from addRow.trigger and addRow.createProject (Plus icon already renders the symbol) across en/de/es/pl
- Reorganize timesheet UI components from backend/staff/timesheets/components to lib/timesheets-ui

* feat(staff): persist grid membership with show_in_grid + remove row in My Timesheets

Adds show_in_grid column on staff_time_project_members (+ backfill), new
self-service PATCH endpoint, and X remove button with confirm dialog.
Closes a gap in the original UX spec where "+ Add row" was only local state.

* feat(staff): add project colors, sidebar timer indicator, and inline list descriptions

- Phase 3: color field (varchar 20) on staff_time_projects with 12-color palette
- ColorPicker component, ProjectColorDot rendered in grid/AddRow/Timer/ListView
- Auto-fallback color from project name hash (djb2)
- Sidebar timer indicator widget with pulsing dot, persists via sessionStorage
- Inline editable descriptions in ListView (click to edit, Enter/blur to save)
- show_in_grid column on staff_time_project_members with backfill migration
- Self-service PATCH /api/staff/timesheets/my-projects/{projectId} endpoint
- X button to remove rows from grid with confirm dialog
- Unit tests for colors.ts (13 passing)
- i18n keys for 4 languages (en/pl/es/de)

* fix(staff): show add row button when user has assignments but empty grid

Switch the empty state condition from projects.length to allAssignedProjects.length
so users who haven't opted any project into their grid still see the "+ Add row"
control instead of being stuck on the "create a project" screen.

* fix(staff): resolve DELETE employee from time project returning 400

The mapInput for the employees DELETE action read `raw` directly, but the
CRUD factory passes `raw = { body, query }` — so the id coming in the query
string never reached the zod parser. Align with the customers/people pattern:
read id from parsed.body.id ?? parsed.id ?? parsed.query.id ?? URL search params.

* feat(staff): add timesheets projects portfolio view (Phase A + B)

Redesign /backend/staff/timesheets/projects into a role-aware portfolio
with table and cards view modes. PM sees team-wide aggregates; Collaborator
sees personal hours scoped via mine=1 filter.

Backend
- New aggregate helpers: computeProjectsKpis, computeProjectHoursTrend,
  listProjectMembersPreview + 26 unit tests
- New GET /api/staff/timesheets/projects/kpis endpoint with role-aware
  PM/Collab response shapes (openApi + zod schemas)
- New response enricher staff.timesheets-projects-portfolio targeting
  staff:staff_time_project — adds _staff.{hoursWeek, hoursTrend,
  members, memberCount, myRole} via batched SQL (no N+1)
- Extend /api/staff/timesheets/time-projects with mine=1 filter and
  include query param

UI
- ProjectsKpiStrip (5 PM cards / 3 Collab cards with delta badges)
- SavedViewTabs (status + Mine, URL-synced)
- ViewModeToggle + useProjectsViewMode (localStorage persisted)
- ProjectCard, ProjectsCards grid (3 col)
- HoursSparkline (SVG, 7 weeks, theme-aware)
- ProjectMembersAvatarStack (max 4 + N overflow, dark mode palette)
- Enriched table columns: color dot, status badge, team/role, sparkline,
  relative updated-at
- Inline refresh (no skeleton flash on filter/search)
- Dark mode tokens across all new components + fix project name in
  My Timesheets grid

i18n: 31+ new staff.timesheets.projects.portfolio.* keys in
en/de/pl/es (DE/PL/ES placeholders pending translation)

Spec: .ai/specs/2026-04-24-timesheets-projects-portfolio-view.md
Pre-impl analysis: .ai/specs/analysis/

Lessons: QueryEngine doesn't support $or top-level filters; CRUD
factory's enricher feature gating expects rbac.getGrantedFeatures()
which RbacService doesn't expose — so enrichers with features array
are silently skipped. Routed ACL via route metadata + inline manage
check instead.

* fix(staff): prevent cross-employee time-entry leak on GET endpoint

Add self-scope interceptor for GET /api/staff/timesheets/time-entries that forces staffMemberId to the caller's own staff member when the user lacks staff.timesheets.manage_all (or staff.* wildcard).

Mirrors the existing self-scope pattern used by the dashboard widget endpoint. Extracts the wildcard ACL check into a shared helper to avoid duplication between both interceptors.

Fixes review finding #1 on PR open-mercato#1111.

* fix(staff): enforce unique constraint on project code and member assignment

MikroORM v6 silently drops the unique flag when the @Index options bag also carries a where clause. The original migration emitted plain 'create index' statements instead of 'create unique index', allowing duplicate project codes within an (org, tenant) and duplicate active assignments of the same staff member to one project.

Add a fix-up migration that drops the affected indexes and recreates them as partial unique indexes (where deleted_at is null) so reuse of codes after soft-delete keeps working.

Affected indexes: staff_time_projects_code_unique_idx, staff_time_project_members_unique_idx.

Fixes review finding #2 on PR open-mercato#1111.

* fix(dashboards): remove cross-module staff coupling from widgets/data route

The route was importing StaffTeamMember from the staff module and inlining self-scope enforcement for staff:staff_time_entries entityType, violating the architectural rule against cross-module ORM coupling.

Replace the inline check with a proper invocation of runApiInterceptorsBefore. The interceptor 'staff.timesheets.self-scope-widget-data' already declared in staff/api/interceptors.ts now runs effectively (until now it was registered but never invoked because custom routes do not auto-run interceptors).

Side effects: the route is now open to interceptor injection from any module, not just staff. The staff interceptor itself was not changed.

Fixes review finding #3 on PR open-mercato#1111.

* fix(staff): make bulk time-entries save atomic

Wrap the create/update/soft-delete loop in em.transactional so the whole batch is committed or rolled back as a unit. A mid-loop failure no longer leaves the database in a partial state.

Also move the existingEntries lookup inside the transaction to avoid a read-modify-write race between fetching current rows and applying mutations.

Fixes review finding #4 on PR open-mercato#1111.

* fix(staff): emit CRUD side effects from bulk time-entries save

The bulk endpoint mutated entities directly inside a single em.flush(), so the highest-traffic write path was silently skipping the staff.timesheets.time_entry.created/updated/deleted events, query index updates, and cache invalidation that the per-row commands provide.

Collect a per-row action log inside the transaction (created/updated/deleted), then after the transaction commits dispatch emitCrudSideEffects for each entity and flushCrudSideEffects once at the end. Events fire only after the DB changes are durable, per the side-effects guideline in core/AGENTS.md.

Fixes review finding #5 on PR open-mercato#1111.

* fix(staff): wire mutation guards into custom write routes

AGENTS.md requires every non-makeCrudRoute write to call validateCrudMutationGuard before mutating and runCrudMutationGuardAfterSuccess after success so record locks, conflict detection, and ACL-driven mutation policies actually fire. The six timesheets custom write routes shipped without it.

Add a thin staff/api/guards.ts helper around runMutationGuards + bridgeLegacyGuard (mirrors integrations/api/guards.ts) and wire it into:

- time-entries/bulk (POST) - update on staff.timesheets.time_entry

- time-entries/[id]/timer-start (POST) - update on staff.timesheets.time_entry

- time-entries/[id]/timer-stop (POST) - update on staff.timesheets.time_entry

- time-entries/[id]/segments (POST) - create on staff.timesheets.time_entry_segment

- time-entries/[id]/segments/[segmentId] (PATCH) - update on staff.timesheets.time_entry_segment

- my-projects/[projectId] (PATCH) - update on staff.timesheets.time_project_member

Each route now blocks on guard rejection (422 with the guard body) and dispatches afterSuccess callbacks after the flush succeeds.

Fixes review finding #6 on PR open-mercato#1111.

* fix(staff): drop vitest import from colors test

The colors test imported describe/it/expect from vitest, which is not a dependency, so the test could not run. Drop the import and rely on jest globals like the rest of the module.

Fixes review finding #7 on PR open-mercato#1111.

* fix(staff): extract pure helpers so unit tests can run

computeProjectsKpis and listProjectMembersPreview both imported MikroORM entities at module top-level, so the unit tests added for SPEC-069 Step 9 transitively loaded @mikro-orm/core ESM and exploded under the jest preset. The tests never executed.

Move the pure helpers into their own files that don't import entities:

- timesheets-projects/kpiMath.ts: deltaPct, minutesToHours

- timesheets-projects/initials.ts: computeInitials

Update the helper test files to import from the new pure modules. computeProjectsKpis and listProjectMembersPreview now import (and re-export computeInitials) from the new files so callers keep working.

Result: 26/26 helper tests now pass.

Fixes review finding open-mercato#8 on PR open-mercato#1111.

* fix(staff): wire timesheets page writes through useGuardedMutation

The My Timesheets page is a custom backend page (not a CrudForm), so its four write call sites (bulk time-entries POST and three my-projects PATCH calls) bypassed the global mutation injection hooks. Record-lock conflict handling, scoped request headers, and the standard onBeforeSave/onAfterSave hooks never fired.

Wrap each write in runMutation({ operation, context, mutationPayload }) so global injection modules can run their before/after hooks and consume mutation errors consistently. Each call site passes a stable resourceKind plus the project or staff member id as resourceId.

Fixes review finding open-mercato#9 on PR open-mercato#1111.

* fix(staff): use readJsonSafe consistently in timesheets routes

Three timesheets write routes still read JSON via req.json().catch(...) while the rest of the module already adopted readJsonSafe (see my-projects/[projectId]). Pick the conventional helper everywhere.

Affected:

- time-entries/bulk (POST)

- time-entries/[id]/segments (POST)

- time-entries/[id]/segments/[segmentId] (PATCH)

Fixes review finding open-mercato#10 on PR open-mercato#1111.

* fix(staff): use resolveOrganizationScopeForRequest in segment PATCH

Every peer timesheets route resolves the active scope via resolveOrganizationScopeForRequest so org switching in multi-org tenants works. The segment PATCH was reading auth.tenantId/auth.orgId directly, so requests sent from a non-default organization landed on the wrong scope.

Reorder the handler to create the container before validating scope, then derive tenantId/organizationId from the resolver with the auth values as fallback, mirroring the bulk and timer routes.

Fixes review finding open-mercato#11 on PR open-mercato#1111.

* fix(staff): tighten projectId UUID validation in my-projects PATCH

The old /^[0-9a-f-]{36}$/i regex accepts any 36-character mix of hex and dashes (e.g. 36 dashes), so junk ids slipped through to the DB query. Replace it with z.string().uuid() so only well-formed UUIDs are accepted.

Fixes review finding open-mercato#12 on PR open-mercato#1111.

* fix(staff): use apiCallOrThrow for timesheets writes

Mixed patterns inside the same files: some writes used apiCallOrThrow / readApiResultOrThrow while others called apiCall and checked res.ok manually. Pick the convention so server error bodies propagate uniformly through raiseCrudError instead of a hand-rolled 'throw new Error(await res.response.text())'.

Updated:

- backend/staff/timesheets/page.tsx — 4 writes inside runMutation (bulk save + 3 my-projects PATCH)

- lib/timesheets-ui/TimerBar.tsx — 3 writes (time-entries POST, timer-start, timer-stop) consolidated into try/catch so the flash message paths are unchanged

Read-only GETs that use a fallback on failure stay on apiCall.

Fixes review finding open-mercato#13 on PR open-mercato#1111.

* fix(staff): sort i18n keys to satisfy i18n:check-sync

yarn i18n:check-sync was failing on the four staff locale files (en/pl/es/de) with 'unsorted keys'. Run --fix to reorder; no key or value content changes.

Fixes review finding open-mercato#14 on PR open-mercato#1111.

* test(staff): regression guard for time-entries self-scope leak

Spec §Security calls out the self-scope rule but no integration test covered it. Add TC-STAFF-023 so the GET cross-employee leak fixed in a7704babd cannot regress unnoticed.

The test logs in as admin to create a time entry owned by the admin's own staff member, then logs in as employee (manage_own only, no manage_all) and issues GET /api/staff/timesheets/time-entries?staffMemberId=<admin's id>. It asserts the admin entry never appears and that every returned row belongs to the employee — proof the staff/api interceptor rewrote the filter to the caller's own staff member id.

Self-contained: creates project + assignment + entry in setup, cleans up in finally.

Fixes review finding open-mercato#15 on PR open-mercato#1111.

* fix(staff): seed timesheets dashboard widgets into role defaults

The Time Reporting and Hours by Project widgets ship with defaultEnabled:false so the global dashboard seed never associated them with any role. Existing tenants ended up with the widgets installed but invisible — and TC-STAFF-022 had to skip in CI because nothing would render.

Wire staff/setup.ts seedDefaults to call appendWidgetsToRoles for superadmin, admin, and employee with both timesheets widget ids. appendWidgetsToRoles is idempotent and only adds missing ids, so re-running setup on existing tenants is safe. The dashboards module sits before staff in modules.ts, so the DashboardRoleWidgets rows it creates already exist when staff's seed runs.

Update TC-STAFF-022's skip comment to explain the seed is now in place; remove the skip once CI runs against a freshly-seeded tenant.

Fixes review finding open-mercato#16 on PR open-mercato#1111.

* docs(spec): clarify TimeProject.customer_id is optional

The Data Models table and Projects API contract both marked customer_id as required, but the actual entity and validator have always treated it as nullable — internal projects have no customer. The spec was the inconsistent side.

Update line 259 (Data Models) and line 421 (Create/Update fields) to call out the column as optional, and log the doc fix in the changelog.

Fixes review finding open-mercato#18 on PR open-mercato#1111.

* docs(spec): rename SPEC-069 file to date+slug convention

.ai/specs/AGENTS.md mandates {date}-{title}.md filenames and forbids new SPEC- prefixes; the timesheets spec was the lone outlier in this PR's surface. git mv preserves history, the README link is updated, and a changelog entry is added inside the spec. Textual references to 'SPEC-069' stay as a human identifier.

Fixes review finding open-mercato#19 on PR open-mercato#1111.

* docs(spec): log SPEC-069 filename normalization in changelog

* chore(staff): regenerate snapshot and lucide registry after develop rebase

* fix(staff): handle duplicate project code with 409 and validate timeProjectId in bulk save (closes open-mercato#2304)

* fix(staff): i18n timesheets relative time and aria-labels, add seed-timesheets-widgets CLI command (closes open-mercato#2305)

* fix(staff): use wildcard-aware hasFeature for manage_all ACL check (closes open-mercato#2303)

* fix(staff): load ACL via rbacService when JWT lacks features, sort i18n keys alphabetically

* fix(staff): enforce time-entry ownership on writes and emit timer lifecycle events (H-1, H-2)

* fix(staff): timesheets follow-up — M-1..M-4 from review (assign side-effects, indexer plumbing, ref validation, bulk stale-id 422)

---------

Co-authored-by: migsilva89 <migdrum@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant