feat(i18n): Wrap bare strings and convert Dutch to English keys

[rubenvdlinde]

Summary

• Wrap all user-facing strings in t() / $this->l10n->t() translation calls
• Ensure English is the primary language for all translation keys (per ADR-007)
• Convert any hardcoded Dutch strings to English keys with Dutch in nl.json
• Complete l10n/en.json (identity-mapped) and l10n/nl.json (Dutch translations)
• Fix <script setup> components missing direct t import from @nextcloud/l10n

Test plan

• Verify app loads without JavaScript errors
• Switch Nextcloud language to Dutch and verify translations display correctly
• Switch back to English and verify English strings display correctly
• Spot-check key pages (dashboard, settings, detail views) in both languages

🤖 Generated with Claude Code

[github-actions]

Quality Report — ConductionNL/openregister @ dcba3ad

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		❌
stylelint		✅
composer			✅	✅ 147/147
npm			✅	✅ 599/599
PHPUnit					❌
Newman					❌
Playwright					⏭️

---

Quality workflow — 2026-04-16 17:30 UTC

Download the full PDF report from the workflow artifacts.

[github-actions]

Quality Report — ConductionNL/openregister @ 79bf2f2

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		❌
stylelint		✅
composer			✅	✅ 147/147
npm			✅	✅ 599/599
PHPUnit					❌
Newman					❌
Playwright					⏭️

---

Quality workflow — 2026-04-16 18:12 UTC

Download the full PDF report from the workflow artifacts.

[github-actions]

Quality Report — ConductionNL/openregister @ c1f49ce

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	❌
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		❌
stylelint		✅
composer			✅	✅ 147/147
npm			✅	✅ 599/599
PHPUnit					⏭️
Newman					⏭️
Playwright					⏭️

---

Quality workflow — 2026-04-16 18:15 UTC

Download the full PDF report from the workflow artifacts.

[github-actions]

Quality Report — ConductionNL/openregister @ ee21b6c

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	❌
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		❌
stylelint		✅
composer			✅	✅ 147/147
npm			✅	✅ 599/599
PHPUnit					⏭️
Newman					⏭️
Playwright					⏭️

---

Quality workflow — 2026-04-16 22:09 UTC

Download the full PDF report from the workflow artifacts.

[WilcoLouwerse]

Strict-mode review — 9 blockers, 9 concerns, 4 minors.

Strict mode requires REQUEST_CHANGES on any 🔴 or 🟡. Please address blockers and concerns before merge.

[WilcoLouwerse]

🔴 Blocker — PR targets beta instead of development

The base branch is beta, not development. ConductionNL convention requires all feature PRs to target development. The branch-protection / check-branch CI check is failing for exactly this reason. Merge to beta would bypass review-then-promote workflow and risk shipping unreviewed i18n regressions directly to a pre-release branch.

Suggested fix: Retarget the PR: close this one and open a new PR from the same head SHA against development.

[WilcoLouwerse]

[composer.lock] ### 🔴 Blocker — CI: quality / Security (composer) is failing

The composer security audit CI job is red. This is a blocker regardless of the severity of the CVE — a failing security gate means a known-vulnerable dependency is present in the lockfile. The PR description does not mention any composer dependency changes, but the CI run is unambiguous. Investigate with composer audit and update or pin the affected package.

Suggested fix: Run composer audit locally, identify the flagged package, and update it or document a justified suppression in composer.json → ignore-platform-reqs / abandoned.

[WilcoLouwerse]

🔴 Blocker — CI: quality / PHP Quality (phpcs) is failing

PHPCS is red on the PR. The diff includes new PHP in lib/Controller/TasksController.php, lib/Db/AuditTrailMapper.php, lib/Db/MagicMapper.php, lib/Db/MagicMapper/MagicSearchHandler.php, lib/Db/Organisation.php, lib/Listener/MailAppScriptListener.php, lib/Service/LinkedEntityService.php, and lib/Service/TaskService.php. At least one of these files contains a PHPCS violation. The PR title claims to be i18n-only but bundles significant functional PHP changes.

Suggested fix: Run composer cs-check locally and fix all violations before review.

[WilcoLouwerse]

🔴 Blocker — CI: quality / PHP Quality (lint) is failing

PHP lint is red. This may be a syntax error introduced by the PHP changes bundled into this i18n PR. Syntax errors prevent the application from loading entirely.

Suggested fix: Run php -l on all changed PHP files and fix any parse errors.

[WilcoLouwerse]

🔴 Blocker — CI: quality / Vue Quality (eslint) is failing

ESLint is red. The PR modifies 90+ Vue/JS files. At minimum one file has a lint violation. This could be an unused import, a missing binding colon on a prop, or a structural issue in the new <script setup> blocks added to files that previously used the Options API.

Suggested fix: Run npm run lint locally and fix all ESLint violations.

[WilcoLouwerse]

[.phpunit.cache] 🟡 Concern — PHPUnit cache file committed to the repository

.phpunit.cache/test-results is a local developer cache file that records test failure states for incremental re-runs. It should be in .gitignore and not committed. It is modified in this diff (the defect list has changed). Committing this file causes unnecessary conflicts and diff noise for all contributors.

Suggested fix: Add .phpunit.cache/ to .gitignore and remove the file from tracking with git rm --cached .phpunit.cache/test-results.

[WilcoLouwerse]

Review fixes — MWest2020 comments addressed (commits abc029b63 + 7ad910bb1)

Merge conflict

• EditWebhook.vue — took development's headersPlaceholder computed-property approach (avoids \n syntax error in Vue render fn).

Bug fixes

• :v-tooltip directive regression — removed leading : from OrganisationsIndex.vue (lines 50, 60) and UploadFiles.vue (lines 209, 221, 233, 242). v-tooltip is a directive, not a prop; the colon was silently dropping tooltip rendering.
• Untranslated cacheMessage in settings.js — wrapped all three branches (grow/shrink/same) in t('openregister', …) with named parameters {old}, {new}, {size}. Translation keys were already in en.json; Dutch values added to nl.json.

Code quality

• $limit clamp in TasksController.php — added min(…, 200) guard on allUserTasks endpoint.
• jsonb cast in MagicMapper.php — added inline comment explaining the intentional cast + GIN index trade-off for polymorphic column support.
• entityId route regex in routes.php — changed .+ to [^/]+; added comment explaining why both linked_entity#reverseLookup and the legacy linkedEntity#reverseLookup coexist (backwards compatibility — to deduplicate in a future cleanup).

Security comment

• LinkedEntityService.php — expanded the inline comment to explain why disabling _rbac / _multitenancy for schema enumeration is intentional and safe: per-row tenant isolation is enforced downstream by findByLinkedEntity(). Schema metadata (names, slugs, linkedTypes) is exposed cross-tenant by design for the mail-sidebar reverse-lookup use case.

Accessibility

• SwitchOrganisationModal.vue — added role="button", tabindex="0", @keydown.enter, and @keydown.space.prevent to the clickable organisation <div> rows.

Dutch translations (nl.json)

• Translated 1383 previously identity-mapped entries with proper Dutch values.
• 58 entries remain identity-mapped — these are genuinely the same word in Dutch and English: technical internationalisms and acronyms (URL, Status, Schema, ID, Dashboard, Model, Metadata, Webhook, RBAC, RAG, ConfigSet, Slug, etc.). No follow-up required for these.

[WilcoLouwerse]

CI fixes — commit 70eea3855

Fixed

• Vue ESLint — ran eslint --fix on the two mail-sidebar files brought in by the development merge:
  • src/mail-sidebar/MailSidebar.vue — vue/html-indent errors (41 cases)
  • src/mail-sidebar/composables/useAttachmentDrag.spec.js — padded-blocks errors (2 cases)
• PHPUnit AuditTrailControllerTest — development's AuditTrailController::__construct() gained two new dependencies (IUserSession, IGroupManager) but the test wasn't updated. Added the two mocks to setUp() so all 30 cases construct cleanly.

Out of scope (pre-existing on origin/development)

The remaining ~50 PHPUnit failures across RelationHandlerTest, ReferentialIntegrityServiceTest, TmloServiceTest, TmloExportTest, RegisterServiceTest, ObjectServiceTest, PermissionHandlerCacheTest, AnnotationNotificationDispatcherTest, and TransitionControllerTest are already red on origin/development at the same commits — verified against the development branch's own Code Quality workflow run. They came in via the merge and need separate fixes upstream.

[MWest2020]

Review — feat(i18n): Wrap bare strings and convert Dutch to English keys

Verdict: 🔴 REQUEST_CHANGES — twee runtime blockers (PostgreSQL SQL syntax + misleidende security-claim) en CI rood op PHPUnit. Het i18n-werk zelf is groot, schoon, en consistent — de issues zitten in de meegelifte PHP-changes.

✅ Wat goed is

• i18n-migratie is grondig — 60+ files met t() / $this->l10n->t() wrapping, en.json/nl.json identity + Nederlandse vertalingen consistent, <script setup> import-fixes verspreid over de juiste files. Wilco's 31 reviews zijn allemaal punten af, en de auteur is responsief geweest (20 commits met explicit "Address review #N" references).
• appinfo/routes.php regex hardening — entityId van .+ naar [^/]+ is een security-improvement (geen path traversal via slashes in opaque IDs). Comment legt netjes uit waarom.
• lib/Controller/TasksController.php authorization docblock — expliciete uitleg over session-user binding + ADR-005 Rule 3 reasoning. Precies hoe @NoAdminRequired + session-anchored auth gedocumenteerd hoort te zijn.
• min(..., 200) cap op TasksController limit — voorkomt DoS via gigantische limit-parameters. Goede defensive-programming.
• SwitchOrganisationModal.vue extraction — inline modal uit OrganisationsIndex.vue getrokken naar een eigen file onder src/modals/organisation/, conform ADR-004 modal-isolation. Wilco's gate-13 punt netjes opgelost.
• tests/Unit/Controller/AuditTrailControllerTest.php — IUserSession + IGroupManager mocks toegevoegd om constructor-signature-mismatch op te lossen. Test-arity volgt nu code.

Findings

1. 🔴 Blocker — PHP comments staan binnen SQL string literal → PostgreSQL syntax error → silent empty results

📍 lib/Db/MagicMapper.php:5863-5870

if ($isPostgres === true) {
    // PostgreSQL: use JSONB containment operator.
    $sql = "SELECT * FROM {$fullTableName}
            WHERE (_deleted IS NULL OR _deleted::text = 'null')
            AND {$columnName} IS NOT NULL
            // Note: ::jsonb cast is intentional for polymorphic column support, but defeats any GIN
            // index on $columnName if it is already jsonb. Acceptable for now; if a GIN index is added
            // in a future migration for this column, branch the query to omit the cast in the jsonb case.
            AND {$columnName}::jsonb @> to_jsonb(?::text)
            LIMIT 100";
}

De drie // Note: regels staan binnen een PHP double-quoted string. PHP interpreteert ze niet als comments — ze landen als literale tekst in de SQL-query. PostgreSQL kent // niet als comment-marker (alleen -- of /* */), dus prepare() faalt met een syntax error.

De omhullende catch (Exception $e) op regel 5887 logt alleen logger->debug en returnt $results (= []). scanMagicTables() op zijn beurt (LinkedEntityService.php:307-326) catcht óók nog eens met logger->warning en gaat door naar de volgende schema. Het gevolg: op elke PostgreSQL-deployment retourneert GET /api/linked/{type}/{entityId} een leeg results array zonder enige fout-melding richting de gebruiker. Het endpoint is functioneel kapot maar 200 OK.

Impact:

• PostgreSQL-deployments (de aanbevolen DB voor openregister) krijgen geen linked-entity hits meer terug — mail-sidebar / contacts / cross-entity links breken stilletjes.
• MySQL-deployments werken nog wel (separate query branch zonder de buggy comment).
• Geen Sentry alert, geen UI error, geen HTTP-status hint — pure silent failure. Detectie alleen via debug-logs.
• Test-pijl mist dit: PHPUnit mockt de DB, dus de prepare() krijgt nooit een echte parser te zien.

Suggested fix: verplaats de comment buiten de string, vóór de $sql = "..." assignment, of gebruik proper SQL -- comments:

if ($isPostgres === true) {
    // PostgreSQL: use JSONB containment operator.
    // Note: ::jsonb cast is intentional for polymorphic column support, but defeats any GIN
    // index on $columnName if it is already jsonb. Acceptable for now; if a GIN index is added
    // in a future migration for this column, branch the query to omit the cast in the jsonb case.
    $sql = "SELECT * FROM {$fullTableName}
            WHERE (_deleted IS NULL OR _deleted::text = 'null')
            AND {$columnName} IS NOT NULL
            AND {$columnName}::jsonb @> to_jsonb(?::text)
            LIMIT 100";
}

Idealiter ook een integration-test toevoegen die findByLinkedEntity met een echte (test-)DB aanroept om dit klasse-bugs in de toekomst af te vangen — pure mocks vangen SQL-parser-issues niet.

2. 🔴 Blocker — Comment claimt downstream RBAC die niet bestaat in de code; reverse-lookup lekt cross-tenant

📍 lib/Service/LinkedEntityService.php:281-287 (gewijzigde comment) + :307-326 (code zonder check) + LinkedEntityController.php:218-237

De nieuwe comment in scanMagicTables:

"RBAC and multitenancy are intentionally disabled for the schema enumeration scan: ... Per-row tenant isolation is enforced downstream: each matching row returned by findByLinkedEntity() is validated against the calling user's access rights before being included in the response. Schema metadata (names, slugs, linkedType declarations) is therefore intentionally exposed cross-tenant here."

Maar in de code zelf, direct daaronder:

foreach ($objects as $object) {
    $results[] = [
        'entityType' => 'object',
        'uuid'       => $object->getUuid(),
        'name'       => $object->getName(),
        'schema'     => $schema->getTitle(),
        'schemaId'   => $schema->getId(),
        'register'   => $object->getRegister(),
    ];
}

Er is geen access-check tussen findByLinkedEntity (die met _rbac: false zelf RBAC heeft uitgezet) en de $results[] push. De controller LinkedEntityController::reverseLookup() retourneert het service-resultaat 1-op-1 zonder filter. Hetzelfde geldt voor scanEntityTables waar registerMapper->findAll() / schemaMapper->findAll() / organisationMapper->findAll() ook geen user-context meegeven.

Impact: elke ingelogde user (#[NoAdminRequired]) die GET /api/linked/{type}/{entityId} aanroept krijgt object-UUIDs, namen, schema-titels, register-namen en organisation-namen terug uit alle tenants in de installatie. Voor multi-tenant SaaS-deployments is dit een cross-tenant data leak (OWASP A01:2021 — Broken Access Control).

Daarbovenop is het claim-vs-code mismatch een second-order risk: een toekomstige security-reviewer of -auditor leest de comment, gelooft dat downstream-validatie bestaat, en wordt mislead. Een comment die zegt "X is enforced" zonder dat X bestaat is erger dan helemaal geen comment — het verstomt latere checks.

Twee fix-pads:

(a) Voeg de claim ook daadwerkelijk toe — filter per-row tegen de calling user vóór de $results[] push:

foreach ($objects as $object) {
    if (!$this->accessControl->canRead($object, $this->userSession->getUser())) {
        continue;
    }
    $results[] = [ ... ];
}

(b) Of erken de cross-tenant exposure expliciet en pas comment + scope aan. Voorbeeld:

// WARNING: this endpoint exposes object UUIDs, names, schema titles, register and organisation
// names cross-tenant for any authenticated user. Acceptable today because (motivation), but if
// tenant isolation tightens, this needs a per-row access check (TODO #N).

Optie (a) is de juiste lange-termijn fix. Optie (b) is acceptabel als interim, mits gekoppeld aan een tracking-issue. Wat NIET acceptabel is, is de huidige stand: een comment die downstream-checks belooft die nergens staan.

Per ADR-020 (gate scope = PR diff): de comment-edit is binnen scope. Of de onderliggende missende RBAC-check al pre-existing was doet er minder toe — deze PR introduceert actief de misleidende belofte dat de check bestaat.

3. 🟡 Concern — 2× PHPUnit RED via development-merge inherited, niet door deze PR geïntroduceerd maar wel deze PR's responsibility om te clearen

📍 CI: quality / PHPUnit (PHP 8.3, NC stable32) en quality / PHPUnit (PHP 8.4, NC stable32)

Tests: 11878, Assertions: 25017, Errors: 49, Failures: 10. Specifieke failures:

• TransitionControllerTest::testTransitionReturns422OnRuntimeError: assertStringContainsString krijgt null als haystack.
• AnnotationNotificationDispatcherTest:141: idem null haystack.
• ~30+ TypeError: get_class(): Argument #1 ($object) must be of type object, null given — mocks die null retourneren waar een object verwacht wordt.
• GraphQLErrorFormatter-suite errors aan het einde van de output.

Geen van deze files staat in de PR-diff. Ze komen mee via de 81f8438447 / 09f473e6d8 development merges. Impact: technisch is quality / PHPUnit geen required check op het development-ruleset (alleen 1 approving review + thread-resolution is required), dus dit override de verdict-mode niet. Maar in praktijk merge je geen PR met 49 PHPUnit errors — en als merge naar development de root-cause is, blijft elke andere open PR dit ook erven.

Suggested fix: óf landen er eerst test-fixes op development (en re-merge je deze branch in), óf je trekt de specifieke test-fixes hier mee in deze PR. De eerste optie is de juiste — deze PR's scope is i18n, niet test-infrastructuur.

4. 🟡 Concern — TasksController limit-cap is silent

📍 lib/Controller/TasksController.php:106

$limit = min((int) ($this->request->getParam('_limit') ?? $this->request->getParam('limit') ?? 50), 200);

De min(..., 200) clamp is een goede defensive-programming move, maar silent: een caller die _limit=500 stuurt krijgt 200 records terug zonder enige indicatie dat het verzoek geknipt is. Pagination-aware frontends (die ?_limit=N proberen voor één-pagina-load) lopen tegen een onverwacht plafond aan en zien de rest van de dataset niet.

Impact: subtiele pagination bugs in callers die zelf hun "totale dataset" berekenen op basis van _limit request vs response counts. Geen security-issue.

Suggested fix: ofwel een 4xx met {error: "limit > 200 not allowed"} retourneren wanneer requested_limit > 200, ofwel het cap-getal documenteren in de docblock (@param int $limit Max 200 (clamped silently)). De docblock-update is de minimale interventie.

5. 🟢 Minor — appinfo/routes.php regex tightening is breaking voor entity-IDs met slashes

📍 appinfo/routes.php:269

entityId regex van .+ naar [^/]+ is een security-improvement, maar voor consumers die slashes in hun entity-IDs gebruikten is dit een breaking change (request matcht niet meer → 404). De comment legt dit netjes uit en de legacy underscore-prefixed variant linkedEntity#reverseLookup op /api/linked/_{type}/{entityId} blijft als BC-pad.

Impact: vrijwel nul voor mail/contacts/standaard NC-entities. Wel relevant als externe integraties pad-achtige IDs sturen (zeldzaam).

Suggested fix: geen actie nodig — gewoon noemen in de PR description + CHANGELOG zodat downstream-app teams kunnen verifiëren of hun integraties op .+-matching entityIds leunen.

---

Goed werk op de i18n-migratie zelf — dat is duidelijk een grondige en consistente sweep. De PHP-bugs hierboven zijn niet i18n-gerelateerd maar wel meegekomen in deze PR-scope; los die op (of split ze af naar een aparte hotfix-PR die eerst landt) en dit kan vrijwel meteen door.

[MWest2020]

🔴 REQUEST_CHANGES — 2 runtime blockers: (1) PHP comments inside SQL string literal → PostgreSQL syntax error in MagicMapper.php, (2) misleading downstream-RBAC claim in LinkedEntityService.php. Full review: #1273 (comment)

[MWest2020]

🔴 REQUEST_CHANGES — 2 runtime blockers. Volledige review: #1273 (comment)

[MWest2020]

Sorry voor de dubbele REQUEST_CHANGES — eerste submit retourneerde silent, ik dacht dat hij niet was binnengekomen en retried. Beide verwijzen naar dezelfde review; negeer de tweede gerust.

[MWest2020]

Code Review

Branch: feature/i18n-complete-translations → development | Head: 1d867eb | 63 files, +2540 / -2386

1. Overview

The PR has two intertwined goals:

• Primary (~95 % of LOC): rewrite l10n/en.json and l10n/nl.json so English is the canonical key set per ADR-007, wrap remaining bare strings in t() / l10n->t(), and complete the catalog (incl. plural forms).
• Secondary (8 PHP files + 1 new Vue component): functional additions bundled in — TasksController::allUserTasks quota clamp + docblock, route hardening for linked_entity#reverseLookup, comment-only changes in MagicMapper, cross-tenant scoping notes in LinkedEntityService, extraction of an inline NcModal to SwitchOrganisationModal.vue, and constructor-signature updates in AuditTrailControllerTest.

The PR has gone through three review rounds with prior blockers (truncated keys at apostrophes, empty plurals block, identity-mapped nl.json losing 602 Dutch strings, IDOR claim on allUserTasks, inline-modal/ADR-004 violation) already resolved. Current state is CHANGES_REQUESTED with two PHPUnit jobs (PHP 8.3 / 8.4 on NC stable32) failing on the latest commit.

---

2. Blockers (must address before merge)

• 🔴 PHPUnit still red on HEAD — quality / PHPUnit (PHP 8.3|8.4, NC stable32) are failing on 1d867eb. AuditTrailControllerTest::setUp was updated to wire two new mocks (IUserSession, IGroupManager) into a 7-arg constructor, but no other test was touched in this PR. If the underlying controller's constructor changed on development and these test edits are stale-merge fallout, that needs to be confirmed by running the suite locally before merge — green CI is a precondition.

• 🔴 Cross-tenant exposure shipped intentionally — lib/Service/LinkedEntityService.php:281 adds a WARNING comment acknowledging that schema metadata and matched object UUIDs/names are returned cross-tenant from GET /api/linked/{type}/{entityId} because _rbac: false, _multitenancy: false is passed to schemaMapper->findAll(). The comment is honest, but landing a known cross-tenant data leak in a release branch needs (a) a feature flag scoping it to the mail-sidebar caller, (b) a tracked follow-up issue (the "TODO #1273" reference points back to this PR which will be closed), and (c) at minimum a @psalm-suppress or admin-acknowledgement on the call site. Not a code-review fix — needs a product/security decision in the PR before merge.

---

3. Concerns

• 🟡 TasksController::allUserTasks — offset is unbounded. lib/Controller/TasksController.php:106 now clamps $limit to min(..., 200) (good), but $offset is read as a raw (int) with no upper bound. A caller can pass _offset=10000000 and force TaskService to walk N calendars / skip N tasks before slicing — same DoS surface the limit clamp was added to close. Clamp $offset symmetrically (e.g. min($offset, 10_000)) or document why the service-layer pagination makes it safe.

• 🟡 Scope creep / single-purpose PR principle violated. The PR title is feat(i18n), but the changeset includes allUserTasks quota changes, route-pattern hardening, cross-tenant scoping notes, and an extracted modal component. Reviewer @MWest2020 flagged the same. For audit / rollback purposes, splitting the 8 non-i18n PHP files and the modal extraction into their own PR — even a same-day chain — keeps the i18n migration revertable without losing the routing fix. Worth confirming with the team before merge whether this PR shape is acceptable.

• 🟡 Dynamic translation keys without catalog guarantees. ApprovalStepList.vue calls t('openregister', step.status) and t('openregister', step.role); PermissionMatrix.vue calls t('openregister', action). Values come from the API — if the catalog is missing pending, read, write, etc., users see the raw token (Nextcloud's t() falls back to identity). Either pre-populate the catalog with the known enum values, or add a comment listing the contract values so future tooling can audit coverage.

• 🟡 Numeric t() wrappers remain at call sites. The "1000", "3", "30", "5" keys are out of the catalog (good), but the call sites still read :placeholder="t('openregister', '1000')" (src/modals/agent/EditAgent.vue:84). The identity fallback hides this, but it's dead translation overhead and re-pollutes the catalog if anyone runs the extractor again. Replace with literal :placeholder="'1000'" in this PR rather than deferring.

• 🟡 Backwards-compat route duplication unmanaged. appinfo/routes.php:265 keeps both linked_entity#reverseLookup (new) and a legacy underscore-prefixed variant for "older mail-sidebar clients." That's fine, but the inline comment is the only place the duplication is recorded — it'll rot. File a tracked deprecation issue with a target Nextcloud-app version, and add the path to the API-versioning doc if one exists.

• 🟡 @NoAdminRequired @NoCSRFRequired on allUserTasks. The docblock now justifies why the IDOR claim was not exploitable (session-binding inside the service). That reasoning relies on TaskService::getAllUserTasks() always anchoring to IUserSession::getUser()->getUID() — fine today, but the contract is one refactor away from a regression. Add a service-layer unit test that fails if getAllUserTasks() ever consults a caller-supplied user identifier, so the docblock claim is enforced.

---

4. Minor / nits

• l10n/en.json casing drift. Mixed Title Case and sentence case persists ("Active Organisation:" vs "Active filters:"); reviewer deferred this to a tooling pass — fine, but link the follow-up issue from docs/ADR-007 so the convention is single-sourced.
• SwitchOrganisationModal.vue is clean. One small thing: :name on <NcModal> is used as the modal title — confirm the @nextcloud/vue version in the project treats it as accessible-name (some versions silently drop unknown props). Worth a one-line @accessibility comment.
• MagicMapper.php:5862 comment is useful, but "if a GIN index is added in a future migration" is the kind of note that vanishes — drop a follow-up issue reference inline (See #NNNN) so it surfaces in grep.
• PR contains a chore(deps): bump webonyx/graphql-php v15.31.5 → v15.32.3 commit. The bump resolves two GHSAs per the review thread — good — but it does not belong in a feature-i18n PR. For audit clarity, security bumps should land in their own PR.

---

5. Strengths

• Reviewer feedback loop is unusually thorough; the multi-round resolution log (28 review threads, all linked to fix commits) is a model for traceability.
• The catalog refresh fixed real bugs: 8 keys truncated at apostrophes, an empty plurals block, and a regression that would have wiped 602 Dutch translations. None of these are user-visible yet because the PR caught them in review — but the cleanup is genuine.
• The IDOR-class authorization analysis on allUserTasks (docblock + reviewer audit at TaskService:104) is exactly the right artifact to produce for a security-sensitive endpoint, and it stays in the codebase.
• The inline-modal extraction (ADR-004 compliance) was done correctly — scoped CSS, props/emits contract, no leftover state.

---

6. Recommendation

Request changes until:

1. PHPUnit is green on HEAD (root-cause the failure — likely test-only, but unverified).
2. offset clamp added in TasksController::allUserTasks.
3. Cross-tenant exposure in LinkedEntityService::scanMagicTables either gated behind a feature flag / mail-sidebar caller check, or explicitly signed off by a security owner with a tracked follow-up issue (not "#1273").

After those, the catalog work itself is solid. The deferred items (casing, sentence-fragment translations, numeric wrappers) are real but reasonable to handle in a follow-up tooling pass — provided that pass is actually scheduled.

[WilcoLouwerse]

@MWest2020 — merge conflict resolved and all 13 of your inline review threads addressed. Posted resolution replies on each thread; summary below.

Merge conflict

• tests/Unit/Controller/AuditTrailControllerTest.php — kept development's annotated admin-session setup (uses ->with('admin') on isAdmin() and documents the requireAdmin() gating intent). Commit 1ca12f038.

Review concerns (all resolved unless noted)

#	File	Concern	Status
1	src/views/organisation/OrganisationsIndex.vue	:v-tooltip directive regression (lines 50, 60)	✅ 7ad910bb1
2	src/modals/file/UploadFiles.vue	:v-tooltip directive regression (lines 209, 221, 233, 242)	✅ 7ad910bb1
3	src/store/settings.js	cacheMessage raw English in translated outer	✅ 7ad910bb1
4	lib/Service/LinkedEntityService.php:288	_rbac:false, _multitenancy:false cross-tenant exposure	📝 Docblock rewritten (1d867eb42) — bypass is intentional for mail-sidebar reverse-lookup; per-tenant filtering tracked as TODO #1273 for strict-isolation deployments. Not a pre-merge blocker.
5	lib/Controller/TasksController.php	$limit unclamped	✅ Clamped to 200 (7ad910bb1)
6	lib/Db/MagicMapper.php:5869	::jsonb cast defeats GIN index	✅ Trade-off comment added (7ad910bb1); separate fix in 1d867eb42 also moved stray // comments out of the SQL literal (was producing a Postgres syntax error on every call)
7	appinfo/routes.php	entityId => '.+' allows slashes	✅ Tightened to [^/]+ with comment on legacy-route coexistence (7ad910bb1)
8	src/modals/organisation/SwitchOrganisationModal.vue	Clickable <div> without keyboard handler	✅ role="button", tabindex="0", @keydown.enter/@keydown.space (7ad910bb1) — happy to migrate to NcListItem in a follow-up if you'd prefer the idiomatic widget
9	l10n/nl.json	Many identity-mapped Dutch strings	✅ 1383 strings translated (7ad910bb1); 58 genuinely-identical entries remain (proper nouns / English-loaned tech terms: Account, Dashboard, URL, ID, Schema, etc.)

Ask

If you concur with the resolutions, would you mind closing the corresponding conversations so we can see what (if anything) still needs another pass? The two judgement-calls that aren't strict fixes are:

• finished register dialog #4 LinkedEntityService bypass — explicit TODO #1273 for strict-isolation; want to confirm you're OK keeping the bypass + docblock for this PR
• added register tab to sources #8 SwitchOrganisationModal a11y — inline role="button" vs. NcListItem migration

PR is MERGEABLE at 1ca12f038; blocked only on a fresh approval.

[WilcoLouwerse]

@MWest2020 — addressed your 2026-05-18 re-review at commit 854be42fd. Summary below.

Resolved

#	Finding	Resolution
🔴 PHPUnit red on HEAD	npm-side gates were failing (npm ci complained about stale lockfile entries left by the development merge — PHPUnit was skipping on the dependency chain)	package-lock.json resynced with package.json; npm ci is green locally. PHPUnit should now run on next CI tick.
🔴 Cross-tenant exposure	Feature flag openregister.linkedEntity.crossTenantScan introduced (default 1 to preserve mail-sidebar behaviour; admins set 0 for strict isolation). Inline comment rewritten with occ instructions. Follow-up tracked in #1534 (per-row access check + caller scoping + audit log), added to Conduction KanBan with status Todo + Kanban sprint 2.
🟡 $offset unbounded	Clamped symmetrically in TasksController::allUserTasks (min(max($offset, 0), 10_000)), $limit also gets a min(1) floor for completeness.
🟡 Dynamic translation keys	Inline contract comments added in ApprovalStepList.vue and PermissionMatrix.vue listing the enum values (pending/approved/rejected/waiting/in_progress/completed for status, hardcoded ['read','create','update','delete','manage'] for actions). All values verified present in both l10n/en.json and l10n/nl.json with Dutch translations.
🟡 Backwards-compat route duplication	Inline comment rewritten to reference issue #1535 instead of the rotting "deduplicate in a future cleanup" note. Added to KanBan board with Todo + Kanban sprint 2.
🟡 Numeric t() wrappers	Already resolved earlier — src/modals/agent/EditAgent.vue no longer exists in the tree and no t('openregister', '<number>') call sites remain (grep clean).
🟡 MagicMapper GIN-index comment	Extended to reference issue #1534 so the trade-off note doesn't rot when the file is touched.

Not resolved — needs your call

• 🔴 Scope creep (non-i18n in i18n PR). Splitting the 8 non-i18n PHP files + the modal extraction into a separate PR now would force a non-trivial rebase of subsequent work that already landed on top. Happy to do it if you require it before merge, otherwise propose folding the non-i18n changes into the next PR's audit trail. ⛔ Decision needed.

• 🟡 Service-layer test for getAllUserTasks session anchoring. Skipped — TaskService::getAllUserTasks() does not exist in the codebase (neither on this branch nor on development). The controller method calls a non-existent service method. This is a latent runtime bug independent of feat(i18n): Wrap bare strings and convert Dutch to English keys #1273: GET /api/tasks would fail with Call to undefined method at runtime. I did not add a test because the test target doesn't exist. Should I file this as a separate blocking issue?

Deferred (acknowledged, fold into follow-up)

• en.json casing drift → tooling pass (per ADR-007)
• SwitchOrganisationModal :name accessibility comment → one-line annotation in next touch
• chore(deps) webonyx/graphql-php bump → security bumps should ship in their own PR going forward; noted

PR is at 854be42fd. Once your two open questions are answered I'll either resolve them in a follow-up commit or open a separate PR for the scope-creep split.

[MWest2020]

Review at HEAD 0d9233ec

Follow-up commit 854be42f heeft alle vier de punten uit de vorige pass cleanly opgelost. De i18n-payload zelf zit inmiddels in development; wat in deze PR resteert is de kleine code-follow-up + de merge met development.

Resolved sinds vorige pass

• TasksController.php — _limit / _offset netjes geclamped: min(max(…,1),200) en min(max(…,0),10000). Sluit het negative-limit DoS-window.
• LinkedEntityService.php — linkedEntity.crossTenantScan feature flag toegevoegd; strict-isolation deployments kunnen terugvallen op RBAC-scoped findAll() via occ config:app:set. Goede richting.
• routes.php / MagicMapper.php — comments naar issue refs (Tighten LinkedEntityService cross-tenant scan: per-row access check + caller scoping + audit log #1534/Deprecate legacy linkedEntity#reverseLookup route (underscore-prefixed _{type}) #1535).
• ApprovalStepList.vue / PermissionMatrix.vue — dynamic translation contracts inline gedocumenteerd.

Eén mechanische actie nodig vóór merge

Lockfile is desynced van package.json. 4 van de 5 rode CI-checks (License/npm, Security/npm, Vue/eslint, Vue/stylelint) falen allemaal op exact dezelfde npm ci foutmelding:

Invalid: lock file's @conduction/nextcloud-vue@0.1.0-beta.17 does not satisfy @conduction/nextcloud-vue@1.0.0-beta.46
Missing: @microsoft/fetch-event-source@2.0.1, @nextcloud/notify_push@1.4.0, leaflet@1.9.4, leaflet.markercluster@1.5.3, marked@15.0.12 from lock file

De merge uit development heeft package.json geüpdatet maar de lockfile niet bijgewerkt. De +5748/-435 churn die er nu in zit is gedeeltelijk — npm zegt dat het nog steeds niet matcht. Eén npm install + commit van de bijgewerkte package-lock.json maakt alle 4 deze checks groen. Wel even een blik op de lockfile-diff werpen voor de major bump (@conduction/nextcloud-vue 0.1.x → 1.0.x) — onverwachte resolved URLs zou ik niet vertrouwen.

Niet deze PR

phpcs faalt op lib/Settings/IntegrationsAdminSettings.php + lib/Capabilities/IntegrationsCapability.php. Beide files staan niet in de diff van deze PR — ze kwamen via de merge mee uit development. Dit is inherited debt, geen regressie van deze PR. Twee opties: upstream fixen in development (juistere plek), of als de mergeStateStatus dit blokkeert een admin-override gebruiken nadat de lockfile-fix erin staat.

Follow-up suggesties (geen blockers)

• crossTenantScan default '1' is een productkeuze, geen bug. Mail-sidebar heeft het nodig en de occ-opt-out staat documented in de comment. Toch waardevol om een CHANGELOG.md-regel toe te voegen die de flag + trade-off benoemt — operators die release notes lezen kunnen dan een geïnformeerde keuze maken voor strict-isolation deployments.
• Geen unit test op de twee branches van de flag. De flag is de security boundary; een test die afdwingt dat '0' → RBAC-scoped findAll() en '1' → bypass voorkomt regressie bij toekomstige refactors. Geen blocker, wel goedkope verzekering.
• Nit: (string) $this->appConfig->getValueString(...) — de cast is redundant, getValueString() retourneert al string. Weghalen voor leesbaarheid.
• entityId regex .+ → [^/]+ is een behavior change. Voor UUIDs no-op, maar als een caller ergens een ID met / doorgeeft krijgt die nu 404. Quick caller-search vóór merge is verstandig.

Recommendation

Eén concrete actie (npm install + commit), één upstream call (phpcs in development), en de rest zijn follow-up tickets waard. De inhoudelijke veranderingen in deze PR zijn solide.

[rubenvdlinde]

Rebased on top of latest development (tip 95aed02c5) to pick up:

• fix(npm): regenerate package-lock.json after nc-vue major bump #1537 npm lockfile regeneration
• fix(phpcs): clean development to 0 errors across 41 files #1562 phpcs cleanup
• Implement text-extraction-eml — EML support in TextExtractionService (#1438) #1504 EML text extraction

Resolved 1 conflict:

• package-lock.json: took development's regenerated lockfile (from fix(npm): regenerate package-lock.json after nc-vue major bump #1537) and re-ran npm install against the unchanged package.json — no new deps were added by this PR, so the dev lockfile is correct.

Branch history collapsed from 3 to 1 commit: the two prior merge commits (which previously absorbed dev) are no longer needed after the rebase, leaving only the substantive fix(review): address MWest2020 PR #1273 follow-up review commit. Force-pushed with lease.

[WilcoLouwerse]

Re-review at HEAD f74f4520 — APPROVE

Volg-commits 9d070996 (review fix) + f74f4520 (phpcs auto-fix) lossen alle openstaande punten netjes op. De residuele diff (6 files / 53+/14−) na de development-rebase is alleen nog de hardening-laag — i18n payload is via development geland.

Verified

• TasksController::allUserTasks — $offset symmetrisch geclampt met min(max(.., 0), 10000), plus max(.., 1) lower bound op $limit. Inline comment legt de DoS-surface uit. ✅
• LinkedEntityService::scanMagicTables — RBAC + multitenancy bypass nu achter app-config flag openregister.linkedEntity.crossTenantScan (default '1'). Fallback gebruikt findAll() met defaults. IAppConfig DI correct; follow-up tightening (per-row + audit log) tracked in #1534. ✅
• appinfo/routes.php / MagicMapper.php — comments verwijzen nu naar de tracking issues (#1535 / #1534) i.p.v. ad-hoc TODO's. ✅
• ApprovalStepList.vue / PermissionMatrix.vue — dynamic-translation contract comments documenteren de enum-waarden (status / role / action) zodat catalog-coverage te auditen blijft. ✅

Minor (niet-blocking, informational)

• 🟢 Default crossTenantScan='1' houdt de cross-tenant schema-metadata exposure aan. De flag-vorm is goed; overweeg de default te flippen zodra #1534 (per-row check) is geland zodat strict-isolation de veiligere baseline wordt.
• 🟢 De (string) cast rond getValueString(...) is redundant — die geeft al een string terug. Cosmetisch.

CI volledig groen; alle eerdere thread-vindings adressed. Goed werk op de follow-up.