Repository navigation
feat: make read, submit and remove awaitable - #110
Conversation
Use a colon, a comma or a new sentence instead, matching the house style for prose. Mechanical change: 59 lines, no wording changes otherwise. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`const { isSuccess } = await remove({ id })` returned the reactive object
at once, before the request ended, and `isSuccess` was a ComputedRef,
always truthy, so the success branch also ran on failed requests.
The value returned by the three actions is now a PromiseLike, like
VueUse's useFetch. Awaiting it settles on the execution started by the
call and resolves to the plain snapshot `execute()` resolves to. It never
rejects, resolves at once to the current snapshot when the call starts no
execution (`immediate: false`, or `executeWhen` false), and never starts
a request itself, so thenable assimilation cannot run an action twice.
The template refs of the provider components stay non thenable.
Fixes found while making those guarantees hold:
- a read joining an in-flight request with the same params rejected when
that request failed, lost the shared error, and reported an aborted
shared request as not aborted;
- a request that could not be built (a missing path parameter) made
`execute()` reject and left the query idle;
- actions are now cleaned up when their effect scope is disposed, and an
action awaited outside any scope (an event handler) releases its query
once it settles, unless `keepAlive`; such queries used to stay in the
store for ever.
Code that awaited an action and then used its refs now gets plain
values: see the migration note in the CHANGELOG.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 42 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (8)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (18)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. Walkthrough
ChangesAwaitable action results
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant Caller
participant Action as read(), submit(), or remove()
participant Execution as execution handler
participant Request as request construction
participant Awaitable as toAwaitable
Caller->>Action: create action
Action->>Execution: retain initial execution promise
Execution->>Request: construct request
Action->>Awaitable: provide execution and snapshot
Caller->>Awaitable: await action
Awaitable-->>Caller: execution snapshot
Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue is identified. Awaiting actions now returns plain execution snapshots rather than reactive refs, so consumers relying on refs after awaiting must adopt the documented compatibility change. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Overlapping executions created outside an effect scope can release the live query before its request finishes, losing request tracking and the eventual result. The inspected request paths retain their existing authority, but underlying authentication and transport controls were not evaluated. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 44.44% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 10 files. (8 skipped: 8 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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. A rabbit awaits by the clover patch, Comment |
SonarCloud rule S7739 flags any object with a `then` property, which failed the quality gate on reliability. The awaitable is intentional, and the rule exempts a `then` member of a class that declares `implements PromiseLike`: move `then` into such a class so the intent is stated in the code instead of suppressed. `then` stays an own arrow function field, so destructuring, spreading and reactive proxies keep working, and the public types are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Prefer `toHaveLength()` and `toBeUndefined()` over generic `toBe()` assertions for clearer failure reports (SonarCloud S5906). Only the lines added by this branch; existing tests are left as they are. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
S7739 exempts a `then` field declared by a class that implements PromiseLike, but still flagged the `this.then = ...` assignment in the constructor. Initialize the field directly and keep its state in `#` private fields, which stay out of the object the action returns. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|



Problem
The obvious code looks correct and silently is not:
The returned object was not thenable, so
awaitreturned it at once, before the DELETE ended, andisSuccesswas aComputedRef, always truthy. The success branch ran before the request finished, and also when it failed. TypeScript did not flag it. A real app on 2.0.8 had 17 such calls, all showing success alerts on failed deletes.What changes
read(),submit()andremove()return aPromiseLike(like VueUse'suseFetch). Awaiting it settles on the execution started by the call and resolves to the same plain snapshotexecute()resolves to: booleans and values, not refs.await remove(...)types as that snapshot in the emitted.d.ts.execute(),keepAliveand the providers keep working as before.Design decisions
immediate: falseresolves at once to the current snapshot instead of typing that overload as not awaitable. TypeScript acceptsawaiton any value, so a "not awaitable" type would not stop the code, it would bring the bug back (refs, truthyisSuccess).immediatecan also be a runtime boolean, andexecuteWhenfalse at call time is not expressible in types. With the snapshot,isSuccessisfalse, so the wrong usage fails safe. No request is sent. The same rule applies whenexecuteWhenis false at call time.autoExecute, later executions started by a params change do not change the result:await execute()waits for one of those.execute(): a failure resolves withisError: true, an aborted request withaborted: true.thennever starts a request and reuses one promise, soPromise.resolve(),Promise.all()orreturnfrom an async function run the action once. The snapshot is not thenable, and the store state never holds the awaitable (tested). Onlythenis defined, nevercatch: Vue'sisPromiseneeds both, sosetup() { return read() }does not become an async setup. Pinia'swrapActionand vitest spies checkinstanceof Promise, so the object passes through unchanged.Fixes found while making the guarantees hold
read()joining an in-flight request with the same params rejected (unhandled rejection) when that request failed, and its query never showed the error. It now resolves with the error, keeps the shared error, and reports an aborted shared request asaborted: true.RepositoryHttptemplate with a missing path parameter) madeexecute()reject and left the query idle. The query now gets the error.setup()(event handlers, which is exactly the pattern above) were never cleaned up, so their query, hash and items stayed in the store for ever. Cleanup now follows the effect scope (tryOnScopeDispose): components behave as before, andeffectScope()and Pinia setup stores are covered too. An action awaited with no owner releases its query once the awaited execution settles, unlesskeepAlive. A non-awaited ownerless action keeps today's behavior (callcleanup()), so code that keeps its refs alive outside components is not broken.Compatibility note
Released as a minor, but one case changes behavior: code that awaited an action (or returned it from an async function) and then used its refs now gets plain values.
data.valueafter the await becomes a type error; destructuring after a top-levelawaitin<script setup>renders once and no longer updates, with no error. The CHANGELOG ("Changed") and the README "Await" section document the migration: drop theawait, or keep the returned object and await it on its own.Docs
README "Await" section, skill references (
read.md,submit.md,remove.md) andSKILL.md, CHANGELOG entry[2.1.0]. The first commit also replaces the em and en dashes in the skill files (mechanical, 59 lines).Review
An independent read-only review round (a separate model, one round) produced 8 findings, each verified against the code:
@volverjs/data2.0.x reports aread()network error as aborted (onlyread:create,updateandremoverethrow);PromiseLike-only choice;then: it would not protectdefineExpose(read())(property access still finds it), and it would make{ ...read() }silently non-awaitable. Documented thedefineExposecaveat instead.Checks
pnpm test: 8 files, 81/81 passedCI=true pnpm lint: clean (full rule set, not the reduced editor mode)pnpm type-check: cleanpnpm build: clean. A consumer file compiled againstdist/index.d.tsconfirmsawait remove(...)types as the plain snapshot.SonarCloud
thento an object") failed the gate on reliability. The thenable is intentional, and the rule exempts athenmember of a class that declaresimplements PromiseLike, sothennow lives in such a class instead of being suppressed. It is still an own arrow function field, and the public types are unchanged.toHaveLength()andtoBeUndefined(). Existing tests are left as they are.Not in this PR
read()that joined it, or asubmit()/remove()with the same params): tracked in Re-executing a query can abort a request that belongs to another query #111. With this PR the victim at least resolves withaborted: true.expectTypeOfassertions in the tests are checked bypnpm type-checklocally, not in CI.🤖 Generated with Claude Code
Summary by CodeRabbit
read(),submit(), andremove()can now be awaited to get a plain snapshot of the execution, including success, error, or abort status. Awaiting an action that starts no request returns its current snapshot immediately.keepAliveto retain them.