feat(frameworks): add shared Next.js detector pipeline + enforced next lint (TS/JS)#414
feat(frameworks): add shared Next.js detector pipeline + enforced next lint (TS/JS)#414MacHatter1 wants to merge 2 commits intopeteromallet:mainfrom
Conversation
peteromallet
left a comment
There was a problem hiding this comment.
Review
Well-structured feature with a solid architecture (types → catalog → detect → scanners → phase). The layered separation will make it easy to add frameworks beyond Next.js. That said, there are some correctness issues and the PR is large enough that splitting would help.
Architecture strengths
- Clean layered separation: contracts → data-driven signatures → generic scoring → framework-specific adapters
- Shared
_framework/node/location for cross-language Node tooling - Caching via
lang.review_cacheavoids redundant detection
Correctness issues (high priority)
-
scan_nextjs_use_server_in_clientfalse-positives on valid inline server actions — a'use client'module is allowed to contain functions with'use server'inside the function body (inline server actions, valid Next.js 14+). The scanner flags any'use server'presence in a'use client'module. The PR already correctly handles the reverse case in_find_misplaced_module_use_server_directivewith indentation awareness but doesn't apply the same logic here. -
_has_use_server_directive_anywherematches comments/string literals —_USE_SERVER_LITERAL_REmatches the raw string anywhere on a line, so// don't use 'use server' hereorconst x = "use server"would trigger false positives. -
Error/middleware scanners miss
.js/.jsxextensions —scan_nextjs_error_files_missing_use_clienttargets only.ts/.tsxbut is shared with the JS plugin. Same issue for_is_middleware. JS Next.js projects won't get these checks.
Code quality (medium priority)
-
572-line
phase.pyis copy-paste — ~25 identical blocks (call scanner, iterate entries,make_issue(), log). A data-driven approach (list of scanner configs with id/tier/confidence/summary-template) could cut this to ~50 lines and eliminate copy-paste bugs. -
JS plugin hard-coupled to TS plugin internals —
javascript/phases_nextjs.pyimportsdetect_primary_ts_frameworkdirectly from the TS package. The detection logic is language-agnostic. Consider moving shared framework detection tolanguages/_framework/or at least dropping_ts_from the name. -
Hardcoded framework IDs in generic scoring —
detect.py:100-108hasif sig.id == "nextjs"/elif sig.id == "vite"inside what should be a generic function. Should be a field onFrameworkSignature(e.g.,script_pattern). -
Dead Vite signature —
catalog.py:39-49defines a Vite signature with no scanners, no registry entry, no tests. Forward-looking scaffolding adds confusion.
Missing coverage
-
No unit tests for
run_next_lint— JSON extraction, relativization, and error handling paths untested (only monkeypatched away). -
No
--skip-slowintegration fornext lint— 180s subprocess timeout with no check for the existing--skip-slowflag.
Suggestion: split the PR
This is ~2800 new lines across 23 files bundling generic framework infra + Next.js scanners + next lint + JS parity + a tsc fallback chain change. Consider:
- PR A: Generic framework detection layer (types, catalog, detect, scoring wiring)
- PR B: Next.js scanners + phase +
next lint+ tests - PR C: The
tscfallback chain change inunused.py(unrelated)
Good work on the overall design — the issues above are mostly about tightening the implementation. The false-positive items (#1-3) are the most important to address before merge.
|
Hey @MacHatter1 — nice work on the architecture here, the layered separation (types → catalog → detect → scanners → phase) is solid and will scale well to other frameworks. There are a few things to address before we can merge: Must fix (correctness):
Should fix: Let me know if you have questions on any of these! |
|
@MacHatter1 — one more thing I wanted to share after thinking through the architecture more carefully. This isn't blocking the correctness fixes above, but I think it's worth discussing before we merge because it affects how future framework detectors would be built on top of this. The core idea is greatFramework-specific detection is a real gap right now. A Next.js project has a whole class of issues (misplaced directives, missing The factoring concernThe current approach puts the framework detection layer inside the TypeScript plugin ( The reason this happens is that framework detection ("is this a Next.js project?") is actually language-agnostic — it reads Suggested factoringSomething like: Detection goes in This also naturally solves the The phase.py patternThe other thing worth considering: the 572-line SummaryNone of this takes away from the quality of the scanners themselves or the detection logic — those are solid. It's really just about where things live so that future framework detectors (React, Vue, Angular, etc.) can follow the same pattern cleanly. Happy to chat through any of this if it's helpful! |
|
@MacHatter1 — I opened #418 to capture some longer-term architectural thinking about how framework detection should work at scale, building on what you've started here. The TL;DR: your scanners and detection logic are great, but we think framework detection should follow the tree-sitter pattern — spec-driven, data-driven, working as a horizontal layer across both shallow and deep plugins. That way adding a new framework is just adding a spec file, and both TS and JS get coverage from the same spec automatically (no cross-plugin imports needed). Would love your thoughts on it since you've done the actual work of building the first framework detector and know best what the scanner authoring experience needs to look like. The issue has the full reasoning and a concrete proposed structure. |
|
@peteromallet I've converted this PR to a draft for now until a way forward from #418 is reached. |
PR: Next.js framework smell depth + shared framework detection + JS parity
Summary
Before this branch, Desloppify had no dedicated Next.js framework detector layer. Next.js repos were only evaluated by generic TypeScript/Javascript detectors (smells, coupling, security, etc.).
This PR adds a shared TypeScript framework detection layer and deep Next.js framework smell scanning, then reuses that stack for JavaScript so Next.js issues are detected consistently across TS/JS projects.
It also integrates
next lintas a framework-level quality gate: when Next.js is primary and lint cannot run, we emit anext_lint::unavailableissue instead of silently skipping.Root Cause
package.json.use_server_not_firstproduced false positives for valid inline server actions.What Changed
Shared framework layer
desloppify/languages/typescript/frameworks/*desloppify/languages/typescript/frameworks/nextjs/info.pydesloppify/languages/typescript/frameworks/nextjs/phase.pydesloppify/languages/typescript/frameworks/nextjs/scanners.pyNext.js lint integration (enforced behavior)
desloppify/languages/_framework/node/next_lint.pynpx --no-install next lint --format json.next_lint::unavailable(failing issue).JavaScript parity
desloppify/languages/javascript/phases_nextjs.pydesloppify/languages/javascript/__init__.pyDetector and scoring wiring
nextjsandnext_lintin detector catalog and display ordering.Scanner depth (Next.js)
Includes high-value checks for:
app/next/navigationusage inpages/process.envnon-NEXT_PUBLIC_*)next/headmisuse in App Routernext/documentmisuseuse clientwhere neededFalse-positive fixes
use serverplacement logic so valid nested inline server actions are not flagged.External path detection fix
package.jsonfor external scan targets.Impacted Shared Surfaces
desloppify/languages/typescript/frameworks/detect.pydesloppify/languages/typescript/phases_smells.pydesloppify/base/discovery/source.pydesloppify/base/registry/catalog_entries.pydesloppify/base/registry/catalog_models.pydesloppify/engine/_scoring/policy/core.pyWhy This Is Systemic
Regression Protection
Added/updated tests
desloppify/languages/typescript/tests/test_ts_nextjs_framework.pydesloppify/languages/javascript/tests/test_js_nextjs_framework.pydesloppify/tests/review/work_queue_cases.pydesloppify/languages/typescript/tests/test_ts_unused.pyTest result
6051 passed, 145 skippedOSS Benchmark (baseline vs this branch)
All scans run with:
--lang typescript --profile objective --skip-slow --no-badgeImportant context: baseline has
nextjs=0in these comparisons because baseline did not include a Next.js-specific detector path.Control repos (no expected Next.js smells)
vercel/next-app-router-playgroundnextjs=0, new:nextjs=0vercel/nextjs-subscription-paymentsnextjs=0, new:nextjs=0Issue-bearing repos
shadcn-ui/taxonomynextjs=0nextjs=1app/+pages/) project smelldescope-sample-apps/linkedin-sample-appnextjs=0nextjs=2next/head"use client")Notes