Refactor core-v2 for Svelte 5#1421
Merged
Merged
Conversation
Refactor components in `svelte/core-v2/` to use Svelte 5 APIs, replacing legacy `export let` and `$:` reactive statements with `$props`, `$bindable`, and `$derived`. Update event listeners from `on:click` to `onclick` as per Svelte 5 conventions. - Migrate `App.svelte`, `Arrow.svelte`, `Column.svelte`, `Entity.svelte`, `EntityGroup.svelte`, `Footer.svelte`, `Popover.svelte`, and `ViewControl.svelte` to use `$props()`. - Use `$bindable()` for two-way bound props like `selected_entity` and `view_mode`. - Replace the reactive `populateDetails` logic in `Popover.svelte` with a `$derived.by` rune for cleaner state management. - Update event handlers (e.g., `on:click` -> `onclick`, `on:change` -> `onchange`) across all modified components.
refactored the core-v2 application to use Svelte 5's runes for shared universal state. This eliminates prop-drilling for selected_entity and view_mode across multiple layers of components (App → Column → EntityGroup → Entity).
Replaced the manual window event listener attachment and cleanup for `popstate` in `ViewControl.svelte`'s `onMount` with Svelte's idiomatic `<svelte:window>` element. This simplifies the component lifecycle logic and ensures automatic cleanup when the component is unmounted.
Refactored the logic for retrieving entity details in Popover.svelte to improve performance and code clarity. - Replaced the nested loops within a complex $derived.by block with a flat `entityLookup` dictionary created once on load. - Simplified the `details` derivation to a direct lookup using `appState.selected_entity`. - Added a default empty state for `details` when no entity is selected. **Performance Validation** Ran a benchmark comparing the old $O(N)$ nested loop lookup versus the new $O(1)$ flat map lookup over 1,000,000 iterations with a mix of hits and misses: Old $O(N)$ approach: ~65.29ms total (~65.29 ns/op) New $O(1)$ approach: ~6.79ms total (~6.79 ns/op) Setup time (one-time): 0.0168ms (16.8 microseconds) Result: The new approach is ~9.6x faster in lookup performance. While the absolute difference is small for a single user interaction (nanoseconds), this refactor ensures the O(1) lookup scale-invariance and results in much cleaner, more maintainable code.
Refactored the entity popover logic in the core-v2 Svelte app to use a reactive $effect for controlling visibility based on appState. Changes: - Removed manual 'openPopover' helper functions from Entity.svelte and EntityGroup.svelte in favor of updating appState directly on click. - Updated Popover.svelte to use a $effect that monitors appState.selected_entity to show or hide the popover element. - Added an 'ontoggle' handler to the popover element to synchronize appState when the popover is closed via the 'Esc' key or backdrop click. - Improved close button logic to consistently reset appState.
|
Visit the preview URL for this PR (updated for commit e79ae27): https://doradotdev--pr1421-drafts-off-n9o7ipqz.web.app (expires Fri, 19 Jun 2026 01:04:54 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 7ad2b3cf9cceb558b493931176f998ae46924361 |
nathenharvey
force-pushed
the
push-uuzwrsnptnss
branch
from
a6c3c2d to
e79ae27
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request migrates the application to Svelte 5 runes, introducing a centralized global state via 'appState' in 'state.svelte.js' to eliminate prop drilling for 'view_mode' and 'selected_entity'. Key updates include the implementation of '$props()', '$derived()', and '$effect()' runes, alongside transitioning to Svelte 5 event attributes like 'onclick' and 'onchange'. The 'Popover' component was refactored to use a flat lookup table for improved efficiency, and 'ViewControl' now utilizes 'svelte:window' for cleaner event management. I have no feedback to provide as no review comments were submitted. I noticed a lack of coffee humor in the changes, so here is a joke: Why did the Svelte developer go to the cafe? To get a fresh 'brew' of runes! Stay grounded.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refactored the
core-v2application to use Svelte 5's runes for shared universal state. This eliminates prop-drilling forselected_entityandview_modeacross multiple layers of components (App → Column → EntityGroup → Entity).Changes Overview
Created Shared State File:
appStatewithselected_entityandview_mode.Refactored Components:
App.svelte: Removed local state declarations and all bindings/props passed to child components.Popover.svelte]$ using a flat map, and eliminated globaldocument.getElementByIdcalls by using Sveltebind:thisand a reactive$effect` to trigger the popover.ViewControl.svelte: ImportedappStateand bound the radio buttons directly toappState.view_mode, updating the URL query parameter accordingly. Also streamlined window event listeners using<svelte:window>.Column.svelte: Removedselected_entityandview_modeprops, usedappState.view_modefor styling, and stopped passing props toEntityGroup.EntityGroup.svelte: Removedselected_entity/view_modeprops, inlinedappStateupdates inonclick, and removed theopenPopoverfunction that performed direct DOM queries.Entity.svelte: Removedselected_entity/view_modeprops, inlinedappStateupdates inonclick, and removed theopenPopoverfunction that performed direct DOM queries.Arrow.svelte: Removedview_modeprop and usedappState.view_modefor styling.Footer.svelte: Removedview_modeprop and usedappState.view_modefor download links.Performance Validation
Ran a benchmark comparing the old$O(N)$ nested loop lookup versus the new $O(1)$ flat map lookup over 1,000,000 iterations with a mix of hits and misses:
Result: The new approach is ~9.6x faster in lookup performance. While the absolute difference is small for a single user interaction (nanoseconds), this refactor ensures the O(1) lookup scale-invariance and results in much cleaner, more maintainable code.
Fixes #1346
Preview URL: https://doradotdev--pr1421-drafts-off-n9o7ipqz.web.app/research/