chore(FR-3028): add ESLint rule forbidding fragment $key type at its spread site - #9518
Draft
yomybaby wants to merge 3 commits into
Draft
chore(FR-3028): add ESLint rule forbidding fragment $key type at its spread site#9518yomybaby wants to merge 3 commits into
yomybaby wants to merge 3 commits into
Conversation
…spread site
Add `bai/no-fragment-key-at-spread-site` to eslint-config-bai, registered at
`error` for `**/*.{ts,tsx}` in the shared `react` config so both `react/` and
`packages/backend.ai-ui/` pick it up.
The rule flags typing a local value (a `useState` cell or a plain variable)
with a fragment's generated `$key` at a site that already spreads that fragment
in a `graphql` literal. The local query response already carries
`$fragmentSpreads` and is assignable to `Foo$key`, so importing the child's
`$key` only adds type coupling to the child; the type should be derived from
the local query node instead.
Exempt: the fragment-owning file (it defines the fragment), prop and callback
parameter positions (the fragment-component prop contract), and files that
never spread the fragment.
Fixes the 12 violations the rule reports on main by deriving local query node
types and dropping the now-unused `$key` imports. Type annotations only — no
runtime behaviour changes.
Reviving PR #7692 by nowgnuesLee, which was closed unmerged; the rule and the
call-site fixes are re-derived against current main, and the rule now has a
RuleTester suite in the root vitest project.
Co-Authored-By: Seungwon Lee <seungwon@lablup.com>
Claude-Session: https://claude.ai/code/session_011RFmSEBxxxvCXyquSPtqVJ
Contributor
Coverage Report for root-coverage
File CoverageNo changed files found. |
Contributor
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Four moderate rule-correctness and coverage issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a shared ESLint rule forbidding Relay fragment $key types at local fragment spread sites and updates existing violations.
Changes:
- Adds and enables
bai/no-fragment-key-at-spread-site. - Adds RuleTester and CI coverage.
- Replaces affected
$keyannotations with locally derived query types.
Required fixes:
- Moderate (2 votes): Parse or tokenize GraphQL literals to avoid comment/string false positives and false negatives.
- Moderate (2 votes): Track generated GraphQL imports and aliases instead of matching identifier spelling alone.
- Moderate (1 vote): Traverse object type members so nested state shapes are checked.
- Moderate (1 vote): Recognize
React.useStateand add corresponding test coverage.
File summaries
| File | Description |
|---|---|
vitest.config.ts |
Includes ESLint rule tests in the root suite. |
react/src/pages/ReservoirArtifactDetailPage.tsx |
Uses locally derived revision node types. |
react/src/pages/ProjectAdminDeploymentsPage.tsx |
Derives revision state locally. |
react/src/pages/DeploymentListPage.tsx |
Derives revision state locally. |
react/src/components/UserSettingModal.tsx |
Derives created-keypair state locally. |
react/src/components/UserResourcePolicyList.tsx |
Derives policy state locally. |
react/src/components/ResourcePresetList.tsx |
Derives preset state locally. |
react/src/components/ProjectResourcePolicyList.tsx |
Derives policy state locally. |
react/src/components/KeypairResourcePolicyList.tsx |
Derives policy state types locally. |
react/src/components/DeploymentReplicasCard.tsx |
Derives revision state and removes a cast. |
react/src/components/BulkCreateUserFromCSVModal.tsx |
Derives created-keypair state locally. |
react/src/components/AdminUserCredentialList.tsx |
Derives keypair state locally. |
packages/eslint-config-bai/react.js |
Enables the new rule for TypeScript. |
packages/eslint-config-bai/bai-plugin.test.js |
Adds RuleTester coverage. |
packages/eslint-config-bai/bai-plugin.js |
Implements the rule; requires the fixes listed above. |
.github/workflows/vitest-root.yml |
Runs root tests for ESLint-config changes. |
Review details
- Files reviewed: 16/16 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+45
to
+50
| if ( | ||
| typeNode.type === "TSTypeReference" && | ||
| typeNode.typeName?.type === "Identifier" && | ||
| isKeyTypeName(typeNode.typeName.name) | ||
| ) { | ||
| out.push(typeNode); |
Comment on lines
+61
to
+64
| const typeArgs = typeNode.typeArguments || typeNode.typeParameters; | ||
| if (typeArgs && Array.isArray(typeArgs.params)) { | ||
| typeArgs.params.forEach((c) => collectKeyTypeRefs(c, out)); | ||
| } |
Skip GraphQL `#` comments and string / block-string tokens before the textual scan, so a spread or a fragment definition named only in prose neither registers as a spread nor exempts a real violation. Recognize `React.useState<Foo$key>` alongside the bare call form; the member-expression spelling is established in packages/backend.ai-ui. Four RuleTester cases cover both, keeping the suite at 13 passing. Claude-Session: https://claude.ai/code/session_011RFmSEBxxxvCXyquSPtqVJ
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Two moderate correctness issues can cause false positives and allow aliased generated $key imports to bypass the rule.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
packages/eslint-config-bai/bai-plugin.js:87
- This does not verify that the referenced name is an imported Relay generated type. It therefore reports unrelated local types named
Foo$key, while an actual generated import can bypass the rule viaimport { Foo$key as FooKey } ...becauseFooKeyno longer has the suffix. The issue's contract explicitly requires an import from__generated__/*.graphql; collect those import specifiers (including their local aliases) and match candidates through that map, with positive and negative RuleTester cases.
if (
typeNode.type === "TSTypeReference" &&
typeNode.typeName?.type === "Identifier" &&
isKeyTypeName(typeNode.typeName.name)
) {
out.push(typeNode);
- Files reviewed: 16/16 changed files
- Comments generated: 1
- Review effort level: Balanced
Close a block string at its first UNESCAPED `"""`. A `\"""` escape is the one sequence a GraphQL block string has, and treating it as the closing delimiter let the rest of the string be scanned as syntax. Claude-Session: https://claude.ai/code/session_011RFmSEBxxxvCXyquSPtqVJ
Member
Author
|
Ready-gate status — two Copilot passes run, PR stays a draft pending two decisions. Fixed and resolved (3):
Rule suite 9 -> 14 passing; Left open for a human call (2) — both threads are unreplied on purpose:
|
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
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.
Resolves #7691 (FR-3028)
Summary
Adds a custom
bai/no-fragment-key-at-spread-siteESLint rule toeslint-config-bai, registered aterrorfor**/*.{ts,tsx}in the sharedreactconfig, so bothreact/andpackages/backend.ai-ui/pick it up.The rule flags typing a local value (a
useStatecell or a plain variable annotation) with a Relay fragment's generated$keytype at a site that already spreads that fragment in agraphqlliteral. When a component spreads...Fooin its own query/fragment, the response already carries$fragmentSpreadsand is assignable toFoo$key— so importing the child's$keythere only adds type coupling to the child. The type should be derived from the local query node instead.This is a nice-to-have project convention, not a Relay-mandated pattern.
Exemptions (intentionally not flagged)
fragment Foo on ...— they legitimately useFoo$keyfor their prop type and/oruseFragment<Foo$key>....Foo(the ref is sourced from a prop elsewhere).... on Typeinline fragments, and JS object/array spreads outside agraphqlliteral.Design decisions
graphqlliteral, not the whole source. Relay literals in this repo carry no${}interpolation, so concatenating the quasis gives the complete document — and scanning the literal rather than the file is what keeps JS spread operators and doc strings from being mistaken for fragment spreads.eslint-config-baiis plain ESM with no build step and no vitest config of its own, sovitest.config.tsnow includespackages/eslint-config-bai/**/*.test.js(and thepackages/**exclude carves that one package out)..github/workflows/vitest-root.ymlgained the matching path filter.Fixed violations (12, across 11 files)
Replaced
useState<X$key>with the local query node type and dropped the now-unused$keyimport:react/src/components/:AdminUserCredentialList,BulkCreateUserFromCSVModal,DeploymentReplicasCard,KeypairResourcePolicyList(2),ProjectResourcePolicyList,ResourcePresetList,UserResourcePolicyList,UserSettingModalreact/src/pages/:DeploymentListPage,ProjectAdminDeploymentsPage,ReservoirArtifactDetailPagePlural fragment refs (
UserSettingModal,BulkCreateUserFromCSVModal,ReservoirArtifactDetailPage) useReadonlyArray<Node>.DeploymentReplicasCardalso drops anas DeploymentRevisionDetail_revision$keycast that the derived type makes unnecessary. Type annotations and imports only — no runtime behaviour changes.Credit
This revives #7692 by @nowgnuesLee, which was closed unmerged on 2026-07-27. The rule is his; the call-site fixes are re-derived against current
main(two of his targets have since been deleted, one was renamedDeploymentReplicasTab→DeploymentReplicasCard, andBulkCreateUserFromCSVModalnewly matches). The RuleTester suite is new.Tests
pnpm exec vitest run packages/eslint-config-bai/bai-plugin.test.js→ 14 passed (8 valid cases covering each exemption plus the comment / string / escaped-block-string scan cases, 6 invalid coveringuseState,React.useState, plural refs, plain variable annotations, a file that defines a different fragment, and afragment Foo on ...that appears only in prose).pnpm exec vitest run(root suite) → 8 files, 171 passed / 6 skipped — confirms the new file is picked up by the root project.pnpm exec eslint srcinreact/and inpackages/backend.ai-ui/→ 0bai/no-fragment-key-at-spread-siteviolations remaining, 0 eslint errors total.Verification
bash scripts/verify.sh→=== ALL PASS ===(Relay / Lint script coverage / Lint / Format / TypeScript / TypeScript (agent-cli) / Vite warmup paths / StyleX cssInjectionTarget / Astryx theme build / Astryx integration / z-index ladder mirrors / Agent mappings / Terminology — all PASS.)
No CSS or theme tokens touched, so the Astryx token gate is not implicated.
Review notes
packages/eslint-config-bai/bai-plugin.test.jsis the fastest read — thevalidarray is the exemption list, one case each.DeploymentReplicasCard.tsx:507, where a now-redundantas ...$keycast is removed from the<Link onClick>handler.tsc(inverify.sh) is the real gate here.vitest.config.tschangespackages/**→packages/!(eslint-config-bai)/**inexclude; the other three package suites (backend.ai-ui,backend.ai-agent-cli,backend.ai-client) keep their own configs and workflows untouched.Checklist: (if applicable)
bash scripts/verify.shends with=== ALL PASS ===packages/eslint-config-bai/bai-plugin.test.jshttps://claude.ai/code/session_011RFmSEBxxxvCXyquSPtqVJ