Update Lexicon to Svelte 5 #884
Conversation
Update the lexicon/+page.svelte and +page.ts files to use svelte 5. Update the Lexicon components to work with the new runes state system. Bare minimum functionality.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis set of changes refactors the lexicon feature by centralizing data access, state management, and formatting into a new Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Lexicon Page / Components
participant Lexicon as lexicon Store
participant DB as SQL.js Database
UI->>Lexicon: lexicon.setup(dictionaryConfig)
Lexicon->>DB: Load SQLite & WASM, initialize DB
Lexicon->>Lexicon: Parse alphabets, writing systems, indexes
UI->>Lexicon: lexicon.fetchReversalWords({ letter, language })
Lexicon->>DB: Query reversal words for letter/language
Lexicon-->>UI: Return word list
UI->>Lexicon: lexicon.data()
Lexicon-->>UI: Return formatted XML/HTML for word(s)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 8
🧹 Nitpick comments (5)
src/lib/components/WordNavigationStrip.svelte (1)
6-31: Consider simplifying the complex word matching logic.The
currentIndexcalculation handles multiple cases with nested conditions. Consider extracting this logic into a helper function for better readability and testability.+ function findWordIndex(word, currentWord) { + if (typeof word !== 'object') return false; + + // Match by unique ID + if (word.id && currentWord.index) { + return word.id === currentWord.index; + } + + // Match homonyms by word and homonym index + if (word.word && currentWord.word && + word.homonym_index !== undefined && + currentWord.homonym_index !== undefined) { + return word.word === currentWord.word && + word.homonym_index === currentWord.homonym_index; + } + + // Match regular words + return word.word === currentWord.word; + } + let currentIndex = $derived( - wordsList.findIndex((word) => { - // Handle both vernacular and reversal word structures - if (typeof word === 'object') { - if (word.id && currentWord.index) { - // For vernacular words, match by ID which is unique - return word.id === currentWord.index; - } else if ( - word.word && - currentWord.word && - word.homonym_index !== undefined && - currentWord.homonym_index !== undefined - ) { - // For vernacular words with homonyms, match both word and homonym index - return ( - word.word === currentWord.word && - word.homonym_index === currentWord.homonym_index - ); - } else { - // For regular vernacular words - return word.word === currentWord.word; - } - } - return false; - }) + wordsList.findIndex((word) => findWordIndex(word, currentWord)) );src/lib/components/LexiconEntryView.svelte (1)
61-63: Simplify style string concatenationThe conditional semicolon appending can be simplified.
- if (cleaned && !cleaned.endsWith(';')) { - cleaned += ';'; - } + cleaned = cleaned ? `${cleaned.trim()};` : '';src/lib/data/lexicon.svelte.ts (3)
358-361: Unusual property renaming approachUsing
Object.definePropertyto rename a property is unnecessarily complex.- // Remap 'name' to 'word' for consistency with reversalWord. - Object.defineProperty(entry, 'word', - Object.getOwnPropertyDescriptor(entry, 'name')); - delete entry['name']; + // Remap 'name' to 'word' for consistency with reversalWord. + entry.word = entry.name; + delete entry.name;
368-370: Address database caching issueThe TODO comment indicates a critical caching issue that needs resolution.
The comment mentions database failures when cache is cleared. This could cause production issues. Would you like me to help implement a proper cache invalidation strategy or open an issue to track this?
389-390: Remove commented codeThe return statement is commented out, which makes the function's purpose unclear.
- } - //return db; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
-
src/lib/components/LexiconEntryView.svelte(4 hunks) -
src/lib/components/LexiconLanguageTabs.svelte(2 hunks) -
src/lib/components/LexiconListViewHeader.svelte(1 hunks) -
src/lib/components/LexiconReversalListView.svelte(0 hunks) -
src/lib/components/LexiconVernacularListView.svelte(0 hunks) -
src/lib/components/TextSelectionToolbar.svelte(0 hunks) -
src/lib/components/WordNavigationStrip.svelte(3 hunks) -
src/lib/data/lexicon.svelte.ts(1 hunks) -
src/routes/lexicon/+page.svelte(6 hunks) -
src/routes/lexicon/+page.ts(1 hunks) -
src/routes/lexicon/search/+page.svelte(2 hunks)
💤 Files with no reviewable changes (3)
- src/lib/components/TextSelectionToolbar.svelte
- src/lib/components/LexiconVernacularListView.svelte
- src/lib/components/LexiconReversalListView.svelte
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: judah-sotomayor
PR: sillsdev/appbuilder-pwa#840
File: src/lib/components/NoteDialog.svelte:94-95
Timestamp: 2025-06-16T18:49:02.188Z
Learning: The appbuilder-pwa project uses Svelte 5, as evidenced by "svelte": "^5" in package.json and the use of Svelte 5 runes syntax like $props() and $derived(). In Svelte 5, onclick is the preferred event handler syntax over on:click.
Learnt from: judah-sotomayor
PR: sillsdev/appbuilder-pwa#820
File: src/lib/components/Modal.svelte:28-34
Timestamp: 2025-06-05T17:25:45.457Z
Learning: In the sillsdev/appbuilder-pwa repository, the Modal.svelte component has an existing bug where `{{ id }.showModal()}` syntax was used instead of calling the exported `showModal` function directly. This bug predates Svelte 5 migration efforts.
Learnt from: chrisvire
PR: sillsdev/appbuilder-pwa#845
File: src/routes/text/+page.svelte:322-324
Timestamp: 2025-07-04T18:50:06.709Z
Learning: In Svelte 5, variables that hold the result of $derived runes should be accessed directly without the $ prefix. The $ prefix is only needed for traditional Svelte stores (writable(), readable(), derived()), not for variables that hold the result of runes like $state() or $derived().
Learnt from: chrisvire
PR: sillsdev/appbuilder-pwa#845
File: src/routes/text/+page.svelte:3-3
Timestamp: 2025-07-04T18:51:27.821Z
Learning: In SvelteKit 2.12 and Svelte 5, `$app/stores` was deprecated in favor of `$app/state`. When migrating from Svelte 4 to Svelte 5, imports should be changed from `import { page } from '$app/stores';` to `import { page } from '$app/state';`. This change aligns with Svelte 5's reactivity model and provides more granular control over state updates.
Learnt from: chrisvire
PR: sillsdev/appbuilder-pwa#845
File: src/routes/text/+page.svelte:3-3
Timestamp: 2025-07-04T18:51:27.821Z
Learning: In Svelte 5, the `page` store is imported from `$app/state` instead of `$app/stores`. This is part of the migration from Svelte 4 to Svelte 5 where `$app/stores` is being deprecated in favor of `$app/state`.
src/routes/lexicon/search/+page.svelte (6)
Learnt from: chrisvire
PR: #845
File: src/routes/text/+page.svelte:3-3
Timestamp: 2025-07-04T18:51:27.821Z
Learning: In Svelte 5, the page store is imported from $app/state instead of $app/stores. This is part of the migration from Svelte 4 to Svelte 5 where $app/stores is being deprecated in favor of $app/state.
Learnt from: chrisvire
PR: #845
File: src/routes/text/+page.svelte:3-3
Timestamp: 2025-07-04T18:51:27.821Z
Learning: In SvelteKit 2.12 and Svelte 5, $app/stores was deprecated in favor of $app/state. When migrating from Svelte 4 to Svelte 5, imports should be changed from import { page } from '$app/stores'; to import { page } from '$app/state';. This change aligns with Svelte 5's reactivity model and provides more granular control over state updates.
Learnt from: judah-sotomayor
PR: #820
File: src/lib/components/Modal.svelte:28-34
Timestamp: 2025-06-05T17:25:45.457Z
Learning: In the sillsdev/appbuilder-pwa repository, the Modal.svelte component has an existing bug where {{ id }.showModal()} syntax was used instead of calling the exported showModal function directly. This bug predates Svelte 5 migration efforts.
Learnt from: judah-sotomayor
PR: #840
File: src/lib/components/NoteDialog.svelte:68-70
Timestamp: 2025-06-16T18:48:21.767Z
Learning: In Svelte 5, event handler syntax changed from Svelte 4. Use onclick={handler} instead of on:click={handler}. Event modifiers like |preventDefault are no longer supported and must be handled inside the handler function. Only one handler per event is allowed per element.
Learnt from: judah-sotomayor
PR: #840
File: src/lib/components/NoteDialog.svelte:68-70
Timestamp: 2025-06-16T18:48:21.767Z
Learning: In Svelte 5, the event handler syntax has changed from Svelte 4. The onclick attribute is valid in Svelte 5, unlike Svelte 4 where on:click was required.
Learnt from: judah-sotomayor
PR: #840
File: src/lib/components/NoteDialog.svelte:94-95
Timestamp: 2025-06-16T18:49:02.188Z
Learning: The appbuilder-pwa project uses Svelte 5, as evidenced by "svelte": "^5" in package.json and the use of Svelte 5 runes syntax like $props() and $derived(). In Svelte 5, onclick is the preferred event handler syntax over on:click.
src/lib/components/LexiconLanguageTabs.svelte (10)
Learnt from: chrisvire
PR: #839
File: src/lib/components/BottomNavigationBar.svelte:12-13
Timestamp: 2025-06-18T17:13:27.707Z
Learning: In Svelte 5, destructuring props using let { propName = defaultValue } = $props(); is the correct and idiomatic way to handle component props. Props destructured from $props() maintain full reactivity and update automatically when parent components change the prop values.
Learnt from: judah-sotomayor
PR: #840
File: src/lib/components/NoteDialog.svelte:68-70
Timestamp: 2025-06-16T18:48:21.767Z
Learning: In Svelte 5, event handler syntax changed from Svelte 4. Use onclick={handler} instead of on:click={handler}. Event modifiers like |preventDefault are no longer supported and must be handled inside the handler function. Only one handler per event is allowed per element.
Learnt from: chrisvire
PR: #839
File: src/lib/components/BottomNavigationBar.svelte:12-13
Timestamp: 2025-06-18T17:13:27.707Z
Learning: The standard Svelte 5 migration pattern converts export let propName = defaultValue; to let { propName = defaultValue } = $props(); while maintaining proper reactivity without requiring additional reactive constructs.
Learnt from: chrisvire
PR: #839
File: src/lib/components/BottomNavigationBar.svelte:12-13
Timestamp: 2025-06-18T17:13:27.707Z
Learning: In Svelte 5, destructuring props using let { propName = defaultValue } = $props(); is the correct and idiomatic way to handle component props. This pattern maintains reactivity and is the recommended approach, not using $derived() for prop access.
Learnt from: chrisvire
PR: #839
File: src/lib/components/BottomNavigationBar.svelte:12-13
Timestamp: 2025-06-18T17:13:27.707Z
Learning: The conversion from export let propName = defaultValue; (Svelte 4) to let { propName = defaultValue } = $props(); (Svelte 5) is the standard migration pattern and maintains proper reactivity without requiring additional reactive constructs.
Learnt from: judah-sotomayor
PR: #840
File: src/lib/components/NoteDialog.svelte:94-95
Timestamp: 2025-06-16T18:49:02.188Z
Learning: The appbuilder-pwa project uses Svelte 5, as evidenced by "svelte": "^5" in package.json and the use of Svelte 5 runes syntax like $props() and $derived(). In Svelte 5, onclick is the preferred event handler syntax over on:click.
Learnt from: judah-sotomayor
PR: #840
File: src/lib/components/NoteDialog.svelte:68-70
Timestamp: 2025-06-16T18:48:21.767Z
Learning: In Svelte 5, the event handler syntax has changed from Svelte 4. The onclick attribute is valid in Svelte 5, unlike Svelte 4 where on:click was required.
Learnt from: chrisvire
PR: #845
File: src/routes/text/+page.svelte:3-3
Timestamp: 2025-07-04T18:51:27.821Z
Learning: In SvelteKit 2.12 and Svelte 5, $app/stores was deprecated in favor of $app/state. When migrating from Svelte 4 to Svelte 5, imports should be changed from import { page } from '$app/stores'; to import { page } from '$app/state';. This change aligns with Svelte 5's reactivity model and provides more granular control over state updates.
Learnt from: judah-sotomayor
PR: #820
File: src/lib/components/Modal.svelte:28-34
Timestamp: 2025-06-05T17:25:45.457Z
Learning: In the sillsdev/appbuilder-pwa repository, the Modal.svelte component has an existing bug where {{ id }.showModal()} syntax was used instead of calling the exported showModal function directly. This bug predates Svelte 5 migration efforts.
Learnt from: judah-sotomayor
PR: #840
File: src/lib/components/TextAppearanceSelector.svelte:165-197
Timestamp: 2025-06-16T18:47:15.122Z
Learning: In src/lib/components/TextAppearanceSelector.svelte, the user judah-sotomayor considers theme selection buttons not relevant for screen-reader users and prefers not to add accessibility labels to theme buttons.
src/lib/components/WordNavigationStrip.svelte (11)
Learnt from: chrisvire
PR: #839
File: src/lib/components/BottomNavigationBar.svelte:12-13
Timestamp: 2025-06-18T17:13:27.707Z
Learning: In Svelte 5, destructuring props using let { propName = defaultValue } = $props(); is the correct and idiomatic way to handle component props. This pattern maintains reactivity and is the recommended approach, not using $derived() for prop access.
Learnt from: chrisvire
PR: #839
File: src/lib/components/BottomNavigationBar.svelte:12-13
Timestamp: 2025-06-18T17:13:27.707Z
Learning: In Svelte 5, destructuring props using let { propName = defaultValue } = $props(); is the correct and idiomatic way to handle component props. Props destructured from $props() maintain full reactivity and update automatically when parent components change the prop values.
Learnt from: chrisvire
PR: #845
File: src/routes/text/+page.svelte:3-3
Timestamp: 2025-07-04T18:51:27.821Z
Learning: In SvelteKit 2.12 and Svelte 5, $app/stores was deprecated in favor of $app/state. When migrating from Svelte 4 to Svelte 5, imports should be changed from import { page } from '$app/stores'; to import { page } from '$app/state';. This change aligns with Svelte 5's reactivity model and provides more granular control over state updates.
Learnt from: chrisvire
PR: #839
File: src/lib/components/BottomNavigationBar.svelte:12-13
Timestamp: 2025-06-18T17:13:27.707Z
Learning: The $derived rune in Svelte 5 is specifically for creating computed reactive states that depend on other reactive variables, not for accessing component props. Using $derived to access props is incorrect and goes against Svelte 5 idioms.
Learnt from: chrisvire
PR: #845
File: src/routes/text/+page.svelte:3-3
Timestamp: 2025-07-04T18:51:27.821Z
Learning: In Svelte 5, the page store is imported from $app/state instead of $app/stores. This is part of the migration from Svelte 4 to Svelte 5 where $app/stores is being deprecated in favor of $app/state.
Learnt from: chrisvire
PR: #845
File: src/routes/text/+page.svelte:322-324
Timestamp: 2025-07-04T18:50:06.709Z
Learning: In Svelte 5, variables that hold the result of $derived runes should be accessed directly without the $ prefix. The $ prefix is only needed for traditional Svelte stores (writable(), readable(), derived()), not for variables that hold the result of runes like $state() or $derived().
Learnt from: chrisvire
PR: #839
File: src/lib/components/BottomNavigationBar.svelte:12-13
Timestamp: 2025-06-18T17:13:27.707Z
Learning: The standard Svelte 5 migration pattern converts export let propName = defaultValue; to let { propName = defaultValue } = $props(); while maintaining proper reactivity without requiring additional reactive constructs.
Learnt from: chrisvire
PR: #839
File: src/lib/components/BottomNavigationBar.svelte:12-13
Timestamp: 2025-06-18T17:13:27.707Z
Learning: The conversion from export let propName = defaultValue; (Svelte 4) to let { propName = defaultValue } = $props(); (Svelte 5) is the standard migration pattern and maintains proper reactivity without requiring additional reactive constructs.
Learnt from: judah-sotomayor
PR: #840
File: src/lib/components/NoteDialog.svelte:94-95
Timestamp: 2025-06-16T18:49:02.188Z
Learning: The appbuilder-pwa project uses Svelte 5, as evidenced by "svelte": "^5" in package.json and the use of Svelte 5 runes syntax like $props() and $derived(). In Svelte 5, onclick is the preferred event handler syntax over on:click.
Learnt from: chrisvire
PR: #845
File: src/routes/text/+page.svelte:201-209
Timestamp: 2025-07-04T18:53:08.369Z
Learning: In Svelte 5, $state creates a reactive rune, not a store. Stores are created with functions like writable(), readable(), and derived(). When using bind:this to bind DOM elements, use plain variables rather than reactive runes to ensure direct DOM property access works correctly.
Learnt from: chrisvire
PR: #845
File: src/routes/text/+page.svelte:380-381
Timestamp: 2025-07-04T18:48:12.143Z
Learning: In Svelte 5, variables declared with $state() are state runes and should be accessed directly without the $ prefix. The $ prefix is only needed for stores (writable(), readable(), derived()), not for state runes. State runes are inherently reactive without requiring the $ prefix.
src/lib/components/LexiconEntryView.svelte (1)
Learnt from: judah-sotomayor
PR: #820
File: src/lib/components/Modal.svelte:28-34
Timestamp: 2025-06-05T17:25:45.457Z
Learning: In the sillsdev/appbuilder-pwa repository, the Modal.svelte component has an existing bug where {{ id }.showModal()} syntax was used instead of calling the exported showModal function directly. This bug predates Svelte 5 migration efforts.
src/lib/components/LexiconListViewHeader.svelte (4)
Learnt from: chrisvire
PR: #839
File: src/lib/components/BottomNavigationBar.svelte:12-13
Timestamp: 2025-06-18T17:13:27.707Z
Learning: In Svelte 5, destructuring props using let { propName = defaultValue } = $props(); is the correct and idiomatic way to handle component props. Props destructured from $props() maintain full reactivity and update automatically when parent components change the prop values.
Learnt from: chrisvire
PR: #839
File: src/lib/components/BottomNavigationBar.svelte:12-13
Timestamp: 2025-06-18T17:13:27.707Z
Learning: The standard Svelte 5 migration pattern converts export let propName = defaultValue; to let { propName = defaultValue } = $props(); while maintaining proper reactivity without requiring additional reactive constructs.
Learnt from: chrisvire
PR: #839
File: src/lib/components/BottomNavigationBar.svelte:12-13
Timestamp: 2025-06-18T17:13:27.707Z
Learning: In Svelte 5, destructuring props using let { propName = defaultValue } = $props(); is the correct and idiomatic way to handle component props. This pattern maintains reactivity and is the recommended approach, not using $derived() for prop access.
Learnt from: chrisvire
PR: #839
File: src/lib/components/BottomNavigationBar.svelte:12-13
Timestamp: 2025-06-18T17:13:27.707Z
Learning: The conversion from export let propName = defaultValue; (Svelte 4) to let { propName = defaultValue } = $props(); (Svelte 5) is the standard migration pattern and maintains proper reactivity without requiring additional reactive constructs.
src/routes/lexicon/+page.svelte (4)
Learnt from: chrisvire
PR: #845
File: src/routes/text/+page.svelte:3-3
Timestamp: 2025-07-04T18:51:27.821Z
Learning: In Svelte 5, the page store is imported from $app/state instead of $app/stores. This is part of the migration from Svelte 4 to Svelte 5 where $app/stores is being deprecated in favor of $app/state.
Learnt from: chrisvire
PR: #845
File: src/routes/text/+page.svelte:3-3
Timestamp: 2025-07-04T18:51:27.821Z
Learning: In SvelteKit 2.12 and Svelte 5, $app/stores was deprecated in favor of $app/state. When migrating from Svelte 4 to Svelte 5, imports should be changed from import { page } from '$app/stores'; to import { page } from '$app/state';. This change aligns with Svelte 5's reactivity model and provides more granular control over state updates.
Learnt from: chrisvire
PR: #845
File: src/routes/text/+page.svelte:322-324
Timestamp: 2025-07-04T18:50:06.709Z
Learning: In Svelte 5, variables that hold the result of $derived runes should be accessed directly without the $ prefix. The $ prefix is only needed for traditional Svelte stores (writable(), readable(), derived()), not for variables that hold the result of runes like $state() or $derived().
Learnt from: chrisvire
PR: #845
File: src/routes/text/+page.svelte:201-209
Timestamp: 2025-07-04T18:53:08.369Z
Learning: In Svelte 5, $state creates a reactive rune, not a store. Stores are created with functions like writable(), readable(), and derived(). When using bind:this to bind DOM elements, use plain variables rather than reactive runes to ensure direct DOM property access works correctly.
🧬 Code Graph Analysis (2)
src/routes/lexicon/+page.ts (2)
config/index.d.ts (1)
DictionaryConfig(316-332)src/lib/data/lexicon.svelte.ts (1)
lexicon(497-497)
src/lib/data/lexicon.svelte.ts (2)
src/lib/data/stores/lexicon.ts (3)
sqlJs(45-45)sqlDb(46-46)initializeDatabase(48-69)config/index.d.ts (2)
DictionaryWritingSystemConfig(129-140)DictionaryConfig(316-332)
🪛 GitHub Actions: Lint
src/routes/lexicon/+page.svelte
[error] 152-152: Build failed: Cannot assign to derived state at line 152:28. See https://svelte.dev/e/constant_assignment
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (10)
src/routes/lexicon/search/+page.svelte (1)
55-58: LGTM! Consistent event handler naming.The renaming from
selectWordtoonSelectWordfollows the established event handler naming convention used throughout the lexicon components. This improves code consistency and readability.Also applies to: 126-126, 132-132
src/lib/components/LexiconLanguageTabs.svelte (2)
5-5: LGTM! Proper Svelte 5 props pattern.The migration from individual
export letstatements to destructuring from$props()follows the standard Svelte 5 migration pattern and maintains full reactivity.
13-14: LGTM! Correct Svelte 5 event handler syntax.The change from
on:clickandon:keydowntoonclickandonkeydownfollows the proper Svelte 5 event handler syntax.Also applies to: 29-30
src/lib/components/LexiconListViewHeader.svelte (1)
5-12: LGTM! Consistent Svelte 5 props migration.The refactor from individual
export letdeclarations to destructuring from$props()correctly follows the Svelte 5 migration pattern, maintaining reactivity for all props.src/routes/lexicon/+page.ts (1)
3-3: LGTM! Clean centralization of lexicon data management.The refactor successfully moves database initialization and data fetching to the centralized
lexiconstore, improving separation of concerns and reducing code duplication.Also applies to: 35-37
src/lib/components/WordNavigationStrip.svelte (3)
2-2: LGTM! Proper Svelte 5 props pattern.The migration from store imports to destructured props from
$props()correctly implements the Svelte 5 pattern.
6-31: LGTM! Correct use of $derived for reactive values.The migration from reactive statements to
$derivedrunes properly implements Svelte 5's reactivity model.Also applies to: 34-37
75-75: LGTM! Correct Svelte 5 event handler syntax.The change from
on:clicktoonclickfollows the proper Svelte 5 event handler syntax.Also applies to: 98-98
src/lib/components/LexiconEntryView.svelte (1)
91-91: Good use of Svelte 5's @Attach directiveThe use of
@attachdirective for DOM manipulation is the correct approach in Svelte 5 for imperative DOM updates.src/routes/lexicon/+page.svelte (1)
151-151: Correct use of Svelte 5 event handler syntaxGood use of
onclickinstead ofon:click, which aligns with Svelte 5 conventions.
| freshSpans.forEach((span) => { | ||
| span.addEventListener('click', () => { | ||
| selectedLanguageStore.set(get(vernacularLanguageStore)); | ||
| lexicon.setLanguage(); |
There was a problem hiding this comment.
Missing parameter in lexicon.setLanguage() call
The lexicon.setLanguage() method is called without any parameters. Based on the context, it should likely receive the language from the clicked element.
Consider passing the appropriate language parameter:
- lexicon.setLanguage();
+ const language = span.getAttribute('data-language');
+ if (language) lexicon.setLanguage(language);If the setLanguage method doesn't require parameters, please verify its implementation in the lexicon store.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/lib/components/LexiconEntryView.svelte at line 19, the call to
lexicon.setLanguage() is missing a required language parameter. Update the call
to pass the language value obtained from the clicked element or relevant
context. If setLanguage does not require parameters, verify its implementation
in the lexicon store to confirm and adjust usage accordingly.
| let sqlJs: typeof initSqlJs = null; | ||
| let sqlDb: Database = null; |
There was a problem hiding this comment.
State variables not using Svelte 5 runes
The sqlJs and sqlDb variables should use $state runes for proper reactivity tracking in Svelte 5.
- let sqlJs: typeof initSqlJs = null;
- let sqlDb: Database = null;
+ let sqlJs: typeof initSqlJs = $state(null);
+ let sqlDb: Database = $state(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.
| let sqlJs: typeof initSqlJs = null; | |
| let sqlDb: Database = null; | |
| let sqlJs: typeof initSqlJs = $state(null); | |
| let sqlDb: Database = $state(null); |
🤖 Prompt for AI Agents
In src/lib/data/lexicon.svelte.ts around lines 7 to 8, the variables sqlJs and
sqlDb are declared as regular variables but should use Svelte 5 $state runes for
reactivity. Change their declarations to use $state to ensure proper reactive
tracking by Svelte 5, replacing "let sqlJs" and "let sqlDb" with "$state sqlJs"
and "$state sqlDb" respectively.
src/lib/data/lexicon.svelte.ts
Outdated
| if (letter in reversals.letters[language]) { | ||
| // Reversal already contains the letter we want. | ||
| return; | ||
| } |
There was a problem hiding this comment.
Incorrect letter existence check
The condition checks if letter exists as a key in the array, but reversals.letters[language] is an array, not an object.
- if (letter in reversals.letters[language]) {
- // Reversal already contains the letter we want.
- return;
- }
+ if (reversals.letters[language].includes(letter)) {
+ // Reversal already contains the letter we want.
+ return;
+ }📝 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.
| if (letter in reversals.letters[language]) { | |
| // Reversal already contains the letter we want. | |
| return; | |
| } | |
| if (reversals.letters[language].includes(letter)) { | |
| // Reversal already contains the letter we want. | |
| return; | |
| } |
🤖 Prompt for AI Agents
In src/lib/data/lexicon.svelte.ts around lines 266 to 269, the code incorrectly
checks if 'letter' exists as a key in 'reversals.letters[language]', which is an
array. Replace the 'letter in reversals.letters[language]' check with a method
appropriate for arrays, such as using 'includes' to check if the array contains
the letter.
src/lib/data/lexicon.svelte.ts
Outdated
| await Promise.all([ | ||
| lettersToLoad.map(letter => fetchReversalLetterData({ letter, language })), | ||
| ]); | ||
|
|
There was a problem hiding this comment.
Incorrect Promise.all usage
The Promise.all is being passed an array of arrays instead of a flat array of promises.
- await Promise.all([
- lettersToLoad.map(letter => fetchReversalLetterData({ letter, language })),
- ]);
+ await Promise.all(
+ lettersToLoad.map(letter => fetchReversalLetterData({ letter, language }))
+ );📝 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 Promise.all([ | |
| lettersToLoad.map(letter => fetchReversalLetterData({ letter, language })), | |
| ]); | |
| await Promise.all( | |
| lettersToLoad.map(letter => fetchReversalLetterData({ letter, language })) | |
| ); |
🤖 Prompt for AI Agents
In src/lib/data/lexicon.svelte.ts around lines 277 to 280, the Promise.all call
is incorrectly passed an array containing the result of map, which itself
returns an array, resulting in a nested array of promises. To fix this, remove
the square brackets around the map call so that Promise.all receives a flat
array of promises directly from lettersToLoad.map.
| return { | ||
| alphabets, | ||
| languages, | ||
| data, | ||
| currentVernacularWords, | ||
| currentReversalWords, | ||
| currentReversalLetters, | ||
| withFetch, | ||
| ...withFetch({ fetch }) | ||
| }; | ||
| } |
There was a problem hiding this comment.
Missing setLanguage method
The lexicon object doesn't include a setLanguage method, but it's called in LexiconEntryView.svelte.
Add the missing method:
return {
alphabets,
languages,
data,
currentVernacularWords,
currentReversalWords,
currentReversalLetters,
+ setLanguage(language?: string) {
+ if (language) {
+ selected.language = language;
+ }
+ },
withFetch,
...withFetch({ fetch })
};📝 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.
| return { | |
| alphabets, | |
| languages, | |
| data, | |
| currentVernacularWords, | |
| currentReversalWords, | |
| currentReversalLetters, | |
| withFetch, | |
| ...withFetch({ fetch }) | |
| }; | |
| } | |
| return { | |
| alphabets, | |
| languages, | |
| data, | |
| currentVernacularWords, | |
| currentReversalWords, | |
| currentReversalLetters, | |
| setLanguage(language?: string) { | |
| if (language) { | |
| selected.language = language; | |
| } | |
| }, | |
| withFetch, | |
| ...withFetch({ fetch }) | |
| }; | |
| } |
🤖 Prompt for AI Agents
In src/lib/data/lexicon.svelte.ts between lines 462 and 472, the returned
lexicon object lacks the setLanguage method, which is called in
LexiconEntryView.svelte. Add a setLanguage method to this object that updates
the current language state appropriately, ensuring it matches the expected usage
in LexiconEntryView.svelte.
| } | ||
| function selectWord(word) { | ||
| let reversalLetters = $derived(lexicon.currentReversalLetters()); |
There was a problem hiding this comment.
Incorrect $derived usage
The reversalLetters is calling lexicon.currentReversalLetters() without passing the required language parameter.
- let reversalLetters = $derived(lexicon.currentReversalLetters());
+ let reversalLetters = $derived(lexicon.currentReversalLetters(selected.language));📝 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.
| let reversalLetters = $derived(lexicon.currentReversalLetters()); | |
| let reversalLetters = $derived(lexicon.currentReversalLetters(selected.language)); |
🤖 Prompt for AI Agents
In src/routes/lexicon/+page.svelte at line 19, the call to
lexicon.currentReversalLetters() is missing the required language parameter.
Update the call to pass the appropriate language argument to
currentReversalLetters() to ensure it functions correctly.
| let reversalLetters = $derived(lexicon.currentReversalLetters()); | ||
| let vernacularWords = $derived(lexicon.currentVernacularWords()); | ||
| let scrollContainer; | ||
| let wordIds = $derived(selectedWord.indexes || null); |
There was a problem hiding this comment.
Cannot assign to derived state - Pipeline failure
The wordIds variable is defined as a $derived value on line 22, which makes it read-only. The assignment on line 152 causes the build failure.
Since wordIds is derived from selectedWord.indexes, you should modify selectedWord instead:
<button
class="dy-btn dy-btn-ghost dy-btn-circle"
onclick={() => {
- wordIds = null;
+ selectedWord = null;
gotoRoute(`/lexicon/search`);
}}
>Also applies to: 151-154
🤖 Prompt for AI Agents
In src/routes/lexicon/+page.svelte around lines 22 and 151-154, the variable
wordIds is assigned from a derived store, making it read-only and causing build
failures when reassigned. To fix this, remove any direct assignments to wordIds
and instead update the underlying selectedWord store or its indexes property
that wordIds derives from. This ensures the derived store updates reactively
without direct mutation.
| return { | ||
| vernacularAlphabet, | ||
| reversalAlphabets, | ||
| reversalLanguages, | ||
| reversalIndexes | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify that removed data is accessible through the lexicon store.
The return value no longer includes reversalAlphabets and reversalIndexes that were previously returned. Please ensure these are now accessible through the centralized lexicon store for any components that depend on them.
🏁 Script executed:
#!/bin/bash
# Description: Check if components are accessing the removed data through the lexicon store
# Search for usage of reversalAlphabets and reversalIndexes
rg -A 3 "reversalAlphabets|reversalIndexes" --type svelte --type ts
# Check lexicon store exports to see if it provides these data points
ast-grep --pattern 'export const $_ = $_' src/lib/data/lexicon.svelte.tsLength of output: 250
🏁 Script executed:
#!/bin/bash
# 1. Locate the file that defines the lexicon store
rg -l "createLexicon" -g "*.ts"
# 2. List all exported members in the lexicon store file
# (replace path below with the file found in step 1 if different)
rg -n "export (const|function|type|interface|let)" src/lib/data/lexicon.ts
# 3. Check whether reversalAlphabets and reversalIndexes are defined or re-exported there
rg -n "reversalAlphabets" src/lib/data/lexicon.ts
rg -n "reversalIndexes" src/lib/data/lexicon.ts
# 4. As a fallback, search the entire data directory for those identifiers
rg -n "reversalAlphabets" src/lib/data
rg -n "reversalIndexes" src/lib/dataLength of output: 806
🏁 Script executed:
#!/bin/bash
# Locate createLexicon definition and usage
echo "Occurrences of createLexicon:"
rg -n "createLexicon" src/lib/data/lexicon.svelte.ts
# Determine the line where createLexicon is defined (const or function)
def_line=$(rg -n "const createLexicon" src/lib/data/lexicon.svelte.ts | head -n1 | cut -d: -f1)
if [ -z "$def_line" ]; then
def_line=$(rg -n "function createLexicon" src/lib/data/lexicon.svelte.ts | head -n1 | cut -d: -f1)
fi
# Print the body of createLexicon (100 lines) to inspect the returned object
if [ -n "$def_line" ]; then
echo "Showing createLexicon body (lines $def_line to $((def_line+100))):"
sed -n "${def_line},$((def_line+100))p" src/lib/data/lexicon.svelte.ts
else
echo "createLexicon definition not found."
fiLength of output: 3631
🏁 Script executed:
#!/bin/bash
# Inspect the return object from createLexicon to verify exposed properties
echo "=== Lines 300-350 ==="
sed -n '300,350p' src/lib/data/lexicon.svelte.ts
echo "=== Lines 350-450 ==="
sed -n '350,450p' src/lib/data/lexicon.svelte.ts
echo "=== Lines 450-497 ==="
sed -n '450,497p' src/lib/data/lexicon.svelte.tsLength of output: 6817
Add missing reversalIndexes accessor to the lexicon store
We verified that createLexicon() (in src/lib/data/lexicon.svelte.ts) returns only these members:
• alphabets()
• languages()
• data()
• currentVernacularWords()
• currentReversalWords()
• currentReversalLetters()
• withFetch / …withFetch
While alphabets() exposes your reversalAlphabets, there is no way to retrieve the underlying reversals.indexes (formerly reversalIndexes). Any component still depending on those indexes will break.
Please expose the indexes in the returned store API. For example, in src/lib/data/lexicon.svelte.ts, update the final return block of createLexicon():
return {
alphabets,
languages,
data,
currentVernacularWords,
currentReversalWords,
currentReversalLetters,
+ /** Expose the full reversal‐indexes map */
+ reversalIndexes(): { [language: string]: ReversalIndex } {
+ return reversals.indexes;
+ },
withFetch,
...withFetch({ fetch })
};And in src/routes/lexicon/+page.ts, consume it via:
const { vernacularAlphabet, reversalLanguages } = await lexicon.setup();
// now also:
const reversalIndexes = lexicon.reversalIndexes();This will restore access to the previously returned data through your centralized store.
🤖 Prompt for AI Agents
In src/routes/lexicon/+page.ts around lines 39 to 42, the returned object from
lexicon.setup() is missing the reversalIndexes accessor, which is needed to
access the underlying reversals.indexes data. To fix this, update the return
statement to also include reversalIndexes by calling lexicon.reversalIndexes(),
ensuring components depending on these indexes continue to function correctly.
|
Akoose project has multiple reversals: Muna Dictionary has English and Indonesian reversals |
Cache previously only added a letter after its async fetch completed. This allowed multiple requests to launch for one letter. Now it adds the letter, and removes it again if the request fails.
|
@judah-sotomayor What is the status of this PR? I would like @FyreByrd to finish it up. |
It's in a bit of a limbo. The main structure should be in place, but there's some bugs in that and not everything is wired up yet. I will check for unpushed code on the Mac and make sure it's all on Github. |
Could you reply back with the bugs that you know about? Did you find any unpushed code? |
|
@judah-sotomayor were you trying to address multiple reversals? |
The Runes-based lexicon system should support multiple reversals, yes. However, I don't believe the current page.svelte supports them. I could be mistaken about that. |
From memory, I believe the reversal loading is not done quite correctly--it may load out-of-order, and clicking on a letter will trigger the load to that letter, but will not jump there unless that letter's words were already loaded. I apologize, I should have documented these more thoroughly sooner. I have not run the project in a few weeks, I will try to get some time to spin it up and look at everything. I have not made time to look for code yet. I will do that this evening, if I have not commented again to confirm or deny by 0900 tomorrow sic @FyreByrd on me. |
|
All code should be up now. |
2 participants
Per #819
This PR introduces two related changes.
Summary by CodeRabbit
Refactor
New Features
Chores