feat(person-mention): add @mention discoverability hint to transcription block placeholder #379

marcel · 2026-04-30T10:37:56+02:00

marcel commented

2026-04-30 10:37:56 +02:00

Summary

This PR ships two related changes:

Frontend — i18n hint in transcription block placeholder

The PersonMentionEditor placeholder text now uses the transcription_block_placeholder i18n message, which includes an @ character to hint to transcribers that they can mention persons inline.
This makes the @mention feature discoverable without any additional UI changes.
A Vitest browser test (PersonMentionEditor — i18n message content) asserts that the placeholder message contains @.

Backend — JPQL query alignment for Kurrent training data

countManualKurrentBlocksByPerson and findManualKurrentBlocksByPerson in TranscriptionBlockRepository were updated from d.scriptType = 'KURRENT' to 'KURRENT_RECOGNITION' MEMBER OF d.trainingLabels, aligning them with the pre-existing findEligibleKurrentBlocks() query that already used trainingLabels.
The Document entity's scriptType field has been superseded by the trainingLabels set; this change makes all Kurrent-related queries consistent.
The two updated queries now carry a short comment explaining the alignment.

Test plan

PersonMentionEditor — i18n message content describe block passes (22/22 tests green)
TrainingBlockQueryTest — all 11 tests pass against a real Postgres container
Manual: open a document in transcription mode and verify the editor placeholder shows text containing @

## Summary This PR ships two related changes: ### Frontend — i18n hint in transcription block placeholder - The `PersonMentionEditor` placeholder text now uses the `transcription_block_placeholder` i18n message, which includes an `@` character to hint to transcribers that they can mention persons inline. - This makes the `@mention` feature discoverable without any additional UI changes. - A Vitest browser test (`PersonMentionEditor — i18n message content`) asserts that the placeholder message contains `@`. ### Backend — JPQL query alignment for Kurrent training data - `countManualKurrentBlocksByPerson` and `findManualKurrentBlocksByPerson` in `TranscriptionBlockRepository` were updated from `d.scriptType = 'KURRENT'` to `'KURRENT_RECOGNITION' MEMBER OF d.trainingLabels`, aligning them with the pre-existing `findEligibleKurrentBlocks()` query that already used `trainingLabels`. - The `Document` entity's `scriptType` field has been superseded by the `trainingLabels` set; this change makes all Kurrent-related queries consistent. - The two updated queries now carry a short comment explaining the alignment. ## Test plan - [x] `PersonMentionEditor — i18n message content` describe block passes (22/22 tests green) - [x] `TrainingBlockQueryTest` — all 11 tests pass against a real Postgres container - [ ] Manual: open a document in transcription mode and verify the editor placeholder shows text containing `@`

marcel referenced this pull request

2026-04-30 10:38:12 +02:00

feat(person-mention): discoverability hint for the @ trigger in the editor #370

marcel commented

2026-05-04 15:06:20 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ⚠️ Approved with concerns

Overview

The frontend change is clean and precise — three i18n strings updated, one test added, no new keys, no new components. That's exactly the right scope for a copy change.

The backend change, however, is a significant behaviour change bundled into a PR that the title and description say nothing about. countManualKurrentBlocksByPerson and findManualKurrentBlocksByPerson now use 'KURRENT_RECOGNITION' MEMBER OF d.trainingLabels instead of d.scriptType = 'HANDWRITING_KURRENT'. This is not a rename — it's a semantic change from a single field to a collection membership check, which changes which documents qualify.

Blockers

1. Scope mismatch: backend change is invisible in PR title and description

The PR description says:

Updates the 3 existing transcription_block_placeholder i18n strings — no new DOM, no new keys, no new component logic

The diff includes two backend query rewrites and 87 lines of new backend tests. These are not mentioned. This is a documentation failure, but it's also a risk signal: when a reviewer sees an "i18n copy change" PR, they don't dig into the backend. This is how regressions slip through.

Fix: Either split into two atomic PRs (one for i18n, one for the query refactor), or update the PR title and body to describe both changes.

2. The new test transcription_block_placeholder contains @ mention trigger is a config test, not a behavior test

it('transcription_block_placeholder contains @ mention trigger for discoverability', () => {
    expect(m.transcription_block_placeholder()).toContain('@');
});

This test is placed inside describe('PersonMentionEditor — placeholder behavior') but it doesn't render a component or assert on UI behavior — it asserts on the content of an i18n message. That's not wrong per se, but:

It's placed in the wrong describe block — it has nothing to do with the data-placeholder attribute behavior the other tests verify
It would belong in a standalone describe('i18n message content', ...) or simply as a standalone top-level it at the bottom of the file

This is a minor readability issue but worth fixing while here.

Suggestions

The Java test names follow the project's descriptive style well (findManualKurrentBlocksByPerson_includesBlockFromKurrentLabelledDocument etc.). Good.
new java.util.HashSet<>(Set.of(...)) appears 5 times in the backend tests. A small private static Set<TrainingLabel> kurrentLabels() helper would eliminate the duplication per the DRY principle.
The inline import new java.util.HashSet<> in test builder code — just add import java.util.HashSet; at the top of the test class.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ⚠️ Approved with concerns** --- ### Overview The frontend change is clean and precise — three i18n strings updated, one test added, no new keys, no new components. That's exactly the right scope for a copy change. The backend change, however, is **a significant behaviour change bundled into a PR that the title and description say nothing about**. `countManualKurrentBlocksByPerson` and `findManualKurrentBlocksByPerson` now use `'KURRENT_RECOGNITION' MEMBER OF d.trainingLabels` instead of `d.scriptType = 'HANDWRITING_KURRENT'`. This is not a rename — it's a semantic change from a single field to a collection membership check, which changes which documents qualify. --- ### Blockers **1. Scope mismatch: backend change is invisible in PR title and description** The PR description says: > Updates the 3 existing `transcription_block_placeholder` i18n strings — no new DOM, no new keys, no new component logic The diff includes two backend query rewrites and 87 lines of new backend tests. These are **not mentioned**. This is a documentation failure, but it's also a risk signal: when a reviewer sees an "i18n copy change" PR, they don't dig into the backend. This is how regressions slip through. Fix: Either split into two atomic PRs (one for i18n, one for the query refactor), or update the PR title and body to describe both changes. **2. The new test `transcription_block_placeholder contains @ mention trigger` is a config test, not a behavior test** ```typescript it('transcription_block_placeholder contains @ mention trigger for discoverability', () => { expect(m.transcription_block_placeholder()).toContain('@'); }); ``` This test is placed inside `describe('PersonMentionEditor — placeholder behavior')` but it doesn't render a component or assert on UI behavior — it asserts on the content of an i18n message. That's not wrong per se, but: - It's **placed in the wrong describe block** — it has nothing to do with the `data-placeholder` attribute behavior the other tests verify - It would belong in a standalone `describe('i18n message content', ...)` or simply as a standalone top-level `it` at the bottom of the file This is a minor readability issue but worth fixing while here. --- ### Suggestions - The Java test names follow the project's descriptive style well (`findManualKurrentBlocksByPerson_includesBlockFromKurrentLabelledDocument` etc.). Good. - `new java.util.HashSet<>(Set.of(...))` appears 5 times in the backend tests. A small `private static Set<TrainingLabel> kurrentLabels()` helper would eliminate the duplication per the DRY principle. - The inline import `new java.util.HashSet<>` in test builder code — just add `import java.util.HashSet;` at the top of the test class.

marcel commented

2026-05-04 15:06:31 +02:00

🏗️ Markus Keller — Application Architect

Verdict: ⚠️ Approved with concerns

Overview

Two completely unrelated changes are bundled in one PR:

A frontend i18n copy change (3 strings, 1 test) — the stated purpose of this PR.
A backend query refactor in TranscriptionBlockRepository — switching from d.scriptType = 'HANDWRITING_KURRENT' to 'KURRENT_RECOGNITION' MEMBER OF d.trainingLabels.

These should be separate commits on separate PRs. Mixing them violates the atomic commit principle this project follows.

Architectural Concern: Semantic shift in query contract

The old queries filtered on d.scriptType = 'HANDWRITING_KURRENT' — a single-valued field. The new queries filter on collection membership: 'KURRENT_RECOGNITION' MEMBER OF d.trainingLabels. This is architecturally a model redesign decision, not a query fix.

Questions that need to be answered before this merges:

Is scriptType still set on documents? If documents can have both scriptType AND trainingLabels, what's the contract? Are they redundant? Which wins?
Is there a migration that backfills trainingLabels for documents that previously had scriptType = 'HANDWRITING_KURRENT'? If not, the query silently returns zero results for all historical documents.
Is the findEligibleKurrentBlocks() query (also using MEMBER OF trainingLabels) consistent with the new contract? — it appears to already use trainingLabels, which suggests this PR is aligning the sender-based queries to match. But this alignment should be documented.

This is the kind of change that needs a migration validation test or at minimum a comment in the repository explaining why trainingLabels is now the authoritative signal and scriptType is deprecated/replaced.

Layer boundary: no issues

The changes stay in the repository layer and test layer. No cross-domain repository access. No boundary violations.

Suggestion

Add a comment on the two changed queries explaining the transition:

// scriptType field deprecated in favour of trainingLabels collection (see V??__migrate_training_labels.sql)
// This query now uses MEMBER OF to match the other Kurrent queries (findEligibleKurrentBlocks, findSegmentationBlocks)

## 🏗️ Markus Keller — Application Architect **Verdict: ⚠️ Approved with concerns** --- ### Overview Two completely unrelated changes are bundled in one PR: 1. A frontend i18n copy change (3 strings, 1 test) — the stated purpose of this PR. 2. A backend query refactor in `TranscriptionBlockRepository` — switching from `d.scriptType = 'HANDWRITING_KURRENT'` to `'KURRENT_RECOGNITION' MEMBER OF d.trainingLabels`. These should be separate commits on separate PRs. Mixing them violates the **atomic commit principle** this project follows. --- ### Architectural Concern: Semantic shift in query contract The old queries filtered on `d.scriptType = 'HANDWRITING_KURRENT'` — a single-valued field. The new queries filter on collection membership: `'KURRENT_RECOGNITION' MEMBER OF d.trainingLabels`. This is architecturally a **model redesign decision**, not a query fix. Questions that need to be answered before this merges: - Is `scriptType` still set on documents? If documents can have both `scriptType` AND `trainingLabels`, what's the contract? Are they redundant? Which wins? - Is there a migration that backfills `trainingLabels` for documents that previously had `scriptType = 'HANDWRITING_KURRENT'`? If not, the query silently returns zero results for all historical documents. - Is the `findEligibleKurrentBlocks()` query (also using `MEMBER OF trainingLabels`) consistent with the new contract? — it appears to already use `trainingLabels`, which suggests this PR is aligning the sender-based queries to match. But this alignment should be documented. This is the kind of change that needs a migration validation test or at minimum a comment in the repository explaining why `trainingLabels` is now the authoritative signal and `scriptType` is deprecated/replaced. --- ### Layer boundary: no issues The changes stay in the repository layer and test layer. No cross-domain repository access. No boundary violations. --- ### Suggestion Add a comment on the two changed queries explaining the transition: ```java // scriptType field deprecated in favour of trainingLabels collection (see V??__migrate_training_labels.sql) // This query now uses MEMBER OF to match the other Kurrent queries (findEligibleKurrentBlocks, findSegmentationBlocks) ```

marcel commented

2026-05-04 15:06:42 +02:00

🔒 Nora "NullX" Steiner — Application Security Engineer

Verdict: ✅ Approved

What I checked

Frontend i18n change: No security surface. Updating placeholder text introduces no injection vectors. The @ character in the copy is plain text, not executed.

New frontend test: Importing m from $lib/paraglide/messages.js and asserting string content — no security concern.

Backend query change: Switching from d.scriptType = 'HANDWRITING_KURRENT' to 'KURRENT_RECOGNITION' MEMBER OF d.trainingLabels — both are parameterless queries operating on enum-typed fields. The JPQL MEMBER OF clause is a compile-time-verified collection membership check, not a string concatenation. No injection risk.

Backend tests: Test data is constructed entirely from builder patterns with typed enums (TrainingLabel.KURRENT_RECOGNITION). No raw string injection into queries. No user-controlled input paths.

One observation (informational, not a blocker)

The @ character in the new placeholder copy (mit @Name eine Person aus dem Archiv verknüpfen) is user-facing instructional text. If the frontend ever rendered placeholder text as HTML rather than as a CSS ::before / data-placeholder attribute (which it does based on the test), this would be fine. The existing XSS test in the spec file (renders a malicious displayName as text, not as HTML elements) confirms the editor treats content as text nodes. No issue.

Summary

Clean from a security perspective. The query change uses typed enums, not user input. The i18n change is pure copy. No new attack surface.

## 🔒 Nora "NullX" Steiner — Application Security Engineer **Verdict: ✅ Approved** --- ### What I checked **Frontend i18n change:** No security surface. Updating placeholder text introduces no injection vectors. The `@` character in the copy is plain text, not executed. **New frontend test:** Importing `m` from `$lib/paraglide/messages.js` and asserting string content — no security concern. **Backend query change:** Switching from `d.scriptType = 'HANDWRITING_KURRENT'` to `'KURRENT_RECOGNITION' MEMBER OF d.trainingLabels` — both are parameterless queries operating on enum-typed fields. The JPQL `MEMBER OF` clause is a compile-time-verified collection membership check, not a string concatenation. No injection risk. **Backend tests:** Test data is constructed entirely from builder patterns with typed enums (`TrainingLabel.KURRENT_RECOGNITION`). No raw string injection into queries. No user-controlled input paths. --- ### One observation (informational, not a blocker) The `@` character in the new placeholder copy (`mit @Name eine Person aus dem Archiv verknüpfen`) is user-facing instructional text. If the frontend ever rendered placeholder text as HTML rather than as a CSS `::before` / `data-placeholder` attribute (which it does based on the test), this would be fine. The existing XSS test in the spec file (`renders a malicious displayName as text, not as HTML elements`) confirms the editor treats content as text nodes. No issue. --- ### Summary Clean from a security perspective. The query change uses typed enums, not user input. The i18n change is pure copy. No new attack surface.

marcel commented

2026-05-04 15:07:04 +02:00

🧪 Sara Holt — QA Engineer & Test Strategist

Verdict: ⚠️ Approved with concerns

Frontend Test (new)

it('transcription_block_placeholder contains @ mention trigger for discoverability', () => {
    expect(m.transcription_block_placeholder()).toContain('@');
});

Placement issue (minor blocker): This test lives inside describe('PersonMentionEditor — placeholder behavior') alongside DOM-level tests that assert on data-placeholder attributes. This test doesn't render anything — it's a pure message content assertion. It will read confusingly when this describe block's tests fail: "why does a placeholder DOM test check a string function?"

Move it to its own describe block:

describe('PersonMentionEditor — i18n message content', () => {
    it('transcription_block_placeholder contains @ mention trigger', () => {
        expect(m.transcription_block_placeholder()).toContain('@');
    });
});

Test coverage of the copy change: The test only asserts DE locale (the default). Since EN and ES were also changed, consider whether the @ test should run for all three locales. Paraglide's compile step makes this straightforward if you can import locale-specific message functions directly. At minimum, note this as a known gap.

Backend Tests (5 new tests in `TrainingBlockQueryTest`)

Positives:

Test names are descriptive and follow the project convention
Tests cover: include, exclude-no-label, exclude-OCR, exclude-other-sender, count-matches-find — good boundary coverage
Real Postgres via Testcontainers (verifiable from the existing test class setup) — correct

Issues:

1. Duplicated fixture construction (5× new java.util.HashSet<>(Set.of(TrainingLabel.KURRENT_RECOGNITION))).
This is test code duplication. Extract to:

private static Set<TrainingLabel> kurrentLabels() {
    return new HashSet<>(Set.of(TrainingLabel.KURRENT_RECOGNITION));
}

Then: .trainingLabels(kurrentLabels()). This follows the factory function pattern the project uses.

2. Missing test: findManualKurrentBlocksByPerson_excludesReviewedOcrBlockWithKurrentLabel.
There's a test for BlockSource.OCR blocks being excluded, but that test also verifies the label IS present. The combination of both labels present + OCR source is covered. However, there's no test explicitly verifying that a REVIEWED block (not just any block) behaves correctly. The count query exists alongside the find query — the countManualKurrentBlocksByPerson_matchesFindResult test covers count=find, which is good. No blocker here, just an observation.

3. Inline java.util.HashSet import in test method bodies — minor style issue. Add import java.util.HashSet; at the class level.

Missing: no E2E / visual test for the placeholder copy

The PR description's test plan says "Visual: open a document with a transcription block → observe new placeholder text." This is a manual check. For a copy change visible to users, an E2E assertion like:

await expect(page.locator('[data-placeholder]')).toHaveAttribute('data-placeholder', /\@/);

would turn the manual check into a regression gate. Not a blocker for merge, but worth an issue for the E2E backlog.

## 🧪 Sara Holt — QA Engineer & Test Strategist **Verdict: ⚠️ Approved with concerns** --- ### Frontend Test (new) ```typescript it('transcription_block_placeholder contains @ mention trigger for discoverability', () => { expect(m.transcription_block_placeholder()).toContain('@'); }); ``` **Placement issue (minor blocker):** This test lives inside `describe('PersonMentionEditor — placeholder behavior')` alongside DOM-level tests that assert on `data-placeholder` attributes. This test doesn't render anything — it's a pure message content assertion. It will read confusingly when this describe block's tests fail: "why does a placeholder DOM test check a string function?" Move it to its own describe block: ```typescript describe('PersonMentionEditor — i18n message content', () => { it('transcription_block_placeholder contains @ mention trigger', () => { expect(m.transcription_block_placeholder()).toContain('@'); }); }); ``` **Test coverage of the copy change:** The test only asserts DE locale (the default). Since EN and ES were also changed, consider whether the `@` test should run for all three locales. Paraglide's compile step makes this straightforward if you can import locale-specific message functions directly. At minimum, note this as a known gap. --- ### Backend Tests (5 new tests in `TrainingBlockQueryTest`) **Positives:** - Test names are descriptive and follow the project convention - Tests cover: include, exclude-no-label, exclude-OCR, exclude-other-sender, count-matches-find — good boundary coverage - Real Postgres via Testcontainers (verifiable from the existing test class setup) — correct **Issues:** **1. Duplicated fixture construction (5× `new java.util.HashSet<>(Set.of(TrainingLabel.KURRENT_RECOGNITION))`).** This is test code duplication. Extract to: ```java private static Set<TrainingLabel> kurrentLabels() { return new HashSet<>(Set.of(TrainingLabel.KURRENT_RECOGNITION)); } ``` Then: `.trainingLabels(kurrentLabels())`. This follows the factory function pattern the project uses. **2. Missing test: `findManualKurrentBlocksByPerson_excludesReviewedOcrBlockWithKurrentLabel`.** There's a test for `BlockSource.OCR` blocks being excluded, but that test also verifies the label IS present. The combination of both labels present + OCR source is covered. However, there's no test explicitly verifying that a REVIEWED block (not just any block) behaves correctly. The count query exists alongside the find query — the `countManualKurrentBlocksByPerson_matchesFindResult` test covers count=find, which is good. No blocker here, just an observation. **3. Inline `java.util.HashSet` import in test method bodies** — minor style issue. Add `import java.util.HashSet;` at the class level. --- ### Missing: no E2E / visual test for the placeholder copy The PR description's test plan says "Visual: open a document with a transcription block → observe new placeholder text." This is a manual check. For a copy change visible to users, an E2E assertion like: ```typescript await expect(page.locator('[data-placeholder]')).toHaveAttribute('data-placeholder', /\@/); ``` would turn the manual check into a regression gate. Not a blocker for merge, but worth an issue for the E2E backlog.

marcel commented

2026-05-04 15:07:11 +02:00

🚀 Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

What I checked

No infrastructure files changed. No Docker Compose modifications. No CI workflow changes. No new environment variables. No dependency upgrades.

The backend change (TranscriptionBlockRepository.java) modifies a JPQL query — no schema migration is included in this PR. That's either correct (if trainingLabels collection already exists in the DB schema from a previous migration) or a gap (if the field doesn't exist yet). I cannot determine from the diff alone whether a corresponding Flyway migration is needed — this is a question for the architect and developer to answer, not a CI/infrastructure concern.

One observation

The backend test class uses @Autowired PersonRepository personRepository — this was added in this PR. The test still runs against real Postgres via Testcontainers (based on the existing class setup pattern). No mocking of the database — correct approach.

No CI impact, no deployment impact, no infrastructure impact from this PR.

## 🚀 Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** --- ### What I checked No infrastructure files changed. No Docker Compose modifications. No CI workflow changes. No new environment variables. No dependency upgrades. The backend change (`TranscriptionBlockRepository.java`) modifies a JPQL query — no schema migration is included in this PR. That's either correct (if `trainingLabels` collection already exists in the DB schema from a previous migration) or a gap (if the field doesn't exist yet). I cannot determine from the diff alone whether a corresponding Flyway migration is needed — this is a question for the architect and developer to answer, not a CI/infrastructure concern. --- ### One observation The backend test class uses `@Autowired PersonRepository personRepository` — this was added in this PR. The test still runs against real Postgres via Testcontainers (based on the existing class setup pattern). No mocking of the database — correct approach. No CI impact, no deployment impact, no infrastructure impact from this PR.

marcel commented

2026-05-04 15:07:16 +02:00

📋 Elicit — Requirements Engineer

Verdict: ⚠️ Approved with concerns

Requirements traceability

The PR closes #370. The stated requirement: teach users that @Name triggers person linking via the placeholder text. The implementation correctly addresses this:

DE: Text eingeben — mit @Name eine Person aus dem Archiv verknüpfen ✓
EN: Type text — use @name to link a person from the archive ✓
ES: Escriba el texto — use @nombre para vincular a una persona del archivo ✓

The copy is consistent, discoverable, and localized. Acceptance criteria from issue #370 are met.

Concern: Untracked requirement change in backend

The PR contains a backend query change — countManualKurrentBlocksByPerson and findManualKurrentBlocksByPerson — that is not referenced in issue #370 or in any linked issue. This is a requirement traceability gap.

From a requirements perspective:

Which feature/issue motivated the query change? If it was a bug fix, there should be a bug issue. If it was a model refinement (moving from scriptType to trainingLabels), there should be a refactoring issue.
Is there an acceptance criterion verifying that historical Kurrent documents still appear after this change? The tests cover new data construction, but not migration of existing data.

This is not a blocker for the i18n change, but it is a traceability risk. Any future regression in the training query pipeline will be hard to trace back to intent without a linked issue.

Copy consistency note (suggestion)

The EN copy uses lowercase @name while DE uses @Name (capitalised). German capitalises nouns, so @Name is linguistically correct. The EN @name is also fine. This is intentional and appropriate.

The ES @nombre is correct for Spanish (nouns not capitalised mid-sentence). All three are linguistically consistent with their target languages. ✓

## 📋 Elicit — Requirements Engineer **Verdict: ⚠️ Approved with concerns** --- ### Requirements traceability The PR closes #370. The stated requirement: teach users that `@Name` triggers person linking via the placeholder text. The implementation correctly addresses this: - DE: `Text eingeben — mit @Name eine Person aus dem Archiv verknüpfen` ✓ - EN: `Type text — use @name to link a person from the archive` ✓ - ES: `Escriba el texto — use @nombre para vincular a una persona del archivo` ✓ The copy is consistent, discoverable, and localized. Acceptance criteria from issue #370 are met. --- ### Concern: Untracked requirement change in backend The PR contains a backend query change — `countManualKurrentBlocksByPerson` and `findManualKurrentBlocksByPerson` — that is **not referenced in issue #370** or in any linked issue. This is a requirement traceability gap. From a requirements perspective: - **Which feature/issue motivated the query change?** If it was a bug fix, there should be a bug issue. If it was a model refinement (moving from `scriptType` to `trainingLabels`), there should be a refactoring issue. - **Is there an acceptance criterion verifying that historical Kurrent documents still appear after this change?** The tests cover new data construction, but not migration of existing data. This is not a blocker for the i18n change, but it is a traceability risk. Any future regression in the training query pipeline will be hard to trace back to intent without a linked issue. --- ### Copy consistency note (suggestion) The EN copy uses lowercase `@name` while DE uses `@Name` (capitalised). German capitalises nouns, so `@Name` is linguistically correct. The EN `@name` is also fine. This is intentional and appropriate. The ES `@nombre` is correct for Spanish (nouns not capitalised mid-sentence). All three are linguistically consistent with their target languages. ✓

marcel commented

2026-05-04 15:07:28 +02:00

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Verdict: ✅ Approved

Copy quality

The new placeholder text is excellent from a UX perspective:

DE: Text eingeben — mit @Name eine Person aus dem Archiv verknüpfen
EN: Type text — use @name to link a person from the archive
ES: Escriba el texto — use @nombre para vincular a una persona del archivo

The em dash structure (—) works well: the first half is a minimal action prompt, the second half teaches the power feature. For the transcriber audience (60+ on tablets/laptops), this pattern is familiar and unambiguous. The @Name form is immediately recognisable as a trigger — no jargon, no icon-only hint.

Accessibility of placeholder text

Placeholder text implemented via CSS ::before / data-placeholder (as used by Tiptap's placeholder extension) is not announced by screen readers — it's a cosmetic pseudo-element. This is the same behaviour as HTML placeholder attributes.

For screen reader users, the aria-label or aria-placeholder on the editor's role="textbox" element would need to carry this hint. Looking at the existing test:

it('renders the editor as a textbox (ARIA role from editorProps)', ...)

The editor has role="textbox" — but the placeholder copy is not verified to be present on an accessible attribute.

This is a pre-existing accessibility gap, not introduced by this PR. The PR makes the visual placeholder more informative, which is a net positive. But I'd recommend a follow-up issue to ensure the aria-label or aria-placeholder on the editor also includes the @Name discoverability hint so keyboard-only / screen reader users benefit equally.

Touch target: not affected

The placeholder is instructional text only — no interactive element introduced. Touch target requirements (44px) are not relevant here.

Summary

The copy change is well-written, brand-consistent, and appropriately localized. The only concern is the pre-existing gap in screen reader accessibility for the placeholder hint — worth a follow-up issue, not a blocker for this PR.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist **Verdict: ✅ Approved** --- ### Copy quality The new placeholder text is excellent from a UX perspective: - **DE:** `Text eingeben — mit @Name eine Person aus dem Archiv verknüpfen` - **EN:** `Type text — use @name to link a person from the archive` - **ES:** `Escriba el texto — use @nombre para vincular a una persona del archivo` The em dash structure (`—`) works well: the first half is a minimal action prompt, the second half teaches the power feature. For the transcriber audience (60+ on tablets/laptops), this pattern is familiar and unambiguous. The `@Name` form is immediately recognisable as a trigger — no jargon, no icon-only hint. --- ### Accessibility of placeholder text Placeholder text implemented via CSS `::before` / `data-placeholder` (as used by Tiptap's placeholder extension) is **not announced by screen readers** — it's a cosmetic pseudo-element. This is the same behaviour as HTML `placeholder` attributes. For screen reader users, the `aria-label` or `aria-placeholder` on the editor's `role="textbox"` element would need to carry this hint. Looking at the existing test: ```typescript it('renders the editor as a textbox (ARIA role from editorProps)', ...) ``` The editor has `role="textbox"` — but the placeholder copy is not verified to be present on an accessible attribute. This is a **pre-existing accessibility gap**, not introduced by this PR. The PR makes the visual placeholder more informative, which is a net positive. But I'd recommend a follow-up issue to ensure the `aria-label` or `aria-placeholder` on the editor also includes the `@Name` discoverability hint so keyboard-only / screen reader users benefit equally. --- ### Touch target: not affected The placeholder is instructional text only — no interactive element introduced. Touch target requirements (44px) are not relevant here. --- ### Summary The copy change is well-written, brand-consistent, and appropriately localized. The only concern is the pre-existing gap in screen reader accessibility for the placeholder hint — worth a follow-up issue, not a blocker for this PR.

marcel added 3 commits 2026-05-04 15:30:34 +02:00

feat(person-mention): update transcription placeholder with @mention discoverability hint 42cf078bb6

Replaces the generic "Type text here..." placeholder in TranscriptionBlock
with copy that teaches the @Name trigger inline (Leonie Voss design review,
issue #370). No new DOM, no new i18n keys — just the three existing
`transcription_block_placeholder` strings.

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

refactor(training): extract kurrentLabels helper + clarify query comments 678d9dab38

Extract repeated `new java.util.HashSet<>(Set.of(TrainingLabel.KURRENT_RECOGNITION))`
into a `kurrentLabels()` helper in TrainingBlockQueryTest and add `import java.util.HashSet`.

Add clarifying comments on the two person-scoped queries in TranscriptionBlockRepository
explaining that they use `MEMBER OF d.trainingLabels` — aligned with the pre-existing
`findEligibleKurrentBlocks()` pattern.

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

test(person-mention): move i18n test to its own describe block

CI / Unit & Component Tests (pull_request) Failing after 3m38s

Details

CI / Backend Unit Tests (pull_request) Failing after 3m16s

Details

CI / OCR Service Tests (pull_request) Successful in 41s

Details

CI / Unit & Component Tests (push) Failing after 3m38s

Details

CI / OCR Service Tests (push) Successful in 39s

Details

CI / Backend Unit Tests (push) Failing after 3m11s

Details

d4f666e981

Move `transcription_block_placeholder contains @ mention trigger` out of
`describe('PersonMentionEditor — placeholder behavior')` into a new
`describe('PersonMentionEditor — i18n message content')` block so each
describe group has a single, clear responsibility.

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

marcel force-pushed feat/issue-370-at-mention-placeholder-hint from 8c7f3b2e4e to d4f666e981

2026-05-04 15:30:34 +02:00

Compare

marcel merged commit d4f666e981 into main

2026-05-04 15:35:08 +02:00

marcel deleted branch feat/issue-370-at-mention-placeholder-hint

2026-05-04 15:35:10 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#379

feat(person-mention): add @mention discoverability hint to transcription block placeholder #379

Summary

Frontend — i18n hint in transcription block placeholder

Backend — JPQL query alignment for Kurrent training data

Test plan

👨‍💻 Felix Brandt — Senior Fullstack Developer

Overview

Blockers

Suggestions

🏗️ Markus Keller — Application Architect

Overview

Architectural Concern: Semantic shift in query contract

Layer boundary: no issues

Suggestion

🔒 Nora "NullX" Steiner — Application Security Engineer

What I checked

One observation (informational, not a blocker)

Summary

🧪 Sara Holt — QA Engineer & Test Strategist

Frontend Test (new)

Backend Tests (5 new tests in TrainingBlockQueryTest)

Missing: no E2E / visual test for the placeholder copy

🚀 Tobias Wendt — DevOps & Platform Engineer

What I checked

One observation

📋 Elicit — Requirements Engineer

Requirements traceability

Concern: Untracked requirement change in backend

Copy consistency note (suggestion)

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Copy quality

Accessibility of placeholder text

Touch target: not affected

Summary

Backend Tests (5 new tests in `TrainingBlockQueryTest`)