Feature/migrate FontList, FontSelector, and HistoryCard #863
+72
−64
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
*leverage $derived rune for reactive statements *convert export let properties to $props *convert $: to $event
* implement interface in type.ts * correct issues from initial pull request
* Correct $effect in SearchResultList * Migrate search within lexicon
* Update using $state, $derived, and $event runes
The original code does not accomplish desired effect, meaning migration to Svelte 5 is incomplete until fixed and operational.
Contributor
WalkthroughThis change refactors several Svelte components to adopt the new Svelte 5 reactive syntax, including Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FontSelector
participant FontList
User->>FontSelector: Click "Change Font"
FontSelector->>FontList: Open modal, bind selectedFont
User->>FontList: Click font option
FontList->>FontSelector: selectedFont updated via two-way binding
User->>FontSelector: Click "OK"
FontSelector->>FontSelector: Update currentFonts with selectedFont
Estimated code review effort2 (~15 minutes) Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/lib/components/SearchResultList.svelte (1)
40-47: Fix prop mutation - props are read-only in Svelte 5.The line
restore = false;on line 42 attempts to mutate a prop, which is not allowed in Svelte 5. Props destructured from$props()are read-only.Consider one of these solutions:
Option 1: Use a local state variable
- let { collection, results, queryDone, restore, queryId }: Props = $props(); + let { collection, results, queryDone, restore, queryId }: Props = $props(); + let restoreLocal = $state(restore); function onResults() { - if (restore) { - restore = false; + if (restoreLocal) { + restoreLocal = false; const resultsLength = localStorage.getItem('search-result-display-length'); loadMore(parseInt(resultsLength, 10)); } ensureScreenFilled(); }Option 2: Emit an event to parent component
+ import { createEventDispatcher } from 'svelte'; + const dispatch = createEventDispatcher(); function onResults() { if (restore) { - restore = false; + dispatch('restore-complete'); const resultsLength = localStorage.getItem('search-result-display-length'); loadMore(parseInt(resultsLength, 10)); } ensureScreenFilled(); }
🧹 Nitpick comments (3)
src/lib/components/FontSelector.svelte (1)
22-23: Remove debugging console.log before production.The console.log appears to be for debugging purposes and should be removed before merging to production.
- console.log(`${selectedFont}`);src/lib/components/FontList.svelte (1)
15-15: Remove debugging console.log before production.The console.log appears to be for debugging purposes and should be removed before merging to production.
- console.log(`Selected font: ${selectedFont}`);src/lib/components/HistoryCard.svelte (1)
49-51: Consider addressing the accessibility issues.While the ignore comments have been updated, the underlying accessibility issues remain. The clickable div still lacks keyboard event handlers. Consider adding proper keyboard support or using a semantic button element.
Consider this more accessible approach:
-<!-- svelte-ignore a11y_click_events_have_key_events --> -<!-- svelte-ignore a11y_no_static_element_interactions --> -<div style="text-decoration:none;" onclick={onHistoryClick}> +<button style="text-decoration:none;" onclick={onHistoryClick} class="w-full text-left">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/lib/components/FontList.svelte(1 hunks)src/lib/components/FontSelector.svelte(2 hunks)src/lib/components/HistoryCard.svelte(2 hunks)src/lib/components/SearchForm.svelte(1 hunks)src/lib/components/SearchResultCard.svelte(2 hunks)src/lib/components/SearchResultList.svelte(1 hunks)src/lib/types.ts(1 hunks)src/routes/search/[collection]/[[savedResults]]/+page.svelte(3 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
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#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: sillsdev/appbuilder-pwa#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: 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#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: sillsdev/appbuilder-pwa#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: 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: chrisvire
PR: sillsdev/appbuilder-pwa#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: chrisvire
PR: sillsdev/appbuilder-pwa#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: 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 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/lib/components/SearchForm.svelte (3)
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`.
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.
src/lib/components/HistoryCard.svelte (10)
Learnt from: judah-sotomayor
PR: sillsdev/appbuilder-pwa#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: sillsdev/appbuilder-pwa#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: sillsdev/appbuilder-pwa#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: 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#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: sillsdev/appbuilder-pwa#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: chrisvire
PR: sillsdev/appbuilder-pwa#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: 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: chrisvire
PR: sillsdev/appbuilder-pwa#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: 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/search/[collection]/[[savedResults]]/+page.svelte (8)
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`.
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#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: 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#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: 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#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: 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.
src/lib/components/SearchResultCard.svelte (10)
Learnt from: judah-sotomayor
PR: sillsdev/appbuilder-pwa#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: 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#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: sillsdev/appbuilder-pwa#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: sillsdev/appbuilder-pwa#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: sillsdev/appbuilder-pwa#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: sillsdev/appbuilder-pwa#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: 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#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: 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/lib/components/SearchResultList.svelte (10)
Learnt from: chrisvire
PR: sillsdev/appbuilder-pwa#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: sillsdev/appbuilder-pwa#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: sillsdev/appbuilder-pwa#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: sillsdev/appbuilder-pwa#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: sillsdev/appbuilder-pwa#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: 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: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: 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: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: sillsdev/appbuilder-pwa#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.
src/lib/components/FontSelector.svelte (8)
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: judah-sotomayor
PR: sillsdev/appbuilder-pwa#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.
Learnt from: chrisvire
PR: sillsdev/appbuilder-pwa#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: 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: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.
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 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: judah-sotomayor
PR: sillsdev/appbuilder-pwa#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.
src/lib/components/FontList.svelte (10)
Learnt from: judah-sotomayor
PR: sillsdev/appbuilder-pwa#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.
Learnt from: judah-sotomayor
PR: sillsdev/appbuilder-pwa#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: sillsdev/appbuilder-pwa#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: 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: chrisvire
PR: sillsdev/appbuilder-pwa#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: 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#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: sillsdev/appbuilder-pwa#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: sillsdev/appbuilder-pwa#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: 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().
🔇 Additional comments (26)
src/lib/components/FontSelector.svelte (7)
1-1: LGTM - Updated accessibility comment format.The change from hyphens to underscores in Svelte ignore comments aligns with Svelte 5 standards.
12-13: LGTM - Proper Svelte 5 state management.The migration to
$state()for reactive variables is correct for Svelte 5. The initialization ofselectedFontwithcurrentFontmaintains the expected behavior.
16-16: LGTM - Simplified state assignment.Direct assignment to
selectedFontis cleaner than the previous approach accessing throughfontList.
26-26: Clarify the purpose of the commented code.The commented line
//currentFont.set(selectedFont);suggests incomplete implementation. Please clarify if this code should be removed or if there's a specific reason to keep it commented.Is this line needed for the font change functionality mentioned in the PR objectives, or can it be safely removed?
32-32: LGTM - Proper two-way binding implementation.The change to
bind:selectedFontcorrectly implements two-way binding with the FontList component, replacing the previous event-driven approach.
34-34: LGTM - Consistent accessibility comment format.The accessibility ignore comments have been updated to use underscores, maintaining consistency with Svelte 5 standards.
Also applies to: 39-39
43-43: LGTM - Correct Svelte 5 event handling syntax.The change from
on:clicktoonclickis correct for Svelte 5 event handling.src/lib/components/FontList.svelte (5)
5-5: LGTM - Simplified svelte:options.Removing the
accessorsoption simplifies the component configuration, which is appropriate for the new prop binding approach.
11-11: LGTM - Proper Svelte 5 bindable prop pattern.The use of
$bindable()forselectedFontcorrectly implements two-way binding in Svelte 5, replacing the previous event dispatching approach.
21-21: LGTM - Updated accessibility comment format.The accessibility ignore comments have been properly updated to use underscores instead of hyphens, aligning with Svelte 5 standards.
Also applies to: 23-24
26-26: LGTM - Correct Svelte 5 event handling syntax.The change from
on:clicktoonclickis correct for Svelte 5 event handling.
31-31: LGTM - Fixed style property.Good catch fixing the
font-familystyle property to ensure consistent font rendering.src/lib/types.ts (1)
5-9: Good refactoring to centralize the type definition!Moving
SearchFormSubmitEventto a shared types file improves code organization and enables reuse across components.src/lib/components/SearchForm.svelte (1)
5-5: Correct import of centralized type.The type import aligns with the refactoring to centralize
SearchFormSubmitEventin$lib/types.src/lib/components/HistoryCard.svelte (2)
15-19: Correct Svelte 5 props pattern.The migration from
export letto interface with$props()destructuring follows the standard Svelte 5 pattern.
21-30: Proper use of $derived for reactive computations.The conversion from
$:reactive statements to$derivedis the correct Svelte 5 pattern for computed values.src/lib/components/SearchResultCard.svelte (3)
11-17: Correct Svelte 5 props migration.The interface-based props pattern with
$props()destructuring is the standard Svelte 5 approach.
25-27: Appropriate use of $derived.Wrapping the computed direction value with
$derivedis correct for reactive derived state in Svelte 5.
77-77: Verify if preventDefault is needed.The event modifier
|preventDefaultwas removed when migrating fromon:clicktoonclick. Please verify this doesn't cause unintended navigation or form submission.If preventDefault is needed, update the onClick handler:
function onClick() { + event.preventDefault(); navigateToText({ docSet, collection, book: result.reference.bookCode, chapter: result.reference.chapter, verse: result.reference.verses }); }src/routes/search/[collection]/[[savedResults]]/+page.svelte (4)
4-4: Correct type import from centralized location.The type-only import of
SearchFormSubmitEventproperly uses the centralized type definition.Also applies to: 14-14
17-17: Proper Svelte 5 props pattern.The migration to
$props()destructuring is correct.
19-27: Correct use of $state for reactive variables.The state variables are properly wrapped with
$state()and include appropriate initial values and type annotation.
124-124: Correct event handler migration.The change from
on:scrolltoonscrollfollows Svelte 5 conventions.src/lib/components/SearchResultList.svelte (3)
15-22: LGTM! Correct Svelte 5 props migration pattern.The migration from individual
export letstatements to the Props interface with destructuring from$props()follows the correct Svelte 5 pattern and maintains full reactivity.
25-29: LGTM! Proper use of Svelte 5 reactive primitives.The conversion from reactive declarations (
$:) to$derivedand$stateis correct:
$derivedis appropriately used for computed values that depend on other reactive variables$stateis properly used for mutable state that will be updated
31-38: LGTM! Proper use of $effect for reactive side effects.The effect blocks correctly handle reactive side effects:
- First effect clears results when
queryIdchanges- Second effect processes new results when the
resultsarray changes
Comment on lines
+30
to
+33
| // The following block should be inside a function or reactive statement, not at top-level | ||
| // if (scrollDiv) { | ||
| // scrollPosition = scrollDiv.scrollTop; | ||
| // } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove or fix the commented code.
This commented code appears to be an incomplete refactoring attempt. Accessing scrollDiv at the top level would fail since reactive state can't be accessed outside of reactive contexts.
Either remove this commented code or move it into an appropriate reactive context like an event handler or $effect:
-// The following block should be inside a function or reactive statement, not at top-level
-// if (scrollDiv) {
-// scrollPosition = scrollDiv.scrollTop;
-// }
+$effect(() => {
+ if (scrollDiv) {
+ scrollPosition = scrollDiv.scrollTop;
+ }
+});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/routes/search/[collection]/[[savedResults]]/+page.svelte around lines 30
to 33, the commented code attempts to access scrollDiv at the top level, which
is invalid. Remove this commented code or move it inside a reactive statement or
an event handler where scrollDiv is accessible, such as within a $: reactive
block or a function triggered by a scroll event.
Update to svelte 5, run various debugging tests
* Migrate to svelte 5 syntax * Use runes, such as $derived and $props
KarsonDahl
changed the title
KarsonDahl
changed the title
* Actually fix issue, use npx vite build to find error
KarsonDahl
commented
Jul 10, 2025
Member
|
@KarsonDahl Does this PR also fix the text selection that you noticed? |
AslanRules738
added a commit
to AslanRules738/appbuilder-pwa
that referenced
this pull request
Jul 25, 2025
* Draft one of search Load Data from SQL for search Issue sillsdev#863 work in progress for dab search Issue sillsdev#683 Added search button Search UI changes, Hamburger menu, Back navigation Added Search sub-bar Removed hardcoded color for search Issue sillsdev#683 Added special characters, whole words, accents and tones options to DAB search First full implementation of Search Continued work on search Finished select word support for search Freeze header on search scroll Fix search icon misalign Freeze word navigation strip Added support for multi-word searching. Greatly improved efficiency Formatting * Update src/lib/components/LexiconXMLView.svelte Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Add container style and xml backgorund color * Replac hardcoded colors * Add background colors for view * Fix background color sty;es * Fix xml view background color * used autosubscribe * Implement full DAB styles * Background color typo * fixed language tab bug * bug fixes * changed direct subscribe to be auto in search page * extra word navigation; coderabbit changes * style fixes * Fix alphabetstrip background color * Adjust text color for lexicon entries * error checking for search worker * fixed scrolling bug * deleting redundant conditional, console.logs --------- Co-authored-by: Christopher-Shiu <124838176+Christopher-Shiu@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Carson Kramer <carsondkramer@gmail.com> Co-authored-by: aidanpscott <aidanpscott@gmail.com> Co-authored-by: AslanRules <39661498+AslanRules@users.noreply.github.com>
Contributor
Author
|
@chrisvire After reviewing both codebases and hopping around, it seems that the old code does work- the issues I was having have to do with the default whenever there are no fonts from the PWA data files. This is not an error, but a successful failsafe. The way that I was attempting to fix the issue initially triggered this, and it seemed as though this default was not the design. After review, no further action must be taken. |
chrisvire
requested changes
Jul 29, 2025
Member
chrisvire
left a comment
chrisvire
left a comment
There was a problem hiding this comment.
@KarsonDahl I was testing with the SAB Test PWA project. I was in the first collection (cuk). In main, I get this list of fonts:
In this branch, I get this:
Contributor
Author
|
This is an issue I ran into as well. As far as I could tell, this issue stemmed from not having any fonts installed. |
Member
|
I switch back and forth between branches with the same |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The original code does not accomplish desired effect of changing the font, so updating to Svelte 5 is incomplete until the code is operational.
Summary by CodeRabbit
Bug Fixes
Refactor
Style