Open
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughImplements custom view (layout) creation and synchronization across the full stack: backend adds Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
github-actions
bot
added
the
💻 FW Lite
Contributor
Contributor
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
rmunn
reviewed
Mar 23, 2026
Contributor
rmunn
left a comment
rmunn
left a comment
There was a problem hiding this comment.
Review still in process, got as far as EditCustomView and haven't looked at every file yet. But I had a few comments that I might as well send you now and not make you wait longer for them.
| public virtual Guid Id { get; set; } | ||
| public DateTimeOffset? DeletedAt { get; set; } | ||
|
|
||
| public virtual required string Name { get; set; } |
Contributor
There was a problem hiding this comment.
Do we want the extra complexity of a MultiString here? Or are we going to assume that each project will be using one language for the project config, and will not want, e.g., to be able to name their custom views in both English and German?
I'm almost certain I agree with the choice of a single-language string here rather than a MultiString, but I do want to make sure we have thought about that decision.
Collaborator
Author
There was a problem hiding this comment.
Yeah, good question. I wouldn't even know which writing-systems we should offer them to provide names for. Vernacular? Analysis? Both?
I'm expecting custom view names to be project-specific enough that users will be able to pick a name that all collaborators understand. So, yeah, I'd just opt for simple strings.
myieye
force-pushed
the
feature/custom-view-model
branch
from
45bf07b to
0bc4275
Compare
Contributor
|
I want to record in writing our discussion about custom tasks that we had a few weeks ago. The idea was that in some cases, the project manager might want to define certain tasks that aren't among the ones we had defined (missing Part of Speech, etc.). For those, they might want to reorder certain fields, e.g. they might want a task that combines "missing Gloss" and "missing Definition", but putting Definition before Gloss rather than after it. So a custom task would involve the following:
After that discussion, we decided that reordering fields in custom views was not necessary, and for those cases where a different field order might be desired, a custom task would fit every use case we could come up with. |
rmunn
reviewed
Mar 24, 2026
Contributor
rmunn
left a comment
rmunn
left a comment
There was a problem hiding this comment.
There's currently a bug where the UI will get "stuck" to whichever custom view you created last, and you can no longer select the original FW Classic or FW Lite views. The View radio button group does nothing, and has no buttons selected.
I have more comments below, but haven't dived into the UI code much yet. I'll have more comments on it later, but wanted to report this bug right away.
Comment on lines
+40
to
+45
| migrationBuilder.CreateIndex( | ||
| name: "IX_CustomView_SnapshotId", | ||
| table: "CustomView", | ||
| column: "SnapshotId", | ||
| unique: true); | ||
| } |
Contributor
There was a problem hiding this comment.
Wondering if we'll want an index on the Name column. Probably not worth the extra DB space, there won't be that many custom views so a sequential scan (which is what typically happens in the absence of an index) won't actually take that long. And we just might be loading all custom views into the frontend anyway, in which case the index would be a waste of bytes.
Collaborator
Author
There was a problem hiding this comment.
Yeah, I expect we'll always query all or by ID.
Comment on lines
+848
to
+853
| private void AssertManagerRoleForCustomViewWrite() | ||
| { | ||
| if (ProjectData.Role == UserProjectRole.Manager) return; | ||
| throw new UnauthorizedAccessException( | ||
| $"Only managers can manage custom views."); | ||
| } |
Contributor
There was a problem hiding this comment.
Not sure this belongs in the API. I think this belongs in the controllers or the GraphQL layer. The API will be called when updates come in via a sync that were created by other people, i.e. I'm a user and the EditCustomView change was created by a manager, but I'm the one who's currently logged in as UpdateCustomView is being called by the sync code.
Collaborator
Author
There was a problem hiding this comment.
Actually, the API is not used when updates come in via a sync. The API is only used for creating change objects, not for receiving change objects. Received change objects are applied directly to the DB/data model.
We also assert read-permission here in the API, which is the same kind of authorization validation.
We could also put the validation in our JsInvokable class, but that's essentially just an adapter so that we can plug our APIs into the browser. If we add a web API in the future, that would also point at this code here and should "inherit" the same validation.
Comment on lines
+44
to
+55
| // Visibility = presence in the array. Field order in the list is ignored; builtin order is used, | ||
| // If view-level ordering is desired, an explicit flag should be introduced on the view, | ||
| // since project-level field ordering is more likely. | ||
| public virtual ViewField[] EntryFields { get; set; } = []; | ||
| public virtual ViewField[] SenseFields { get; set; } = []; | ||
| public virtual ViewField[] ExampleFields { get; set; } = []; | ||
|
|
||
| // Visibility = presence in the array. WS order comes from the project. | ||
| // If view-level WS ordering were introduced, it should probably propagate to views, | ||
| // so an explicit flag would be appropriate. | ||
| public virtual ViewWritingSystem[]? Vernacular { get; set; } // null = inherit project defaults | ||
| public virtual ViewWritingSystem[]? Analysis { get; set; } // null = inherit project defaults |
Contributor
There was a problem hiding this comment.
These are intended to be immutable within the lifetime of a CustomView object, right? If a custom view is changed, this would result in a new object being loaded from the DB? If so, it might be useful to have a public method IsVisible, taking either a ViewField or a ViewWritingSystem. It would, when first called, create a HashSet from the field ids, storing it in the CustomView object. Then lookup for each field is O(1); there are enough Entry and Sense fields that O(N^2) starts to become noticeable.
... But maybe not. I haven't benchmarked this, and maybe O(N^2) where N is 10 is tiny and not noticeable even on a 5-year-old smartphone. In which case, just reading through the standard fields and checking each one against the EntryFields array is fine for simplicity's sake.
Collaborator
Author
There was a problem hiding this comment.
I'm not sure if we'll ever check field visibility in C#. We'd have to add that in TS.
I actually do create a Record<string, Field> in the editor component.
|
|
||
| export function useViewService(): ViewService { | ||
| const service = getContext<ViewService>(contextKey); | ||
| if (!service) throw new Error('ViewService not initialized. Did you forget to call initViewService()?'); |
Contributor
There was a problem hiding this comment.
What is the purpose of requiring service consumers to call init() first? Is it simply so that the persist option can be set appropriately in one place, then all subsequent calls to useViewService get the same object? I tend to dislike APIs that require you to call init() before they're usable, and I always wonder whether the API could have called init() for me with default options. Is that possible here? Or is the papercut of having to call init() small enough that we should just make init() be called every time, because it's rather important whether or not persist will be true or false in different places?
Collaborator
Author
There was a problem hiding this comment.
Yes, the persist option was the original reason that I wanted to force callers to intentionally initialize it first.
I was just changing it to call init with no args/the default instead of throwing.
Then I realized that we're using svelte-context here. So, unless we initialize it higher up we're going to end up with multiple instances of the ViewService, which isn't necessarily problematic, but it's undesirable.
Other similar services use our ProjectContext's getOrAdd. But that's for project-wide singletons.
This is almost a singleton, but not, because we intentionaly override it sometimes.
So, I think I like the forced-init pattern here. And init doesn't have to be called "every time". It just needs to be called once at the project root. Then only components that want to override it need to call init again.
| } | ||
|
|
||
| filterAndSortWs(writingSystems: IWritingSystem[], override?: string[]) { | ||
| filterAndSortWs(writingSystems: IWritingSystem[], override?: IViewWritingSystem[]) { |
Contributor
There was a problem hiding this comment.
This function doesn't do any sorting. Should we rename it to filterWs?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/FwLite/LcmCrdt/CrdtProjectsService.cs (1)
242-248:⚠️ Potential issue | 🔴 CriticalCustom view writing system references must exist or be validated. The
PredefinedCustomViewsmethod seeds a view with hardcoded writing system IDs ("de", "en", "en-Zxxx-x-audio") that don't exist whenSeedSystemDataruns. While this works for example projects becauseSampleProjectDatacreates these systems afterward, projects withSeedNewProjectData=truebut noAfterCreatecallback would have dangling references. Either ensure these writing systems are created before or duringSeedSystemData, or usenullvalues for Vernacular/Analysis to inherit project defaults, or add validation to prevent references to non-existent writing systems.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/FwLite/LcmCrdt/CrdtProjectsService.cs` around lines 242 - 248, SeedSystemData currently calls PredefinedCustomViews which seeds custom views that reference hardcoded writing system IDs ("de","en","en-Zxxx-x-audio"); ensure those writing systems exist or avoid dangling refs by either (1) creating those writing systems inside SeedSystemData before calling PredefinedCustomViews (add creation/ensure logic), (2) modify PredefinedCustomViews to use null for Vernacular/Analysis where appropriate so project defaults are inherited, or (3) add validation inside PredefinedCustomViews (or called helper) to throw/skip if a referenced writing system doesn't exist; update the implementation of SeedSystemData/PredefinedCustomViews accordingly and keep SampleProjectData/AfterCreate behavior unchanged.frontend/viewer/src/lib/views/OverrideFields.svelte (1)
14-19:⚠️ Potential issue | 🟡 Minor
shownFields = []default will filter out all fields, though currently all usages explicitly provide this prop.While the empty
shownFieldsarray default will indeed filter out all fields (line 36), all current usages explicitly passshownFields: SubjectPopup passestask.subjectFields, NewEntryDialog passes a concrete field list, and audio-input.svelte even intentionally passes an empty array via ternary (line 295:fieldProps?.fieldId ? [fieldProps.fieldId] : []).This suggests the component design expects callers to always provide
shownFieldsexplicitly. However, if future callers omit this prop, they would unexpectedly hide all content. Consider clarifying the intent:
- If the component should always require explicit
shownFields, update the type to make it required (remove the default)- If the component should show all fields when
shownFieldsis omitted, add the conditional:if (shownFields.length === 0) return entityFields;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/viewer/src/lib/views/OverrideFields.svelte` around lines 14 - 19, The defaulting of shownFields to [] causes silent filtering when callers omit the prop; make shownFields required instead by removing the default in the destructuring (replace "shownFields = []" with "shownFields") and update the Props type so shownFields is non-optional/required; ensure the rest of the component (e.g., logic using shownFields) assumes it exists and update any type imports if necessary.
🧹 Nitpick comments (7)
frontend/viewer/src/lib/views/custom/CustomViewEntityFields.svelte (1)
20-22: Consider usingObject.hasOwninstead ofinoperator.The
inoperator (line 21) checks the entire prototype chain, which could include inherited properties. For checking if a field ID exists as an own property in the entity config,Object.hasOwn(entity, item.fieldId)is more precise.♻️ Suggested fix
function toCommittedFieldItems(next: IViewField[]): IViewField[] { - return next.filter((item) => item.fieldId in entity).map((item) => ({fieldId: item.fieldId})); + return next.filter((item) => Object.hasOwn(entity, item.fieldId)).map((item) => ({fieldId: item.fieldId})); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/viewer/src/lib/views/custom/CustomViewEntityFields.svelte` around lines 20 - 22, Replace the prototype-chain check in toCommittedFieldItems by using Object.hasOwn to test for an own property: in the toCommittedFieldItems(next: IViewField[]) function, change the filter predicate from "item.fieldId in entity" to using Object.hasOwn(entity, item.fieldId) so the function only matches own properties of entity when mapping to committed field items.frontend/viewer/src/lib/views/custom/validation.test.ts (1)
14-15: Minor: Test data uses potentially confusing language codes.
vernacular: [{wsId: 'en'}]andanalysis: [{wsId: 'fr'}]use codes that might be confusing since 'en' (English) is typically an analysis language in linguistic fieldwork contexts. Consider using more typical codes like'xyz'for vernacular and'en'for analysis, or adding a comment clarifying these are just test placeholders.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/viewer/src/lib/views/custom/validation.test.ts` around lines 14 - 15, Update the test fixture in validation.test.ts so the vernacular and analysis writing-system codes are not confusing: replace vernacular: [{wsId: 'en'}] and analysis: [{wsId: 'fr'}] with clearer test codes (e.g., vernacular: [{wsId: 'xyz'}] and analysis: [{wsId: 'en'}]) or add an inline comment next to the vernacular and analysis entries to state they're arbitrary placeholders; adjust the IViewWritingSystem arrays accordingly so tests remain semantically clear.frontend/viewer/src/lib/views/custom/CreateCustomViewDialog.svelte (1)
14-17: Consider handling errors inhandleCreate.If
onCreatethrows an error, the dialog remains open but the user has no feedback about what went wrong. This could be intentional (letting the parent handle errors), but consider whether error feedback is needed here.♻️ Optional: Add try-catch for error feedback
async function handleCreate(result: CustomView) { + try { await onCreate(result); open = false; + } catch (e) { + // Error handling is delegated to onCreate callback + // Dialog stays open so user can retry + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/viewer/src/lib/views/custom/CreateCustomViewDialog.svelte` around lines 14 - 17, handleCreate currently awaits onCreate(result) then closes the dialog, but if onCreate throws the dialog stays open with no user feedback; wrap the await call in a try-catch inside handleCreate, on success set open = false, on error set a local error state (e.g. createError) or call the existing notification function and keep the dialog open (or rethrow if parent should handle), and ensure the UI binds to createError to display the message; update references to handleCreate and onCreate accordingly.frontend/viewer/src/project/data/custom-view-service.test.ts (1)
19-24: Force the create tests to use a server-assigned id.The add fixtures start from
FW_LITE_VIEW, so they already carry the built-inFwLiteid. Because the mock echoesargs.id, a regression that posts that built-in id tocreateCustomViewwould still pass here, and the create flow could end up selecting the built-in view instead of the new custom one. Make the mock always return a fresh id and assert that the payload does not reuse a built-in id.As per coding guidelines, "Always double-check that test 'fixes' haven't made checks or tests meaningless (e.g., asserting expect(true).toBe(true))."🧪 Proposed test hardening
const apiSpy = { - createCustomView: vi.fn((args: ICustomView) => Promise.resolve({...args, id: args.id ?? 'new-id'})), + createCustomView: vi.fn((args: ICustomView) => Promise.resolve({...args, id: 'new-id'})), updateCustomView: vi.fn((_id: string, args: ICustomView) => Promise.resolve(args)), deleteCustomView: vi.fn(() => Promise.resolve()), };await service.add(view); const sent: ICustomView = apiSpy.createCustomView.mock.calls[0][0]; + expect(sent.id).not.toBe(ViewBase.FwLite); + expect(sent.id).not.toBe(ViewBase.FieldWorks); const sentFieldIds = sent.entryFields.map(f => f.fieldId);Also applies to: 80-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/viewer/src/project/data/custom-view-service.test.ts` around lines 19 - 24, The createCustomView mock in createMockService currently echoes args.id which can mask regressions that reuse the built-in FwLite id; change the createCustomView implementation so it ignores args.id and always returns a freshly generated id (e.g., a fixed "new-id" or generated value) instead of echoing the input, and update the related create tests to assert that the payload passed into createCustomView did not contain the built-in FW_LITE_VIEW/FwLite id and that the flow uses the server-assigned id returned by the mock; apply the same hardening to other create tests in the file (around the create test block referenced) so they don’t pass if the client reuses a built-in id.frontend/viewer/src/project/data/custom-view-service.svelte.ts (2)
71-75: Comment mentions "ordering" but implementation only applies visibility.The JSDoc states "Applies API visibility and ordering to the default fields," but the implementation iterates over
defaults(preserving their order) and only sets visibility based on API field presence. If field ordering from the API is intentional, consider reordering; otherwise, update the comment to reflect the actual behavior.📝 Suggested comment fix (if ordering is not needed)
- /** Applies API visibility and ordering to the default fields, backfilling any missing fields as hidden. */ + /** Applies API visibility to the default fields, backfilling any missing fields as hidden. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/viewer/src/project/data/custom-view-service.svelte.ts` around lines 71 - 75, The JSDoc for `#resolveViewFields` says it applies "API visibility and ordering" but the implementation only applies visibility and preserves the order of defaults; update either the implementation or the comment. If ordering is intended, change `#resolveViewFields` to produce fields in the API-specified order (use apiFields to map order, backfill missing defaults as hidden); if ordering is not intended, simply update the JSDoc to state that it only sets visibility based on apiFields while keeping the defaults order. Reference symbols: `#resolveViewFields`, apiFields, defaults, TypedViewField, IViewField.
62-69: Frontend-only properties may leak to API payload.The spread
...customViewincludes the frontend-onlycustom: trueproperty (and potentially other view-specific fields likeroot). While the backend may ignore unknown properties, explicitly picking only API-relevant fields would be cleaner and prevent accidental payload bloat.♻️ Suggested fix
`#toApiCustomView`(customView: View): ICustomView { return { - ...customView, + id: customView.id, + name: customView.name, + base: customView.base, + vernacular: customView.vernacular, + analysis: customView.analysis, entryFields: customView.entryFields.filter(field => field.show), senseFields: customView.senseFields.filter(field => field.show), exampleFields: customView.exampleFields.filter(field => field.show), }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/viewer/src/project/data/custom-view-service.svelte.ts` around lines 62 - 69, The toApiCustomView function currently spreads ...customView which can leak frontend-only properties (e.g. custom: true, root) into the API payload; replace the spread with an explicit object containing only API-relevant fields (e.g. id, name, description, visibility, and the filtered entryFields/senseFields/exampleFields) so the result includes the filtered field arrays and only the expected view properties, ensuring no frontend-only fields are sent to the server.frontend/viewer/src/lib/views/view-data.ts (1)
17-27: Consider makingRootViewandCustomViewmutually exclusive in the type system.The implementation correctly tags API-backed views with
custom: trueinCustomViewService.#buildCustomView()and built-in views withroot: true. However, the interfaces don't prevent a view from having both discriminants. Addcustom?: nevertoRootViewandroot?: nevertoCustomViewto enforce mutual exclusivity at the type level and improve clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/viewer/src/lib/views/view-data.ts` around lines 17 - 27, Make RootView and CustomView mutually exclusive by adding discriminant-exclusion fields: change RootView to include custom?: never and change CustomView to include root?: never so a view cannot have both discriminants; update the TypeScript interfaces RootView and CustomView in view-data.ts (and keep isCustomView(view: View): view is CustomView unchanged) so the compiler enforces exclusivity and matches how CustomViewService.#buildCustomView produces custom-tagged views and built-in views get root tagged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyDbModel.verified.txt`:
- Around line 103-124: Update the CustomView entity and EF Core mapping to add
the missing DefaultAsOf and DefaultFilter properties and make the Analysis and
Vernacular arrays non-nullable with jsonb storage; specifically add properties
DefaultAsOf and DefaultFilter to the CustomView class, update the
DbContext/OnModelCreating mapping for CustomView to configure Analysis and
Vernacular as Required() with Relational:ColumnType = "jsonb" and configure
defaults for DefaultAsOf/DefaultFilter (or appropriate column settings), then
add a migration and regenerate the snapshot so the snapshot output includes
DefaultAsOf, DefaultFilter, and required Analysis/Vernacular fields in the
persisted model.
In `@backend/FwLite/LcmCrdt.Tests/MiniLcmTests/CustomViewTests.cs`:
- Around line 1-3: The new CustomViewTests class
(CustomViewTests(MiniLcmApiFixture) : IClassFixture<MiniLcmApiFixture>) contains
shared assertions that must be moved into a TestsBase-style base class in the
MiniLcm.Tests project; create a MiniLcm.Tests base test class (e.g.,
CustomViewTestsBase) that accepts the necessary fixtures and contains all shared
test methods/assertions, then replace the current CustomViewTests with thin
derived classes in both LcmCrdt.Tests/MiniLcmTests and FwDataMiniLcmBridge.Tests
that inherit from CustomViewTestsBase and only provide the concrete
MiniLcmApiFixture setup/ctor. Ensure method names and signatures from the
original (shared) tests are preserved in the base class so both implementations
run the same assertions.
In `@backend/FwLite/LcmCrdt/Changes/CreateCustomViewChange.cs`:
- Around line 13-19: The CreateCustomViewChange class currently stores direct
references to the caller's arrays (EntryFields, SenseFields, ExampleFields,
Vernacular, Analysis), so change the assignments in the CreateCustomViewChange
constructor to clone each array (and, if ViewField/ViewWritingSystem are mutable
reference types, deep-clone each element) instead of assigning the reference,
and apply the same defensive cloning again inside NewEntity() before
materializing the CustomView; this ensures CreateCustomViewChange and
NewEntity() hold independent, immutable copies of the field and writing-system
arrays.
- Around line 27-33: CreateCustomViewChange is missing fields to persist the
view's default-selection and filter state; add properties DefaultAsOf (e.g.,
DateTime? or matching model type) and DefaultFilter (matching the filter
DTO/type used by CustomView) to the class alongside Name, Base, EntryFields,
SenseFields, ExampleFields, Vernacular, and Analysis, and update its
constructor/serialization/mapping code (the methods that convert to/from the
synced CustomView model) to include these two properties so the DefaultAsOf and
DefaultFilter round-trip through persistence and sync.
In `@backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs`:
- Around line 831-837: UpdateCustomView currently forwards the provided
customView without ensuring its Id matches the route parameter id; update the
method (UpdateCustomView) to explicitly validate/normalize the payload Id:
either throw a BadRequest/ArgumentException if customView.Id is non-empty and !=
id, or set customView.Id = id before creating the EditCustomViewChange so the
change uses the route Id; update the call site that creates new
EditCustomViewChange(existing.Id, customView) accordingly and add a regression
test that submits a mismatched Id to ensure the API rejects or normalizes it as
implemented.
In `@backend/FwLite/LcmCrdt/Migrations/LcmCrdtDbContextModelSnapshot.cs`:
- Around line 170-213: The CustomView snapshot is missing the DefaultAsOf and
DefaultFilter columns; update the entity mapping for MiniLcm.Models.CustomView
to add b.Property<DateTimeOffset?>("DefaultAsOf").HasColumnType("TEXT"); and
b.Property<string>("DefaultFilter").HasColumnType("jsonb"); (or the appropriate
nullable types you use for timestamp and JSON filter), then update the
CustomView model class to include DefaultAsOf and DefaultFilter properties and
regenerate the LcmCrdtDbContextModelSnapshot so the new columns are persisted
and index/migration snapshots are up to date.
In `@frontend/viewer/src/lib/components/ui/checkbox/checkbox-item.svelte`:
- Line 25: The conditional for adding the cursor class is inverted: update the
class expression inside the Label component where cn('flex items-center gap-4
md:gap-2 max-md:py-3', restProps.disabled || 'cursor-pointer') is used so that
the 'cursor-pointer' class is added only when the item is NOT disabled (e.g.,
use a negation like !restProps.disabled && 'cursor-pointer' or a ternary),
keeping the rest of the cn call and props intact; reference the Label usage and
the restProps.disabled symbol to locate the change.
In
`@frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/ICustomView.ts`:
- Around line 11-22: Add the missing default-view contract fields by updating
the .NET model CustomView to include the properties defaultAsOf (nullable
DateTime or appropriate type) and defaultFilter (string or appropriate filter
type), then regenerate the TypeScript interfaces so ICustomView includes
defaultAsOf and defaultFilter; ensure the generated ICustomView interface (in
MiniLcm/Models/ICustomView.ts) lists defaultAsOf? and defaultFilter? with the
correct types matching the .NET model to allow frontend/backend round-tripping
of the default custom-view state.
In `@frontend/viewer/src/lib/views/custom/CustomViewForm.svelte`:
- Around line 59-74: Clear any previous error at the start of submit() (set
error = null) to avoid stale errors; then perform validation as now, and wrap
the await onSubmit(value) call in try/catch/finally so that saving is still
reset in finally but any exception from onSubmit is caught and surfaced to the
user by setting error to a translated message (use $t(pt(...,
viewService.currentView)) or include the thrown error.message), rather than
swallowing it—keep value.name = value.name.trim() before submit and reference
the submit(), validateForm(), onSubmit(), saving, value and
viewService.currentView symbols when making this change.
In `@frontend/viewer/src/lib/views/custom/ManageCustomViewsDialog.svelte`:
- Around line 35-37: openEdit currently does a shallow copy (editValue = {
...view }) so nested objects/arrays like entryFields, senseFields,
exampleFields, vernacular, and analysis remain shared; change it to perform a
deep clone of the passed CustomView (e.g., use structuredClone(view) or a
project deepClone helper) before assigning to editValue so edits/cancel do not
mutate the live view, then set editOpen = true as before.
- Around line 71-95: The row currently renders as a native <button> containing a
nested Button (which also renders a <button>), causing invalid nested buttons;
change the outer row element to a non-button container (e.g., <div class="flex
w-full ...">) and move the click handler that invokes openEdit(view) onto that
container (use Svelte's on:click={() => openEdit(view)}), keep the delete
control as the Button component calling onDelete(view) (with
e.stopPropagation()) so it remains a sibling control, and ensure accessibility
by adding role="button" and tabindex="0" and handling keydown Enter/Space to
call openEdit(view) if keyboard activation is needed.
In `@frontend/viewer/src/lib/views/custom/validation.ts`:
- Around line 8-20: The validator currently checks for headword presence by
looking for fieldId values in validateForm but ignores visibility; update
validateForm to consider only visible entryFields (filter where field.show ===
true) when computing hasHeadword (i.e., check params.entryFields.filter(f =>
f.show).some(f => f.fieldId === 'lexemeForm' || f.fieldId === 'citationForm')).
Also change the params typing for entryFields/senseFields/exampleFields to use
the editable frontend field shape (the editable field interface used by the
custom-view editor) so the compiler enforces the presence of the show property
and catches hidden-field cases at compile time.
In `@frontend/viewer/src/project/browse/ViewPicker.svelte`:
- Around line 57-59: Replace the hardcoded "Classic" and "Lite" labels with
Lingui template-literal translations so they are extracted for i18n; inside
ViewPicker.svelte where you currently render ({view.base === ViewBase.FieldWorks
? 'Classic' : 'Lite'}) (within the isCustomView(view) block), swap the raw
strings for $t`Classic` and $t`Lite` respectively (using the Svelte Lingui `$t`
template literal syntax) so the view base label becomes ({view.base ===
ViewBase.FieldWorks ? $t`Classic` : $t`Lite`}).
In `@frontend/viewer/src/project/demo/in-memory-demo-api.ts`:
- Around line 179-183: supportedFeatures() currently advertises audio support
while getFileStream() and saveFile() return NotSupported; change the demo to not
advertise audio by returning audio: false in supportedFeatures() (the object
satisfying IMiniLcmFeatures) until getFileStream() and saveFile() are
implemented, so UI won't enable audio paths prematurely.
---
Outside diff comments:
In `@backend/FwLite/LcmCrdt/CrdtProjectsService.cs`:
- Around line 242-248: SeedSystemData currently calls PredefinedCustomViews
which seeds custom views that reference hardcoded writing system IDs
("de","en","en-Zxxx-x-audio"); ensure those writing systems exist or avoid
dangling refs by either (1) creating those writing systems inside SeedSystemData
before calling PredefinedCustomViews (add creation/ensure logic), (2) modify
PredefinedCustomViews to use null for Vernacular/Analysis where appropriate so
project defaults are inherited, or (3) add validation inside
PredefinedCustomViews (or called helper) to throw/skip if a referenced writing
system doesn't exist; update the implementation of
SeedSystemData/PredefinedCustomViews accordingly and keep
SampleProjectData/AfterCreate behavior unchanged.
In `@frontend/viewer/src/lib/views/OverrideFields.svelte`:
- Around line 14-19: The defaulting of shownFields to [] causes silent filtering
when callers omit the prop; make shownFields required instead by removing the
default in the destructuring (replace "shownFields = []" with "shownFields") and
update the Props type so shownFields is non-optional/required; ensure the rest
of the component (e.g., logic using shownFields) assumes it exists and update
any type imports if necessary.
---
Nitpick comments:
In `@frontend/viewer/src/lib/views/custom/CreateCustomViewDialog.svelte`:
- Around line 14-17: handleCreate currently awaits onCreate(result) then closes
the dialog, but if onCreate throws the dialog stays open with no user feedback;
wrap the await call in a try-catch inside handleCreate, on success set open =
false, on error set a local error state (e.g. createError) or call the existing
notification function and keep the dialog open (or rethrow if parent should
handle), and ensure the UI binds to createError to display the message; update
references to handleCreate and onCreate accordingly.
In `@frontend/viewer/src/lib/views/custom/CustomViewEntityFields.svelte`:
- Around line 20-22: Replace the prototype-chain check in toCommittedFieldItems
by using Object.hasOwn to test for an own property: in the
toCommittedFieldItems(next: IViewField[]) function, change the filter predicate
from "item.fieldId in entity" to using Object.hasOwn(entity, item.fieldId) so
the function only matches own properties of entity when mapping to committed
field items.
In `@frontend/viewer/src/lib/views/custom/validation.test.ts`:
- Around line 14-15: Update the test fixture in validation.test.ts so the
vernacular and analysis writing-system codes are not confusing: replace
vernacular: [{wsId: 'en'}] and analysis: [{wsId: 'fr'}] with clearer test codes
(e.g., vernacular: [{wsId: 'xyz'}] and analysis: [{wsId: 'en'}]) or add an
inline comment next to the vernacular and analysis entries to state they're
arbitrary placeholders; adjust the IViewWritingSystem arrays accordingly so
tests remain semantically clear.
In `@frontend/viewer/src/lib/views/view-data.ts`:
- Around line 17-27: Make RootView and CustomView mutually exclusive by adding
discriminant-exclusion fields: change RootView to include custom?: never and
change CustomView to include root?: never so a view cannot have both
discriminants; update the TypeScript interfaces RootView and CustomView in
view-data.ts (and keep isCustomView(view: View): view is CustomView unchanged)
so the compiler enforces exclusivity and matches how
CustomViewService.#buildCustomView produces custom-tagged views and built-in
views get root tagged.
In `@frontend/viewer/src/project/data/custom-view-service.svelte.ts`:
- Around line 71-75: The JSDoc for `#resolveViewFields` says it applies "API
visibility and ordering" but the implementation only applies visibility and
preserves the order of defaults; update either the implementation or the
comment. If ordering is intended, change `#resolveViewFields` to produce fields in
the API-specified order (use apiFields to map order, backfill missing defaults
as hidden); if ordering is not intended, simply update the JSDoc to state that
it only sets visibility based on apiFields while keeping the defaults order.
Reference symbols: `#resolveViewFields`, apiFields, defaults, TypedViewField,
IViewField.
- Around line 62-69: The toApiCustomView function currently spreads
...customView which can leak frontend-only properties (e.g. custom: true, root)
into the API payload; replace the spread with an explicit object containing only
API-relevant fields (e.g. id, name, description, visibility, and the filtered
entryFields/senseFields/exampleFields) so the result includes the filtered field
arrays and only the expected view properties, ensuring no frontend-only fields
are sent to the server.
In `@frontend/viewer/src/project/data/custom-view-service.test.ts`:
- Around line 19-24: The createCustomView mock in createMockService currently
echoes args.id which can mask regressions that reuse the built-in FwLite id;
change the createCustomView implementation so it ignores args.id and always
returns a freshly generated id (e.g., a fixed "new-id" or generated value)
instead of echoing the input, and update the related create tests to assert that
the payload passed into createCustomView did not contain the built-in
FW_LITE_VIEW/FwLite id and that the flow uses the server-assigned id returned by
the mock; apply the same hardening to other create tests in the file (around the
create test block referenced) so they don’t pass if the client reuses a built-in
id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5e2d9ec4-735d-409f-b0e6-5c74d0cad475
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (106)
backend/FwLite/FwLiteShared/Services/MiniLcmJsInvokable.csbackend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.csbackend/FwLite/LcmCrdt.Tests/Changes/ChangeDeserializationRegressionData.latest.verified.txtbackend/FwLite/LcmCrdt.Tests/Changes/UseChangesTests.csbackend/FwLite/LcmCrdt.Tests/ConfigRegistrationTests.csbackend/FwLite/LcmCrdt.Tests/Data/SnapshotDeserializationRegressionData.latest.verified.txtbackend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyChangeModels.verified.txtbackend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyDbModel.verified.txtbackend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyIObjectWithIdModels.verified.txtbackend/FwLite/LcmCrdt.Tests/MiniLcmTests/CustomViewTests.csbackend/FwLite/LcmCrdt/Changes/CreateCustomViewChange.csbackend/FwLite/LcmCrdt/Changes/EditCustomViewChange.csbackend/FwLite/LcmCrdt/CrdtMiniLcmApi.csbackend/FwLite/LcmCrdt/CrdtProjectsService.csbackend/FwLite/LcmCrdt/Data/MiniLcmRepository.csbackend/FwLite/LcmCrdt/LcmCrdtDbContext.csbackend/FwLite/LcmCrdt/LcmCrdtKernel.csbackend/FwLite/LcmCrdt/Migrations/20260218112222_AddCustomViews.Designer.csbackend/FwLite/LcmCrdt/Migrations/20260218112222_AddCustomViews.csbackend/FwLite/LcmCrdt/Migrations/LcmCrdtDbContextModelSnapshot.csbackend/FwLite/LcmCrdt/Objects/PreDefinedData.csbackend/FwLite/MiniLcm/IMiniLcmReadApi.csbackend/FwLite/MiniLcm/IMiniLcmWriteApi.csbackend/FwLite/MiniLcm/Models/CustomView.csbackend/FwLite/MiniLcm/Models/IObjectWithId.csfrontend/viewer/.storybook/decorators/FWLiteDecorator.sveltefrontend/viewer/package.jsonfrontend/viewer/src/ShadcnProjectView.sveltefrontend/viewer/src/lib/DialogsProvider.sveltefrontend/viewer/src/lib/activity/ActivityItemChangePreview.sveltefrontend/viewer/src/lib/components/editor/field/field-root.sveltefrontend/viewer/src/lib/components/editor/field/field-title.sveltefrontend/viewer/src/lib/components/responsive-dialog/responsive-dialog.sveltefrontend/viewer/src/lib/components/ui/checkbox/checkbox-group.sveltefrontend/viewer/src/lib/components/ui/checkbox/checkbox-item.sveltefrontend/viewer/src/lib/components/ui/checkbox/index.tsfrontend/viewer/src/lib/components/ui/radio-group/radio-group-item.sveltefrontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IMiniLcmJsInvokable.tsfrontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/ICustomView.tsfrontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/IViewField.tsfrontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/IViewWritingSystem.tsfrontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/ViewBase.tsfrontend/viewer/src/lib/dotnet-types/index.tsfrontend/viewer/src/lib/entry-editor/EditEntryDialog.sveltefrontend/viewer/src/lib/entry-editor/EntryOrSenseItemList.sveltefrontend/viewer/src/lib/entry-editor/EntryOrSensePicker.sveltefrontend/viewer/src/lib/entry-editor/NewEntryButton.sveltefrontend/viewer/src/lib/entry-editor/NewEntryDialog.sveltefrontend/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.sveltefrontend/viewer/src/lib/entry-editor/object-editors/AddSenseFab.sveltefrontend/viewer/src/lib/entry-editor/object-editors/EntryEditor.sveltefrontend/viewer/src/lib/entry-editor/object-editors/EntryEditorPrimitive.sveltefrontend/viewer/src/lib/entry-editor/object-editors/ExampleEditorPrimitive.sveltefrontend/viewer/src/lib/entry-editor/object-editors/ObjectHeader.sveltefrontend/viewer/src/lib/entry-editor/object-editors/SenseEditorPrimitive.sveltefrontend/viewer/src/lib/sandbox/EditorSandbox.sveltefrontend/viewer/src/lib/sandbox/Sandbox.sveltefrontend/viewer/src/lib/services/dialogs-service.tsfrontend/viewer/src/lib/storage/project-storage.svelte.tsfrontend/viewer/src/lib/views/OverrideFields.sveltefrontend/viewer/src/lib/views/ViewT.sveltefrontend/viewer/src/lib/views/custom/CreateCustomViewDialog.sveltefrontend/viewer/src/lib/views/custom/CustomViewEntityFields.sveltefrontend/viewer/src/lib/views/custom/CustomViewForm.sveltefrontend/viewer/src/lib/views/custom/CustomViewWritingSystems.sveltefrontend/viewer/src/lib/views/custom/EditCustomViewDialog.sveltefrontend/viewer/src/lib/views/custom/ManageCustomViewsButton.sveltefrontend/viewer/src/lib/views/custom/ManageCustomViewsDialog.sveltefrontend/viewer/src/lib/views/custom/validation.test.tsfrontend/viewer/src/lib/views/custom/validation.tsfrontend/viewer/src/lib/views/entity-config.tsfrontend/viewer/src/lib/views/fields.tsfrontend/viewer/src/lib/views/view-data.tsfrontend/viewer/src/lib/views/view-service.svelte.test.tsfrontend/viewer/src/lib/views/view-service.svelte.tsfrontend/viewer/src/lib/views/view-service.tsfrontend/viewer/src/lib/views/view-text.tsfrontend/viewer/src/locales/en.pofrontend/viewer/src/locales/es.pofrontend/viewer/src/locales/fr.pofrontend/viewer/src/locales/id.pofrontend/viewer/src/locales/ko.pofrontend/viewer/src/locales/ms.pofrontend/viewer/src/locales/sw.pofrontend/viewer/src/locales/vi.pofrontend/viewer/src/project/PrimaryNewEntryButton.sveltefrontend/viewer/src/project/browse/BrowseView.sveltefrontend/viewer/src/project/browse/EditorViewOptions.sveltefrontend/viewer/src/project/browse/EntryListViewOptions.sveltefrontend/viewer/src/project/browse/EntryMenu.sveltefrontend/viewer/src/project/browse/EntryView.sveltefrontend/viewer/src/project/browse/SearchFilter.sveltefrontend/viewer/src/project/browse/ViewPicker.sveltefrontend/viewer/src/project/browse/filter/FieldSelect.sveltefrontend/viewer/src/project/browse/filter/PartOfSpeechSelect.sveltefrontend/viewer/src/project/data/custom-view-service.svelte.tsfrontend/viewer/src/project/data/custom-view-service.test.tsfrontend/viewer/src/project/data/index.tsfrontend/viewer/src/project/data/writing-system-service.svelte.tsfrontend/viewer/src/project/demo/demo-entry-data.tsfrontend/viewer/src/project/demo/in-memory-demo-api.tsfrontend/viewer/src/project/tasks/SubjectPopup.sveltefrontend/viewer/src/project/tasks/tasks-service.tsfrontend/viewer/src/stories/editor/fields/FieldDecorator.svelte
💤 Files with no reviewable changes (2)
- frontend/viewer/src/lib/views/view-service.ts
- frontend/viewer/src/lib/views/fields.ts
backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyDbModel.verified.txt
Show resolved
Hide resolved
frontend/viewer/src/lib/views/custom/ManageCustomViewsDialog.svelte
Outdated
Show resolved
Hide resolved
frontend/viewer/src/lib/views/custom/ManageCustomViewsDialog.svelte
Outdated
Show resolved
Hide resolved
Collaborator
Author
myieye
force-pushed
the
feature/custom-view-model
branch
from
70d7bec to
e5b66cb
Compare
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/viewer/src/locales/vi.po (1)
181-2077:⚠️ Potential issue | 🟡 MinorFill the new Vietnamese translations before shipping custom views.
Line 181 and many other new custom-view entries in this range still have
msgstr "", so the UI will fall back to English for Vietnamese users.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/viewer/src/locales/vi.po` around lines 181 - 2077, The vi.po file contains many new msgid entries left with empty msgstr (e.g., "All", "Analysis", "Based on", "Fields", "Create Custom View", "Create View", "Custom", "Custom view", "Manage custom views", "Edit Custom View", "Save View", "Name", "My custom view...", validation strings like "Name is required", and other msgids shown in the diff); fill each msgstr with the correct Vietnamese translation while preserving placeholders like {0} and '#' and respecting plural/forms, then run your i18n lint/msgfmt checks to ensure the PO file is valid before shipping custom views.
🧹 Nitpick comments (6)
frontend/viewer/src/project/demo/demo-entry-data.ts (1)
87-96: Clarify the font value for the audio writing system.The
fontproperty is set to???(line 92), which appears to be a placeholder. For an audio writing system, consider using a more appropriate value:
- An empty string
''if font is not applicable to audio writing systems- A default font name if the UI still requires a font reference
- A more descriptive placeholder like
'N/A'or'audio-ws'to indicate intentUsing
???may cause confusion or validation issues when this demo data is consumed by tests or UI components.📝 Proposed alternatives
Option 1: Use empty string if not applicable
- 'font': '???', + 'font': '',Option 2: Use descriptive placeholder
- 'font': '???', + 'font': 'N/A',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/viewer/src/project/demo/demo-entry-data.ts` around lines 87 - 96, The demo writing-system entry with id 'seh-Zxxx-x-audio' (wsId 'seh-Zxxx-x-audio', isAudio: true) currently has font set to '???'; replace this placeholder with an appropriate value — either an empty string '' if fonts are not applicable to audio writing systems, a descriptive placeholder like 'N/A' or 'audio-ws', or a default font name required by the UI — and ensure the font property is valid for consumers/validators that read the demo data.frontend/viewer/src/lib/views/custom/validation.ts (1)
11-15: Missing semicolon after object literal.The returned object on lines 11-14 is missing a trailing semicolon, which is inconsistent with the other return statements in this function.
🔧 Suggested fix
if (!hasHeadword) { return { lite: gt`Word or Display as is required`, classic: gt`Lexeme form or Citation form is required` - } + }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/viewer/src/lib/views/custom/validation.ts` around lines 11 - 15, The object literal returned in the validation function (the return that produces { lite: gt`Word or Display as is required`, classic: gt`Lexeme form or Citation form is required` }) is missing a trailing semicolon; update that return statement to end with a semicolon to match the other returns in validation.ts (locate the return inside the validation helper/function in frontend/viewer/src/lib/views/custom/validation.ts that produces the lite/classic object and add the semicolon after the closing brace).frontend/viewer/src/lib/views/view-service.svelte.test.ts (1)
116-119: Minor typo in test description.The test description says "throw" instead of "throws".
✏️ Suggested fix
- it('selectView with unknown ID throw', () => { + it('selectView with unknown ID throws', () => { const {service} = createViewService(); expect(() => service.selectView('nonexistent')).toThrow(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/viewer/src/lib/views/view-service.svelte.test.ts` around lines 116 - 119, Update the test description string for the test that exercises service.selectView with a nonexistent id: change the it(...) label from "selectView with unknown ID throw" to "selectView with unknown ID throws" in the test that calls createViewService() and expects service.selectView('nonexistent') toThrow(); this is the test referencing the selectView method in the view-service.svelte.test.ts file.backend/FwLite/LcmCrdt.Tests/MiniLcmTests/CustomViewTests.cs (1)
48-53: Assert the data stays unchanged after auth failures.These tests stop at
ThrowsAsync, so they would still pass if the API mutated the view before rejecting the caller. Add a postcondition after each failure path to lock in the manager-only contract.🔍 Example follow-up assertions
[Fact] public async Task CreateCustomView_RejectsEditor() { await SetCurrentUser(EditorUserId, UserProjectRole.Editor); await Assert.ThrowsAsync<UnauthorizedAccessException>(() => fixture.Api.CreateCustomView(NewCustomView("Should Fail"))); + (await fixture.Api.GetCustomViews().ToArrayAsync()) + .Should().NotContain(v => v.Name == "Should Fail"); } @@ await SetCurrentUser(EditorUserId, UserProjectRole.Editor); await Assert.ThrowsAsync<UnauthorizedAccessException>(() => fixture.Api.UpdateCustomView(created with { Name = "Should Fail" })); + var reloaded = await fixture.Api.GetCustomView(created.Id); + reloaded.Should().NotBeNull(); + reloaded!.Name.Should().Be(created.Name); @@ await SetCurrentUser(EditorUserId, UserProjectRole.Editor); await Assert.ThrowsAsync<UnauthorizedAccessException>(() => fixture.Api.DeleteCustomView(view.Id)); + (await fixture.Api.GetCustomView(view.Id)).Should().NotBeNull(); }Also applies to: 65-73, 87-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/FwLite/LcmCrdt.Tests/MiniLcmTests/CustomViewTests.cs` around lines 48 - 53, The test CreateCustomView_RejectsEditor currently only asserts an UnauthorizedAccessException; after the Assert.ThrowsAsync add a postcondition that verifies no data was changed (e.g., capture the list/count of custom views before the call and after, or call the appropriate read method on fixture.Api such as GetCustomViews and assert it matches the pre-call snapshot) to ensure CreateCustomView(EditorUserId, NewCustomView(...)) did not mutate state; apply the same pattern to the other failing tests referenced (lines 65-73 and 87-94) so each unauthorized call (fixture.Api.CreateCustomView, etc.) is followed by a read/assert that the view collection is unchanged.backend/FwLite/MiniLcm/Models/CustomView.cs (1)
8-20: Consider usingrecordinstead ofclassfor immutable DTOs.
ViewFieldandViewWritingSystemare simple data containers that could berecordtypes, which aligns with the project's preference for records for DTOs. However, if mutability is required for JSON deserialization or editing workflows, the currentclassapproach is acceptable.♻️ Optional refactor to use records
-public class ViewField -{ - public required string FieldId { get; set; } - // Future possibilities: IsReadOnly, HideIfEmpty, custom writing-system selection etc. - - public ViewField Copy() - { - return new ViewField - { - FieldId = FieldId - }; - } -} +public record ViewField +{ + public required string FieldId { get; init; } + // Future possibilities: IsReadOnly, HideIfEmpty, custom writing-system selection etc. +} -public class ViewWritingSystem -{ - public required WritingSystemId WsId { get; set; } - // Future possibilities: IsReadOnly etc. - - public ViewWritingSystem Copy() - { - return new ViewWritingSystem - { - WsId = WsId - }; - } -} +public record ViewWritingSystem +{ + public required WritingSystemId WsId { get; init; } + // Future possibilities: IsReadOnly etc. +}Note: Records have built-in
withexpressions for copying, eliminating the need for explicitCopy()methods. TheCustomView.Copy()method would then usef with {}instead off.Copy().As per coding guidelines: "Prefer Records for DTOs and immutable data structures in C#"
Also applies to: 22-34
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/FwLite/MiniLcm/Models/CustomView.cs` around lines 8 - 20, The DTO-like types ViewField (and similarly ViewWritingSystem) should be converted to C# record types to follow the project's preference for immutable DTOs and to remove manual copy methods; change the declaration from class ViewField to a record (positional or record class as appropriate), remove the custom Copy() method, and update any call sites such as CustomView.Copy() to use the record 'with' expression (e.g., f with {}) instead of f.Copy(); if mutability is required for JSON deserialization or editing workflows, keep the current classes but document why and leave Copy() as-is.frontend/viewer/src/lib/views/view-service.svelte.ts (1)
62-64: Fire-and-forget async persistence is appropriate here.Using
voidto discard the promise fromset()is acceptable for non-critical persistence operations where the UI doesn't need to wait for completion. Consider whether errors should be logged.♻️ Optional: Add error handling for persistence failures
if (this.#persist) { - void this.#projectStorage.currentView.set(viewId); + this.#projectStorage.currentView.set(viewId).catch((e) => { + console.error('Failed to persist view selection:', e); + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/viewer/src/lib/views/view-service.svelte.ts` around lines 62 - 64, The current fire-and-forget use of void this.#projectStorage.currentView.set(viewId) is acceptable but should optionally capture and log persistence errors; update the call in the class (the block guarded by this.#persist) to still not await the promise but attach a .catch handler that logs the error (use an existing logger on the class if available, e.g. this.#logger.error, or fallback to console.error) so failures are visible while keeping the operation fire-and-forget; leave the this.#persist guard and the call to this.#projectStorage.currentView.set intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/FwLite/LcmCrdt/Changes/CreateCustomViewChange.cs`:
- Around line 35-47: NewEntity currently materializes a CustomView without
validating that referenced custom fields or writing systems still exist; update
CreateCustomViewChange.NewEntity to validate all referenced IDs (EntryFields,
SenseFields, ExampleFields and the writing-system references in Vernacular and
Analysis) using the IChangeContext (e.g., context.EntityExists /
context.TryGetEntity or equivalent) and if any referenced entity is missing,
reject the change by aborting/throwing a clear error (or alternatively filter
out missing IDs before constructing the CustomView), otherwise return the
constructed CustomView as before; ensure you reference EntityId, Name, Base and
the field lists when building the final CustomView.
In `@frontend/viewer/src/lib/components/ui/button/button.svelte`:
- Around line 26-29: The size CSS entries in the sizes mapping (keys: default,
xs, sm, lg) use has-[>:first-child[class*="i-mdi-"]] and
has-[>:last-child[class*="i-mdi-"]], which only match direct child icons but
your icon is wrapped in a span; remove the direct-child combinator (>) so the
has- selectors target descendants instead (update all occurrences in the sizes
mapping), and verify the changes also cover the icon wrapper span used when
rendering icons in the component.
In
`@frontend/viewer/src/lib/entry-editor/object-editors/SenseEditorPrimitive.svelte`:
- Around line 42-46: senseFields currently includes every configured sense field
and is used directly in fieldRecord(...) and objectTemplateAreas(...), which
causes empty grid areas when custom fields are present; filter the derived
senseFields to only the built-in primitive IDs that SenseEditorPrimitive.svelte
supports before passing into fieldRecord and objectTemplateAreas. Specifically,
replace uses of the full senseFields (the variable created at the top and used
in objectTemplateAreas(...) in the Editor.SubGrid props) with a filtered list
(e.g., derive filteredSenseFields from senseFields containing only the built-in
IDs handled by this component) and then call fieldRecord(filteredSenseFields)
and objectTemplateAreas(filteredSenseFields) so custom view fields do not create
empty grid slots.
In `@frontend/viewer/src/lib/views/custom/CustomViewForm.svelte`:
- Around line 27-32: The form currently assigns value = $state(editingValue ??
defaultCustomView()) which reuses the caller's ICustomView and lets form edits
mutate the parent object; change this to create a draft clone when initializing
value (e.g., deep-copy editingValue if present, otherwise defaultCustomView())
so the form works on an isolated draft; ensure fieldSelectionDirty is still
initialized from the presence of editingValue but not by referencing the
original object, and make Cancel simply discard the draft (or reset value to a
fresh clone of editingValue) so unsaved edits never leak back to the caller
while Submit continues to pass the draft to onSubmit.
- Around line 61-75: The submit() function trims value.name before validation
and prevents re-entrant submissions by returning early if saving is true;
specifically, at the top of submit(), guard with "if (saving) return;", then
trim value.name (value.name = value.name.trim()) before calling
validateForm(value) so whitespace-only names are caught, and only set saving =
true after validation passes (or set it immediately after the re-entrancy guard
and keep it true through the try/finally around await onSubmit(value)); keep
using validateForm, onSubmit, saving, and error variables as present.
---
Outside diff comments:
In `@frontend/viewer/src/locales/vi.po`:
- Around line 181-2077: The vi.po file contains many new msgid entries left with
empty msgstr (e.g., "All", "Analysis", "Based on", "Fields", "Create Custom
View", "Create View", "Custom", "Custom view", "Manage custom views", "Edit
Custom View", "Save View", "Name", "My custom view...", validation strings like
"Name is required", and other msgids shown in the diff); fill each msgstr with
the correct Vietnamese translation while preserving placeholders like {0} and
'#' and respecting plural/forms, then run your i18n lint/msgfmt checks to ensure
the PO file is valid before shipping custom views.
---
Nitpick comments:
In `@backend/FwLite/LcmCrdt.Tests/MiniLcmTests/CustomViewTests.cs`:
- Around line 48-53: The test CreateCustomView_RejectsEditor currently only
asserts an UnauthorizedAccessException; after the Assert.ThrowsAsync add a
postcondition that verifies no data was changed (e.g., capture the list/count of
custom views before the call and after, or call the appropriate read method on
fixture.Api such as GetCustomViews and assert it matches the pre-call snapshot)
to ensure CreateCustomView(EditorUserId, NewCustomView(...)) did not mutate
state; apply the same pattern to the other failing tests referenced (lines 65-73
and 87-94) so each unauthorized call (fixture.Api.CreateCustomView, etc.) is
followed by a read/assert that the view collection is unchanged.
In `@backend/FwLite/MiniLcm/Models/CustomView.cs`:
- Around line 8-20: The DTO-like types ViewField (and similarly
ViewWritingSystem) should be converted to C# record types to follow the
project's preference for immutable DTOs and to remove manual copy methods;
change the declaration from class ViewField to a record (positional or record
class as appropriate), remove the custom Copy() method, and update any call
sites such as CustomView.Copy() to use the record 'with' expression (e.g., f
with {}) instead of f.Copy(); if mutability is required for JSON deserialization
or editing workflows, keep the current classes but document why and leave Copy()
as-is.
In `@frontend/viewer/src/lib/views/custom/validation.ts`:
- Around line 11-15: The object literal returned in the validation function (the
return that produces { lite: gt`Word or Display as is required`, classic:
gt`Lexeme form or Citation form is required` }) is missing a trailing semicolon;
update that return statement to end with a semicolon to match the other returns
in validation.ts (locate the return inside the validation helper/function in
frontend/viewer/src/lib/views/custom/validation.ts that produces the
lite/classic object and add the semicolon after the closing brace).
In `@frontend/viewer/src/lib/views/view-service.svelte.test.ts`:
- Around line 116-119: Update the test description string for the test that
exercises service.selectView with a nonexistent id: change the it(...) label
from "selectView with unknown ID throw" to "selectView with unknown ID throws"
in the test that calls createViewService() and expects
service.selectView('nonexistent') toThrow(); this is the test referencing the
selectView method in the view-service.svelte.test.ts file.
In `@frontend/viewer/src/lib/views/view-service.svelte.ts`:
- Around line 62-64: The current fire-and-forget use of void
this.#projectStorage.currentView.set(viewId) is acceptable but should optionally
capture and log persistence errors; update the call in the class (the block
guarded by this.#persist) to still not await the promise but attach a .catch
handler that logs the error (use an existing logger on the class if available,
e.g. this.#logger.error, or fallback to console.error) so failures are visible
while keeping the operation fire-and-forget; leave the this.#persist guard and
the call to this.#projectStorage.currentView.set intact.
In `@frontend/viewer/src/project/demo/demo-entry-data.ts`:
- Around line 87-96: The demo writing-system entry with id 'seh-Zxxx-x-audio'
(wsId 'seh-Zxxx-x-audio', isAudio: true) currently has font set to '???';
replace this placeholder with an appropriate value — either an empty string ''
if fonts are not applicable to audio writing systems, a descriptive placeholder
like 'N/A' or 'audio-ws', or a default font name required by the UI — and ensure
the font property is valid for consumers/validators that read the demo data.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8cb72b39-964a-4a37-b4b0-42d247c5310c
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (108)
backend/FwLite/FwLiteShared/Services/MiniLcmJsInvokable.csbackend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.csbackend/FwLite/LcmCrdt.Tests/Changes/ChangeDeserializationRegressionData.latest.verified.txtbackend/FwLite/LcmCrdt.Tests/Changes/UseChangesTests.csbackend/FwLite/LcmCrdt.Tests/ConfigRegistrationTests.csbackend/FwLite/LcmCrdt.Tests/Data/SnapshotDeserializationRegressionData.latest.verified.txtbackend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyChangeModels.verified.txtbackend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyDbModel.verified.txtbackend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyIObjectWithIdModels.verified.txtbackend/FwLite/LcmCrdt.Tests/MiniLcmTests/CustomViewTests.csbackend/FwLite/LcmCrdt/Changes/CreateCustomViewChange.csbackend/FwLite/LcmCrdt/Changes/EditCustomViewChange.csbackend/FwLite/LcmCrdt/CrdtMiniLcmApi.csbackend/FwLite/LcmCrdt/CrdtProjectsService.csbackend/FwLite/LcmCrdt/Data/MiniLcmRepository.csbackend/FwLite/LcmCrdt/LcmCrdtDbContext.csbackend/FwLite/LcmCrdt/LcmCrdtKernel.csbackend/FwLite/LcmCrdt/Migrations/20260218112222_AddCustomViews.Designer.csbackend/FwLite/LcmCrdt/Migrations/20260218112222_AddCustomViews.csbackend/FwLite/LcmCrdt/Migrations/LcmCrdtDbContextModelSnapshot.csbackend/FwLite/LcmCrdt/Objects/PreDefinedData.csbackend/FwLite/MiniLcm/IMiniLcmReadApi.csbackend/FwLite/MiniLcm/IMiniLcmWriteApi.csbackend/FwLite/MiniLcm/Models/CustomView.csbackend/FwLite/MiniLcm/Models/IObjectWithId.csfrontend/viewer/.storybook/decorators/FWLiteDecorator.sveltefrontend/viewer/package.jsonfrontend/viewer/src/ShadcnProjectView.sveltefrontend/viewer/src/lib/DialogsProvider.sveltefrontend/viewer/src/lib/activity/ActivityItemChangePreview.sveltefrontend/viewer/src/lib/components/editor/field/field-root.sveltefrontend/viewer/src/lib/components/editor/field/field-title.sveltefrontend/viewer/src/lib/components/responsive-dialog/responsive-dialog.sveltefrontend/viewer/src/lib/components/ui/badge/badge.sveltefrontend/viewer/src/lib/components/ui/button/button.sveltefrontend/viewer/src/lib/components/ui/checkbox/checkbox-group.sveltefrontend/viewer/src/lib/components/ui/checkbox/checkbox-item.sveltefrontend/viewer/src/lib/components/ui/checkbox/index.tsfrontend/viewer/src/lib/components/ui/radio-group/radio-group-item.sveltefrontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IMiniLcmFeatures.tsfrontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IMiniLcmJsInvokable.tsfrontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/ICustomView.tsfrontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/IViewField.tsfrontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/IViewWritingSystem.tsfrontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/ViewBase.tsfrontend/viewer/src/lib/dotnet-types/index.tsfrontend/viewer/src/lib/entry-editor/EditEntryDialog.sveltefrontend/viewer/src/lib/entry-editor/EntryOrSenseItemList.sveltefrontend/viewer/src/lib/entry-editor/EntryOrSensePicker.sveltefrontend/viewer/src/lib/entry-editor/NewEntryButton.sveltefrontend/viewer/src/lib/entry-editor/NewEntryDialog.sveltefrontend/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.sveltefrontend/viewer/src/lib/entry-editor/object-editors/AddSenseFab.sveltefrontend/viewer/src/lib/entry-editor/object-editors/EntryEditor.sveltefrontend/viewer/src/lib/entry-editor/object-editors/EntryEditorPrimitive.sveltefrontend/viewer/src/lib/entry-editor/object-editors/ExampleEditorPrimitive.sveltefrontend/viewer/src/lib/entry-editor/object-editors/ObjectHeader.sveltefrontend/viewer/src/lib/entry-editor/object-editors/SenseEditorPrimitive.sveltefrontend/viewer/src/lib/sandbox/EditorSandbox.sveltefrontend/viewer/src/lib/sandbox/Sandbox.sveltefrontend/viewer/src/lib/services/dialogs-service.tsfrontend/viewer/src/lib/storage/project-storage.svelte.tsfrontend/viewer/src/lib/views/OverrideFields.sveltefrontend/viewer/src/lib/views/ViewT.sveltefrontend/viewer/src/lib/views/custom/CreateCustomViewDialog.sveltefrontend/viewer/src/lib/views/custom/CustomViewEntityFields.sveltefrontend/viewer/src/lib/views/custom/CustomViewForm.sveltefrontend/viewer/src/lib/views/custom/CustomViewWritingSystems.sveltefrontend/viewer/src/lib/views/custom/EditCustomViewDialog.sveltefrontend/viewer/src/lib/views/custom/ManageCustomViewsButton.sveltefrontend/viewer/src/lib/views/custom/ManageCustomViewsDialog.sveltefrontend/viewer/src/lib/views/custom/validation.test.tsfrontend/viewer/src/lib/views/custom/validation.tsfrontend/viewer/src/lib/views/entity-config.tsfrontend/viewer/src/lib/views/fields.tsfrontend/viewer/src/lib/views/view-data.tsfrontend/viewer/src/lib/views/view-service.svelte.test.tsfrontend/viewer/src/lib/views/view-service.svelte.tsfrontend/viewer/src/lib/views/view-service.tsfrontend/viewer/src/lib/views/view-text.tsfrontend/viewer/src/locales/en.pofrontend/viewer/src/locales/es.pofrontend/viewer/src/locales/fr.pofrontend/viewer/src/locales/id.pofrontend/viewer/src/locales/ko.pofrontend/viewer/src/locales/ms.pofrontend/viewer/src/locales/sw.pofrontend/viewer/src/locales/vi.pofrontend/viewer/src/project/PrimaryNewEntryButton.sveltefrontend/viewer/src/project/browse/BrowseView.sveltefrontend/viewer/src/project/browse/EditorViewOptions.sveltefrontend/viewer/src/project/browse/EntryListViewOptions.sveltefrontend/viewer/src/project/browse/EntryMenu.sveltefrontend/viewer/src/project/browse/EntryView.sveltefrontend/viewer/src/project/browse/SearchFilter.sveltefrontend/viewer/src/project/browse/ViewPicker.sveltefrontend/viewer/src/project/browse/filter/FieldSelect.sveltefrontend/viewer/src/project/browse/filter/PartOfSpeechSelect.sveltefrontend/viewer/src/project/data/custom-view-service.svelte.tsfrontend/viewer/src/project/data/index.tsfrontend/viewer/src/project/data/writing-system-service.svelte.tsfrontend/viewer/src/project/demo/demo-entry-data.tsfrontend/viewer/src/project/demo/in-memory-demo-api.tsfrontend/viewer/src/project/tasks/SubjectPopup.sveltefrontend/viewer/src/project/tasks/tasks-service.tsfrontend/viewer/src/stories/editor/fields/FieldDecorator.svelte
💤 Files with no reviewable changes (2)
- frontend/viewer/src/lib/views/view-service.ts
- frontend/viewer/src/lib/views/fields.ts
frontend/viewer/src/lib/entry-editor/object-editors/SenseEditorPrimitive.svelte
Show resolved
Hide resolved
myieye
force-pushed
the
feature/custom-view-model
branch
from
e5b66cb to
eff125d
Compare
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
2 participants
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.
Resolves #1049
Resolves #1985