Full virtual scrolling for entry list #2129
Conversation
github-actions
bot
added
💻 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 📝 WalkthroughWalkthroughAdds GetEntryIndex API for resolving entry positions within filtered/sorted lists, refactors sorting/filtering logic across backend layers, introduces virtual-scrolling entry loader with batch loading and cache management, and adds comprehensive test coverage and UI components for progressive entry loading. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In `@frontend/viewer/eslint.config.js`:
- Around line 65-69: The '@typescript-eslint/naming-convention' rule entry has
an invalid modifier token: the 'modifiers' array uses a single string 'static
readonly' which breaks ESLint schema validation; update the 'modifiers' property
in the rule object (the one with 'selector': 'default') to use separate modifier
strings (e.g., ['const'] or ['static','readonly'] as appropriate) and if you
need different semantics add a separate rule object for the
['static','readonly'] case so each modifier is its own array element.
In `@frontend/viewer/src/lib/services/entry-loader-service.svelte.ts`:
- Around line 330-348: In the private async method `#runQuietResetOnce`, remove
the stray debug statement console.log(entries[0]) so no debug output is left in
production; simply delete that line in the function (the block that awaits
Promise.all, checks generation, and calls `#swapCachesForQuietReset` should remain
unchanged).
- Around line 308-318: The RangeError message in the private method
`#markBatchRequested` currently says "Batch number must be positive" but the check
allows zero; update the error text to reflect that zero is valid (e.g., "Batch
number must be non-negative" or "Batch number must not be negative") so the
message matches the condition (keep the same check logic in `#markBatchRequested`
and only change the thrown message).
- Around line 350-353: In `#fetchEntriesForQuietReset`, the call batches.sort()
performs lexicographic sorting causing incorrect offset for multi-digit batch
indices; change the code to determine the numeric minimum batch (e.g., use a
numeric comparator like (a,b)=>a-b on a copy of batches or use
Math.min(...batches)) and compute offset = minBatch * this.batchSize, then call
this.#fetchRange(offset, this.batchSize * batches.length) so offsets are correct
for batches ≥ 10.
- Around line 355-379: In `#swapCachesForQuietReset` the code uses
batches.sort()[0], which sorts lexicographically and mutates the array — replace
that with a numeric, non-mutating selection (e.g., use Math.min(...batches) or
batches.slice().sort((a,b)=>a-b)[0]) to compute offset correctly; also remove
the debug console.log('QUIET RESET COMPLETE') at the end of the method. Ensure
you update the offset calculation and delete the debug log inside the
`#swapCachesForQuietReset` method (consistent with similar fix in
`#fetchEntriesForQuietReset`).
- Around line 146-156: Save the current generation into a local variable at the
start of getOrLoadEntryIndex (e.g., const currentGen = this.#generation) before
calling await this.#api.getEntryIndex(...); after the await, compare currentGen
to this.#generation and if they differ do not cache the returned index or
overwrite this.#idToIndex (simply return the existing cached value from
this.#idToIndex.get(id) or undefined), otherwise continue to set
this.#idToIndex.set(id, index) and return the index; reference
getOrLoadEntryIndex, this.#idToIndex, this.#generation, this.#buildQueryOptions,
this.#deps.search, and this.#api.getEntryIndex when applying the change.
In `@frontend/viewer/src/project/browse/EntriesList.svelte`:
- Around line 104-112: The current $effect launches an async block that awaits
entryLoader?.getOrLoadEntryIndex(selectedEntryId) and then calls untrack(() =>
void scrollToEntry(vList!, selectedEntryId)), which can scroll to a stale
selectedEntryId if it changes while awaiting; fix by capturing the current
selectedEntryId into a local const (e.g., const targetId = selectedEntryId)
before the await, and before calling scrollToEntry verify that selectedEntryId
=== targetId (or that the entry still exists) to avoid acting on a stale id;
update the async block around entryLoader.getOrLoadEntryIndex, scrollToEntry,
and vList references (symbols: $effect, selectedEntryId,
entryLoader.getOrLoadEntryIndex, scrollToEntry, vList, untrack) accordingly.
In `@frontend/viewer/src/project/tasks/TaskView.svelte`:
- Around line 68-71: Wrap the async call to entriesList?.selectNextEntry()
inside a try/catch inside the onNextEntry handler to prevent unhandled
rejections: call selectNextEntry() in try, check the returned next as before,
and in catch log the error and invoke a clear fallback such as calling onDone()
or triggering your app's toast/error handler (use the existing toast/error
utility if available) so the UI doesn't remain stuck; update the onNextEntry
anonymous handler accordingly (referencing entriesList.selectNextEntry and
onDone).
In `@frontend/viewer/tests/entries-list-v2.test.ts`:
- Around line 288-292: The test currently uses an unsafe double-cast ('Headword'
as unknown as SortField) inside page.evaluate; instead, pass the SortField enum
value from the Node test scope into the browser context and use it in the call
to testUtils.demoApi.getEntries (e.g., call page.evaluate with SortField as an
argument), so replace the inline cast with a parameterized value derived from
the imported SortField and reference it inside the page.evaluate callback when
invoking getEntries.
In `@frontend/viewer/tests/entries-list.test.ts`:
- Around line 114-139: The test "scroll to specific entry maintains entry
visibility" currently may assert toContainText('') when selectedText is
null/short; ensure you capture a non-empty selectedText from firstEntry (use
getLocators/firstEntry), assert selectedText is truthy (fail the test early if
not), normalize it (trim) and compute an expected snippet as either the full
selectedText when length < 10 or selectedText.slice(0,10) otherwise, then use
that expected snippet in the expect on firstEntryAfter
(entryRows/firstEntryAfter) so the assertion never trivially passes with an
empty string.
🧹 Nitpick comments (15)
frontend/viewer/src/project/browse/sort/options.ts (1)
1-9: LGTM - Clean extraction of sort configuration.Good refactoring to centralize sort options in a dedicated module. The
as constassertion enables proper type inference forSortConfig, and usingSortFieldfrom generated types maintains type safety with the backend.Consider using a stricter type for
dirto prevent typos and improve autocomplete:type SortDirection = 'asc' | 'desc'; export const sortOptions = [ {field: SortField.SearchRelevance, dir: 'asc' as SortDirection}, {field: SortField.Headword, dir: 'asc' as SortDirection}, {field: SortField.Headword, dir: 'desc' as SortDirection} ] as const;Alternatively, if
SortDirectionexists in the generated types, consider importing it from there.backend/FwLite/LcmCrdt/Data/MiniLcmRepository.cs (1)
263-281: Performance concern: full entity loading during linear scan.The fallback implementation is acknowledged as sub-optimal, but the current approach loads full
Entryobjects with all related data (Senses, ExampleSentences, PartOfSpeech, ComplexForms, Components) just to find an index. For large datasets, this is both CPU and memory intensive.Consider a lighter-weight query that only selects entry IDs in sorted order, then finds the index:
♻️ Suggested optimization
public async Task<int> GetEntryIndex(Guid entryId, string? query = null, QueryOptions? options = null) { - // This is a fallback implementation that's not optimal for large datasets, - // but it works correctly. Ideally, we'd use ROW_NUMBER() window function with linq2db - // for better performance on large entry lists. For now, we enumerate through sorted entries - // and count until we find the target entry. - - var rowIndex = 0; - await foreach (var entry in GetEntries(query, options)) - { - if (entry.Id == entryId) - { - return rowIndex; - } - rowIndex++; - } - - return -1; + // Fallback: enumerate IDs only (avoids loading full entities) + options = await EnsureWritingSystemIsPopulated(options ?? QueryOptions.Default); + var queryable = Entries; + (queryable, var sortingHandled) = await FilterEntries(queryable, query, options, options.Order); + if (!sortingHandled) + queryable = await ApplySorting(queryable, options, query); + + var rowIndex = 0; + await foreach (var id in queryable.Select(e => e.Id).AsAsyncEnumerable()) + { + if (id == entryId) return rowIndex; + rowIndex++; + } + return -1; }frontend/viewer/src/lib/components/Delayed.svelte (1)
76-80: Minor: redundant reset call.
startLoading()callsreset(), but the watch effect already callsreset()beforestartLoading(). This redundancy is harmless but could be removed for clarity.frontend/viewer/src/stories/primitives/delayed.stories.svelte (1)
31-59: Minor: timing measurement captures render time, not load start.The
{@conststart = Date.now()}is evaluated on each render of the snippet, soend - startwill be close to 0ms rather than the actual load duration. For demo purposes this is fine, but if accurate timing is desired, the start time would need to be captured before the load begins.frontend/viewer/src/project/demo/in-memory-demo-api.ts (1)
263-275: Silent failure when entry not found in createSense/createExampleSentence.If the entry doesn't exist,
entry?.senses.push(sense)silently does nothing, but the method still returns the sense as if it was created. This inconsistency could mask bugs during testing. Consider throwing an error likegetEntrydoes:♻️ Suggested fix
createSense(entryGuid: string, sense: ISense): Promise<ISense> { const entry = this._entries.find(e => e.id === entryGuid); - entry?.senses.push(sense); - if (entry) this.#projectEventBus.notifyEntryUpdated(entry); + if (!entry) throw new Error(`Entry ${entryGuid} not found`); + entry.senses.push(sense); + this.#projectEventBus.notifyEntryUpdated(entry); return Promise.resolve(sense); } createExampleSentence(entryGuid: string, senseGuid: string, exampleSentence: IExampleSentence): Promise<IExampleSentence> { const entry = this._entries.find(e => e.id === entryGuid); - entry?.senses.find(s => s.id === senseGuid)?.exampleSentences.push(exampleSentence); - if (entry) this.#projectEventBus.notifyEntryUpdated(entry); + if (!entry) throw new Error(`Entry ${entryGuid} not found`); + const sense = entry.senses.find(s => s.id === senseGuid); + if (!sense) throw new Error(`Sense ${senseGuid} not found`); + sense.exampleSentences.push(exampleSentence); + this.#projectEventBus.notifyEntryUpdated(entry); return Promise.resolve(exampleSentence); }backend/FwLite/MiniLcm.Tests/QueryEntryTestsBase.cs (1)
229-245: Make morph-type tests deterministic across implementations.These tests assume entries without an explicit MorphType are treated as Stem. If any backend returns null/unspecified instead, the OR filter could miss entries. Consider explicitly setting MorphType on test entries (or adding a Root entry) to avoid backend-specific defaults.
AGENTS.md (1)
51-88: Consider toning down repeated exclamation marks for consistency.The emphasis is useful, but the tone could read cleaner with fewer exclamation marks.
frontend/viewer/tests/entries-list.test.ts (3)
3-6: Clarify or remove the "vibe coded" comment.The comment "This file is vibe coded" is unclear and unprofessional for a test file that will be maintained long-term. Consider removing it or replacing it with a meaningful description of the test coverage.
46-66: ReplacewaitForTimeoutwith explicit condition waits to reduce flakiness.Using
page.waitForTimeout(500)is a Playwright anti-pattern that can cause flaky tests. Consider waiting for a specific condition instead.♻️ Suggested improvement
- await page.waitForTimeout(500); - - const scrollTop = await vlist.evaluate((el) => el.scrollTop); - expect(scrollTop).toBeGreaterThan(850); + // Wait for scroll to settle and content to render + await expect(async () => { + const scrollTop = await vlist.evaluate((el) => el.scrollTop); + expect(scrollTop).toBeGreaterThan(850); + }).toPass({timeout: 2000});
142-146: Consider extractingwaitForProjectViewReadyto a shared test utilities module.This helper function is duplicated in
entries-list-v2.test.tswith slight variations. Extracting it to a shared module would improve maintainability and ensure consistent behavior across test files.backend/FwLite/FwLiteWeb/Routes/MiniLcmRoutes.cs (1)
142-159: Consider adding input validation forstartandsizeparameters.Negative values for
startor non-positive values forsizecould cause unexpected behavior or exceptions downstream. Consider validating these parameters and returning appropriate error responses.♻️ Suggested validation
public static Task<EntryWindowResponse> GetEntriesWindow( [FromQuery] int start, [FromQuery] int size, [AsParameters] MiniLcmQueryOptions options, [FromServices] MiniLcmHolder holder) { + if (start < 0) return Task.FromResult(new EntryWindowResponse(start, [])); + if (size <= 0) return Task.FromResult(new EntryWindowResponse(start, [])); var api = holder.MiniLcmApi; return api.GetEntriesWindow(start, size, null, options.ToQueryOptions()); }frontend/viewer/tests/entries-list-v2.test.ts (2)
53-75: Proportional scroll calculation may be imprecise for variable-height items.The
scrollToIndexfunction calculates scroll position as a proportion of total scroll height. This works well for fixed-height items but may be imprecise if VList uses variable or estimated heights. The function signature includes_itemHeightbut doesn't use it.💡 Consider using the itemHeight parameter
- async function scrollToIndex(page: Page, targetIndex: number, _itemHeight: number): Promise<void> { + async function scrollToIndex(page: Page, targetIndex: number, itemHeight: number): Promise<void> { const {vlist} = getLocators(page); - // Get total count for proportional scrolling - const {totalCount} = await page.evaluate(async () => { - const api = window.__PLAYWRIGHT_UTILS__.demoApi; - return {totalCount: await api.countEntries()}; - }); - - // Use proportion of total scroll for accurate positioning - // This accounts for VList's estimated item heights across all items - const targetScroll = await vlist.evaluate((el, params) => { - const {idx, total} = params; - return (idx / total) * el.scrollHeight; - }, {idx: targetIndex, total: totalCount}); + // Use measured item height for more precise positioning + const targetScroll = targetIndex * itemHeight;Alternatively, if proportional scrolling is intentional due to VList's behavior, document this choice with a comment.
109-109: Empty lines can be removed for cleaner code.There are consecutive empty lines at lines 109-110 and 125-126 that could be consolidated.
Also applies to: 125-125
backend/FwLite/MiniLcm.Tests/EntryWindowTestsBase.cs (1)
155-169: Consider adding tests for edge cases with invalid inputs.The test suite covers normal operation well, but consider adding tests for:
- Negative
startvalue- Zero or negative
sizevalue- Very large
sizevalue (beyond MaxPageSize limit)FwDataMiniLcmApi validates these constraints and throws
ArgumentOutOfRangeException. Tests documenting this behavior would clarify expected API behavior for callers.frontend/viewer/src/lib/services/entry-loader-service.svelte.ts (1)
162-176: Minor inconsistency:removeEntryByIdis fire-and-forget whileupdateEntryawaits.Both methods trigger a quiet reset, but
removeEntryByIdusesvoid this.quietReset()(fire-and-forget) whileupdateEntryawaits it. If this is intentional, a brief comment explaining the difference would help maintainability.
myieye
force-pushed
the
feature/virtual-scrolling-entries
branch
from
fd070f2 to
949312c
Compare
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@AGENTS.md`:
- Around line 43-46: The bullet items in AGENTS.md use repeated exclamation
points in the critical-code notes; edit the bullets (`backend/FwLite/AGENTS.md`
and `backend/FwHeadless/AGENTS.md` entries shown in the list) to tone down
punctuation by replacing repeated exclamation marks with a single exclamation or
a period (e.g., "Critical code! Data loss risks." or "Critical code. Data loss
risks."), keeping the warning text but removing extra emphasis for better
readability.
In `@backend/LfClassicData/LfClassicMiniLcmApi.cs`:
- Around line 428-431: The GetEntryIndex method in LfClassicMiniLcmApi currently
throws NotImplementedException but the API route mapping in LfClassicRoutes.cs
doesn't expose /entry/{id:Guid}/index, so either implement the method or
explicitly document/declare it unsupported: if you choose to implement, add the
actual indexing logic inside LfClassicMiniLcmApi.GetEntryIndex and then add the
route in LfClassicRoutes.cs to map the endpoint; if you choose not to support
indexing for LfClassic, replace the NotImplementedException with a
NotSupportedException (or add an XML doc comment on
LfClassicMiniLcmApi.GetEntryIndex stating entry indexing is not supported for
LfClassic projects) so callers and maintainers know this is intentional.
In `@frontend/viewer/AGENTS.md`:
- Around line 18-21: The heading "### Generated .NET Types" contains a sentence
where "API interfaces" is redundant; update the sentence to simplify wording by
removing "API interfaces" and rephrasing to something like: "This project
depends on TypeScript types and APIs generated from .NET (via
`Reinforced.Typings`). If you change .NET models or `JSInvokable` APIs, rebuild
the backend to update these types." Locate the paragraph under the "###
Generated .NET Types" heading in AGENTS.md and replace the current sentence with
the simplified version while preserving the note about rebuilding after changing
.NET models or `JSInvokable` APIs.
In `@frontend/viewer/src/project/browse/EntriesList.svelte`:
- Around line 94-103: The module currently computes skeletonRowCount using
Math.random() at import time (skeletonRowCount) which causes SSR hydration
mismatches; change this to a deterministic initial value (e.g., const
skeletonRowCount = 10) and move any randomness into a client-only onMount block
that updates skeletonRowCount after mount (so the indexArray/$derived reacts to
the updated value), ensuring entryLoader, indexArray and the $derived logic
remain unchanged but rely on the reactive skeletonRowCount variable instead of a
randomized module-level value.
In `@frontend/viewer/src/project/browse/EntryView.svelte`:
- Around line 66-70: The restore() handler currently runs even when the user
lacks write access and doesn't refresh state after re-creating the entry; add a
write-guard at the top of restore() (e.g., check an existing permission flag
like canWrite or writeAccess and return if false) and after awaiting
miniLcmApi.createEntry(entry) trigger a refresh of the view (call the
component’s refetch function such as fetchEntry(entry.id) or fetchEntries(), or
emit a 'refresh' event via dispatch) before setting deleted = false so the UI
reflects the newly recreated entry; keep references to restore(), entry,
miniLcmApi.createEntry, deleted and the chosen refetch/dispatch function when
making the change.
- Around line 120-130: The pt call and the Restore translation use the wrong
syntax for Lingui extraction; update the pt invocation to use tagged templates
for the literal keys (replace pt('entry', 'word', $currentView) with
pt($t`entry`, $t`word`, $currentView`)) and change the Restore button
translation from function-call syntax ($t('Restore')) to the tagged-template
form ($t`Restore`), leaving the restore() onclick and surrounding Alert/Button
components unchanged.
In `@frontend/viewer/src/project/demo/in-memory-demo-api.ts`:
- Around line 105-109: The getEntryIndex implementation ignores requested sort
order by calling getFilteredSortedEntries with the wrong options type; update
getEntryIndex (and the similar implementation at the other location) to
propagate IIndexQueryOptions.order into the filtering/sorting step—either
convert/forward the IIndexQueryOptions to the IFilterQueryOptions expected by
getFilteredSortedEntries or overload/getFilteredSortedEntries to accept
IIndexQueryOptions so the provided order is applied before calling
entries.findIndex; ensure the same fix is applied to the duplicate method around
the other occurrence (the one currently mirroring getEntryIndex logic).
- Around line 88-91: The mockJsEventListener currently resolves immediately
causing tight polling loops; change nextEventAsync on mockJsEventListener to
mimic production long‑poll semantics by returning a never‑resolving promise
(e.g., new Promise(() => {})) instead of Promise.resolve(null!), keep lastEvent
as Promise.resolve(null) (or leave as-is) so consumers won't busy-loop; update
the IJsEventListener mock implementation for nextEventAsync accordingly.
In `@frontend/viewer/tests/entries-list-v2.test.ts`:
- Around line 258-262: The test updates only lexemeForm.seh which can leave the
UI headword (citationForm) unchanged and cause flakiness; inside the
page.evaluate block where you build updated and call
testUtils.demoApi.updateEntry(entry, updated), also set
updated.lexemeForm.citationForm (or updated.lexemeForm.en as appropriate) to the
same newLexemeForm so the citation/headword is mutated along with seh before
calling updateEntry; locate this in the page.evaluate closure where
newLexemeForm, updated, and testUtils.demoApi.updateEntry are used and add the
citationForm assignment to keep headword and seh in sync.
🧹 Nitpick comments (4)
frontend/viewer/src/lib/components/ui/alert/alert.svelte (1)
8-10: Confirm light‑mode border intent for destructive alerts.Line 9 drops
border-destructive/50, so light mode will inherit the default border color while using a destructive background. If you still want a destructive border in light mode, consider adding it back.💡 Optional tweak
- destructive: 'bg-destructive text-destructive-foreground dark:border-destructive-foreground/50 [&>svg]:text-destructive', + destructive: 'bg-destructive text-destructive-foreground border-destructive/50 dark:border-destructive-foreground/50 [&>svg]:text-destructive',backend/FwLite/AGENTS.md (1)
33-43: Good documentation addition for generated types.This section clearly explains the TypeScript type generation workflow and where to find configuration. Minor style nit: "API interfaces" on line 35 is slightly redundant—could simply say "APIs" or "types and interfaces."
backend/FwLite/MiniLcm.Tests/QueryEntryTestsBase.cs (1)
229-245: Consider setting MorphType explicitly in setup to avoid backend-default coupling.
These tests assume the default morph type is alwaysStem; if a backend changes defaults, this could become brittle. Making MorphType explicit in the test data would make the assertions more robust.frontend/viewer/tests/entry-edit-persists.test.ts (1)
17-25: Use shared readiness helper to avoid drift.There’s already a
waitForProjectViewReadyhelper infrontend/viewer/tests/test-utils.tswith optionalwaitForTestUtils. Reusing it keeps readiness checks consistent across Playwright suites.♻️ Suggested refactor
-import {type Page, expect, test} from '@playwright/test'; +import {type Page, expect, test} from '@playwright/test'; +import {waitForProjectViewReady} from './test-utils'; @@ - async function waitForProjectViewReady(page: Page) { - await expect(page.locator('.i-mdi-loading')).toHaveCount(0, {timeout: 10000}); - await page.waitForFunction(() => document.fonts.ready); - await expect(page.locator('[data-skeleton]')).toHaveCount(0, {timeout: 10000}); - } - - async function waitForTestUtils(page: Page) { - await page.waitForFunction(() => window.__PLAYWRIGHT_UTILS__?.demoApi, {timeout: 5000}); - } @@ test.beforeEach(async ({page}) => { await page.goto('/testing/project-view'); - await waitForProjectViewReady(page); - await waitForTestUtils(page); + await waitForProjectViewReady(page, true); });Also applies to: 44-48
| - `backend/AGENTS.md` - General backend guidelines | ||
| - `backend/LexBoxApi/AGENTS.md` - API & GraphQL specific rules | ||
| - `backend/FwLite/AGENTS.md` - **FwLite/CRDT** (Critical code! Data loss risks!) | ||
| - `backend/FwHeadless/AGENTS.md` - **FwHeadless guide** (Critical code! Data loss risks! Mercurial sync, FwData processing) |
There was a problem hiding this comment.
Tone down repeated exclamation points in critical-code bullets.
Minor style nit from static analysis; trimming punctuation improves readability.
✏️ Suggested tweak
-- `backend/FwLite/AGENTS.md` - **FwLite/CRDT** (Critical code! Data loss risks!)
-- `backend/FwHeadless/AGENTS.md` - **FwHeadless guide** (Critical code! Data loss risks! Mercurial sync, FwData processing)
+- `backend/FwLite/AGENTS.md` - **FwLite/CRDT** (Critical code; data loss risks.)
+- `backend/FwHeadless/AGENTS.md` - **FwHeadless guide** (Critical code; data loss risks. Mercurial sync, FwData processing)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - `backend/AGENTS.md` - General backend guidelines | |
| - `backend/LexBoxApi/AGENTS.md` - API & GraphQL specific rules | |
| - `backend/FwLite/AGENTS.md` - **FwLite/CRDT** (Critical code! Data loss risks!) | |
| - `backend/FwHeadless/AGENTS.md` - **FwHeadless guide** (Critical code! Data loss risks! Mercurial sync, FwData processing) | |
| - `backend/AGENTS.md` - General backend guidelines | |
| - `backend/LexBoxApi/AGENTS.md` - API & GraphQL specific rules | |
| - `backend/FwLite/AGENTS.md` - **FwLite/CRDT** (Critical code; data loss risks.) | |
| - `backend/FwHeadless/AGENTS.md` - **FwHeadless guide** (Critical code; data loss risks. Mercurial sync, FwData processing) |
🧰 Tools
🪛 LanguageTool
[style] ~46-~46: Using many exclamation marks might seem excessive (in this case: 5 exclamation marks for a text that’s 3417 characters long)
Context: ... guide** (Critical code! Data loss risks! Mercurial sync, FwData processing) - `f...
(EN_EXCESSIVE_EXCLAMATION)
🤖 Prompt for AI Agents
In `@AGENTS.md` around lines 43 - 46, The bullet items in AGENTS.md use repeated
exclamation points in the critical-code notes; edit the bullets
(`backend/FwLite/AGENTS.md` and `backend/FwHeadless/AGENTS.md` entries shown in
the list) to tone down punctuation by replacing repeated exclamation marks with
a single exclamation or a period (e.g., "Critical code! Data loss risks." or
"Critical code. Data loss risks."), keeping the warning text but removing extra
emphasis for better readability.
| public Task<int> GetEntryIndex(Guid entryId, string? query = null, IndexQueryOptions? options = null) | ||
| { | ||
| throw new NotImplementedException(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find all call sites for GetEntryIndex
rg -n 'GetEntryIndex\s*\(' -A 2 -B 2Repository: sillsdev/languageforge-lexbox
Length of output: 12188
🏁 Script executed:
# Find LfClassicMiniLcmApi usage and instantiation
rg -n 'LfClassicMiniLcmApi' -A 2 -B 2Repository: sillsdev/languageforge-lexbox
Length of output: 1511
🏁 Script executed:
# Look for other MiniLcm implementations to compare
fd -e '.cs' -path '*MiniLcm*' | head -20Repository: sillsdev/languageforge-lexbox
Length of output: 241
🏁 Script executed:
# Find usage of MiniLcmRoutes or GetEntryIndex endpoint calls
rg -n 'GetEntryIndex' backend/FwLite/FwLiteWeb -B 5 -A 5Repository: sillsdev/languageforge-lexbox
Length of output: 2748
🏁 Script executed:
# Search for LfClassicLexboxApiProvider usage
rg -n 'LfClassicLexboxApiProvider' -B 3 -A 3Repository: sillsdev/languageforge-lexbox
Length of output: 5114
🏁 Script executed:
# Check if there's any routing or capability checks for LfClassic
rg -n 'LfClassic' backend/FwLite/FwLiteWeb -B 2 -A 2Repository: sillsdev/languageforge-lexbox
Length of output: 55
🏁 Script executed:
# Look at the full MiniLcmRoutes to understand context
cat -n backend/FwLite/FwLiteWeb/Routes/MiniLcmRoutes.cs | sed -n '135,155p'Repository: sillsdev/languageforge-lexbox
Length of output: 934
🏁 Script executed:
# Check full LfClassicRoutes.cs to see all mapped endpoints
cat -n backend/LfClassicData/LfClassicRoutes.csRepository: sillsdev/languageforge-lexbox
Length of output: 5326
🏁 Script executed:
# Specifically search for any GetEntryIndex in LfClassicRoutes
rg -n 'GetEntryIndex' backend/LfClassicData/LfClassicRoutes.csRepository: sillsdev/languageforge-lexbox
Length of output: 55
🏁 Script executed:
# Check if there are any direct calls to GetEntryIndex on LfClassicMiniLcmApi or IMiniLcmReadApi instances from LfClassic code
rg -n 'GetEntryIndex' backend/LfClassicData -B 3 -A 3Repository: sillsdev/languageforge-lexbox
Length of output: 648
🏁 Script executed:
# Search for any places where IMiniLcmReadApi.GetEntryIndex might be called generically
rg -n '\.GetEntryIndex' -B 2 -A 2 | head -100Repository: sillsdev/languageforge-lexbox
Length of output: 8543
GetEntryIndex stub is not exposed in LfClassic routes — consider removing or documenting the limitation.
The GetEntryIndex endpoint (/entry/{id:Guid}/index) is not mapped in LfClassicRoutes.cs, so the NotImplementedException stub cannot be reached through the LfClassic public API. However, since LfClassicMiniLcmApi implements IMiniLcmReadApi, the method is part of the interface contract. Either implement the method or add a comment documenting that entry indexing is not supported for LfClassic projects.
🤖 Prompt for AI Agents
In `@backend/LfClassicData/LfClassicMiniLcmApi.cs` around lines 428 - 431, The
GetEntryIndex method in LfClassicMiniLcmApi currently throws
NotImplementedException but the API route mapping in LfClassicRoutes.cs doesn't
expose /entry/{id:Guid}/index, so either implement the method or explicitly
document/declare it unsupported: if you choose to implement, add the actual
indexing logic inside LfClassicMiniLcmApi.GetEntryIndex and then add the route
in LfClassicRoutes.cs to map the endpoint; if you choose not to support indexing
for LfClassic, replace the NotImplementedException with a NotSupportedException
(or add an XML doc comment on LfClassicMiniLcmApi.GetEntryIndex stating entry
indexing is not supported for LfClassic projects) so callers and maintainers
know this is intentional.
| ### Generated .NET Types | ||
|
|
||
| This project depends on TypeScript types and API interfaces generated from .NET (via `Reinforced.Typings`). If you change .NET models or `JSInvokable` APIs, you must rebuild the backend to update these types. | ||
|
|
There was a problem hiding this comment.
Minor wording fix: “API interfaces” is redundant.
Consider simplifying the phrasing.
✏️ Suggested edit
-This project depends on TypeScript types and API interfaces generated from .NET (via `Reinforced.Typings`). If you change .NET models or `JSInvokable` APIs, you must rebuild the backend to update these types.
+This project depends on TypeScript types and APIs generated from .NET (via `Reinforced.Typings`). If you change .NET models or `JSInvokable` APIs, you must rebuild the backend to update these types.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### Generated .NET Types | |
| This project depends on TypeScript types and API interfaces generated from .NET (via `Reinforced.Typings`). If you change .NET models or `JSInvokable` APIs, you must rebuild the backend to update these types. | |
| ### Generated .NET Types | |
| This project depends on TypeScript types and APIs generated from .NET (via `Reinforced.Typings`). If you change .NET models or `JSInvokable` APIs, you must rebuild the backend to update these types. |
🧰 Tools
🪛 LanguageTool
[style] ~20-~20: This phrase is redundant (‘I’ stands for ‘interfaces’). Use simply “APIs”.
Context: ...project depends on TypeScript types and API interfaces generated from .NET (via `Reinforced.Ty...
(ACRONYM_TAUTOLOGY)
🤖 Prompt for AI Agents
In `@frontend/viewer/AGENTS.md` around lines 18 - 21, The heading "### Generated
.NET Types" contains a sentence where "API interfaces" is redundant; update the
sentence to simplify wording by removing "API interfaces" and rephrasing to
something like: "This project depends on TypeScript types and APIs generated
from .NET (via `Reinforced.Typings`). If you change .NET models or `JSInvokable`
APIs, rebuild the backend to update these types." Locate the paragraph under the
"### Generated .NET Types" heading in AGENTS.md and replace the current sentence
with the simplified version while preserving the note about rebuilding after
changing .NET models or `JSInvokable` APIs.
| // Generate a random number of skeleton rows between 10 and 13 | ||
| const skeletonRowCount = Math.ceil(Math.random() * 10) + 3; | ||
| // Generate index array for virtual list. | ||
| // We use a small number of skeletons if the total count is not yet known | ||
| // to avoid a "white phase" between initial load and list initialization. | ||
| const indexArray = $derived( | ||
| entryLoader?.totalCount !== undefined | ||
| ? Array.from({ length: entryLoader.totalCount }, (_, i) => i) | ||
| : Array.from({ length: skeletonRowCount }, (_, i) => i) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the full file to understand context
head -n 110 frontend/viewer/src/project/browse/EntriesList.svelte | tail -n 30Repository: sillsdev/languageforge-lexbox
Length of output: 1059
🏁 Script executed:
# Check if there's a svelte.config.js to see if SSR is enabled
cat svelte.config.js 2>/dev/null || echo "File not found"Repository: sillsdev/languageforge-lexbox
Length of output: 87
🏁 Script executed:
# Check the full imports and structure of the file
head -n 20 frontend/viewer/src/project/browse/EntriesList.svelteRepository: sillsdev/languageforge-lexbox
Length of output: 1208
🏁 Script executed:
# Check if onMount is already imported or used in this file
rg "onMount" frontend/viewer/src/project/browse/EntriesList.svelteRepository: sillsdev/languageforge-lexbox
Length of output: 55
🏁 Script executed:
# Check if untrack is imported
rg "import.*untrack" frontend/viewer/src/project/browse/EntriesList.svelteRepository: sillsdev/languageforge-lexbox
Length of output: 106
🏁 Script executed:
# Look for SvelteKit config in alternative locations
find . -maxdepth 2 -name "svelte.config.*" -o -name "vite.config.*" | head -20Repository: sillsdev/languageforge-lexbox
Length of output: 126
🏁 Script executed:
# Check package.json for SvelteKit version and SSR settings
cat package.json | grep -A 10 '"dependencies"' | head -20Repository: sillsdev/languageforge-lexbox
Length of output: 117
🏁 Script executed:
# Check if there's a SvelteKit config in the root
ls -la | grep -E "svelte|vite"Repository: sillsdev/languageforge-lexbox
Length of output: 55
🏁 Script executed:
# Look for any SvelteKit project files
find . -maxdepth 2 -type f -name "*.config.*" | head -20Repository: sillsdev/languageforge-lexbox
Length of output: 439
🏁 Script executed:
# Check the SvelteKit config for SSR settings
cat frontend/svelte.config.jsRepository: sillsdev/languageforge-lexbox
Length of output: 953
🏁 Script executed:
# Check frontend package.json for Svelte version
cat frontend/package.json | grep -E '"svelte|"@sveltejs' | head -10Repository: sillsdev/languageforge-lexbox
Length of output: 518
🏁 Script executed:
# Check how other components in viewer handle similar patterns with onMount or $effect
rg "onMount|\\$effect" frontend/viewer/src --type svelte | head -20Repository: sillsdev/languageforge-lexbox
Length of output: 189
🏁 Script executed:
# Check if there are other uses of Math.random in the codebase to see patterns
rg "Math\.random" frontend/viewer --type svelteRepository: sillsdev/languageforge-lexbox
Length of output: 103
🏁 Script executed:
# Verify Svelte 5 patterns - check if $state is used elsewhere
rg "\\\$state\(" frontend/viewer/src --type svelte | head -10Repository: sillsdev/languageforge-lexbox
Length of output: 103
🏁 Script executed:
# Search for onMount and $effect patterns using simpler approach
rg "onMount" frontend/viewer/src | head -15Repository: sillsdev/languageforge-lexbox
Length of output: 1373
🏁 Script executed:
# Search for Math.random usage in the project
rg "Math\.random" frontend/viewerRepository: sillsdev/languageforge-lexbox
Length of output: 1724
🏁 Script executed:
# Search for $state usage patterns
rg '\$state\(' frontend/viewer/src | head -10Repository: sillsdev/languageforge-lexbox
Length of output: 1162
Avoid randomized skeleton count during SSR hydration.
Math.random() at module initialization causes server/client mismatch during SSR hydration. Use a deterministic initial value and randomize post-mount if variation is needed.
Suggested fix
-import {untrack} from 'svelte';
+import {onMount, untrack} from 'svelte';
@@
- // Generate a random number of skeleton rows between 10 and 13
- const skeletonRowCount = Math.ceil(Math.random() * 10) + 3;
+ // Deterministic initial value to avoid SSR mismatch; randomize after mount if desired
+ let skeletonRowCount = $state(12);
+ onMount(() => {
+ skeletonRowCount = Math.ceil(Math.random() * 10) + 3;
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Generate a random number of skeleton rows between 10 and 13 | |
| const skeletonRowCount = Math.ceil(Math.random() * 10) + 3; | |
| // Generate index array for virtual list. | |
| // We use a small number of skeletons if the total count is not yet known | |
| // to avoid a "white phase" between initial load and list initialization. | |
| const indexArray = $derived( | |
| entryLoader?.totalCount !== undefined | |
| ? Array.from({ length: entryLoader.totalCount }, (_, i) => i) | |
| : Array.from({ length: skeletonRowCount }, (_, i) => i) | |
| // Deterministic initial value to avoid SSR mismatch; randomize after mount if desired | |
| let skeletonRowCount = $state(12); | |
| onMount(() => { | |
| skeletonRowCount = Math.ceil(Math.random() * 10) + 3; | |
| }); | |
| // Generate index array for virtual list. | |
| // We use a small number of skeletons if the total count is not yet known | |
| // to avoid a "white phase" between initial load and list initialization. | |
| const indexArray = $derived( | |
| entryLoader?.totalCount !== undefined | |
| ? Array.from({ length: entryLoader.totalCount }, (_, i) => i) | |
| : Array.from({ length: skeletonRowCount }, (_, i) => i) |
🤖 Prompt for AI Agents
In `@frontend/viewer/src/project/browse/EntriesList.svelte` around lines 94 - 103,
The module currently computes skeletonRowCount using Math.random() at import
time (skeletonRowCount) which causes SSR hydration mismatches; change this to a
deterministic initial value (e.g., const skeletonRowCount = 10) and move any
randomness into a client-only onMount block that updates skeletonRowCount after
mount (so the indexArray/$derived reacts to the updated value), ensuring
entryLoader, indexArray and the $derived logic remain unchanged but rely on the
reactive skeletonRowCount variable instead of a randomized module-level value.
| async function restore() { | ||
| if (!entry) return; | ||
| await miniLcmApi.createEntry(entry); | ||
| deleted = false; | ||
| } |
There was a problem hiding this comment.
Guard restore for read‑only users and refresh state after re‑create
restore() can be invoked even when write access is off, and the view may stay stale after re‑creating the entry. Consider a write guard and a refetch once restored.
🛠️ Suggested fix
async function restore() {
- if (!entry) return;
- await miniLcmApi.createEntry(entry);
- deleted = false;
+ if (!entry || !features.write) return;
+ await miniLcmApi.createEntry(entry);
+ deleted = false;
+ await entryResource.refetch();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function restore() { | |
| if (!entry) return; | |
| await miniLcmApi.createEntry(entry); | |
| deleted = false; | |
| } | |
| async function restore() { | |
| if (!entry || !features.write) return; | |
| await miniLcmApi.createEntry(entry); | |
| deleted = false; | |
| await entryResource.refetch(); | |
| } |
🤖 Prompt for AI Agents
In `@frontend/viewer/src/project/browse/EntryView.svelte` around lines 66 - 70,
The restore() handler currently runs even when the user lacks write access and
doesn't refresh state after re-creating the entry; add a write-guard at the top
of restore() (e.g., check an existing permission flag like canWrite or
writeAccess and return if false) and after awaiting
miniLcmApi.createEntry(entry) trigger a refresh of the view (call the
component’s refetch function such as fetchEntry(entry.id) or fetchEntries(), or
emit a 'refresh' event via dispatch) before setting deleted = false so the UI
reflects the newly recreated entry; keep references to restore(), entry,
miniLcmApi.createEntry, deleted and the chosen refetch/dispatch function when
making the change.
| {@const entity = pt('entry', 'word', $currentView)} | ||
| <div class="mb-2 px-2"> | ||
| <Alert.Root variant="destructive"> | ||
| <Alert.Description class="flex justify-between items-center"> | ||
| <span class="inline-flex gap-2"> | ||
| <Icon icon="i-mdi-alert-circle" class="size-5" /> | ||
| {$t`This ${entity} was deleted`} | ||
| </span> | ||
| <Button size="sm" variant="secondary" onclick={() => restore()}> | ||
| {$t('Restore')} | ||
| </Button> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find frontend -name "*.svelte" -o -name "*.ts" | head -20Repository: sillsdev/languageforge-lexbox
Length of output: 929
🏁 Script executed:
cat -n frontend/viewer/src/project/browse/EntryView.svelte | sed -n '115,135p'Repository: sillsdev/languageforge-lexbox
Length of output: 1018
🏁 Script executed:
rg "pt\(" frontend -A 2 -B 2 --max-count 10Repository: sillsdev/languageforge-lexbox
Length of output: 25628
🏁 Script executed:
rg "function pt\(|const pt =|export.*pt" frontend -A 3Repository: sillsdev/languageforge-lexbox
Length of output: 12505
🏁 Script executed:
rg "\$t\`" frontend/viewer --max-count 5 -B 1Repository: sillsdev/languageforge-lexbox
Length of output: 55
🏁 Script executed:
rg "\$t\(" frontend/viewer --max-count 5 -B 1Repository: sillsdev/languageforge-lexbox
Length of output: 55
🏁 Script executed:
rg "export.*function pt|export const pt" frontendRepository: sillsdev/languageforge-lexbox
Length of output: 55
🏁 Script executed:
rg "const pt.*=" frontend/viewer -A 3Repository: sillsdev/languageforge-lexbox
Length of output: 55
🏁 Script executed:
cat frontend/viewer/I18N_CONTEXT_GUIDE.mdRepository: sillsdev/languageforge-lexbox
Length of output: 9279
🏁 Script executed:
rg "pt\(\$t\`" frontend/viewer --max-count 15Repository: sillsdev/languageforge-lexbox
Length of output: 55
Use tagged template literals for i18n extraction
Line 120: pt('entry', 'word', $currentView) uses raw strings that won't be extracted for translation. Wrap both strings with $t tagged templates as documented in the project's i18n pattern: pt($tentry, $tword, $currentView).
Line 129: $t('Restore') uses function call syntax instead of the tagged template $tRestore`` required for Lingui string extraction.
🔤 Suggested fix
- {`@const` entity = pt('entry', 'word', $currentView)}
+ {`@const` entity = pt($t`entry`, $t`word`, $currentView)}
...
- <Button size="sm" variant="secondary" onclick={() => restore()}>
- {$t('Restore')}
- </Button>
+ <Button size="sm" variant="secondary" onclick={() => restore()}>
+ {$t`Restore`}
+ </Button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {@const entity = pt('entry', 'word', $currentView)} | |
| <div class="mb-2 px-2"> | |
| <Alert.Root variant="destructive"> | |
| <Alert.Description class="flex justify-between items-center"> | |
| <span class="inline-flex gap-2"> | |
| <Icon icon="i-mdi-alert-circle" class="size-5" /> | |
| {$t`This ${entity} was deleted`} | |
| </span> | |
| <Button size="sm" variant="secondary" onclick={() => restore()}> | |
| {$t('Restore')} | |
| </Button> | |
| {`@const` entity = pt($t`entry`, $t`word`, $currentView)} | |
| <div class="mb-2 px-2"> | |
| <Alert.Root variant="destructive"> | |
| <Alert.Description class="flex justify-between items-center"> | |
| <span class="inline-flex gap-2"> | |
| <Icon icon="i-mdi-alert-circle" class="size-5" /> | |
| {$t`This ${entity} was deleted`} | |
| </span> | |
| <Button size="sm" variant="secondary" onclick={() => restore()}> | |
| {$t`Restore`} | |
| </Button> |
🤖 Prompt for AI Agents
In `@frontend/viewer/src/project/browse/EntryView.svelte` around lines 120 - 130,
The pt call and the Restore translation use the wrong syntax for Lingui
extraction; update the pt invocation to use tagged templates for the literal
keys (replace pt('entry', 'word', $currentView) with pt($t`entry`, $t`word`,
$currentView`)) and change the Restore button translation from function-call
syntax ($t('Restore')) to the tagged-template form ($t`Restore`), leaving the
restore() onclick and surrounding Alert/Button components unchanged.
| const mockJsEventListener: IJsEventListener = { | ||
| nextEventAsync: () => Promise.resolve(null!), | ||
| lastEvent: () => Promise.resolve(null) | ||
| }; |
There was a problem hiding this comment.
Mock JsEventListener should mimic long‑poll semantics.
Resolving immediately can cause tight polling loops in consumers. Consider matching the production “never-resolving” behavior for stability in the demo.
🛠️ Suggested change
+type NextEvent = Awaited<ReturnType<IJsEventListener['nextEventAsync']>>;
const mockJsEventListener: IJsEventListener = {
- nextEventAsync: () => Promise.resolve(null!),
+ nextEventAsync: () => new Promise<NextEvent>(() => {}),
lastEvent: () => Promise.resolve(null)
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const mockJsEventListener: IJsEventListener = { | |
| nextEventAsync: () => Promise.resolve(null!), | |
| lastEvent: () => Promise.resolve(null) | |
| }; | |
| type NextEvent = Awaited<ReturnType<IJsEventListener['nextEventAsync']>>; | |
| const mockJsEventListener: IJsEventListener = { | |
| nextEventAsync: () => new Promise<NextEvent>(() => {}), | |
| lastEvent: () => Promise.resolve(null) | |
| }; |
🤖 Prompt for AI Agents
In `@frontend/viewer/src/project/demo/in-memory-demo-api.ts` around lines 88 - 91,
The mockJsEventListener currently resolves immediately causing tight polling
loops; change nextEventAsync on mockJsEventListener to mimic production
long‑poll semantics by returning a never‑resolving promise (e.g., new Promise(()
=> {})) instead of Promise.resolve(null!), keep lastEvent as
Promise.resolve(null) (or leave as-is) so consumers won't busy-loop; update the
IJsEventListener mock implementation for nextEventAsync accordingly.
| async getEntryIndex(entryId: string, query?: string, options?: IIndexQueryOptions): Promise<number> { | ||
| await delay(100); | ||
| const entries = this.getFilteredSortedEntries(query, options); | ||
| return entries.findIndex(e => e.id === entryId); | ||
| } |
There was a problem hiding this comment.
getEntryIndex ignores requested sort order.
IIndexQueryOptions includes order, but the helper takes IFilterQueryOptions and always uses default sorting. This can return the wrong index when the UI uses non-default sort settings.
🐛 Suggested fix
- async getEntryIndex(entryId: string, query?: string, options?: IIndexQueryOptions): Promise<number> {
+ async getEntryIndex(entryId: string, query?: string, options?: IIndexQueryOptions): Promise<number> {
await delay(100);
- const entries = this.getFilteredSortedEntries(query, options);
+ const entries = this.getFilteredSortedEntries(query, options);
return entries.findIndex(e => e.id === entryId);
}
@@
- private getFilteredSortedEntries(query?: string, options?: IFilterQueryOptions): IEntry[] {
+ private getFilteredSortedEntries(query?: string, options?: IIndexQueryOptions): IEntry[] {
const entries = this.getFilteredEntries(query, options);
const defaultWs = writingSystems.vernacular[0].wsId;
- // For getEntryIndex, we just need filtering, but we'll also sort for consistency
- // Note: IFilterQueryOptions doesn't have order, so we use default sort
+ const sortWs = pickWs(options?.order?.writingSystem ?? 'default', defaultWs);
+ const ascending = options?.order?.ascending ?? true;
return entries.sort((e1, e2) => {
- const v1 = this.#writingSystemService.headword(e1, defaultWs);
- const v2 = this.#writingSystemService.headword(e2, defaultWs);
+ const v1 = this.#writingSystemService.headword(e1, sortWs);
+ const v2 = this.#writingSystemService.headword(e2, sortWs);
if (!v2) return -1;
if (!v1) return 1;
- const compare = v1.localeCompare(v2, defaultWs);
- return compare === 0 ? e1.id.localeCompare(e2.id) : compare;
+ const compare = v1.localeCompare(v2, sortWs);
+ const result = compare === 0 ? e1.id.localeCompare(e2.id) : compare;
+ return ascending ? result : -result;
});
}Also applies to: 232-245
🤖 Prompt for AI Agents
In `@frontend/viewer/src/project/demo/in-memory-demo-api.ts` around lines 105 -
109, The getEntryIndex implementation ignores requested sort order by calling
getFilteredSortedEntries with the wrong options type; update getEntryIndex (and
the similar implementation at the other location) to propagate
IIndexQueryOptions.order into the filtering/sorting step—either convert/forward
the IIndexQueryOptions to the IFilterQueryOptions expected by
getFilteredSortedEntries or overload/getFilteredSortedEntries to accept
IIndexQueryOptions so the provided order is applied before calling
entries.findIndex; ensure the same fix is applied to the duplicate method around
the other occurrence (the one currently mirroring getEntryIndex logic).
| await page.evaluate(async (entry) => { | ||
| const testUtils = window.__PLAYWRIGHT_UTILS__; | ||
| const newLexemeForm = `-UPDATED-${entry.lexemeForm.seh || entry.lexemeForm.en || 'entry'}`; | ||
| const updated = {...entry, lexemeForm: {...entry.lexemeForm, seh: newLexemeForm}}; | ||
| await testUtils.demoApi.updateEntry(entry, updated); |
There was a problem hiding this comment.
Update test should also mutate citationForm to avoid headword mismatch.
If entries already have a citation form, the UI headword can stay unchanged, making this test flaky.
🐛 Suggested fix
- const updated = {...entry, lexemeForm: {...entry.lexemeForm, seh: newLexemeForm}};
+ const updated = {
+ ...entry,
+ lexemeForm: {...entry.lexemeForm, seh: newLexemeForm},
+ citationForm: {...(entry.citationForm ?? {}), seh: newLexemeForm},
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await page.evaluate(async (entry) => { | |
| const testUtils = window.__PLAYWRIGHT_UTILS__; | |
| const newLexemeForm = `-UPDATED-${entry.lexemeForm.seh || entry.lexemeForm.en || 'entry'}`; | |
| const updated = {...entry, lexemeForm: {...entry.lexemeForm, seh: newLexemeForm}}; | |
| await testUtils.demoApi.updateEntry(entry, updated); | |
| await page.evaluate(async (entry) => { | |
| const testUtils = window.__PLAYWRIGHT_UTILS__; | |
| const newLexemeForm = `-UPDATED-${entry.lexemeForm.seh || entry.lexemeForm.en || 'entry'}`; | |
| const updated = { | |
| ...entry, | |
| lexemeForm: {...entry.lexemeForm, seh: newLexemeForm}, | |
| citationForm: {...(entry.citationForm ?? {}), seh: newLexemeForm}, | |
| }; | |
| await testUtils.demoApi.updateEntry(entry, updated); |
🤖 Prompt for AI Agents
In `@frontend/viewer/tests/entries-list-v2.test.ts` around lines 258 - 262, The
test updates only lexemeForm.seh which can leave the UI headword (citationForm)
unchanged and cause flakiness; inside the page.evaluate block where you build
updated and call testUtils.demoApi.updateEntry(entry, updated), also set
updated.lexemeForm.citationForm (or updated.lexemeForm.en as appropriate) to the
same newLexemeForm so the citation/headword is mutated along with seh before
calling updateEntry; locate this in the page.evaluate closure where
newLexemeForm, updated, and testUtils.demoApi.updateEntry are used and add the
citationForm assignment to keep headword and seh in sync.
No description provided.