Feat lit-query: add initial lit-query package#10501
Open
chughgaurav wants to merge 6 commits intoTanStack:mainfrom
Open
Feat lit-query: add initial lit-query package#10501chughgaurav wants to merge 6 commits intoTanStack:mainfrom
chughgaurav wants to merge 6 commits intoTanStack:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant Host as Lit Host
participant Controller as Query Controller
participant Context as Lit Context
participant Client as QueryClient
Host->>Controller: addController(host)
Controller->>Controller: queueMicrotask(onConnected)
Host->>Controller: hostConnected()
Controller->>Context: dispatch ContextEvent(queryClientContext)
Context-->>Controller: resolved QueryClient or undefined
alt client available
Controller->>Client: create Observer & subscribe
Client-->>Controller: emit initial result/updates
Controller->>Host: requestUpdate()
else missing client
Controller->>Controller: set missing-client placeholder
end
Host->>Controller: hostDisconnected()
Controller->>Client: unsubscribe observer (if any)
sequenceDiagram
participant Runner as Playwright Runner
participant DevServer as Dev Server (pnpm/vite)
participant Browser as Chromium
participant App as Demo App
Runner->>DevServer: ensureServer() (probe, spawn if needed)
DevServer-->>Runner: ready or error
Runner->>Browser: launch()
Browser->>App: navigate(baseUrl)
App-->>Browser: render UI / respond to API
Browser->>Runner: scenario assertions (waitForText, actions)
alt failure
Runner->>Browser: capture screenshot & HTML
end
Runner->>Browser: close()
Runner->>DevServer: stopServer()
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (4)
packages/lit-query/src/useIsFetching.ts (1)
13-108: Extract the shared counter-controller lifecycle.
IsFetchingControlleris now almost identical toIsMutatingControllerinpackages/lit-query/src/useIsMutating.ts; the only real differences are which cache is subscribed to and whichQueryClientcounter is read. Pulling the shared sync/subscribe lifecycle into a small generic helper will keep future fixes from drifting between the two hooks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lit-query/src/useIsFetching.ts` around lines 13 - 108, The IsFetchingController and IsMutatingController share nearly identical lifecycle logic (constructor, syncClient, subscribe/unsubscribe, onConnected/onDisconnected/onHostUpdate/onQueryClientChanged); extract that into a small reusable generic helper or base class (e.g., SharedCounterController or lifecycle mixin) that accepts the cache-subscribe function and the counter-read function as parameters, then refactor IsFetchingController and IsMutatingController to delegate to that helper: keep unique methods computeValue (or the counter-read lambda) and the cache selection (queryClient.getQueryCache() vs queryClient.getMutationCache()) as injectable dependencies, reuse tryGetQueryClient/connectedState from BaseController, and ensure subscribe() uses the injected cache subscribe and the result setter from BaseController so both controllers share the sync/subscribe lifecycle.packages/lit-query/examples/lit-query-pagination-app/vite.config.ts (1)
5-14: Optional: deduplicate identicalserver/previewsettings.This is correct as-is; a tiny DRY refactor can reduce future drift.
♻️ Proposed refactor
import { defineConfig } from 'vite' import { DEMO_PORT } from './config/ports.js' +const localServerConfig = () => ({ + host: '127.0.0.1', + port: DEMO_PORT, + strictPort: true, +}) + export default defineConfig({ - server: { - host: '127.0.0.1', - port: DEMO_PORT, - strictPort: true, - }, - preview: { - host: '127.0.0.1', - port: DEMO_PORT, - strictPort: true, - }, + server: localServerConfig(), + preview: localServerConfig(), })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lit-query/examples/lit-query-pagination-app/vite.config.ts` around lines 5 - 14, Duplicate server and preview configs (server and preview) both use identical settings; extract the shared settings into a single constant (e.g., commonHostConfig or hostConfig) that references DEMO_PORT and then use that constant for both server and preview (via direct assignment or spreading) so changes to host, port or strictPort only need to be made in one place.packages/lit-query/scripts/check-cjs-types-smoke.mjs (1)
46-47: Add defensive check fornpm pack --jsonoutput.If
npm packfails or returns an empty array, the destructuringconst [{ filename }]will throw an unclear error. Consider adding validation.♻️ Proposed defensive parsing
- const [{ filename }] = JSON.parse(stdout) + const packResults = JSON.parse(stdout) + if (!Array.isArray(packResults) || packResults.length === 0 || !packResults[0].filename) { + throw new Error(`Unexpected npm pack output: ${stdout}`) + } + const [{ filename }] = packResults return resolve(destination, filename)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lit-query/scripts/check-cjs-types-smoke.mjs` around lines 46 - 47, The current destructuring const [{ filename }] = JSON.parse(stdout) can throw if npm pack returns empty or invalid JSON; change parse logic in this module to validate the parsed value before destructuring: parse stdout with JSON.parse(stdout), ensure the result is an array with at least one element and that result[0].filename is a string, and only then call resolve(destination, filename); otherwise throw or return a clear error indicating npm pack returned no artifacts (include stdout in the error for debugging). Ensure checks are applied where JSON.parse(stdout) and resolve(destination, filename) are used so the code fails with a clear message instead of an uncaught destructuring error.packages/lit-query/src/tests/context-provider.test.ts (1)
16-31: Consider adding afterEach cleanup for DOM elements.The test appends elements to
document.bodyand removes them manually, but if an assertion fails beforeprovider.remove(), the element remains in the DOM and could affect subsequent tests.♻️ Suggested pattern for test isolation
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' // At describe level: let testContainer: HTMLDivElement beforeEach(() => { testContainer = document.createElement('div') document.body.append(testContainer) }) afterEach(() => { testContainer.remove() }) // Then in tests, append to testContainer instead: testContainer.append(provider)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lit-query/src/tests/context-provider.test.ts` around lines 16 - 31, Add test-level DOM isolation by creating a test container in beforeEach and cleaning it up in afterEach, then append the QueryClientProvider instance (provider) to that container instead of document.body; update the test suite around the use of QueryClientProvider, useQueryClient, and resolveQueryClient to rely on the testContainer so that provider.remove() is not required for cleanup and the DOM is always removed even if assertions fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/lit-query/examples/lit-query-e2e-app/config/port.js`:
- Around line 9-12: The port validation currently uses Number.parseInt on
envPort which accepts prefixes like "4173abc"; update the logic around
envPort/parsedPort/isValidPort to first verify envPort contains only digits
(e.g., /^\d+$/) before parsing, then parse to a number and enforce
Number.isInteger(parsedPort) && parsedPort > 0 && parsedPort <= 65535 so
non-numeric suffixes are rejected; adjust any error/default branch to rely on
this stricter check.
In `@packages/lit-query/examples/lit-query-pagination-app/config/ports.js`:
- Around line 10-12: The current logic uses Number.parseInt(rawValue, 10) and
only checks the numeric range, which accepts strings with trailing characters;
update the validation in ports.js so rawValue is first tested against a
full-digit regex (e.g., /^\d+$/) before parsing, then parse with
Number.parseInt(rawValue, 10) into parsed and keep the existing range check
(parsed >= 1 && parsed <= 65535) to set valid; reference the variables rawValue,
parsed and valid when making this change.
In `@packages/lit-query/examples/lit-query-pagination-app/server/index.mjs`:
- Around line 37-60: Both parsePositiveInt and parseNonNegativeInt currently use
Number.parseInt which accepts partial numeric strings like "1abc"; change them
to first verify the entire rawValue is a pure integer string (e.g., use a regex
that matches only digits for non-negative values, and digits for positive
checks) before converting, then parse with Number.parseInt or Number() and apply
the existing bounds checks; ensure the functions still return fallback when
rawValue is null/empty and return undefined for malformed or out-of-range
values.
In `@packages/lit-query/examples/lit-query-pagination-app/src/api.ts`:
- Around line 68-85: readJsonOrThrow currently catches JSON parse errors and
returns null, hiding malformed/empty 2xx bodies; change its logic so you do not
swallow parse errors for successful responses: attempt to parse the body with
response.json() without a blanket .catch for OK responses (let the parse error
propagate or explicitly throw a descriptive error for malformed JSON when
response.ok), while preserving tolerant behavior for non-ok responses (e.g., try
to parse error payload and if parsing fails fall back to the provided
fallbackMessage). Update the implementation in readJsonOrThrow to parse and
handle errors accordingly so successful responses never return null due to
swallowed JSON parse failures.
In `@packages/lit-query/examples/lit-query-ssr-app/scripts/dev.mjs`:
- Around line 23-39: The child process `server` can emit an `error` (spawn
failure) that will never resolve the awaited `once(server, 'exit')`, so modify
the shutdown/wait logic around `server` (and keep `stopServer`) to race the
`'error'` event with `'exit'`/`'close'`: create Promises using `once(server,
'error')` and `once(server, 'exit')` (or `'close'`) and use Promise.race to
await whichever happens first, then on `error` ensure you log/throw the error so
the script fails fast instead of hanging.
In
`@packages/lit-query/examples/lit-query-ssr-app/scripts/run-script-with-public-origin.mjs`:
- Around line 32-38: The code awaits the child process 'exit' event which can
hang if a spawn 'error' occurs; update the wait to listen for 'close' instead of
'exit' (i.e., change the await once(child, 'exit') usage to once(child,
'close')) and keep the existing child.on('error', ...) handler so
process.exitCode is set on errors; this ensures the wrapper always proceeds
after either 'exit' or 'error' and that stdio is closed (look for the
variables/function names child and once and the lines awaiting the 'exit'
event).
In `@packages/lit-query/examples/lit-query-ssr-app/src/api.ts`:
- Around line 51-57: The queryFn in createDataQueryOptions currently ignores the
TanStack Query context and doesn't forward the AbortSignal, breaking
cancellation; update the queryFn signature to accept the context parameter (from
createDataQueryOptions → queryFn: async ({ signal }) => { ... }) and pass that
signal into fetch(resolveApiUrl('/api/data', apiBaseUrl), { signal }); keep
using DATA_QUERY_KEY and return readJson<DataResponse>(response) unchanged so
cancellation is preserved when unmounting or aborting queries.
In `@packages/lit-query/package.json`:
- Around line 85-93: The package manifest currently pins "@tanstack/query-core"
to a published version under "dependencies"; replace that entry with a workspace
link by setting "@tanstack/query-core": "workspace:*" in the package.json
dependencies (keep the peerDependencies constraint as-is) so local monorepo
changes to query-core are used during builds and tests; update the
"dependencies" object where "@tanstack/query-core" appears to the workspace
spec.
In `@packages/lit-query/README.md`:
- Around line 216-233: The README's "Quality Gates" section references missing
docs files (`docs/TEST_MATRIX.md`, `docs/TEST_MATRIX_INTEGRATION.md`,
`docs/TEST_MATRIX_PERF.md`, `docs/RFC-v4.1.md`); either add those document files
with the expected content or remove/replace those links in the README to avoid
broken references. Update the section in packages/lit-query/README.md to either
point to existing documents or to inline the current gate commands (e.g., the
`Current local quality gate` and `Full local demo gate`) so contributors aren't
directed to non-existent files.
In `@packages/lit-query/scripts/check-package.mjs`:
- Around line 8-11: The execFileSync call that assigns rawOutput uses the
literal 'npm' which fails on Windows because the executable is 'npm.cmd'; update
the invocation in check-package.mjs to pick the correct binary based on platform
(e.g., determine npmBinary = process.platform === 'win32' ? 'npm.cmd' : 'npm' or
derive from process.env.npm_execpath) and call execFileSync(npmBinary,
['pack','--dry-run','--json'], { cwd: repoRoot, encoding: 'utf8' }) so the
script works on Windows and POSIX systems; keep the same args and options and
only change the binary selection logic around the execFileSync usage that sets
rawOutput.
In `@packages/lit-query/src/createMutationController.ts`:
- Around line 55-83: The idle fallback mutation result returned in
createMutationController/createIdleMutationResult is missing the public
MutationObserverResult field isPaused, causing mutation().isPaused to be
undefined; add isPaused: false to the returned object literal (alongside
isIdle/isPending/isSuccess/isError) so the shape matches
MutationObserverResult<TData,TError,TVariables,TOnMutateResult> and consumers
receive a boolean in the idle state.
In `@packages/lit-query/src/tests/client-switch-controllers.test.ts`:
- Around line 312-320: Tests currently call custom-element lifecycle methods
directly (consumer.connectedCallback() and consumer.disconnectedCallback()),
which double-runs controller hooks; instead remove those manual calls and use
DOM operations to drive lifecycle: use providerA.removeChild(consumer) and
providerB.append(consumer) (or append consumer to document.body to simulate
“connected without a provider”) so the browser invokes
disconnectedCallback/connectedCallback naturally; update the assertions around
consumer.mutation.mutateAsync to run after the DOM-based attach/detach and apply
the same fix to the other occurrences mentioned (lines near 377-389 and 451-465)
referencing providerA, providerB and consumer.
In `@packages/lit-query/src/tests/counters-and-state.test.ts`:
- Line 1: explicitCountersClient is shared mutable state causing leaks if a test
fails before the manual cleanup at the end of the file; move its teardown into
an afterEach hook so every test resets it. Add an afterEach that checks
explicitCountersClient and, if set, calls its close/dispose/shutdown method (or
nulls it) and then assigns undefined (or resets to initial value); remove the
one-off cleanup at the end of the file so teardown always runs even on test
failures.
In `@packages/lit-query/src/tests/infinite-and-options.test.ts`:
- Line 1: The tests leak shared state by cleaning up explicitInfiniteClient
inline; move the explicitInfiniteClient teardown into an afterEach hook so it's
always disposed even if a test fails (reference the explicitInfiniteClient
variable and the test suite in infinite-and-options.test.ts), i.e., add an
afterEach that checks for and closes/disposes explicitInfiniteClient and resets
it to undefined, and remove the inline cleanup to ensure each test runs with a
fresh provider/client.
In `@packages/lit-query/src/tests/testHost.ts`:
- Around line 86-97: The waitFor helper currently only checks boolean returns
and fails to handle assertion functions that throw or return undefined; update
waitFor to support sync or async assertions by calling await
Promise.resolve(assertion()) inside a try/catch loop, treat any truthy
(non-false) return value as success and return immediately, and on exceptions
capture the thrown error into a lastError variable and continue polling until
timeout, at which point throw the lastError (or a timeout Error if none) so
failing expectations bubble correctly; modify the function named waitFor
accordingly to implement this behavior.
---
Nitpick comments:
In `@packages/lit-query/examples/lit-query-pagination-app/vite.config.ts`:
- Around line 5-14: Duplicate server and preview configs (server and preview)
both use identical settings; extract the shared settings into a single constant
(e.g., commonHostConfig or hostConfig) that references DEMO_PORT and then use
that constant for both server and preview (via direct assignment or spreading)
so changes to host, port or strictPort only need to be made in one place.
In `@packages/lit-query/scripts/check-cjs-types-smoke.mjs`:
- Around line 46-47: The current destructuring const [{ filename }] =
JSON.parse(stdout) can throw if npm pack returns empty or invalid JSON; change
parse logic in this module to validate the parsed value before destructuring:
parse stdout with JSON.parse(stdout), ensure the result is an array with at
least one element and that result[0].filename is a string, and only then call
resolve(destination, filename); otherwise throw or return a clear error
indicating npm pack returned no artifacts (include stdout in the error for
debugging). Ensure checks are applied where JSON.parse(stdout) and
resolve(destination, filename) are used so the code fails with a clear message
instead of an uncaught destructuring error.
In `@packages/lit-query/src/tests/context-provider.test.ts`:
- Around line 16-31: Add test-level DOM isolation by creating a test container
in beforeEach and cleaning it up in afterEach, then append the
QueryClientProvider instance (provider) to that container instead of
document.body; update the test suite around the use of QueryClientProvider,
useQueryClient, and resolveQueryClient to rely on the testContainer so that
provider.remove() is not required for cleanup and the DOM is always removed even
if assertions fail.
In `@packages/lit-query/src/useIsFetching.ts`:
- Around line 13-108: The IsFetchingController and IsMutatingController share
nearly identical lifecycle logic (constructor, syncClient,
subscribe/unsubscribe,
onConnected/onDisconnected/onHostUpdate/onQueryClientChanged); extract that into
a small reusable generic helper or base class (e.g., SharedCounterController or
lifecycle mixin) that accepts the cache-subscribe function and the counter-read
function as parameters, then refactor IsFetchingController and
IsMutatingController to delegate to that helper: keep unique methods
computeValue (or the counter-read lambda) and the cache selection
(queryClient.getQueryCache() vs queryClient.getMutationCache()) as injectable
dependencies, reuse tryGetQueryClient/connectedState from BaseController, and
ensure subscribe() uses the injected cache subscribe and the result setter from
BaseController so both controllers share the sync/subscribe lifecycle.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 33495bdd-700c-4c8d-b2ea-f30ed64c2f1e
📒 Files selected for processing (95)
packages/lit-query/.editorconfigpackages/lit-query/.npmignorepackages/lit-query/.prettierignorepackages/lit-query/README.mdpackages/lit-query/eslint.config.jspackages/lit-query/examples/lit-query-e2e-app/basic-query.htmlpackages/lit-query/examples/lit-query-e2e-app/config/port.d.tspackages/lit-query/examples/lit-query-e2e-app/config/port.jspackages/lit-query/examples/lit-query-e2e-app/e2e/lib.mjspackages/lit-query/examples/lit-query-e2e-app/e2e/lifecycle-contract.mjspackages/lit-query/examples/lit-query-e2e-app/e2e/lifecycle-reconnect.mjspackages/lit-query/examples/lit-query-e2e-app/e2e/mutation-error.mjspackages/lit-query/examples/lit-query-e2e-app/e2e/query-error.mjspackages/lit-query/examples/lit-query-e2e-app/e2e/refetch-button.mjspackages/lit-query/examples/lit-query-e2e-app/e2e/smoke.mjspackages/lit-query/examples/lit-query-e2e-app/index.htmlpackages/lit-query/examples/lit-query-e2e-app/lifecycle-contract.htmlpackages/lit-query/examples/lit-query-e2e-app/mutation.htmlpackages/lit-query/examples/lit-query-e2e-app/package.jsonpackages/lit-query/examples/lit-query-e2e-app/src/basic-query.tspackages/lit-query/examples/lit-query-e2e-app/src/lifecycle-contract.tspackages/lit-query/examples/lit-query-e2e-app/src/main.tspackages/lit-query/examples/lit-query-e2e-app/src/mutation.tspackages/lit-query/examples/lit-query-e2e-app/src/todoApi.tspackages/lit-query/examples/lit-query-e2e-app/tsconfig.jsonpackages/lit-query/examples/lit-query-e2e-app/vite.config.tspackages/lit-query/examples/lit-query-pagination-app/config/ports.d.tspackages/lit-query/examples/lit-query-pagination-app/config/ports.jspackages/lit-query/examples/lit-query-pagination-app/e2e/boundary.mjspackages/lit-query/examples/lit-query-pagination-app/e2e/error-recovery.mjspackages/lit-query/examples/lit-query-pagination-app/e2e/lib.mjspackages/lit-query/examples/lit-query-pagination-app/e2e/mutations.mjspackages/lit-query/examples/lit-query-pagination-app/e2e/prefetch.mjspackages/lit-query/examples/lit-query-pagination-app/e2e/smoke.mjspackages/lit-query/examples/lit-query-pagination-app/index.htmlpackages/lit-query/examples/lit-query-pagination-app/package.jsonpackages/lit-query/examples/lit-query-pagination-app/scripts/dev.mjspackages/lit-query/examples/lit-query-pagination-app/server/index.mjspackages/lit-query/examples/lit-query-pagination-app/src/api.tspackages/lit-query/examples/lit-query-pagination-app/src/main.tspackages/lit-query/examples/lit-query-pagination-app/src/vite-env.d.tspackages/lit-query/examples/lit-query-pagination-app/tsconfig.jsonpackages/lit-query/examples/lit-query-pagination-app/vite.config.tspackages/lit-query/examples/lit-query-ssr-app/config/ports.d.tspackages/lit-query/examples/lit-query-ssr-app/config/ports.jspackages/lit-query/examples/lit-query-ssr-app/e2e/error-recovery.mjspackages/lit-query/examples/lit-query-ssr-app/e2e/lib.mjspackages/lit-query/examples/lit-query-ssr-app/e2e/refreshing.mjspackages/lit-query/examples/lit-query-ssr-app/e2e/ssr-smoke.mjspackages/lit-query/examples/lit-query-ssr-app/index.htmlpackages/lit-query/examples/lit-query-ssr-app/package.jsonpackages/lit-query/examples/lit-query-ssr-app/scripts/dev.mjspackages/lit-query/examples/lit-query-ssr-app/scripts/run-script-with-public-origin.mjspackages/lit-query/examples/lit-query-ssr-app/server/index.mjspackages/lit-query/examples/lit-query-ssr-app/src/api.tspackages/lit-query/examples/lit-query-ssr-app/src/app.tspackages/lit-query/examples/lit-query-ssr-app/src/main.tspackages/lit-query/examples/lit-query-ssr-app/tsconfig.jsonpackages/lit-query/examples/lit-query-ssr-app/vite.config.tspackages/lit-query/package.jsonpackages/lit-query/prettier.config.jspackages/lit-query/scripts/check-cjs-types-smoke.mjspackages/lit-query/scripts/check-package.mjspackages/lit-query/scripts/l3-stress.mjspackages/lit-query/scripts/measure-bundle.mjspackages/lit-query/scripts/write-cjs-package.mjspackages/lit-query/src/QueryClientProvider.tspackages/lit-query/src/accessor.tspackages/lit-query/src/context.tspackages/lit-query/src/controllers/BaseController.tspackages/lit-query/src/createInfiniteQueryController.tspackages/lit-query/src/createMutationController.tspackages/lit-query/src/createQueriesController.tspackages/lit-query/src/createQueryController.tspackages/lit-query/src/index.tspackages/lit-query/src/infiniteQueryOptions.tspackages/lit-query/src/mutationOptions.tspackages/lit-query/src/queryOptions.tspackages/lit-query/src/tests/client-switch-controllers.test.tspackages/lit-query/src/tests/context-provider.test.tspackages/lit-query/src/tests/counters-and-state.test.tspackages/lit-query/src/tests/infinite-and-options.test.tspackages/lit-query/src/tests/mutation-controller.test.tspackages/lit-query/src/tests/queries-controller.test.tspackages/lit-query/src/tests/query-controller.test.tspackages/lit-query/src/tests/testHost.tspackages/lit-query/src/tests/type-inference.test.tspackages/lit-query/src/types.tspackages/lit-query/src/useIsFetching.tspackages/lit-query/src/useIsMutating.tspackages/lit-query/src/useMutationState.tspackages/lit-query/tsconfig.build.cjs.jsonpackages/lit-query/tsconfig.build.jsonpackages/lit-query/tsconfig.jsonpackages/lit-query/vitest.config.ts
Comment on lines
+9
to
+12
| const parsedPort = Number.parseInt(envPort, 10) | ||
| const isValidPort = | ||
| Number.isInteger(parsedPort) && parsedPort > 0 && parsedPort <= 65535 | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In JavaScript, does Number.parseInt("4173abc", 10) return 4173, and what is the recommended strict validation pattern for environment variable ports?
💡 Result:
Yes, Number.parseInt("4173abc", 10) returns 4173. This is because parseInt parses until it encounters a non-digit character in the specified radix (base 10 here), ignoring trailing invalid characters. The recommended strict validation pattern for environment variable ports in Node.js combines parsing with validation to ensure the entire string consists only of valid digits (no trailing junk, decimals, etc.) and the value is in the valid port range (typically 0-65535, often >0 and <=65535 for TCP ports). A common pattern, seen in Express generator's normalizePort and various best practices: function normalizePort(val) { const port = parseInt(val, 10); if (isNaN(port)) return val; // Allow named pipes if (port >= 0) return port; return false; } For stricter integer-only validation (rejecting anything not purely digits): - Test if the string matches /^\d+$/ (optionally allowing leading + or - if needed, but ports are unsigned). - Then parseInt and check range. Example strict port parser: function parseStrictPort(str) { if (!/^\d+$/.test(str)) return NaN; const port = Number.parseInt(str, 10); return (port >= 0 && port <= 65535) ? port : NaN; } Usage: const port = parseStrictPort(process.env.PORT); if (isNaN(port)) { throw new Error('Invalid PORT'); }
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/parseInt
- 2: https://udn.realityripple.com/docs/Web/JavaScript/Reference/Global_Objects/parseInt
- 3: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseInt?retiredLocale=de
- 4: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseInt
- 5: https://www.techonthenet.com/js/number_parseint.php
- 6: https://stackoverflow.com/questions/43417975/parse-an-integer-and-only-an-integer-in-javascript
- 7: https://stackoverflow.com/questions/10834796/validate-that-a-string-is-a-positive-integer
- 8: https://stackoverflow.com/questions/37674069/javascript-check-if-string-can-be-converted-to-integer
- 9: https://stackoverflow.com/questions/55988881/what-does-normalizeport-function-do-in-nodejs
🏁 Script executed:
cat -n packages/lit-query/examples/lit-query-e2e-app/config/port.js | sed -n '1,20p'Repository: TanStack/query
Length of output: 649
Use strict numeric validation for DEMO_PORT.
Lines 9-12 currently accept values like "4173abc" because Number.parseInt is prefix-tolerant, silently accepting invalid environment input. Enforce digit-only validation before parsing.
Proposed fix
- const parsedPort = Number.parseInt(envPort, 10)
- const isValidPort =
- Number.isInteger(parsedPort) && parsedPort > 0 && parsedPort <= 65535
+ const isDigitsOnly = /^\d+$/.test(envPort)
+ const parsedPort = Number(envPort)
+ const isValidPort =
+ isDigitsOnly && Number.isInteger(parsedPort) && parsedPort > 0 && parsedPort <= 65535📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const parsedPort = Number.parseInt(envPort, 10) | |
| const isValidPort = | |
| Number.isInteger(parsedPort) && parsedPort > 0 && parsedPort <= 65535 | |
| const isDigitsOnly = /^\d+$/.test(envPort) | |
| const parsedPort = Number(envPort) | |
| const isValidPort = | |
| isDigitsOnly && Number.isInteger(parsedPort) && parsedPort > 0 && parsedPort <= 65535 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/lit-query/examples/lit-query-e2e-app/config/port.js` around lines 9
- 12, The port validation currently uses Number.parseInt on envPort which
accepts prefixes like "4173abc"; update the logic around
envPort/parsedPort/isValidPort to first verify envPort contains only digits
(e.g., /^\d+$/) before parsing, then parse to a number and enforce
Number.isInteger(parsedPort) && parsedPort > 0 && parsedPort <= 65535 so
non-numeric suffixes are rejected; adjust any error/default branch to rely on
this stricter check.
Comment on lines
+10
to
+12
| const parsed = Number.parseInt(rawValue, 10) | ||
| const valid = Number.isInteger(parsed) && parsed >= 1 && parsed <= 65_535 | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does JavaScript parseInt accept trailing non-numeric characters, and how should a port env var be validated to ensure the whole string is numeric?
💡 Result:
Yes, JavaScript's parseInt accepts trailing non-numeric characters and parses only the leading numeric portion, returning the integer value up to the first invalid character. For example, parseInt("123abc", 10) returns 123. To validate a port environment variable (e.g., process.env.PORT) and ensure the whole string is numeric (a valid integer port 0-65535), use this approach: 1. Trim whitespace. 2. Parse with parseInt(string, 10). 3. Check if the parsed number equals the original trimmed string (ensures no trailing invalid chars). 4. Verify it's an integer within port range. Example code: function validatePort(str) { const trimmed = str.trim; if (trimmed === '') return false; const num = parseInt(trimmed, 10); return num >= 0 && num <= 65535 && trimmed === num.toString; } // Usage: const portStr = process.env.PORT; const port = parseInt(portStr, 10); if (validatePort(portStr) && port !== NaN) { // Use port } else { // Handle invalid: use default like 3000 } This catches cases like "3000foo" (parses to 3000 but validation fails), "3000.5" (parses to 3000 but string mismatch), empty, non-numeric. Alternatively, use regex /^\d+$/ for digits-only check before parsing, or libraries like envalid.port or zod.coerce.number.int.min(0).max(65535).
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseInt.
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseInt
- 3: https://stackoverflow.com/questions/21060645/js-validation-ipport
- 4: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/isInteger
- 5: https://stackoverflow.com/questions/37674069/javascript-check-if-string-can-be-converted-to-integer
- 6: https://stackoverflow.com/questions/10834796/validate-that-a-string-is-a-positive-integer
🏁 Script executed:
find . -name "ports.js" -type f | head -20Repository: TanStack/query
Length of output: 192
🏁 Script executed:
if [ -f "packages/lit-query/examples/lit-query-pagination-app/config/ports.js" ]; then
echo "=== File exists ==="
wc -l "packages/lit-query/examples/lit-query-pagination-app/config/ports.js"
echo "=== Lines 1-20 ==="
sed -n '1,20p' "packages/lit-query/examples/lit-query-pagination-app/config/ports.js"
fiRepository: TanStack/query
Length of output: 616
Tighten env port parsing to reject malformed strings with trailing characters.
Number.parseInt() accepts strings with trailing non-numeric characters (e.g., "4184foo" parses as 4184). Line 10-12 would incorrectly validate such inputs since it only checks if the parsed result is a valid integer in range, not if the entire input string is numeric.
Use a regex check before parsing to ensure the full string contains only digits:
Proposed fix
- const parsed = Number.parseInt(rawValue, 10)
- const valid = Number.isInteger(parsed) && parsed >= 1 && parsed <= 65_535
+ const isDigitsOnly = /^\d+$/.test(rawValue)
+ const parsed = Number(rawValue)
+ const valid = isDigitsOnly && Number.isInteger(parsed) && parsed >= 1 && parsed <= 65_535🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/lit-query/examples/lit-query-pagination-app/config/ports.js` around
lines 10 - 12, The current logic uses Number.parseInt(rawValue, 10) and only
checks the numeric range, which accepts strings with trailing characters; update
the validation in ports.js so rawValue is first tested against a full-digit
regex (e.g., /^\d+$/) before parsing, then parse with Number.parseInt(rawValue,
10) into parsed and keep the existing range check (parsed >= 1 && parsed <=
65535) to set valid; reference the variables rawValue, parsed and valid when
making this change.
Comment on lines
+37
to
+60
| function parsePositiveInt(rawValue, fallback) { | ||
| if (rawValue == null || rawValue === '') { | ||
| return fallback | ||
| } | ||
|
|
||
| const parsed = Number.parseInt(rawValue, 10) | ||
| if (!Number.isInteger(parsed) || parsed < 1) { | ||
| return undefined | ||
| } | ||
|
|
||
| return parsed | ||
| } | ||
|
|
||
| function parseNonNegativeInt(rawValue, fallback) { | ||
| if (rawValue == null || rawValue === '') { | ||
| return fallback | ||
| } | ||
|
|
||
| const parsed = Number.parseInt(rawValue, 10) | ||
| if (!Number.isInteger(parsed) || parsed < 0) { | ||
| return undefined | ||
| } | ||
|
|
||
| return parsed |
Contributor
There was a problem hiding this comment.
Reject partial numeric strings here.
Number.parseInt will accept inputs like "1abc" and "10ms", so these helpers currently treat malformed query params as valid. If the endpoint is meant to validate integers, use a full-string check before converting.
Safer integer parsing
function parsePositiveInt(rawValue, fallback) {
if (rawValue == null || rawValue === '') {
return fallback
}
- const parsed = Number.parseInt(rawValue, 10)
- if (!Number.isInteger(parsed) || parsed < 1) {
+ if (!/^[1-9]\d*$/.test(rawValue)) {
return undefined
}
+ const parsed = Number(rawValue)
return parsed
}
function parseNonNegativeInt(rawValue, fallback) {
if (rawValue == null || rawValue === '') {
return fallback
}
- const parsed = Number.parseInt(rawValue, 10)
- if (!Number.isInteger(parsed) || parsed < 0) {
+ if (!/^\d+$/.test(rawValue)) {
return undefined
}
+ const parsed = Number(rawValue)
return parsed
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/lit-query/examples/lit-query-pagination-app/server/index.mjs` around
lines 37 - 60, Both parsePositiveInt and parseNonNegativeInt currently use
Number.parseInt which accepts partial numeric strings like "1abc"; change them
to first verify the entire rawValue is a pure integer string (e.g., use a regex
that matches only digits for non-negative values, and digits for positive
checks) before converting, then parse with Number.parseInt or Number() and apply
the existing bounds checks; ensure the functions still return fallback when
rawValue is null/empty and return undefined for malformed or out-of-range
values.
Comment on lines
+312
to
+320
| providerA.removeChild(consumer) | ||
| await new Promise((resolve) => setTimeout(resolve, 0)) | ||
| consumer.connectedCallback() | ||
| await expect(consumer.mutation.mutateAsync(2)).rejects.toThrow( | ||
| /No QueryClient available/, | ||
| ) | ||
|
|
||
| consumer.disconnectedCallback() | ||
| providerB.append(consumer) |
Contributor
There was a problem hiding this comment.
Avoid calling custom-element lifecycle callbacks manually.
These tests are putting the host into states the browser never creates. removeChild/append already drive disconnectedCallback and connectedCallback; the extra manual calls double-run controller hooks and can mask real client-resolution or observer-cleanup bugs. To test “connected without a provider”, append the consumer to document.body instead of invoking the callbacks directly.
Also applies to: 377-389, 451-465
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/lit-query/src/tests/client-switch-controllers.test.ts` around lines
312 - 320, Tests currently call custom-element lifecycle methods directly
(consumer.connectedCallback() and consumer.disconnectedCallback()), which
double-runs controller hooks; instead remove those manual calls and use DOM
operations to drive lifecycle: use providerA.removeChild(consumer) and
providerB.append(consumer) (or append consumer to document.body to simulate
“connected without a provider”) so the browser invokes
disconnectedCallback/connectedCallback naturally; update the assertions around
consumer.mutation.mutateAsync to run after the DOM-based attach/detach and apply
the same fix to the other occurrences mentioned (lines near 377-389 and 451-465)
referencing providerA, providerB and consumer.
| @@ -0,0 +1,604 @@ | |||
| import { describe, expect, it } from 'vitest' | |||
Contributor
There was a problem hiding this comment.
Move explicitCountersClient cleanup into afterEach.
explicitCountersClient is shared mutable state. If an assertion fails before Line 211 runs, later tests in this file stop covering the provider path and start using the leaked explicit client instead.
🧪 Suggested test hardening
-import { describe, expect, it } from 'vitest'
+import { afterEach, describe, expect, it } from 'vitest'
import { QueryClient } from '@tanstack/query-core'
@@
let explicitCountersClient: QueryClient | undefined
+
+afterEach(() => {
+ explicitCountersClient = undefined
+})Also applies to: 22-22
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/lit-query/src/tests/counters-and-state.test.ts` at line 1,
explicitCountersClient is shared mutable state causing leaks if a test fails
before the manual cleanup at the end of the file; move its teardown into an
afterEach hook so every test resets it. Add an afterEach that checks
explicitCountersClient and, if set, calls its close/dispose/shutdown method (or
nulls it) and then assigns undefined (or resets to initial value); remove the
one-off cleanup at the end of the file so teardown always runs even on test
failures.
| @@ -0,0 +1,377 @@ | |||
| import { describe, expect, it } from 'vitest' | |||
Contributor
There was a problem hiding this comment.
Move explicitInfiniteClient cleanup into afterEach.
This file has the same shared-state leak risk: if LC-INF-02 fails before Line 129, later tests can accidentally bypass provider coverage by reusing the leaked explicit client.
🧪 Suggested test hardening
-import { describe, expect, it } from 'vitest'
+import { afterEach, describe, expect, it } from 'vitest'
import { QueryClient } from '@tanstack/query-core'
@@
let explicitInfiniteClient: QueryClient | undefined
+
+afterEach(() => {
+ explicitInfiniteClient = undefined
+})Also applies to: 23-23
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/lit-query/src/tests/infinite-and-options.test.ts` at line 1, The
tests leak shared state by cleaning up explicitInfiniteClient inline; move the
explicitInfiniteClient teardown into an afterEach hook so it's always disposed
even if a test fails (reference the explicitInfiniteClient variable and the test
suite in infinite-and-options.test.ts), i.e., add an afterEach that checks for
and closes/disposes explicitInfiniteClient and resets it to undefined, and
remove the inline cleanup to ensure each test runs with a fresh provider/client.
Comment on lines
+86
to
+97
| export async function waitFor( | ||
| assertion: () => boolean, | ||
| timeoutMs = 2000, | ||
| ): Promise<void> { | ||
| const startedAt = Date.now() | ||
| while (!assertion()) { | ||
| if (Date.now() - startedAt > timeoutMs) { | ||
| throw new Error(`Timed out waiting for assertion after ${timeoutMs}ms`) | ||
| } | ||
| await new Promise((resolve) => setTimeout(resolve, 10)) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
waitFor only handles boolean predicates right now.
A passing callback like () => expect(x).toBe(1) returns undefined, so this loop keeps polling until timeout. Thrown assertions also abort immediately instead of retrying, which makes the helper brittle for normal test usage.
💡 Suggested fix
export async function waitFor(
- assertion: () => boolean,
+ assertion: () => unknown,
timeoutMs = 2000,
): Promise<void> {
const startedAt = Date.now()
- while (!assertion()) {
- if (Date.now() - startedAt > timeoutMs) {
- throw new Error(`Timed out waiting for assertion after ${timeoutMs}ms`)
+ let lastError: unknown
+
+ while (Date.now() - startedAt <= timeoutMs) {
+ try {
+ const result = assertion()
+ if (result !== false) {
+ return
+ }
+ } catch (error) {
+ lastError = error
}
+
await new Promise((resolve) => setTimeout(resolve, 10))
}
+
+ throw lastError ?? new Error(`Timed out waiting for assertion after ${timeoutMs}ms`)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export async function waitFor( | |
| assertion: () => boolean, | |
| timeoutMs = 2000, | |
| ): Promise<void> { | |
| const startedAt = Date.now() | |
| while (!assertion()) { | |
| if (Date.now() - startedAt > timeoutMs) { | |
| throw new Error(`Timed out waiting for assertion after ${timeoutMs}ms`) | |
| } | |
| await new Promise((resolve) => setTimeout(resolve, 10)) | |
| } | |
| } | |
| export async function waitFor( | |
| assertion: () => unknown, | |
| timeoutMs = 2000, | |
| ): Promise<void> { | |
| const startedAt = Date.now() | |
| let lastError: unknown | |
| while (Date.now() - startedAt <= timeoutMs) { | |
| try { | |
| const result = assertion() | |
| if (result !== false) { | |
| return | |
| } | |
| } catch (error) { | |
| lastError = error | |
| } | |
| await new Promise((resolve) => setTimeout(resolve, 10)) | |
| } | |
| throw lastError ?? new Error(`Timed out waiting for assertion after ${timeoutMs}ms`) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/lit-query/src/tests/testHost.ts` around lines 86 - 97, The waitFor
helper currently only checks boolean returns and fails to handle assertion
functions that throw or return undefined; update waitFor to support sync or
async assertions by calling await Promise.resolve(assertion()) inside a
try/catch loop, treat any truthy (non-false) return value as success and return
immediately, and on exceptions capture the thrown error into a lastError
variable and continue polling until timeout, at which point throw the lastError
(or a timeout Error if none) so failing expectations bubble correctly; modify
the function named waitFor accordingly to implement this behavior.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/lit-query/src/useMutationState.ts (2)
113-118: Simplify condition:selectis always a function by type definition.Since
selectis typed as(mutation: Mutation) => TResult, thetypeofcheck is redundant—it will always be'function'whenselectis defined. Using!== undefinedis clearer.♻️ Suggested simplification
private shouldRefreshOnHostUpdate(): boolean { return ( typeof this.options.filters === 'function' || - typeof this.options.select === 'function' + this.options.select !== undefined ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lit-query/src/useMutationState.ts` around lines 113 - 118, The condition in shouldRefreshOnHostUpdate() uses typeof this.options.select === 'function' but select is statically typed as a function when present; change the check to test presence instead (e.g., this.options.select !== undefined) and keep the filters check as-is (typeof this.options.filters === 'function'); update the shouldRefreshOnHostUpdate method to use the presence check for options.select to simplify the logic and avoid a redundant typeof check.
54-58: Consider removing unnecessarysyncClient()call inonDisconnected.After unsubscribing on line 55-56, calling
syncClient()appears unnecessary—we're disconnecting so the client reference state doesn't need updating, and the return value is discarded.♻️ Suggested simplification
protected onDisconnected(): void { this.unsubscribe?.() this.unsubscribe = undefined - this.syncClient() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lit-query/src/useMutationState.ts` around lines 54 - 58, The onDisconnected method calls this.unsubscribe and then calls this.syncClient(), but since we're disconnecting the client the syncClient() call is unnecessary and its return value is ignored; remove the this.syncClient() call from the onDisconnected() method (leave the unsubscribe clearing logic intact) so only the unsubscribe cleanup runs in onDisconnected.packages/lit-query/src/createQueriesController.ts (2)
179-179: Consider usingundefinedinstead of empty array for initial result.The empty array
[]cast toTCombinedResultcould cause unexpected behavior ifTCombinedResultis not array-like (e.g., whencombinereturns an object). While this value is typically replaced before access, usingundefinedas the initial sentinel and handling it inreadCurrentwould be more type-safe.♻️ Suggested approach
- super(host, [] as unknown as TCombinedResult, queryClient) + super(host, undefined as unknown as TCombinedResult, queryClient)Then ensure
readCurrenthandles the undefined case, which it already does viaplaceholderInitializedtracking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lit-query/src/createQueriesController.ts` at line 179, The constructor currently passes an empty array cast to TCombinedResult into super (super(host, [] as unknown as TCombinedResult, queryClient)), which is unsafe if TCombinedResult isn't array-like; change that initial result to undefined (pass undefined as the initial sentinel) and ensure the class handles this by using the existing placeholderInitialized logic and readCurrent to return a proper value when the result is undefined; update the super call to use undefined and verify readCurrent and any consumers of the initial value (e.g., placeholderInitialized, readCurrent) correctly handle undefined.
346-361: Document the early-read handshake pattern.The error swallowing based on
placeholderRetryableFailureis clever for handling reads during class-field initialization (before accessors are ready), but this subtle timing behavior could confuse maintainers. Consider adding a brief inline comment explaining the microtask window.📝 Suggested documentation
readCurrent(): TCombinedResult { if (!this.queryClient && !this.observer && !this.placeholderInitialized) { try { - // Early reads can happen during class-field initialization, before - // accessors referenced by queries/combine are ready. Retry normally - // after construction finishes and only surface errors after that point. + // Early reads can happen during class-field initialization, before + // accessors referenced by queries/combine are ready. The queueMicrotask + // in the constructor gives a brief window where errors are swallowed, + // allowing the host to complete initialization before we surface errors. this.setPlaceholderResult() } catch (error) { if (!this.placeholderRetryableFailure) { throw error } + // Error swallowed during handshake window - will retry on next access } } return this.current }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lit-query/src/createQueriesController.ts` around lines 346 - 361, Add an inline comment in readCurrent explaining the early-read handshake: document that setPlaceholderResult is retried to handle reads during class-field initialization (a microtask window before accessors are wired), why placeholderRetryableFailure suppresses errors during that window, and when errors will be surfaced (after placeholderInitialized becomes true); reference the readCurrent method, setPlaceholderResult call, placeholderInitialized and placeholderRetryableFailure flags, and the returned current result so future maintainers understand the timing semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/lit-query/src/createQueriesController.ts`:
- Line 179: The constructor currently passes an empty array cast to
TCombinedResult into super (super(host, [] as unknown as TCombinedResult,
queryClient)), which is unsafe if TCombinedResult isn't array-like; change that
initial result to undefined (pass undefined as the initial sentinel) and ensure
the class handles this by using the existing placeholderInitialized logic and
readCurrent to return a proper value when the result is undefined; update the
super call to use undefined and verify readCurrent and any consumers of the
initial value (e.g., placeholderInitialized, readCurrent) correctly handle
undefined.
- Around line 346-361: Add an inline comment in readCurrent explaining the
early-read handshake: document that setPlaceholderResult is retried to handle
reads during class-field initialization (a microtask window before accessors are
wired), why placeholderRetryableFailure suppresses errors during that window,
and when errors will be surfaced (after placeholderInitialized becomes true);
reference the readCurrent method, setPlaceholderResult call,
placeholderInitialized and placeholderRetryableFailure flags, and the returned
current result so future maintainers understand the timing semantics.
In `@packages/lit-query/src/useMutationState.ts`:
- Around line 113-118: The condition in shouldRefreshOnHostUpdate() uses typeof
this.options.select === 'function' but select is statically typed as a function
when present; change the check to test presence instead (e.g.,
this.options.select !== undefined) and keep the filters check as-is (typeof
this.options.filters === 'function'); update the shouldRefreshOnHostUpdate
method to use the presence check for options.select to simplify the logic and
avoid a redundant typeof check.
- Around line 54-58: The onDisconnected method calls this.unsubscribe and then
calls this.syncClient(), but since we're disconnecting the client the
syncClient() call is unnecessary and its return value is ignored; remove the
this.syncClient() call from the onDisconnected() method (leave the unsubscribe
clearing logic intact) so only the unsubscribe cleanup runs in onDisconnected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e23f0f9a-ca96-457f-8103-84e2e95b80a0
📒 Files selected for processing (7)
packages/lit-query/src/createQueriesController.tspackages/lit-query/src/index.tspackages/lit-query/src/queryOptions.tspackages/lit-query/src/tests/counters-and-state.test.tspackages/lit-query/src/tests/type-inference.test.tspackages/lit-query/src/types.tspackages/lit-query/src/useMutationState.ts
✅ Files skipped from review due to trivial changes (1)
- packages/lit-query/src/tests/counters-and-state.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/lit-query/src/types.ts
- packages/lit-query/src/tests/type-inference.test.ts
- packages/lit-query/src/index.ts
- Fix createQueriesController tuple type inference with recursive types - Add DataTag support to queryOptions - Fix build config and vitest custom-condition resolution - Fix example install scripts for standalone bootstrap
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
packages/lit-query/package.json (1)
51-51: Avoid rebuilding twice indemo:gateto reduce CI/dev runtime.
demo:gaterunsbuild, thenpackage:build(which runsbuildagain). Foldingpackage:checkintopackage:buildlets the gate invoke one packaging command without redundant work.♻️ Suggested script cleanup
- "package:build": "pnpm run build && pnpm run test:build", + "package:build": "pnpm run build && pnpm run package:check && pnpm run test:build", ... - "demo:gate": "pnpm run typecheck && pnpm run lint && pnpm run format:check && pnpm test && pnpm run build && pnpm run package:check && pnpm run package:build && pnpm run example:e2e:all:nobuild && pnpm run example:ssr:e2e:all:nobuild && pnpm run measure:bundle:raw && pnpm run perf:l3:raw", + "demo:gate": "pnpm run typecheck && pnpm run lint && pnpm run format:check && pnpm test && pnpm run package:build && pnpm run example:e2e:all:nobuild && pnpm run example:ssr:e2e:all:nobuild && pnpm run measure:bundle:raw && pnpm run perf:l3:raw",Also applies to: 89-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lit-query/package.json` at line 51, Update the npm scripts to remove the duplicate build: change the "package:build" script so it performs the packaging/verification steps currently in "package:check" (fold package:check's duties into package:build) and no longer redundantly runs "build" if the gate already calls build; then adjust "demo:gate" to invoke only "pnpm run package:build" (instead of running "build" and then "package:build") so the CI/dev flow runs the package command once. Ensure references to "package:check", "package:build", and "demo:gate" are updated accordingly and remove or repurpose the old "package:check" script if no longer needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/lit-query/src/createQueriesController.ts`:
- Around line 1-22: The import block mixes type-only and value imports causing
lint errors; split type-only imports from value imports and remove duplicates
for both `@tanstack/query-core` and ./accessor.js: import runtime values
(QueriesObserver, createValueAccessor, readAccessor, BaseController,
createMissingQueryClientError) in one group and import only types (DefaultError,
DefinedQueryObserverResult, OmitKeyof, QueriesObserverOptions, QueryFunction,
QueryKey, QueryObserverOptions, QueryObserverResult, ThrowOnError, QueryClient,
ReactiveControllerHost, Accessor, ValueAccessor) in a separate "import type"
group, and reorder groups to satisfy import/order so all external deps come
before local modules.
- Around line 289-295: The synchronous call to resolveQueriesOptions(options,
queryClient) can throw during construction when an explicit queryClient is
provided; wrap that call and the subsequent QueriesObserver creation and result
computation (resolveQueriesOptions, new QueriesObserver(...),
observer.getCurrentResult(), computeResult) in a try-catch so any thrown error
is caught and the element is left in a safe state (mirroring readCurrent’s
guarded path): on catch assign this.queryClient and this.observer only if
created, set this.result to an error or empty fallback result, and rethrow or
store the error state as appropriate for later handling so construction does not
allow an uncaught exception to propagate.
- Around line 435-442: The placeholders are created with undefined data, so pass
each query's initialData into createPendingQueryObserverResult before calling
resolvedOptions.combine: for each item in queries (obtained via
readAccessor(resolvedOptions.queries)), extract its options.initialData (or
resolved initialData via readAccessor if options is an accessor) and call
createPendingQueryObserverResult(initialData) to build placeholders that satisfy
the DefinedQueryObserverResult contract; then pass that placeholders array to
resolvedOptions.combine (or return placeholders) so consumers (e.g.,
result[0].data.name) see the initial data. Ensure you reference resolvedOptions,
queries, createPendingQueryObserverResult, combine, and readAccessor when making
the change.
In `@packages/lit-query/src/QueryClientProvider.ts`:
- Around line 1-10: The import ordering violates the lint rule because a
type-only import (QueryClient from '@tanstack/query-core') appears before value
imports from 'lit' and './context.js'; reorder the import statements in
QueryClientProvider.ts so that value imports (ContextProvider from
'@lit/context', LitElement, html, TemplateResult from 'lit', and the named
exports createMissingQueryClientError, queryClientContext,
registerDefaultQueryClient, unregisterDefaultQueryClient from './context.js')
come first, and then place the type-only import of QueryClient after them to
satisfy import/order.
- Around line 47-53: When this.client becomes undefined the code currently
throws createMissingQueryClientError() before cleaning up, which leaves the
previously mounted client/state registered; update the QueryClientProvider
branch that checks const nextClient = this.client and if (!nextClient) to first
perform the provider's teardown/cleanup logic for the previous client
(unregister it as the default client and clear subscriptions/state) and then
throw createMissingQueryClientError(); place the cleanup call(s) immediately
before the throw so symbols like client, isConnected and
createMissingQueryClientError remain as-is and only the order changes.
In `@packages/lit-query/src/tests/type-inference.test.ts`:
- Around line 1-7: The import line mixes value and type specifiers causing lint
failures; split into two imports: one importing runtime values (dataTagSymbol,
QueryClient) and a separate type-only import for DefinedQueryObserverResult and
QueryObserverResult using the type keyword (e.g. import { type
DefinedQueryObserverResult, type QueryObserverResult } from
'@tanstack/query-core'), and ensure the two import statements are ordered/sorted
according to project rules (values import first, then type-only import).
---
Nitpick comments:
In `@packages/lit-query/package.json`:
- Line 51: Update the npm scripts to remove the duplicate build: change the
"package:build" script so it performs the packaging/verification steps currently
in "package:check" (fold package:check's duties into package:build) and no
longer redundantly runs "build" if the gate already calls build; then adjust
"demo:gate" to invoke only "pnpm run package:build" (instead of running "build"
and then "package:build") so the CI/dev flow runs the package command once.
Ensure references to "package:check", "package:build", and "demo:gate" are
updated accordingly and remove or repurpose the old "package:check" script if no
longer needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c1f340c5-0f5b-4dda-9bc0-d68f3409b53c
📒 Files selected for processing (11)
packages/lit-query/package.jsonpackages/lit-query/scripts/check-cjs-types-smoke.mjspackages/lit-query/src/QueryClientProvider.tspackages/lit-query/src/createQueriesController.tspackages/lit-query/src/queryOptions.tspackages/lit-query/src/tests/type-inference.test.tspackages/lit-query/src/types.tspackages/lit-query/tsconfig.build.cjs.jsonpackages/lit-query/tsconfig.build.jsonpackages/lit-query/tsconfig.jsonpackages/lit-query/vitest.config.ts
✅ Files skipped from review due to trivial changes (3)
- packages/lit-query/tsconfig.build.json
- packages/lit-query/tsconfig.build.cjs.json
- packages/lit-query/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/lit-query/vitest.config.ts
- packages/lit-query/scripts/check-cjs-types-smoke.mjs
- packages/lit-query/src/queryOptions.ts
Comment on lines
+1
to
+22
| import { | ||
| QueriesObserver, | ||
| type DefaultError, | ||
| type DefinedQueryObserverResult, | ||
| type OmitKeyof, | ||
| type QueriesObserverOptions, | ||
| type QueryFunction, | ||
| type QueryKey, | ||
| type QueryObserverOptions, | ||
| type QueryObserverResult, | ||
| type ThrowOnError, | ||
| } from '@tanstack/query-core' | ||
| import type { QueryClient } from '@tanstack/query-core' | ||
| import type { ReactiveControllerHost } from 'lit' | ||
| import { | ||
| createValueAccessor, | ||
| readAccessor, | ||
| type Accessor, | ||
| type ValueAccessor, | ||
| } from './accessor.js' | ||
| import { createMissingQueryClientError } from './context.js' | ||
| import { BaseController } from './controllers/BaseController.js' |
Contributor
There was a problem hiding this comment.
Normalize this import block before lint runs.
This file currently trips import/consistent-type-specifier-style, import/no-duplicates, and import/order. Split the value imports from the type-only imports for both @tanstack/query-core and ./accessor.js.
Suggested diff
-import {
- QueriesObserver,
- type DefaultError,
- type DefinedQueryObserverResult,
- type OmitKeyof,
- type QueriesObserverOptions,
- type QueryFunction,
- type QueryKey,
- type QueryObserverOptions,
- type QueryObserverResult,
- type ThrowOnError,
-} from '@tanstack/query-core'
-import type { QueryClient } from '@tanstack/query-core'
-import type { ReactiveControllerHost } from 'lit'
-import {
- createValueAccessor,
- readAccessor,
- type Accessor,
- type ValueAccessor,
-} from './accessor.js'
-import { createMissingQueryClientError } from './context.js'
-import { BaseController } from './controllers/BaseController.js'
+import { QueriesObserver } from '@tanstack/query-core'
+import { createValueAccessor, readAccessor } from './accessor.js'
+import { createMissingQueryClientError } from './context.js'
+import { BaseController } from './controllers/BaseController.js'
+
+import type {
+ DefaultError,
+ DefinedQueryObserverResult,
+ OmitKeyof,
+ QueriesObserverOptions,
+ QueryClient,
+ QueryFunction,
+ QueryKey,
+ QueryObserverOptions,
+ QueryObserverResult,
+ ThrowOnError,
+} from '@tanstack/query-core'
+import type { ReactiveControllerHost } from 'lit'
+import type { Accessor, ValueAccessor } from './accessor.js'🧰 Tools
🪛 ESLint
[error] 3-3: Member 'DefaultError' of the import declaration should be sorted alphabetically.
(sort-imports)
[error] 3-3: Prefer using a top-level type-only import instead of inline type specifiers.
(import/consistent-type-specifier-style)
[error] 4-4: Prefer using a top-level type-only import instead of inline type specifiers.
(import/consistent-type-specifier-style)
[error] 5-5: Prefer using a top-level type-only import instead of inline type specifiers.
(import/consistent-type-specifier-style)
[error] 6-6: Prefer using a top-level type-only import instead of inline type specifiers.
(import/consistent-type-specifier-style)
[error] 7-7: Prefer using a top-level type-only import instead of inline type specifiers.
(import/consistent-type-specifier-style)
[error] 8-8: Prefer using a top-level type-only import instead of inline type specifiers.
(import/consistent-type-specifier-style)
[error] 9-9: Prefer using a top-level type-only import instead of inline type specifiers.
(import/consistent-type-specifier-style)
[error] 10-10: Prefer using a top-level type-only import instead of inline type specifiers.
(import/consistent-type-specifier-style)
[error] 11-11: Prefer using a top-level type-only import instead of inline type specifiers.
(import/consistent-type-specifier-style)
[error] 12-12: '@tanstack/query-core' imported multiple times.
(import/no-duplicates)
[error] 13-13: @tanstack/query-core type import should occur after import of ./controllers/BaseController.js
(import/order)
[error] 13-13: '@tanstack/query-core' imported multiple times.
(import/no-duplicates)
[error] 14-14: lit type import should occur after import of ./controllers/BaseController.js
(import/order)
[error] 18-18: Member 'Accessor' of the import declaration should be sorted alphabetically.
(sort-imports)
[error] 18-18: Prefer using a top-level type-only import instead of inline type specifiers.
(import/consistent-type-specifier-style)
[error] 19-19: Prefer using a top-level type-only import instead of inline type specifiers.
(import/consistent-type-specifier-style)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/lit-query/src/createQueriesController.ts` around lines 1 - 22, The
import block mixes type-only and value imports causing lint errors; split
type-only imports from value imports and remove duplicates for both
`@tanstack/query-core` and ./accessor.js: import runtime values (QueriesObserver,
createValueAccessor, readAccessor, BaseController,
createMissingQueryClientError) in one group and import only types (DefaultError,
DefinedQueryObserverResult, OmitKeyof, QueriesObserverOptions, QueryFunction,
QueryKey, QueryObserverOptions, QueryObserverResult, ThrowOnError, QueryClient,
ReactiveControllerHost, Accessor, ValueAccessor) in a separate "import type"
group, and reorder groups to satisfy import/order so all external deps come
before local modules.
Comment on lines
+289
to
+295
| const { queries, combine } = resolveQueriesOptions(options, queryClient) | ||
| const observer = new QueriesObserver(queryClient, queries, { | ||
| combine, | ||
| } as QueriesObserverOptions<TCombinedResult>) | ||
| this.queryClient = queryClient | ||
| this.observer = observer | ||
| this.result = this.computeResult(observer.getCurrentResult(), combine) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the file
fd -t f "createQueriesController.ts" packages/lit-query/src/Repository: TanStack/query
Length of output: 107
🏁 Script executed:
# Read the file to check its size first
wc -l packages/lit-query/src/createQueriesController.tsRepository: TanStack/query
Length of output: 111
🏁 Script executed:
# Read lines 280-300 to see the constructor code
sed -n '280,300p' packages/lit-query/src/createQueriesController.tsRepository: TanStack/query
Length of output: 638
🏁 Script executed:
# Read lines 440-465 to see the readCurrent() method
sed -n '440,465p' packages/lit-query/src/createQueriesController.tsRepository: TanStack/query
Length of output: 870
🏁 Script executed:
# Search for resolveQueriesOptions definition
rg "resolveQueriesOptions" packages/lit-query/src/ -A 10Repository: TanStack/query
Length of output: 2804
🏁 Script executed:
# Get more context on the constructor to see the full flow
sed -n '260,310p' packages/lit-query/src/createQueriesController.tsRepository: TanStack/query
Length of output: 1494
🏁 Script executed:
# Check if there's a try-catch around the constructor code or any protection
sed -n '250,320p' packages/lit-query/src/createQueriesController.tsRepository: TanStack/query
Length of output: 1887
🏁 Script executed:
# Look at the entire setClient method to understand the full flow
rg "private setClient|private.*client" packages/lit-query/src/createQueriesController.ts -A 20Repository: TanStack/query
Length of output: 579
Wrap resolveQueriesOptions() in a try-catch when initializing with an explicit client.
The constructor calls resolveQueriesOptions() synchronously when a queryClient is passed (lines 289-295). If the queries accessor throws during this call, the error will propagate uncaught during element construction. The readCurrent() method handles this hazard for deferred initialization with a try-catch guard, but the explicit-client path bypasses that protection entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/lit-query/src/createQueriesController.ts` around lines 289 - 295,
The synchronous call to resolveQueriesOptions(options, queryClient) can throw
during construction when an explicit queryClient is provided; wrap that call and
the subsequent QueriesObserver creation and result computation
(resolveQueriesOptions, new QueriesObserver(...), observer.getCurrentResult(),
computeResult) in a try-catch so any thrown error is caught and the element is
left in a safe state (mirroring readCurrent’s guarded path): on catch assign
this.queryClient and this.observer only if created, set this.result to an error
or empty fallback result, and rethrow or store the error state as appropriate
for later handling so construction does not allow an uncaught exception to
propagate.
Comment on lines
+1
to
+10
| import { ContextProvider } from '@lit/context' | ||
| import type { QueryClient } from '@tanstack/query-core' | ||
| import type { TemplateResult } from 'lit' | ||
| import { LitElement, html } from 'lit' | ||
| import { | ||
| createMissingQueryClientError, | ||
| queryClientContext, | ||
| registerDefaultQueryClient, | ||
| unregisterDefaultQueryClient, | ||
| } from './context.js' |
Contributor
There was a problem hiding this comment.
Fix the import grouping so this file passes lint.
lit and ./context.js need to be ordered before the type-only @tanstack/query-core import, otherwise import/order keeps failing here.
💡 Minimal reorder
import { ContextProvider } from '@lit/context'
-import type { QueryClient } from '@tanstack/query-core'
-import type { TemplateResult } from 'lit'
-import { LitElement, html } from 'lit'
+import { LitElement, html, type TemplateResult } from 'lit'
import {
createMissingQueryClientError,
queryClientContext,
registerDefaultQueryClient,
unregisterDefaultQueryClient,
} from './context.js'
+import type { QueryClient } from '@tanstack/query-core'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { ContextProvider } from '@lit/context' | |
| import type { QueryClient } from '@tanstack/query-core' | |
| import type { TemplateResult } from 'lit' | |
| import { LitElement, html } from 'lit' | |
| import { | |
| createMissingQueryClientError, | |
| queryClientContext, | |
| registerDefaultQueryClient, | |
| unregisterDefaultQueryClient, | |
| } from './context.js' | |
| import { ContextProvider } from '@lit/context' | |
| import { LitElement, html, type TemplateResult } from 'lit' | |
| import { | |
| createMissingQueryClientError, | |
| queryClientContext, | |
| registerDefaultQueryClient, | |
| unregisterDefaultQueryClient, | |
| } from './context.js' | |
| import type { QueryClient } from '@tanstack/query-core' |
🧰 Tools
🪛 ESLint
[error] 4-4: lit import should occur before type import of @tanstack/query-core
(import/order)
[error] 5-10: ./context.js import should occur before type import of @tanstack/query-core
(import/order)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/lit-query/src/QueryClientProvider.ts` around lines 1 - 10, The
import ordering violates the lint rule because a type-only import (QueryClient
from '@tanstack/query-core') appears before value imports from 'lit' and
'./context.js'; reorder the import statements in QueryClientProvider.ts so that
value imports (ContextProvider from '@lit/context', LitElement, html,
TemplateResult from 'lit', and the named exports createMissingQueryClientError,
queryClientContext, registerDefaultQueryClient, unregisterDefaultQueryClient
from './context.js') come first, and then place the type-only import of
QueryClient after them to satisfy import/order.
Comment on lines
+47
to
+53
| const nextClient = this.client | ||
| if (!nextClient) { | ||
| if (this.isConnected) { | ||
| throw createMissingQueryClientError() | ||
| } | ||
|
|
||
| return |
Contributor
There was a problem hiding this comment.
Tear down the previous client before throwing on client = undefined.
Line 50 throws before any cleanup. If a connected provider had already mounted A and client is later cleared, A stays mounted and registered as the default client until disconnect, so the provider keeps stale subscriptions/state alive after the error.
🧹 Minimal cleanup before raising
const nextClient = this.client
if (!nextClient) {
if (this.isConnected) {
+ this.unmountClient(this.mountedClient)
throw createMissingQueryClientError()
}
return
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/lit-query/src/QueryClientProvider.ts` around lines 47 - 53, When
this.client becomes undefined the code currently throws
createMissingQueryClientError() before cleaning up, which leaves the previously
mounted client/state registered; update the QueryClientProvider branch that
checks const nextClient = this.client and if (!nextClient) to first perform the
provider's teardown/cleanup logic for the previous client (unregister it as the
default client and clear subscriptions/state) and then throw
createMissingQueryClientError(); place the cleanup call(s) immediately before
the throw so symbols like client, isConnected and createMissingQueryClientError
remain as-is and only the order changes.
Comment on lines
+1
to
+7
| import { | ||
| dataTagSymbol, | ||
| QueryClient, | ||
| type DefinedQueryObserverResult, | ||
| type QueryObserverResult, | ||
| } from '@tanstack/query-core' | ||
| import { describe, expectTypeOf, it } from 'vitest' |
Contributor
There was a problem hiding this comment.
Split the value and type imports here.
This block currently fails import/consistent-type-specifier-style and sort-imports, so the new type test will not pass lint as written.
Suggested diff
-import {
- dataTagSymbol,
- QueryClient,
- type DefinedQueryObserverResult,
- type QueryObserverResult,
-} from '@tanstack/query-core'
+import { QueryClient, dataTagSymbol } from '@tanstack/query-core'
+import type {
+ DefinedQueryObserverResult,
+ QueryObserverResult,
+} from '@tanstack/query-core'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { | |
| dataTagSymbol, | |
| QueryClient, | |
| type DefinedQueryObserverResult, | |
| type QueryObserverResult, | |
| } from '@tanstack/query-core' | |
| import { describe, expectTypeOf, it } from 'vitest' | |
| import { QueryClient, dataTagSymbol } from '@tanstack/query-core' | |
| import type { | |
| DefinedQueryObserverResult, | |
| QueryObserverResult, | |
| } from '@tanstack/query-core' | |
| import { describe, expectTypeOf, it } from 'vitest' |
🧰 Tools
🪛 ESLint
[error] 3-3: Member 'QueryClient' of the import declaration should be sorted alphabetically.
(sort-imports)
[error] 4-4: Prefer using a top-level type-only import instead of inline type specifiers.
(import/consistent-type-specifier-style)
[error] 5-5: Prefer using a top-level type-only import instead of inline type specifiers.
(import/consistent-type-specifier-style)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/lit-query/src/tests/type-inference.test.ts` around lines 1 - 7, The
import line mixes value and type specifiers causing lint failures; split into
two imports: one importing runtime values (dataTagSymbol, QueryClient) and a
separate type-only import for DefinedQueryObserverResult and QueryObserverResult
using the type keyword (e.g. import { type DefinedQueryObserverResult, type
QueryObserverResult } from '@tanstack/query-core'), and ensure the two import
statements are ordered/sorted according to project rules (values import first,
then type-only import).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (2)
packages/lit-query/examples/lit-query-ssr-app/scripts/run-script-with-public-origin.mjs (1)
37-38:⚠️ Potential issue | 🟠 MajorAwait
'close'instead of'exit'to avoid hangs on spawn failure.Node's child_process may not emit
'exit'if an'error'occurs (e.g., spawn failure), soawait once(child, 'exit')can hang.'close'reliably fires after either'exit'or'error'and guarantees stdio is closed.Proposed fix
-const [code] = await once(child, 'exit') +const [code] = await once(child, 'close') process.exitCode = code ?? 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lit-query/examples/lit-query-ssr-app/scripts/run-script-with-public-origin.mjs` around lines 37 - 38, The code awaits once(child, 'exit') which can hang on spawn failures; change the event awaited to 'close' instead so the promise resolves after either 'exit' or 'error' and stdio is closed—update the call in this file where once(child, 'exit') is used (referencing the once function and the child variable) to await once(child, 'close') and then set process.exitCode from the returned code value as before.packages/lit-query/package.json (1)
92-100:⚠️ Potential issue | 🟠 MajorFix dependency duplication in
lit-queryto match the pattern used across all other framework adapters.
litis declared as both a runtime dependency (^3.3.1) and a peer dependency (>=2.8.0 <4). Similarly,@tanstack/query-coreappears in bothdependencies(workspace:*) andpeerDependencies(^5.0.0). This deviates from the consistent pattern across all sibling adapters:@tanstack/query-coreshould be a dependency only, and the framework (lit,react,preact, etc.) should be peer-only.Having
litin both places forces consumers to bundle Lit at runtime, which breaksReactiveElementinstanceof checks, decorator metadata, and context propagation. Listing@tanstack/query-coreas a peer also introduces unnecessary pnpm peer-resolution warnings.Suggested change
"dependencies": { "@lit/context": "^1.1.6", - "@tanstack/query-core": "workspace:*", - "lit": "^3.3.1" + "@tanstack/query-core": "workspace:*" }, "peerDependencies": { - "@tanstack/query-core": "^5.0.0", "lit": ">=2.8.0 <4" }, "devDependencies": { + "lit": "^3.3.1",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lit-query/package.json` around lines 92 - 100, In package.json for the lit-query package, remove the duplicate entries so that `@tanstack/query-core` exists only under "dependencies" (keep "workspace:*") and "lit" exists only under "peerDependencies" (keep ">=2.8.0 <4"); specifically delete "lit": "^3.3.1" from "dependencies" and delete "@tanstack/query-core": "^5.0.0" from "peerDependencies" so the file follows the same adapter pattern as the other framework adapters.
🧹 Nitpick comments (11)
packages/lit-query/package.json (1)
33-36:build:depshardcodes a sibling-package build and bypasses the monorepo orchestrator.
"build:deps": "pnpm --dir ../query-core run build"reaches across packages by relative path and rebuildsquery-coreunconditionally on everybuild. In this repo, cross-package build ordering is normally handled by Nx (nx run-many/ task graph), which is much faster (cache-aware) and won't break if the directory layout changes. Consider droppingbuild:depsfrombuildand relying on the workspace task runner to ensurequery-coreis built first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lit-query/package.json` around lines 33 - 36, The "build" script currently invokes the sibling-targeted "build:deps" which hardcodes a relative path to ../query-core; remove that cross-package invocation by editing the "build" npm script to stop calling "build:deps" (e.g., change "build": "pnpm run build:deps && pnpm run build:esm && pnpm run build:cjs" to only run the local steps) and either delete or keep "build:deps" as a standalone script if needed, relying on the monorepo/task runner (Nx) to orchestrate cross-package builds instead of pnpm --dir ../query-core; ensure references to "build:deps" in package.json are removed so local "build" no longer rebuilds query-core via a relative path.packages/lit-query/examples/lit-query-ssr-app/server/index.mjs (2)
188-224: No timeout around the SSR prefetch.
queryClient.prefetchQuery(...)inheritsretry: falsebut has no upstream deadline. If the upstream/api/dataendpoint (orSSR_PUBLIC_ORIGIN) hangs, this request will stall indefinitely, occupying a Node socket and eventually hitting the client's own timeout with no 500 fallback. Since this is an example/test server used by E2E, anAbortSignal.timeout(...)(or similar) around the prefetch would make hangs observable and still trigger the 500 branch in the route handler.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lit-query/examples/lit-query-ssr-app/server/index.mjs` around lines 188 - 224, Add a bounded deadline around the SSR prefetch in renderPage: create an AbortSignal via AbortSignal.timeout(ms) (or equivalent) and pass it into queryClient.prefetchQuery (alongside/createDataQueryOptions(SSR_PUBLIC_ORIGIN) or merged into the options) so the upstream fetch to SSR_PUBLIC_ORIGIN can't hang indefinitely; ensure you clean up/revoke the timeout if needed and let the aborted/error case propagate so the existing error branch that returns a 500 is exercised (i.e., throw or let the prefetch rejection surface when the signal aborts).
48-57: Function name is misleading in a server-side context.
createBrowserQueryClient()is invoked only on the server (insiderenderPage) to drive SSR prefetch + dehydration. The "browser" prefix reads as if this were the client-side instance and can confuse future readers. Consider renaming tocreateSsrQueryClient(or similar) for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lit-query/examples/lit-query-ssr-app/server/index.mjs` around lines 48 - 57, The function name createBrowserQueryClient is misleading for a server-side SSR use; rename the function to createSsrQueryClient (or createServerQueryClient) and update all references (e.g., where renderPage calls createBrowserQueryClient) to the new name so the intent is clear; ensure the exported symbol (if any) and any imports/uses are renamed consistently to avoid breaking references and update any comments/docs accordingly.packages/lit-query/examples/lit-query-ssr-app/e2e/lib.mjs (2)
163-198: No request timeout on the test-control endpoints.
resetTestState,getRequestCount,armNextDataError, andarmNextDataDelayall do barefetch(...)without anAbortSignal. If the SSR server hangs while serving/api/*, every E2E script that calls these helpers hangs until Playwright's outer timeout kicks in (or, for node scripts, indefinitely). A shortAbortSignal.timeout(...)would fail fast with a clear message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lit-query/examples/lit-query-ssr-app/e2e/lib.mjs` around lines 163 - 198, The four test-control helpers (resetTestState, getRequestCount, armNextDataError, armNextDataDelay) call fetch without an AbortSignal which lets E2E scripts hang if the SSR server stalls; update each fetch call to pass a short AbortSignal.timeout (e.g., const DEFAULT_TIMEOUT_MS = 5000) via the fetch options' signal to fail fast and return a clear timeout error, and reuse that constant across the functions so all requests (including the GET in getRequestCount) time out consistently; ensure the signal is passed for both POST and GET calls and that downstream code (e.g., awaiting response.json()) will receive the abort if the timeout fires.
267-326:runBuild/ensureServerleak on scenario-setup failure; also not parity with sibling harnesses.Two small things:
runBuild()andensureServer()run outside thetry/finallyblock (lines 274–278). IfrunBuildthrows there's nothing to clean up, but if anything betweenensureServer()returning and entering thetryfails (extremely unlikely, but e.g. a synchronous throw from assigninglet browser), the started server would leak. Moving them inside thetrykeepsstopServer(server)as the single cleanup path.- This file hard-codes
httpProbeTimeoutMs,serverReadyTimeoutMs,selectorTimeoutMswhereas the e2e-app/pagination-app harnesses exposePW_HTTP_PROBE_TIMEOUT_MS,PW_SERVER_READY_TIMEOUT_MS,PW_WAIT_FOR_TEXT_TIMEOUT_MS. Slow CI runners will hit these silently. Aligning the three modules (or extracting a shared one) would avoid the drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lit-query/examples/lit-query-ssr-app/e2e/lib.mjs` around lines 267 - 326, The setup leaks: move the runBuild() and ensureServer() calls into the try block inside withBrowserPage so that stopServer(server) in the finally always runs if either setup step or subsequent synchronous code throws; keep server declared outside try so finally can reference it and ensure captureFailureArtifacts(page) still runs on catches. Also stop hard-coding probe/ready/selector timeouts in this module—replace the local httpProbeTimeoutMs, serverReadyTimeoutMs, selectorTimeoutMs with the same env-driven constants used by sibling harnesses (PW_HTTP_PROBE_TIMEOUT_MS, PW_SERVER_READY_TIMEOUT_MS, PW_WAIT_FOR_TEXT_TIMEOUT_MS) or import a shared timeout config so slow CI runners get the higher timeouts.packages/lit-query/examples/lit-query-e2e-app/e2e/lib.mjs (2)
273-300: Prefer Playwright locator overpage.evaluatefor text polling.The pagination example's twin helper (
packages/lit-query/examples/lit-query-pagination-app/e2e/lib.mjs) already usespage.locator(selector).textContent({ timeout: 500 })for the same job, which avoids round-tripping throughevaluate, is auto-retriable, and won't throw on detached frames. Since the two modules are otherwise near-duplicates, it's worth aligning them so future maintainers only have one pattern to learn.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lit-query/examples/lit-query-e2e-app/e2e/lib.mjs` around lines 273 - 300, The waitForText function currently polls via page.evaluate which causes unnecessary round-trips and can fail on detached frames; replace its polling logic to use Playwright's Locator API (use page.locator(selector).textContent({ timeout: 500 }) inside the loop or use locator.waitFor with a short timeout) so you read text via locator (auto-retriable and frame-safe) until the expectedText is found or textWaitTimeoutMs elapses; update references to the function name waitForText and keep the same timeout behavior but rely on locator.textContent({ timeout: 500 }) instead of page.evaluate.
1-313: Heavy duplication with the other twoe2e/lib.mjsharnesses.This file is ~95% identical to
packages/lit-query/examples/lit-query-pagination-app/e2e/lib.mjsand partially overlapspackages/lit-query/examples/lit-query-ssr-app/e2e/lib.mjs(server spawn, strict-port detection,isServerReady,waitForServer,stopProcess/stopServer, artifact capture,withBrowserPage,readNumberFromTail). Any future fix (e.g., theENOENT/timeout tweaks, Playwright API changes) has to be made in triplicate. Consider extracting a shared harness underpackages/lit-query/examples/_shared/(or similar) that the three example apps consume, keeping only app-specific knobs (ports, dev-script names, extra API wiring) in each example.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lit-query/examples/lit-query-e2e-app/e2e/lib.mjs` around lines 1 - 313, This file duplicates most of the E2E harness logic (startServer, isServerReady, waitForServer, stopServer, captureFailureArtifacts, withBrowserPage, readNumberFromTail, etc.); extract a shared harness module under packages/lit-query/examples/_shared/ that exports the generic functions (startServer, isServerReady, waitForServer, stopServer, captureFailureArtifacts, withBrowserPage, waitForText, readNumberFromTail and helpers like sanitizeForPath, readPositiveIntEnv) and update each example (this file and the two other e2e/lib.mjs) to import the shared module and only define app-specific knobs (host, port/DEMO_PORT, baseUrl, packageManagerCommand, projectDir, allowExistingServer, artifactDir, timeouts) and any different dev-script args, so fixes and API changes are made once in the shared harness while each example keeps minimal per-app overrides.packages/lit-query/examples/lit-query-pagination-app/e2e/lib.mjs (1)
298-329:waitForNthTextduplicateswaitForText.These two functions differ only in
page.locator(selector)vspage.locator(selector).nth(index). A single helper that accepts an optionalindex(or a pre-built locator) would halve the surface area and keep future changes (timeouts, retry cadence, error message format) in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lit-query/examples/lit-query-pagination-app/e2e/lib.mjs` around lines 298 - 329, Refactor the duplicated helpers waitForNthText and waitForText into a single function (e.g., waitForTextWithIndex) that accepts either an optional index parameter or a Locator parameter so it can use page.locator(selector) or page.locator(selector).nth(index) without duplicating logic; move the retry loop, timeout handling, text extraction and final error formatting into that single function (keep the same default timeout and retry cadence), replace existing calls to waitForText and waitForNthText to call the new helper (or pass a pre-built Locator) and remove the old functions to keep behavior and messages consistent.packages/lit-query/src/createQueryController.ts (1)
234-261: Consider isolating thethis.queryClientassignment out ofdefaultOptions.
defaultOptionsmutatesthis.queryClientat line 248, which is a side effect hidden inside an options-resolution helper. It works today because every caller (refreshOptions,suspense, the constructor) already establishedthis.queryClientviasyncClientor the constructor path. Pulling that side effect out (and lettingdefaultOptionsbe a pure function ofclient/this.options) would make the controller easier to reason about and bring it in line with the sibling controllers (createInfiniteQueryController,createMutationController) whosedefaultOptionsis side-effect free.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lit-query/src/createQueryController.ts` around lines 234 - 261, The defaultOptions method currently mutates this.queryClient (assignment at defaultOptions) which hides side effects; make defaultOptions a pure function by removing the assignment this.queryClient = resolvedClient and instead have callers establish the client before calling defaultOptions: call tryGetQueryClient() (or pass the resolved client) and assign it to this.queryClient in the caller sites such as the constructor, refreshOptions, and suspense (and any other places that call defaultOptions), mirroring the side-effect-free behavior of createInfiniteQueryController and createMutationController; ensure defaultOptions still throws createMissingQueryClientError() if no client is provided/resolved but does not modify object state.packages/lit-query/src/createMutationController.ts (1)
168-179: Confirmmutateswallow is intended.
void this.observer.mutate(...).catch(() => {})intentionally suppresses the rejection somutate()is non-throwing, while error state is still surfaced via the observer subscription (and any per-callmutateOptions.onError). This mirrors React'suseMutation.mutate. Worth a one-line code comment beyond the current "swallow" note documenting that error visibility flows through the subscription so future readers don't try to add re-throwing here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lit-query/src/createMutationController.ts` around lines 168 - 179, In createMutationController.ts, update the mutate implementation (mutate method calling this.observer.mutate) to keep the current swallow behavior but add a single clarifying comment: state that rejections are intentionally not re-thrown here because mutation errors are surfaced via the observer subscription and per-call mutateOptions.onError (mirroring useMutation.mutate), so future readers don't reintroduce a throw; leave the existing void this.observer.mutate(...).catch(() => { /* ... */ }) behavior intact.packages/lit-query/src/createInfiniteQueryController.ts (1)
240-260: Optional: keepsyncClientconsistent across controllers.
createQueryController.tsline 211 guards reuse withnextClient === this.queryClient && this.observer; this controller (and the mutation/queries controllers) only checks the client. In current flowsqueryClientandobserverare always set/cleared together, so this isn't a bug, but adding the same&& this.observerdefensive check would make the four controllers' lifecycle paths uniform and easier to reason about during future edits.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lit-query/src/createInfiniteQueryController.ts` around lines 240 - 260, The syncClient method in createInfiniteQueryController currently returns early when nextClient === this.queryClient but doesn't check that this.observer is set; update the early-return guard to mirror other controllers by using nextClient === this.queryClient && this.observer so we only skip reinitialization when both the client and observer are present. Modify the syncClient function (which calls tryGetQueryClient, unsubscribeObserver, defaultOptions, new InfiniteQueryObserver, setResult, and createPendingInfiniteQueryResult) to use this combined condition to keep lifecycle behavior consistent across controllers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/lit-query/examples/lit-query-pagination-app/e2e/lib.mjs`:
- Around line 169-210: The current ensureServers() treats api/web readiness as a
single gate, which can return both as reused even if only one is up; change
ensureServers() to probe api and web independently using existingApiReady and
existingWebReady, and then only reuse the ones that are actually ready: if
existingApiReady is true set apiResult = undefined else startProcess('api',
['run','dev:api']) and waitForServer(...) for it; similarly for web with
startProcess('web', ['run','dev:web'], { VITE_PAGINATION_API_PORT:
String(API_PORT) }) and waitForServer(...); when allowExistingServer is false,
throw only if both exist? or follow current policy but ensure you never return {
api: undefined, web: undefined } unless both probes succeeded—so ensure the
function stops/cleans up via stopProcess(...) for any started process on errors
(use the same stopProcess and try/catch pattern already used for startProcess,
waitForServer, and stopProcess).
In `@packages/lit-query/examples/lit-query-pagination-app/src/api.ts`:
- Line 14: The property declaration uses the shorthand array type "projects:
Project[]" which triggers the `@typescript-eslint/array-type` rule; update the
declaration to use the generic form "Array<Project>" for the projects property
(the "projects" field in the relevant type/interface in src/api.ts) so it
conforms to the project's configured array-type style.
In `@packages/lit-query/examples/lit-query-ssr-app/e2e/lib.mjs`:
- Around line 250-265: The captureFailureArtifacts function can throw during
mkdir/screenshot/writeFile and mask the original test failure; wrap the
filesystem and Puppeteer calls inside a try/catch within captureFailureArtifacts
(around mkdir, page.screenshot, writeFile and the console.error lines) and on
error log a non-throwing warning (including the caught error) instead of
allowing the exception to propagate so the original scenario error remains the
one re-thrown by the outer catch; keep the existing behavior when page is falsy.
In `@packages/lit-query/examples/lit-query-ssr-app/server/index.mjs`:
- Around line 262-266: The POST handler that sets failNextDataRequest for
'/api/test/fail-next-data' is unguarded (see the branch that checks method ===
'POST' && requestUrl.pathname === '/api/test/fail-next-data' and sets
failNextDataRequest), so add a guard to prevent remote arming: require either
process.env.NODE_ENV !== 'production' or verify a shared secret header (e.g.,
check req.headers['x-test-secret'] === process.env.TEST_SECRET) before flipping
failNextDataRequest, and return a 403 if the check fails; ensure the guard is
applied before calling writeJson so the example route cannot be triggered in
production or by unauthenticated clients.
- Around line 152-164: The readTemplate function currently treats any
SystemError with a code property as "missing build"; change the error branch to
only handle file-not-found scenarios by checking error.code === 'ENOENT' (and
optionally error.code === 'ENOTDIR') before throwing the "Run pnpm run build"
Error, and rethrow all other errors unchanged; locate the logic inside
readTemplate that inspects error and narrow the condition to match only 'ENOENT'
(and 'ENOTDIR') so permissions/other FS errors propagate.
- Around line 22-25: The import of ../src/app.ts should use a .js specifier to
follow idiomatic ESM and match the rest of the file; update the import statement
that brings in getSsrQueryControllerCreationCount and
resetSsrQueryControllerCreationCount to reference ../src/app.js instead of
../src/app.ts so tooling can map the .ts source while the runtime uses .js
specifiers.
In `@packages/lit-query/package.json`:
- Around line 56-88: The three npm install scripts (example:install,
example:pagination:install, example:ssr:install) use "npm --prefix" which
creates package-lock.json and bypasses pnpm workspace behavior; update each
script to use "pnpm --dir <example-dir> install" to match the other scripts
(e.g., change the command for example:install, example:pagination:install, and
example:ssr:install to use pnpm --dir instead of npm --prefix).
In `@packages/lit-query/src/tests/mutation-controller.test.ts`:
- Line 18: Move the shared variable explicitMutationClient reset into an
afterEach hook to avoid test leakage: currently explicitMutationClient is
declared at top-level and only reset in-line, so if LC-MUT-02 fails earlier
subsequent tests that create ContextMutationHostElement can inherit the leaked
client and bypass provider resolution; add an afterEach(() => {
explicitMutationClient = undefined }) (or equivalent cleanup) in the
mutation-controller.test.ts test suite and ensure any existing inline resets
around lines 71-107 are removed/relocated to this afterEach to guarantee each
test starts with a clean explicitMutationClient.
In `@packages/lit-query/src/tests/queries-controller.test.ts`:
- Line 19: Move the cleanup of the top-level variable explicitQueriesClient into
an afterEach block to avoid leaking the explicit QueryClient between tests;
specifically, add or update an afterEach that sets explicitQueriesClient =
undefined (and any related teardown used for explicit client state) so tests
that instantiate ContextQueriesHostElement never inadvertently reuse a previous
explicit client; target the tests in queries-controller.test.ts that reference
explicitQueriesClient (lines around 144-192 and the declaration at the top) and
remove any existing resets from inside individual tests so the single afterEach
handles it.
---
Duplicate comments:
In
`@packages/lit-query/examples/lit-query-ssr-app/scripts/run-script-with-public-origin.mjs`:
- Around line 37-38: The code awaits once(child, 'exit') which can hang on spawn
failures; change the event awaited to 'close' instead so the promise resolves
after either 'exit' or 'error' and stdio is closed—update the call in this file
where once(child, 'exit') is used (referencing the once function and the child
variable) to await once(child, 'close') and then set process.exitCode from the
returned code value as before.
In `@packages/lit-query/package.json`:
- Around line 92-100: In package.json for the lit-query package, remove the
duplicate entries so that `@tanstack/query-core` exists only under "dependencies"
(keep "workspace:*") and "lit" exists only under "peerDependencies" (keep
">=2.8.0 <4"); specifically delete "lit": "^3.3.1" from "dependencies" and
delete "@tanstack/query-core": "^5.0.0" from "peerDependencies" so the file
follows the same adapter pattern as the other framework adapters.
---
Nitpick comments:
In `@packages/lit-query/examples/lit-query-e2e-app/e2e/lib.mjs`:
- Around line 273-300: The waitForText function currently polls via
page.evaluate which causes unnecessary round-trips and can fail on detached
frames; replace its polling logic to use Playwright's Locator API (use
page.locator(selector).textContent({ timeout: 500 }) inside the loop or use
locator.waitFor with a short timeout) so you read text via locator
(auto-retriable and frame-safe) until the expectedText is found or
textWaitTimeoutMs elapses; update references to the function name waitForText
and keep the same timeout behavior but rely on locator.textContent({ timeout:
500 }) instead of page.evaluate.
- Around line 1-313: This file duplicates most of the E2E harness logic
(startServer, isServerReady, waitForServer, stopServer, captureFailureArtifacts,
withBrowserPage, readNumberFromTail, etc.); extract a shared harness module
under packages/lit-query/examples/_shared/ that exports the generic functions
(startServer, isServerReady, waitForServer, stopServer, captureFailureArtifacts,
withBrowserPage, waitForText, readNumberFromTail and helpers like
sanitizeForPath, readPositiveIntEnv) and update each example (this file and the
two other e2e/lib.mjs) to import the shared module and only define app-specific
knobs (host, port/DEMO_PORT, baseUrl, packageManagerCommand, projectDir,
allowExistingServer, artifactDir, timeouts) and any different dev-script args,
so fixes and API changes are made once in the shared harness while each example
keeps minimal per-app overrides.
In `@packages/lit-query/examples/lit-query-pagination-app/e2e/lib.mjs`:
- Around line 298-329: Refactor the duplicated helpers waitForNthText and
waitForText into a single function (e.g., waitForTextWithIndex) that accepts
either an optional index parameter or a Locator parameter so it can use
page.locator(selector) or page.locator(selector).nth(index) without duplicating
logic; move the retry loop, timeout handling, text extraction and final error
formatting into that single function (keep the same default timeout and retry
cadence), replace existing calls to waitForText and waitForNthText to call the
new helper (or pass a pre-built Locator) and remove the old functions to keep
behavior and messages consistent.
In `@packages/lit-query/examples/lit-query-ssr-app/e2e/lib.mjs`:
- Around line 163-198: The four test-control helpers (resetTestState,
getRequestCount, armNextDataError, armNextDataDelay) call fetch without an
AbortSignal which lets E2E scripts hang if the SSR server stalls; update each
fetch call to pass a short AbortSignal.timeout (e.g., const DEFAULT_TIMEOUT_MS =
5000) via the fetch options' signal to fail fast and return a clear timeout
error, and reuse that constant across the functions so all requests (including
the GET in getRequestCount) time out consistently; ensure the signal is passed
for both POST and GET calls and that downstream code (e.g., awaiting
response.json()) will receive the abort if the timeout fires.
- Around line 267-326: The setup leaks: move the runBuild() and ensureServer()
calls into the try block inside withBrowserPage so that stopServer(server) in
the finally always runs if either setup step or subsequent synchronous code
throws; keep server declared outside try so finally can reference it and ensure
captureFailureArtifacts(page) still runs on catches. Also stop hard-coding
probe/ready/selector timeouts in this module—replace the local
httpProbeTimeoutMs, serverReadyTimeoutMs, selectorTimeoutMs with the same
env-driven constants used by sibling harnesses (PW_HTTP_PROBE_TIMEOUT_MS,
PW_SERVER_READY_TIMEOUT_MS, PW_WAIT_FOR_TEXT_TIMEOUT_MS) or import a shared
timeout config so slow CI runners get the higher timeouts.
In `@packages/lit-query/examples/lit-query-ssr-app/server/index.mjs`:
- Around line 188-224: Add a bounded deadline around the SSR prefetch in
renderPage: create an AbortSignal via AbortSignal.timeout(ms) (or equivalent)
and pass it into queryClient.prefetchQuery
(alongside/createDataQueryOptions(SSR_PUBLIC_ORIGIN) or merged into the options)
so the upstream fetch to SSR_PUBLIC_ORIGIN can't hang indefinitely; ensure you
clean up/revoke the timeout if needed and let the aborted/error case propagate
so the existing error branch that returns a 500 is exercised (i.e., throw or let
the prefetch rejection surface when the signal aborts).
- Around line 48-57: The function name createBrowserQueryClient is misleading
for a server-side SSR use; rename the function to createSsrQueryClient (or
createServerQueryClient) and update all references (e.g., where renderPage calls
createBrowserQueryClient) to the new name so the intent is clear; ensure the
exported symbol (if any) and any imports/uses are renamed consistently to avoid
breaking references and update any comments/docs accordingly.
In `@packages/lit-query/package.json`:
- Around line 33-36: The "build" script currently invokes the sibling-targeted
"build:deps" which hardcodes a relative path to ../query-core; remove that
cross-package invocation by editing the "build" npm script to stop calling
"build:deps" (e.g., change "build": "pnpm run build:deps && pnpm run build:esm
&& pnpm run build:cjs" to only run the local steps) and either delete or keep
"build:deps" as a standalone script if needed, relying on the monorepo/task
runner (Nx) to orchestrate cross-package builds instead of pnpm --dir
../query-core; ensure references to "build:deps" in package.json are removed so
local "build" no longer rebuilds query-core via a relative path.
In `@packages/lit-query/src/createInfiniteQueryController.ts`:
- Around line 240-260: The syncClient method in createInfiniteQueryController
currently returns early when nextClient === this.queryClient but doesn't check
that this.observer is set; update the early-return guard to mirror other
controllers by using nextClient === this.queryClient && this.observer so we only
skip reinitialization when both the client and observer are present. Modify the
syncClient function (which calls tryGetQueryClient, unsubscribeObserver,
defaultOptions, new InfiniteQueryObserver, setResult, and
createPendingInfiniteQueryResult) to use this combined condition to keep
lifecycle behavior consistent across controllers.
In `@packages/lit-query/src/createMutationController.ts`:
- Around line 168-179: In createMutationController.ts, update the mutate
implementation (mutate method calling this.observer.mutate) to keep the current
swallow behavior but add a single clarifying comment: state that rejections are
intentionally not re-thrown here because mutation errors are surfaced via the
observer subscription and per-call mutateOptions.onError (mirroring
useMutation.mutate), so future readers don't reintroduce a throw; leave the
existing void this.observer.mutate(...).catch(() => { /* ... */ }) behavior
intact.
In `@packages/lit-query/src/createQueryController.ts`:
- Around line 234-261: The defaultOptions method currently mutates
this.queryClient (assignment at defaultOptions) which hides side effects; make
defaultOptions a pure function by removing the assignment this.queryClient =
resolvedClient and instead have callers establish the client before calling
defaultOptions: call tryGetQueryClient() (or pass the resolved client) and
assign it to this.queryClient in the caller sites such as the constructor,
refreshOptions, and suspense (and any other places that call defaultOptions),
mirroring the side-effect-free behavior of createInfiniteQueryController and
createMutationController; ensure defaultOptions still throws
createMissingQueryClientError() if no client is provided/resolved but does not
modify object state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 36bdf0ae-b6fa-4a43-8178-f697ee79031f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (24)
.changeset/lemon-memes-divide.md.gitignoreknip.jsonpackages/lit-query/examples/lit-query-e2e-app/e2e/lib.mjspackages/lit-query/examples/lit-query-e2e-app/package.jsonpackages/lit-query/examples/lit-query-pagination-app/e2e/lib.mjspackages/lit-query/examples/lit-query-pagination-app/package.jsonpackages/lit-query/examples/lit-query-pagination-app/scripts/dev.mjspackages/lit-query/examples/lit-query-pagination-app/src/api.tspackages/lit-query/examples/lit-query-ssr-app/e2e/lib.mjspackages/lit-query/examples/lit-query-ssr-app/package.jsonpackages/lit-query/examples/lit-query-ssr-app/scripts/dev.mjspackages/lit-query/examples/lit-query-ssr-app/scripts/run-script-with-public-origin.mjspackages/lit-query/examples/lit-query-ssr-app/server/index.mjspackages/lit-query/package.jsonpackages/lit-query/src/createInfiniteQueryController.tspackages/lit-query/src/createMutationController.tspackages/lit-query/src/createQueriesController.tspackages/lit-query/src/createQueryController.tspackages/lit-query/src/tests/infinite-and-options.test.tspackages/lit-query/src/tests/mutation-controller.test.tspackages/lit-query/src/tests/queries-controller.test.tspackages/lit-query/src/tests/query-controller.test.tspackages/lit-query/src/types.ts
✅ Files skipped from review due to trivial changes (8)
- .changeset/lemon-memes-divide.md
- knip.json
- .gitignore
- packages/lit-query/examples/lit-query-pagination-app/package.json
- packages/lit-query/examples/lit-query-pagination-app/scripts/dev.mjs
- packages/lit-query/examples/lit-query-ssr-app/package.json
- packages/lit-query/examples/lit-query-e2e-app/package.json
- packages/lit-query/src/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/lit-query/examples/lit-query-ssr-app/scripts/dev.mjs
Comment on lines
+169
to
+210
| async function ensureServers() { | ||
| const existingApiReady = await isServerReady(`${apiUrl}/api/projects?page=1`) | ||
| const existingWebReady = await isServerReady(baseUrl) | ||
|
|
||
| if (existingApiReady || existingWebReady) { | ||
| if (!allowExistingServer) { | ||
| throw new Error( | ||
| `Refusing to reuse existing server(s) at ${baseUrl} / ${apiUrl}. Set PW_ALLOW_EXISTING_SERVER=true to allow reuse.`, | ||
| ) | ||
| } | ||
|
|
||
| console.log(`Using existing servers at ${baseUrl} and ${apiUrl}`) | ||
| return { api: undefined, web: undefined } | ||
| } | ||
|
|
||
| const api = startProcess('api', ['run', 'dev:api']) | ||
|
|
||
| try { | ||
| await waitForServer( | ||
| `${apiUrl}/api/projects?page=1`, | ||
| api.child, | ||
| api.getStartupError, | ||
| ) | ||
| } catch (error) { | ||
| await stopProcess(api) | ||
| throw error | ||
| } | ||
|
|
||
| const web = startProcess('web', ['run', 'dev:web'], { | ||
| VITE_PAGINATION_API_PORT: String(API_PORT), | ||
| }) | ||
|
|
||
| try { | ||
| await waitForServer(baseUrl, web.child, web.getStartupError) | ||
| } catch (error) { | ||
| await stopProcess(web) | ||
| await stopProcess(api) | ||
| throw error | ||
| } | ||
|
|
||
| return { api, web } | ||
| } |
Contributor
There was a problem hiding this comment.
Asymmetric "existing server" detection can leave the harness in a half-booted state.
The readiness check treats the api-or-web pair as a single gate: if only one side responds, the logic either refuses outright (misleading: the user never asked to reuse the other half) or, when PW_ALLOW_EXISTING_SERVER=true, returns { api: undefined, web: undefined } and proceeds to the scenario with whichever side isn't actually up. Probing each independently and only starting the missing one (or at minimum requiring both to be ready for the reuse path) would make the behavior predictable. It would also avoid the API's request counter being bumped by the readiness probe against /api/projects?page=1 on a shared server between runs.
🛠 Sketch
- const existingApiReady = await isServerReady(`${apiUrl}/api/projects?page=1`)
- const existingWebReady = await isServerReady(baseUrl)
-
- if (existingApiReady || existingWebReady) {
- if (!allowExistingServer) {
- throw new Error(
- `Refusing to reuse existing server(s) at ${baseUrl} / ${apiUrl}. Set PW_ALLOW_EXISTING_SERVER=true to allow reuse.`,
- )
- }
-
- console.log(`Using existing servers at ${baseUrl} and ${apiUrl}`)
- return { api: undefined, web: undefined }
- }
+ const existingApiReady = await isServerReady(`${apiUrl}/api/projects?page=1`)
+ const existingWebReady = await isServerReady(baseUrl)
+
+ if (existingApiReady && existingWebReady) {
+ if (!allowExistingServer) {
+ throw new Error(
+ `Refusing to reuse existing servers at ${baseUrl} / ${apiUrl}. Set PW_ALLOW_EXISTING_SERVER=true to allow reuse.`,
+ )
+ }
+ console.log(`Using existing servers at ${baseUrl} and ${apiUrl}`)
+ return { api: undefined, web: undefined }
+ }
+
+ if (existingApiReady !== existingWebReady) {
+ throw new Error(
+ `Only one of api (${apiUrl}) / web (${baseUrl}) is already running; refusing to start a half-booted stack.`,
+ )
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/lit-query/examples/lit-query-pagination-app/e2e/lib.mjs` around
lines 169 - 210, The current ensureServers() treats api/web readiness as a
single gate, which can return both as reused even if only one is up; change
ensureServers() to probe api and web independently using existingApiReady and
existingWebReady, and then only reuse the ones that are actually ready: if
existingApiReady is true set apiResult = undefined else startProcess('api',
['run','dev:api']) and waitForServer(...) for it; similarly for web with
startProcess('web', ['run','dev:web'], { VITE_PAGINATION_API_PORT:
String(API_PORT) }) and waitForServer(...); when allowExistingServer is false,
throw only if both exist? or follow current policy but ensure you never return {
api: undefined, web: undefined } unless both probes succeeded—so ensure the
function stops/cleans up via stopProcess(...) for any started process on errors
(use the same stopProcess and try/catch pattern already used for startProcess,
waitForServer, and stopProcess).
| totalPages: number | ||
| totalProjects: number | ||
| hasMore: boolean | ||
| projects: Project[] |
Contributor
There was a problem hiding this comment.
Use Array<Project> to satisfy ESLint @typescript-eslint/array-type.
Static analysis flags Project[] here; align with the project's configured array-type style.
✏️ Proposed fix
- projects: Project[]
+ projects: Array<Project>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| projects: Project[] | |
| projects: Array<Project> |
🧰 Tools
🪛 ESLint
[error] 14-14: Array type using 'Project[]' is forbidden. Use 'Array' instead.
(@typescript-eslint/array-type)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/lit-query/examples/lit-query-pagination-app/src/api.ts` at line 14,
The property declaration uses the shorthand array type "projects: Project[]"
which triggers the `@typescript-eslint/array-type` rule; update the declaration to
use the generic form "Array<Project>" for the projects property (the "projects"
field in the relevant type/interface in src/api.ts) so it conforms to the
project's configured array-type style.
Comment on lines
+250
to
+265
| async function captureFailureArtifacts(page) { | ||
| if (!page) { | ||
| return | ||
| } | ||
|
|
||
| const timestamp = new Date().toISOString().replace(/[:.]/g, '-') | ||
| const screenshotPath = join(artifactDir, `${timestamp}-ssr-e2e.png`) | ||
| const htmlPath = join(artifactDir, `${timestamp}-ssr-e2e.html`) | ||
|
|
||
| await mkdir(artifactDir, { recursive: true }) | ||
| await page.screenshot({ path: screenshotPath, fullPage: true }) | ||
| await writeFile(htmlPath, await page.content(), 'utf8') | ||
|
|
||
| console.error(`Saved failure screenshot: ${screenshotPath}`) | ||
| console.error(`Saved failure page HTML: ${htmlPath}`) | ||
| } |
Contributor
There was a problem hiding this comment.
Artifact capture can throw and shadow the real failure.
Unlike the other two harnesses, captureFailureArtifacts here doesn't guard mkdir/screenshot/writeFile. If any of those reject (e.g., disk full, page already closed), the error from the scenario is lost because the catch block at lines 320–322 awaits this function before re-throwing the original error.
🛠 Proposed fix
async function captureFailureArtifacts(page) {
if (!page) {
return
}
const timestamp = new Date().toISOString().replace(/[:.]/g, '-')
const screenshotPath = join(artifactDir, `${timestamp}-ssr-e2e.png`)
const htmlPath = join(artifactDir, `${timestamp}-ssr-e2e.html`)
- await mkdir(artifactDir, { recursive: true })
- await page.screenshot({ path: screenshotPath, fullPage: true })
- await writeFile(htmlPath, await page.content(), 'utf8')
-
- console.error(`Saved failure screenshot: ${screenshotPath}`)
- console.error(`Saved failure page HTML: ${htmlPath}`)
+ try {
+ await mkdir(artifactDir, { recursive: true })
+ await page.screenshot({ path: screenshotPath, fullPage: true })
+ await writeFile(htmlPath, await page.content(), 'utf8')
+ console.error(`Saved failure screenshot: ${screenshotPath}`)
+ console.error(`Saved failure page HTML: ${htmlPath}`)
+ } catch (artifactError) {
+ console.error('Failed to capture failure artifacts:', artifactError)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function captureFailureArtifacts(page) { | |
| if (!page) { | |
| return | |
| } | |
| const timestamp = new Date().toISOString().replace(/[:.]/g, '-') | |
| const screenshotPath = join(artifactDir, `${timestamp}-ssr-e2e.png`) | |
| const htmlPath = join(artifactDir, `${timestamp}-ssr-e2e.html`) | |
| await mkdir(artifactDir, { recursive: true }) | |
| await page.screenshot({ path: screenshotPath, fullPage: true }) | |
| await writeFile(htmlPath, await page.content(), 'utf8') | |
| console.error(`Saved failure screenshot: ${screenshotPath}`) | |
| console.error(`Saved failure page HTML: ${htmlPath}`) | |
| } | |
| async function captureFailureArtifacts(page) { | |
| if (!page) { | |
| return | |
| } | |
| const timestamp = new Date().toISOString().replace(/[:.]/g, '-') | |
| const screenshotPath = join(artifactDir, `${timestamp}-ssr-e2e.png`) | |
| const htmlPath = join(artifactDir, `${timestamp}-ssr-e2e.html`) | |
| try { | |
| await mkdir(artifactDir, { recursive: true }) | |
| await page.screenshot({ path: screenshotPath, fullPage: true }) | |
| await writeFile(htmlPath, await page.content(), 'utf8') | |
| console.error(`Saved failure screenshot: ${screenshotPath}`) | |
| console.error(`Saved failure page HTML: ${htmlPath}`) | |
| } catch (artifactError) { | |
| console.error('Failed to capture failure artifacts:', artifactError) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/lit-query/examples/lit-query-ssr-app/e2e/lib.mjs` around lines 250 -
265, The captureFailureArtifacts function can throw during
mkdir/screenshot/writeFile and mask the original test failure; wrap the
filesystem and Puppeteer calls inside a try/catch within captureFailureArtifacts
(around mkdir, page.screenshot, writeFile and the console.error lines) and on
error log a non-throwing warning (including the caught error) instead of
allowing the exception to propagate so the original scenario error remains the
one re-thrown by the outer catch; keep the existing behavior when page is falsy.
Comment on lines
+22
to
+25
| import { | ||
| getSsrQueryControllerCreationCount, | ||
| resetSsrQueryControllerCreationCount, | ||
| } from '../src/app.ts' |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how this server is invoked and whether a TS loader is wired up.
fd -H -t f 'package.json' packages/lit-query/examples/lit-query-ssr-app | xargs -I{} sh -c 'echo "=== {} ==="; cat {}'
echo
echo "=== Searching for loader flags / tsx usage ==="
rg -nP -C2 '(tsx|ts-node|--loader|--experimental-(strip|transform)-types|--import)' packages/lit-query/examples/lit-query-ssr-app
echo
echo "=== Does src/app.ts exist and is it referenced via .js elsewhere? ==="
fd -H 'app.(ts|mts|js|mjs)' packages/lit-query/examples/lit-query-ssr-app/src
rg -nP "from '(\.\./)+src/app\.(ts|js|mjs)'" packages/lit-query/examples/lit-query-ssr-appRepository: TanStack/query
Length of output: 2422
🏁 Script executed:
#!/bin/bash
# Check vite configuration
fd -H -t f 'vite.config' packages/lit-query/examples/lit-query-ssr-app | head -5
echo
# Look at the full vite config
cat packages/lit-query/examples/lit-query-ssr-app/vite.config.ts 2>/dev/null || cat packages/lit-query/examples/lit-query-ssr-app/vite.config.js 2>/dev/null
echo
# Check e2e and other scripts to see if server is launched differently
echo "=== Checking e2e scripts for server invocation ==="
fd -H -t f '\.mjs$|\.ts$' packages/lit-query/examples/lit-query-ssr-app/e2e packages/lit-query/examples/lit-query-ssr-app/scripts | head -10
echo
rg -n 'server|localhost|import.*index.mjs' packages/lit-query/examples/lit-query-ssr-app/e2e packages/lit-query/examples/lit-query-ssr-app/scriptsRepository: TanStack/query
Length of output: 3652
🏁 Script executed:
#!/bin/bash
# Look at server/index.mjs to see all imports
echo "=== server/index.mjs imports ==="
head -30 packages/lit-query/examples/lit-query-ssr-app/server/index.mjs
echo
echo "=== Check if app.ts file exists and what it exports ==="
head -20 packages/lit-query/examples/lit-query-ssr-app/src/app.ts
echo
echo "=== Search for any other server entry points ==="
fd -H -t f 'index\.(mjs|js|ts)' packages/lit-query/examples/lit-query-ssr-app/server
echo
echo "=== Verify .js imports are actually used ==="
rg -n "from '\.\./src" packages/lit-query/examples/lit-query-ssr-app/server/index.mjsRepository: TanStack/query
Length of output: 1852
Use .js extension for TypeScript imports to maintain idiomatic ESM pattern and consistency.
While tsx is configured for dev:server, the import of ../src/app.ts is inconsistent with the rest of the file which uses ../src/api.js. Node's ESM standard is to use .js specifiers for all imports (even for .ts sources), letting the build tools handle the mapping. This makes the code more portable and aligns with the pattern already used for api.js. Change to '../src/app.js'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/lit-query/examples/lit-query-ssr-app/server/index.mjs` around lines
22 - 25, The import of ../src/app.ts should use a .js specifier to follow
idiomatic ESM and match the rest of the file; update the import statement that
brings in getSsrQueryControllerCreationCount and
resetSsrQueryControllerCreationCount to reference ../src/app.js instead of
../src/app.ts so tooling can map the .ts source while the runtime uses .js
specifiers.
Comment on lines
+152
to
+164
| async function readTemplate() { | ||
| try { | ||
| return await readFile(templatePath, 'utf8') | ||
| } catch (error) { | ||
| if (error && typeof error === 'object' && 'code' in error) { | ||
| throw new Error( | ||
| 'Missing built client assets. Run "pnpm run build" in examples/lit-query-ssr-app first.', | ||
| ) | ||
| } | ||
|
|
||
| throw error | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
readTemplate masks non-ENOENT errors behind the "missing build" message.
The 'code' in error guard matches any SystemError (e.g. EACCES, EISDIR, EMFILE), not just "file not found". A real permissions or FS error while reading an existing dist/index.html will be reported as "Run pnpm run build first", which is misleading. Narrow the branch to ENOENT (and ideally ENOTDIR) so other errors propagate unchanged.
🛠 Proposed fix
} catch (error) {
- if (error && typeof error === 'object' && 'code' in error) {
- throw new Error(
- 'Missing built client assets. Run "pnpm run build" in examples/lit-query-ssr-app first.',
- )
- }
-
- throw error
+ if (
+ error &&
+ typeof error === 'object' &&
+ 'code' in error &&
+ (error.code === 'ENOENT' || error.code === 'ENOTDIR')
+ ) {
+ throw new Error(
+ 'Missing built client assets. Run "pnpm run build" in examples/lit-query-ssr-app first.',
+ )
+ }
+
+ throw error
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function readTemplate() { | |
| try { | |
| return await readFile(templatePath, 'utf8') | |
| } catch (error) { | |
| if (error && typeof error === 'object' && 'code' in error) { | |
| throw new Error( | |
| 'Missing built client assets. Run "pnpm run build" in examples/lit-query-ssr-app first.', | |
| ) | |
| } | |
| throw error | |
| } | |
| } | |
| async function readTemplate() { | |
| try { | |
| return await readFile(templatePath, 'utf8') | |
| } catch (error) { | |
| if ( | |
| error && | |
| typeof error === 'object' && | |
| 'code' in error && | |
| (error.code === 'ENOENT' || error.code === 'ENOTDIR') | |
| ) { | |
| throw new Error( | |
| 'Missing built client assets. Run "pnpm run build" in examples/lit-query-ssr-app first.', | |
| ) | |
| } | |
| throw error | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/lit-query/examples/lit-query-ssr-app/server/index.mjs` around lines
152 - 164, The readTemplate function currently treats any SystemError with a
code property as "missing build"; change the error branch to only handle
file-not-found scenarios by checking error.code === 'ENOENT' (and optionally
error.code === 'ENOTDIR') before throwing the "Run pnpm run build" Error, and
rethrow all other errors unchanged; locate the logic inside readTemplate that
inspects error and narrow the condition to match only 'ENOENT' (and 'ENOTDIR')
so permissions/other FS errors propagate.
Comment on lines
+262
to
+266
| if (method === 'POST' && requestUrl.pathname === '/api/test/fail-next-data') { | ||
| failNextDataRequest = true | ||
| writeJson(res, 200, { ok: true }) | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
/api/test/fail-next-data has no arming guard.
Anyone reaching this server (including the SSR page itself if something hit /api/test/* by accident) can arm the next failure, and CORS is wide open (access-control-allow-origin: *). For an in-repo example/test server this is acceptable, but please keep these routes clearly scoped to local E2E use and consider gating them on process.env.NODE_ENV !== 'production' or a shared secret header so the example isn't ever deployed as-is with remotely writable test hooks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/lit-query/examples/lit-query-ssr-app/server/index.mjs` around lines
262 - 266, The POST handler that sets failNextDataRequest for
'/api/test/fail-next-data' is unguarded (see the branch that checks method ===
'POST' && requestUrl.pathname === '/api/test/fail-next-data' and sets
failNextDataRequest), so add a guard to prevent remote arming: require either
process.env.NODE_ENV !== 'production' or verify a shared secret header (e.g.,
check req.headers['x-test-secret'] === process.env.TEST_SECRET) before flipping
failNextDataRequest, and return a 403 if the check fails; ensure the guard is
applied before calling writeJson so the example route cannot be triggered in
production or by unauthenticated clients.
Comment on lines
+56
to
+88
| "example:install": "npm --prefix examples/lit-query-e2e-app install", | ||
| "example:dev": "pnpm --dir examples/lit-query-e2e-app run dev", | ||
| "example:e2e": "pnpm run build && pnpm --dir examples/lit-query-e2e-app run e2e", | ||
| "example:e2e:headed": "pnpm run build && pnpm --dir examples/lit-query-e2e-app run e2e:headed", | ||
| "example:e2e:query-error": "pnpm run build && pnpm --dir examples/lit-query-e2e-app run e2e:query-error", | ||
| "example:e2e:mutation-error": "pnpm run build && pnpm --dir examples/lit-query-e2e-app run e2e:mutation-error", | ||
| "example:e2e:refetch-button": "pnpm run build && pnpm --dir examples/lit-query-e2e-app run e2e:refetch-button", | ||
| "example:e2e:lifecycle-reconnect": "pnpm run build && pnpm --dir examples/lit-query-e2e-app run e2e:lifecycle-reconnect", | ||
| "example:e2e:lifecycle-contract": "pnpm run build && pnpm --dir examples/lit-query-e2e-app run e2e:lifecycle-contract", | ||
| "example:e2e:all": "pnpm run build && pnpm --dir examples/lit-query-e2e-app run e2e:all", | ||
| "example:e2e:all:nobuild": "pnpm --dir examples/lit-query-e2e-app run e2e:all", | ||
| "example:e2e:all:headed": "pnpm run build && pnpm --dir examples/lit-query-e2e-app run e2e:all:headed", | ||
| "example:pagination:install": "npm --prefix examples/lit-query-pagination-app install", | ||
| "example:pagination:dev": "pnpm --dir examples/lit-query-pagination-app run dev", | ||
| "example:pagination:e2e": "pnpm run build && pnpm --dir examples/lit-query-pagination-app run e2e", | ||
| "example:pagination:e2e:prefetch": "pnpm run build && pnpm --dir examples/lit-query-pagination-app run e2e:prefetch", | ||
| "example:pagination:e2e:error": "pnpm run build && pnpm --dir examples/lit-query-pagination-app run e2e:error", | ||
| "example:pagination:e2e:mutations": "pnpm run build && pnpm --dir examples/lit-query-pagination-app run e2e:mutations", | ||
| "example:pagination:e2e:boundary": "pnpm run build && pnpm --dir examples/lit-query-pagination-app run e2e:boundary", | ||
| "example:pagination:e2e:all": "pnpm run build && pnpm --dir examples/lit-query-pagination-app run e2e:all", | ||
| "example:pagination:e2e:all:headed": "pnpm run build && pnpm --dir examples/lit-query-pagination-app run e2e:all:headed", | ||
| "example:ssr:install": "npm --prefix examples/lit-query-ssr-app install", | ||
| "example:ssr:dev": "pnpm run build && pnpm --dir examples/lit-query-ssr-app run dev", | ||
| "example:ssr:dev:public-origin": "pnpm run build && pnpm --dir examples/lit-query-ssr-app run dev:public-origin", | ||
| "example:ssr:e2e:default": "pnpm run build && pnpm --dir examples/lit-query-ssr-app run e2e", | ||
| "example:ssr:e2e:error": "pnpm run build && pnpm --dir examples/lit-query-ssr-app run e2e:error", | ||
| "example:ssr:e2e:refreshing": "pnpm run build && pnpm --dir examples/lit-query-ssr-app run e2e:refreshing", | ||
| "example:ssr:e2e:public-origin": "pnpm run build && pnpm --dir examples/lit-query-ssr-app run e2e:public-origin", | ||
| "example:ssr:e2e": "pnpm run example:ssr:e2e:default && pnpm run example:ssr:e2e:public-origin", | ||
| "example:ssr:e2e:all:default": "pnpm run build && pnpm --dir examples/lit-query-ssr-app run e2e:all", | ||
| "example:ssr:e2e:all:nobuild": "pnpm --dir examples/lit-query-ssr-app run e2e:all && pnpm --dir examples/lit-query-ssr-app run e2e:public-origin", | ||
| "example:ssr:e2e:all": "pnpm run example:ssr:e2e:all:default && pnpm run example:ssr:e2e:public-origin", | ||
| "demo:setup": "pnpm run example:install && npm --prefix examples/lit-query-e2e-app exec playwright install chromium", |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether the example apps are part of the pnpm workspace and how they reference lit-query.
fd -t f 'pnpm-workspace.yaml' --max-depth 2
fd -t f '^package.json$' packages/lit-query/examples | xargs -I{} sh -c 'echo "=== {} ==="; jq "{name, dependencies, devDependencies}" {}'Repository: TanStack/query
Length of output: 1178
Avoid npm --prefix for example installs inside a pnpm workspace.
Using npm --prefix examples/... creates package-lock.json files in the pnpm-managed workspace and bypasses workspace features. The other scripts consistently use pnpm --dir examples/..., so replace the three npm --prefix install commands (example:install, example:pagination:install, example:ssr:install) with pnpm --dir examples/... install for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/lit-query/package.json` around lines 56 - 88, The three npm install
scripts (example:install, example:pagination:install, example:ssr:install) use
"npm --prefix" which creates package-lock.json and bypasses pnpm workspace
behavior; update each script to use "pnpm --dir <example-dir> install" to match
the other scripts (e.g., change the command for example:install,
example:pagination:install, and example:ssr:install to use pnpm --dir instead of
npm --prefix).
| customElements.define(providerTagName, QueryClientProvider) | ||
| } | ||
|
|
||
| let explicitMutationClient: QueryClient | undefined |
Contributor
There was a problem hiding this comment.
Move explicitMutationClient reset into afterEach to avoid shared-state leakage.
If LC-MUT-02 fails before line 105, subsequent tests that construct ContextMutationHostElement will inadvertently bind to the leaked explicitMutationClient and bypass provider context resolution, masking real failures. Same hardening pattern noted previously for infinite-and-options.test.ts.
🧪 Suggested hardening
-import { describe, expect, it } from 'vitest'
+import { afterEach, describe, expect, it } from 'vitest'
@@
let explicitMutationClient: QueryClient | undefined
+
+afterEach(() => {
+ explicitMutationClient = undefined
+})Also applies to: 71-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/lit-query/src/tests/mutation-controller.test.ts` at line 18, Move
the shared variable explicitMutationClient reset into an afterEach hook to avoid
test leakage: currently explicitMutationClient is declared at top-level and only
reset in-line, so if LC-MUT-02 fails earlier subsequent tests that create
ContextMutationHostElement can inherit the leaked client and bypass provider
resolution; add an afterEach(() => { explicitMutationClient = undefined }) (or
equivalent cleanup) in the mutation-controller.test.ts test suite and ensure any
existing inline resets around lines 71-107 are removed/relocated to this
afterEach to guarantee each test starts with a clean explicitMutationClient.
| customElements.define(providerTagName, QueryClientProvider) | ||
| } | ||
|
|
||
| let explicitQueriesClient: QueryClient | undefined |
Contributor
There was a problem hiding this comment.
Move explicitQueriesClient reset into afterEach.
Same risk pattern as in mutation-controller.test.ts and the previously flagged infinite-and-options.test.ts: if LC-QUERIES-02 fails before line 190, later tests using ContextQueriesHostElement silently bind to the leaked explicit client and skip provider-context coverage.
🧪 Suggested hardening
-import { describe, expect, it } from 'vitest'
+import { afterEach, describe, expect, it } from 'vitest'
@@
let explicitQueriesClient: QueryClient | undefined
+
+afterEach(() => {
+ explicitQueriesClient = undefined
+})Also applies to: 144-192
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/lit-query/src/tests/queries-controller.test.ts` at line 19, Move the
cleanup of the top-level variable explicitQueriesClient into an afterEach block
to avoid leaking the explicit QueryClient between tests; specifically, add or
update an afterEach that sets explicitQueriesClient = undefined (and any related
teardown used for explicit client state) so tests that instantiate
ContextQueriesHostElement never inadvertently reuse a previous explicit client;
target the tests in queries-controller.test.ts that reference
explicitQueriesClient (lines around 144-192 and the declaration at the top) and
remove any existing resets from inside individual tests so the single afterEach
handles it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎯 Changes
Adds an initial
@tanstack/lit-querypackage that brings TanStack Query Core to Lit via reactive controllers.Included in this PR:
QueryClientProviderand query-client resolution helpersuseIsFetching,useIsMutating, anduseMutationStatequeryOptions,infiniteQueryOptions, andmutationOptionsDocumentation
Tests / E2E
Notes
This PR is focused on the initial package integration and API surface for Lit.
✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit
@tanstack/lit-querypackage, a TanStack Query integration for Lit componentsQueryClientProvidercomponent for context management