fix: make annotation placeholder rows respect property cardinality (closes #203)#273
Open
R-Hart80 wants to merge 2 commits into
Open
fix: make annotation placeholder rows respect property cardinality (closes #203)#273R-Hart80 wants to merge 2 commits into
R-Hart80 wants to merge 2 commits into
Conversation
…loses CatholicOS#203) Annotation properties like skos:prefLabel (SKOS S14) and skos:definition allow at most one value per language tag, but the editor was unconditionally appending an empty @en placeholder even when an English value already existed, inviting a duplicate that violates the cardinality constraint. - Add `AnnotationCardinality` type (`single` | `single-per-lang` | `multiple`) and a `cardinality` field to every entry in `ANNOTATION_PROPERTIES`. - Add `getAnnotationCardinality(iri)` helper (defaults to `"multiple"` for unknown IRIs, the safe non-restrictive fallback). - Centralise placeholder logic in `lib/ontology/annotationCardinality.ts`: `ensureTrailingPlaceholder(values, cardinality)` replaces the three identical copies of `ensureTrailingEmpty` that lived in the three panels. - `"multiple"` → unchanged trailing-empty behaviour. - `"single-per-lang"` → placeholder lang is the first common language (en → pt → es → fr → de → it) not yet covered by a filled value; no placeholder when all common langs are present. - `"single"` → no placeholder once any value is filled. - Update ClassDetailPanel, PropertyDetailPanel, IndividualDetailPanel to use the cardinality-aware helper at every call site. - Keep `ensureTrailingEmpty` exported from ClassDetailPanel as a deprecated wrapper so existing test imports compile without changes. - Add 23 unit tests covering all three cardinality modes and the cardinality lookup helper. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR implements cardinality-aware trailing placeholder handling for annotation values. It adds a type system and metadata catalog to distinguish single, single-per-lang, and multiple cardinality annotations, centralizes placeholder logic into a pure helper, and applies it across the three editor detail panels to prevent duplicate rows for constrained annotations like skos:prefLabel. ChangesCardinality-aware Annotation Placeholders
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/editor/IndividualDetailPanel.tsx (1)
277-289:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInline annotation add/restore paths still bypass cardinality normalization.
Line 692 and Line 695 hardcode a trailing
@enplaceholder, which breaks"single"/"single-per-lang"behavior. Line 288 also restores annotations without re-normalizing, so stale drafts can keep invalid placeholder shapes.Suggested fix
- setEditAnnotations(d.annotations); + setEditAnnotations( + d.annotations.map((a) => ({ + ...a, + values: ensureTrailingPlaceholder( + a.values.map((v) => ({ ...v })), + getAnnotationCardinality(a.property_iri), + ), + })), + ); ... onAdd={(propIri, value, lang) => { setEditAnnotations((prev) => { const existing = prev.find((a) => a.property_iri === propIri); + const cardinality = getAnnotationCardinality(propIri); if (existing) { return prev.map((a) => a.property_iri === propIri - ? { ...a, values: [...a.values.filter((v) => v.value.trim()), { value, lang }, { value: "", lang: "en" }] } : a + ? { + ...a, + values: ensureTrailingPlaceholder( + [...a.values.filter((v) => v.value.trim()), { value, lang }], + cardinality, + ), + } + : a ); } - return [...prev, { property_iri: propIri, values: [{ value, lang }, { value: "", lang: "en" }] }]; + return [ + ...prev, + { + property_iri: propIri, + values: ensureTrailingPlaceholder([{ value, lang }], cardinality), + }, + ]; }); }}Also applies to: 687-696
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/editor/IndividualDetailPanel.tsx` around lines 277 - 289, Restored drafts are bypassing cardinality normalization causing hardcoded trailing `@en` placeholders and invalid annotation shapes; when restoring in IndividualDetailPanel replace the direct restore calls that use ensureTrailingPlaceholder with the correct cardinality modes (e.g., use "single" or "single-per-lang" consistently for setEditComments and setEditDefinitions) and re-run the same normalization used on user edits before calling setEditAnnotations (and other annotation setters like setEditComments/setEditDefinitions) so annotations and label/definition placeholders conform to cardinality rules; locate and update the restore logic around setEditComments, setEditDefinitions, setEditAnnotations, and related setters to invoke the normalization helper instead of hardcoding "`@en`".components/editor/PropertyDetailPanel.tsx (1)
285-297:⚠️ Potential issue | 🟠 Major | ⚡ Quick winProperty annotation add/restore flow still reintroduces non-cardinality placeholders.
Line 734 and Line 738 still hardcode
... + { value: "", lang: "en" }, so"single"/"single-per-lang"constraints are bypassed on add. Line 296 restores raw annotation arrays without re-normalization.Suggested fix
- setEditAnnotations(d.annotations); + setEditAnnotations( + d.annotations.map((a) => ({ + ...a, + values: ensureTrailingPlaceholder( + a.values.map((v) => ({ ...v })), + getAnnotationCardinality(a.property_iri), + ), + })), + ); ... onAdd={(propIri, value, lang) => { setEditAnnotations((prev) => { const existing = prev.find((a) => a.property_iri === propIri); + const cardinality = getAnnotationCardinality(propIri); if (existing) { return prev.map((a) => a.property_iri === propIri - ? { ...a, values: [...a.values.filter((v) => v.value.trim()), { value, lang }, { value: "", lang: "en" }] } + ? { + ...a, + values: ensureTrailingPlaceholder( + [...a.values.filter((v) => v.value.trim()), { value, lang }], + cardinality, + ), + } : a ); } - return [...prev, { property_iri: propIri, values: [{ value, lang }, { value: "", lang: "en" }] }]; + return [ + ...prev, + { + property_iri: propIri, + values: ensureTrailingPlaceholder([{ value, lang }], cardinality), + }, + ]; }); }}Also applies to: 728-739
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/editor/PropertyDetailPanel.tsx` around lines 285 - 297, The property annotation add/restore flow is reintroducing incorrect hardcoded placeholders and bypassing cardinality rules; fix it by normalizing annotations through the existing helper instead of injecting raw placeholders: update the add-annotation code paths (e.g., any addAnnotation handler) to call ensureTrailingPlaceholder(...) with the correct mode ("single" or "single-per-lang") rather than appending { value: "", lang: "en" }, and when restoring drafts in the PropertyDetailPanel restore block (where setEditAnnotations(d.annotations) is used) call setEditAnnotations(ensureTrailingPlaceholder(d.annotations, "<appropriate-mode>")) so annotations respect single vs multi-per-language constraints; use the same ensureTrailingPlaceholder utility that other fields (comments/definitions) use to determine placeholder shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/ontology/annotationCardinality.ts`:
- Around line 34-37: The code in the add-row logic uses COMMON_LANGS and returns
early when nextLang is undefined which prevents adding additional valid locales
beyond that list; change the behavior in the block that computes
filledLangs/nextLang so that when no COMMON_LANGS remain you still append a new
empty row with an open/blank locale instead of returning early — i.e., in the
code around filledLangs, nextLang and the final return replace the early return
with returning [...values, { value: "", lang: nextLang ?? "" }] (or equivalent
so lang is empty/placeholder when nextLang is undefined) so users can add
locales like "ja" even if not in COMMON_LANGS.
---
Outside diff comments:
In `@components/editor/IndividualDetailPanel.tsx`:
- Around line 277-289: Restored drafts are bypassing cardinality normalization
causing hardcoded trailing `@en` placeholders and invalid annotation shapes; when
restoring in IndividualDetailPanel replace the direct restore calls that use
ensureTrailingPlaceholder with the correct cardinality modes (e.g., use "single"
or "single-per-lang" consistently for setEditComments and setEditDefinitions)
and re-run the same normalization used on user edits before calling
setEditAnnotations (and other annotation setters like
setEditComments/setEditDefinitions) so annotations and label/definition
placeholders conform to cardinality rules; locate and update the restore logic
around setEditComments, setEditDefinitions, setEditAnnotations, and related
setters to invoke the normalization helper instead of hardcoding "`@en`".
In `@components/editor/PropertyDetailPanel.tsx`:
- Around line 285-297: The property annotation add/restore flow is reintroducing
incorrect hardcoded placeholders and bypassing cardinality rules; fix it by
normalizing annotations through the existing helper instead of injecting raw
placeholders: update the add-annotation code paths (e.g., any addAnnotation
handler) to call ensureTrailingPlaceholder(...) with the correct mode ("single"
or "single-per-lang") rather than appending { value: "", lang: "en" }, and when
restoring drafts in the PropertyDetailPanel restore block (where
setEditAnnotations(d.annotations) is used) call
setEditAnnotations(ensureTrailingPlaceholder(d.annotations,
"<appropriate-mode>")) so annotations respect single vs multi-per-language
constraints; use the same ensureTrailingPlaceholder utility that other fields
(comments/definitions) use to determine placeholder shape.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b92fdd71-7cf0-472b-abb6-79354da8523c
📒 Files selected for processing (6)
__tests__/lib/ontology/annotationCardinality.test.tscomponents/editor/ClassDetailPanel.tsxcomponents/editor/IndividualDetailPanel.tsxcomponents/editor/PropertyDetailPanel.tsxlib/ontology/annotationCardinality.tslib/ontology/annotationProperties.ts
When all common languages (en, pt, es, fr, de, it) are already filled, the add-row logic now appends a blank-lang placeholder instead of returning early — letting users type locales like "ja" or "ko" that are not in the predefined list. Also normalises filledLangs comparison by trimming and lower-casing each lang tag, preventing missed matches from casing differences. Addresses CodeRabbit review comment on PR CatholicOS#273. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/ontology/annotationCardinality.ts (1)
13-13: ⚡ Quick winUpdate JSDoc to reflect blank-lang placeholder behavior.
The comment states "no placeholder when all common langs are filled," but the implementation now adds a blank-lang placeholder (
{ value: "", lang: "" }) in that case (lines 41-44). This could confuse developers reading the code.📝 Suggested documentation fix
- * - "single-per-lang": placeholder lang is the first common language not yet - * covered by a filled value; no placeholder when all common langs are filled. + * - "single-per-lang": placeholder lang is the first common language not yet + * covered by a filled value; blank-lang placeholder when all common langs are filled.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/ontology/annotationCardinality.ts` at line 13, Update the JSDoc comment in the annotationCardinality.ts file that currently states "no placeholder when all common langs are filled" to accurately reflect the actual implementation behavior. The implementation adds a blank-lang placeholder object with empty value and lang properties in that case (see lines 41-44), so the documentation should be corrected to indicate that a blank-lang placeholder IS added when all common languages are filled, not that no placeholder is added.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lib/ontology/annotationCardinality.ts`:
- Line 13: Update the JSDoc comment in the annotationCardinality.ts file that
currently states "no placeholder when all common langs are filled" to accurately
reflect the actual implementation behavior. The implementation adds a blank-lang
placeholder object with empty value and lang properties in that case (see lines
41-44), so the documentation should be corrected to indicate that a blank-lang
placeholder IS added when all common languages are filled, not that no
placeholder is added.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24023c38-a259-496c-8c80-37b6a5e6572b
📒 Files selected for processing (2)
__tests__/lib/ontology/annotationCardinality.test.tslib/ontology/annotationCardinality.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
skos:prefLabel @enalready filled → editor now shows a@ptplaceholder (not a second@en), respecting SKOS S14.skos:definition @enalready filled → same behaviour (single-per-lang).skos:notation/dcterms:created/dcterms:modifiedetc. filled → no trailing empty at all (single).skos:altLabel,rdfs:comment, all DC Elements → unchanged (always one trailing empty,multiple).Changes
lib/ontology/annotationProperties.tsAnnotationCardinalitytype +cardinalityfield on everyKnownAnnotationProperty; addgetAnnotationCardinality(iri)helperlib/ontology/annotationCardinality.ts(new)ensureTrailingPlaceholder(values, cardinality)— replaces three identical copies ofensureTrailingEmptyClassDetailPanel.tsxensureTrailingPlaceholderfor annotation value arrays; keepensureTrailingEmptyas deprecated wrapper for backwards-compatPropertyDetailPanel.tsxeditDefinitionsnow uses"single-per-lang"IndividualDetailPanel.tsxeditLabelsandeditDefinitionsnow use"single-per-lang"__tests__/lib/ontology/annotationCardinality.test.ts(new)Test plan
skos:prefLabel "Foo"@en→ detail panel shows the value row + one empty@ptrow (not@en).skos:altLabel "Bar"@en→ still shows value row + empty@en(multi-valued, unchanged).dcterms:created "2024-01-01"→ no trailing empty row.npx vitest run __tests__/lib/ontology/annotationCardinality.test.ts→ 23/23 pass.LanguagePickerfailure is unrelated to this PR).Closes #203.
🤖 Generated with Claude Code
Summary by CodeRabbit