Namespace field IDs by entity type to prevent naming collisions#2171
Namespace field IDs by entity type to prevent naming collisions#2171
Conversation
Split the flat FieldId union into EntryFieldId, SenseFieldId, and
ExampleFieldId, keeping FieldId as the backward-compatible union.
Restructure ViewFields from a flat Record<FieldId, FieldView> to a
nested { entry, sense, example } structure so field IDs are scoped
per entity type. This prevents naming collisions when persisting
custom view configurations and prepares for the CustomView backend
model with separate field arrays per entity type.
https://claude.ai/code/session_01NVUuxxWYXoQym9zB9A1UM7
These were only used by FW_CLASSIC_VIEW which now uses showAllFields. recursiveSpread is kept for future custom view definitions. https://claude.ai/code/session_01NVUuxxWYXoQym9zB9A1UM7
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR refactors the field type system from a flat structure in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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 |
github-actions
bot
added
the
💻 FW Lite
UI unit Tests 1 files ± 0 3 suites - 47 0s ⏱️ -23s Results for commit d4b9f37. ± Comparison against base commit b04aee5. This pull request removes 138 and adds 10 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
myieye
force-pushed
the
claude/investigate-field-id-conflicts-RfYJq
branch
from
c6bda6a to
11a3a54
Compare
![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
🤖 Fix all issues with AI agents
In
`@frontend/viewer/src/lib/entry-editor/object-editors/ExampleEditorPrimitive.svelte`:
- Line 18: The parameter name in the onchange callback is misleading: change the
first parameter name from sense to example in the signature "onchange?: (sense:
IExampleSentence, field: ExampleFieldId) => void;" so it reads something like
"onchange?: (example: IExampleSentence, field: ExampleFieldId) => void;" (update
any local references/usages in ExampleEditorPrimitive.svelte that refer to the
old name to match the new parameter name).
🧹 Nitpick comments (3)
frontend/viewer/src/lib/views/OverrideFields.svelte (2)
39-47:overrideEntityFieldsapplies a flatFieldId[]across all entity types, partially undermining the namespacing goal.
shownFieldsis a mixedFieldId[]applied identically to entry, sense, and example fields. While this works becauseincludes()simply returnsfalsefor non-matching keys, it means a sense-only field like'gloss'is needlessly checked against entry fields, and vice versa. If a naming collision is ever introduced (the scenario this PR guards against), this function would incorrectly show/hide fields across entity boundaries.Consider accepting per-entity shown fields to match the nested structure:
Suggested approach
interface Props { - shownFields?: FieldId[]; + shownFields?: { + entry?: EntryFieldId[]; + sense?: SenseFieldId[]; + example?: ExampleFieldId[]; + }; respectOrder?: boolean; overrides?: Overrides; children?: Snippet; }This would make the override truly namespace-aware. If this is deferred, at minimum add a comment explaining why the flat list is safe today.
40-46: Unsafeas Tcast onObject.fromEntriesresult.
Object.fromEntries(...)returns{ [k: string]: ... }, which is then cast toT(the specific entity fields type). This silently erases type safety — if a key is missing or extra keys are introduced, TypeScript won't flag it. This is a known TypeScript limitation withObject.fromEntries, so the cast is pragmatic, but worth noting as a trade-off.frontend/viewer/src/lib/views/view-data.ts (1)
72-79:mergeFieldsoverride parameter type is looser than necessary.The
overridesparameter accepts anyFieldIdkey regardless of entity type, so you could accidentally pass e.g.{sentence: ...}when merging entry fields without a type error. Since this is a private helper and callers are well-typed, it's not a bug today, but tightening the signature would catch mistakes if usage expands.Suggested tightening
-function mergeFields<T>(base: T, overrides?: Partial<Record<FieldId, Partial<FieldView>>>): T { +function mergeFields<T extends EntityViewFields>(base: T, overrides?: Partial<Record<keyof T & string, Partial<FieldView>>>): T {
frontend/viewer/src/lib/entry-editor/object-editors/ExampleEditorPrimitive.svelte
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
2 participants
This PR namespaces field IDs by entity type to prevent naming collisions.
This puts us in a better place to safely persist custom views that handle future naming collisions.