Skip to content

Commit 923b742

Browse files
jensneuseclaude
andcommitted
refactor(plan): extract caching state out of Visitor into cachingPlannerState
ysmolski flagged that the caching feature added ~900 LOC of two new responsibilities on top of the planner visitor: a parallel ProvidesData tree for caching, and per-fetch caching configuration. Visitor was already 1600+ LOC before; this PR added another 900+ on top. Extracts those two responsibilities into a sub-struct on Visitor: type Visitor struct { ... caching *cachingPlannerState } Sub-struct (Option B) chosen over a sibling visitor (Option A) because the highest-risk part of this code is walker callback ordering. A sibling visitor adds ordering risk with AllowVisitor, datasource planner visitors, DefferOnEnterField, and the cost visitor. A sub-struct preserves callback ordering exactly, with only Visitor registered on the walker. Moved to caching_planner_state.go (936 LOC): - Fields: entityAnalyticsCache, requestScopedVisibleResponseKeys, requestScopedFetchAliases, plannerObjects, plannerCurrentFields, plannerResponsePaths, plannerEntityBoundaryPaths. - Methods: 27 caching-specific methods including configureFetchCaching, trackFieldForPlanner, popFieldsForPlanner, configureSubscriptionEntityCachePopulation, entityCacheAnalytics, polymorphicEntityCacheAnalytics, configureMutationEntityImpact, etc. - Helper: extractKeyFields. Preserved on Visitor (load-bearing): - fieldPlanners — cost visitor captures by pointer. - plannerFields — fetch deps/reasons read it. - fieldEnclosingTypeNames — caching state reads through parent. - Public RequestScopedFetchAlias — kept for external API stability, delegates to v.caching.fetchAlias. Net diff: visitor.go shrunk from 2646 to 1766 LOC (-880, -33%). New caching_planner_state.go: 936 LOC. Three test files updated for moved helpers (call-site rewrites only, no logic change): - visitor_path_normalization_test.go - request_scoped_provides_data_test.go - visitor_subscription_entity_population_test.go Also adds a "Code Comment Conventions" section to CLAUDE.md banning PR/issue/reviewer references in code comments. Two such references that crept in during this refactor have been removed from caching_planner_state.go and visitor_path_normalization_test.go. Tests: full plan, resolve, execution suites green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent dd589f8 commit 923b742

7 files changed

Lines changed: 1002 additions & 920 deletions

CLAUDE.md

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,30 @@ request token) to prevent production abuse. The gate is in `GraphQLHandler.cachi
170170
`router/core/graphql_handler.go`. Disabling L1 via these headers also disables @requestScoped
171171
coordinate L1 (since it shares the `EnableL1Cache` flag).
172172

173+
## Code Comment Conventions
174+
175+
**Never reference pull requests, issue numbers, review threads, or reviewer names in code comments.**
176+
177+
Comments live in the codebase forever and outlive the workflow context they were written in.
178+
A `PR #1259` reference is meaningful for two weeks and noise for the next ten years.
179+
Reviewer attribution (`as requested in ysmolski's review`, `addresses SkArchon's comment`) belongs in commit messages and PR descriptions, never in source files.
180+
181+
If a comment exists to explain a non-obvious behavior, explain the **behavior**, not the historical reason it was added.
182+
183+
```go
184+
// CORRECT — explains the invariant
185+
// isEntityRootField previously compared a non-normalized current path against a
186+
// normalized boundary path. Without normalizing here first, queries that wrap the
187+
// boundary in `... on User { ... }` cause the prefix check to silently fail.
188+
189+
// WRONGreferences the PR / review / ticket where the fix was discussed
190+
// Regression guard for the A42 bug in PR #1259 raised by ysmolski:
191+
// isEntityRootField previously compared a non-normalized current path...
192+
```
193+
194+
This applies to all code comments — production, tests, doc comments, file headers.
195+
Commit messages may reference PRs and reviewers; code may not.
196+
173197
## Testing Conventions
174198

175199
**Before writing or modifying any test, read the package's `CLAUDE.md` if one exists.**

0 commit comments

Comments
 (0)