feat(lit-query): add render method to query controllers#10791
Open
EskiMojo14 wants to merge 10 commits into
Open
feat(lit-query): add render method to query controllers#10791EskiMojo14 wants to merge 10 commits into
EskiMojo14 wants to merge 10 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a typed rendering utility and a ChangesRender Method Feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant LitTemplate
participant QueryController
participant renderResult
participant StatusRenderer
LitTemplate->>QueryController: call render(renderers)
QueryController->>renderResult: pass controller.current and renderers
renderResult->>StatusRenderer: invoke renderer matching result.status
StatusRenderer-->>LitTemplate: return rendered output or undefined
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/framework/lit/guides/mutations.md (1)
172-185:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winImport
nothingin the example snippet.This example uses
nothing(Line 179) but the imports shown in this guide only includehtml/LitElement, so copy-pasting the snippet can fail.Proposed doc fix
-import { LitElement, html } from 'lit' +import { LitElement, html, nothing } from 'lit'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/framework/lit/guides/mutations.md` around lines 172 - 185, The example uses the special sentinel nothing in the render() example but the shown imports only include html/LitElement; update the snippet imports to also import nothing (e.g., from 'lit') so copy-pasting works and the pending template can return nothing; ensure the updated example shows nothing alongside html/LitElement imports and keep addTodo.render(...) and the pending/error/success handlers unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/lit-query/src/createMutationController.ts`:
- Line 72: Update the JSDoc in createMutationController.ts: replace the phrase
"query result" with "mutation result" in the comment for the accessor inside
createMutationController (the JSDoc that begins "/** Renders the query result
... */") so the description correctly refers to mutation results.
In `@packages/lit-query/src/createQueryController.ts`:
- Line 19: The import in createQueryController.ts incorrectly treats
RendererResult and ResultRenderers as runtime values; change the import to use
type-only qualifiers for RendererResult and ResultRenderers (e.g., import type {
RendererResult, ResultRenderers } from './render.js') while keeping renderResult
as a normal import, and apply the same type-only import change in
createMutationController.ts and createInfiniteQueryController.ts so all three
modules import those two symbols as types only.
---
Outside diff comments:
In `@docs/framework/lit/guides/mutations.md`:
- Around line 172-185: The example uses the special sentinel nothing in the
render() example but the shown imports only include html/LitElement; update the
snippet imports to also import nothing (e.g., from 'lit') so copy-pasting works
and the pending template can return nothing; ensure the updated example shows
nothing alongside html/LitElement imports and keep addTodo.render(...) and the
pending/error/success handlers unchanged.
🪄 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: bf4f14c7-08aa-4d27-a0e7-1ce5aa4b29b2
📒 Files selected for processing (13)
.changeset/crisp-sloths-sneeze.mddocs/framework/lit/guides/infinite-queries.mddocs/framework/lit/guides/mutations.mddocs/framework/lit/guides/queries.mdpackages/lit-query/src/createInfiniteQueryController.tspackages/lit-query/src/createMutationController.tspackages/lit-query/src/createQueryController.tspackages/lit-query/src/index.tspackages/lit-query/src/render.tspackages/lit-query/src/tests/infinite-and-options.test.tspackages/lit-query/src/tests/mutation-controller.test.tspackages/lit-query/src/tests/query-controller.test.tspackages/lit-query/src/tests/render.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/lit-query/src/tests/infinite-and-options.test.ts`:
- Around line 510-551: Wrap the bodies of the infinite.render tests, including
the additionally referenced tests, in try/finally blocks so controller cleanup
always runs. Keep setup and assertions in the try block, and move each
infinite.destroy() call into the finally block; use the test’s existing infinite
controller variable and preserve all current assertions.
In `@packages/lit-query/src/tests/query-controller.test.ts`:
- Around line 1516-1555: Wrap the bodies of all three render tests, including
“renders by current query status via query.render” and the two following tests,
in try/finally blocks. Keep setup and assertions in the try block, and move each
query.destroy() call into its corresponding finally block to guarantee
controller cleanup when assertions fail.
🪄 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: a79e1635-c1b1-40e4-b419-7cd4d017be7d
📒 Files selected for processing (13)
.changeset/crisp-sloths-sneeze.mddocs/framework/lit/guides/infinite-queries.mddocs/framework/lit/guides/mutations.mddocs/framework/lit/guides/queries.mdpackages/lit-query/src/createInfiniteQueryController.tspackages/lit-query/src/createMutationController.tspackages/lit-query/src/createQueryController.tspackages/lit-query/src/index.tspackages/lit-query/src/render.tspackages/lit-query/src/tests/infinite-and-options.test.tspackages/lit-query/src/tests/mutation-controller.test.tspackages/lit-query/src/tests/query-controller.test.tspackages/lit-query/src/tests/render.test.ts
✅ Files skipped from review due to trivial changes (4)
- docs/framework/lit/guides/mutations.md
- .changeset/crisp-sloths-sneeze.md
- packages/lit-query/src/tests/mutation-controller.test.ts
- docs/framework/lit/guides/queries.md
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/lit-query/src/tests/render.test.ts
- packages/lit-query/src/index.ts
- packages/lit-query/src/createQueryController.ts
- docs/framework/lit/guides/infinite-queries.md
- packages/lit-query/src/createInfiniteQueryController.ts
- packages/lit-query/src/createMutationController.ts
- packages/lit-query/src/render.ts
Comment on lines
+1516
to
+1555
| it('renders by current query status via query.render', async () => { | ||
| const client = new QueryClient({ | ||
| defaultOptions: { | ||
| queries: { | ||
| retry: false, | ||
| }, | ||
| }, | ||
| }) | ||
|
|
||
| const host = new TestControllerHost() | ||
|
|
||
| const query = createQueryController( | ||
| host, | ||
| { | ||
| queryKey: ['query-controller', 'render-01'], | ||
| queryFn: async () => 'ok', | ||
| }, | ||
| client, | ||
| ) | ||
|
|
||
| const pendingUi = query.render({ | ||
| pending: () => 'pending-ui', | ||
| success: () => 'success-ui', | ||
| error: () => 'error-ui', | ||
| }) | ||
| expect(pendingUi).toBe('pending-ui') | ||
|
|
||
| host.connect() | ||
| host.update() | ||
| await waitFor(() => query().isSuccess) | ||
|
|
||
| const successUi = query.render({ | ||
| pending: () => 'pending-ui', | ||
| success: (result) => `success-${result.data}`, | ||
| error: () => 'error-ui', | ||
| }) | ||
| expect(successUi).toBe('success-ok') | ||
|
|
||
| query.destroy() | ||
| }) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use try/finally for controller cleanup to match existing test patterns.
The new render tests call query.destroy() directly at the end, but if an expect throws, the controller won't be cleaned up. Existing tests in this file (e.g., lines 189–208, 237–263, 288–309) wrap the body in try/finally to guarantee cleanup. Adopt the same pattern for consistency and to prevent potential test pollution from lingering observers.
♻️ Proposed fix for the first test (apply similarly to the other two)
const query = createQueryController(
host,
{
queryKey: ['query-controller', 'render-01'],
queryFn: async () => 'ok',
},
client,
)
- const pendingUi = query.render({
- pending: () => 'pending-ui',
- success: () => 'success-ui',
- error: () => 'error-ui',
- })
- expect(pendingUi).toBe('pending-ui')
-
- host.connect()
- host.update()
- await waitFor(() => query().isSuccess)
-
- const successUi = query.render({
- pending: () => 'pending-ui',
- success: (result) => `success-${result.data}`,
- error: () => 'error-ui',
- })
- expect(successUi).toBe('success-ok')
-
- query.destroy()
+ try {
+ const pendingUi = query.render({
+ pending: () => 'pending-ui',
+ success: () => 'success-ui',
+ error: () => 'error-ui',
+ })
+ expect(pendingUi).toBe('pending-ui')
+
+ host.connect()
+ host.update()
+ await waitFor(() => query().isSuccess)
+
+ const successUi = query.render({
+ pending: () => 'pending-ui',
+ success: (result) => `success-${result.data}`,
+ error: () => 'error-ui',
+ })
+ expect(successUi).toBe('success-ok')
+ } finally {
+ query.destroy()
+ }Also applies to: 1557-1591, 1593-1631
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/lit-query/src/tests/query-controller.test.ts` around lines 1516 -
1555, Wrap the bodies of all three render tests, including “renders by current
query status via query.render” and the two following tests, in try/finally
blocks. Keep setup and assertions in the try block, and move each
query.destroy() call into its corresponding finally block to guarantee
controller cleanup when assertions fail.
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
see #10711 - adds a render method based on the query status, based on the Tasks API
✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit
render(...)method to query, mutation, and infinite-query controllers for rendering based onpending/error/successstates.renderResulthelper to support consistent, type-safe result rendering.render(...)selection across states, including fallback behavior when no renderer is provided.