Explore Apollo Client/Server Enhancement Options and Guidance #325
Conversation
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
jason-t-hankins
added
the
documentation
Contributor
Reviewer's GuideAdds four architecture decision records (ADRs) documenting Apollo Client/Server performance and caching strategies, and enables Mermaid diagram support in the Docusaurus docs app to render the new sequence/flow diagrams. Sequence diagram for permission-aware cache lookup in a GraphQL resolversequenceDiagram
participant Client
participant Resolver
participant Cache
participant Database
Client->>Resolver: Query protectedField
Resolver->>Resolver: Evaluate user permissions
alt Unauthorized
Resolver-->>Client: Error or null
else Authorized
Resolver->>Cache: get(cacheKey(userId, role, permissions, query, variables))
alt Cache hit
Cache-->>Resolver: Cached result
Resolver-->>Client: Cached result
else Cache miss
Resolver->>Database: Execute query
Database-->>Resolver: Fresh data
Resolver->>Cache: set(cacheKey, data, ttl)
Resolver-->>Client: Fresh data
end
end
Flow diagram for Apollo Client cache policy selection tiersgraph TD
Start[Query execution] --> Sensitivity{Data sensitivity}
Sensitivity -->|Highly sensitive<br/>passwords, OTP, cards| NoCache[no-cache]
Sensitivity -->|Sensitive<br/>balances, private messages| NetworkOnly[network-only]
Sensitivity -->|User-specific<br/>feeds, dashboards| CacheAndNetwork[cache-and-network]
Sensitivity -->|Public/static<br/>plans, profiles| CacheFirst[cache-first]
NoCache --> NoCacheBehavior[Bypass cache entirely]
NetworkOnly --> NetworkOnlyBehavior[Always fetch from network,<br/>do not reuse cached result]
CacheAndNetwork --> CacheAndNetworkBehavior[Return cached data immediately,<br/>then refresh in background]
CacheFirst --> CacheFirstBehavior[Use cache if present,<br/>fallback to network on miss]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In the ADR markdown files there are a few copy/paste artifacts that make them harder to read (e.g., 0026 has both a structured
## Consequenceslist and a separate### Consequencesbullet section, and 0024 has## More Informationduplicated); consider consolidating these sections so each ADR has a single, clearly structured consequences/info block. - The
PermissionAwareCacheexample in 0026 currently omits some implementation details (e.g.,this.cacheis never initialized andmatchesPatternis referenced but not defined); it would be helpful to either stub these explicitly or annotate the snippet as pseudo-code to avoid confusion for readers trying to copy the pattern. - In 0027, the reference to "See ADR 0001 for re-render optimization details" appears inconsistent with the ADR numbering introduced in this PR (the useFragment guidance lives in 0024); updating cross-references to the correct ADR IDs will make the guidance easier to navigate.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the ADR markdown files there are a few copy/paste artifacts that make them harder to read (e.g., 0026 has both a structured `## Consequences` list and a separate `### Consequences` bullet section, and 0024 has `## More Information` duplicated); consider consolidating these sections so each ADR has a single, clearly structured consequences/info block.
- The `PermissionAwareCache` example in 0026 currently omits some implementation details (e.g., `this.cache` is never initialized and `matchesPattern` is referenced but not defined); it would be helpful to either stub these explicitly or annotate the snippet as pseudo-code to avoid confusion for readers trying to copy the pattern.
- In 0027, the reference to "See ADR 0001 for re-render optimization details" appears inconsistent with the ADR numbering introduced in this PR (the useFragment guidance lives in 0024); updating cross-references to the correct ADR IDs will make the guidance easier to navigate.
## Individual Comments
### Comment 1
<location> `apps/docs/docs/decisions/0025-public-graphql-caching.md:284` </location>
<code_context>
+
+| Query | Public? | Reason |
+|-------|---------|--------|
+| `userById()` | ✅ Yes | Should be able to see users pages, specifics may be blocked |
+| `accountPlans()` | ✅ Yes | Shown to unauthenticated users during signup |
+| `currentUser()` | ❌ No | requires authentication and could cause errors |
</code_context>
<issue_to_address>
**issue (typo):** Fix possessive form in "users pages"
Change to the possessive form, e.g. "users' pages" or "user pages," for correct grammar.
```suggestion
| `userById()` | ✅ Yes | Should be able to see users' pages, specifics may be blocked |
```
</issue_to_address>
### Comment 2
<location> `apps/docs/docs/decisions/0024-usefragment-vs-httpbatch-dataloader.md:128` </location>
<code_context>
+ - **Research**: Cloudflare study shows 35-50% improvement in multi-query scenarios
+
+ **Important Limitation:**
+ - HTTP Batching does not provide improvement on static web pages or sites with minimal queries.
+ - Large batchIntervals and small batchIntervals will have linear effects on the performance depending on the number of simultaneous requests. (If you have a large batch and a small number of requests, you may end up waiting longer than necessary)
+
+### Implementation for ShareThrift
</code_context>
<issue_to_address>
**suggestion (typo):** Fix "batchIntervals" spacing and consider simplifying wording
In "Large batchIntervals and small batchIntervals", this should likely be "batch intervals" (with a space). You could also avoid repeating the term by rephrasing, e.g., "Large and small batch intervals will have linear effects..."
```suggestion
- Large and small batch intervals will have linear effects on performance depending on the number of simultaneous requests. (If you have a large batch and a small number of requests, you may end up waiting longer than necessary)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
apps/docs/docs/decisions/0024-usefragment-vs-httpbatch-dataloader.md
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces comprehensive guidance for Apollo Client/Server performance optimization in ShareThrift through four new Architecture Decision Records (ADRs). The documentation is based on practical experimentation in a separate "Social-Feed" demo repository and provides actionable patterns for GraphQL optimization at multiple layers: client-side rendering, network transport, server-side caching, and public CDN caching.
Key Changes
- ADR 0024 establishes guidance on three complementary optimization patterns: useFragment for re-render reduction, HTTP batching for network consolidation, and DataLoader for N+1 query elimination
- ADR 0025 defines a dual-endpoint architecture pattern for enabling CDN caching of public GraphQL content while maintaining security for authenticated requests
- ADR 0026 documents a permission-aware caching strategy that safely caches server-side data while preventing unauthorized access across different user roles
- ADR 0027 provides a tiered client-side caching strategy based on data sensitivity, from public static content to highly sensitive credentials
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 18 comments.
| File | Description |
|---|---|
apps/docs/docs/decisions/0024-usefragment-vs-httpbatch-dataloader.md |
Documents GraphQL optimization patterns (useFragment, HTTP batching, DataLoader) with implementation guidance, trade-offs, and validation testing results |
apps/docs/docs/decisions/0025-public-graphql-caching.md |
Defines separate endpoint architecture for public vs authenticated GraphQL requests with CDN caching support using APQ and GET requests |
apps/docs/docs/decisions/0026-permission-aware-caching.md |
Establishes permission-aware cache key structure to safely cache server-side GraphQL responses while preventing data leakage between user roles |
apps/docs/docs/decisions/0027-client-side-caching.md |
Specifies tiered Apollo Client cache policies based on data sensitivity (cache-first, cache-and-network, network-only, no-cache) with field-level masking |
apps/docs/docs/decisions/0024-usefragment-vs-httpbatch-dataloader.md
Outdated
Show resolved
Hide resolved
apps/docs/docs/decisions/0024-usefragment-vs-httpbatch-dataloader.md
Outdated
Show resolved
Hide resolved
jasonmorais
reviewed
Dec 16, 2025
Contributor
jasonmorais
left a comment
jasonmorais
left a comment
There was a problem hiding this comment.
Didnt fully granulary go through the last file but i think theres enough to do for now:
-
We dont want actual code examples in here. Where there is code try to explain with english, or try to use mermaid diagrams. This should be a high level document/documents. An example is in the pr i linked in one of the comments for a mermaid diagram. I would also verify all common sections are included, besides the one i mentioned, along with keeping that consistency
-
Please resolve the copilot and sourcery comments remaining.
apps/docs/docs/decisions/0024-usefragment-vs-httpbatch-dataloader.md
Outdated
Show resolved
Hide resolved
apps/docs/docs/decisions/0024-usefragment-vs-httpbatch-dataloader.md
Outdated
Show resolved
Hide resolved
apps/docs/docs/decisions/0024-usefragment-vs-httpbatch-dataloader.md
Outdated
Show resolved
Hide resolved
apps/docs/docs/decisions/0024-usefragment-vs-httpbatch-dataloader.md
Outdated
Show resolved
Hide resolved
apps/docs/docs/decisions/0024-usefragment-vs-httpbatch-dataloader.md
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `apps/docs/docs/decisions/0024-usefragment-vs-httpbatch-dataloader.md:91` </location>
<code_context>
+
+### Automatic Persisted Queries (APQ) Compatibility
+
+APQ sends query hashes instead of full query strings to reduce request size. DataLoader, HTTP Batching (POST), useFragment are compatible with APQ. However, HTTP Batching (GET) is not compatible.Must choose between HTTP Batching (POST) OR CDN Caching (GET) - cannot use both simultaneously. **GET mode** (`useGETForHashedQueries: true`)
+
+### Fragment Colocation vs Container Pattern
</code_context>
<issue_to_address>
**suggestion (typo):** Fix minor grammar in the APQ compatibility sentence.
Please fix the missing space and tweak the phrasing, e.g.: `DataLoader, HTTP Batching (POST), and useFragment are compatible with APQ. However, HTTP Batching (GET) is not compatible. You must choose between HTTP Batching (POST) OR CDN Caching (GET) – you cannot use both simultaneously.`
```suggestion
APQ sends query hashes instead of full query strings to reduce request size. DataLoader, HTTP Batching (POST), and useFragment are compatible with APQ. However, HTTP Batching (GET) is not compatible. You must choose between HTTP Batching (POST) OR CDN Caching (GET) – you cannot use both simultaneously. **GET mode** (`useGETForHashedQueries: true`)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
jasonmorais
reviewed
Dec 18, 2025
Contributor
jasonmorais
left a comment
jasonmorais
left a comment
There was a problem hiding this comment.
One small comment, after that will forward to nick.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Created 4 new ADRs to develop actionable guidance on how and when to implement performance enhancements to the Apollo Client/Server within Sharethrift. The following four areas were explored in a sample 'social media' repo, as well as documented in separate ADRs:
Click here to test sample repo
Closes #240
Summary by Sourcery
Document GraphQL performance and caching strategies for ShareThrift and enable Mermaid diagram support in the docs site.
Enhancements:
Documentation: