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
📝 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
|
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 {
| example: IExampleSentence; | ||
| readonly?: boolean; | ||
| onchange?: (sense: IExampleSentence, field: FieldId) => void; | ||
| onchange?: (sense: IExampleSentence, field: ExampleFieldId) => void; |
There was a problem hiding this comment.
Misleading parameter name: sense should be example.
The onchange callback parameter is named sense but its type is IExampleSentence. This appears to be a copy-paste artifact from the sense editor.
Proposed fix
- onchange?: (sense: IExampleSentence, field: ExampleFieldId) => void;
+ onchange?: (example: IExampleSentence, field: ExampleFieldId) => void;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onchange?: (sense: IExampleSentence, field: ExampleFieldId) => void; | |
| onchange?: (example: IExampleSentence, field: ExampleFieldId) => void; |
🤖 Prompt for AI Agents
In
`@frontend/viewer/src/lib/entry-editor/object-editors/ExampleEditorPrimitive.svelte`
at 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).
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.