Conversation
…instances Source upstream commit: 19030df Port scope: adapted (resolved Task/Instances/Shared/Relay conflicts against fork runtime layout).
Source upstream commit: c96e27a Port scope: adapted (resolved bun.lock conflict with upstream dependency updates).
…d add tests Source upstream commit: 485ea45 Port scope: not-applicable (fork has no eslint.config.mjs and no scripts/eslint-plugin-evolu.* files).
Source upstream commit: 9803147 Port scope: selective (ported common/vue PURE annotation updates and tree-shaking snapshot changes; skipped missing apps/web path).
Source upstream commit: dfc84a4 Port scope: adapted.
Source upstream commit: ce057ed Port scope: adapted.
Source upstream commit: 4b2d924 Port scope: adapted.
Source upstream commit: 864bb43 Port scope: adapted.
Source upstream commit: d30c2ac Port scope: adapted.
Source upstream commit: cd6b74d Port scope: selective (ported Query/Kysely API rename into existing fork structure and examples; skipped missing .changeset and apps/web docs/playgrounds paths).
Source upstream commit: 69a55fb Port scope: adapted.
Source upstream commit: a0b7371 Port scope: not-applicable (upstream change touches missing apps/web playground path in this fork).
…m guidance Source upstream commit: ef36be2 Port scope: adapted.
Source upstream commit: c803aef Port scope: adapted.
Source upstream commit: 617ee37 Port scope: adapted.
…style Source upstream commit: 768356c Port scope: adapted.
Source upstream commit: a726b7b Port scope: adapted (standardized runner variable naming while preserving fork relay/db runtime wiring).
Restore cross-package build compatibility after the sync wave by adding Task API aliases, adapting Sync/Shared wiring to updated Resources semantics, and aligning relay/react-native helpers with updated Result/Promise typings.
… example, and improve resource handling in common resources.
miccy
added
upstream
📝 WalkthroughWalkthroughVelké refaktoringy a funkční rozšíření: přejmenování Runner→Run, rozšíření Ref/Store API, nový asynchronní Resources model, odstranění Instances a přesun Kysely JSON helperů do Query, plus řada drobných anotací a závislostních aktualizací. Changes
Sequence Diagram(s)(žádný diagram generován) Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Báseň
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c32892fd10
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const query of [ | ||
| sql` | ||
| create table if not exists evolu_writeKey ( | ||
| create table evolu_writeKey ( |
There was a problem hiding this comment.
createRelayStorageTables now uses plain CREATE TABLE for relay tables, so rerunning startup against an existing database will fail with table ... already exists and abort relay initialization. This function is used during normal relay boot with persisted SQLite files, so removing IF NOT EXISTS turns a routine restart into a hard failure; keep table creation idempotent (or explicitly gate it behind migration checks).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR syncs updates from common-v8 across the monorepo, focusing on API refactors (notably Ref/Store), improved local-first query JSON handling, and more robust resource/relay lifecycle management, alongside dependency and documentation updates.
Changes:
- Refactors mutable state utilities (
Ref,Store) to a richer, more consistent method set and updates tests accordingly. - Consolidates/updates Kysely SQLite JSON helper functionality into
local-first/Query.tswith Evolu-specific JSON prefixing + recursive parsing, and updates downstream usages/examples. - Refactors resource and worker/relay internals (introducing async
Resources, SharedWorker changes, relay startup tweaks) and updates dependencies + docs.
Reviewed changes
Copilot reviewed 53 out of 55 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/web/src/Task.ts | Renames browser wording to web-platform wording in docs/group. |
| packages/vue/src/provideEvolu.ts | Adds a lint suppression comment above evoluInstanceMap. |
| packages/vue/package.json | Bumps Vue peer dependency patch. |
| packages/svelte/package.json | Bumps Svelte dependency + peer dependency patch. |
| packages/react-native/src/Polyfills.ts | Improves typing for Promise.try polyfill. |
| packages/nodejs/src/local-first/Relay.ts | Improves sqlite acquisition handling and avoids runner variable shadowing. |
| packages/common/test/local-first/Query.test.ts | Adds coverage for Evolu JSON-prefix Kysely helpers + recursive parsing behavior. |
| packages/common/test/local-first/Kysely.test.ts | Switches tests to new helper exports and loosens error-message matching. |
| packages/common/test/local-first/Evolu.test.ts | Updates tests for export behavior and snapshot stability; wires test WebSocket dep. |
| packages/common/test/WebSocket.test.ts | Switches setup/teardown to beforeAll/afterAll for shared server. |
| packages/common/test/TreeShaking.test.ts | Updates expected bundle sizes after changes. |
| packages/common/test/Store.test.ts | Expands test suite for new Store API and revised equality/notification semantics. |
| packages/common/test/Resources.test.ts | Adds rollback test for legacy resource add-consumer behavior when creation throws. |
| packages/common/test/Ref.test.ts | Expands test suite for new Ref API and updated semantics. |
| packages/common/test/Instances.test.ts | Removes tests for deleted Instances utility. |
| packages/common/src/local-first/Timestamp.ts | Adds /*#__PURE__*/ annotations to branded types. |
| packages/common/src/local-first/Sync.ts | Reworks sync resource handling, websockets management, and mutex storage (Map-based). |
| packages/common/src/local-first/Storage.ts | Adds /*#__PURE__*/ annotations and adjusts type construction. |
| packages/common/src/local-first/Shared.ts | Refactors SharedWorker lifecycle, adds transport resources, and changes SharedEvolu disposal model. |
| packages/common/src/local-first/Schema.ts | Updates docs/types/imports for new Evolu JSON helpers; adds /*#__PURE__*/ on Kysely construction and types. |
| packages/common/src/local-first/Relay.ts | Replaces per-owner mutex multiton with keyed mutex; adjusts write logic and table creation. |
| packages/common/src/local-first/Query.ts | Adds Evolu-prefixed JSON helper implementations, exports Kysely utilities, and improves recursive JSON parsing. |
| packages/common/src/local-first/Protocol.ts | Adds /*#__PURE__*/ annotations to protocol constants. |
| packages/common/src/local-first/Kysely.ts | Removes legacy Kysely helper module (moved into Query.ts). |
| packages/common/src/local-first/Evolu.ts | Makes OnExport without pending export throw (assert) instead of being ignored. |
| packages/common/src/local-first/Db.ts | Removes leader heartbeat sender and changes export payload handling. |
| packages/common/src/index.ts | Removes Instances export and re-exports new Kysely/Evolu JSON helpers/types from Query.ts. |
| packages/common/src/WebSocket.ts | Adds testCreateWebSocket helper. |
| packages/common/src/Type.ts | Adds /*#__PURE__*/ annotations in a few branded type constructors. |
| packages/common/src/Time.ts | Adds /*#__PURE__*/ annotation to branded Millis type constructor. |
| packages/common/src/Test.ts | Adds /*#__PURE__*/ annotation to deterministic entropy bytes. |
| packages/common/src/Store.ts | Updates store API semantics and wiring to match new Ref API. |
| packages/common/src/Sqlite.ts | Refactors driver creation variable naming/logging; adds /*#__PURE__*/ annotations in schema types. |
| packages/common/src/Set.ts | Marks emptySet construction as /*#__PURE__*/. |
| packages/common/src/Schedule.ts | Adds /*#__PURE__*/ annotations inside retry strategy composition. |
| packages/common/src/Result.ts | Removes unused UnknownError import and expands Result docs/guidance. |
| packages/common/src/Resources.ts | Introduces async Resources alongside legacy sync resources, adds rollback behavior, overloads createResources. |
| packages/common/src/Ref.ts | Refactors Ref to richer API (get/set/update/modify variants) and changes semantics. |
| packages/common/src/Number.ts | Adds /*#__PURE__*/ annotation to branded number type constructor. |
| packages/common/src/Instances.ts | Deletes unused Instances multiton utility. |
| packages/common/package.json | Bumps msgpackr and fast-check versions. |
| package.json | Bumps turbo version. |
| examples/vue-vite-pwa/package.json | Bumps Vue patch version. |
| examples/svelte-vite-pwa/package.json | Bumps Svelte patch version. |
| examples/react-vite-pwa/src/components/EvoluMinimalExample.tsx | Updates type narrowing from Evolu.kysely.NotNull to Evolu.KyselyNotNull. |
| examples/react-nextjs/components/EvoluMinimalExample.tsx | Updates type narrowing from Evolu.kysely.NotNull to Evolu.KyselyNotNull. |
| examples/react-expo/package.json | Bumps react-native-safe-area-context. |
| examples/react-expo/app/index.tsx | Updates type narrowing from Evolu.kysely.NotNull to Evolu.KyselyNotNull. |
| examples/react-electron/components/EvoluMinimalExample.tsx | Updates type narrowing from Evolu.kysely.NotNull to Evolu.KyselyNotNull. |
| examples/angular-vite-pwa/package.json | Bumps Angular patch versions. |
| biome.json | Bumps Biome schema URL. |
| apps/relay/src/startBunRelay.ts | Uses getOrThrow for sqlite acquisition. |
Comments suppressed due to low confidence (1)
packages/common/src/local-first/Relay.ts:324
createRelayStorageTablesnow uses plaincreate table ...withoutif not exists. This will throw on subsequent relay starts against an existing database (e.g.,apps/relay/src/startBunRelay.tscallscreateRelayStorageTables(deps)unconditionally). Restoreif not existshere, or make the call sites guard table creation based on DB existence/migrations.
packages/vue/src/provideEvolu.ts
Outdated
| // eslint-disable-next-line evolu/require-pure-annotation | ||
| export const evoluInstanceMap = new WeakMap< | ||
| ComponentInternalInstance, |
There was a problem hiding this comment.
This file adds an eslint-disable-next-line evolu/require-pure-annotation directive, but the repo uses Biome (and has no ESLint config), so this suppression is likely ineffective and adds tooling noise. Prefer addressing the underlying requirement (e.g., add a /*#__PURE__*/ annotation if tree-shaking needs it) or use a Biome suppression if there’s an actual Biome rule to silence.
| onDispose: () => { | ||
| void runWithSharedEvoluDeps.daemon( | ||
| sharedEvolusMutexByName.withLock(message.name, () => { | ||
| sharedEvolusByName.delete(message.name); | ||
| return ok(); | ||
| }), | ||
| ); | ||
| }, | ||
| }), |
There was a problem hiding this comment.
SharedEvolu now implements AsyncDisposable, but the onDispose callback passed to createSharedEvolu only deletes the instance from sharedEvolusByName and never calls sharedEvolu[Symbol.asyncDispose](). When the last port disposes (if (evoluPorts.size === 0) onDispose()), this leaks the callbacks/timeout/fiber state and (when appOwner is set) keeps transports.addConsumer references alive. Update the disposal path to actually dispose the SharedEvolu instance (and ideally await it) before removing it from the map.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (6)
packages/common/src/Task.ts (1)
1742-1751: Zvažte zjednodušení extrakce task argumentu.IIFE na řádcích 1742-1746 lze zjednodušit na ternární výraz, protože logika je přímočará.
♻️ Návrh zjednodušení
export function concurrently<T, E, D = unknown>( concurrencyOrTask: Concurrency | Task<T, E, D>, taskOrFallback?: Task<T, E, D>, ): Task<T, E, D> { const isTask = isFunction(concurrencyOrTask); - const task = (() => { - if (isTask) return concurrencyOrTask; - assert(taskOrFallback, "Task is required when concurrency is provided."); - return taskOrFallback; - })(); + if (!isTask) { + assert(taskOrFallback, "Task is required when concurrency is provided."); + } + const task = isTask ? concurrencyOrTask : taskOrFallback!; return Object.assign((run: Run<D>) => run(task), { [concurrencyBehaviorSymbol]: isTask ? maxPositiveInt : concurrencyOrTask, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/common/src/Task.ts` around lines 1742 - 1751, The IIFE used to derive task can be replaced with a single ternary expression: directly set task = isTask ? concurrencyOrTask : (assert(taskOrFallback, "Task is required when concurrency is provided."), taskOrFallback) or otherwise ensure the assertion runs and task gets taskOrFallback when isTask is false; update the code that returns Object.assign((run: Run<D>) => run(task), { [concurrencyBehaviorSymbol]: isTask ? maxPositiveInt : concurrencyOrTask }) to use the simplified task binding (referencing isTask, concurrencyOrTask, taskOrFallback, concurrencyBehaviorSymbol, and maxPositiveInt).packages/react-native/src/Polyfills.ts (1)
37-40: Preferujte readonly args a odstraňte zbytečnou aserci.
argsse nikde nemění, takžeArray<unknown>zbytečně zužuje veřejný podpis.Promise.resolve(func(...args))automaticky vracíPromise<Awaited<T>>bez nutnosti explicitní aserce.♻️ Navržená úprava
- try?: <T, U extends Array<unknown>>( + try?: <T, U extends ReadonlyArray<unknown>>( func: (...args: U) => T | PromiseLike<T>, ...args: U ) => Promise<Awaited<T>>; @@ - PromiseStatic.try = <T, U extends Array<unknown>>( + PromiseStatic.try = <T, U extends ReadonlyArray<unknown>>( func: (...args: U) => T | PromiseLike<T>, ...args: U - ): Promise<Awaited<T>> => - new Promise<Awaited<T>>((resolve, reject) => { - try { - resolve(func(...args) as Awaited<T>); - } catch (error) { - reject(error); - } - }); + ): Promise<Awaited<T>> => { + try { + return Promise.resolve(func(...args)); + } catch (error) { + return Promise.reject(error); + } + };Platí pro: 37–40, 58–68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native/src/Polyfills.ts` around lines 37 - 40, Upravte veřejné podpisy metody/variantu označené jako "try?" (a obdobné přetížení v rozsahu 58–68) tak, aby používaly readonly typ pro argumenty (např. ...args: readonly unknown[]) místo mutovatelného Array<unknown>, a odstraňte zbytečnou explicitní aserci typu při vracení Promise (ponechte Promise.resolve(func(...args)) bez "as Promise<Awaited<T>>"), čímž zachováte správný návratový typ Awaited<T> a reflektujete, že args se nemění.packages/common/test/WebSocket.test.ts (1)
21-42: Sdílený WS server mezi testy zhoršuje izolaci.Tahle změna zavádí jednu společnou serverovou instanci a globální
portpro celý soubor. Když některý test nechá otevřený socket nebo změní serverový stav, další testy už neběží nad čistým prostředím, takže vznikající nestabilitu bude těžké reprodukovat. Pokud chcete zůstat ubeforeAll, přidejte aspoň explicitní reset serveru mezi testy.Based on learnings "Never rely on global state or shared mutable deps between tests. Create fresh deps at the start of each test".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/common/test/WebSocket.test.ts` around lines 21 - 42, The tests share a single global WebSocket server via the module-level variable port and beforeAll/afterAll (createServer/closeServer, startWsServer/stopWsServer), which breaks isolation; change to create and tear down a fresh server for each test (use beforeEach/afterEach or call an explicit reset between tests) so that each test invokes createServer/startWsServer and closes it with closeServer/stopWsServer (or resets server state) and remove reliance on the global port variable to ensure no leftover sockets or mutated server state leak between tests.packages/common/test/Ref.test.ts (1)
11-133: Přidejte i compile-time aserce pro novéRefAPI.Runtime pokrytí je tu dobré, ale hlavní změna API je i typová. Bez
expectTypeOfnezachytíte regresi jakoset(): voidnebo změněný návratový typ ugetAndSet/setAndGet/modify.As per coding guidelines "Use `expectTypeOf` from Vitest for compile-time type assertions in tests".💡 Možné doplnění
-import { describe, expect, test } from "vitest"; +import { describe, expect, expectTypeOf, test } from "vitest"; import { createRef } from "../src/Ref.js"; +test("preserves the Ref type contract", () => { + const ref = createRef(1); + + expectTypeOf(ref.set(2)).toEqualTypeOf<void>(); + expectTypeOf(ref.getAndSet(2)).toEqualTypeOf<number>(); + expectTypeOf(ref.setAndGet(2)).toEqualTypeOf<number>(); + expectTypeOf( + ref.modify((current) => [`value:${current}`, current + 1] as const), + ).toEqualTypeOf<string>(); +}); + describe("get", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/common/test/Ref.test.ts` around lines 11 - 133, Add compile-time type assertions using Vitest's expectTypeOf for the new Ref API: import expectTypeOf and assert types for createRef return and each method signature (createRef, Ref.set, Ref.get, getAndSet, setAndGet, update, getAndUpdate, updateAndGet, modify) to ensure set returns void, get returns the state type, getAndSet/setAndGet/modify return the correct value types, and updater callbacks accept and return the state type; place these expectTypeOf checks alongside the runtime tests to catch type regressions.packages/common/src/local-first/Sync.ts (1)
430-444: Type casting pro kontrolu AbortError.Type casting
(result.error as { type?: string }).typeje křehký přístup. Zvažte zavedení type guard funkce pro kontrolu AbortError.♻️ Návrh type guard funkce
const isAbortError = (error: unknown): boolean => typeof error === "object" && error !== null && "type" in error && (error as { type: unknown }).type === "AbortError";Pak lze použít:
-if ((result.error as { type?: string }).type !== "AbortError") { +if (!isAbortError(result.error)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/common/src/local-first/Sync.ts` around lines 430 - 444, Replace the fragile cast when checking for an AbortError in the syncRun result handler by adding a reusable type guard (e.g., isAbortError) and using it instead of `(result.error as { type?: string }).type`; update the success/error callback in syncRun where you currently call config.onError(createUnknownError(result.error)) and the conditional `if ((result.error as { type?: string }).type !== "AbortError")` to use isAbortError(result.error) to decide whether to call createUnknownError, keeping the surrounding logic (syncOwnerRefs.has(owner.id), sendSubscribeForOwner(owner)) unchanged.packages/common/src/Store.ts (1)
94-99: Nadbytečné voláníref.get()vupdateAndGet.Metoda
ref.updateAndGet(updater)již vrací aktualizovaný stav, takže následné voláníref.get()je redundantní. Pro konzistenci s ostatními metodami by stačilo vrátit výsledek přímo.♻️ Možná optimalizace
updateAndGet: (updater) => { const previousState = ref.get(); - ref.updateAndGet(updater); + const newState = ref.updateAndGet(updater); notifyIfChanged(previousState); - return ref.get(); + return newState; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/common/src/Store.ts` around lines 94 - 99, The updateAndGet implementation calls ref.updateAndGet(updater) but then redundantly calls ref.get(); change it to capture and return the value returned by ref.updateAndGet(updater) instead of calling ref.get(), while still calling notifyIfChanged(previousState) as before; update the function named updateAndGet to set something like const newState = ref.updateAndGet(updater); notifyIfChanged(previousState); return newState so you avoid the extra ref.get() call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/common/src/local-first/Evolu.ts`:
- Around line 766-773: The OnExport handler currently asserts that
exportDatabasePending exists, which crashes if an OnExport arrives during/after
asyncDispose; modify the case "OnExport" in the message handler to replace the
assert with a conditional: if exportDatabasePending is non-null call
exportDatabasePending.resolve(message.file) and set exportDatabasePending =
null, otherwise ignore the message (optionally log a debug message); ensure this
change covers race scenarios between exportDatabase() and asyncDispose() and
references the symbols exportDatabasePending, exportDatabase(), asyncDispose,
and the "OnExport" case.
In `@packages/common/src/local-first/Query.ts`:
- Around line 123-127: makePatches is still comparing values via eqSqliteValue
after casting to SqliteValue, which generates false patches for nested JSON
returned by evoluJsonObjectFrom/evoluJsonArrayFrom because parseSqliteJsonArray
produces new object references; update makePatches to detect when a Row value is
an object or array (per the Row type) and perform a structural/deep equality
check (or compare canonical JSON) instead of eqSqliteValue for those cases;
adjust logic around makePatches, eqSqliteValue, and parseSqliteJsonArray so
nested parsed JSON is compared structurally rather than by reference to avoid
spurious diffs.
In `@packages/common/src/local-first/Relay.ts`:
- Around line 114-115: deleteOwner currently bypasses the same per-owner keyed
mutex used by writeMessages (mutexByOwnerId), allowing concurrent
deleteOwner(ownerId) and writeMessages(ownerId, ...) to race; fix by running the
entire deleteOwner body under the same keyed lock (use
mutexByOwnerId.run(ownerId, async () => { ... }) or equivalent) so deletes and
writes for the same OwnerId are serialized, and apply the same change to the
other delete path mentioned (the block around the 283-300 region) so all
owner-deletion code uses mutexByOwnerId for synchronization.
- Around line 184-190: Duplicate timestamps in the incoming batch are counted
twice for quota even though the DB will store only one row; before computing
incomingBytes and calling updateOwnerUsage, deduplicate
messagesWithTimestampBytes by timestamp.toString() (e.g., build a Map keyed by
timestamp string and keep the first/representative message) to produce a
dedupedMessages array, then use that deduped array when creating
existingTimestampsSet filtering, calculating incomingBytes, and before the
insert (affects the logic around existingTimestampsSet,
messagesWithTimestampBytes, newMessages and updateOwnerUsage; also apply same
fix in the similar block around lines 205–245).
In `@packages/common/src/local-first/Shared.ts`:
- Around line 195-202: Callback onDispose currently schedules a delete via
runWithSharedEvoluDeps.daemon + sharedEvolusMutexByName.withLock but doesn't
verify the instance identity or call the instance's async disposer, so a newly
created instance with the same name can be removed and transports/timeouts
remain registered; change the onDispose handler to (1) capture the specific
sharedEvolu instance at creation time, (2) inside the mutexed function compare
sharedEvolusByName.get(message.name) === capturedInstance before deleting, and
(3) if deleting, invoke await capturedInstance[Symbol.asyncDispose]() (or call
its async dispose method) to unregister transports/timeouts before removing the
map entry (references: onDispose, runWithSharedEvoluDeps.daemon,
sharedEvolusMutexByName.withLock, sharedEvolusByName, CreateEvolu, sharedEvolu,
Symbol.asyncDispose).
- Around line 530-558: The dispose branch currently removes evoluPorts and queue
entries but doesn't clear the corresponding db worker mapping, which can leave
activeDbWorkerPort pointing at a closed tab; add and maintain a mapping (e.g.,
dbWorkerPorts: Map<evoluPortId, DbWorkerPort>) and in this dispose block delete
dbWorkerPorts.delete(evoluPortId) and if activeDbWorkerPort refers to that
evoluPortId then immediately drop the leader by setting activeDbWorkerPort =
null and performing the same cleanup you do when isActiveQueueItemDisposed
(clearActiveLeaderTimeout, abort queueProcessingFiber, cancel
activeQueueCallbackId), then continue with evoluPorts.delete,
rowsByQueryByEvoluPortId.delete, queue splices and ensureQueueProcessing so no
requests are sent to a dead port.
- Around line 159-162: The cache keyed only by Name (sharedEvolusByName,
sharedEvolusMutexByName) causes createSharedEvolu and transports.addConsumer to
be skipped when the first CreateEvolu for a name has no appOwner or a different
owner; change the caching strategy so the key includes both name and appOwner
(or owner identifier) OR detect owner changes and re-run createSharedEvolu and
transports.addConsumer for the existing SharedEvolu; update
runWithSharedEvoluDeps usage and the lookup logic around
sharedEvolusByName/sharedEvolusMutexByName (and the code block referenced at
183-214) to use the composite key (name+appOwner) or to patch an existing
instance by invoking createSharedEvolu/transports.addConsumer when owner
differs.
In `@packages/common/src/WebSocket.ts`:
- Around line 321-337: The testCreateWebSocket fake currently keeps reporting an
open socket even after [Symbol.dispose] is called; modify testCreateWebSocket to
maintain an internal "closed" flag set when [Symbol.dispose]() runs and make
send, isOpen and getReadyState consult that flag: after disposal isOpen() should
return false, getReadyState() should return "closed", and send() should fail
(return an error/result indicating closed) instead of succeeding. Update the
returned object in testCreateWebSocket to use this internal state so
cleanup-path tests behave like a real closed socket.
---
Nitpick comments:
In `@packages/common/src/local-first/Sync.ts`:
- Around line 430-444: Replace the fragile cast when checking for an AbortError
in the syncRun result handler by adding a reusable type guard (e.g.,
isAbortError) and using it instead of `(result.error as { type?: string
}).type`; update the success/error callback in syncRun where you currently call
config.onError(createUnknownError(result.error)) and the conditional `if
((result.error as { type?: string }).type !== "AbortError")` to use
isAbortError(result.error) to decide whether to call createUnknownError, keeping
the surrounding logic (syncOwnerRefs.has(owner.id),
sendSubscribeForOwner(owner)) unchanged.
In `@packages/common/src/Store.ts`:
- Around line 94-99: The updateAndGet implementation calls
ref.updateAndGet(updater) but then redundantly calls ref.get(); change it to
capture and return the value returned by ref.updateAndGet(updater) instead of
calling ref.get(), while still calling notifyIfChanged(previousState) as before;
update the function named updateAndGet to set something like const newState =
ref.updateAndGet(updater); notifyIfChanged(previousState); return newState so
you avoid the extra ref.get() call.
In `@packages/common/src/Task.ts`:
- Around line 1742-1751: The IIFE used to derive task can be replaced with a
single ternary expression: directly set task = isTask ? concurrencyOrTask :
(assert(taskOrFallback, "Task is required when concurrency is provided."),
taskOrFallback) or otherwise ensure the assertion runs and task gets
taskOrFallback when isTask is false; update the code that returns
Object.assign((run: Run<D>) => run(task), { [concurrencyBehaviorSymbol]: isTask
? maxPositiveInt : concurrencyOrTask }) to use the simplified task binding
(referencing isTask, concurrencyOrTask, taskOrFallback,
concurrencyBehaviorSymbol, and maxPositiveInt).
In `@packages/common/test/Ref.test.ts`:
- Around line 11-133: Add compile-time type assertions using Vitest's
expectTypeOf for the new Ref API: import expectTypeOf and assert types for
createRef return and each method signature (createRef, Ref.set, Ref.get,
getAndSet, setAndGet, update, getAndUpdate, updateAndGet, modify) to ensure set
returns void, get returns the state type, getAndSet/setAndGet/modify return the
correct value types, and updater callbacks accept and return the state type;
place these expectTypeOf checks alongside the runtime tests to catch type
regressions.
In `@packages/common/test/WebSocket.test.ts`:
- Around line 21-42: The tests share a single global WebSocket server via the
module-level variable port and beforeAll/afterAll (createServer/closeServer,
startWsServer/stopWsServer), which breaks isolation; change to create and tear
down a fresh server for each test (use beforeEach/afterEach or call an explicit
reset between tests) so that each test invokes createServer/startWsServer and
closes it with closeServer/stopWsServer (or resets server state) and remove
reliance on the global port variable to ensure no leftover sockets or mutated
server state leak between tests.
In `@packages/react-native/src/Polyfills.ts`:
- Around line 37-40: Upravte veřejné podpisy metody/variantu označené jako
"try?" (a obdobné přetížení v rozsahu 58–68) tak, aby používaly readonly typ pro
argumenty (např. ...args: readonly unknown[]) místo mutovatelného
Array<unknown>, a odstraňte zbytečnou explicitní aserci typu při vracení Promise
(ponechte Promise.resolve(func(...args)) bez "as Promise<Awaited<T>>"), čímž
zachováte správný návratový typ Awaited<T> a reflektujete, že args se nemění.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4a62656c-09dd-4060-822c-dad96857d728
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (54)
apps/relay/src/startBunRelay.tsbiome.jsonexamples/angular-vite-pwa/package.jsonexamples/react-electron/components/EvoluMinimalExample.tsxexamples/react-expo/app/index.tsxexamples/react-expo/package.jsonexamples/react-nextjs/components/EvoluMinimalExample.tsxexamples/react-vite-pwa/src/components/EvoluMinimalExample.tsxexamples/svelte-vite-pwa/package.jsonexamples/vue-vite-pwa/package.jsonpackage.jsonpackages/common/package.jsonpackages/common/src/Instances.tspackages/common/src/Number.tspackages/common/src/Ref.tspackages/common/src/Resources.tspackages/common/src/Result.tspackages/common/src/Schedule.tspackages/common/src/Set.tspackages/common/src/Sqlite.tspackages/common/src/Store.tspackages/common/src/Task.tspackages/common/src/Test.tspackages/common/src/Time.tspackages/common/src/Type.tspackages/common/src/WebSocket.tspackages/common/src/index.tspackages/common/src/local-first/Db.tspackages/common/src/local-first/Evolu.tspackages/common/src/local-first/Kysely.tspackages/common/src/local-first/Protocol.tspackages/common/src/local-first/Query.tspackages/common/src/local-first/Relay.tspackages/common/src/local-first/Schema.tspackages/common/src/local-first/Shared.tspackages/common/src/local-first/Storage.tspackages/common/src/local-first/Sync.tspackages/common/src/local-first/Timestamp.tspackages/common/test/Instances.test.tspackages/common/test/Ref.test.tspackages/common/test/Resources.test.tspackages/common/test/Store.test.tspackages/common/test/Task.test.tspackages/common/test/TreeShaking.test.tspackages/common/test/WebSocket.test.tspackages/common/test/local-first/Evolu.test.tspackages/common/test/local-first/Kysely.test.tspackages/common/test/local-first/Query.test.tspackages/nodejs/src/local-first/Relay.tspackages/react-native/src/Polyfills.tspackages/svelte/package.jsonpackages/vue/package.jsonpackages/vue/src/provideEvolu.tspackages/web/src/Task.ts
💤 Files with no reviewable changes (3)
- packages/common/src/local-first/Kysely.ts
- packages/common/test/Instances.test.ts
- packages/common/src/Instances.ts
| case "OnExport": { | ||
| if (exportDatabasePending) { | ||
| exportDatabasePending.resolve(message.file); | ||
| exportDatabasePending = null; | ||
| } | ||
| // Silently ignore late OnExport after disposal | ||
| assert( | ||
| exportDatabasePending, | ||
| "OnExport received without pending export.", | ||
| ); | ||
| exportDatabasePending.resolve(message.file); | ||
| exportDatabasePending = null; | ||
| break; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Hledání, jak je řešen disposal v jiných message handlerech
ast-grep --pattern $'case "$_": {
$$$
assert($_, $_);
$$$
}'
# Kontrola, zda existují testy pro disposal během exportu
rg -n "OnExport|exportDatabase" --type ts -g '*test*'Repository: SQLoot/evolu-plan-b
Length of output: 2330
🏁 Script executed:
#!/bin/bash
# Read the disposal code (lines 892-896)
sed -n '880,910p' packages/common/src/local-first/Evolu.ts
# Read the export test that checks the assertion
sed -n '1203,1240p' packages/common/test/local-first/Evolu.test.ts
# Check if there are tests combining disposal and exportDatabase
rg -n "dispose|asyncDispose" packages/common/test/local-first/Evolu.test.ts -B 3 -A 3 | head -100Repository: SQLoot/evolu-plan-b
Length of output: 5044
🏁 Script executed:
#!/bin/bash
# Check the message handling loop to understand ordering
sed -n '750,810p' packages/common/src/local-first/Evolu.ts
# Check how worker messages are processed
rg -n "postEvoluOutput|OnExport" packages/common/src/local-first/Evolu.ts -B 2 -A 5 | head -80Repository: SQLoot/evolu-plan-b
Length of output: 2212
🏁 Script executed:
#!/bin/bash
# Look for the complete asyncDispose implementation
sed -n '880,920p' packages/common/src/local-first/Evolu.ts
# Check the entire OnExport case and surrounding code
sed -n '760,800p' packages/common/src/local-first/Evolu.tsRepository: SQLoot/evolu-plan-b
Length of output: 2561
Opravit ráci condition při zásahu OnExport po dispose.
Aserce selže, pokud OnExport zpráva dorazí během nebo krátce po asyncDispose:
exportDatabase()nastavíexportDatabasePendinga odešle "Export" zprávu do workeruasyncDispose()nastavíexportDatabasePending = nulla odešle "Dispose" zprávu- Worker může poslat
OnExportzprávu dříve, než se Dispose zpráva zpracuje (nebo pokud byl export již zahájen) - Handler obdrží
OnExportsexportDatabasePending === null→ aserce selže a aplikace spadne
Vzhledem k tomu, že test "does not execute mutate onComplete callback after dispose" (linie 527) očekává graceful handling pozdních zpráv po dispose, by měla aserce být nahrazena podmínkou:
Návrh opravy
case "OnExport": {
- assert(
- exportDatabasePending,
- "OnExport received without pending export.",
- );
- exportDatabasePending.resolve(message.file);
- exportDatabasePending = null;
+ if (exportDatabasePending) {
+ exportDatabasePending.resolve(message.file);
+ exportDatabasePending = null;
+ }
+ // Ignore late OnExport after disposal - race with asyncDispose is expected.
break;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/common/src/local-first/Evolu.ts` around lines 766 - 773, The
OnExport handler currently asserts that exportDatabasePending exists, which
crashes if an OnExport arrives during/after asyncDispose; modify the case
"OnExport" in the message handler to replace the assert with a conditional: if
exportDatabasePending is non-null call
exportDatabasePending.resolve(message.file) and set exportDatabasePending =
null, otherwise ignore the message (optionally log a debug message); ensure this
change covers race scenarios between exportDatabase() and asyncDispose() and
references the symbols exportDatabasePending, exportDatabase(), asyncDispose,
and the "OnExport" case.
| export interface Row { | ||
| readonly [key: string]: | ||
| | SqliteValue | ||
| | Row // for jsonObjectFrom from kysely/helpers/sqlite | ||
| | ReadonlyArray<Row>; // for jsonArrayFrom from kysely/helpers/sqlite | ||
| | Row // for evoluJsonObjectFrom | ||
| | ReadonlyArray<Row>; // for evoluJsonArrayFrom |
There was a problem hiding this comment.
Vnořené JSON výsledky teď vypadají jako změna při každém refreshi.
Row teď připouští objekty a pole, ale makePatches v tomhle souboru stále porovnává hodnoty přes eqSqliteValue(...) po castu na SqliteValue. Jakmile query vrátí evoluJsonObjectFrom nebo evoluJsonArrayFrom, po parseSqliteJsonArray dostanete nové objektové reference a diff začne generovat falešné patche i bez datové změny.
💡 Možná oprava
+const eqRowValue = (a: unknown, b: unknown): boolean => {
+ if (Array.isArray(a) && Array.isArray(b)) {
+ return a.length === b.length && a.every((value, index) => eqRowValue(value, b[index]));
+ }
+
+ if (isPlainObject(a) && isPlainObject(b)) {
+ const aKeys = Object.keys(a);
+ const bKeys = Object.keys(b);
+ return (
+ aKeys.length === bKeys.length &&
+ aKeys.every((key) => eqRowValue(a[key], b[key]))
+ );
+ }
+
+ return eqSqliteValue(a as SqliteValue, b as SqliteValue);
+};
+
export const makePatches = (
previousRows: ReadonlyArray<Row> | undefined,
nextRows: ReadonlyArray<Row>,
): ReadonlyArray<Patch> => {
@@
- if (
- !eqSqliteValue(
- previousRow[key] as SqliteValue,
- nextRow[key] as SqliteValue,
- )
- ) {
+ if (!eqRowValue(previousRow[key], nextRow[key])) {
replaceAtPatches.push({ op: "replaceAt", value: nextRow, index: i });
break;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/common/src/local-first/Query.ts` around lines 123 - 127, makePatches
is still comparing values via eqSqliteValue after casting to SqliteValue, which
generates false patches for nested JSON returned by
evoluJsonObjectFrom/evoluJsonArrayFrom because parseSqliteJsonArray produces new
object references; update makePatches to detect when a Row value is an object or
array (per the Row type) and perform a structural/deep equality check (or
compare canonical JSON) instead of eqSqliteValue for those cases; adjust logic
around makePatches, eqSqliteValue, and parseSqliteJsonArray so nested parsed
JSON is compared structurally rather than by reference to avoid spurious diffs.
| /** Mutex keyed by OwnerId to prevent concurrent writes. */ | ||
| const mutexByOwnerId = createMutexByKey<OwnerId>(); |
There was a problem hiding this comment.
deleteOwner obchází stejný keyed lock jako writeMessages.
Zápisy zpráv jsou teď serializované přes mutexByOwnerId, ale mazání stejného ownera ne. Paralelní deleteOwner(ownerId) a writeMessages(ownerId, ...) tak může ownera smazat a hned znovu částečně vytvořit, včetně rozjetých usage metadat. Mazání musí běžet pod stejnou synchronizací jako zápisy.
Also applies to: 283-300
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/common/src/local-first/Relay.ts` around lines 114 - 115, deleteOwner
currently bypasses the same per-owner keyed mutex used by writeMessages
(mutexByOwnerId), allowing concurrent deleteOwner(ownerId) and
writeMessages(ownerId, ...) to race; fix by running the entire deleteOwner body
under the same keyed lock (use mutexByOwnerId.run(ownerId, async () => { ... })
or equivalent) so deletes and writes for the same OwnerId are serialized, and
apply the same change to the other delete path mentioned (the block around the
283-300 region) so all owner-deletion code uses mutexByOwnerId for
synchronization.
| const existingTimestampsSet = new Set( | ||
| existingTimestampsResult.map((t) => t.toString()), | ||
| ); | ||
| const newMessages = filterArray( | ||
| messagesWithTimestampBytes, | ||
| (m) => !existingTimestampsSet.has(m.timestamp.toString()), | ||
| ); |
There was a problem hiding this comment.
Deduplikujte timestamp ještě před výpočtem kvóty.
existingTimestampsSet odfiltruje jen to, co už je v DB. Když jeden batch obsahuje stejný timestamp dvakrát, obě položky projdou do newMessages, incomingBytes se započítá 2× a updateOwnerUsage navýší usage, i když insert into evolu_message ... on conflict do nothing uloží jen jednu řádku. Tím se rozbije accounting a můžete vracet falešný StorageQuotaError.
Možná oprava
- const existingTimestampsSet = new Set(
+ const seenTimestamps = new Set(
existingTimestampsResult.map((t) => t.toString()),
);
const newMessages = filterArray(
messagesWithTimestampBytes,
- (m) => !existingTimestampsSet.has(m.timestamp.toString()),
+ (m) => {
+ const timestamp = m.timestamp.toString();
+ if (seenTimestamps.has(timestamp)) return false;
+ seenTimestamps.add(timestamp);
+ return true;
+ },
);Also applies to: 205-245
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/common/src/local-first/Relay.ts` around lines 184 - 190, Duplicate
timestamps in the incoming batch are counted twice for quota even though the DB
will store only one row; before computing incomingBytes and calling
updateOwnerUsage, deduplicate messagesWithTimestampBytes by timestamp.toString()
(e.g., build a Map keyed by timestamp string and keep the first/representative
message) to produce a dedupedMessages array, then use that deduped array when
creating existingTimestampsSet filtering, calculating incomingBytes, and before
the insert (affects the logic around existingTimestampsSet,
messagesWithTimestampBytes, newMessages and updateOwnerUsage; also apply same
fix in the similar block around lines 205–245).
| const runWithSharedEvoluDeps = run.addDeps({ transports }); | ||
| const sharedEvolusByName = new Map<Name, SharedEvolu>(); | ||
| const sharedEvolusMutexByName = stack.use(createMutexByKey<Name>()); | ||
|
|
There was a problem hiding this comment.
Reuse jen podle name ignoruje později dodaný appOwner.
Stačí, aby první CreateEvolu přišel bez appOwner. Další CreateEvolu pro stejné name už znovu použije existující instanci, takže createSharedEvolu(...) neběží a transports.addConsumer(...) se nikdy nespustí. Stejný problém nastane i při změně ownera pro stejné jméno.
Also applies to: 183-214
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/common/src/local-first/Shared.ts` around lines 159 - 162, The cache
keyed only by Name (sharedEvolusByName, sharedEvolusMutexByName) causes
createSharedEvolu and transports.addConsumer to be skipped when the first
CreateEvolu for a name has no appOwner or a different owner; change the caching
strategy so the key includes both name and appOwner (or owner identifier) OR
detect owner changes and re-run createSharedEvolu and transports.addConsumer for
the existing SharedEvolu; update runWithSharedEvoluDeps usage and the lookup
logic around sharedEvolusByName/sharedEvolusMutexByName (and the code block
referenced at 183-214) to use the composite key (name+appOwner) or to patch an
existing instance by invoking createSharedEvolu/transports.addConsumer when
owner differs.
| onDispose: () => { | ||
| void runWithSharedEvoluDeps.daemon( | ||
| sharedEvolusMutexByName.withLock(message.name, () => { | ||
| sharedEvolusByName.delete(message.name); | ||
| return ok(); | ||
| }), | ||
| ); | ||
| }, |
There was a problem hiding this comment.
onDispose může smazat z mapy znovu aktivní instanci a nikdy ji nedisponuje.
Mazání je odložené přes daemon(...) a bez kontroly identity. Když mezi Dispose a tímto callbackem přijde nové CreateEvolu pro stejné name, instance dostane nové porty, ale pozdější onDispose ji stejně odstraní z mapy. Navíc se tu vůbec nevolá sharedEvolu[Symbol.asyncDispose](), takže zůstanou registrované transporty, timeouty a callbacky.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/common/src/local-first/Shared.ts` around lines 195 - 202, Callback
onDispose currently schedules a delete via runWithSharedEvoluDeps.daemon +
sharedEvolusMutexByName.withLock but doesn't verify the instance identity or
call the instance's async disposer, so a newly created instance with the same
name can be removed and transports/timeouts remain registered; change the
onDispose handler to (1) capture the specific sharedEvolu instance at creation
time, (2) inside the mutexed function compare
sharedEvolusByName.get(message.name) === capturedInstance before deleting, and
(3) if deleting, invoke await capturedInstance[Symbol.asyncDispose]() (or call
its async dispose method) to unregister transports/timeouts before removing the
map entry (references: onDispose, runWithSharedEvoluDeps.daemon,
sharedEvolusMutexByName.withLock, sharedEvolusByName, CreateEvolu, sharedEvolu,
Symbol.asyncDispose).
| const isActiveQueueItemDisposed = | ||
| isNonEmptyArray(queue) && queue[0].evoluPortId === evoluPortId; | ||
|
|
||
| evoluPorts.delete(evoluPortId); | ||
| rowsByQueryByEvoluPortId.delete(evoluPortId); | ||
|
|
||
| for (let i = queue.length - 1; i >= 0; i -= 1) { | ||
| if (queue[i].evoluPortId === evoluPortId) { | ||
| queue.splice(i, 1); | ||
| } | ||
| } | ||
|
|
||
| if (isActiveQueueItemDisposed) { | ||
| clearActiveLeaderTimeout(); | ||
| queueProcessingFiber?.abort(); | ||
| queueProcessingFiber = null; | ||
|
|
||
| if (activeQueueCallbackId) { | ||
| callbacks.cancel(activeQueueCallbackId); | ||
| activeQueueCallbackId = null; | ||
| } | ||
|
|
||
| // Require leader reacquire before dispatching remaining queue. | ||
| activeDbWorkerPort = null; | ||
| } | ||
|
|
||
| if (activeDbWorkerPort) ensureQueueProcessing(); | ||
| if (evoluPorts.size === 0) onDispose(); | ||
| ensureQueueProcessing(); | ||
|
|
There was a problem hiding this comment.
Po Dispose může activeDbWorkerPort dál ukazovat na zavřený tab.
Tahle větev maže jen evoluPort a frontu, ale nepáruje odpovídající dbWorkerPort. Pokud zavřený tab držel leadera bez aktivní queue položky, activeDbWorkerPort zůstane nenulový a další requesty se budou posílat do mrtvého portu až do 35s timeoutu. Uložte vazbu evoluPortId -> dbWorkerPort a při dispose ji vždy odstraňte, případně leadera hned shoďte.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/common/src/local-first/Shared.ts` around lines 530 - 558, The
dispose branch currently removes evoluPorts and queue entries but doesn't clear
the corresponding db worker mapping, which can leave activeDbWorkerPort pointing
at a closed tab; add and maintain a mapping (e.g., dbWorkerPorts:
Map<evoluPortId, DbWorkerPort>) and in this dispose block delete
dbWorkerPorts.delete(evoluPortId) and if activeDbWorkerPort refers to that
evoluPortId then immediately drop the leader by setting activeDbWorkerPort =
null and performing the same cleanup you do when isActiveQueueItemDisposed
(clearActiveLeaderTimeout, abort queueProcessingFiber, cancel
activeQueueCallbackId), then continue with evoluPorts.delete,
rowsByQueryByEvoluPortId.delete, queue splices and ensureQueueProcessing so no
requests are sent to a dead port.
| addConsumer: (consumer, resourceConfigs) => async (run) => { | ||
| if (disposing) return ok(); | ||
|
|
||
| const consumerId = getConsumerId(consumer); | ||
|
|
||
| for (const resourceConfig of resourceConfigs) { | ||
| if (disposing) return ok(); | ||
|
|
||
| const resourceId = getResourceId(resourceConfig); | ||
| resourceIdsWithMutex.add(resourceId); | ||
|
|
||
| const result = await run( | ||
| unabortable( | ||
| mutexByResourceId.withLock(resourceId, async () => { | ||
| if (disposing) return ok(); | ||
| clearDisposalTimeout(resourceId); | ||
|
|
||
| let resource = resourcesById.get(resourceId); | ||
| if (!resource) { | ||
| resource = await createResource(resourceConfig); | ||
| resourcesById.set(resourceId, resource); | ||
| } | ||
|
|
||
| let consumerRefCountsByConsumerId = | ||
| consumerRefCountsByResourceId.get(resourceId); | ||
| if (!consumerRefCountsByConsumerId) { | ||
| consumerRefCountsByConsumerId = createRefCount<TConsumerId>(); | ||
| consumerRefCountsByResourceId.set( | ||
| resourceId, | ||
| consumerRefCountsByConsumerId, | ||
| ); | ||
| } | ||
|
|
||
| const nextCount = | ||
| consumerRefCountsByConsumerId.increment(consumerId); | ||
|
|
||
| if (nextCount === 1) { | ||
| consumerIdsByResourceId.add(resourceId, consumerId); | ||
| } | ||
|
|
||
| return ok(); | ||
| }), | ||
| ), | ||
| ); | ||
| if (!result.ok) return result; | ||
| } | ||
|
|
||
| return ok(); | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Ověření, zda existují testy pokrývající selhání createResource v async path
rg -n "addConsumer.*fail|createResource.*throw|createResource.*error" packages/common/test/Repository: SQLoot/evolu-plan-b
Length of output: 180
🏁 Script executed:
cat -n packages/common/test/Resources.test.ts | sed -n '80,120p'Repository: SQLoot/evolu-plan-b
Length of output: 1585
🏁 Script executed:
cat -n packages/common/src/Resources.ts | sed -n '245,310p'Repository: SQLoot/evolu-plan-b
Length of output: 2606
🏁 Script executed:
cat -n packages/common/test/Resources.test.ts | sed -n '85,150p'Repository: SQLoot/evolu-plan-b
Length of output: 2674
🏁 Script executed:
# Search for any rollback or cleanup logic in the addConsumer implementation
rg -n "rollback|cleanup|restore" packages/common/src/Resources.tsRepository: SQLoot/evolu-plan-b
Length of output: 305
🏁 Script executed:
cat -n packages/common/src/Resources.ts | sed -n '477,540p'Repository: SQLoot/evolu-plan-b
Length of output: 2464
🏁 Script executed:
# Check how rollbackAddConsumer is called in the context of addConsumer errors
cat -n packages/common/src/Resources.ts | sed -n '249,300p'Repository: SQLoot/evolu-plan-b
Length of output: 2095
🏁 Script executed:
# Check the structure of createResources - find where rollbackAddConsumer is called and how it relates to addConsumer
cat -n packages/common/src/Resources.ts | sed -n '600,650p'Repository: SQLoot/evolu-plan-b
Length of output: 2048
🏁 Script executed:
# Look for createResources function definition and its return type to understand the structure
cat -n packages/common/src/Resources.ts | sed -n '1,50p'Repository: SQLoot/evolu-plan-b
Length of output: 1780
🏁 Script executed:
# Find where the synchronous addConsumer is defined
rg -n "addConsumer.*consumer.*resourceConfigs.*=>.*\{" packages/common/src/Resources.ts | head -5Repository: SQLoot/evolu-plan-b
Length of output: 185
🏁 Script executed:
# Get a broader context around the rollback to understand the full implementation
cat -n packages/common/src/Resources.ts | sed -n '550,620p'Repository: SQLoot/evolu-plan-b
Length of output: 2811
🏁 Script executed:
# Check what createResources returns and understand the relationship between the two versions
cat -n packages/common/src/Resources.ts | sed -n '100,180p'Repository: SQLoot/evolu-plan-b
Length of output: 2842
🏁 Script executed:
# Search for where the async addConsumer (line 249) is used/called
rg -n "mutexByResourceId\|unabortable" packages/common/src/Resources.ts | head -20Repository: SQLoot/evolu-plan-b
Length of output: 45
🏁 Script executed:
# Verify the relationship: are there two versions (async and legacy)? Let's look at the export/API
rg -n "export.*createAsyncResources\|export.*createLegacyResources\|export.*createResources" packages/common/src/Resources.tsRepository: SQLoot/evolu-plan-b
Length of output: 45
🏁 Script executed:
# Check the entire structure: are both functions exported as createResources?
cat -n packages/common/src/Resources.ts | tail -100 | head -50Repository: SQLoot/evolu-plan-b
Length of output: 1347
🏁 Script executed:
# Look at the complete async version implementation (around line 249 and beyond)
cat -n packages/common/src/Resources.ts | sed -n '168,250p'Repository: SQLoot/evolu-plan-b
Length of output: 3045
🏁 Script executed:
# Check if there's error handling or try-catch around the addConsumer async version
cat -n packages/common/src/Resources.ts | sed -n '240,340p'Repository: SQLoot/evolu-plan-b
Length of output: 4129
🏁 Script executed:
# Check the test file to see which createResources version it uses
cat -n packages/common/test/Resources.test.ts | sed -n '1,100p'Repository: SQLoot/evolu-plan-b
Length of output: 3503
🏁 Script executed:
# Look for any test for the async Resources version (the one at line 249)
rg -n "createAsyncResources\|Resources<" packages/common/test/Resources.test.ts | head -20Repository: SQLoot/evolu-plan-b
Length of output: 45
Async verze addConsumer postrádá rollback při selhání createResource.
Pokud createResource(resourceConfig) na řádku 268 selže během iterace zdroje N, zdroje úspěšně přidané v předchozích iteracích (1 až N-1) zůstanou v resourcesById bez rollbacku. Na rozdíl od synchronní createLegacyResources (která implementuje úplný rollback v try-catch bloku na řádcích 580-617), async verze chybí ochrana pro částečné selhání.
| export const testCreateWebSocket = | ||
| ({ | ||
| throwOnCreate = false, | ||
| }: TestCreateWebSocketOptions = {}): CreateWebSocket => | ||
| () => | ||
| async () => { | ||
| if (throwOnCreate) { | ||
| throw new Error("testCreateWebSocket: throwOnCreate"); | ||
| } | ||
|
|
||
| return ok({ | ||
| send: () => ok(), | ||
| getReadyState: () => "open", | ||
| isOpen: () => true, | ||
| [Symbol.dispose]: () => {}, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Testovací helper po dispose pořád simuluje otevřený socket.
Po zavolání [Symbol.dispose]() tenhle fake stále vrací isOpen() === true, getReadyState() === "open" a send() dál uspěje. Testy nad cleanup cestami tak můžou projít, i když by reálná implementace už byla zavřená.
💡 Návrh úpravy
export const testCreateWebSocket =
({
throwOnCreate = false,
}: TestCreateWebSocketOptions = {}): CreateWebSocket =>
() =>
async () => {
if (throwOnCreate) {
throw new Error("testCreateWebSocket: throwOnCreate");
}
+ let disposed = false;
+
return ok({
- send: () => ok(),
- getReadyState: () => "open",
- isOpen: () => true,
- [Symbol.dispose]: () => {},
+ send: () => (disposed ? err({ type: "WebSocketSendError" }) : ok()),
+ getReadyState: () => (disposed ? "closed" : "open"),
+ isOpen: () => !disposed,
+ [Symbol.dispose]: () => {
+ disposed = true;
+ },
});
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/common/src/WebSocket.ts` around lines 321 - 337, The
testCreateWebSocket fake currently keeps reporting an open socket even after
[Symbol.dispose] is called; modify testCreateWebSocket to maintain an internal
"closed" flag set when [Symbol.dispose]() runs and make send, isOpen and
getReadyState consult that flag: after disposal isOpen() should return false,
getReadyState() should return "closed", and send() should fail (return an
error/result indicating closed) instead of succeeding. Update the returned
object in testCreateWebSocket to use this internal state so cleanup-path tests
behave like a real closed socket.
There was a problem hiding this comment.
♻️ Duplicate comments (5)
packages/common/src/local-first/Relay.ts (2)
283-300:⚠️ Potential issue | 🔴 Critical
deleteOwnermusí běžet pod stejnýmmutexByOwnerIdjako zápisy.
writeMessagesje teď serializované perownerId, aledeleteOwnerten samý lock obchází. ParalelnídeleteOwner(ownerId)awriteMessages(ownerId, ...)tak pořád můžou smazaného ownera hned částečně obnovit, včetně usage metadat.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/common/src/local-first/Relay.ts` around lines 283 - 300, deleteOwner currently runs its DB transaction without acquiring the per-owner lock used by writeMessages; wrap the entire deletion logic (the deps.sqlite.transaction callback and the call to sqliteStorageBase.deleteOwner) in the same mutex acquisition used by writeMessages (mutexByOwnerId for the given ownerId) so deleteOwner(ownerId) is serialized with writeMessages(ownerId,...). Acquire mutexByOwnerId(ownerId) before starting the transaction and release it after sqliteStorageBase.deleteOwner completes to prevent concurrent writeMessages from recreating data.
184-190:⚠️ Potential issue | 🟠 MajorDeduplicitní
timestampodfiltrujte ještě před výpočtem kvóty.
existingTimestampsSetpokrývá jen hodnoty už uložené v DB. Když jeden batch obsahuje stejnýtimestampvíckrát, všechny duplicitní položky projdou donewMessages, takže se na Lines 205-211 započítají bajty 2× a na Lines 248-253 se navýší usage, i kdyžinsert ... on conflict do nothinguloží jen jednu zprávu. To umí vrátit falešnýStorageQuotaError.Možná oprava
- const existingTimestampsSet = new Set( + const seenTimestamps = new Set( existingTimestampsResult.map((t) => t.toString()), ); const newMessages = filterArray( messagesWithTimestampBytes, - (m) => !existingTimestampsSet.has(m.timestamp.toString()), + (m) => { + const timestamp = m.timestamp.toString(); + if (seenTimestamps.has(timestamp)) return false; + seenTimestamps.add(timestamp); + return true; + }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/common/src/local-first/Relay.ts` around lines 184 - 190, Batch-internal duplicate timestamps are not being removed before quota calculation, causing bytes and usage to be double-counted; deduplicate messagesWithTimestampBytes by timestamp.toString() first (e.g., produce a batchUnique list/Set keyed by m.timestamp.toString() that keeps the first occurrence) and then compute existingTimestampsSet and filter against it to produce newMessages; ensure you reference the same deduplicated collection when calculating bytes and incrementing usage (symbols: messagesWithTimestampBytes, existingTimestampsSet, newMessages, filterArray, StorageQuotaError) so the quota only accounts for unique timestamps in the incoming batch.packages/common/src/local-first/Shared.ts (3)
159-161:⚠️ Potential issue | 🟠 MajorKlíč cache musí zahrnout i
appOwner.Instance se tady znovu používá jen podle
name, takže prvníCreateEvolubez ownera nebo s jiným ownerem zablokuje pozdější inicializaci pro stejnýname. V takovém případě secreateSharedEvolu(...)znovu nespustí atransports.addConsumer(...)pro nového ownera vůbec neproběhne.Also applies to: 183-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/common/src/local-first/Shared.ts` around lines 159 - 161, The cache/mutex key currently uses only Name causing different appOwner instances to collide; update sharedEvolusByName and sharedEvolusMutexByName to use a composite key including appOwner (e.g. { name, appOwner } or a stringified composite) and change the createMutexByKey generic to that composite type so createSharedEvolu(...) and transports.addConsumer(...) are keyed per owner; apply the same composite-key change for the other occurrences in the createSharedEvolu block (around the 183-225 region) so each owner gets its own mutex and map entry.
195-215:⚠️ Potential issue | 🟠 Major
onDisposemůže zlikvidovat nově vytvořenou instanci.Callback z mapy znovu načítá hodnotu jen podle
message.namea bez kontroly identity ji disposne i smaže. Pokud mezitím vznikne nová instance pro stejnéname, odstraní se omylem ta nová místo původní.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/common/src/local-first/Shared.ts` around lines 195 - 215, The onDispose callback currently looks up sharedEvolusByName.get(message.name) and disposes whatever is found, which can accidentally dispose a newly-created instance; fix by closing over the specific instance created (e.g., capture const created = maybeSharedEvolu when inserting) and inside runWithSharedEvoluDeps.daemon/sharedEvolusMutexByName.withLock only call created[Symbol.asyncDispose]() and delete from sharedEvolusByName if sharedEvolusByName.get(message.name) === created to ensure identity match (use the same maybeSharedEvolu reference) before disposing and removing it.
332-345:⚠️ Potential issue | 🟠 MajorPo
Disposemůže leader dál ukazovat na mrtvýdbWorkerPort.Chybí vazba mezi
evoluPortIda jehodbWorkerPort, takže dispose vyčistí jen frontu aevoluPort. Když zavřený tab držel leadera a zrovna nebyl na čele fronty, další requesty se budou posílat do zavřeného portu až do timeoutu heartbeat.Also applies to: 543-570
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/common/src/local-first/Relay.ts`:
- Around line 283-300: deleteOwner currently runs its DB transaction without
acquiring the per-owner lock used by writeMessages; wrap the entire deletion
logic (the deps.sqlite.transaction callback and the call to
sqliteStorageBase.deleteOwner) in the same mutex acquisition used by
writeMessages (mutexByOwnerId for the given ownerId) so deleteOwner(ownerId) is
serialized with writeMessages(ownerId,...). Acquire mutexByOwnerId(ownerId)
before starting the transaction and release it after
sqliteStorageBase.deleteOwner completes to prevent concurrent writeMessages from
recreating data.
- Around line 184-190: Batch-internal duplicate timestamps are not being removed
before quota calculation, causing bytes and usage to be double-counted;
deduplicate messagesWithTimestampBytes by timestamp.toString() first (e.g.,
produce a batchUnique list/Set keyed by m.timestamp.toString() that keeps the
first occurrence) and then compute existingTimestampsSet and filter against it
to produce newMessages; ensure you reference the same deduplicated collection
when calculating bytes and incrementing usage (symbols:
messagesWithTimestampBytes, existingTimestampsSet, newMessages, filterArray,
StorageQuotaError) so the quota only accounts for unique timestamps in the
incoming batch.
In `@packages/common/src/local-first/Shared.ts`:
- Around line 159-161: The cache/mutex key currently uses only Name causing
different appOwner instances to collide; update sharedEvolusByName and
sharedEvolusMutexByName to use a composite key including appOwner (e.g. { name,
appOwner } or a stringified composite) and change the createMutexByKey generic
to that composite type so createSharedEvolu(...) and transports.addConsumer(...)
are keyed per owner; apply the same composite-key change for the other
occurrences in the createSharedEvolu block (around the 183-225 region) so each
owner gets its own mutex and map entry.
- Around line 195-215: The onDispose callback currently looks up
sharedEvolusByName.get(message.name) and disposes whatever is found, which can
accidentally dispose a newly-created instance; fix by closing over the specific
instance created (e.g., capture const created = maybeSharedEvolu when inserting)
and inside runWithSharedEvoluDeps.daemon/sharedEvolusMutexByName.withLock only
call created[Symbol.asyncDispose]() and delete from sharedEvolusByName if
sharedEvolusByName.get(message.name) === created to ensure identity match (use
the same maybeSharedEvolu reference) before disposing and removing it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3d4936f3-82bb-4d3c-831f-36b6a91c99fa
📒 Files selected for processing (3)
packages/common/src/local-first/Relay.tspackages/common/src/local-first/Shared.tspackages/vue/src/provideEvolu.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/vue/src/provideEvolu.ts
This pull request includes several dependency updates across the project, a refactor of the
Refutility to provide a richer and more consistent API for mutable references, and documentation improvements for theResultmodule. Additionally, the unusedInstancesmultiton utility has been removed. The most significant changes are grouped below.Refactor and API Improvements
Refutility inpackages/common/src/Ref.tsto provide a more expressive and consistent API for mutable references, including new methods likegetAndSet,setAndGet,update,getAndUpdate,updateAndGet, and a new signature formodify. The documentation was also clarified and simplified. [1] [2]Code and Dependency Cleanup
Instancesmultiton utility frompackages/common/src/Instances.tsto simplify the codebase.packages/common/src/Number.tsto ensure proper use of branding withlessThanOrEqualTo.packages/common/src/Result.ts.Dependency Updates
package.json,packages/common/package.json, and several example apppackage.jsonfiles (Angular, React, Svelte, Vue) to their latest patch or minor versions for improved compatibility and security. [1] [2] [3] [4] [5] [6] [7] [8]Documentation and Style Improvements
packages/common/src/Result.tsto explain when and how to use theResulttype, the intended imperative coding style, and guidance on error handling boundaries. [1] [2] [3] [4] [5] [6]Minor Code Fixes
Evolu.KyselyNotNullinstead of the outdatedEvolu.kysely.NotNullfor type narrowing. [1] [2] [3] [4]getOkforgetOrThrowinapps/relay/src/startBunRelay.tsfor more explicit error handling. [1] [2]Summary by CodeRabbit
Nové funkce
Vylepšení
Změny API