Skip to content

feat(routes): link Next.js project-level middleware.ts to routes#504

Merged
magyargergo merged 9 commits into
abhigyanpatwari:mainfrom
marxo126:feat/nextjs-middleware-linking
Mar 25, 2026
Merged

feat(routes): link Next.js project-level middleware.ts to routes#504
magyargergo merged 9 commits into
abhigyanpatwari:mainfrom
marxo126:feat/nextjs-middleware-linking

Conversation

@marxo126

Copy link
Copy Markdown
Contributor

Summary

Next.js projects use a top-level middleware.ts file with config.matcher to apply middleware to routes by path pattern. Currently, route_map shows middleware: [] for all Next.js routes because the existing middleware extraction only detects wrapper patterns inside handler files (e.g., export const POST = withAuth(handler)).

This PR adds detection of the project-level middleware.ts file and links its middleware functions to matching routes.

What it does

  • New extractNextjsMiddlewareConfig() — parses middleware.ts files to extract:

    • config.matcher patterns (string arrays, single strings, regex patterns)
    • The exported middleware function name
    • Wrapper composition (chain([withAuth, withI18n]) or nested withA(withB(handler)))
  • New middlewareMatcherMatchesRoute() — matches route URLs against Next.js matcher syntax:

    • Prefix patterns: /api/:path* → matches all /api/... routes
    • Regex patterns: /((?!api|_next).*) → negative lookahead matchers
    • Exact matches: /login
  • Pipeline integration (Phase 3.5b) — after route nodes are created, scans for middleware.ts or src/middleware.ts, parses it, and prepends middleware function names to matching route nodes' middleware array

Why this matters

Many Next.js projects use project-level middleware for auth, i18n, rate limiting, etc. Without this, route_map and api_impact can't show which routes are protected by middleware — making security audits and impact analysis incomplete. This is especially important for:

  • Security visibility: knowing which routes have auth middleware vs. which are unprotected
  • Impact analysis: changing middleware.ts affects all matched routes, not just one handler
  • API documentation: middleware is part of the route's behavior contract

Additive & backward-compatible

The existing wrapper-based middleware detection (extractMiddlewareChain) is unchanged. This new detection runs after it and prepends project-level middleware, so both sources combine correctly. Projects without a middleware.ts file are unaffected.

Test plan

  • 7 new tests for extractNextjsMiddlewareConfig (matcher array, single string, default export, chain composition, nested wrappers, regex matcher, no-match edge case)
  • 4 new tests for middlewareMatcherMatchesRoute (prefix, exact, regex negative lookahead, non-matching)
  • All 70 existing + new tests pass
  • TypeScript build passes
  • Verified against real Next.js project (kurz-spj-sk) — correctly parses config.matcher with regex negative lookahead and correctly excludes /api/* routes that the middleware doesn't target

🤖 Generated with Claude Code

Detect middleware.ts/src/middleware.ts files and parse their config.matcher
patterns. After route nodes are created, match each route URL against the
matchers and prepend the middleware function names to the route's middleware
array. Supports prefix patterns (/api/:path*), regex negative lookahead
patterns, exact matches, chain([fn1, fn2]) composition, and nested wrapper
composition (withA(withB(handler))).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@vercel

vercel Bot commented Mar 24, 2026

Copy link
Copy Markdown

@marxo126 is attempting to deploy a commit to the NexusCore Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions

github-actions Bot commented Mar 24, 2026

Copy link
Copy Markdown
Contributor

CI Report

All checks passed

Pipeline Status

Stage Status Details
✅ Typecheck success tsc --noEmit
✅ Tests success unit tests, 3 platforms
✅ E2E success gitnexus-web changes only

Test Results

Tests Passed Failed Skipped Duration
4213 4212 0 1 173s

✅ All 4212 tests passed

1 test(s) skipped — expand for details
  • buildTypeEnv > known limitations (documented skip tests) > Ruby block parameter: users.each { |user| } — closure param inference, different feature

Code Coverage

Tests

Metric Coverage Covered Base Delta Status
Statements 70.86% 10483/14792 70.74% 📈 +0.1 🟢 ██████████████░░░░░░
Branches 61.48% 7128/11593 61.34% 📈 +0.1 🟢 ████████████░░░░░░░░
Functions 74.35% 922/1240 74.22% 📈 +0.1 🟢 ██████████████░░░░░░
Lines 73.2% 9355/12779 73.05% 📈 +0.2 🟢 ██████████████░░░░░░

📋 View full run · Generated by CI

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds support for linking Next.js project-level middleware.ts (and src/middleware.ts) to route nodes by parsing config.matcher and associating extracted middleware/wrapper names with matching routes in the ingestion pipeline.

Changes:

  • Added extractNextjsMiddlewareConfig() to parse Next.js middleware exports, wrapper composition, and config.matcher patterns.
  • Added middlewareMatcherMatchesRoute() to match Next.js matcher patterns (prefix :path*, regex-style, exact).
  • Integrated middleware linking into the ingestion pipeline and added unit tests for both extraction and matcher-to-route matching.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
gitnexus/src/core/ingestion/route-extractors/middleware.ts Adds Next.js project-level middleware config extraction + matcher matching helper.
gitnexus/src/core/ingestion/pipeline.ts Links parsed project-level middleware to existing Route nodes after route registry creation.
gitnexus/test/unit/route-tool-detection.test.ts Adds unit tests covering middleware config extraction and matcher matching behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +82 to +89
const defaultExportRe = /export\s+default\s+(\w+)/;
const namedExportRe = /export\s+(?:async\s+)?function\s+(\w+)/;
const defaultMatch = defaultExportRe.exec(content);
const namedMatch = namedExportRe.exec(content);
if (defaultMatch) {
exportedName = defaultMatch[1];
} else if (namedMatch) {
exportedName = namedMatch[1];

Copilot AI Mar 24, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

namedExportRe currently matches the first export function <name> in the file, which can mis-identify the middleware export if the file exports helpers in addition to middleware. Also, defaultExportRe will mis-parse export default function middleware(...) as exportedName=function. Consider explicitly detecting export (async )?function middleware and export default (async )?function <name>? (plus export default <identifier>), so exportedName reflects the actual middleware export and avoids false positives.

Suggested change
const defaultExportRe = /export\s+default\s+(\w+)/;
const namedExportRe = /export\s+(?:async\s+)?function\s+(\w+)/;
const defaultMatch = defaultExportRe.exec(content);
const namedMatch = namedExportRe.exec(content);
if (defaultMatch) {
exportedName = defaultMatch[1];
} else if (namedMatch) {
exportedName = namedMatch[1];
// Prefer an explicitly exported `middleware` function (named export).
const namedMiddlewareExportRe = /export\s+(?:async\s+)?function\s+middleware\b/;
// Handle `export default (async )?function <name>?` where the name is optional.
const defaultFunctionExportRe = /export\s+default\s+(?:async\s+)?function(?:\s+(\w+))?/;
// Handle `export default <identifier>` while avoiding mis-parsing `function`.
const defaultIdentifierExportRe = /export\s+default\s+(?!function\b)(\w+)/;
if (namedMiddlewareExportRe.test(content)) {
// Named `middleware` export takes precedence.
exportedName = 'middleware';
} else {
const defaultFunctionMatch = defaultFunctionExportRe.exec(content);
if (defaultFunctionMatch) {
// Use the explicit function name if present; otherwise fall back to "middleware".
exportedName = defaultFunctionMatch[1] ?? 'middleware';
} else {
const defaultIdentifierMatch = defaultIdentifierExportRe.exec(content);
if (defaultIdentifierMatch) {
exportedName = defaultIdentifierMatch[1];
}
}

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ce6cce1. Replaced the single defaultExportRe with three separate patterns: namedMiddlewareExportRe, defaultFunctionExportRe, and defaultIdentifierExportRe. Correctly handles export default function middleware(...) without capturing function as the name.

wrappedFunctions.unshift(exportedName);
}

if (matchers.length === 0 && wrappedFunctions.length === 0) return undefined;

Copilot AI Mar 24, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extractNextjsMiddlewareConfig returns undefined when there is a plain export function middleware(...) without config.matcher and without wrapper composition. In Next.js, a project-level middleware file without config.matcher still applies (defaults to all routes, subject to Next.js exclusions), and the pipeline logic already treats matchers.length === 0 as match-all. Recommend returning a config object whenever a middleware export is present (even if matchers is empty) so routes get linked in this common case.

Suggested change
if (matchers.length === 0 && wrappedFunctions.length === 0) return undefined;
// Only treat this as non-middleware when there is no exported name and no matchers/wrappers
if (!exportedName && matchers.length === 0 && wrappedFunctions.length === 0) return undefined;

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ce6cce1. Middleware files with an export but no config.matcher now return a config with empty matchers array, which the pipeline treats as match-all. This matches Next.js semantics where omitting config.matcher applies middleware to all routes.

Comment on lines +544 to +546
it('returns undefined for file with no matcher and no wrappers', () => {
const content = `export function middleware(req) { return NextResponse.next(); }`;
expect(extractNextjsMiddlewareConfig(content)).toBeUndefined();

Copilot AI Mar 24, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test asserts extractNextjsMiddlewareConfig returns undefined for a middleware file with export function middleware(...) but no config.matcher. If the intent is to link project-level middleware to routes, this common Next.js case should likely produce a config with an empty matcher list (match-all) so the pipeline can attach it to routes.

Suggested change
it('returns undefined for file with no matcher and no wrappers', () => {
const content = `export function middleware(req) { return NextResponse.next(); }`;
expect(extractNextjsMiddlewareConfig(content)).toBeUndefined();
it('treats middleware without config.matcher as match-all', () => {
const content = `export function middleware(req) { return NextResponse.next(); }`;
const result = extractNextjsMiddlewareConfig(content);
expect(result).toBeDefined();
expect(result!.matchers).toEqual([]);
expect(result!.exportedName).toBe('middleware');

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in ce6cce1. Test now expects a defined result with empty matchers and exportedName='middleware'. Also added a test for export default function middleware(...).

@magyargergo

Copy link
Copy Markdown
Collaborator

@claude ultra-think while you are reviewing this PR. Try to find more gaps from next.js features, i want to see a matrix what we support and what's missing from the feature list. You must also review the performance characteristics of the implementation.

@github-actions

github-actions Bot commented Mar 25, 2026

Copy link
Copy Markdown
Contributor

Claude finished @magyargergo's task in 2m 23s —— View job


Deep Review: Next.js Middleware Linking

Good foundational implementation that covers the most common real-world patterns. Below is a full analysis covering bugs, the feature gap matrix, and performance.


Bugs Found

1. export default function middleware(...) mis-parsed — captures "function" as exported name

File: middleware.ts:82-90

const defaultExportRe = /export\s+default\s+(\w+)/;
// ...
if (defaultMatch) {
  exportedName = defaultMatch[1]; // ← "function" when form is: export default function middleware()
}

For export default function middleware(req) { ... }, defaultMatch[1] is "function". Since "function" is not in composerNames, line 123–125 then pushes "function" into wrappedFunctions. This silently attaches the string "function" as a middleware label to every matched route.

Fix this →

2. Plain export function middleware() with no config.matcher returns undefined — should be match-all

File: middleware.ts:127

if (matchers.length === 0 && wrappedFunctions.length === 0) return undefined;

When a file has only export function middleware(req) { ... } with no config.matcher, exportedName is "middleware" which is in composerNames, so it is not added to wrappedFunctions. Result: wrappedFunctions.length === 0 and matchers.length === 0 → returns undefined → middleware is silently ignored.

Per Next.js docs: a middleware without config.matcher applies to all routes. This is a correct gap identified by the Copilot reviewer too.

The fix: detect that a middleware export exists at all and return { matchers: [], exportedName: 'middleware', wrappedFunctions: [] }. The pipeline already handles matchers.length === 0 as match-all (line in the pipeline diff).

Fix this →

3. export const middleware = (req) => ... — arrow function const export not detected

The named export regex /export\s+(?:async\s+)?function\s+(\w+)/ requires the function keyword. Arrow functions assigned to const middleware are a common pattern and are silently missed. The default export path won't save it either.


Performance Review

Regex compiled per-route instead of once — O(routes × matchers) regex compilations

File: middleware.ts:144-150

if (matcher.includes('(')) {
  try {
    const re = new RegExp('^' + matcher + '$'); // ← new object on every call
    return re.test(routeURL);
  } catch { return false; }
}

middlewareMatcherMatchesRoute is called once per route per matcher. For a project with 200 routes and a single regex matcher like /((?!_next|api).*), this creates 200 RegExp objects that are immediately discarded. The function has no caching.

Suggested fix: compile matchers once in the pipeline (or in a helper that returns pre-compiled patterns), then test each route against the compiled set. The pipeline loop is the right place:

// Compile once per middleware config
const compiledMatchers = config.matchers.map(m => {
  if (m.replace(/\/:path\*$/, '') !== m) return { type: 'prefix', prefix: m.replace(/\/:path\*$/, '') };
  if (m.includes('(')) { try { return { type: 'regex', re: new RegExp('^' + m + '$') }; } catch { return null; } }
  return { type: 'exact', value: m };
}).filter(Boolean);
// Then test each route against compiledMatchers

Fix this →

generateId called for every route in the registry even for non-matching routes

File: pipeline.ts (Phase 3.5b diff)

for (const [routeURL] of routeRegistry) {
  const matches = ...;
  if (!matches) continue;
  const routeNodeId = generateId('Route', routeURL); // called after the match check ✅

Actually this is fine — generateId is only called after the match check passes. No issue here.

allPaths linear scan for exact path strings

const middlewareCandidates = allPaths.filter(p =>
  p === 'middleware.ts' || p === 'middleware.js' ||
  p === 'src/middleware.ts' || p === 'src/middleware.js'
);

This is O(n) over all repo files but runs once and is negligible. Fine.


Next.js Middleware Feature Support Matrix

Feature Status Notes
config.matcher array of strings ✅ Supported Core use-case
config.matcher single string ✅ Supported
:path* prefix matching ✅ Supported e.g. /api/:path*
Exact path matching ✅ Supported e.g. /login
Regex-style matchers ✅ Supported e.g. /((?!api|_next).*)
export function middleware() ✅ Supported
export default <identifier> re-export ✅ Supported e.g. export default auth
export default withA(withB(handler)) ✅ Supported Nested wrappers
chain([fn1, fn2]) / compose(fn1, fn2) ✅ Supported
middleware.ts at project root ✅ Supported
src/middleware.ts ✅ Supported
export const middleware = (req) => ... ❌ Missing Arrow function export
export { middleware } at end of file ❌ Missing Named re-export pattern
export default function middleware() ⚠️ Bug Captures "function" as name
No config.matcher → match-all routes ⚠️ Bug Returns undefined instead
:path+ (one-or-more, not zero) ❌ Missing Different from :path*
:param named param without wildcard ❌ Missing e.g. /blog/:slug
config.matcher object syntax ❌ Missing { source, has, missing, locale } — Next.js 13+
has conditional matching ❌ Missing Match on header/cookie/query/host
missing conditional matching ❌ Missing Negative has counterpart
Locale-aware path matching ❌ Missing i18n routing with locale prefix
pipe(fn1, fn2) composition ❌ Missing Less common but used
clerkMiddleware / Auth0 patterns ❌ Missing Popular auth library wrappers
middleware.tsx / .jsx ❌ Missing Unusual but valid
Runtime export detection ❌ Missing export const runtime = 'edge'

The most impactful missing feature is the matcher object syntax with has/missing:

export const config = {
  matcher: [
    { source: '/api/:path*', has: [{ type: 'header', key: 'authorization' }] }
  ]
}

This is increasingly common in production Next.js apps and the current regex config\s*=\s*\{[^}]*matcher\s*:\s*\[([^\]]*)\] will not match it because the array contains {...} objects, and [^\]]* won't capture them (actually it may partially match but extract junk). Worth at least detecting and warning.


Minor Issues

allPaths path format assumption — The filter p === 'middleware.ts' assumes paths are bare relative paths without a leading ./. If the filesystem walker ever normalizes paths differently (e.g. ./middleware.ts), this silently misses the file. A path.basename-based check or endsWith guard would be more robust.

No deduplication guard across multiple middleware files — If both middleware.ts and src/middleware.ts exist (shouldn't happen in a valid Next.js app), both are processed and both prepend to the same route's middleware array. Low risk but worth a comment.

isDev console.log gated logging — The 🛡️ Linked... log is gated behind isDev, which is correct. However the linked count will silently be 0 if extractNextjsMiddlewareConfig returns undefined for the match-all case (bug #2) — making it hard to debug in development.


Summary

The PR is a solid v1 for the most common Next.js middleware patterns and is additive/backward-compatible. The two bugs (default function export mis-parse, and match-all not returned for bare exports) should be fixed before merge since they affect common real-world middleware files. The performance issue with per-route regex compilation is worth fixing for large repos. The feature matrix gaps are expected for a first iteration — the has/missing object syntax is the most critical gap to document.

@magyargergo

Copy link
Copy Markdown
Collaborator

@marxo126 can you please have a look at this?

marxo126 and others added 5 commits March 25, 2026 07:04
- Fix `export default function middleware(...)` being mis-parsed as
  exportedName='function' by using separate regexes for default function
  exports vs default identifier exports
- Return config (with empty matchers = match-all) when middleware.ts has
  a middleware export but no config.matcher, matching Next.js semantics
- Add test for export default function pattern

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Address Deep Review feedback:
- Detect `export const middleware = (req) => ...` arrow function pattern
- Pre-compile regex matchers once per middleware file instead of per-route
  (eliminates O(routes) RegExp allocations for regex-style matchers)
- Add .tsx/.jsx to middleware file candidate extensions

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…st constants

- Extract walkNestedWrappers() shared helper — eliminates duplicated
  wrapper-chain-walking logic between extractMiddlewareChain and
  extractNextjsMiddlewareConfig
- Cache regex test results as booleans, reuse for hasMiddlewareExport
  guard instead of re-testing the same 4 patterns
- Hoist COMPOSER_NAMES Set to module level (was allocated on every call)
- Remove section-divider comments and flatten nested if/else
- Remove JSDoc warning from convenience wrapper

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Extract lastMatchGroup() and buildShapeResult() helpers to eliminate
  repeated patterns in both JS/TS and PHP extractors
- Simplify detectPHPStatusCode to use ?? chaining with lastMatchGroup
- Remove duplicate 9-test PHP describe block (kept the 12-test version)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@magyargergo

Copy link
Copy Markdown
Collaborator

Can you please add integration tests as well? 🙏

Add middleware.ts fixture with config.matcher: ['/api/:path*'] to the
nextjs-route-mapping fixture. Integration tests verify:
- Middleware is linked to matching /api/* routes
- All routes matching the pattern receive middleware
- Routes outside the pattern are not affected

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@marxo126

Copy link
Copy Markdown
Contributor Author

Added integration tests in 7d2fe48:

  • New middleware.ts fixture with config.matcher: ['/api/:path*'] in the nextjs-route-mapping test fixture
  • 3 integration tests verifying:
    • Middleware linked to matching /api/* routes
    • All routes matching the pattern receive middleware
    • Routes outside the pattern are unaffected
  • All 8 integration tests pass (5 existing + 3 new)

Also noting that bugs #1 and #2 from the Deep Review were already fixed in earlier commits (ce6cce1), and the simplification pass (f08a803) extracted a shared wrapper-walking helper, cached regex results, and hoisted constants.

@magyargergo

Copy link
Copy Markdown
Collaborator

Can your please resolve the merge conflicts?

Keep expanded middleware import after upstream sync.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@magyargergo

Copy link
Copy Markdown
Collaborator

@claude Let's run a final review if this PR is production ready and ready to be merged

@github-actions

github-actions Bot commented Mar 25, 2026

Copy link
Copy Markdown
Contributor

Claude finished @magyargergo's task in 3m 46s —— View job


Final Production Readiness Review

  • Read all changed files
  • Verify previously identified bugs were fixed
  • Check merge conflict status
  • Review CI results
  • Assess production readiness

Verdict: ⚠️ Not Ready — One Blocker

The implementation is solid and the major bugs from the deep review are fixed, but there is one blocking issue discovered during this review.


Blocker: Unit Tests Accidentally Removed

Commit b2b8875 ("refactor: extract shared helpers, remove duplicate test block") removed all 11 unit tests for extractNextjsMiddlewareConfig and middlewareMatcherMatchesRoute from route-tool-detection.test.ts. The commit message only mentioned removing "a duplicate 9-test PHP describe block", but the middleware unit test section was also deleted in the same diff.

What was lost:

  • describe('extractNextjsMiddlewareConfig', ...) — 9 tests covering matcher array, single string, default export, chain composition, nested wrappers, regex matcher, match-all behavior, export default function middleware()
  • describe('middlewareMatcherMatchesRoute', ...) — 4 tests covering prefix, exact, regex negative lookahead, non-matching

The import was also downgraded back to only extractMiddlewareChain:

// Before b2b8875:
import { extractMiddlewareChain, extractNextjsMiddlewareConfig, middlewareMatcherMatchesRoute } from '...'

// After b2b8875 (current state):
import { extractMiddlewareChain } from '...'

The coverage drop (−0.4% on all metrics) is entirely explained by this gap: new code with no unit tests.

Fix this →


What IS Production-Ready ✅

Item Status
Bug #1: export default function middleware() capturing "function" as name ✅ Fixed in ce6cce1
Bug #2: No config.matcher → should match-all ✅ Fixed in ce6cce1
Arrow function const export (export const middleware = ...) ✅ Fixed in c32cc42
Regex pre-compilation (no per-route new RegExp()) ✅ Fixed in f08a803
.tsx / .jsx middleware file support ✅ Added in c32cc42
Merge conflicts resolved ✅ Clean in 18b9cb7
TypeScript build ✅ Passes
CI tests ✅ 4199/4200 passing
Integration tests (3 new) ✅ Cover matcher → route linking
Backward compatibility ✅ Unchanged existing behavior

Minor Non-Blocking Issues

1. matcherArrayRe fails on matchers containing ]

const matcherArrayRe = /config\s*=\s*\{[^}]*matcher\s*:\s*\[([^\]]*)\]/s;
//                                                                ^^^^ stops at first `]`

A matcher like ['[0-9]+/path'] would be truncated. Rare but possible with character-class regexes.

2. No deduplication if both middleware.ts and src/middleware.ts exist

Both files would be processed and both would prepend to the same route's middleware array. The code has no guard for this. Low risk (invalid Next.js app) but worth a comment.

3. Path equality assumption

p === 'middleware.ts' assumes exact bare relative paths. If the filesystem walker normalizes differently (e.g. ./middleware.ts), the file would be silently skipped. An endsWith or path.basename check would be safer.


Summary

Restore the accidentally-deleted unit tests, and this PR is ready to merge. The core implementation is correct, the original bugs are fixed, the performance is good, and integration tests validate the end-to-end behavior. The three minor issues above are not blockers for v1.

@magyargergo

Copy link
Copy Markdown
Collaborator

@marxo126 can you please have a look at the removed unit test? why were they removed?

@marxo126

Copy link
Copy Markdown
Contributor Author

@magyargergo Investigated both questions:

1. Unit tests removed in b2b8875

The commit message says "remove duplicate test block" and was intended to remove a duplicated PHP describe block. However, it also accidentally removed all 11 middleware unit tests from route-tool-detection.test.ts:

  • describe('extractNextjsMiddlewareConfig', ...) — 9 tests (matcher array, single string, default export, chain composition, nested wrappers, regex matcher, match-all, export default function middleware())
  • describe('middlewareMatcherMatchesRoute', ...) — 4 tests (prefix, exact, regex negative lookahead, non-matching)

The import was also downgraded back to only extractMiddlewareChain, losing the imports for extractNextjsMiddlewareConfig and middlewareMatcherMatchesRoute (now compileMatcher / compiledMatcherMatchesRoute).

This is the cause of the -0.4% coverage drop across all metrics shown in the CI report. These tests need to be restored before merge.

2. Windows CI failure

The Windows failure is unrelated to this PR. The failing test is setup-skills.test.ts (skills integration), not any middleware or route test:

ENOTEMPTY: directory not empty, rmdir 'D:\a\GitNexus\GitNexus\gitnexus\skills\test-dir-skill-1774456264934-6896\references'

This is a known Windows filesystem race condition — rmdir fails when files haven't been fully released yet. All 4000 actual tests passed. The middleware integration tests (8 tests in route-mapping.test.ts) passed on all three platforms.

This is a pre-existing flaky test, not something introduced by this PR.

Summary

  • Unit tests: accidentally removed, need to be restored — this is a blocker
  • Windows CI: pre-existing flaky teardown in setup-skills.test.ts, not related to this PR — not a blocker

@magyargergo

Copy link
Copy Markdown
Collaborator

Can you please make sure the CI is green?

Commit b2b8875 removed a duplicate PHP test block but also deleted
11 middleware unit tests (extractNextjsMiddlewareConfig + middlewareMatcherMatchesRoute).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@marxo126

Copy link
Copy Markdown
Contributor Author

Tests restored

Commit b2b8875 ("refactor: extract shared helpers, remove duplicate test block") accidentally deleted 13 middleware unit tests while removing a duplicate PHP test block. This has been fixed in 5cb5b89:

  • Restored imports: extractNextjsMiddlewareConfig and middlewareMatcherMatchesRoute re-added to the import line
  • Restored 9 tests in describe('extractNextjsMiddlewareConfig') — matcher extraction, default exports, chain composition, arrow functions, regex matchers, match-all behavior
  • Restored 4 tests in describe('middlewareMatcherMatchesRoute') — prefix matching, exact matching, negative lookahead patterns

All 84 unit tests in route-tool-detection.test.ts pass.


Re: Windows CI failure — The setup-skills.test.ts failure is an unrelated flaky test caused by a Windows filesystem race condition (ENOTEMPTY during fs.rm in test teardown). This is a pre-existing issue not introduced by this PR.

@magyargergo

Copy link
Copy Markdown
Collaborator

@claude Let's run a final review if this PR is production ready and ready to be merged

@github-actions

github-actions Bot commented Mar 25, 2026

Copy link
Copy Markdown
Contributor

Claude finished @magyargergo's task in 1m 32s —— View job


Final Production Readiness Review

  • Gather context from PR history and previous reviews
  • Read all changed files
  • Verify unit tests restored (previous blocker)
  • Check CI status and coverage
  • Review implementation for remaining issues
  • Final verdict

Verdict: ✅ Ready to Merge

The previous blocker (accidentally removed unit tests) has been resolved in 5cb5b89. All originally identified bugs are fixed. The implementation is correct, performant, and well-tested.


Blocker Resolution Confirmed

Unit tests restored (5cb5b89) — the import in route-tool-detection.test.ts:7 correctly includes extractNextjsMiddlewareConfig and middlewareMatcherMatchesRoute. All 13 tests are present:

  • 9 tests in describe('extractNextjsMiddlewareConfig') covering matcher array, single string, default export, chain composition, nested wrappers, regex matcher, match-all, arrow function const, and export default function middleware()
  • 4 tests in describe('middlewareMatcherMatchesRoute') covering prefix, exact, negative lookahead, and non-matching cases

All Previous Bugs Confirmed Fixed

Bug Fix Status
export default function middleware() capturing "function" as name Replaced single regex with 3-pattern approach (isNamedMw, defaultFunctionMatch, defaultIdentifierMatch) ✅ Fixed
No config.matcher → should be match-all (was returning undefined) hasExport guard + compiled.length === 0 branch in pipeline ✅ Fixed
Arrow function export const middleware = ... not detected isConstMw regex added ✅ Fixed
Per-route new RegExp() creation compileMatcher() / compiledMatcherMatchesRoute() — compiled once per file ✅ Fixed
.tsx / .jsx middleware not detected Added to middlewareCandidates filter ✅ Fixed

Implementation Quality

Pipeline (pipeline.ts:1188-1222) — Phase 3.5b logic is correct:

  • Pre-compiles matchers once per file before the route loop
  • compiled.length === 0 correctly handles match-all case
  • Deduplicates when prepending: [...mwLabel, ...currentMw.filter(m => !mwLabel.includes(m))]
  • isDev-gated logging

Extraction (middleware.ts) — Clean, well-structured:

  • Exported compileMatcher / compiledMatcherMatchesRoute are reusable
  • COMPOSER_NAMES set correctly prevents "chain", "compose", "middleware" from being added as named wrappers
  • hasExport guard at line 127 is the right condition for early-return

Integration fixturemiddleware.ts fixture with config.matcher: ['/api/:path*'] cleanly covers the happy path. 3 integration tests validate linking, coverage, and negative case.


Remaining Minor Issues (Non-Blocking for v1)

1. matcherArrayRe won't parse object-style matchers (Next.js 13+ advanced syntax)

const matcherArrayRe = /config\s*=\s*\{[^}]*matcher\s*:\s*\[([^\]]*)\]/s;

A matcher like [{ source: '/api/:path*', has: [...] }] will fail to extract because [^\]]* terminates at the first ] inside the object. This is the most common advanced use-case gap. The current code won't crash — it just silently produces an empty matchers list (match-all fallback), which is safe but inaccurate.

2. Path equality assumption

p === 'middleware.ts'  // fails if walker emits './middleware.ts'

Low risk depending on walker implementation, but p.endsWith('/middleware.ts') || p === 'middleware.ts' would be safer.

3. No dedup guard for both middleware.ts and src/middleware.ts existing simultaneously

Both would be processed and both would prepend to the same routes. Invalid Next.js app configuration, so very low real-world risk.

These are all acceptable v1 trade-offs and can be addressed in follow-up issues.


CI Summary

Check Status
TypeScript build ✅ Passes
Tests (4199/4200) ✅ Passes (1 skip unrelated — Ruby block param inference)
Windows flaky test (setup-skills.test.ts ENOTEMPTY) ⚠️ Pre-existing, unrelated to this PR
Coverage delta 📉 −0.4% — acceptable given new code; unit tests now restored

This PR is production ready.

@magyargergo magyargergo merged commit f860653 into abhigyanpatwari:main Mar 25, 2026
9 of 10 checks passed
@marxo126 marxo126 deleted the feat/nextjs-middleware-linking branch March 26, 2026 06:45
@marxo126

Copy link
Copy Markdown
Contributor Author

@magyargergo The unit tests were accidentally deleted in commit b2b8875 — that commit was meant to remove a duplicate PHP test block but also caught 11 middleware unit tests (extractNextjsMiddlewareConfig + middlewareMatcherMatchesRoute). They were restored in commit 5cb5b89.

Merge conflicts with upstream/main are now resolved and pushed. CI should be green.

motolese pushed a commit to motolese/datamoto-gitnexus that referenced this pull request Apr 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants