Basic word collection by semantic domain (with scope creep) #2104
Conversation
github-actions
bot
added
the
💻 FW Lite
|
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 📝 WalkthroughWalkthroughThis PR refactors entry creation with sense management UI capabilities in the dialog, introduces ObjectHeader component for consistent header rendering, enforces readonly semantics across type signatures, and integrates part-of-speech and semantic domain filtering into the browse view components. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
![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
🧹 Nitpick comments (5)
frontend/viewer/src/project/browse/EntriesList.svelte (1)
155-158: Consider consistency in handling undefined semantic domain.The code converts an undefined
semanticDomainto an empty array[], whileBrowseView.svelte(line 35) passesundefinedin the same scenario. For consistency and clearer intent (distinguishing "not specified" from "empty list"), consider usingundefinedhere as well.Apply this diff if you prefer consistency:
- const entry = await dialogsService.createNewEntry(undefined, { - semanticDomains: semanticDomain ? [semanticDomain] : [], - partOfSpeech: partOfSpeech, - }); + const entry = await dialogsService.createNewEntry(undefined, { + semanticDomains: semanticDomain ? [semanticDomain] : undefined, + partOfSpeech, + });frontend/viewer/src/lib/entry-editor/NewEntryDialog.svelte (4)
32-35:senseinitialization is unused before firstopenWithValue
let sense = $state<ISense | undefined>(defaultSense(entry.id));creates an initial sense tied to the initialentry, but bothentryandsenseare overwritten inopenWithValuebefore the dialog is ever opened, so this initializer is effectively dead.You can simplify by initializing with
undefinedto make the lifecycle clearer:- // svelte-ignore state_referenced_locally - let sense = $state<ISense | undefined>(defaultSense(entry.id)); + // svelte-ignore state_referenced_locally + let sense = $state<ISense | undefined>(undefined);
70-79: Validation relies onentry.sensesbeing in sync withsense
validateEntrychecksentry.senses[0]rather than the standalonesensestate. Right now this works because:
addSense()always setsentry.senses = [sense].createEntry()overwritesentry.sensesfromsensebefore callingvalidateEntry.However, when deleting a sense you only set
sense = undefinedand leaveentry.sensesuntouched untilcreateEntry()runs. It’s safe but a bit fragile.Consider eagerly syncing
entry.senseswhen clearingsenseto keep the data model obviously consistent:- <Button onclick={() => sense = undefined} size="icon" variant="secondary" icon="i-mdi-trash-can" /> + <Button + onclick={() => { + sense = undefined; + entry.senses = []; + }} + size="icon" + variant="secondary" + icon="i-mdi-trash-can" + />or centralize this in a small helper.
81-83: Clarify types of template flags forundefinedusage
partOfSpeechIsFromTemplate/semanticDomainIsFromTemplateare deliberately set toundefinedinaddSense(), but their declarations use$state<boolean>(). Depending on how$stateis typed, this may or may not explicitly includeundefinedin the variable type.To make the intent obvious to TypeScript and future readers, consider widening the type:
- let partOfSpeechIsFromTemplate = $state<boolean>(); + let partOfSpeechIsFromTemplate = $state<boolean | undefined>(); ... - let semanticDomainIsFromTemplate = $state<boolean>(); + let semanticDomainIsFromTemplate = $state<boolean | undefined>();
96-119: UnifySenseTemplatealias withnewSense/senseTemplatetypesYou export
SenseTemplateas:export type SenseTemplate = Partial<Pick<ISense, 'partOfSpeech' | 'semanticDomains'>>;but both
senseTemplatestate and thenewSenseparameter usePartial<Omit<ISense, 'partOfSpeechId'>>, which is broader and doesn’t referenceSenseTemplateat all.For clarity and to reduce type drift, consider using the exported alias consistently:
- let senseTemplate = $state<Partial<Omit<ISense, 'partOfSpeechId'>>>(); + let senseTemplate = $state<SenseTemplate>(); ... - function openWithValue(newEntry: Partial<IEntry>, newSense?: Partial<Omit<ISense, 'partOfSpeechId'>>): Promise<IEntry | undefined> { + function openWithValue(newEntry: Partial<IEntry>, newSense?: SenseTemplate): Promise<IEntry | undefined> {If you intentionally want to allow more fields (e.g. for future pre-population reuse), then it might be better to adjust
SenseTemplateitself to match that, so external callers don’t see a stricter type than the implementation actually supports.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
frontend/viewer/src/lib/entry-editor/NewEntryDialog.svelte(4 hunks)frontend/viewer/src/lib/entry-editor/field-editors/ComplexFormComponents.svelte(1 hunks)frontend/viewer/src/lib/entry-editor/field-editors/ComplexForms.svelte(1 hunks)frontend/viewer/src/lib/entry-editor/object-editors/AddSenseButton.svelte(1 hunks)frontend/viewer/src/lib/entry-editor/object-editors/EntryEditor.svelte(4 hunks)frontend/viewer/src/lib/entry-editor/object-editors/ObjectHeader.svelte(1 hunks)frontend/viewer/src/lib/entry-editor/object-editors/SenseEditorPrimitive.svelte(3 hunks)frontend/viewer/src/lib/services/dialogs-service.ts(2 hunks)frontend/viewer/src/lib/writing-system-service.svelte.ts(3 hunks)frontend/viewer/src/project/browse/BrowseView.svelte(4 hunks)frontend/viewer/src/project/browse/EntriesList.svelte(4 hunks)frontend/viewer/src/project/browse/SearchFilter.svelte(3 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-05-27T06:18:33.852Z
Learnt from: hahn-kev
Repo: sillsdev/languageforge-lexbox PR: 1710
File: frontend/viewer/src/project/browse/BrowseView.svelte:17-19
Timestamp: 2025-05-27T06:18:33.852Z
Learning: The NewEntryButton component in frontend/viewer/src/project/NewEntryButton.svelte already internally checks features.write permission and conditionally renders based on write access, so external disabled props are not needed.
Applied to files:
frontend/viewer/src/lib/entry-editor/field-editors/ComplexFormComponents.sveltefrontend/viewer/src/lib/entry-editor/field-editors/ComplexForms.sveltefrontend/viewer/src/lib/entry-editor/object-editors/AddSenseButton.svelte
📚 Learning: 2025-07-30T04:53:41.702Z
Learnt from: rmunn
Repo: sillsdev/languageforge-lexbox PR: 1844
File: frontend/viewer/src/lib/entry-editor/ItemListItem.svelte:26-37
Timestamp: 2025-07-30T04:53:41.702Z
Learning: In frontend/viewer/src/lib/entry-editor/ItemListItem.svelte, the TODO comments for unused props `index` and `actions` are intentional reminders for future work to be completed in a separate PR, not issues to be resolved immediately. These represent planned functionality that will be implemented later.
Applied to files:
frontend/viewer/src/lib/entry-editor/field-editors/ComplexFormComponents.sveltefrontend/viewer/src/lib/entry-editor/field-editors/ComplexForms.sveltefrontend/viewer/src/project/browse/EntriesList.sveltefrontend/viewer/src/lib/entry-editor/object-editors/EntryEditor.sveltefrontend/viewer/src/lib/entry-editor/object-editors/AddSenseButton.sveltefrontend/viewer/src/lib/entry-editor/object-editors/SenseEditorPrimitive.sveltefrontend/viewer/src/lib/entry-editor/NewEntryDialog.sveltefrontend/viewer/src/project/browse/SearchFilter.sveltefrontend/viewer/src/project/browse/BrowseView.svelte
📚 Learning: 2025-06-18T05:13:00.591Z
Learnt from: hahn-kev
Repo: sillsdev/languageforge-lexbox PR: 1757
File: frontend/viewer/src/lib/components/field-editors/multi-select.svelte:130-136
Timestamp: 2025-06-18T05:13:00.591Z
Learning: In frontend/viewer/src/lib/components/field-editors/multi-select.svelte, the computeCommandScore function from 'bits-ui' handles empty filter strings appropriately and does not hide all options when the filter is empty, contrary to initial analysis assumptions.
Applied to files:
frontend/viewer/src/lib/entry-editor/field-editors/ComplexForms.sveltefrontend/viewer/src/lib/entry-editor/object-editors/SenseEditorPrimitive.sveltefrontend/viewer/src/project/browse/SearchFilter.sveltefrontend/viewer/src/project/browse/BrowseView.svelte
📚 Learning: 2025-07-22T09:19:37.386Z
Learnt from: rmunn
Repo: sillsdev/languageforge-lexbox PR: 1836
File: frontend/viewer/src/lib/components/audio/AudioDialog.svelte:25-25
Timestamp: 2025-07-22T09:19:37.386Z
Learning: In the sillsdev/languageforge-lexbox project, when file size limits or other constants need to be shared between C# backend and TypeScript frontend code, prefer exposing them through Reinforced.Typings type generation rather than hardcoding the values separately. This ensures consistency and prevents discrepancies when values change.
Applied to files:
frontend/viewer/src/project/browse/EntriesList.sveltefrontend/viewer/src/lib/writing-system-service.svelte.ts
📚 Learning: 2025-07-31T16:00:49.635Z
Learnt from: imnasnainaec
Repo: sillsdev/languageforge-lexbox PR: 1867
File: platform.bible-extension/src/types/fw-lite-extension.d.ts:4-22
Timestamp: 2025-07-31T16:00:49.635Z
Learning: In the sillsdev/languageforge-lexbox repository, the platform.bible-extension is intentionally tightly coupled with the frontend's dotnet-types. The relative imports from `../../../frontend/viewer/src/lib/dotnet-types/index.js` in the extension's type declarations are by design, not a maintainability issue that needs to be addressed.
Applied to files:
frontend/viewer/src/project/browse/EntriesList.sveltefrontend/viewer/src/project/browse/BrowseView.svelte
📚 Learning: 2025-08-14T12:54:04.004Z
Learnt from: myieye
Repo: sillsdev/languageforge-lexbox PR: 1906
File: frontend/viewer/src/lib/components/ui/alert-dialog/alert-dialog-root.svelte:11-13
Timestamp: 2025-08-14T12:54:04.004Z
Learning: In Svelte 5, children are rendered using snippets with {render children?.()} syntax instead of the legacy <slot /> approach used in earlier Svelte versions. Components should destructure children from props and use {render children?.()} to render child content.
Applied to files:
frontend/viewer/src/lib/entry-editor/object-editors/ObjectHeader.svelte
📚 Learning: 2025-08-14T12:50:25.135Z
Learnt from: myieye
Repo: sillsdev/languageforge-lexbox PR: 1906
File: frontend/viewer/src/lib/components/ui/dialog-shared/dialog-shared-root.svelte:3-3
Timestamp: 2025-08-14T12:50:25.135Z
Learning: In the dialog-shared-root.svelte component, the module-level `openDialogs` state is intentionally shared across all component instances to coordinate dialog stacking and overlay behavior across the entire application. This enables proper z-index management where newer dialogs appear on top and only the bottom dialog shows its overlay.
Applied to files:
frontend/viewer/src/lib/entry-editor/NewEntryDialog.sveltefrontend/viewer/src/project/browse/SearchFilter.svelte
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Build UI / publish-ui
- GitHub Check: Build API / publish-api
- GitHub Check: check-and-lint
- GitHub Check: Build FW Lite and run tests
- GitHub Check: frontend-component-unit-tests
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (29)
frontend/viewer/src/lib/entry-editor/field-editors/ComplexFormComponents.svelte (1)
11-11: LGTM! Type safety improvement.The introduction of
ReadonlyDeep<IEntry>for the entry prop appropriately enforces immutability, as the component only reads from the entry object and never mutates it.Also applies to: 16-16
frontend/viewer/src/lib/writing-system-service.svelte.ts (1)
15-15: LGTM! Consistent type safety enhancement.The
ReadonlyDeep<IEntry>parameter type for both the class method and standaloneheadwordfunction correctly enforces immutability, as both implementations only read from the entry'scitationFormandlexemeFormproperties.Also applies to: 123-123, 210-210
frontend/viewer/src/lib/entry-editor/field-editors/ComplexForms.svelte (3)
2-5: LGTM: Inline type import is good practice.The inline type import for
EntrySenseSelectionon line 2 correctly distinguishes type-only imports, which is a TypeScript best practice.
16-16: LGTM: Readonly entry type improves immutability.Changing the entry prop to
ReadonlyDeep<IEntry>correctly enforces immutability. All usages ofentrywithin this component (lines 35, 36, 43-45) are read-only operations, making this change safe and beneficial for type safety.
11-11: No issues found with the ReadonlyDeep import.The latest stable version of type-fest is 5.2.0, while the codebase uses 4.41.0. However, verification confirms that ReadonlyDeep is available and unchanged between versions, with no known issues at the version being used. The import is correct and functional as written.
frontend/viewer/src/lib/entry-editor/object-editors/AddSenseButton.svelte (1)
1-14: LGTM!The component is well-structured and correctly implements view-aware translations for the Add Sense/Meaning button. The use of
pt()for both the title and label ensures consistent terminology across different views.frontend/viewer/src/lib/entry-editor/object-editors/SenseEditorPrimitive.svelte (2)
22-31: LGTM! Optional snippet props for field descriptions.The addition of
partOfSpeechDescriptionandsemanticDomainsDescriptionsnippets provides a clean extension point for injecting custom UI adjacent to these fields. The implementation correctly uses optional types and chaining.
81-81: LGTM! Snippet rendering is correctly implemented.The snippet rendering uses correct Svelte 5 syntax with
{@render ...?.()}, and the optional chaining ensures safe rendering when descriptions are not provided.Also applies to: 96-96
frontend/viewer/src/project/browse/SearchFilter.svelte (2)
31-41: LGTM! Props correctly refactored to bindable state.The conversion of
semanticDomainandpartOfSpeechfrom internal state to bindable props enables parent components to control and react to these filter values. The type annotations are correct.
77-83: LGTM! Filter construction logic is correct.The gridify filter construction properly handles semantic domain filtering with subdomain inclusion support and part-of-speech filtering. The operators used (
^for prefix match,=for exact match) align with the gridify filter syntax mentioned in the codebase.frontend/viewer/src/project/browse/BrowseView.svelte (3)
28-29: LGTM! Filter state properly initialized.The new state variables for semantic domain and part-of-speech filtering are correctly typed and initialized.
33-40: LGTM! Pre-selection context passed to new entry dialog.The
newEntryfunction correctly passes the current semantic domain and part-of-speech filter values to the entry creation dialog, enabling the pre-selection behavior described in the PR objectives. The conversion ofsemanticDomainto an array is appropriate for the API.
63-63: LGTM! Filter values correctly bound and passed through component hierarchy.The two-way bindings to
SearchFilterand one-way props toEntriesListestablish the proper data flow for the filtering feature.Also applies to: 74-75
frontend/viewer/src/project/browse/EntriesList.svelte (1)
45-46: LGTM! Props correctly added for filter context.The new props enable
EntriesListto receive the current filter context and pass it to the entry creation dialog.Also applies to: 56-57
frontend/viewer/src/lib/entry-editor/object-editors/EntryEditor.svelte (4)
34-35: LGTM! New components imported for header refactoring.The imports support the consolidation of header rendering logic into dedicated components.
170-178: LGTM! Sense header refactored to use ObjectHeader component.The refactoring correctly extracts sense header rendering to the
ObjectHeadercomponent while preserving sticky positioning behavior and action buttons. The conditional sticky class application based onmodalModeis appropriate.
187-194: LGTM! Example header refactored to use ObjectHeader component.The example header correctly uses
ObjectHeaderwithtype="example". Example headers don't need sticky positioning since they're nested within sense sections, which is the appropriate behavior.
224-224: LGTM! AddSense button refactored to use AddSenseButton component.The extracted button component promotes reusability and consistent styling.
frontend/viewer/src/lib/entry-editor/object-editors/ObjectHeader.svelte (1)
1-38: LGTM! Well-structured header component for sense and example sections.The
ObjectHeadercomponent provides a clean abstraction for rendering section headers with consistent styling and structure. The implementation correctly:
- Uses view-aware translations for "Sense"/"Meaning" terminology
- Supports optional numbering via the
indexprop- Renders optional action buttons via the
childrensnippet- Spreads
HTMLAttributesfor flexibility (e.g., custom classes)frontend/viewer/src/lib/services/dialogs-service.ts (3)
5-5: LGTM! SenseTemplate type imported for new-entry dialog integration.The import enables the dialog service to accept and pass sense template data for pre-population.
23-26: LGTM! Dialog invoker signature updated consistently.Both the private field type and setter correctly include the optional
newSenseparameter, maintaining type safety.
28-37: LGTM! createNewEntry correctly extended with sense template support.The method signature and implementation correctly thread the optional
newSenseparameter through to the dialog invocation, enabling the pre-selection feature described in the PR objectives.frontend/viewer/src/lib/entry-editor/NewEntryDialog.svelte (7)
53-59: Sense assignment increateEntryaligns with “no-sense” behaviorSetting
entry.senses = sense ? [sense] : [];right before validation and save correctly ensures:
- Entries with no sense (after deleting the sense and not re-adding) are created with an empty senses array.
- Validation only requires definition/gloss when a sense is actually present.
This is consistent with the intended UX around deleting a sense and optionally re‑adding it.
81-94: Template-origin flags logic looks correct and matches UX intentThe runes-based effects for
partOfSpeechIsFromTemplateandsemanticDomainIsFromTemplatecorrectly:
- Start as
undefined.- Compute whether the current
sensematches the template values.- Stop updating once they flip to
false, so the “From active filter” label disappears permanently after the user diverges from the template.- Are reset to
undefinedinaddSense(), so a new sense can again show the label if it matches the template.This neatly matches the intended badge semantics without extra bookkeeping.
112-119:addSensematches pre-pop & re-add semantics
addSense()correctly:
- Resets template-origin flags.
- Creates a fresh sense tied to
entry.id.- Clones
senseTemplate.semanticDomainsinto a new array to avoid aliasing.- Merges
senseTemplateovertmpSense, then overridesidandentryIdto ensure they stay coherent.- Updates
entry.senses = [sense].This gives the expected behavior:
- Initial sense is pre-populated from the active filter.
- After deleting, re-adding uses the same pre-pop logic as the original.
136-148: Field visibility logic matches the PR requirementsThe
OverrideFieldsshownFieldsconfiguration meets the described UX:
- Always shows:
lexemeForm,citationForm,gloss,definition,partOfSpeechId— giving consistent PoS visibility.- Only shows
semanticDomainswhen the template provided domains:...(senseTemplate?.semanticDomains?.length ? ['semanticDomains'] as const : [])Because this depends on
senseTemplate(not the mutablesense), the semantic domain UI remains visible even if the user clears the pre-selected domains, which is exactly the requested behavior.
152-180: Sense header & delete/add behavior align with new UX
ObjectHeader type="sense"with just a trash button removes the history button and index, matching the simplified Sense/Meaning actions.- The delete button sets
sense = undefinedwith no confirmation dialog, and the{:else}branch exposes anAddSenseButtonhooked toaddSense(), which reuses the template-based pre-population.Aside from the optional suggestion to also clear
entry.senseswhen deleting (see earlier comment), this flow looks solid and consistent with the PR description.
157-174: Template-derived descriptions are correctly wired intoSenseEditorPrimitiveThe
partOfSpeechDescriptionandsemanticDomainsDescriptionsnippets display the “From active filter” label only when the corresponding*IsFromTemplateflags are truthy, and include a filter icon for clarity.This is a clean way to surface template provenance inside the sense editor without coupling
SenseEditorPrimitiveto filter state.
96-110: No breaking changes—refactoring is properly encapsulated.The search confirms that
openWithValueappears only withinNewEntryDialog.svelte(its definition at line 96 and assignment todialogsService.invokeNewEntryDialogat line 39). There are no direct imports or external usages of this function anywhere in the codebase. The only cross-file reference to NewEntryDialog.svelte is a type-only import indialogs-service.ts, which remains unaffected. All dialog invocations now correctly flow throughdialogsService, as intended.
…ollect-words-by-semantic-domain-feature
myieye
deleted the
2103-rudimentary-collect-words-by-semantic-domain-feature
branch
Resolves #2103 (https://community.software.sil.org/t/new-word-dialog-should-contain-part-of-speech/10796)
Also implements: https://community.software.sil.org/t/collect-words-by-semantic-domain/10949/1
When filtering by a semantic domain, that semantic domain is now pre-selected when creating new entries:
Note Semantic domains are not visible in the new entry dialog unless there's a pre-selection:
But if there was a pre-selection and that selection is cleared we do not suddenly hide the semantic domains:
Scope creep:
I just went ahead and did the same thing for part of speech:
It's been a requested feature to also show part of speech in the new entry dialog, so that happened along the way here. We always show it (i.e. not only when there's a pre-selected PoS).
We were incorrectly showing the Sense/Meaning history button in the dialog, because I touched that code anyway, I fixed it so that we're only showing the delete button:
Before:
After:
Note that the index/"1" is also gone now, because it was meaningless.
Final change:
The delete sense button doesn't have a confirmation dialog anymore. I don't think it's important.
A user would presumably click it, because they don't want to provide a gloss. So, the confirmation just adds extra clicks.
Also, the chance of losing something important is minimal, because the sense hasn't been created yet.
But to counter the new ease of deletion, you can now re-add the sense: (which uses the same pre-population code as the originally generated sense)