Skip to content

Conversation

@cngonzalez
Copy link
Member

@cngonzalez cngonzalez commented Sep 2, 2025

Description

There was a subtle bug that could happen if you used a nested ResourceProvider with a perspective (or any other way of implicitly getting a perspective) that pointed to the same project and dataset as another ResourceProvider, wherein you might get the wrong query result. You can see this if you run this branch and check the new route in Kitchensink without the queryStore fix, as below:

Screenshot 2025-09-02 at 3.26.59 PM.png

Every query store is instanced to a project and dataset, and it caches the results of a query. If you were to make the exact same query in a different provider with a different perspective, your "new" perspective would not be saved in the query key, and you would be served the old cached query result with from the "wrong" perspective, because neither query saved their perspective as part of their query key.

(this wasn't a problem with something like useDocument({...documentHandle, perspective}) because all options got memoized into the key)

This PR rectifies this by forcing every query key to have a perspective. It also adds tests and e2e tests. New results below:

Screenshot 2025-09-02 at 3.32.19 PM.png

What to review

The only significant logic change is in queryStore.ts. Everything else is testing / future-proofing.

  • The new PerspectivesRoute component that demonstrates nested ResourceProvider components with different perspectives
  • The updated query store implementation that now properly separates cache entries by perspective
  • The new e2e test that verifies published panels don't show draft content

Testing

Added a comprehensive e2e test that:

  • Creates a published author document
  • Creates a draft overlay with modified content
  • Verifies that the drafts panel shows the draft content
  • Verifies that the published panel shows only the published content

Fun gif

@vercel
Copy link

vercel bot commented Sep 2, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
sdk-docs Ready Ready Preview Comment Sep 2, 2025 7:59pm
sdk-kitchensink-react Ready Ready Preview Comment Sep 2, 2025 7:59pm

Copy link
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@cngonzalez cngonzalez changed the title fix(core): add perspective to cache key to avoid collisions fix(core): add perspective to query cache key to avoid collisions Sep 2, 2025
[selectedDocument, selectedPerspective],
)

const documentProjectionOptions = useMemo(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related, but the old implementation caused infinite loading due to recalculating the params passed to documentProjection. That might be something we want to resolve in the hook itself at a later date, but it was annoying me 😅

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm I thought kitchensink was using react 19 and compiler, maybe we removed it

@cngonzalez cngonzalez force-pushed the 09-02-fix_core_add_perspective_to_cache_key_to_avoid_collisions branch from ec7f9d0 to 31a82c6 Compare September 2, 2025 14:14
@cngonzalez cngonzalez marked this pull request as ready for review September 2, 2025 14:21
@cngonzalez cngonzalez requested a review from a team as a code owner September 2, 2025 14:21
@cngonzalez cngonzalez requested a review from binoy14 September 2, 2025 14:21
@cngonzalez cngonzalez enabled auto-merge (squash) September 2, 2025 15:07
@cngonzalez cngonzalez requested a review from Copilot September 2, 2025 15:10
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a query cache collision bug that occurred when using nested ResourceProviders with different perspectives but the same project/dataset. The core issue was that query cache keys didn't include perspective information, causing incorrect cached results to be served across different perspective contexts.

Key changes:

  • Added perspective normalization to query cache keys to prevent collisions
  • Created comprehensive test coverage for both implicit and explicit perspective scenarios
  • Added a demonstration route and e2e tests to verify the fix

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
packages/core/src/query/queryStore.ts Core fix: normalizes query options to include perspective in cache keys
packages/core/src/query/queryStore.test.ts Unit tests for perspective-based cache separation
apps/kitchensink-react/src/routes/PerspectivesRoute.tsx Demo component showing nested ResourceProviders with different perspectives
apps/kitchensink-react/src/AppRoutes.tsx Route registration for the new perspectives demo
apps/kitchensink-react/src/routes/releases/ReleasesRoute.tsx Refactored to use memoized projection options
apps/kitchensink-react/e2e/perspectives.spec.ts E2e test verifying published panels don't show draft content

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Contributor

@binoy14 binoy14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense to me, thanks

[selectedDocument, selectedPerspective],
)

const documentProjectionOptions = useMemo(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm I thought kitchensink was using react 19 and compiler, maybe we removed it

@cngonzalez cngonzalez merged commit c68f9b7 into main Sep 2, 2025
19 checks passed
@cngonzalez cngonzalez deleted the 09-02-fix_core_add_perspective_to_cache_key_to_avoid_collisions branch September 2, 2025 20:01
@squiggler squiggler bot mentioned this pull request Sep 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants