chore: update comments and remove unused fields#1280
Conversation
ysmolski
requested review from
Noroth,
StarpTech,
devsergiy and
jensneuse
as code owners
WalkthroughRemoved or reordered fields in several structs, deleted some private fields and their initialization, and added/adjusted doc comments. Changes touch planner visitors/builders and the GraphQLResponse struct; no new functions or control flow were introduced. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
v2/pkg/engine/plan/node_selection_visitor.go (2)
551-565: Nit: typo dissalowTypeName → disallowTypeNameMinor readability fix; keeps the intent clear when scanning the code.
Apply:
- // so we have to skip adding __typename field - dissalowTypeName := requirementsFromInterfaceObject && requirementsToInterfaceObject + // so we have to skip adding __typename field + disallowTypeName := requirementsFromInterfaceObject && requirementsToInterfaceObject @@ - allowTypeName := !dissalowTypeName && i == 0 + allowTypeName := !disallowTypeName && i == 0
675-678: Nit: fix comment typo “stoping” → “stopping”Small polish to comment text.
- // skip walking into a rewritten field instead of stoping the whole visitor + // skip walking into a rewritten field instead of stopping the whole visitorv2/pkg/engine/plan/path_builder_visitor.go (1)
418-421: Remove commented-out code to avoid confusionLeftover commented call to processPendingFieldRequirements; if intentionally dead, consider removing.
- // c.processPendingFieldRequirements(ref)v2/pkg/engine/plan/node_selection_builder.go (1)
48-49: Add brief docs for the last two fields for consistencyThe last two fields could benefit from matching inline comments, like the others.
- fieldRefDependsOn map[int][]int - fieldDependencyKind map[fieldDependencyKey]fieldDependencyKind + fieldRefDependsOn map[int][]int // fieldRefDependsOn maps a fieldRef to the list of fieldRefs it depends on (DS-agnostic), used e.g. for suggestion recalculation after rewrites + fieldDependencyKind map[fieldDependencyKey]fieldDependencyKind // fieldDependencyKind marks whether a dependency is from @key or @requires
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
v2/pkg/engine/plan/abstract_selection_rewriter.go(0 hunks)v2/pkg/engine/plan/node_selection_builder.go(1 hunks)v2/pkg/engine/plan/node_selection_visitor.go(1 hunks)v2/pkg/engine/plan/path_builder_visitor.go(1 hunks)v2/pkg/engine/resolve/response.go(1 hunks)
💤 Files with no reviewable changes (1)
- v2/pkg/engine/plan/abstract_selection_rewriter.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
v2/pkg/engine/plan/node_selection_builder.go (3)
v2/pkg/engine/plan/datasource_configuration.go (1)
DataSource(256-269)v2/pkg/engine/plan/datasource_filter_node_suggestions.go (1)
NodeSuggestions(91-96)v2/pkg/engine/plan/federation_metadata.go (1)
FederationFieldConfiguration(73-82)
v2/pkg/engine/resolve/response.go (5)
v2/pkg/engine/resolve/fetch.go (1)
FetchItem(27-32)pkg/engine/resolve/resolve.go (1)
Fetches(318-318)v2/pkg/engine/resolve/fetchtree.go (1)
FetchTreeNode(8-15)v2/pkg/engine/resolve/loader.go (1)
DataSourceInfo(46-49)v2/pkg/engine/resolve/node_object.go (1)
Object(8-16)
🔇 Additional comments (6)
v2/pkg/engine/plan/node_selection_visitor.go (1)
15-16: No-op comment line — OK to keepThe inserted blank comment line in the header block is non-functional and harmless. Keeps spacing readable.
v2/pkg/engine/plan/path_builder_visitor.go (1)
23-24: Cleanup Verified – No Stale References Remaining
Removal ofsuggestionsSelectionReasonsEnabledandskipFieldsRefsfrompath_builder_visitor.gois safe:
- No occurrences of
suggestionsSelectionReasonsEnabledfound anywhere in the repo- All remaining
skipFieldsRefsreferences belong to other visitors (node_selection_visitor, visitor.go, planner.go) and are intendedv2/pkg/engine/plan/node_selection_builder.go (2)
30-47: Helpful field documentation — LGTMThe added inline comments to NodeSelectionResult improve readability and maintenance. No behavior change.
83-85: Reset method remains appropriateKeeping ResetSkipFieldRefs as a narrow-scoped reset hook on the builder is fine given skipFieldsRefs lives in the node selection visitor.
v2/pkg/engine/resolve/response.go (2)
26-41: Doc comment and field reordering — sensible and clearThe lifecycle doc clarifies responsibilities (Planner → PostProcessor → Resolver). Reordering to group RawFetches/Info before Fetches/DataSources is coherent with that flow. No functional issues spotted.
46-49: Ignore removal of RenameTypeName – it’s still in use
TheRenameTypeNamestruct is referenced by the v2 resolve machinery and its tests, so it cannot be removed:
- In v2/pkg/engine/resolve/context.go (line 21),
RenameTypeNames []RenameTypeNameon the Context struct- In v2/pkg/engine/resolve/resolvable.go (line 42), the
renameTypeNames []RenameTypeNamefield and its assignments- In v2/pkg/engine/resolve/resolve_test.go (e.g. around line 1735), test cases populate
RenameTypeNames: []RenameTypeName{…}You can safely disregard the original suggestion.
Likely an incorrect or invalid review comment.
ysmolski
force-pushed
the
yury/eng-7554-06-high-level-planner-overview
branch
from
ee260b9 to
e81b978
Compare
|
All fixed, please review |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
v2/pkg/engine/plan/path_builder_visitor.go (1)
596-605: Bug: early return skips notifying other dependent planners; use continue instead.In addPlannerDependencies, returning when notifyPlannerIdx == plannedOnPlannerId exits the whole function and prevents notifying remaining planners in plannerIds. This likely drops dependency edges.
Apply:
- if notifyPlannerIdx == plannedOnPlannerId { - return - // c.walker.StopWithInternalErr(fmt.Errorf("wrong fetch dependencies planner %d depends on itself", notifyPlannerIdx)) - } + if notifyPlannerIdx == plannedOnPlannerId { + // Skip self-dependency but continue notifying others + continue + // c.walker.StopWithInternalErr(fmt.Errorf("wrong fetch dependencies planner %d depends on itself", notifyPlannerIdx)) + }
♻️ Duplicate comments (1)
v2/pkg/engine/resolve/response.go (1)
26-41: Doc comment + field order: clear and aligned with previous feedback.The new comment crisply documents the data flow, and the field order matches prior review suggestions. Looks good.
🧹 Nitpick comments (2)
v2/pkg/engine/plan/path_builder_visitor.go (1)
212-214: Typo in comment: “occurence” → “occurrence”.- // NOTE: it returns first occurence of such path + // NOTE: it returns first occurrence of such pathv2/pkg/engine/resolve/response.go (1)
26-26: Wording nit in docstring.-// GraphQLResponse contains an ordered tree of fetches and a shape of response. +// GraphQLResponse contains an ordered tree of fetches and the response shape.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
v2/pkg/engine/plan/abstract_selection_rewriter.go(0 hunks)v2/pkg/engine/plan/node_selection_builder.go(1 hunks)v2/pkg/engine/plan/node_selection_visitor.go(1 hunks)v2/pkg/engine/plan/path_builder_visitor.go(1 hunks)v2/pkg/engine/resolve/response.go(1 hunks)
💤 Files with no reviewable changes (1)
- v2/pkg/engine/plan/abstract_selection_rewriter.go
✅ Files skipped from review due to trivial changes (1)
- v2/pkg/engine/plan/node_selection_builder.go
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/pkg/engine/plan/node_selection_visitor.go
🧰 Additional context used
🧬 Code graph analysis (1)
v2/pkg/engine/resolve/response.go (4)
v2/pkg/engine/resolve/node_object.go (1)
Object(8-16)v2/pkg/engine/resolve/fetch.go (1)
FetchItem(27-32)v2/pkg/engine/resolve/fetchtree.go (1)
FetchTreeNode(8-15)v2/pkg/engine/resolve/loader.go (1)
DataSourceInfo(46-49)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
🔇 Additional comments (3)
v2/pkg/engine/plan/path_builder_visitor.go (1)
22-24: No residual references in path_builder_visitor.go
Thergsearch confirms that neithersuggestionsSelectionReasonsEnablednorskipFieldsRefsappear in path_builder_visitor.go. All remainingskipFieldsRefshits are in the node-selection visitors and builders (intended and unrelated to the pathBuilderVisitor changes). No further removals are needed here.v2/pkg/engine/resolve/response.go (2)
47-50: No removal needed: RenameTypeName is still in active useA quick search shows
RenameTypeNameis referenced in both v2 and legacy resolver code as well as in tests, confirming that it remains necessary:
- v2/pkg/engine/resolve/response.go – defines the
RenameTypeNamestruct- v2/pkg/engine/resolve/resolvable.go – holds
renameTypeNames []RenameTypeNamefor resolution logic- v2/pkg/engine/resolve/context.go – carries
RenameTypeNames []RenameTypeNamein the execution context and copies it (line 184)- pkg/engine/resolve/resolve.go – maintains
RenameTypeNames []RenameTypeName(lines 135 & 1645) and its own type definition at line 1648- v2/pkg/engine/resolve/resolve_test.go & pkg/engine/resolve/resolve_test.go – initialize
RenameTypeNames: []RenameTypeName{…}in multiple test casesSince removing this type would break core logic and test coverage, no refactor is needed at this time.
34-41: GraphQLResponse.RenameTypeNames Removal Is a Source-Breaking ChangeDropping the exported
RenameTypeNamesfield fromGraphQLResponsebreaks any downstream code that previously set or read it (and any unkeyed composite literals ofGraphQLResponse), including our own internal consumers. We can see existing references toRenameTypeNamesin:
- v2/pkg/engine/resolve/context.go (definition and copy logic)
- v2/pkg/engine/resolve/resolvable.go (assignment from context)
- pkg/engine/resolve/resolve.go (core rename logic)
- resolve_test.go files (test fixtures still populating
RenameTypeNames)If this removal is intentional:
- Bump the major version per SemVer.
- Document the breaking change in the release notes.
Otherwise, consider a soft-deprecation approach:
- Reintroduce the field in
GraphQLResponsewith a// Deprecated: …comment.- Have any reads return a zero or no-op value.
Finally, manually verify there are no unkeyed composite literals of
GraphQLResponsethroughout the codebase (e.g., viarg -n 'GraphQLResponse\s*{') to guard against silent field-reordering breaks.
Based on Yury's onboarding sessions.
ysmolski
force-pushed
the
yury/eng-7554-06-high-level-planner-overview
branch
from
f2f032a to
0ac9fe8
Compare
2 participants
Based on Yury's onboarding sessions.
Summary by CodeRabbit
Refactor
Documentation
Style
No user-facing feature or behavior changes.