feat: add architectural lint rules#39
Conversation
…sted-component, no-universal-in-universal)
WalkthroughThis PR adds three ESLint rules, shared JSX and filename helpers, rule docs and tests, and a new plugin architecture config that registers the rules. ChangesArchitecture rule set
Sequence Diagram(s)sequenceDiagram
participant noUniversalRule
participant getImportGraph
participant importGraph
participant contextReport
noUniversalRule->>getImportGraph: load server-metafile.json
getImportGraph->>importGraph: transitiveDeps(filePath)
getImportGraph->>importGraph: findChain(source, target)
noUniversalRule->>contextReport: report transitive universal nesting
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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)
⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 markdownlint-cli2 (0.22.1)rules/no-universal-in-universal/readme.mdmarkdownlint-cli2 v0.22.1 (markdownlint v0.40.0) 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. 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: 3
🧹 Nitpick comments (1)
rules/no-restricted-layer-import/test.js (1)
4-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd export-path coverage for this rule.
Line 4-99 currently validates only
importpaths, but the rule also enforcesexport ... fromandexport * from. Add valid/invalid cases for both export forms to prevent regressions in those visitors.🤖 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 `@rules/no-restricted-layer-import/test.js` around lines 4 - 99, The no-restricted-layer-import tests only cover import statements, so add coverage in the test('no-restricted-layer-import') cases for both export-from forms handled by the rule. Update the valid and invalid arrays with examples using the same layer combinations already present, but expressed as export ... from and export * from, so the visitors in the rule are exercised and regressions are caught. Use the existing messages['restricted layer import'] expectations and the same path/layer examples to keep the new cases aligned with the current rule behavior.
🤖 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 `@rules/no-universal-in-universal/index.js`:
- Line 27: The universal import check in no-universal-in-universal/index.js only
recognizes specifiers ending with .universal, so .universal.js imports are
skipped. Update the source extension guard in the rule logic to also match
.universal.js (and any equivalent universal JS specifier pattern) before the
nested-universal validation runs, keeping the change localized to the import
detection path around the existing endsWith check.
- Around line 36-38: The JSX member-name check in the matching logic is using
the wrong part of the expression for namespace imports. Update the handling in
the JSX element name extraction so the comparison for JSXMemberExpression uses
the namespace/object identifier from the member expression rather than the
property name, and ensure the matching path in this rule correctly recognizes
imports like UI in <UI.Inner />.
In `@rules/no-universal-in-universal/test.js`:
- Around line 22-35: Add a failing test case in the no-universal-in-universal
rule spec to cover namespace imports from a .universal module, using the
existing invalid array in test.js and the messages['universal in universal']
assertion. The new case should import the universal module as a namespace (for
example via ImportNamespaceSpecifier) and render a nested member like
Universal.Inner in Outer.universal.js, so the rule’s import resolution logic is
exercised for member expressions as well as default/named imports.
---
Nitpick comments:
In `@rules/no-restricted-layer-import/test.js`:
- Around line 4-99: The no-restricted-layer-import tests only cover import
statements, so add coverage in the test('no-restricted-layer-import') cases for
both export-from forms handled by the rule. Update the valid and invalid arrays
with examples using the same layer combinations already present, but expressed
as export ... from and export * from, so the visitors in the rule are exercised
and regressions are caught. Use the existing messages['restricted layer import']
expectations and the same path/layer examples to keep the new cases aligned with
the current rule behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: db075aa3-3394-4b98-ba1e-b45c98daeac1
📒 Files selected for processing (12)
index.jsmachinery/ast.jsmachinery/filename.jsrules/no-nested-component/index.jsrules/no-nested-component/readme.mdrules/no-nested-component/test.jsrules/no-restricted-layer-import/index.jsrules/no-restricted-layer-import/readme.mdrules/no-restricted-layer-import/test.jsrules/no-universal-in-universal/index.jsrules/no-universal-in-universal/readme.mdrules/no-universal-in-universal/test.js
| return { | ||
| ImportDeclaration(node) { | ||
| const source = node.source.value | ||
| if (!source.endsWith('.universal')) return |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Universal import detection misses .universal.js specifiers.
Line 27 only matches .universal, so imports like /features/Foo.universal.js are silently skipped and nested-universal violations can pass unreported.
Proposed fix
- if (!source.endsWith('.universal')) return
+ if (!source.endsWith('.universal') && !source.endsWith('.universal.js')) return📝 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.
| if (!source.endsWith('.universal')) return | |
| if (!source.endsWith('.universal') && !source.endsWith('.universal.js')) return |
🤖 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 `@rules/no-universal-in-universal/index.js` at line 27, The universal import
check in no-universal-in-universal/index.js only recognizes specifiers ending
with .universal, so .universal.js imports are skipped. Update the source
extension guard in the rule logic to also match .universal.js (and any
equivalent universal JS specifier pattern) before the nested-universal
validation runs, keeping the change localized to the import detection path
around the existing endsWith check.
Reads @kaliber/build's .kaliber-build/server-metafile.json to detect .universal.js files that transitively depend on other .universal.js files through intermediate regular files. Gracefully degrades to direct-only checking if no metafile exists.
- 27 tests for machinery/importGraph.js covering lifecycle, circular deps, diamond deps, deep chains, external imports, and realistic Kaliber patterns - Expanded rule tests: namespace imports, member expressions, multiple imports - Fix: JSXMemberExpression now checks object name (the namespace) not property name, so <Inner.Component /> correctly flags 'Inner' from .universal import
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
- layerFromSegments: shared segment→layer parsing (internal) - getLayerFromImportPath: import path to layer name - isSublayerOf: replaces raw startsWith(parent + '/') checks - matchesLayerPattern: wildcard + parent matching for allow-lists - isComponentName (ast.js): replaces inline char[0] uppercase checks - no-restricted-layer-import: uses helpers, removed local duplicates - no-nested-component: uses isComponentName from ast.js - Expanded tests: 29 no-nested-component, 43 no-restricted-layer-import
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
rules/no-universal-in-universal/index.js (1)
54-56: 📐 Maintainability & Code Quality | 🔵 TrivialUse
context.cwdinstead of deprecatedcontext.getCwd()
context.getCwd()was deprecated in ESLint 9 and fully removed in ESLint 10. Since the codebase targets modern environments, replace the conditional check with the standard property access.♻️ Suggested change
- const cwd = context.getCwd ? context.getCwd() : process.cwd() + const cwd = context.cwd🤖 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 `@rules/no-universal-in-universal/index.js` around lines 54 - 56, The Program:exit handler in no-universal-in-universal/index.js still uses the deprecated context.getCwd() fallback. Update that logic to read the current working directory from context.cwd directly in the Program:exit function, and remove the conditional/deprecated API usage so getImportGraph(cwd) continues to receive the resolved cwd value.
🤖 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 `@machinery/importGraph.js`:
- Around line 20-34: The module-level cache in getImportGraph is only keyed by
projectRoot, so changes to server-metafile.json are not picked up during
watch/LSP sessions. Update getImportGraph to also track the metafile’s mtime (or
equivalent freshness signal) alongside cachedRoot/cached, and only return the
cached parse when the file timestamp matches; otherwise reread and reparse the
metafile before storing the new cache.
In `@machinery/importGraph.test.js`:
- Around line 9-12: The afterEach cleanup in importGraph.test.js uses an empty
catch block around rmSync, which triggers ESLint no-empty and can fail CI if the
plugin lints itself. Update the catch in the test cleanup to explicitly
acknowledge the intentionally ignored error, or otherwise handle the ignored
error in a way that satisfies the lint rule while keeping the existing
_resetCache and rmSync fixture cleanup behavior.
In `@rules/no-universal-in-universal/index.js`:
- Around line 62-79: The transitive universal nesting check in the
no-universal-in-universal rule is using a path value that may contain Windows
backslashes, which prevents it from matching the esbuild graph keys. Update the
logic around the `relPath` lookup in `index.js` so the path is normalized to
forward slashes before calling the graph methods used by the
`transitiveDeps`/`findChain` flow, ensuring the `for (const dep of allDeps)`
check still reports violations consistently across platforms.
---
Nitpick comments:
In `@rules/no-universal-in-universal/index.js`:
- Around line 54-56: The Program:exit handler in
no-universal-in-universal/index.js still uses the deprecated context.getCwd()
fallback. Update that logic to read the current working directory from
context.cwd directly in the Program:exit function, and remove the
conditional/deprecated API usage so getImportGraph(cwd) continues to receive the
resolved cwd value.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 383c17bc-8c22-4475-ba5a-085bb8a1aeb7
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
.gitignoremachinery/ast.jsmachinery/filename.jsmachinery/importGraph.jsmachinery/importGraph.test.jsrules/no-nested-component/index.jsrules/no-nested-component/test.jsrules/no-restricted-layer-import/index.jsrules/no-restricted-layer-import/test.jsrules/no-universal-in-universal/index.jsrules/no-universal-in-universal/readme.mdrules/no-universal-in-universal/test.js
✅ Files skipped from review due to trivial changes (2)
- rules/no-universal-in-universal/readme.md
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (4)
- rules/no-universal-in-universal/test.js
- machinery/filename.js
- machinery/ast.js
- rules/no-nested-component/index.js
| function getImportGraph(projectRoot) { | ||
| if (cached && cachedRoot === projectRoot) return cached | ||
|
|
||
| const metafilePath = join(projectRoot, '.kaliber-build', 'server-metafile.json') | ||
| if (!existsSync(metafilePath)) return null | ||
|
|
||
| try { | ||
| const metafile = JSON.parse(readFileSync(metafilePath, 'utf-8')) | ||
| cached = parseMetafile(metafile) | ||
| cachedRoot = projectRoot | ||
| return cached | ||
| } catch { | ||
| return null | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Module-level cache is never invalidated when the metafile changes.
The docstring states the metafile is rewritten during watch mode, but the cache is keyed only on projectRoot and persists for the lifetime of the process. In a long-lived ESLint session (editor/LSP or --watch), a rebuild that updates server-metafile.json will not be picked up, so transitive-nesting results in no-universal-in-universal can go stale until the process restarts.
Consider invalidating on file mtime.
♻️ Possible approach using mtime
-const { readFileSync, existsSync } = require('node:fs')
+const { readFileSync, existsSync, statSync } = require('node:fs')
...
let cached = null
let cachedRoot = null
+let cachedMtimeMs = null
...
function getImportGraph(projectRoot) {
- if (cached && cachedRoot === projectRoot) return cached
-
const metafilePath = join(projectRoot, '.kaliber-build', 'server-metafile.json')
if (!existsSync(metafilePath)) return null
+
+ const mtimeMs = statSync(metafilePath).mtimeMs
+ if (cached && cachedRoot === projectRoot && cachedMtimeMs === mtimeMs) return cached
try {
const metafile = JSON.parse(readFileSync(metafilePath, 'utf-8'))
cached = parseMetafile(metafile)
cachedRoot = projectRoot
+ cachedMtimeMs = mtimeMs
return cached
} catch {
return null
}
}📝 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.
| function getImportGraph(projectRoot) { | |
| if (cached && cachedRoot === projectRoot) return cached | |
| const metafilePath = join(projectRoot, '.kaliber-build', 'server-metafile.json') | |
| if (!existsSync(metafilePath)) return null | |
| try { | |
| const metafile = JSON.parse(readFileSync(metafilePath, 'utf-8')) | |
| cached = parseMetafile(metafile) | |
| cachedRoot = projectRoot | |
| return cached | |
| } catch { | |
| return null | |
| } | |
| } | |
| const { readFileSync, existsSync, statSync } = require('node:fs') | |
| let cached = null | |
| let cachedRoot = null | |
| let cachedMtimeMs = null | |
| function getImportGraph(projectRoot) { | |
| const metafilePath = join(projectRoot, '.kaliber-build', 'server-metafile.json') | |
| if (!existsSync(metafilePath)) return null | |
| const mtimeMs = statSync(metafilePath).mtimeMs | |
| if (cached && cachedRoot === projectRoot && cachedMtimeMs === mtimeMs) return cached | |
| try { | |
| const metafile = JSON.parse(readFileSync(metafilePath, 'utf-8')) | |
| cached = parseMetafile(metafile) | |
| cachedRoot = projectRoot | |
| cachedMtimeMs = mtimeMs | |
| return cached | |
| } catch { | |
| return null | |
| } | |
| } |
🤖 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 `@machinery/importGraph.js` around lines 20 - 34, The module-level cache in
getImportGraph is only keyed by projectRoot, so changes to server-metafile.json
are not picked up during watch/LSP sessions. Update getImportGraph to also track
the metafile’s mtime (or equivalent freshness signal) alongside
cachedRoot/cached, and only return the cached parse when the file timestamp
matches; otherwise reread and reparse the metafile before storing the new cache.
| afterEach(() => { | ||
| _resetCache() | ||
| try { rmSync(fixtureRoot, { recursive: true, force: true }) } catch {} | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Empty catch block flagged by ESLint (no-empty).
If the plugin lints its own sources, this [error] will fail CI. Add a comment (or destructure the ignored error) to satisfy the rule while keeping the intentional swallow.
🧹 Proposed fix
- try { rmSync(fixtureRoot, { recursive: true, force: true }) } catch {}
+ try { rmSync(fixtureRoot, { recursive: true, force: true }) } catch { /* fixtures may not exist */ }📝 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.
| afterEach(() => { | |
| _resetCache() | |
| try { rmSync(fixtureRoot, { recursive: true, force: true }) } catch {} | |
| }) | |
| afterEach(() => { | |
| _resetCache() | |
| try { rmSync(fixtureRoot, { recursive: true, force: true }) } catch { /* fixtures may not exist */ } | |
| }) |
🧰 Tools
🪛 ESLint
[error] 11-11: Empty block statement.
(no-empty)
🤖 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 `@machinery/importGraph.test.js` around lines 9 - 12, The afterEach cleanup in
importGraph.test.js uses an empty catch block around rmSync, which triggers
ESLint no-empty and can fail CI if the plugin lints itself. Update the catch in
the test cleanup to explicitly acknowledge the intentionally ignored error, or
otherwise handle the ignored error in a way that satisfies the lint rule while
keeping the existing _resetCache and rmSync fixture cleanup behavior.
Source: Linters/SAST tools
| for (const dep of allDeps) { | ||
| if (!dep.endsWith('.universal.js')) continue | ||
|
|
||
| // Skip if already caught by the direct check | ||
| const depWithoutExtension = dep.replace(/\.js$/, '') | ||
| const isDirectImport = [...directUniversalSources].some( | ||
| source => depWithoutExtension.endsWith(source.replace(/^\//, '')) | ||
| ) | ||
| if (isDirectImport) continue | ||
|
|
||
| const chain = graph.findChain(relPath, dep) | ||
| if (!chain) continue | ||
|
|
||
| context.report({ | ||
| message: messages['transitive universal nesting'](chain.join(' → ')), | ||
| loc: { start: { line: 1, column: 0 }, end: { line: 1, column: 0 } }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how esbuild metafile input keys are produced/consumed in this repo
fd -t f 'server-metafile' -H
rg -nP "inputs" -g '*.js' -C2 --iglob '*metafile*'
# Look for any normalization of separators around graph keys
rg -nP "(relative\(|replace\(/\\\\|sep)" rules/no-universal-in-universal machineryRepository: kaliberjs/eslint-plugin
Length of output: 161
Fix Windows path separator mismatch in transitive dependency check.
On Windows, path.relative returns backslashes (\), but esbuild metafile keys (inputs) always use forward slashes (/). The current relPath derived from ESLint's context will not match the graph keys, causing transitiveDeps to return an empty set and silently missing violations.
Normalize relPath to use forward slashes before querying the graph:
- const relPath = relative(cwd, context.filename)
+ const relPath = relative(cwd, context.filename).replace(/\\/g, '/')🤖 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 `@rules/no-universal-in-universal/index.js` around lines 62 - 79, The
transitive universal nesting check in the no-universal-in-universal rule is
using a path value that may contain Windows backslashes, which prevents it from
matching the esbuild graph keys. Update the logic around the `relPath` lookup in
`index.js` so the path is normalized to forward slashes before calling the graph
methods used by the `transitiveDeps`/`findChain` flow, ensuring the `for (const
dep of allDeps)` check still reports violations consistently across platforms.
1 participant
Cherry-picked from
feature/no-duplicate-code— just the architecture rules, without the duplicate code engine (superseded by the semantic clone engine).Rules added
no-restricted-layer-importEnforce architectural layer boundaries — prevents imports that cross forbidden layer boundaries (e.g.
machinery/importing fromfeatures/).no-nested-componentPrevent specific React components from being nested inside each other (e.g.
ContainerinsideContainer,HeadinginsideHeading).no-universal-in-universalDisallow rendering universal components inside other universal files — universal components create separate hydration boundaries and must not be nested.
What's NOT included
The
no-duplicate-coderule and its 400-line text-based engine have been dropped — the semantic clone engine (semantic-clone-research) fully supersedes it with Type 1–4 detection via AI embeddings + a VS Code extension.Summary by CodeRabbit