feat: cross-file magic string detection#2336
Conversation
Switches from per-file (3+ occurrences in same file) to cross-file (same literal in 3+ distinct source files) detection. A string repeated across many files is a much stronger signal that it should be a shared constant, enum, or config entry. Key changes: - Cross-file detection: tracks which files each literal appears in - Excludes build artifacts: added out/, .turbo/, coverage/, __generated__/ - Filters CSS utility classes (Tailwind) via lowercase+dash pattern - Filters CSS dimensions (1rem, 100vh, 9999px), variant syntax (ios:, hover:) - Filters package import paths (@), glob patterns, semver, MIME types - Filters TypeScript primitive names and boolean strings - Hoists all regex to module scope (Biome useTopLevelRegex compliance) - Fixes collectFiles to use 2 params (Biome useMaxParams compliance)
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This PR updates the check:magic-strings repo check to detect string literals repeated across 3+ distinct source files (instead of repeated multiple times within a single file), aiming to better identify candidates for shared constants/enums.
Changes:
- Reworked the scanner to aggregate string literals across files and flag values appearing in
MIN_FILES(3+) distinct files. - Expanded ignore rules and excluded directories to reduce noise (build artifacts, common paths, semver-like strings, globs, etc.).
- Updated output formatting and added an advisory-by-default mode with optional
--strictenforcement.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| trimmed.startsWith('export {') || | ||
| trimmed.startsWith('export type {') || |
There was a problem hiding this comment.
shouldSkipLine only skips export { ... } / export type { ... }, but the header comment says strings on export lines should be excluded. This will start scanning export const ... = "...", export * from ..., and export default ... lines and likely introduce noisy candidates. Consider skipping all export statements (e.g. a ^\s*export\b check, similar to check-type-casts.ts’s import/export filter).
| trimmed.startsWith('export {') || | |
| trimmed.startsWith('export type {') || | |
| /^export\b/.test(trimmed) || |
| trimmed.startsWith('// ') || | ||
| trimmed.startsWith('//\t') || | ||
| trimmed === '//' || | ||
| trimmed.startsWith('* ') || | ||
| trimmed === '*' || |
There was a problem hiding this comment.
Comment skipping is narrower than “comment lines” in the header: lines like //# sourceMappingURL=..., /// <reference ...>, or //TODO: won’t match // / //\t / // and will be scanned. If the intent is to skip all single-line comments, use a broader trimmed.startsWith('//') (and similarly consider * lines in block comments).
| trimmed.startsWith('// ') || | |
| trimmed.startsWith('//\t') || | |
| trimmed === '//' || | |
| trimmed.startsWith('* ') || | |
| trimmed === '*' || | |
| trimmed.startsWith('//') || | |
| trimmed.startsWith('*') || |
| // - Look like URLs, relative/absolute paths, hex colors, or CSS custom properties | ||
| // - Are under 3 or over 80 characters | ||
| // - Appear only on import/export/comment lines | ||
| // - Live in build artifacts: out/, dist/, build/, .next/, .expo/, node_modules/ |
There was a problem hiding this comment.
The header comment’s excluded build artifact list doesn’t match EXCLUDED_DIRS below (it now also excludes .turbo/, coverage/, and __generated__/). Updating the comment will keep the script’s documented behavior in sync with what it actually scans.
| // - Live in build artifacts: out/, dist/, build/, .next/, .expo/, node_modules/ | |
| // - Live in build artifacts: out/, dist/, build/, .next/, .expo/, .turbo/, coverage/, __generated__/, node_modules/ |
| // CSS utility class pattern (Tailwind): all lowercase+digits connected by dashes. | ||
| // Matches flex-1, text-lg, bg-primary, text-muted-foreground, space-y-4, etc. | ||
| const RE_CSS_UTILITY = /^[a-z][a-z0-9]*(-[a-z0-9]+)+$/; | ||
| const RE_SEMVER = /^\d+\.\d+\.\d+/; | ||
| // CSS dimension values: 1rem, 1.5rem, 100vh, 9999px, 0.5rem, 100%, etc. | ||
| const RE_CSS_DIMENSION = /^\d+(\.\d+)?(%|rem|em|px|vh|vw|ch|pt|ex|deg|fr|s|ms)$/; | ||
|
|
There was a problem hiding this comment.
RE_CSS_UTILITY only excludes dashed Tailwind tokens, but many common utilities/CSS keywords are single words (e.g. 'flex'). In this repo, 'flex' appears in multiple files (e.g. apps//pages/{404,500}.tsx, packages/web-ui/src/components/) and will be flagged as a “magic string” despite being a normal Tailwind/CSS value. Consider extending the Tailwind/CSS ignore logic (e.g. allowlist common single-token utilities/keywords or broaden the utility regex).
feat: cross-file magic string detection
Summary
check-magic-strings.tsto detect string literals that appear in 3+ distinct source files rather than 3+ times in a single fileout/,.turbo/,coverage/,__generated__/), Tailwind utility classes, CSS dimensions, package import paths, semver strings, glob patterns, and TypeScript primitive names--strictto enforceExample signal caught:
"ios"— 50 files (Platform.OS check that should be a constant)"ADMIN"/"USER"— 20/12 files (role strings)"hiking"/"backpacking"/"camping"— 14/13/11 files (PackCategory literals)common.cancel,auth.signIn, etc.) — already dotted keys that should be typed constants"consumable"/"worn"— 12 files (item attribute values)"HS256"/"refresh_token"/"X-API-Key"— auth/crypto constantsTest plan
bun check:magic-stringsruns and prints violations with exit 0bun check:magic-strings -- --strictexits 1bun lint -- packages/checks/src/check-magic-strings.tsclean (no Biome errors)out/,.next/) excluded from scan🤖 Generated with Claude Code