feat(#72): SPI v1 slice 1c-v.a — React Router substrate#113
Merged
Conversation
Adds React Router (v6.4+ data-router idiom) substrate detection. Two structural patterns:
1. Router factories — createBrowserRouter / createHashRouter / createMemoryRouter / createStaticRouter. The factory call's receiving variable is tagged with framework_role: 'react_router_router'.
2. Route-module convention — when a file imports from 'react-router' or 'react-router-dom' AND exports a named function or const called exactly 'loader' or 'action', those exports are tagged with framework_role: 'react_router_loader' / 'react_router_action'.
Substrate: three new SpiFrameworkRole variants (react_router_router, react_router_loader, react_router_action).
Detector (src/pipeline/spi/framework-react-router.ts): Collects imports from react-router(-dom) — tracks aliased factory names and a hasReactRouterImport flag. Walks top-level VariableStatements for factory-call initializers; walks top-level FunctionDeclarations and VariableStatements for loader/action named exports. Skips loader/action exports in files that do not import react-router(-dom).
Projector wiring: frameworkForRole maps react_router_* prefix → 'react-router'. nodeKindForRole: react_router_router → 'router', loader/action → 'function'.
Tests: 12 new in spi-framework-react-router.test.ts. Cover: all four router factories, aliased factory imports, react-router (not -dom) module, function-form vs variable-form loader/action exports, negative case (loader/action in non-react-router file not tagged), exact-name match (loaderHelper / myAction skipped), end-to-end projection through to ExtractionNode.
Out of scope (deferred):
* JSX route definitions: <Routes><Route path='/x' element={<X />} /></Routes>. Slice 1c-v.b.
* Hook detection (useNavigate, useLoaderData, useActionData) — structurally non-essential; consumers can filter on framework_role.
📝 WalkthroughWalkthroughThis PR adds React Router framework detection to the SPI v1 type-checker pass. It defines new framework roles, detects router factories and route-module exports in source files, integrates detection into the per-file build loop, maps detected roles to projection output, and provides comprehensive test validation of both detection and projector propagation. ChangesReact Router Framework Detection
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pipeline/spi/framework-react-router.ts`:
- Around line 101-108: The current import scan only sets
bindings.hasReactRouterImport when stmt.importClause exists, so side-effect
imports like import "react-router-dom" are ignored; change the logic in the
ts.isImportDeclaration branch that checks REACT_ROUTER_MODULE_SPECIFIERS so that
if stmt is an import declaration and stmt.moduleSpecifier matches
REACT_ROUTER_MODULE_SPECIFIERS you immediately set bindings.hasReactRouterImport
= true (even if stmt.importClause is null), and only after that check
stmt.importClause and named bindings (ts.isNamedImports) to iterate
named.elements for loader/action detection; keep the existing checks
(ts.isImportDeclaration, ts.isStringLiteral, REACT_ROUTER_MODULE_SPECIFIERS,
named/ts.isNamedImports, named.elements) but reorder so side-effect imports are
counted.
- Around line 78-81: The code currently tags any exported function declaration
by checking ts.isFunctionDeclaration(stmt) && stmt.name &&
hasExportModifier(stmt), but this also catches default exports like `export
default function loader()` which should be excluded; update the condition to
ensure the function is not a default export (e.g., require
!hasDefaultModifier(stmt) or equivalent) before consulting
ROUTE_MODULE_EXPORT_NAMES and calling tagSymbolByName(ctx, stmt.name.text, role)
so only named exports (export function ...) are tagged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c2f5234f-46cf-4dbc-bd47-9e0df5ebcef7
📒 Files selected for processing (6)
src/pipeline/spi/build.tssrc/pipeline/spi/framework-react-router.tssrc/pipeline/spi/index.tssrc/pipeline/spi/projector.tssrc/pipeline/spi/types.tstests/unit/spi-framework-react-router.test.ts
Comment on lines
+78
to
+81
| if (ts.isFunctionDeclaration(stmt) && stmt.name && hasExportModifier(stmt)) { | ||
| const role = ROUTE_MODULE_EXPORT_NAMES.get(stmt.name.text) | ||
| if (role) tagSymbolByName(ctx, stmt.name.text, role) | ||
| continue |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the specific file
find . -name "framework-react-router.ts" -type fRepository: mohanagy/graphify-ts
Length of output: 109
🏁 Script executed:
# Check the file size and read the relevant section
wc -l src/pipeline/spi/framework-react-router.tsRepository: mohanagy/graphify-ts
Length of output: 111
🏁 Script executed:
# Read lines around 78-81 with context
sed -n '60,100p' src/pipeline/spi/framework-react-router.tsRepository: mohanagy/graphify-ts
Length of output: 1693
🏁 Script executed:
# Look for hasExportModifier and related functions
rg "hasExportModifier|hasDefaultModifier" src/pipeline/spi/framework-react-router.tsRepository: mohanagy/graphify-ts
Length of output: 266
🏁 Script executed:
# Check what ROUTE_MODULE_EXPORT_NAMES contains
rg "ROUTE_MODULE_EXPORT_NAMES" src/pipeline/spi/framework-react-router.ts -B 5 -A 5Repository: mohanagy/graphify-ts
Length of output: 1127
🏁 Script executed:
# Let's read the full hasExportModifier function implementation
sed -n '120,147p' src/pipeline/spi/framework-react-router.tsRepository: mohanagy/graphify-ts
Length of output: 832
🏁 Script executed:
# Check if there are any tests or examples with default exports
rg "export default" src/pipeline/spi/framework-react-router.ts -B 3 -A 3Repository: mohanagy/graphify-ts
Length of output: 46
Exclude default exports from loader/action named-export tagging.
The current code tags all function declarations with an export modifier, but export default function loader() is a default export, not a named loader export. Only named exports (e.g., export function loader()) should be tagged per React Router conventions.
Add a check to exclude default exports:
Suggested fix
- if (ts.isFunctionDeclaration(stmt) && stmt.name && hasExportModifier(stmt)) {
+ if (
+ ts.isFunctionDeclaration(stmt) &&
+ stmt.name &&
+ hasExportModifier(stmt) &&
+ !hasDefaultModifier(stmt)
+ ) {
const role = ROUTE_MODULE_EXPORT_NAMES.get(stmt.name.text)
if (role) tagSymbolByName(ctx, stmt.name.text, role)
continue
} function hasExportModifier(node: ts.Node): boolean {
const modifiers = ts.canHaveModifiers(node) ? ts.getModifiers(node) : undefined
if (!modifiers) return false
for (const mod of modifiers) {
if (mod.kind === ts.SyntaxKind.ExportKeyword) return true
}
return false
}
+
+function hasDefaultModifier(node: ts.Node): boolean {
+ const modifiers = ts.canHaveModifiers(node) ? ts.getModifiers(node) : undefined
+ if (!modifiers) return false
+ for (const mod of modifiers) {
+ if (mod.kind === ts.SyntaxKind.DefaultKeyword) return true
+ }
+ return false
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (ts.isFunctionDeclaration(stmt) && stmt.name && hasExportModifier(stmt)) { | |
| const role = ROUTE_MODULE_EXPORT_NAMES.get(stmt.name.text) | |
| if (role) tagSymbolByName(ctx, stmt.name.text, role) | |
| continue | |
| if ( | |
| ts.isFunctionDeclaration(stmt) && | |
| stmt.name && | |
| hasExportModifier(stmt) && | |
| !hasDefaultModifier(stmt) | |
| ) { | |
| const role = ROUTE_MODULE_EXPORT_NAMES.get(stmt.name.text) | |
| if (role) tagSymbolByName(ctx, stmt.name.text, role) | |
| continue |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pipeline/spi/framework-react-router.ts` around lines 78 - 81, The code
currently tags any exported function declaration by checking
ts.isFunctionDeclaration(stmt) && stmt.name && hasExportModifier(stmt), but this
also catches default exports like `export default function loader()` which
should be excluded; update the condition to ensure the function is not a default
export (e.g., require !hasDefaultModifier(stmt) or equivalent) before consulting
ROUTE_MODULE_EXPORT_NAMES and calling tagSymbolByName(ctx, stmt.name.text, role)
so only named exports (export function ...) are tagged.
Comment on lines
+101
to
+108
| if (!ts.isImportDeclaration(stmt) || !stmt.importClause) continue | ||
| if (!ts.isStringLiteral(stmt.moduleSpecifier)) continue | ||
| if (!REACT_ROUTER_MODULE_SPECIFIERS.has(stmt.moduleSpecifier.text)) continue | ||
|
|
||
| bindings.hasReactRouterImport = true | ||
| const named = stmt.importClause.namedBindings | ||
| if (!named || !ts.isNamedImports(named)) continue | ||
| for (const element of named.elements) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "framework-react-router.ts" | head -5Repository: mohanagy/graphify-ts
Length of output: 109
🏁 Script executed:
cd . && cat -n src/pipeline/spi/framework-react-router.ts | head -130Repository: mohanagy/graphify-ts
Length of output: 5923
🏁 Script executed:
python3 << 'EOF'
# Quick verification: TypeScript side-effect imports
# In TypeScript, a side-effect import like: import "react-router-dom"
# Has: { importClause: undefined }
# A normal import like: import { Router } from "react-router-dom"
# Has: { importClause: { namedBindings: { elements: [...] } } }
print("TypeScript import statement structure:")
print("1. import 'module' => importClause is undefined")
print("2. import {} from 'module' => importClause.namedBindings exists")
print("")
print("Therefore, checking '!stmt.importClause' would skip side-effect imports.")
EOFRepository: mohanagy/graphify-ts
Length of output: 289
Count side-effect imports as React Router imports.
At Line 101, the guard !stmt.importClause prevents side-effect imports (import "react-router-dom") from setting hasReactRouterImport, causing files with side-effect imports and named loader/action exports to not be tagged as route modules.
Suggested fix
function collectBindings(sourceFile: ts.SourceFile): ReactRouterBindings {
const bindings: ReactRouterBindings = {
routerFactories: new Set<string>(),
hasReactRouterImport: false,
}
for (const stmt of sourceFile.statements) {
- if (!ts.isImportDeclaration(stmt) || !stmt.importClause) continue
+ if (!ts.isImportDeclaration(stmt)) continue
if (!ts.isStringLiteral(stmt.moduleSpecifier)) continue
if (!REACT_ROUTER_MODULE_SPECIFIERS.has(stmt.moduleSpecifier.text)) continue
bindings.hasReactRouterImport = true
- const named = stmt.importClause.namedBindings
+ const named = stmt.importClause?.namedBindings
if (!named || !ts.isNamedImports(named)) continue
for (const element of named.elements) {
const importedName = element.propertyName?.text ?? element.name.text
if (ROUTER_FACTORY_NAMES.has(importedName)) {
bindings.routerFactories.add(element.name.text)
}
}
}
return bindings
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!ts.isImportDeclaration(stmt) || !stmt.importClause) continue | |
| if (!ts.isStringLiteral(stmt.moduleSpecifier)) continue | |
| if (!REACT_ROUTER_MODULE_SPECIFIERS.has(stmt.moduleSpecifier.text)) continue | |
| bindings.hasReactRouterImport = true | |
| const named = stmt.importClause.namedBindings | |
| if (!named || !ts.isNamedImports(named)) continue | |
| for (const element of named.elements) { | |
| function collectBindings(sourceFile: ts.SourceFile): ReactRouterBindings { | |
| const bindings: ReactRouterBindings = { | |
| routerFactories: new Set<string>(), | |
| hasReactRouterImport: false, | |
| } | |
| for (const stmt of sourceFile.statements) { | |
| if (!ts.isImportDeclaration(stmt)) continue | |
| if (!ts.isStringLiteral(stmt.moduleSpecifier)) continue | |
| if (!REACT_ROUTER_MODULE_SPECIFIERS.has(stmt.moduleSpecifier.text)) continue | |
| bindings.hasReactRouterImport = true | |
| const named = stmt.importClause?.namedBindings | |
| if (!named || !ts.isNamedImports(named)) continue | |
| for (const element of named.elements) { | |
| const importedName = element.propertyName?.text ?? element.name.text | |
| if (ROUTER_FACTORY_NAMES.has(importedName)) { | |
| bindings.routerFactories.add(element.name.text) | |
| } | |
| } | |
| } | |
| return bindings | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pipeline/spi/framework-react-router.ts` around lines 101 - 108, The
current import scan only sets bindings.hasReactRouterImport when
stmt.importClause exists, so side-effect imports like import "react-router-dom"
are ignored; change the logic in the ts.isImportDeclaration branch that checks
REACT_ROUTER_MODULE_SPECIFIERS so that if stmt is an import declaration and
stmt.moduleSpecifier matches REACT_ROUTER_MODULE_SPECIFIERS you immediately set
bindings.hasReactRouterImport = true (even if stmt.importClause is null), and
only after that check stmt.importClause and named bindings (ts.isNamedImports)
to iterate named.elements for loader/action detection; keep the existing checks
(ts.isImportDeclaration, ts.isStringLiteral, REACT_ROUTER_MODULE_SPECIFIERS,
named/ts.isNamedImports, named.elements) but reorder so side-effect imports are
counted.
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
1 participant
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.
First React Router slice — substrate-level detection for the v6.4+ data-router patterns.
Patterns detected
Two structural patterns:
Router factories — `createBrowserRouter` / `createHashRouter` / `createMemoryRouter` / `createStaticRouter`. The factory call's receiving variable is tagged with `framework_role: 'react_router_router'`.
Route-module convention — when a file imports from `'react-router'` or `'react-router-dom'` AND exports a named function or const called exactly `loader` or `action`, those exports are tagged with `framework_role: 'react_router_loader'` / `'react_router_action'`.
Substrate
Three new `SpiFrameworkRole` variants:
Projector wiring
Out of scope (deferred)
Test plan
Refs #72.
Summary by CodeRabbit
Release Notes