Bump svelte to 5.55+ and add detached-resource regression harness#2323
Merged
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR upgrades Svelte to 5.55.7 and implements a wrapper service pattern with ProjectContext lifecycle ownership. ProjectContext now owns cached service roots via ChangesWrapper service pattern and ProjectContext lifecycle
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
github-actions
Bot
added
the
💻 FW Lite
Contributor
UI unit Tests 1 files 61 suites 29s ⏱️ Results for commit aaeb921. ♻️ This comment has been updated with latest results. |
Contributor
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
myieye
added a commit
that referenced
this pull request
Jun 1, 2026
Frontend (pnpm) — direct: @sveltejs/kit 2.49→2.60.1, @opentelemetry/sdk-node
0.208→0.217, @opentelemetry/auto-instrumentations-node 0.67→0.75, js-cookie
3.0.5→3.0.7. Catalog: vite→7.3.2, postcss→8.5.10. In viewer: lint-staged 13→15
(clears micromatch + yaml advisories naturally). Minimal pnpm.overrides for the
three transitives that don't have a clean parent-bump path: cookie (SvelteKit
still pins ^0.6), minimatch@9 (mjml→js-beautify chain), immutable@3
(graphql-codegen→relay-compiler@12 chain).
GitHub Actions — bump actions/{checkout,setup-node,setup-dotnet,upload-artifact,
download-artifact} v4→v5, pnpm/action-setup and arduino/setup-task SHA pins
→ v5/v2 tags, actions/labeler v5→v6, SHA-pin remaining actions, all for Node-24
compatibility ahead of the June 2026 deprecation. Drop the unused Claude Code
workflow.
FwLiteMaui — bump android SupportedOSPlatformVersion 23→24 to clear CA1416
warnings on BlazorWebView APIs (Android 6.0 Marshmallow is rounding-error
share in 2026; the WebView is the app, so the platform attribute was
inaccurate, not actually enabling API 23 support).
FwLiteWeb — drop the Assembly.Location dance, use AppContext.BaseDirectory
directly (IL3000).
The svelte 5.53→5.55 bump + detached-resource test harness moved to #2323; the
Platform.Bible extension (npm) bumps moved to their own PR. This PR is the
pnpm/CI/.NET dependency bumps only.
Not fixed (need follow-up issues): mjml ≤4.18.0 (no patch in v4 line — needs
v5 migration; html-minifier ReDoS is transitive via it).
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
myieye
added a commit
that referenced
this pull request
Jun 1, 2026
Frontend (pnpm) — direct: @sveltejs/kit 2.49→2.60.1, @opentelemetry/sdk-node
0.208→0.217, @opentelemetry/auto-instrumentations-node 0.67→0.75, js-cookie
3.0.5→3.0.7. Catalog: vite→7.3.2, postcss→8.5.10. In viewer: lint-staged 13→15
(clears micromatch + yaml advisories naturally). Minimal pnpm.overrides for the
three transitives that don't have a clean parent-bump path: cookie (SvelteKit
still pins ^0.6), minimatch@9 (mjml→js-beautify chain), immutable@3
(graphql-codegen→relay-compiler@12 chain).
GitHub Actions — bump actions/{checkout,setup-node,setup-dotnet,upload-artifact,
download-artifact} v4→v5, pnpm/action-setup and arduino/setup-task SHA pins
→ v5/v2 tags, actions/labeler v5→v6, SHA-pin remaining actions, all for Node-24
compatibility ahead of the June 2026 deprecation. Drop the unused Claude Code
workflow.
FwLiteMaui — bump android SupportedOSPlatformVersion 23→24 to clear CA1416
warnings on BlazorWebView APIs (Android 6.0 Marshmallow is rounding-error
share in 2026; the WebView is the app, so the platform attribute was
inaccurate, not actually enabling API 23 support).
FwLiteWeb — drop the Assembly.Location dance, use AppContext.BaseDirectory
directly (IL3000).
The svelte 5.53→5.55 bump + detached-resource test harness moved to #2323; the
Platform.Bible extension (npm) bumps moved to their own PR. This PR is the
pnpm/CI/.NET dependency bumps only.
Not fixed (need follow-up issues): mjml ≤4.18.0 (no patch in v4 line — needs
v5 migration; html-minifier ReDoS is transitive via it).
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/viewer/src/project/detached-resource/WrappedResourceHarness.svelte`:
- Around line 3-45: The harness never tears down the ProjectContext causing
leaked $effect.root instances; update WrappedResourceHarness.svelte to call
projectContext.destroy() when the component unmounts by registering a teardown
in the Svelte destroy lifecycle (e.g., import and use onDestroy or the
framework-equivalent) so that projectContext.destroy() runs on unmount; keep
current initProjectContext, controls, and swapApi logic unchanged and only add
the lifecycle hook that invokes projectContext.destroy().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6d08409e-9d0e-48dd-935f-491ee11b2395
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
frontend/pnpm-workspace.yamlfrontend/viewer/.gitignorefrontend/viewer/src/DotnetProjectView.sveltefrontend/viewer/src/project/detached-resource/WrappedResourceConsumer.sveltefrontend/viewer/src/project/detached-resource/WrappedResourceHarness.sveltefrontend/viewer/src/project/detached-resource/detached-api-resource-test-types.tsfrontend/viewer/src/project/detached-resource/wrapped-resource.browser.test.tsfrontend/viewer/src/project/detached-resource/wrapper-service.svelte.tsfrontend/viewer/src/project/project-context.svelte.ts
Bump the svelte catalog entry 5.53→5.55.7 (resolves 5.56.0). This is split out
of the dependency/CI security PR because it is the one bump that changes runtime
behaviour rather than just clearing an advisory.
Fix a cached-service freeze in project-context: own the getOrAdd factories in
$effect.root so a service created by a consumer that unmounts before the
resource resolves is no longer torn down (or frozen) underneath a later
consumer. Add a focused browser-test harness (WrappedResource{Consumer,Harness},
wrapper-service) covering the three distinct failure modes: happy-path render,
creator-unmounts-before-resolve (the canonical regression), and
api-swap-after-creator-unmount (the real-world trigger).
Gitignore eslint_report.json and __screenshots__/ test artifacts.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The $effect.root instances that #ownAndCache creates to own cached-service deriveds aren't tied to the component tree, so they (and their $derived subscriptions) survived past the view that created the context. destroy() existed to tear them down but nothing called it; wire it into onDestroy so roots don't accumulate across project switches. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PartOfSpeechService.current was a `$derived.by`. A cached service's derived can be read back stale once its observers unmount (e.g. the browse list/filter during in-app navigation), so PoS labels disappeared from the dictionary preview even though the parts-of-speech fetch had resolved. Expose it as a plain getter over the resource `$state` (matching WritingSystemService), which can't go stale. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
myieye
force-pushed
the
chore/svelte-5.55-detached-resource
branch
from
7172be2 to
eb7c9e8
Compare
MorphTypesService, PublicationService and CustomViewService exposed resource-backed data via `$derived.by` exactly like PartOfSpeechService did, so they share the same staleness: a cached service's derived can read back stale once its observers unmount. Convert their `current` (and MorphTypes' `prefixes`/`suffixes`, read via decorate() in transient list rows) to plain getters over the resource `$state`, matching WritingSystemService. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The missing parts of speech in the dictionary preview did not reproduce in a clean environment with `$derived.by` — it appears to have been a dev-server stale-state artifact, not the reactive form. The getter conversions were therefore unnecessary and lost memoization, so revert PartOfSpeechService, MorphTypesService, PublicationService and CustomViewService back to `$derived.by`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Both harnesses call initProjectContext but never tore down the context-owned $effect.root instances; call projectContext.destroy() on unmount, mirroring DotnetProjectView. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Transform by appending '-derived' instead of upper-casing - Move getOrAdd caching into useWrapperService (mirrors usePartsOfSpeech) - Assert the pre-swap consumer renders NONE for the post-swap key - Auto-destroy ProjectContext via onDestroy in initProjectContext and drop the manual destroy calls Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…detached-resource
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
1 participant
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.
Split out of #2315 (the dependency/CI security PR) so that PR stays trivially mergeable.
This carries the one change with runtime impact — the svelte catalog bump 5.53→5.55.7 (resolves 5.56.0) — together with the detached-resource work it was entangled with:
getOrAddfactories in$effect.rootso a service created by a consumer that unmounts before its resource resolves isn't torn down/frozen under a later consumer (cached-service freeze).initProjectContextnow registers the matching teardown on its caller'sonDestroy.WrappedResource{Consumer,Harness},wrapper-service) covering the three distinct failure modes: happy-path render, creator-unmounts-before-resolve (canonical regression), api-swap-after-creator-unmount (real-world trigger).This completes a third layer of test coverage — warranted for reactivity this tricky:
detached-resource.test.tscovers theDetachedResourceclass alone,detached-api-resource.browser.test.tscoversProjectContext.apiResourcethrough components asserting the resource directly (both pre-existing), and the newwrapped-resource.browser.test.tscovers a cached service's$derivedover the resource as rendered by a later consumer — the layer where the live bug (stalePartOfSpeechService) actually surfaced, and the only one that fails if the$effect.rootfix is reverted.🤖 Generated with Claude Code