Require variables for legacy GraphQL queries - #8889
Conversation
There was a problem hiding this comment.
Pull request overview
Preserves GraphQL variables during legacy fallback queries, with regression coverage for inherited and replacement variables.
Changes:
- Retains original variables unless fallback values are supplied.
- Adds repository-level regression tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Summary |
|---|---|
src/test/github/githubRepository.test.ts |
Tests inherited and replacement variable behavior. |
src/github/githubRepository.ts |
Preserves variables during legacy fallback. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Dirk Bäumer (@dbaeumer) Alex Ross (@alexr00) could you kindly have a look at this? |
Alex Ross (alexr00)
left a comment
Alex Ross (alexr00)
left a comment
There was a problem hiding this comment.
Thanks for the PR! There does seem to be a bug here, but I think a better fix would be to require that we always pass in new variables, as there is no guarantee that the legacy query and the newer query take the same variables.
a9f362e to
2867ea3
Compare
Tamir Duberstein (tamird)
changed the title
Updated in 2867ea3: Desktop and web builds and all 479 extension-host tests pass for this commit. #8890 has also been rebased on it. Prepared with Codex. |
fd5f8d8 allowed legacy queries to replace their variables, but left the fallback variables optional. PullRequestComments supplied only a query, so retrying discarded owner, name, number, and the pagination cursor and failed with invalid-variable errors. Require variables whenever a legacy query is supplied. Pass the review-comment variables explicitly on every page, while preserving separate argument maps for queries with different inputs. Cover replacement and legacy pagination at the repository boundary. Handle missing repository data before reading review threads, so a missing response produces the intended diagnostic.
2867ea3 to
3a85e91
Compare
Alex Ross (alexr00)
left a comment
Alex Ross (alexr00)
left a comment
There was a problem hiding this comment.
/AzurePipelines run
|
/AzurePipelines run |
|
Azure Pipelines: 2 pipeline(s) were filtered out due to trigger conditions. |
Alex Ross (alexr00)
left a comment
Alex Ross (alexr00)
left a comment
There was a problem hiding this comment.
Looks good, thank you! I'll merge it after we release this week.
|
Alex Ross (@alexr00) thanks! Could you also review #8890 which is stacked on top of this? I'll rebase that one once this one merges, but I don't expect any further code changes. |
fd5f8d8 allowed legacy queries to replace their variables, but left the fallback variables optional. PullRequestComments supplied only a query, so retrying discarded owner, name, number, and the pagination cursor and failed with invalid-variable errors.
Require variables whenever a legacy query is supplied. Pass the review-comment variables explicitly on every page, while preserving separate argument maps for queries with different inputs. Regression coverage exercises replacement and legacy pagination through the repository query boundary.
Handle missing repository data before reading review threads, so a missing response produces the intended diagnostic.
Prepared with Codex.