feat(eslint): add boundaries/dependencies rule for frontend domain imports #429

Merged

marcel merged 8 commits from feat/issue-410-eslint-boundary-rule into main 2026-05-05 18:09:26 +02:00

Conversation 8 Commits 8 Files Changed 20 +918 -91

marcel commented 2026-05-05 17:40:17 +02:00

Owner

Copy Link

Summary

• Installs eslint-plugin-boundaries@6.0.2 and eslint-import-resolver-typescript@4.4.4
• Adds boundaries/dependencies rule to frontend/eslint.config.js with one element type per Tier-1 domain and an explicit allow-list encoding the architectural dependency graph
• Fixes three structural misplacements found during the cleanup: statusDotClass/statusLabel moved to document domain, FieldLabelBadge moved to shared/primitives, MissionControlStrip moved to document domain
• Adds permanent negative fixture (src/lib/tag/__fixtures__/cross-domain.fixture.ts) and lint:boundary-demo script to demonstrate the rule fires
• Documents the rule, allow-list, and escape hatches in COLLABORATING.md

Dependency graph encoded by the rule

Domain	May import from
document	shared, person, tag, ocr, activity, conversation
geschichte	shared, person, document
ocr	shared, document
activity	shared, notification
person, tag, user, notification, conversation	shared only
routes	any domain

Default: disallow — any unlisted cross-domain import is a lint error.

Two eslint-disable-next-line comments remain in shared/discussion/ where person-domain helpers are needed to render participant metadata inline; moving them would lose the person-type context.

Test plan

• npm run lint passes (fixture excluded via --ignore-pattern)
• npm run lint:boundary-demo exits 1 with a boundaries/dependencies error
• npm run test shows the same pre-existing failures and no regressions
• npm run check passes (svelte-check)

Closes #410

## Summary - Installs `eslint-plugin-boundaries@6.0.2` and `eslint-import-resolver-typescript@4.4.4` - Adds `boundaries/dependencies` rule to `frontend/eslint.config.js` with one element type per Tier-1 domain and an explicit allow-list encoding the architectural dependency graph - Fixes three structural misplacements found during the cleanup: `statusDotClass`/`statusLabel` moved to document domain, `FieldLabelBadge` moved to `shared/primitives`, `MissionControlStrip` moved to document domain - Adds permanent negative fixture (`src/lib/tag/__fixtures__/cross-domain.fixture.ts`) and `lint:boundary-demo` script to demonstrate the rule fires - Documents the rule, allow-list, and escape hatches in `COLLABORATING.md` ## Dependency graph encoded by the rule | Domain | May import from | |---|---| | `document` | `shared`, `person`, `tag`, `ocr`, `activity`, `conversation` | | `geschichte` | `shared`, `person`, `document` | | `ocr` | `shared`, `document` | | `activity` | `shared`, `notification` | | `person`, `tag`, `user`, `notification`, `conversation` | `shared` only | | `routes` | any domain | Default: `disallow` — any unlisted cross-domain import is a lint error. Two `eslint-disable-next-line` comments remain in `shared/discussion/` where person-domain helpers are needed to render participant metadata inline; moving them would lose the person-type context. ## Test plan - [ ] `npm run lint` passes (fixture excluded via `--ignore-pattern`) - [ ] `npm run lint:boundary-demo` exits 1 with a `boundaries/dependencies` error - [ ] `npm run test` shows the same pre-existing failures and no regressions - [ ] `npm run check` passes (svelte-check) Closes #410

marcel added 6 commits 2026-05-05 17:40:18 +02:00

chore(deps): install eslint-plugin-boundaries and add boundary lint scripts ... b9db36b644

Adds eslint-plugin-boundaries@6.0.2 and eslint-import-resolver-typescript@4.4.4
as pinned devDependencies. Also adds the lint:boundary-demo script for running
the ESLint boundaries rule against the fixture file, and updates the lint script
to exclude __fixtures__ directories.

Refs #410
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

refactor(document): move statusDotClass and statusLabel to document domain ... 73a199b54c

These functions describe DocumentStatus display logic (dot colours, readable
labels) and belong in the document domain. They were incorrectly placed in
personFormat.ts. Moving them to documentStatusLabel.ts removes the
person → document dependency and prepares the codebase for the
boundaries/dependencies ESLint rule.

Refs #410
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

refactor(shared): move FieldLabelBadge primitive to shared/primitives ... 9f2ba4b4ee

FieldLabelBadge is a generic UI primitive (additive/replace badge used in form
field labels). It lived in the document domain but was already imported by
PersonTypeahead (person domain), creating a person → document coupling.
Moving it to shared/primitives eliminates that cross-domain dependency.

Refs #410
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

refactor(document): move MissionControlStrip to document domain ... 807810953c

MissionControlStrip is a document-processing pipeline visualiser — it
imports document-domain components (SegmentationColumn, TranscriptionColumn,
ReadyColumn) and belongs in the document domain. It was placed in
shared/dashboard, creating a shared → document coupling that the upcoming
boundaries rule would block.

Refs #410
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

feat(eslint): add boundaries/dependencies rule preventing cross-domain imports ... 09e0fd8fa2

Adds eslint-plugin-boundaries with one element type per Tier-1 domain and an
explicit allow-list encoding the architectural dependency graph:
- document may import from: shared, person, tag, ocr, activity, conversation
- geschichte may import from: shared, person, document
- ocr may import from: shared, document
- activity may import from: shared, notification
- all others (person, tag, user, notification, conversation): shared only
- routes may import from any domain

Default is 'disallow', so any unlisted cross-domain import is an error.
Two eslint-disable-next-line comments remain in shared/discussion where
person-domain helpers (getInitials, formatLifeDateRange) are needed to render
participant metadata; moving them to shared would lose the person-type context.

Closes #410
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

docs: add boundary violation fixture and document rule in COLLABORATING.md ... ef55ad7e73

Adds src/lib/tag/__fixtures__/cross-domain.fixture.ts — a permanent fixture
that demonstrates the boundaries rule firing on a tag → person import. The
fixture is excluded from npm run lint via --ignore-pattern; run
npm run lint:boundary-demo to see it produce an error (exit 1).

Documents the full allow-list, the escape hatches ($lib/shared/ move, explicit
rule entry, eslint-disable-next-line), and the verify command in COLLABORATING.md.

Refs #410
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel referenced this pull request 2026-05-05 17:40:36 +02:00

refactor(frontend): add ESLint rule preventing cross-domain imports #410

marcel commented 2026-05-05 17:50:01 +02:00

Author

Owner

Copy Link

🏛️ Markus Keller (@mkeller) — Application Architect

Verdict: ✅ Approved

This is exactly the kind of structural enforcement that pays for itself over time. You've taken a fuzzy convention ("domains should not cross-reference each other") and made it machine-enforceable. The dependency graph encoded in the allow-list is the right graph — it mirrors the backend's layering rules and the decisions that were made during the domain design.

What's done well

• default: 'disallow' is the correct default. Opt-in boundaries fail closed — any new domain starts isolated until an explicit cross-domain dependency is justified and documented.
• The three structural cleanups (statusDotClass/statusLabel → document domain, FieldLabelBadge → shared/primitives, MissionControlStrip → document) are exactly right. These were misplacements that the rule correctly surfaced, and the fix is to move the code, not to add an exception.
• The two eslint-disable-next-line comments in shared/discussion/ are handled honestly. Both comments explain the architectural reasoning ("person initials for avatars", "person date formatting"), and both note the path forward ("extract to shared if it becomes reusable"). This is the right use of the escape hatch.
• The lint:boundary-demo + cross-domain.fixture.ts approach is a clever way to verify the rule actually fires. A lint rule that silently never fires is worse than no rule.
• COLLABORATING.md documentation closes the loop: the rule's intent, the allowed graph, and three explicit resolution strategies. Future contributors have what they need.

Blockers

None.

Suggestions

• $lib/paraglide and $lib/generated are not in the boundaries/elements list. These are cross-cutting imports that appear in nearly every domain. That's fine — files outside the element list are treated as "external" by the plugin and are not subject to the boundary rule. But it's worth a single-line comment in eslint.config.js documenting this intentional choice (// paraglide and generated/api are external — not domain-owned), so the next reader doesn't wonder whether they were forgotten.
• DropZone.svelte lives in src/routes/ and imports from local route files. With the routes element type defined, DropZone.svelte is inside the routes boundary and can import from any domain. That's correct. Just confirm in your test run that route-internal imports (e.g., ./DropZone.svelte from +page.svelte) don't trigger false positives under the rule — relative intra-route imports should be fine since both files are in the routes type.
• The personFormat.ts cleanup removes formatDocumentStatus as a re-export from the person domain. This is the right call — person depending on document was a boundary violation in the opposite direction of what you'd expect. Good catch.

## 🏛️ Markus Keller (@mkeller) — Application Architect **Verdict: ✅ Approved** This is exactly the kind of structural enforcement that pays for itself over time. You've taken a fuzzy convention ("domains should not cross-reference each other") and made it machine-enforceable. The dependency graph encoded in the allow-list is the right graph — it mirrors the backend's layering rules and the decisions that were made during the domain design. ### What's done well - **`default: 'disallow'`** is the correct default. Opt-in boundaries fail closed — any new domain starts isolated until an explicit cross-domain dependency is justified and documented. - **The three structural cleanups** (`statusDotClass`/`statusLabel` → document domain, `FieldLabelBadge` → shared/primitives, `MissionControlStrip` → document) are exactly right. These were misplacements that the rule correctly surfaced, and the fix is to move the code, not to add an exception. - **The two `eslint-disable-next-line` comments** in `shared/discussion/` are handled honestly. Both comments explain the architectural reasoning ("person initials for avatars", "person date formatting"), and both note the path forward ("extract to shared if it becomes reusable"). This is the right use of the escape hatch. - **The `lint:boundary-demo` + `cross-domain.fixture.ts` approach** is a clever way to verify the rule actually fires. A lint rule that silently never fires is worse than no rule. - **COLLABORATING.md documentation** closes the loop: the rule's intent, the allowed graph, and three explicit resolution strategies. Future contributors have what they need. ### Blockers None. ### Suggestions - **`$lib/paraglide` and `$lib/generated` are not in the `boundaries/elements` list.** These are cross-cutting imports that appear in nearly every domain. That's fine — files outside the element list are treated as "external" by the plugin and are not subject to the boundary rule. But it's worth a single-line comment in `eslint.config.js` documenting this intentional choice (`// paraglide and generated/api are external — not domain-owned`), so the next reader doesn't wonder whether they were forgotten. - **`DropZone.svelte` lives in `src/routes/` and imports from local route files.** With the `routes` element type defined, `DropZone.svelte` is inside the `routes` boundary and can import from any domain. That's correct. Just confirm in your test run that route-internal imports (e.g., `./DropZone.svelte` from `+page.svelte`) don't trigger false positives under the rule — relative intra-route imports should be fine since both files are in the `routes` type. - The `personFormat.ts` cleanup removes `formatDocumentStatus` as a re-export from the person domain. This is the right call — `person` depending on `document` was a boundary violation in the opposite direction of what you'd expect. Good catch.

marcel commented 2026-05-05 17:50:28 +02:00

Author

Owner

Copy Link

👨‍💻 Felix Brandt (@felixbrandt) — Fullstack Developer

Verdict: ✅ Approved

Solid refactor. The code moves are clean, the tests follow the code, and the escape hatches are explained. My TDD radar is happy — documentStatusLabel.spec.ts gains tests for statusDotClass and statusLabel when those functions move into the document domain, and personFormat.spec.ts removes them when they leave the person domain. That's the right discipline.

What's done well

• Test migration is complete. When statusDotClass/statusLabel move from personFormat.ts to documentStatusLabel.ts, the tests move with them. Zero coverage gap. The spec files are exact mirrors of the implementation ownership.
• DocumentStatus type is now exported from the domain that owns it (documentStatusLabel.ts), and DocumentStatusChip.svelte imports it from there. Previously it was duplicated inline — that was a code smell waiting to cause a divergence.
• personFormat.ts is cleaner after the surgery. Removing formatDocumentStatus import from the person domain removes a cross-domain dependency that had no business being there. personFormat now formats persons, full stop.
• FieldLabelBadge in $lib/shared/primitives/ is the right home. It's a pure presentational primitive with no domain knowledge. PersonTypeahead.svelte importing it from $lib/document/ was the wrong direction.
• The fixture file comment is clear about the contract: "must NEVER be imported by production code." Good.

Blockers

None.

Suggestions

• statusLabel in documentStatusLabel.ts is a thin wrapper:

  export function statusLabel(status: string): string {
      return formatDocumentStatus(status);
  }
  

  This function adds no value over calling formatDocumentStatus directly. The only reason it exists is to be the name that DocumentStatusChip.svelte already called. Consider whether the call sites should just call formatDocumentStatus directly and this wrapper can be deleted. One name for one thing.

• The --ignore-pattern flag in package.json is fragile. The pattern 'src/lib/**/__fixtures__/**' uses glob-style matching but ESLint CLI --ignore-pattern uses gitignore syntax. Verify that this actually excludes src/lib/tag/__fixtures__/ on the CI runner (Linux path separator). If it doesn't, npm run lint will fail on the fixture file. The safer approach is to add src/lib/**/__fixtures__/ to .eslintignore or to the ignores array in eslint.config.js.

• personFormat.spec.ts properly removes the migrated test groups (statusDotClass, statusLabel) rather than leaving them as dead tests. Appreciated — dead tests that always pass give false confidence.

## 👨‍💻 Felix Brandt (@felixbrandt) — Fullstack Developer **Verdict: ✅ Approved** Solid refactor. The code moves are clean, the tests follow the code, and the escape hatches are explained. My TDD radar is happy — `documentStatusLabel.spec.ts` gains tests for `statusDotClass` and `statusLabel` when those functions move into the document domain, and `personFormat.spec.ts` removes them when they leave the person domain. That's the right discipline. ### What's done well - **Test migration is complete.** When `statusDotClass`/`statusLabel` move from `personFormat.ts` to `documentStatusLabel.ts`, the tests move with them. Zero coverage gap. The spec files are exact mirrors of the implementation ownership. - **`DocumentStatus` type is now exported from the domain that owns it** (`documentStatusLabel.ts`), and `DocumentStatusChip.svelte` imports it from there. Previously it was duplicated inline — that was a code smell waiting to cause a divergence. - **`personFormat.ts` is cleaner after the surgery.** Removing `formatDocumentStatus` import from the person domain removes a cross-domain dependency that had no business being there. `personFormat` now formats persons, full stop. - **`FieldLabelBadge` in `$lib/shared/primitives/`** is the right home. It's a pure presentational primitive with no domain knowledge. `PersonTypeahead.svelte` importing it from `$lib/document/` was the wrong direction. - **The fixture file comment is clear** about the contract: "must NEVER be imported by production code." Good. ### Blockers None. ### Suggestions - **`statusLabel` in `documentStatusLabel.ts` is a thin wrapper:** ```typescript export function statusLabel(status: string): string { return formatDocumentStatus(status); } ``` This function adds no value over calling `formatDocumentStatus` directly. The only reason it exists is to be the name that `DocumentStatusChip.svelte` already called. Consider whether the call sites should just call `formatDocumentStatus` directly and this wrapper can be deleted. One name for one thing. - **The `--ignore-pattern` flag in `package.json` is fragile.** The pattern `'src/lib/**/__fixtures__/**'` uses glob-style matching but ESLint CLI `--ignore-pattern` uses gitignore syntax. Verify that this actually excludes `src/lib/tag/__fixtures__/` on the CI runner (Linux path separator). If it doesn't, `npm run lint` will fail on the fixture file. The safer approach is to add `src/lib/**/__fixtures__/` to `.eslintignore` or to the `ignores` array in `eslint.config.js`. - **`personFormat.spec.ts` properly removes the migrated test groups** (statusDotClass, statusLabel) rather than leaving them as dead tests. Appreciated — dead tests that always pass give false confidence.

marcel commented 2026-05-05 17:50:43 +02:00

Author

Owner

Copy Link

🔒 Nora "NullX" Steiner — Application Security Engineer

Verdict: ✅ Approved

This PR is a developer tooling / architecture change with no runtime attack surface. From a security standpoint, there is nothing alarming here. But there are two things worth noting for supply chain hygiene.

What's done well

• No new runtime dependencies — both eslint-plugin-boundaries@6.0.2 and eslint-import-resolver-typescript@4.4.4 are devDependencies. They never ship to production.
• Pinned exact versions (4.4.4, 6.0.2) rather than semver ranges, which is the right call for dev tooling that affects the build pipeline.
• The structural cleanups are a security-positive side effect: keeping domain functions in their owning domain makes permission and data-access logic easier to audit. When statusDotClass lived in personFormat.ts, a security reviewer scanning the person domain for authorization logic had to mentally filter out document-status presentation code. That noise is gone.

Blockers

None.

Suggestions

• Supply chain: audit the transitive dependency tree. The eslint-import-resolver-typescript@4.4.4 package pulls in @unrs/resolver-binding-* for 15+ platforms (linux-arm, linux-x64-gnu, linux-x64-musl, darwin-arm64, wasm32, etc.), plus @napi-rs/wasm-runtime, @emnapi/core, @emnapi/runtime. These are Rust-compiled native binaries distributed as npm packages — a class of package that has been used in supply chain attacks. Run npm audit to confirm zero CVEs, and consider adding a CI step or periodic Dependabot/Renovate check for this new dependency cluster.

• The handlebars@4.7.9 transitive dependency (pulled in through @boundaries/elements) has a history of prototype pollution and RCE vulnerabilities in older versions. 4.7.9 is the current patched release, so this is not a present vulnerability — but flag it for Renovate/Dependabot tracking so a future 4.7.x with a vuln doesn't stay unnoticed.

• These are informational observations, not blockers. The PR does the right thing architecturally.

## 🔒 Nora "NullX" Steiner — Application Security Engineer **Verdict: ✅ Approved** This PR is a developer tooling / architecture change with no runtime attack surface. From a security standpoint, there is nothing alarming here. But there are two things worth noting for supply chain hygiene. ### What's done well - **No new runtime dependencies** — both `eslint-plugin-boundaries@6.0.2` and `eslint-import-resolver-typescript@4.4.4` are `devDependencies`. They never ship to production. - **Pinned exact versions** (`4.4.4`, `6.0.2`) rather than semver ranges, which is the right call for dev tooling that affects the build pipeline. - **The structural cleanups** are a security-positive side effect: keeping domain functions in their owning domain makes permission and data-access logic easier to audit. When `statusDotClass` lived in `personFormat.ts`, a security reviewer scanning the person domain for authorization logic had to mentally filter out document-status presentation code. That noise is gone. ### Blockers None. ### Suggestions - **Supply chain: audit the transitive dependency tree.** The `eslint-import-resolver-typescript@4.4.4` package pulls in `@unrs/resolver-binding-*` for 15+ platforms (linux-arm, linux-x64-gnu, linux-x64-musl, darwin-arm64, wasm32, etc.), plus `@napi-rs/wasm-runtime`, `@emnapi/core`, `@emnapi/runtime`. These are Rust-compiled native binaries distributed as npm packages — a class of package that has been used in supply chain attacks. Run `npm audit` to confirm zero CVEs, and consider adding a CI step or periodic Dependabot/Renovate check for this new dependency cluster. - **The `handlebars@4.7.9` transitive dependency** (pulled in through `@boundaries/elements`) has a history of prototype pollution and RCE vulnerabilities in older versions. `4.7.9` is the current patched release, so this is not a present vulnerability — but flag it for Renovate/Dependabot tracking so a future `4.7.x` with a vuln doesn't stay unnoticed. - These are informational observations, not blockers. The PR does the right thing architecturally.

marcel commented 2026-05-05 17:50:51 +02:00

Author

Owner

Copy Link

🧪 Sara Holt (@saraholt) — QA Engineer & Test Strategist

Verdict: ✅ Approved

Test coverage follows the code moves correctly. The test pyramid is intact. My main observation is about the lint rule's own testability — the lint:boundary-demo script is a clever smoke test for the rule, but it has a gap.

What's done well

• Tests move with the code. documentStatusLabel.spec.ts gains statusDotClass and statusLabel test groups (5 assertions each) when those functions move into the document domain. personFormat.spec.ts removes the identical groups when they leave. No coverage gap, no orphaned tests.
• FieldLabelBadge.svelte.spec.ts moves with FieldLabelBadge.svelte — good. Test files follow their subject.
• MissionControlStrip.svelte.spec.ts moves with MissionControlStrip.svelte — consistent pattern.
• The cross-domain.fixture.ts negative test is a creative QA instrument. It proves the rule fires, not just that the rule is configured. This is the equivalent of a "test the test" discipline.
• Test names in documentStatusLabel.spec.ts use the arrow notation ('PLACEHOLDER → bg-gray-400') which reads clearly in the test reporter.

Blockers

None.

Suggestions

• The lint:boundary-demo script only validates that the rule fires on ONE violation type (tag → person). There is no automated check that ALL the disallow cases work correctly — e.g., that person → document is actually blocked, or that tag → ocr is blocked. For a foundational architectural rule, consider adding a few more __fixtures__/ cases (one per domain with an illegal cross-domain import) and documenting them. This is a "should" not a "must" — the current demo is better than nothing.

• The --ignore-pattern 'src/lib/**/__fixtures__/**' glob — Sara's concern from the QA side: does this pattern work reliably in both Linux CI and macOS dev environments? The single quotes around the pattern prevent shell expansion on Unix, which is correct. But verify the pattern actually excludes the fixture directory by running npm run lint locally and confirming it exits 0 after this PR.

• No E2E or integration test impact — this PR is purely tooling and structural. Confirmed: no changes to route files that would require new E2E coverage, and the one +page.svelte change is an import path update only.

## 🧪 Sara Holt (@saraholt) — QA Engineer & Test Strategist **Verdict: ✅ Approved** Test coverage follows the code moves correctly. The test pyramid is intact. My main observation is about the lint rule's own testability — the `lint:boundary-demo` script is a clever smoke test for the rule, but it has a gap. ### What's done well - **Tests move with the code.** `documentStatusLabel.spec.ts` gains `statusDotClass` and `statusLabel` test groups (5 assertions each) when those functions move into the document domain. `personFormat.spec.ts` removes the identical groups when they leave. No coverage gap, no orphaned tests. - **`FieldLabelBadge.svelte.spec.ts` moves with `FieldLabelBadge.svelte`** — good. Test files follow their subject. - **`MissionControlStrip.svelte.spec.ts` moves with `MissionControlStrip.svelte`** — consistent pattern. - **The `cross-domain.fixture.ts` negative test** is a creative QA instrument. It proves the rule fires, not just that the rule is configured. This is the equivalent of a "test the test" discipline. - **Test names in `documentStatusLabel.spec.ts`** use the arrow notation (`'PLACEHOLDER → bg-gray-400'`) which reads clearly in the test reporter. ### Blockers None. ### Suggestions - **The `lint:boundary-demo` script only validates that the rule fires on ONE violation type** (tag → person). There is no automated check that ALL the `disallow` cases work correctly — e.g., that `person → document` is actually blocked, or that `tag → ocr` is blocked. For a foundational architectural rule, consider adding a few more `__fixtures__/` cases (one per domain with an illegal cross-domain import) and documenting them. This is a "should" not a "must" — the current demo is better than nothing. - **The `--ignore-pattern 'src/lib/**/__fixtures__/**'` glob** — Sara's concern from the QA side: does this pattern work reliably in both Linux CI and macOS dev environments? The single quotes around the pattern prevent shell expansion on Unix, which is correct. But verify the pattern actually excludes the fixture directory by running `npm run lint` locally and confirming it exits 0 after this PR. - **No E2E or integration test impact** — this PR is purely tooling and structural. Confirmed: no changes to route files that would require new E2E coverage, and the one `+page.svelte` change is an import path update only.

marcel commented 2026-05-05 17:51:00 +02:00

Author

Owner

Copy Link

🚀 Tobias Wendt (@tobiwendt) — DevOps & Platform Engineer

Verdict: ✅ Approved

This is a frontend developer-tooling PR with no infrastructure footprint. Docker Compose, CI pipeline, and deployment topology are untouched. My review is brief.

What's done well

• Both new packages are devDependencies — they're stripped from the production bundle by Vite and never shipped to the runtime image. Zero container image size impact.
• package-lock.json is updated — the lockfile reflects the exact resolved versions, including the native binary platform packages from @unrs/resolver-binding-*. This means npm ci in CI will reproduce the exact same tree, which is the reproducible build behavior we want.
• No new environment variables, secrets, or Docker services — the operational surface is unchanged.

Blockers

None.

Suggestions

• CI lint step: Confirm that the Gitea Actions workflow runs npm run lint (which now includes the boundaries check) as part of the PR pipeline. If the current CI workflow only runs npm run test and npm run check, the boundary rule would never enforce in CI — it would only enforce locally. Check .gitea/workflows/ to confirm npm run lint is in the pipeline. If it's missing, add it.

• The lint:boundary-demo script exits 1 by design. Make sure no CI step runs this script by accident (e.g., via a wildcard npm run lint* pattern) — it would cause a spurious CI failure. The script name with :boundary-demo suffix is distinctive enough that this is unlikely, but worth a look.

• The eslint-import-resolver-typescript package pulls in 15+ optional platform-native binary packages (@unrs/resolver-binding-*). These are all optional and only the relevant one for the runner's platform gets used. On CI runners (Linux x64), only @unrs/resolver-binding-linux-x64-gnu or musl will activate. The rest are no-ops. No action needed, just awareness.

## 🚀 Tobias Wendt (@tobiwendt) — DevOps & Platform Engineer **Verdict: ✅ Approved** This is a frontend developer-tooling PR with no infrastructure footprint. Docker Compose, CI pipeline, and deployment topology are untouched. My review is brief. ### What's done well - **Both new packages are `devDependencies`** — they're stripped from the production bundle by Vite and never shipped to the runtime image. Zero container image size impact. - **`package-lock.json` is updated** — the lockfile reflects the exact resolved versions, including the native binary platform packages from `@unrs/resolver-binding-*`. This means `npm ci` in CI will reproduce the exact same tree, which is the reproducible build behavior we want. - **No new environment variables, secrets, or Docker services** — the operational surface is unchanged. ### Blockers None. ### Suggestions - **CI lint step:** Confirm that the Gitea Actions workflow runs `npm run lint` (which now includes the boundaries check) as part of the PR pipeline. If the current CI workflow only runs `npm run test` and `npm run check`, the boundary rule would never enforce in CI — it would only enforce locally. Check `.gitea/workflows/` to confirm `npm run lint` is in the pipeline. If it's missing, add it. - **The `lint:boundary-demo` script** exits 1 by design. Make sure no CI step runs this script by accident (e.g., via a wildcard `npm run lint*` pattern) — it would cause a spurious CI failure. The script name with `:boundary-demo` suffix is distinctive enough that this is unlikely, but worth a look. - The `eslint-import-resolver-typescript` package pulls in 15+ optional platform-native binary packages (`@unrs/resolver-binding-*`). These are all optional and only the relevant one for the runner's platform gets used. On CI runners (Linux x64), only `@unrs/resolver-binding-linux-x64-gnu` or `musl` will activate. The rest are no-ops. No action needed, just awareness.

marcel commented 2026-05-05 17:51:15 +02:00

Author

Owner

Copy Link

📋 Elicit — Requirements Engineer

Verdict: ✅ Approved

Reviewing this against the original issue intent (enforcing frontend domain boundaries via ESLint). The implementation delivers what was specified.

Requirement traceability check

The PR closes issue #410. Based on the PR description, the core requirements appear to have been:

1. FR: Enforce cross-domain import rules → Delivered via boundaries/dependencies at error severity with default: 'disallow'.
2. FR: Document the allowed dependency graph → Delivered in COLLABORATING.md with a table matching the allow-list in eslint.config.js.
3. FR: Fix existing violations found during rule introduction → Three structural fixes delivered (statusDotClass/statusLabel, FieldLabelBadge, MissionControlStrip).
4. FR: Provide a way to verify the rule fires → Delivered via lint:boundary-demo + cross-domain.fixture.ts.
5. FR: Document escape hatches → Three escape hatch strategies documented in COLLABORATING.md.

Blockers

None.

Suggestions

• Acceptance criterion gap: The PR description lists a test plan but doesn't show whether npm run check (svelte-check) was run. The import path changes (FieldLabelBadge moved, MissionControlStrip moved) update the TypeScript import paths in .svelte files. Svelte-check would catch any broken imports. The PR description marks this as unchecked ([ ]). Confirm this was verified before merge.

• Non-functional requirement: maintainability. The eslint.config.js allow-list is the canonical record of the architectural dependency graph. If a new domain is added to src/lib/ in the future, it will not be in the elements list and will have no boundary enforcement. A comment in eslint.config.js above the boundaries/elements array noting "add new Tier-1 domains here" would prevent this being missed. Low effort, high value for future-you.

• The shared self-import rule ({ from: { type: 'shared' }, allow: { to: { type: ['shared'] } } }) is correct — shared utilities can call each other. This was not explicitly called out in the COLLABORATING.md table. Consider adding shared | shared only as a row in the table for completeness.

## 📋 Elicit — Requirements Engineer **Verdict: ✅ Approved** Reviewing this against the original issue intent (enforcing frontend domain boundaries via ESLint). The implementation delivers what was specified. ### Requirement traceability check The PR closes issue #410. Based on the PR description, the core requirements appear to have been: 1. **FR: Enforce cross-domain import rules** → Delivered via `boundaries/dependencies` at `error` severity with `default: 'disallow'`. 2. **FR: Document the allowed dependency graph** → Delivered in `COLLABORATING.md` with a table matching the allow-list in `eslint.config.js`. 3. **FR: Fix existing violations found during rule introduction** → Three structural fixes delivered (statusDotClass/statusLabel, FieldLabelBadge, MissionControlStrip). 4. **FR: Provide a way to verify the rule fires** → Delivered via `lint:boundary-demo` + `cross-domain.fixture.ts`. 5. **FR: Document escape hatches** → Three escape hatch strategies documented in COLLABORATING.md. ### Blockers None. ### Suggestions - **Acceptance criterion gap:** The PR description lists a test plan but doesn't show whether `npm run check` (svelte-check) was run. The import path changes (FieldLabelBadge moved, MissionControlStrip moved) update the TypeScript import paths in `.svelte` files. Svelte-check would catch any broken imports. The PR description marks this as unchecked (`[ ]`). Confirm this was verified before merge. - **Non-functional requirement: maintainability.** The `eslint.config.js` allow-list is the canonical record of the architectural dependency graph. If a new domain is added to `src/lib/` in the future, it will not be in the elements list and will have no boundary enforcement. A comment in `eslint.config.js` above the `boundaries/elements` array noting "add new Tier-1 domains here" would prevent this being missed. Low effort, high value for future-you. - **The `shared` self-import rule** (`{ from: { type: 'shared' }, allow: { to: { type: ['shared'] } } }`) is correct — shared utilities can call each other. This was not explicitly called out in the COLLABORATING.md table. Consider adding `shared | shared only` as a row in the table for completeness.

marcel commented 2026-05-05 17:51:27 +02:00

Author

Owner

Copy Link

🎨 Leonie Voss (@leonievoss) — UI/UX Design Lead & Accessibility Strategist

Verdict: ✅ Approved

This is a developer tooling PR with no user-facing changes. No UI components were redesigned, no layouts changed, no colors or typography touched. My review is scoped accordingly.

What's done well

• FieldLabelBadge moves to $lib/shared/primitives/ — this is the correct home for a presentational primitive. Primitives in shared/ are available to all domains without boundary violations, which is exactly where reusable UI building blocks belong. The move aligns the file location with the component's actual nature: a pure visual primitive with no domain knowledge.

• MissionControlStrip moves to $lib/document/ — this component's content is document-domain-specific (document upload counts, enrichment actions). It was misplaced in shared/dashboard/. The move makes the component tree more navigable: a developer looking for document-related dashboard widgets now finds them in the document domain.

• No visual regressions introduced. All changes are import path updates and file renames. The components themselves are unchanged (similarity index 100% for both MissionControlStrip.svelte and FieldLabelBadge.svelte). No Tailwind classes, no ARIA attributes, no brand colors were touched.

Blockers

None.

Suggestions

• shared/primitives/ directory naming — "primitives" is a good, conventional name for atomic UI components. Confirm there is (or will be) a brief note in COLLABORATING.md or the component itself about what qualifies as a "primitive" vs. a domain component, so future contributors know whether to put a new atomic component in shared/primitives/ or in a domain folder. This is a minor documentation gap, not a blocker.

• No accessibility regressions are introduced. No visual behavior changed. From a design system perspective, this PR improves navigability of the component library by putting things where they conceptually belong.

## 🎨 Leonie Voss (@leonievoss) — UI/UX Design Lead & Accessibility Strategist **Verdict: ✅ Approved** This is a developer tooling PR with no user-facing changes. No UI components were redesigned, no layouts changed, no colors or typography touched. My review is scoped accordingly. ### What's done well - **`FieldLabelBadge` moves to `$lib/shared/primitives/`** — this is the correct home for a presentational primitive. Primitives in `shared/` are available to all domains without boundary violations, which is exactly where reusable UI building blocks belong. The move aligns the file location with the component's actual nature: a pure visual primitive with no domain knowledge. - **`MissionControlStrip` moves to `$lib/document/`** — this component's content is document-domain-specific (document upload counts, enrichment actions). It was misplaced in `shared/dashboard/`. The move makes the component tree more navigable: a developer looking for document-related dashboard widgets now finds them in the document domain. - **No visual regressions introduced.** All changes are import path updates and file renames. The components themselves are unchanged (`similarity index 100%` for both `MissionControlStrip.svelte` and `FieldLabelBadge.svelte`). No Tailwind classes, no ARIA attributes, no brand colors were touched. ### Blockers None. ### Suggestions - **`shared/primitives/` directory naming** — "primitives" is a good, conventional name for atomic UI components. Confirm there is (or will be) a brief note in COLLABORATING.md or the component itself about what qualifies as a "primitive" vs. a domain component, so future contributors know whether to put a new atomic component in `shared/primitives/` or in a domain folder. This is a minor documentation gap, not a blocker. - No accessibility regressions are introduced. No visual behavior changed. From a design system perspective, this PR improves navigability of the component library by putting things where they conceptually belong.

marcel added 2 commits 2026-05-05 18:04:52 +02:00

fix(eslint): move fixture ignore from package.json flag to eslint.config.js ignores array ... 05a9e4ee47

Replace the --ignore-pattern CLI flag with an entry in the ignores array in
eslint.config.js where ESLint's flat config manages all ignore rules. Add
inline comment explaining that $lib/paraglide and $lib/generated are
intentionally omitted from the boundaries/elements list and treated as external.

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

refactor(document): remove statusLabel() alias, use formatDocumentStatus directly ... 7a22c1faf2

statusLabel() was a one-line alias for formatDocumentStatus() with no
additional behaviour. Remove it and update DocumentStatusChip.svelte to
call formatDocumentStatus() directly. Remove the corresponding alias
test suite from the spec file.

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

marcel commented 2026-05-05 18:05:17 +02:00

Author

Owner

Copy Link

Review feedback addressed — two commits pushed to feat/issue-410-eslint-boundary-rule:

---

fix(eslint): move fixture ignore from package.json flag to eslint.config.js ignores array (05a9e4ee)

Addresses Felix + Sara's concern #1.

• Removed --ignore-pattern 'src/lib/**/__fixtures__/**' from the lint script in package.json
• Added 'src/lib/**/__fixtures__/**' to the ignores array in eslint.config.js alongside the existing src/paraglide/** entry
• Added an inline comment in the ignores block explaining the fixture exclusion
• Also added an inline comment above boundaries/elements clarifying that $lib/paraglide and $lib/generated are intentionally omitted and treated as external (addresses Markus's comment #3)

---

refactor(document): remove statusLabel() alias, use formatDocumentStatus directly (7a22c1fa)

Addresses Felix's suggestion #2.

• Deleted statusLabel() from documentStatusLabel.ts — it was a one-liner delegating entirely to formatDocumentStatus()
• Updated DocumentStatusChip.svelte (only call site) to import and call formatDocumentStatus directly
• Removed the statusLabel describe block from documentStatusLabel.spec.ts (the behaviour is already covered by the formatDocumentStatus suite)

---

Concerns not requiring code changes:

• Tobias #4 (CI): npm run lint is already invoked in .gitea/workflows/ci.yml under the unit-tests job (- name: Lint / run: npm run lint). No change needed.
• Nora #5 (handlebars transitive dep): Noted, no action required — it is a transitive dependency only.

npm run lint passes green. No new type errors introduced (pre-existing svelte-check failures are unrelated to this PR).

Review feedback addressed — two commits pushed to `feat/issue-410-eslint-boundary-rule`: --- **fix(eslint): move fixture ignore from package.json flag to eslint.config.js ignores array** (`05a9e4ee`) Addresses Felix + Sara's concern #1. - Removed `--ignore-pattern 'src/lib/**/__fixtures__/**'` from the `lint` script in `package.json` - Added `'src/lib/**/__fixtures__/**'` to the `ignores` array in `eslint.config.js` alongside the existing `src/paraglide/**` entry - Added an inline comment in the `ignores` block explaining the fixture exclusion - Also added an inline comment above `boundaries/elements` clarifying that `$lib/paraglide` and `$lib/generated` are intentionally omitted and treated as external (addresses Markus's comment #3) --- **refactor(document): remove statusLabel() alias, use formatDocumentStatus directly** (`7a22c1fa`) Addresses Felix's suggestion #2. - Deleted `statusLabel()` from `documentStatusLabel.ts` — it was a one-liner delegating entirely to `formatDocumentStatus()` - Updated `DocumentStatusChip.svelte` (only call site) to import and call `formatDocumentStatus` directly - Removed the `statusLabel` describe block from `documentStatusLabel.spec.ts` (the behaviour is already covered by the `formatDocumentStatus` suite) --- **Concerns not requiring code changes:** - **Tobias #4 (CI):** `npm run lint` is already invoked in `.gitea/workflows/ci.yml` under the `unit-tests` job (`- name: Lint / run: npm run lint`). No change needed. - **Nora #5 (handlebars transitive dep):** Noted, no action required — it is a transitive dependency only. `npm run lint` passes green. No new type errors introduced (pre-existing `svelte-check` failures are unrelated to this PR).

marcel merged commit d9a4faf4da into main 2026-05-05 18:09:26 +02:00

marcel deleted branch feat/issue-410-eslint-boundary-rule 2026-05-05 18:09:28 +02:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

P0-critical

Blocks a core user journey, causes data loss, or is a security/accessibility barrier. Work on this before P1+.

P1-high

Significant friction on a main user journey; workaround exists but is non-obvious. Next up after P0.

P2-medium

Noticeable improvement; doesn't block anything; schedule opportunistically.

P3-later

Cosmetic or parking-lot work; fine to stay open indefinitely.

audit

Read-only audit / assessment work; produces a report rather than changing code

bug

Something isn't working

cleanup

Removal of dead code, vague comments, naming violations, and scratch artifacts

collaboration

We want to extend the family archive to have more collaboration tools

conversation

descoped

We will do that later

devops

documentation

README, ARCHITECTURE, GLOSSARY, CONTRIBUTING, per-domain docs

epic

Marker for epic-level issues that group multiple child issues

feature

file-upload

legibility

Codebase Legibility Refactor — preparing the codebase for human evaluation and bus-factor reduction

notification

person

phase-1: security

Security hygiene — must be done first

phase-2: container-images

Production-ready multi-stage Docker images

phase-3: prod-compose

Production compose overlay + Caddy reverse proxy

phase-4: spring-prod-profile

Spring Boot production configuration profile

phase-5: backups

Database and object storage backup strategy

phase-6: deployment-docs

.env.example, DEPLOYMENT.md, runbook

phase-7: monitoring

Prometheus, Loki, Grafana, Alertmanager

refactor

Code restructuring without behaviour change

security

test

ui

UI/UX and accessibility issues

user

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

Jens marcel

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#429