feat(frameworks): FrameworkSpec layer + Next.js spec (slow next lint)#414
feat(frameworks): FrameworkSpec layer + Next.js spec (slow next lint)#414MacHatter1 wants to merge 3 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. |
… linting - Deleted the TypeScript framework detection module and related classes. - Updated phase smells to remove Next.js framework detection logic. - Refactored tests to utilize new detection methods for Next.js. - Added comprehensive tests for Next.js `next lint` integration, including parsing and tool phase execution. - Ensured that potential issues and coverage warnings are correctly reported during linting.
|
PR description updated to reflect the final Issue #418 / maintainer guidance architecture:
Docs-alignment tweaks (per latest Next.js docs):
Tests: pytest -q → 6061 passed, 144 skipped. |
PR: FrameworkSpec horizontal layer + Next.js spec (Issue #418)
Addresses #418.
Summary
FrameworkSpec) with deterministic presence detection and per-framework phases.FrameworkSpecwith present/absent detection + evidence.next lintas a slow tool phase that respects--skip-slow/include_slow=False.Design (per maintainer guidance)
DetectorPhase.run(path, lang)withLangRuntimeContract— no parallel context object.requires=gates rules, and missing capabilities surface as coverage/confidence degradation, not silent skips.make_tool_phase) rather than adding a parallel system.What Changed
Framework infrastructure
desloppify/languages/_framework/frameworks/types.pydesloppify/languages/_framework/frameworks/registry.pydesloppify/languages/_framework/frameworks/detection.pydesloppify/languages/_framework/frameworks/phases.pypackage.jsonlang.review_cacheNext.js converted to a FrameworkSpec
desloppify/languages/_framework/frameworks/specs/nextjs.pypackage_root(monorepo correctness).Tool integration (
next lint)parse_next_lintparser and tool plumbing updates so framework tools can:cwd(package root)next lintis now a slow phase (skippable).Language plugin wiring
generic_lang(..., frameworks=True)Legacy cleanup
next_lintrunner.Docs alignment
Updated Next.js smells to match current Next.js documentation:
next/navigationimports in Pages Router/shared components.redirect()/permanentRedirect()usage in Client Components as an automatic smell (allowed during render; still discouraged in event handlers).Test Plan
pytest -q6061 passed, 144 skippedOSS smoke scans
Scans run with
--profile objective --skip-slow --no-badge:nextauthjs/next-auth-examplet3-oss/create-t3-turbo(scannedapps/nextjs)