New resource methods - #15091
Conversation
github-actions
Bot
added
APP: Learn
🔵 Review postedLast updated: 2026-07-31 16:31 UTC |
npm Package VersionsWarning The following packages have changed files but no version bump:
If these changes affect published code, consider bumping the version. |
Build Artifacts
Smoke test screenshot |
Relocate the generic `useFetch` reactive read primitive from kolibri-common down into the base kolibri package, so lower-level code (the Resource layer) can build on it without kolibri importing from the unpublished kolibri-common package. - Move `composables/useFetch.js` and its spec into `packages/kolibri`. - Point every existing importer (coach and learn plugins) at the new `kolibri/composables/useFetch` path. - Register the new entry point in the package `exports` map and the generated `internal/apiSpec.js`. Pure relocation - no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QKX2QS51BmgZNJL4EUJU6P
Introduce a smaller, DRF-aligned method surface on the Resource class, alongside the existing Model/Collection methods, so plugins can migrate to it incrementally (first step of learningequality#14777, closes learningequality#15056). - Add a single low-level `request` primitive that every read, write and custom action builds on. The URL is determined entirely by `action` and `routeParams` (object -> named kwargs, array -> positional args); GET uses `params`, writes use `data`, `multipart` is forwarded. - Add REST verbs over it: `retrieve(id)`, `list(params)`, `create(data)`, `update(id, data)`, `delete(id)`, `bulkCreate(data)`, `bulkDelete(params)`. Only `request` takes `action`/`routeParams`; the verbs target the default detail/list routes, keeping signatures close to the old ones. `list` passes the server shape through (array, or `{ results, more, count }` when paginated). - De-duplicate concurrent identical GETs: they share one in-flight request keyed by resolved URL + order-insensitive query params (via qs), cleared on settle including errors. Writes are never coalesced. - `update` diffs against a write-only baseline store of each object's last server representation and PATCHes only changed fields; with no baseline it PATCHes the given fields as-is. The baseline is populated from retrieve/list/create/update and kept coherent when the legacy Model fetch/save paths run. - Add `useRetrieve` / `useList` as Resource methods, thin constructors over useFetch; `useList` supports pagination out of the box. useFetch now tolerates both list shapes. TaskResource's startTask/startTasks now build on create/bulkCreate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QKX2QS51BmgZNJL4EUJU6P
3e2c876 to
6847c44
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15091 — the seven acceptance criteria from #15056 are all implemented, and the new test suite is genuinely thorough (the coalescing block in particular pins negative cases, not just the happy path).
Note on scope: the review passes ran against 3e2c8762, and the head has since advanced to 6847c440. The delta is exactly the fix for what was the one blocking finding — the three runtime imports of the relocated useFetch used a .js extension that packages/kolibri's explicit exports map does not declare, which broke the Coach and Learn webpack bundles (Jest's resolver falls back to legacy resolution, so CI never saw it). That is resolved in 4249fc28/6847c440; nothing else changed, but the newest commits were not otherwise reviewed.
CI green. Manual QA ran against a live dev server (with that import fix applied locally): resource selection, fetchMore pagination (101 → 151 cards), TaskResource.startTask, RTL, and a 412×915 viewport all behaved correctly, with no PR-attributable console or axe-core findings.
Remaining findings, all inline:
- important —
create()'s signature change is a silent break for out-of-tree consumers of the publishedkolibripackage (apiResource.js:1088) - important —
_baselinesgrows without bound fromlist()and is not reset byclearCache()(apiResource.js:1075) - important —
update()can skip the request entirely against a stale baseline (apiResource.js:1120) - suggestion — coalesced GETs hand every caller the same mutable response,
requestthrows synchronously where its siblings reject (apiResource.js:1026) - suggestion —
useRetrieve/useListaccept refs but never watch them (apiResource.js:1208) - suggestion — the
useFetchshape change has no test inuseFetch's own suite (useFetch.js:59) - nitpick — composable specs mock sibling methods rather than the
clientboundary;bulkDeleteleaves baselines behind
Worth noting that retrieve, update, delete, bulkDelete, useList and useRetrieve have zero call sites in this PR, so none of them are exercised outside unit tests — the first adopter will be the first real-world test of the baseline-diff path in particular.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran a phased review pipeline over the pull request diff:
- Classified the diff to select review passes (core, frontend, backend) and whether manual QA was required
- Core review pass checked correctness, design, architecture, testing, completeness, and DRY/SRP/Rule-of-Three principles
- Specialized frontend/backend review passes applied framework-specific lenses where those files changed
- For UI changes: manual QA and an accessibility audit against a live dev server, when available
- Checked CI status and linked issue acceptance criteria
- Synthesized one review from those passes and chose the verdict from the findings, CI status, and QA evidence
| * data | ||
| * @returns {Promise} - Promise that resolves with the created object | ||
| */ | ||
| async create(data, { multipart = false } = {}) { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
important: The signature moved from create(params = {}, multipart = false) to create(data, { multipart = false } = {}). A caller written against the old form — resource.create(payload, true) — still runs: { multipart = false } = true destructures a boolean, multipart falls back to false, and the request goes out as JSON. No error, no warning; it surfaces server-side as a bad payload.
All in-tree callers are fine (TaskResource.startTask is updated here; ContentRequestResource.create(data) and PinnedDeviceResource.create({ instance_id }) pass no second argument). But packages/kolibri is published to npm and consumed out-of-tree, and #15056 is explicit — "Out of Scope: Removing or renaming any existing methods" and "Existing methods are unchanged and keep working."
Either accept the legacy positional boolean for one release (typeof opts === 'boolean' → { multipart: opts }, with a deprecation warning), or at minimum throw on a non-object second argument so the break is loud. list kept its signature and return shape, so no concern there.
Two smaller behaviour changes in the same area worth a sanity check: create() with no arguments now sends data: undefined rather than {}, and startTasks now routes through bulkCreate, which throws TypeError on a non-array where the old create would have POSTed the object.
| const response = await this.request({ params }); | ||
| const results = Array.isArray(response.data) ? response.data : response.data?.results; | ||
| if (Array.isArray(results)) { | ||
| results.forEach(object => this.__setBaseline(object)); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
important: list() deep-clones and stores every object it ever returns, for every resource, whether or not that resource is ever written to. Nothing bounds or evicts _baselines, and clearCache() (line 1288, unchanged) resets only models and collections.
The clearest live case is task polling: useTaskPolling (packages/kolibri-common/composables/useTaskPolling.js:17) hits TaskResource.list({ queue }) every 5 seconds. Job ids are unique per enqueued job, so a content import that enqueues hundreds of jobs leaves hundreds of cloned job objects behind for the lifetime of the SPA — and TaskResource.clear/cancel/restart go through postDetailEndpoint, not Resource.delete, so the this._baselines.delete(...) on line 1147 never fires for them. Resource instances are module singletons. On the low-memory Android hardware Kolibri targets, that is a per-tick clone plus permanent retention paid for a payload optimisation (update's field diff) that TaskResource never uses.
Cheapest fix is to record baselines only where an update plausibly follows — retrieve/create/update — and skip list, which is where the cost is worst and the payoff least likely. Either way, clear _baselines and _inFlight in clearCache() so the existing reset escape hatch actually resets everything the resource holds. Ordering trap if you do: the constructor calls this.clearCache() on line 607, before _inFlight/_baselines are assigned on 610/613 — those assignments need to move above the call.
| } | ||
| if (!Object.keys(payload).length) { | ||
| // Nothing changed, so there is nothing to send. | ||
| return cloneDeep(baseline); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
important: #15056 asks that update send only the changed fields; skipping the request entirely when the diff is empty is an extra step, and that is where the risk lives.
This PR is the first stage of a migration, so old and new layers coexist:
resource.retrieve(id)→ baseline{ name: 'A' }.- Something else writes
name: 'B'— a legacyresource.saveModel({ id, data: { name: 'B' } }), aModel.save(), another tab, another user. None of those paths touch_baselines. resource.update(id, { name: 'A' })→ empty diff → no PATCH is sent, and the promise resolves with the stale{ name: 'A' }.
The caller's write is dropped and the resolved value contradicts the server. Sending the empty-diff PATCH costs one round trip and keeps update truthful; alternatively have saveModel/Model.save refresh the baseline so the two layers stay coherent while both exist.
| * data | ||
| * @returns {Promise} - Promise that resolves with the full response object | ||
| */ | ||
| request({ method = 'GET', action = 'list', routeParams, params, data, multipart = false } = {}) { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: Two things about request as the designated primitive.
Shared response object. Coalesced callers all receive the identical response, and therefore the identical response.data array and element objects; previously two concurrent requests produced two independent payloads. retrieve/list return response.data directly rather than a clone, so nothing insulates them — through useList, two components fetching the same collection concurrently end up with data.value referencing the same array, and an in-place sort or a UI flag set on an item in one silently mutates the other's rendered data. Documenting on request/list/retrieve that returned data is shared and must be treated as immutable would be enough. Related: there is no way to force a fresh read, so create(...) immediately followed by list() can attach to a poll's in-flight list() issued before the write and quietly return a response missing the new object. A force escape hatch would cover it.
Sync throw. __resolveUrl throws before any await and request is not async, so resource.request({ action: 'missing' }) throws synchronously — as the spec asserts. Every other new method is async and surfaces the same class of argument error as a rejection. Since request is documented @returns {Promise}, a caller writing .catch(handle) gets an uncaught throw instead. Marking request async makes the contract uniform for the one method everything else builds on.
| * through to `retrieve`, i.e. `{ params }`. | ||
| * @returns {import('kolibri/composables/useFetch').FetchObject} The fetch state and actions. | ||
| */ | ||
| useRetrieve(id, options) { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: toValue inside the fetchMethod closure is deferred reading, not reactivity — nothing watches id or params. The JSDoc is honest about it, and the examples show the consequence: every call site hand-writes onMounted(() => fetchData()) plus a watch. That is exactly the boilerplate a composable should absorb, and hand-written refetch watchers are where stale-data bugs live (miss a dependency in the watch source and the panel shows the previous facility's users). A caller who passes a computed to something named use* will reasonably expect it to track.
Consider an opt-in { immediate: true } and/or a watch over the reactive inputs so useList(computed(() => ({ member_of: facilityId.value }))) refetches on its own. If auto-watching is deliberately out of scope, say so explicitly in the JSDoc rather than only describing read timing.
Smaller, same place: toValue(options) unwraps only the outer value, so useRetrieve(id, { params: someComputed }) passes the ref itself through to retrieve and into the query string. useList has no such asymmetry because params is the top-level argument. Either unwrap params too or document that only the top level may be reactive.
| // A list endpoint returns a plain array when it is not paginated, and a | ||
| // `{ results, more, count }` object when it is. Handle both, so that a fetchMoreMethod | ||
| // can be supplied without knowing up front which shape will come back. | ||
| const responseData = fetchMoreMethod && !Array.isArray(response) ? response.results : response; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: This array/paginated tolerance is the one functional change in the move, and packages/kolibri/composables/__tests__/useFetch.spec.js was only re-pointed at the new import path — no case was added. It is covered transitively by the Resource.useList tests over in api-resource.spec.js, but behaviour that lives here should be pinned by this file's suite: a case where fetchMoreMethod is supplied and fetchMethod resolves a plain array.
While adding it, worth pinning the count contract too — line 71 leaves count.value at null for the array shape even though the length is known, so useList against an unpaginated endpoint gives you data.value.length items and count.value === null. Defensible, but consumers will trip over it and there is currently no assertion either way.
|
|
||
| beforeEach(() => { | ||
| resource = new Resources.Resource({ name: 'test' }); | ||
| resource.retrieve = jest.fn().mockResolvedValue({ id: 'abc', name: 'test' }); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: The Resource REST methods block mocks resource.client, which is the actual hard boundary; these two blocks mock retrieve/list — sibling methods on the class under test. That verifies useRetrieve forwards to retrieve and that useFetch reshapes whatever retrieve was told to return, but it cannot catch a mismatch between what list() really returns and what useFetch expects, which is precisely the question the useFetch.js:59 change exists to answer. It also couples the specs to the call shape (toHaveBeenCalledWith('abc', undefined) breaks on a refactor to retrieve(id, {}) with no behaviour change). Driving these through the mocked client like the sibling block tests both halves together.
| * @throws {TypeError} - When no query parameters are given, to prevent an unfiltered | ||
| * bulk delete | ||
| */ | ||
| async bulkDelete(params = {}) { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: delete clears the baseline for the object it removed (line 1147); bulkDelete drops the objects server-side but leaves their baselines in the map, so a later retrieve of a recreated object would diff against a baseline for a deleted row. Low impact given bulkDelete is params-driven and the ids aren't known client-side — but worth a comment noting the deliberate gap if it isn't going to be handled.
| } | ||
| // Clear the entry once the request settles, whether it succeeded or failed - a lingering | ||
| // rejected promise would poison every subsequent request for this key. | ||
| const promise = this.client({ url, method, params }).finally(() => { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: The rejected-promise-poisoning failure mode is the one people get wrong when hand-rolling request coalescing, and it is both handled and explained. The stableSerialize key closes the matching key-ordering hole, and both are covered — including should fire a new request once the previous one has failed.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15091 — 1 of 9 prior findings resolved (the useFetch .js extension, which was the only blocking one); 8 still open. HEAD has not moved since the last review, so this pass re-verified the 3e2c8762 → 6847c440 delta (exactly the 11 import-path rewrites, nothing else) and added a manual-QA pass in the running app.
QA found no PR-attributable regressions: lesson/quiz resource selection, coach course assignment, CourseUnitView, and both migrated TaskResource signatures (including multipart forwarding) all behave as before; RTL and responsive check out; axe-core found no new violations. QA also measured three of the open findings live — 3 concurrent list() calls produced 1 request and one shared data object; TaskResource._baselines reached 6 entries after one list() and survived clearCache(); a no-op update() issued 0 requests. Those measurements are folded into the relevant threads below.
Still open (details inline): the create() signature break, unbounded _baselines, the no-op update() fast path, the shared mutable GET response, un-watched refs in useRetrieve/useList, and three test/nitpick items.
One note that maps to no changed line: this branch moves useFetch out of packages/kolibri-common/composables/, which docs/frontend_architecture/composables.rst:39-44 still names as the location for shared composables, and its "Resource fetching" example (lines 182-208) is exactly what useList/useRetrieve now replace. A line in that doc for kolibri/composables vs kolibri-common/composables, plus a mention of the new methods in docs/dataflow/index.rst, would stop the docs steering the next author away from this work.
Prior-finding status
RESOLVED — packages/kolibri/composables/useFetch.js — useFetch imports used a .js extension not in the exports map
UNADDRESSED — packages/kolibri/apiResource.js:1088 — create() signature change is a silent break for out-of-tree consumers
UNADDRESSED — packages/kolibri/apiResource.js:1075 — _baselines grows without bound and is not reset by clearCache()
UNADDRESSED — packages/kolibri/apiResource.js:1120 — update() can skip the request entirely against a stale baseline
UNADDRESSED — packages/kolibri/apiResource.js:1026 — coalesced GETs hand every caller the same mutable response; request throws synchronously
UNADDRESSED — packages/kolibri/apiResource.js:1208 — useRetrieve/useList accept refs but never watch them
UNADDRESSED — packages/kolibri/composables/useFetch.js:59 — the array/paginated tolerance has no test in useFetch's own suite
UNADDRESSED — packages/kolibri/tests/api-resource.spec.js:1736 — composable specs mock sibling methods rather than the client boundary
UNADDRESSED — packages/kolibri/apiResource.js:1180 — bulkDelete leaves baselines behind
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Ran the same phased review passes as a first review (core, frontend/backend lenses, manual QA when required)
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| * data | ||
| * @returns {Promise} - Promise that resolves with the created object | ||
| */ | ||
| async create(data, { multipart = false } = {}) { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
important (still open): create(params = {}, multipart = false) → create(data, { multipart }) is a silent break for any out-of-tree caller (custom plugins, KDP) — an old create(data, true) call now passes true where an options object is expected and silently loses multipart. Either accept both shapes for a release, or call the break out in the PR body/release notes so downstream consumers know to look.
| const response = await this.request({ params }); | ||
| const results = Array.isArray(response.data) ? response.data : response.data?.results; | ||
| if (Array.isArray(results)) { | ||
| results.forEach(object => this.__setBaseline(object)); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
important (still open): every object from every list() is cloneDeepd into _baselines, which is a per-singleton Map that only delete() ever removes from — and clearCache() (line 1288) resets only models/collections, so it does not clear it either.
Measured live this pass: one TaskResource.list({}) put 6 entries in _baselines, and clearCache() left all 6. TaskResource has no update() call site anywhere, so on the Device > Tasks page — polled every 5s by useTaskPolling (packages/kolibri-common/composables/useTaskPolling.js:17), plus facilityTasksQueue and the content-task poller — this is a monotonically growing map of pure dead weight on Kolibri's low-end target hardware.
Since the baseline is documented as "a payload optimization, never a correctness requirement", it can be bounded with no semantic loss: record baselines only in retrieve/create (the single-object reads an update realistically follows), or cap the map. Clearing _baselines in clearCache() is worth doing regardless — clearCache() is the codebase's "forget what you know about the server" hook, and leaving baselines behind means a later update() diffs against a representation the caller has already declared invalid.
There was a problem hiding this comment.
clearCache will be removed anyways, I think the best here will be just to store the baselines comming from retrieve and create, what do you think? @rtibbles.
There was a problem hiding this comment.
Yes, that works — retrieve/create only is the option I would pick too. It bounds the map to objects the caller actually holds a single-object handle on, which is the realistic precondition for an update(), and it costs nothing semantically: an update() after a list()-only fetch just falls back to sending the full payload, which is the current behaviour anyway.
And if clearCache is going away, the stale-baseline-after-clear concern goes with it.
There was a problem hiding this comment.
The other option is to move the baseline for comparison into the composable layer, inside function scope - so that they get cleared on component destroy, rather than persisting here - that gives a natural lifecycle to them that is sufficiently long lasting for our needs, but that we don't have to add extra book keeping for.
There was a problem hiding this comment.
That resolves it more cleanly than bounding the map — component lifecycle is real bookkeeping that already exists, and it also scopes the baseline to the caller that fetched it rather than sharing one across every consumer of the singleton. Deferring to your call on which route to take; either one closes this finding.
There was a problem hiding this comment.
Right — no composable needed. An optional baseline on update is the smaller version of the same idea and lands in the same place: async update(id, data = {}, { params, baseline } = {}), use it when given, send data as-is when not. Then list/retrieve/create stop writing to _baselines and the map goes away entirely — that's a smaller diff than what @rtibbles and I were sketching, not a bigger one. Callers that want the diff already hold the object they rendered the form from, so passing it back is a one-line change at each of those sites.
On maxAge + gc: I'd skip it. It keeps the map and adds eviction on top, and it makes whether the diff happens depend on wall-clock timing — a slow user on the same form gets a full-payload PATCH, a fast one gets a diff. Same bytes on the wire either way (the docstring is right that this is only an optimization), but it's harder to reason about than an explicit argument. The baseline param has no timing behaviour to get wrong.
Either way this is a nice-to-have, not a blocker on the PR.
There was a problem hiding this comment.
Well, I guess the composable could be used to store the few lines that will require storing an initial snapshot. But the diff logic should be kept in the update method.
that's a smaller diff than what @rtibbles and I were sketching, not a bigger one
That will increase the refactor needed (and potential bugs) across the codebase, not talking about the refactor on the Resource class 😅.
There was a problem hiding this comment.
Agreed — that is the same shape: the composable holds the snapshot and hands it to update, update still owns the diffing. Nothing about the diff logic moves out of the resource. Happy to leave the exact split to you; this is a nice-to-have either way.
There was a problem hiding this comment.
So we keep diff checking in the update method, but the baseline storage goes in the composable? Works for me.
There was a problem hiding this comment.
@rtibbles yes, that's the split — the composable (or the caller) holds the snapshot and passes it in; update still does the diffing. No change to what update owns.
@AlexVelezLl that's a fair flag, and it's your call. This was a nice-to-have, not a blocker: the _baselines growth is bounded dead weight, not a correctness problem, and if the per-call-site migration cost outweighs that on the flows you're looking at, keeping the storage in the resource is a legitimate answer. Not pressing it further either way.
| } | ||
| if (!Object.keys(payload).length) { | ||
| // Nothing changed, so there is nothing to send. | ||
| return cloneDeep(baseline); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
important (still open): #15056 asks that update send only the changed fields; skipping the request entirely is a stronger behaviour than that. Measured live: update(id, { name: obj.name }) issued 0 requests and resolved with a copy of the baseline, reporting success without contacting the server. If a sync or another coach changed the object since the baseline was captured, the caller gets stale data and believes it wrote.
One further consequence found this pass: the early return resolves with cloneDeep(baseline), so the resolved shape depends on where the baseline came from. A baseline recorded by list() carries the list endpoint's fields; a real PATCH resolves with the detail endpoint's representation. Several viewsets in kolibri/core/content/api.py override retrieve to return a materially different payload from list, so a caller can get two different object shapes from update() depending only on whether the diff happened to be empty.
Sending the PATCH even when the diff is empty fixes both — the diff stays a pure payload optimization, as the docstring says.
There was a problem hiding this comment.
Two things: For the first one, this was agreed to accept these edge cases of having other sessions edit the same thing (also, this is the same behavior we currently have for saveModel. For the second one, in theory, viewsets list's methods return the same as retrieve methods, so that should be fine.
There was a problem hiding this comment.
Understood — if the concurrent-edit staleness is an accepted trade-off (and matches existing saveModel behaviour), that settles it. Withdrawing this finding.
On the shape point, I was extrapolating from retrieve overrides rather than verifying they diverge from list for the resources this touches, so I have no evidence against your read. Withdrawing that too. Sorry for the noise.
| * data | ||
| * @returns {Promise} - Promise that resolves with the full response object | ||
| */ | ||
| request({ method = 'GET', action = 'list', routeParams, params, data, multipart = false } = {}) { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion (still open): two things about request as the designated primitive.
- Coalesced GETs hand every attached caller the same promise, hence the same
response.datareference. Measured: 3 concurrentlist({available:true})→ 1 network request,a === b === c. Before this PR each caller parsed its own body. Any caller that mutates in place — an in-placesort/splice, or simply assigning into a Vue 2ref, which reactifies the array in place — now affects every concurrent peer;useChannels.fetchChannelsdoes the latter (packages/kolibri-common/composables/useChannels.js:20). Nothing is broken today (I checked the currentlist()call sites), but the ownership change is invisible from the call site and unit tests won't catch a future regression. Either shallow-copydataper attached caller, or document the aliasing as part of the contract. requestisasync-less, so a badactionrejects synchronously via__resolveUrlwhile every sibling method (retrieve,update, …) returns a rejected promise. QA confirmedrequest({action:'nope'})throws rather than rejecting. Makingrequestasyncwould make the primitive's failure mode uniform with its wrappers.
| * through to `retrieve`, i.e. `{ params }`. | ||
| * @returns {import('kolibri/composables/useFetch').FetchObject} The fetch state and actions. | ||
| */ | ||
| useRetrieve(id, options) { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion (still open): toValue inside the fetchMethod closure is deferred reading, not reactivity — nothing watches id/params, so the docstring's watch(datasetId, () => fetchData()) is mandatory at every call site. Given both docstrings advertise refs and getters, an internal watch on the reactive input (or an explicit "you must watch this yourself" line) would stop call sites silently rendering stale data.
nitpick, same place: useFetch documents fetchData: (...args) => Promise<void> and forwards its arguments (useFetch.js:86), but both wrappers build zero-arg closures (lines 1210 and 1236), so fetchData({ page: 2 }) is a silent no-op. Either forward ...args or note in the JSDoc that the reactive id/params are the only input.
| // A list endpoint returns a plain array when it is not paginated, and a | ||
| // `{ results, more, count }` object when it is. Handle both, so that a fetchMoreMethod | ||
| // can be supplied without knowing up front which shape will come back. | ||
| const responseData = fetchMoreMethod && !Array.isArray(response) ? response.results : response; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion (still open): this array/paginated tolerance is the one functional change in the move, and it fixes a real latent bug — the old fetchMoreMethod ? response.results : response turned an unpaginated array response into undefined for any consumer that supplied a fetchMoreMethod. It is only covered indirectly, through useList's "unpaginated array response" case in api-resource.spec.js; packages/kolibri/composables/__tests__/useFetch.spec.js has no case for it. A case there would keep the guard from being refactored away by someone reading only useFetch.
|
|
||
| beforeEach(() => { | ||
| resource = new Resources.Resource({ name: 'test' }); | ||
| resource.retrieve = jest.fn().mockResolvedValue({ id: 'abc', name: 'test' }); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick (still open): the Resource REST methods block mocks resource.client, which is the actual hard boundary; these two composable suites instead stub resource.retrieve / resource.list (line 1805), so they reduce to "the composable calls the method with toValue(...)" and would still pass if retrieve/list were broken. In particular the { results, more, count } unwrapping in useFetch._setData is only ever asserted against a hand-written paginated-looking mock, never against a real list return value. One end-to-end case per suite through the mocked client would tie the halves together. Same for resource.getUrlFunction = jest.fn(...) at line 1383 — kolibri/urls is already mocked, so populating it would exercise the real getUrlFunction → __resolveUrl path.
| * @throws {TypeError} - When no query parameters are given, to prevent an unfiltered | ||
| * bulk delete | ||
| */ | ||
| async bulkDelete(params = {}) { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick (still open): delete clears the baseline for the object it removes (line 1147); bulkDelete drops the objects server-side but leaves their baselines in the map. Harmless while ids are not reused, but it is an inconsistency between the two delete paths, and it compounds the unbounded-growth issue above.
- create/bulkCreate take `multipart` as a positional argument again, so the pre-existing `create(data, multipart)` signature keeps working for out-of-tree consumers of the published package. - `request` is now async, so an unresolvable action rejects instead of throwing synchronously - uniform with every method built on it. - `request` hands each caller its own deep copy of a GET's `data`, so coalesced readers (base CRUD and custom actions alike) can't mutate a payload another reader holds. - Document that useRetrieve/useList are not auto-reactive (refs are read at fetch time, not watched) and that only the top level is unwrapped. - Note the deliberate baseline gap on bulkDelete. - Drive the useRetrieve/useList specs through the mocked client boundary rather than stubbing sibling methods; add a direct useFetch test for the array/paginated shape; add coalesced-copy tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QKX2QS51BmgZNJL4EUJU6P
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15091 — delta re-review of 6847c440..4709ae66.
9 of 11 prior findings resolved or acknowledged; 2 still open (see below). CI is pending and manual QA did not run, so this stays a COMMENT rather than an approval.
The new useUpdate / onSuccess commit brings one real hazard: onSuccess sits outside useFetch's staleness guard, so a superseded fetch can still write a baseline. Details inline.
Two things not worth an inline comment:
- PR body / issue —
useUpdateanduseRetrieve'sonSuccesscame out of the review thread and aren't mentioned in either.useUpdateis a write-side API the follow-up migration issues will need to know exists; a line in the description and a note on #15056 would save the next author from finding it by accident. - docs —
docs/frontend_architecture/composables.rststill points atkolibri-common/composablesand its "Resource fetching" example is whatuseList/useRetrievenow replace. Out of scope for this branch, but it will steer the next author away from this work.
Prior-finding status
RESOLVED — packages/kolibri/composables/useFetch.js — useFetch imports used a .js extension not in the exports map
RESOLVED — packages/kolibri/apiResource.js:1088 — create() signature change is a silent break for out-of-tree consumers
ACKNOWLEDGED — packages/kolibri/apiResource.js:1088 — _baselines grows without bound and is not reset by clearCache()
ACKNOWLEDGED — packages/kolibri/apiResource.js:1136 — update() can skip the request entirely against a stale baseline
RESOLVED — packages/kolibri/apiResource.js:1026 — coalesced GETs hand every caller the same mutable response; request throws synchronously
RESOLVED — packages/kolibri/apiResource.js:1208 — useRetrieve/useList accept refs but never watch them
UNADDRESSED — packages/kolibri/apiResource.js:1229 — useFetch documents fetchData(...args) but both wrappers build zero-arg closures
RESOLVED — packages/kolibri/composables/useFetch.js:59 — the array/paginated tolerance has no test in useFetch's own suite
RESOLVED — packages/kolibri/tests/api-resource.spec.js:1736 — composable specs mock sibling methods rather than the client boundary
RESOLVED — packages/kolibri/apiResource.js:1198 — bulkDelete leaves baselines behind
UNADDRESSED — docs — docs/frontend_architecture/composables.rst still documents the superseded pattern
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| fetchMethod: async () => { | ||
| const object = await this.retrieve(toValue(id), { params: toValue(params) }); | ||
| if (onSuccess) { | ||
| onSuccess(object); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
important: onSuccess fires for fetches that useFetch discards as stale.
useFetch.fetchData only checks newFetchHasStarted() after fetchMethod resolves (packages/kolibri/composables/useFetch.js:86-89), so when a superseded fetch lands last, useFetch correctly drops its result for data.value — but onSuccess has already run with it. The generation guard that exists precisely to prevent out-of-order writes doesn't cover this new side channel.
That bites given the pairing the JSDoc advertises ("Handy for feeding the object to a useUpdate baseline") plus the documented watch(id, () => fetchData()):
useRetrieve(userId, { onSuccess: setBaseline })+useUpdate(userId, form).userIdgoes A → B quickly; both fetches in flight (different ids, so no coalescing).- A resolves last:
data.value= B,baseline= A. - Form shows B (
name: 'Beta'); user typesAlpha, which happens to be A's value. update()diffs against A → empty diff →updatereturns at line 1134 without issuing a PATCH. The rename is silently dropped.
The narrower variant — baseline from the wrong object so a genuinely changed field is omitted from the PATCH — is the same hazard with a quieter ending.
Cleanest fix is to give useFetch an onSuccess option invoked inside fetchData after the newFetchHasStarted() check, so it inherits the existing guard rather than reimplementing it; useRetrieve then just forwards the option. If you'd rather keep the change in apiResource.js, useRetrieve needs its own generation counter incremented per fetchMethod invocation and compared before calling onSuccess.
| this._inFlight.set(key, promise); | ||
| } | ||
| // Coalesced GETs share one response, so clone `data` per caller. | ||
| return promise.then(response => ({ ...response, data: cloneDeep(response.data) })); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: this clone runs on every GET, but the aliasing it defends against only exists when a second caller attaches to an in-flight promise — the originating caller's payload is unshared by construction.
On the hot paths that now route through the new list, that's a full deep copy per request with no aliasing to defend against: TaskResource.list is polled every 5s by useTaskPolling (packages/kolibri-common/composables/useTaskPolling.js:17), plus taskActions.js:40 and facilityTasksQueue.js:29 — and list then cloneDeeps each object again into _baselines (line 1088). Two full passes over the payload per poll on the low-end hardware Kolibri targets.
Cloning only for callers that attach keeps the guarantee (no two callers share data) and costs nothing when there is one caller:
const existing = this._inFlight.get(key);
if (existing) {
// Attached to someone else's request - clone so the two callers don't share a payload.
return existing.then(response => ({ ...response, data: cloneDeep(response.data) }));
}
const promise = this.client({ url, method, params }).finally(() => {
this._inFlight.delete(key);
});
this._inFlight.set(key, promise);
return promise;The spec at api-resource.spec.js:1451 still holds under this — its expect(a.data).not.toBe(shared.data) would just become an assertion about the two callers rather than the raw response.
| */ | ||
| useRetrieve(id, { params, onSuccess } = {}) { | ||
| return useFetch({ | ||
| fetchMethod: async () => { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick (still open): useFetch documents fetchData(...args) as forwarding to fetchMethod, but both wrappers build zero-arg closures (here and at line 1257), so any argument a caller passes to fetchData is silently dropped. Either forward ...args or note in the useRetrieve/useList JSDoc that fetchData takes no arguments.
| if (!id) { | ||
| throw TypeError('An id must be specified'); | ||
| } | ||
| if (baseline === undefined) { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: the null vs undefined distinction on baseline is load-bearing but undocumented. useUpdate always passes baseline explicitly and it is null until setBaseline runs, so a handle that never got a snapshot deliberately bypasses _baselines and sends the full payload. That's the right behaviour, but the JSDoc says only "When provided it takes precedence over any resource-held baseline" — someone writing { baseline: snapshot ?? null } opts out of the resource path without realising it. One clause noting that null means "no baseline, send as-is" while omitting the key means "use whatever the resource has" would close it.
| expect(firstResponse).toEqual(secondResponse); | ||
| }); | ||
|
|
||
| it('should give each coalesced caller an independent copy of response.data', async function () { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: these clone tests pin the actual guarantee (independent payloads) rather than the implementation that currently provides it, so the cheaper clone strategy suggested on apiResource.js:1056 would still be caught if it regressed.
4709ae6 to
04855d9
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15091 — delta re-review of 04855d9c66.
14 of 16 prior findings resolved; 2 still open (see below).
CI passing. Manual QA was required but did not run, so nothing here is based on the running UI.
Still open, and not anchorable to a changed line:
- PR body / issue #15056 — nitpick:
useUpdateis a public write-side API that came out of the review thread rather than the issue's acceptance criteria. A line in the PR description and a note on #15056 would let the follow-up migration issues know it exists. - docs/frontend_architecture/composables.rst — nitpick: still names
kolibri-common/composablesas the home for shared composables, and its "Resource fetching" example is whatuseList/useRetrievenow replace. Out of scope for this branch, but it will steer the next author away from this work.
Two new findings on the useUpdate code added in this delta — see inline.
Prior-finding status
RESOLVED — packages/kolibri/apiResource.js — create(params, multipart) → create(data, { multipart }) is a silent breaking signature change
RESOLVED — packages/kolibri/apiResource.js — list() deep-clones and stores every object it returns into _baselines
RESOLVED — packages/kolibri/apiResource.js:1114 — update skipping the request entirely when the diff is empty
RESOLVED — packages/kolibri/apiResource.js — two things about request as the designated primitive
RESOLVED — packages/kolibri/composables/useFetch.js:62 — array/paginated tolerance is the one functional change in the move
RESOLVED — packages/kolibri/tests/api-resource.spec.js — the Resource REST methods block mocks resource.client
RESOLVED — packages/kolibri/apiResource.js:1167 — bulkDelete drops objects server-side without clearing their baselines
RESOLVED — packages/kolibri/apiResource.js:1036 — rejected-promise poisoning in the in-flight map handled correctly (praise)
RESOLVED — packages/kolibri/apiResource.js — toValue inside the fetchMethod closure is deferred reading, not reactivity
RESOLVED — packages/kolibri/composables/useFetch.js:96 — onSuccess fires for fetches useFetch discards as stale
RESOLVED — packages/kolibri/apiResource.js:1025 — deep clone ran on every GET, not only on coalesced ones
RESOLVED — packages/kolibri/apiResource.js:1198 — useFetch documents fetchData(...args) but both wrappers build zero-arg closures
RESOLVED — packages/kolibri/apiResource.js:1097 — null vs undefined on baseline is load-bearing but undocumented
RESOLVED — packages/kolibri/apiResource.js — _baselines grows without bound and is not reset by clearCache()
UNADDRESSED — PR body / issue #15056 — useUpdate and useRetrieve's onSuccess are still undocumented outside the code
UNADDRESSED — docs/frontend_architecture/composables.rst — still documents the superseded pattern
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| error.value = null; | ||
| try { | ||
| const saved = await request(); | ||
| // The saved object is the freshest server truth, so it becomes the next baseline. |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: create() adopts the saved object as the next baseline, but nothing writes the server's new id back into the bound id. Since create() gates only on toValue(id), after a successful create the handle is still in create mode: a second save POSTs again and silently creates a duplicate, while update() still rejects with update() called without an id.
The spec encodes the real contract — api-resource.spec.js:1924 does id.value = 'abc' under the comment "The consumer adopts the new id" — but the JSDoc never states it, and the doc block above claims the opposite: "this handle can only ever create-or-update that one object, never arbitrary items". It also means id must be a ref/getter for the create→edit flow to work at all, while the signature advertises a plain string as valid.
Either is fine:
- Keep the caller in charge and say so — add a JSDoc line ("after
create()resolves, assign the returned id into the boundidref before any further write") and drop the "can only ever create-or-update that one object" claim. - Or close it in the composable — hold the created id in the closure and fall back to it:
let createdId = null;
const currentId = () => toValue(id) || createdId;
// create(): if (currentId()) reject; on success `createdId = saved[this.idKey]`
// update(): read currentId()useUpdate has no call sites yet, so whichever you pick is the contract the first adopter builds on.
| const create = () => { | ||
| if (toValue(id)) { | ||
| return Promise.reject( | ||
| ReferenceError('create() called for an object that already has an id; use update()'), |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: retrieve/update/delete all throw TypeError('An id must be specified') for a missing or wrong id, and ReferenceError appears exactly once elsewhere in this file — __resolveUrl, for a genuinely unresolvable reference. Both useUpdate guards are the same "wrong argument for this call" condition as Resource.update's, so TypeError would keep the class self-consistent. The spec asserts ReferenceError, so it's a two-line change if you take it.
04855d9 to
fc5a537
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15091 — delta re-review of fc5a537 (Resource.useUpdate + onSuccess).
All 17 prior findings resolved; 5 new findings, all in the new useUpdate. CI passing. No UI behaviour changed (the .vue/composable edits are import-path only), and manual QA did not run this round — nothing was visually verified.
The headline one: useUpdate.run has no sequencing token, while its useFetch-based siblings gained exactly that guard in this same commit. Combined with the empty-diff short-circuit in update, out-of-order responses can leave a stale baseline that makes the next save silently no-op. Details inline.
Comments on lines not in the diff
- packages/kolibri/apiResource.js:1255 — suggestion:
useUpdateis a third composable beyond #15056'suseRetrieve/useListACs, with no call site yet. Two things worth noting on the issue rather than absorbing silently: (a) the AC "update sends only the changed fields, diffed against a stored baseline" is now satisfied only viauseUpdate—Resource.update(id, data)called directly does no diffing, since the resource stores nothing. Defensible redesign, but a change to what #15056 specified. (b) A consumer still writesid.value ? update() : create()— the branch the binding was meant to absorb; a singlesave()dispatching onidwould remove it from every call site. Not asking for it to be pulled, just so follow-up migration issues aren't sized against an API nothing has exercised.
Prior-finding status
RESOLVED — packages/kolibri/apiResource.js — create(params, multipart) → create(data, ...) silent breaking signature change
RESOLVED — packages/kolibri/apiResource.js — list() deep-clones and stores every object into a per-resource _baselines map
RESOLVED — packages/kolibri/apiResource.js — _baselines never cleared by clearCache()
RESOLVED — packages/kolibri/apiResource.js:1106 — empty diff skips the request entirely rather than sending nothing
RESOLVED — packages/kolibri/apiResource.js — request as the designated primitive (two concerns)
RESOLVED — packages/kolibri/apiResource.js — toValue inside the fetchMethod closure is deferred reading, not reactivity
RESOLVED — packages/kolibri/composables/useFetch.js:62 — array/paginated tolerance is a functional change in the move
RESOLVED — packages/kolibri/apiResource.js — onSuccess fires for fetches useFetch discards as stale
RESOLVED — packages/kolibri/apiResource.js — response clone runs on every GET, defends an aliasing case that needs two callers
RESOLVED — packages/kolibri/apiResource.js — useFetch documents fetchData(...args) as forwarding, but the wrappers take none
RESOLVED — packages/kolibri/apiResource.js — null vs undefined on baseline is load-bearing but undocumented
RESOLVED — packages/kolibri/apiResource.js:1273 — create() adopts a baseline but nothing writes the server's new id back
RESOLVED — packages/kolibri/apiResource.js — useUpdate guards threw ReferenceError where the class uses TypeError
RESOLVED — packages/kolibri/apiResource.js:1159 — bulkDelete drops objects the server removed without clearing baselines
RESOLVED — packages/kolibri/tests/api-resource.spec.js — Resource REST methods mocks resource.client rather than the client boundary
RESOLVED — packages/kolibri/apiResource.js:1031 — praise: in-flight coalescing avoids rejected-promise poisoning
RESOLVED — packages/kolibri/tests/api-resource.spec.js — praise: clone tests pin the guarantee, not the implementation
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| } catch (e) { | ||
| error.value = e; | ||
| throw e; | ||
| } finally { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
important: run has no sequencing token, so two overlapping create()/update() calls interfere:
finally { isSaving.value = false }fires when the first call settles, soisSavingreadsfalsewhile a second write is still in flight — a form that disables its save button onisSavingre-enables it mid-save.error.value = nullon line 1270 likewise clears a concurrent call's error.setBaseline(saved)(line 1274) is last-settled-wins, not last-issued-wins. Issue PATCH{name:'A'}then PATCH{name:'B'}; if the responses arrive reversed, the baseline ends as{name:'A'}while the server holds'B'. The nextupdate()withname: 'A'then diffs to an empty payload and short-circuits at line 1104 — no request,errorstays null, and the caller is told it saved. The server keeps'B'.
The fix is the pattern this PR already added to useFetch — a monotonic counter captured at call time, re-checked before touching shared state:
let _writeCount = 0;
const run = async request => {
const current = ++_writeCount;
isSaving.value = true;
error.value = null;
try {
const saved = await request();
if (current === _writeCount) setBaseline(saved);
return saved;
} catch (e) {
if (current === _writeCount) error.value = e;
throw e;
} finally {
if (current === _writeCount) isSaving.value = false;
}
};useFetch guards onSuccess this way in this same commit (useFetch.js:90-98) precisely so a superseded fetch cannot record a stale baseline. The write side needs it for the same reason.
| // The saved object is the freshest server truth, so it becomes the next baseline. | ||
| setBaseline(saved); | ||
| return saved; | ||
| } catch (e) { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: run's catch sets error.value and rethrows, whereas useFetch.fetchData swallows into error.value and resolves (useFetch.js:99-104). Callers of the three new composables therefore need two different error idioms, and one who follows the useRetrieve idiom (watch error, ignore the returned promise) gets an unhandled rejection from useUpdate.
Either drop the rethrow so all three are uniform, or document the divergence — the JSDoc currently advertises error without mentioning that the promise also rejects.
| * passed to `list`. A ref or getter is read at fetch time (not watched). | ||
| * @param {object} [options] - Additional options. | ||
| * @param {(object: object) => void} [options.onSuccess] - Called with the retrieved collection | ||
| * after each successful fetch. |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: useFetch invokes onSuccess(response) with the raw response (useFetch.js:97), not the unwrapped responseData — so for a paginated list this is { results, more, count }, not the collection. The spec only covers the non-paginated array case, so the mismatch isn't caught. Since useRetrieve's doc points onSuccess at baseline-feeding, a reader will expect the same "here is your data" contract. Either say "the raw list response" or unwrap.
| * @param {object} [options] - Additional options. | ||
| * @param {object | import('vue').Ref<object> | (() => object)} [options.params] - Query | ||
| * parameters for `update`. | ||
| * @param {boolean} [options.multipart] - Whether `create` should send multipart form data. |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: line 1290 does toValue(multipart), so a ref or getter works, but this @param says {boolean} — while params on the line above gets the full object | Ref<object> | (() => object) union. Match it, or drop the toValue.
| return; | ||
| } | ||
| _setData(response); | ||
| // Runs only after the staleness check, so a superseded fetch cannot fire onSuccess - |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: placing onSuccess after newFetchHasStarted() rather than at the end of the try is the difference between a usable side-effect hook and a stale-baseline bug generator, and the comment records why. The superseded-fetch case is covered directly in useFetch.spec.js, which is the part that would otherwise silently regress.
43363ed to
a76aa12
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15091 — delta re-review of a76aa12. All prior findings are resolved or acknowledged; the _baselines map is gone and baselines now live in the caller-scoped useUpdate, with a write-side staleness guard mirroring useFetch's read-side one. Three doc/API nitpicks below, none blocking.
CI was still pending at review time, and this PR touches no UI files so no manual QA applies.
Prior-finding status
RESOLVED — packages/kolibri/apiResource.js — create(params, multipart) → create(data, { multipart }) silent breaking change
RESOLVED — packages/kolibri/apiResource.js — list() deep-clones and stores every returned object in _baselines
ACKNOWLEDGED — packages/kolibri/apiResource.js:1106 — update skips the request entirely on an empty diff
RESOLVED — packages/kolibri/apiResource.js — request as the designated primitive
RESOLVED — packages/kolibri/apiResource.js — toValue inside the fetchMethod closure is deferred reading, not reactivity
RESOLVED — packages/kolibri/composables/useFetch.js:62 — array/paginated tolerance is the one functional change in the move
RESOLVED — packages/kolibri/tests/api-resource.spec.js — Resource REST methods mocks resource.client rather than the hard boundary
RESOLVED — packages/kolibri/apiResource.js:1159 — bulkDelete does not clear baselines for the objects it removes
RESOLVED — packages/kolibri/apiResource.js:1031 — request coalescing avoids rejected-promise poisoning (praise)
RESOLVED — packages/kolibri/apiResource.js — onSuccess fires for fetches that useFetch discards as stale
RESOLVED — packages/kolibri/apiResource.js — clone on every GET defends against aliasing that only arises on concurrent callers
RESOLVED — packages/kolibri/apiResource.js — useFetch documents fetchData(...args) forwarding, but the wrappers take none
RESOLVED — packages/kolibri/apiResource.js — null vs undefined distinction on baseline was load-bearing but undocumented
RESOLVED — packages/kolibri/tests/api-resource.spec.js — clone tests pin the guarantee, not the implementation (praise)
RESOLVED — packages/kolibri/apiResource.js:1291 — create() adopts the saved object as the baseline but nothing writes the new id back
RESOLVED — packages/kolibri/apiResource.js — uniform TypeError('An id must be specified') for missing/wrong id
RESOLVED — packages/kolibri/apiResource.js:1301 — run has no sequencing token, so overlapping writes interfere
RESOLVED — packages/kolibri/apiResource.js:1296 — run's catch sets error.value and rethrows, unlike useFetch.fetchData
RESOLVED — packages/kolibri/apiResource.js — onSuccess invoked with the raw response rather than the unwrapped object
RESOLVED — packages/kolibri/apiResource.js:1257 — multipart @param {boolean} vs toValue(multipart) mismatch
RESOLVED — packages/kolibri/composables/useFetch.js:94 — onSuccess placed after newFetchHasStarted() (praise)
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| const create = () => { | ||
| if (toValue(id)) { | ||
| return Promise.reject( | ||
| TypeError('create() called for an object that already has an id; use update()'), |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: these two guard paths (create() with an id, update() without one) reject without going through run, so error is neither set nor cleared — contrary to the docstring at line 1245 ("On failure create()/update() set error and also reject"). A component rendering error shows nothing for a misuse, and worse, keeps displaying a stale error from a previous genuine failure. Either route the guards through run or narrow the docstring to "on request failure".
There was a problem hiding this comment.
This would be a dev error, though. I think we can throw an "unknown error" or something, but we would need to introduce strings here, which I think may be more complicated. This should not happen in production code; it's just a guard for dev purposes.
| * `data` is diffed against it and only changed fields are sent; when omitted (or falsy), | ||
| * `data` is sent unchanged. | ||
| * @returns {Promise} - Promise that resolves with the updated object | ||
| * @throws {TypeError} - When `id` is missing |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: update (like retrieve, delete, bulkCreate, bulkDelete) is async, so this validation rejects rather than throwing synchronously — unlike the sibling accessDetailEndpoint, which really does throw. Same @throws wording for both behaviours is misleading for anyone writing try { resource.retrieve() } without an await; documenting it under @returns as a rejection would be accurate.
There was a problem hiding this comment.
I think throws is more descriptive, we can keep it; we will remove the legacy methods anyway
| * @returns {import('kolibri/composables/useFetch').FetchObject} The fetch state and actions. | ||
| * The returned `fetchData` takes no arguments. | ||
| */ | ||
| useRetrieve(id, { params, onSuccess } = {}) { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: useRetrieve inherits useFetch's list-shaped handling — _setData unconditionally assigns moreParams/count from the response, so a detail payload that happens to carry a count field surfaces as the fetch's count, and hasMore/fetchMore are meaningless here but still returned. Worth returning just data/loading/error/fetchData, or documenting that only that subset is meaningful.
There was a problem hiding this comment.
Yeah, that makes sense, thanks.
| // flight. This mirrors the staleness guard useFetch uses on the read side. | ||
| let writeCount = 0; | ||
| const run = async request => { | ||
| const current = ++writeCount; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: the monotonic writeCount token is the write-side mirror of useFetch's read-side guard, and the tests assert the real failure mode (stale baseline → silent no-op on the next save) rather than the mechanism.
a76aa12 to
93a4150
Compare
| * save: () => Promise<object>, | ||
| * }} | ||
| */ | ||
| useUpdate(id, data, { params, multipart } = {}) { |
There was a problem hiding this comment.
This could potentially be extracted to a different module (similar to the relation between useFetch and useRetrieve, but keeping it here to iterate on it more easily). A working example of a refactor using this would be something like AlexVelezLl@6b3ae79
93a4150 to
ea4f39e
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15091 — 23 of 23 prior findings resolved; 4 prior threads settled. Five new findings below, one important.
Note: the head advanced to ea4f39ea33 after these passes ran. That commit only trims JSDoc (removes the useRetrieve/useList examples and shortens two comments) with no functional change, but it was not part of the reviewed state — hence COMMENT rather than APPROVE. CI is green on the current head.
Manual QA exercised the useFetch move end to end (coach lesson/quiz resource selection incl. fetchMore pagination, course assignment, learn library, device channels/facilities, facility data export) plus RTL, 412px mobile and axe-core audits. No regression attributable to this PR; all a11y violations found were pre-existing in unchanged files (SearchChips.vue button-name, AccordionItem.vue nested-interactive/aria-valid-attr-value) and are out of scope here. Not exercised in the browser: bulkCreate (only reachable via a Studio import, a KDP sync, or a destructive channel delete) and the three new composables, which have no call sites yet.
One thing the doc trim cost: the removed useRetrieve example was the only place documenting the watch(id, () => fetchData()) pairing that the baseline correctness argument depends on — see the comment on update().
Prior-finding status
RESOLVED — packages/kolibri/apiResource.js — create() signature breaking change
RESOLVED — packages/kolibri/apiResource.js — list() deep-clones every object into _baselines
RESOLVED — packages/kolibri/apiResource.js — unbounded _baselines not cleared by clearCache()
RESOLVED — packages/kolibri/apiResource.js:1106 — empty diff skips the PATCH entirely
RESOLVED — packages/kolibri/apiResource.js — request as the designated primitive
RESOLVED — packages/kolibri/apiResource.js — toValue in the fetchMethod closure is deferred, not reactive
RESOLVED — packages/kolibri/composables/useFetch.js:62 — array/paginated tolerance is a functional change in the move
RESOLVED — packages/kolibri/tests/api-resource.spec.js — REST-method block mocks resource.client
RESOLVED — packages/kolibri/apiResource.js:1159 — bulkDelete does not clear baselines
RESOLVED — packages/kolibri/apiResource.js:1031 — praise: rejected-promise poisoning avoided in coalescing
RESOLVED — packages/kolibri/apiResource.js — onSuccess fires for fetches useFetch discards as stale
RESOLVED — packages/kolibri/apiResource.js — clone runs on every GET, not only when aliased
RESOLVED — packages/kolibri/apiResource.js — fetchData(...args) doc vs the no-arg wrappers
RESOLVED — packages/kolibri/apiResource.js — null vs undefined baseline distinction undocumented
RESOLVED — packages/kolibri/tests/api-resource.spec.js — praise: clone tests pin the guarantee, not the implementation
RESOLVED — packages/kolibri/apiResource.js:1276 — create() adopts a baseline but does not write back the new id
RESOLVED — packages/kolibri/apiResource.js — inconsistent TypeError('An id must be specified') guards
RESOLVED — packages/kolibri/apiResource.js:1286 — run had no sequencing token for overlapping writes
RESOLVED — packages/kolibri/apiResource.js:1281 — run both sets error.value and rethrows
RESOLVED — packages/kolibri/apiResource.js — onSuccess receives the raw response, not the unwrapped data
RESOLVED — packages/kolibri/apiResource.js:1242 — multipart @param {boolean} vs toValue
RESOLVED — packages/kolibri/composables/useFetch.js:94 — praise: onSuccess placed after the staleness check
ACKNOWLEDGED — packages/kolibri/apiResource.js:1296 — create()/update() guards reject outside run
ACKNOWLEDGED — packages/kolibri/apiResource.js:1090 — async update rejects rather than throws on a missing id
ACKNOWLEDGED — packages/kolibri/apiResource.js:1185 — useRetrieve inherits useFetch's list-shaped moreParams handling
ACKNOWLEDGED — packages/kolibri/apiResource.js:1271 — praise: monotonic writeCount token
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Ran the same phased review passes as a first review (core, frontend/backend lenses, manual QA when required)
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| onSuccess, | ||
| // `more` is the complete set of query parameters for the next page, so it replaces | ||
| // rather than extends the original params. | ||
| fetchMoreMethod: more => this.list(more), |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
important: useList can only page endpoints that emit a more object, which excludes the most common paginator in the codebase.
fetchMoreMethod and hasMore are both driven off response.more (useFetch.js:73: moreParams.value = response.more || null). Of Kolibri's three paginators, only two emit it:
ValuesViewsetLimitOffsetPagination→{count, more, results}(kolibri/core/utils/pagination.py:194) ✅ValuesViewsetCursorPagination→{more, results}(:312) ✅ — nocount, socount.valuestaysnullValuesViewsetPageNumberPagination→{page, count, total_pages, results}(:104) ❌ nomore
That third one is the most used (grep pagination_class: 7 viewsets on OptionalPageNumberPagination vs 5 on the more-emitting classes). Against one of those, hasMore is permanently false and fetchMore() returns at its first guard, while count.value is set from response.count — a caller sees count === 90, data.value.length === 25, hasMore === false: a silently truncated list with no error and no way to load the rest.
Either teach _setData the page-number shape (derive more from {page, total_pages} when response.more is absent but response.page is present, mirroring get_more()), or keep the more-only contract and say so in this JSDoc.
While here: the onSuccess doc above (line 1199) should carry over useFetch's "Not called for fetchMore" clause (useFetch.js:40) — useList is the one composable where fetchMore is reachable, so a consumer mirroring results into a cache would silently miss every appended page.
There was a problem hiding this comment.
I think this might be best achieved by standardizing some of our backend pagination logic also.
There was a problem hiding this comment.
Agreed — fixing it at the paginator level is the better shape than teaching _setData three response envelopes. Deferring to that; nothing needed here beyond whatever the JSDoc should say once the backend contract settles.
There was a problem hiding this comment.
I think that at the very least, returning page and totalPages will be useful here, because if we don't, these properties get lost as there is no way to access the full response object. I have modified useFetch just to return these, to be able to actually use the composable in these cases. I also considered adding this in separate composables like usePageNumberPagination(Resource.useList()), but there aren't yet too many examples in the codebase to infer a proper abstraction, so I'm just leaving the simple additions on the returned object.
There was a problem hiding this comment.
That works well — surfacing page/totalPages means the page-number shape isn't lossy any more, and the useList JSDoc now states plainly that hasMore/fetchMore are more-only and that page-number endpoints drive the pager through params + fetchData. That was the second option I offered, and the extra refs make it actionable rather than just documented. Agree on holding off on a usePageNumberPagination-style wrapper until there are enough call sites to show what the abstraction should be.
The onSuccess "Not called for fetchMore" clause is carried over too. Nothing outstanding from me here.
| return Promise.reject(TypeError('update() called without an id; use create()')); | ||
| } | ||
| return run(() => | ||
| this.update(currentId, toValue(data), { params: toValue(params), baseline }), |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: baseline is never invalidated when the bound id changes, so a rebind can silently drop changed fields.
baseline is written only by setBaseline, create(), and a successful update() — never by an id change. But id is explicitly a ref/getter the docstring expects to change ("After create() resolves, the caller can set the new id"). If a consumer rebinds id to a different object of the same resource and calls update() before the new baseline is installed, object B's data is diffed against object A's snapshot: any field where B's edited value coincidentally equals A's old value is dropped from the PATCH, and if every field coincides the write is skipped entirely and reported as success.
The useRetrieve(id, { onSuccess: setBaseline }) + watch(id, fetchData) pairing closes most of this, but leaves the window between the id change and the retrieve resolving — and ea4f39ea33 removed the example that was the only place documenting that pairing.
A self-contained guard makes it impossible rather than merely unlikely:
const baselineForId = baseline && baseline[this.idKey] === currentId ? baseline : null;A mismatched baseline then degrades to a full PATCH, which is always correct — consistent with the docstring's framing of the diff as a payload optimisation rather than a correctness requirement.
| const error = ref(null); | ||
| // Not exposed directly - only through setBaseline / create / update. It is read at write | ||
| // time and never rendered, so there is nothing to gain from making it reactive. | ||
| let baseline = null; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: keeping baseline private and non-reactive forecloses the dirty check ~10 components currently hand-roll.
The reasoning in the comment holds for useUpdate's own internals, but the baseline is exactly the state a consumer needs for an unsaved-changes check, and that need recurs — unsavedChanges/isDirty appear in at least ten components today (e.g. facility/.../CloseConfirmationGuard.vue, facility/.../UserCreate/index.vue, coach/.../CloseConfirmationModal.vue), each tracking its own snapshot of the form data.
Since data is already a MaybeRefOrGetter, this is two lines with baseline as a shallowRef:
const isDirty = computed(() => baseline.value != null && !isEqual(toValue(data), baseline.value));Not required for this PR, but "the composable that owns the last server snapshot" is the natural home for it, and adding it later breaks nothing.
| // General case (writes): no dedup, as two writes are two distinct intents. | ||
| return this.client({ url, method, params, data, multipart }); | ||
| } | ||
| const key = `${url}?${stableSerialize(params)}`; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: no way to opt out of GET coalescing for a post-write refetch.
Coalescing is unconditional for every GET, which is what #15056 asks for — but it means a read issued after a write can attach to a GET the server started before it and resolve with pre-write data. Concretely: useTaskPolling fires TaskResource.list({ queue }) on a 5s timer (packages/kolibri-common/composables/useTaskPolling.js:17); a component that starts a task and then calls TaskResource.list({ queue }) to refresh can land inside that in-flight poll and render the task as absent. The next poll corrects it, so this is transient UI rather than data loss.
Worth adding a dedupe: false (or force: true) option on request, threaded through retrieve/list, so a caller needing read-your-writes can say so? Raising it now because the escape hatch is much cheaper to add while request has no consumers than after plugins migrate onto it.
| expect(client).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should diff update against a baseline set via setBaseline', async () => { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: the documented useRetrieve(onSuccess) → useUpdate(setBaseline) wiring is never exercised end to end.
onSuccess exists specifically to feed a baseline (apiResource.js:1175-1177), and that load-then-edit flow is the headline reason both features are in this PR. The tests cover each half in isolation — onSuccess against a jest.fn(), and setBaseline with a literal object here — but nothing asserts that a retrieved object handed straight to setBaseline produces a correctly narrowed PATCH. That composition is where the interesting failure lives: the retrieved shape and the PATCH payload shape are related by the diff, and retrieve hands back data that may have travelled through the coalescing cloneDeep path.
~15 lines: useRetrieve(id, { onSuccess: setBaseline }), fetchData(), mutate one field of the bound data ref, update(), assert a single-field PATCH.
| * save: () => Promise<object>, | ||
| * }} | ||
| */ | ||
| useUpdate(id, data, { params, multipart } = {}) { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: useUpdate also creates, which the name hides. It returns create, update and save, and its own doc leads with the create-then-update lifecycle, so const { save } = FacilityUserResource.useUpdate(id, data) for a new user requires knowing that creating lives in useUpdate. useSave (matching the save() it exists to provide) or useWrite would describe the surface. Cheap while there are no consumers.
There was a problem hiding this comment.
We can also just drop the create in this composable; I'd like to keep useUpdate for discoverability.
There was a problem hiding this comment.
Fair — discoverability of useUpdate beats the naming precision, and it was only a nitpick. Your call either way; I'm fine with it as-is.
rtibbles
left a comment
rtibbles
left a comment
There was a problem hiding this comment.
Just a few thoughts, but this is looking pretty much how I expected - no surprises, and I think the details of the composables will become more apparent as we dig into the actual switch overs. Different kinds of pagination being one of them!
| multipart = false, | ||
| } = {}) { | ||
| const url = this.__resolveUrl(action, routeParams); | ||
| if (method.toUpperCase() !== 'GET') { |
There was a problem hiding this comment.
One alternative here (which we do not need to do) would be to implement this as an axios request interceptor, rather than in the resource layer - but we don't have a strong need to do client based request coalescing, so I am unconcerned.
| * @returns {string} - The resolved URL | ||
| * @throws {ReferenceError} - When no URL is registered for this action | ||
| */ | ||
| __resolveUrl(action, routeParams) { |
There was a problem hiding this comment.
This could just be ...routeParams, no? Then you don't need the is array check as you always get the params as an iterator?
There was a problem hiding this comment.
No, routeParams can also be an object, e.g., __resolveUrl("detail", { pk: ... }).
There was a problem hiding this comment.
Ah, okay, I got you. Yeah, we could just do a ...routeParams and just pass it like ...routeParams, that'd be equivalent 😅. I think I still prefer this way, as it's a bit more explicit that the routeParams argument has the same shape as routeParams on the request method expects (where it'd be a bit less intuitive if we recollect them as ...routeParams).
| onSuccess, | ||
| // `more` is the complete set of query parameters for the next page, so it replaces | ||
| // rather than extends the original params. | ||
| fetchMoreMethod: more => this.list(more), |
There was a problem hiding this comment.
I think this might be best achieved by standardizing some of our backend pagination logic also.
ea4f39e to
6340404
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15091 — delta re-review of 6340404.
29 of 31 prior findings resolved; 1 acknowledged (documented rather than renamed), 1 still open.
Two new suggestions inline, neither blocking. CI passing. Manual QA was required but did not run this round, so nothing here is visually verified; the eleven plugin file changes are import-path and @typedef swaps with no template change.
Still open
packages/kolibri/apiResource.js:1029— suggestion: no way to opt out of GET coalescing for a post-write refetch. Unchanged since the last round; minor, and fine to defer if you'd rather wait for a caller that needs it.
Prior-finding status
RESOLVED — packages/kolibri/apiResource.js — create(params, multipart) → create(data, {multipart}) silent signature break
RESOLVED — packages/kolibri/apiResource.js — unbounded per-singleton _baselines map
RESOLVED — packages/kolibri/apiResource.js:1116 — empty diff skipped the request and resolved with a list-shaped baseline
RESOLVED — packages/kolibri/apiResource.js — request as the designated primitive
RESOLVED — packages/kolibri/apiResource.js — toValue in the fetchMethod closure is deferred reading, not reactivity
RESOLVED — packages/kolibri/composables/useFetch.js:72 — array/paginated tolerance is a functional change in the move
RESOLVED — packages/kolibri/tests/api-resource.spec.js — Resource REST methods mocks resource.client
RESOLVED — packages/kolibri/apiResource.js:1169 — bulkDelete did not clear baselines the way delete does
RESOLVED — packages/kolibri/apiResource.js — praise: rejected-promise poisoning handled in the dedup map
RESOLVED — packages/kolibri/apiResource.js — onSuccess fired for fetches useFetch discards as stale
RESOLVED — packages/kolibri/apiResource.js — clone ran on every GET, not only on coalesced ones
RESOLVED — packages/kolibri/apiResource.js — useFetch documented fetchData(...args) but the wrappers take none
RESOLVED — packages/kolibri/apiResource.js — null vs undefined on baseline was load-bearing but undocumented
RESOLVED — packages/kolibri/tests/api-resource.spec.js — praise: clone tests pin the guarantee, not the implementation
RESOLVED — packages/kolibri/apiResource.js:1313 — create() adopted the saved object as baseline without writing back the new id
RESOLVED — packages/kolibri/apiResource.js — inconsistent TypeError('An id must be specified') phrasing across methods
RESOLVED — packages/kolibri/apiResource.js:1323 — run had no sequencing token; overlapping writes interfered
RESOLVED — packages/kolibri/apiResource.js:1318 — run's catch both sets error.value and rethrows
RESOLVED — packages/kolibri/apiResource.js — onSuccess receives the raw response, not the unwrapped object
RESOLVED — packages/kolibri/apiResource.js:1279 — multipart @param {boolean} while toValue accepts a ref
RESOLVED — packages/kolibri/composables/useFetch.js:108 — praise: onSuccess placed after newFetchHasStarted()
RESOLVED — packages/kolibri/apiResource.js:1333 — guard paths rejected without going through run
RESOLVED — packages/kolibri/apiResource.js:1100 — async validation rejects rather than throwing synchronously
RESOLVED — packages/kolibri/apiResource.js:1200 — useRetrieve inherited useFetch's list-shaped handling
RESOLVED — packages/kolibri/apiResource.js:1308 — praise: monotonic writeCount mirrors the read-side guard
RESOLVED — packages/kolibri/apiResource.js:1235 — useList could only page more-emitting endpoints
RESOLVED — packages/kolibri/apiResource.js — baseline was never invalidated when the bound id changed
RESOLVED — packages/kolibri/apiResource.js — baseline private and non-reactive foreclosed the dirty check
RESOLVED — packages/kolibri/tests/api-resource.spec.js:2009 — documented useRetrieve→useUpdate wiring now exercised end to end
ACKNOWLEDGED — packages/kolibri/apiResource.js:1282 — useUpdate also creates; documented rather than renamed
UNADDRESSED — packages/kolibri/apiResource.js:1029 — no opt-out of GET coalescing for a post-write refetch
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Ran the same phased review passes as a first review (core, frontend/backend lenses, manual QA when required)
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| // the legacy Model/Collection layer did. This only intercepts to log - the rejection still | ||
| // propagates to the caller unchanged. | ||
| const logAndRethrow = error => { | ||
| this.logError(error); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: on a network failure this replaces the real error with a TypeError.
logError (line 1468, unchanged) guards on err.config but then dereferences err.response.statusText (line 1474). Axios sets config and leaves response undefined for a network failure — new AxiosError(msg, ERR_NETWORK, config, request) in axios/lib/adapters/xhr.js omits the 5th argument. So logError throws from inside logAndRethrow, throw error is never reached, and the caller sees TypeError: Cannot read properties of undefined (reading 'statusText') instead of the request error. Cancellations are safe (a Cancel has no config), but the first failure after a drop is not.
The latent bug is pre-existing and shared with the Model/Collection paths, so it isn't a regression you introduced — but request newly puts list/create on it, and those run on polled traffic (useTaskPolling.js:17, facilityTasksQueue.js:29, useDownloadRequests.js:47, useChannels.js:16). Any of those callers inspecting error.response in a .catch now gets a TypeError.
Cheapest fix is widening the existing guard to if (!err.config || !err.response) { return; } — keeps logError's contract (log only what it can describe) and lets the real error propagate on every path, new and legacy.
| * @param {object | import('vue').Ref<object> | (() => object)} [options.params] - Query | ||
| * parameters passed to `retrieve`. A ref or getter is read at fetch time (not watched). | ||
| * @param {(object: object) => void} [options.onSuccess] - Called with the retrieved object | ||
| * after each successful fetch. Handy for feeding the object to a `useUpdate` baseline (via |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: the wiring documented here leaves isDirty true on load.
isDirty is baseline != null && !isEqual(toValue(data), baseline), and onSuccess: setBaseline sets the baseline without seeding data. The spec at api-resource.spec.js:2130-2153 is that pairing verbatim: after await fetchData(), baseline.value is the fetched object while data.value is still null, so isDirty is true before the user has touched anything. The spec assigns data.value on the next line and never reads isDirty, so nothing catches it. A Save button or unsaved-changes guard bound to isDirty — the stated purpose — would fire on page load.
Cheapest fix is to document the two-step form as canonical: onSuccess: obj => { data.value = { ...obj }; setBaseline(obj); }. Alternatively useUpdate could expose a single reset(object) that seeds both, which removes the trap instead of describing it.
| if (existing) { | ||
| // Attached to an in-flight request another caller started - clone `data` so the two | ||
| // callers don't share one mutable payload. | ||
| return existing.then(response => ({ ...response, data: cloneDeep(response.data) })); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: handing coalescing callers their own copy while the originator keeps the raw payload is the right trade — the copy cost falls only on callers that were saved a round trip.
6340404 to
6b4e5b6
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15091 — all prior findings resolved; 1 new suggestion below (doc accuracy on isSaving).
Frontend tests pass on 6b4e5b67; the remaining pending checks are Python jobs unrelated to this diff. Manual QA did not run for this round, so nothing here is based on the running UI.
Prior-finding status
RESOLVED — packages/kolibri/apiResource.js — create(params, multipart) → create(data, { multipart }) silent breaking change
RESOLVED — packages/kolibri/apiResource.js — list() deep-clones and stores every object into _baselines
RESOLVED — packages/kolibri/apiResource.js:1116 — empty diff skips the request entirely instead of sending changed fields
RESOLVED — packages/kolibri/apiResource.js — request as the designated primitive
RESOLVED — packages/kolibri/apiResource.js — toValue inside the fetchMethod closure is deferred reading, not reactivity
RESOLVED — packages/kolibri/composables/useFetch.js:72 — array/paginated tolerance is the one functional change in the move
RESOLVED — packages/kolibri/tests/api-resource.spec.js — Resource REST methods block mocks resource.client
RESOLVED — packages/kolibri/apiResource.js:1169 — bulkDelete does not clear baselines the way delete does
RESOLVED — packages/kolibri/apiResource.js — praise: rejected-promise poisoning avoided in request coalescing
RESOLVED — packages/kolibri/apiResource.js — onSuccess fires for fetches useFetch discards as stale
RESOLVED — packages/kolibri/apiResource.js — clone runs on every GET though aliasing only exists for coalesced callers
RESOLVED — packages/kolibri/apiResource.js — fetchData(...args) forwarding documented but not honoured by the wrappers
RESOLVED — packages/kolibri/apiResource.js — null vs undefined baseline distinction is load-bearing but undocumented
RESOLVED — packages/kolibri/tests/api-resource.spec.js — praise: clone tests pin the guarantee, not the implementation
RESOLVED — packages/kolibri/apiResource.js:1316 — create() adopts the saved object as baseline but does not write the new id back
RESOLVED — packages/kolibri/apiResource.js — inconsistent TypeError('An id must be specified') handling across methods
RESOLVED — packages/kolibri/apiResource.js:1326 — run has no sequencing token, so overlapping writes interfere
RESOLVED — packages/kolibri/apiResource.js:1321 — run both sets error.value and rethrows, unlike useFetch.fetchData
RESOLVED — packages/kolibri/apiResource.js — onSuccess receives the raw response, not the unwrapped data
RESOLVED — packages/kolibri/apiResource.js:1282 — @param {boolean} multipart understates the accepted ref/getter
RESOLVED — packages/kolibri/composables/useFetch.js:108 — praise: onSuccess placed after newFetchHasStarted()
RESOLVED — packages/kolibri/apiResource.js:1336 — guard paths reject without going through run
RESOLVED — packages/kolibri/apiResource.js:1100 — update validation rejects rather than throws synchronously
RESOLVED — packages/kolibri/apiResource.js:1202 — useRetrieve inherits useFetch's list-shaped handling
RESOLVED — packages/kolibri/apiResource.js:1311 — praise: monotonic writeCount mirrors useFetch's read-side guard
RESOLVED — packages/kolibri/apiResource.js:1237 — useList could only page more-emitting endpoints
RESOLVED — packages/kolibri/apiResource.js — baseline never invalidated when the bound id changes
RESOLVED — packages/kolibri/apiResource.js — private, non-reactive baseline forecloses the hand-rolled dirty check
RESOLVED — packages/kolibri/tests/api-resource.spec.js:2020 — useRetrieve(onSuccess) → useUpdate(setBaseline) wiring untested end to end
RESOLVED — packages/kolibri/apiResource.js:1285 — useUpdate also creates, which the name hides
RESOLVED — packages/kolibri/apiResource.js:1029 — no way to opt out of GET coalescing for a post-write refetch
RESOLVED — packages/kolibri/apiResource.js:1022 — network failure replaced the real error with a TypeError
RESOLVED — packages/kolibri/apiResource.js — documented wiring left isDirty true on load
RESOLVED — packages/kolibri/apiResource.js:1034 — praise: coalescing callers get their own copy
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| /** | ||
| * @typedef {object} UpdateObject | ||
| * @property {import('vue').Ref<boolean>} isSaving - Whether a write is in flight. Stays true | ||
| * until the latest-issued write settles (concurrent writes are sequenced). |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: nothing sequences concurrent writes. Two update() calls each go straight to run() → this.update() → request(), and writes are excluded from the in-flight coalescing (apiResource.js:1025-1028). The composable's own test confirms it: api-resource.spec.js:2101-2105 primes client with two deferreds and both are consumed before either settles — two PATCHes on the wire at once.
writeCount protects the local bookkeeping (baseline, isSaving, error), not server state. If the earlier-issued PATCH lands second, the server keeps the older payload while the baseline records the newer one — a lost update with no signal to the caller. The mechanism is fine; it's the parenthetical that will mislead. Something like "the latest-issued write wins the local bookkeeping; writes are not serialised, so a rapidly re-issued save can still race server-side" gives a consumer what they need to reason about a save button.
| expect(isSaving.value).toBe(false); | ||
| }); | ||
|
|
||
| it('should keep the baseline from the last-issued write when responses arrive out of order', async () => { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: this asserts the consequence rather than the mechanism — it drives the reversed settle order, then proves the baseline is right by showing that re-saving B issues no request at all.
useUpdate(id, data, { params, multipart }) is a Resource method beside
useRetrieve/useList - the write-side counterpart to useFetch. Identity and
payload are bound once; the handle can only create-or-update that one object:
create() rejects when the id is set, update() rejects when it is empty. Both
are zero-arg and read the bound refs at call time. It keeps the last server
snapshot (deep-copied) so update sends only changed fields; create sets it,
and setBaseline lets a read set it on load.
The resource keeps no baseline of its own: update() diffs solely against the
{ baseline } the caller passes (typically the composable), so there is no
unbounded per-id store and no stale-baseline skip.
useRetrieve gains an onSuccess option, forwarded to useFetch and invoked
inside its staleness guard, so a fetch superseded by a newer one never fires
it (and never records a stale baseline). Coalesced GETs deep-copy data only
for callers that attach to an in-flight request; the originating caller owns
its payload uncopied.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011rsw11J2rS4mMpKzqR7Msv
6b4e5b6 to
4725b79
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15091 — delta re-review of 4725b794fe. All 34 prior findings are closed out: 33 resolved, 1 acknowledged (the empty-diff short-circuit, kept deliberately). _baselines is gone entirely. Baseline is now an explicit update argument held in useUpdate's closure. useList documents both pagination modes. page/totalPages are surfaced rather than faked into a more cursor. CI is green. Two small new notes below. Nothing blocking.
Manual QA did not run for this PR, so nothing here is based on the rendered UI.
Comments on lines not in the diff
- docs/howtos/working_with_urls_and_api_endpoints.md — suggestion: the one developer-facing doc for this layer still presents
fetchCollection/fetchModel/saveModel(lines 230-240) andaccessListEndpoint/accessDetailEndpoint(lines 486-493) as the Resource API, and its decision guide at line 347 tells readers to reach forfetchCollection.AGENTS.mdpoints agents atdocs/howtos/. That keeps steering new call sites toward the methods this series is retiring. Docs aren't in the acceptance criteria, so this is optional here. A short "New methods (preferred)" block is cheap now, and saves untangling divergent call sites in the migration issues.
Prior-finding status
RESOLVED — packages/kolibri/apiResource.js — create(params, multipart) → create(data, { multipart }) silent signature break
RESOLVED — packages/kolibri/apiResource.js — list() deep-clones and stores every object into a per-singleton _baselines
ACKNOWLEDGED — packages/kolibri/apiResource.js:1116 — empty diff skips the request entirely rather than sending only changed fields
RESOLVED — packages/kolibri/apiResource.js — request as the designated primitive
RESOLVED — packages/kolibri/apiResource.js — toValue inside the fetchMethod closure is deferred reading, not reactivity
RESOLVED — packages/kolibri/composables/useFetch.js:72 — array/paginated tolerance is the one functional change in the move
RESOLVED — packages/kolibri/tests/api-resource.spec.js — Resource REST methods mocks resource.client, the actual hard boundary
RESOLVED — packages/kolibri/apiResource.js:1169 — bulkDelete drops objects server-side without clearing their baselines
RESOLVED — packages/kolibri/apiResource.js — praise: rejected-promise poisoning handled in the coalescing cache
RESOLVED — packages/kolibri/apiResource.js — onSuccess fires for fetches useFetch discards as stale
RESOLVED — packages/kolibri/apiResource.js — GET clone runs unconditionally though aliasing only exists when coalescing
RESOLVED — packages/kolibri/apiResource.js — useFetch documents fetchData(...args) forwarding to fetchMethod
RESOLVED — packages/kolibri/apiResource.js — null vs undefined on baseline is load-bearing but undocumented
RESOLVED — packages/kolibri/tests/api-resource.spec.js — praise: clone tests pin the guarantee, not the implementation
RESOLVED — packages/kolibri/apiResource.js:1318 — create() adopts the saved object as baseline; server id write-back
RESOLVED — packages/kolibri/apiResource.js — TypeError('An id must be specified') consistency across retrieve/update/delete
RESOLVED — packages/kolibri/apiResource.js:1328 — run had no sequencing token for overlapping writes
RESOLVED — packages/kolibri/apiResource.js:1323 — run sets error.value and rethrows, unlike useFetch.fetchData
RESOLVED — packages/kolibri/apiResource.js — useFetch invokes onSuccess(response) with the raw response
RESOLVED — packages/kolibri/apiResource.js:1284 — @param {boolean} multipart vs the toValue call at 1290
RESOLVED — packages/kolibri/composables/useFetch.js:108 — praise: onSuccess placed after newFetchHasStarted()
RESOLVED — packages/kolibri/apiResource.js:1338 — guard paths rejecting without going through run
RESOLVED — packages/kolibri/apiResource.js:1100 — async update rejects rather than throws on validation
RESOLVED — packages/kolibri/apiResource.js:1202 — useRetrieve inheriting useFetch's list-shaped handling
RESOLVED — packages/kolibri/apiResource.js:1313 — praise: monotonic writeCount token mirrors the read-side guard
RESOLVED — packages/kolibri/apiResource.js:1237 — useList could only page endpoints emitting a more object
RESOLVED — packages/kolibri/apiResource.js — baseline never invalidated when the bound id changes
RESOLVED — packages/kolibri/apiResource.js — private non-reactive baseline forecloses the hand-rolled dirty check
RESOLVED — packages/kolibri/apiResource.js:1029 — no opt-out of GET coalescing for a post-write refetch
RESOLVED — packages/kolibri/tests/api-resource.spec.js:2020 — useRetrieve(onSuccess) → useUpdate(setBaseline) untested end to end
RESOLVED — packages/kolibri/apiResource.js:1287 — useUpdate also creates, which the name hides
RESOLVED — packages/kolibri/apiResource.js:1022 — network failure replaced the real error with a TypeError
RESOLVED — packages/kolibri/apiResource.js — documented wiring left isDirty true on load
RESOLVED — packages/kolibri/apiResource.js:1034 — praise: coalescing callers get their own copy, originator keeps the raw payload
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| // Log any request failure before re-raising it, so it surfaces with request context the way | ||
| // the legacy Model/Collection layer did. This only intercepts to log - the rejection still | ||
| // propagates to the caller unchanged. | ||
| const logAndRethrow = error => { |
There was a problem hiding this comment.
nitpick: routing list/create through request also gives them logError, which they never had. The old list went via accessListEndpoint, which never called it (checked against upstream/develop).
Concretely: the un-gated 1s pollers at kolibri/plugins/facility/frontend/views/ImportCsvPage/index.vue:90 and .../DataPage/index.vue:352 both setInterval(this.refreshTaskList, 1000) over TaskResource.list(). During a server outage that is now one console.groupCollapsed + console.trace block per second. That sits on top of the logging.error those actions already produce.
The comment above says this is deliberate. Consistency with Model/Collection is a fair reason to keep it. Just worth a moment's thought about whether the polled read paths want it.
There was a problem hiding this comment.
I think that'd be fine
| count.value = response.count || null; | ||
| // Page-number paginated endpoints emit `page`/`total_pages` instead of a `more` cursor; | ||
| // surface them so consumers can drive jump-to-page navigation via `fetchData`. | ||
| page.value = response.page ?? null; |
There was a problem hiding this comment.
praise: reading page/total_pages straight through, instead of synthesising a more cursor, keeps hasMore/fetchMore honest for the shape that genuinely can't append. The spec also asserts that a more-cursor response leaves page/totalPages null — the half that would otherwise rot silently.
3 participants
Summary
useFetchfromkolibri-commontokolibriso that we can use it on apiResource.js.requestmethod that other custom actions can build on top of.createandlistsignatures were already present on theResourceclass, they were overridden by the new methods, and a small clean-up was needed across the codebase.References
Closes #15056.
Reviewer guidance
listandcreatemigrated methods.AI usage
Fed Claude with the general project plan and specific instructions for this PR, and refined the result iteratively to polish the new methods.