fix: prevent double execution when not found (#339)

yusukebe · web-flow · commit 302b8bc50b02 · 2025-10-31T20:45:06.000+09:00
* fix: prevent double execution when not found

* format
diff --git a/mocks/app/routes/_404.tsx b/mocks/app/routes/_404.tsx
@@ -1,6 +1,7 @@
 import type { NotFoundHandler } from 'hono'
 
 const handler: NotFoundHandler = (c) => {
+  c.res.headers.append('HeaderFrom404', 'Hi')
   return c.render(<h1>Not Found</h1>, {
     title: 'Not Found',
   })
diff --git a/mocks/app/routes/_renderer.tsx b/mocks/app/routes/_renderer.tsx
@@ -1,21 +1,28 @@
+import { createMiddleware } from 'hono/factory'
 import { jsxRenderer } from 'hono/jsx-renderer'
 import { HasIslands } from '../../../src/server'
 
-export default jsxRenderer(
-  ({ children, title }) => {
-    return (
-      <html>
-        <head>
-          <title>{title}</title>
-        </head>
-        <body>
-          {children}
-          <HasIslands>
-            <script type='module' async src='/app/client.ts'></script>
-          </HasIslands>
-        </body>
-      </html>
-    )
-  },
-  { stream: true }
-)
+const middleware = createMiddleware(async (c, next) => {
+  c.res.headers.append('HeaderFromRenderer', 'Hi')
+  const renderer = jsxRenderer(
+    ({ children, title }) => {
+      return (
+        <html>
+          <head>
+            <title>{title}</title>
+          </head>
+          <body>
+            {children}
+            <HasIslands>
+              <script type='module' async src='/app/client.ts'></script>
+            </HasIslands>
+          </body>
+        </html>
+      )
+    },
+    { stream: true }
+  )
+  await renderer(c, next)
+})
+
+export default middleware
diff --git a/src/server/server.ts b/src/server/server.ts
@@ -67,6 +67,12 @@ export const createApp = <E extends Env>(options: BaseServerOptions<E>): Hono<E>
   // Key: directory path, Value: Set of middleware file paths applied to that directory
   const appliedMiddlewaresByDirectory = new Map<string, Set<string>>()
 
+  // Track responses that have already been processed by 404 handler to prevent double execution
+  const processedNotFoundResponses = new WeakSet<Response>()
+
+  // Track requests that have been processed by each renderer to prevent double execution
+  const processedRendererRequests = new WeakMap<MiddlewareHandler, WeakSet<Request>>()
+
   // Share context by AsyncLocalStorage
   app.use(async function ShareContext(c, next) {
     await contextStorage.run(c, () => next())
@@ -128,13 +134,14 @@ export const createApp = <E extends Env>(options: BaseServerOptions<E>): Hono<E>
       if (notFoundHandler) {
         subApp.use(async (c, next) => {
           await next()
-          if (c.res.status === 404) {
+          if (c.res.status === 404 && !processedNotFoundResponses.has(c.res)) {
             const notFoundResponse = await notFoundHandler(c)
             const res = new Response(notFoundResponse.body, {
               status: 404,
               headers: notFoundResponse.headers,
             })
             c.res = res
+            processedNotFoundResponses.add(c.res)
           }
         })
       }
@@ -177,19 +184,34 @@ export const createApp = <E extends Env>(options: BaseServerOptions<E>): Hono<E>
         }
         const rendererDefault = renderer.default
         if (rendererDefault) {
-          subApp.use('*', rendererDefault)
+          // Get or create WeakSet for this renderer
+          if (!processedRendererRequests.has(rendererDefault)) {
+            processedRendererRequests.set(rendererDefault, new WeakSet<Request>())
+          }
+          const processedRequests = processedRendererRequests.get(rendererDefault)!
+
+          // Wrap renderer to check if already processed
+          const wrappedRenderer = createMiddleware(async (c, next) => {
+            if (!processedRequests.has(c.req.raw)) {
+              processedRequests.add(c.req.raw)
+              return rendererDefault(c, next)
+            }
+            return next()
+          })
+
+          subApp.use('*', wrappedRenderer)
 
           // Apply extra middleware for parent routing patterns like /:lang{en} or /:lang?
           const rootPath = dir.replace(rootRegExp, '')
           const isRootLevel = !rootPath.includes('/')
           const isSimpleStructure = !Object.keys(content).some((f) => f.includes('/'))
 
           if (Object.keys(content).length > 0 && isRootLevel && isSimpleStructure) {
-            subApp.use('/', rendererDefault)
+            subApp.use('/', wrappedRenderer)
             Object.keys(content).forEach((filename) => {
               const path = filePathToPath(filename)
               if (path !== '/' && !path.includes('[') && !path.includes('*')) {
-                subApp.use(path, rendererDefault)
+                subApp.use(path, wrappedRenderer)
               }
             })
           }
@@ -320,7 +342,7 @@ export const createApp = <E extends Env>(options: BaseServerOptions<E>): Hono<E>
     const subApp = new Hono<{
       Variables: Variables
     }>()
-    applyNotFound(subApp, dir, notFoundMap)
+    applyNotFound(subApp, dir, notFoundMap, processedNotFoundResponses)
     const rootPath = getRootPath(dir)
     app.route(rootPath, subApp)
   }
@@ -344,7 +366,8 @@ function applyNotFound(
     Variables: Variables
   }>,
   dir: string,
-  map: Record<string, Record<string, NotFoundFile>>
+  map: Record<string, Record<string, NotFoundFile>>,
+  processedNotFoundResponses: WeakSet<Response>
 ) {
   for (const [mapDir, content] of Object.entries(map)) {
     if (dir === mapDir) {
@@ -358,9 +381,13 @@ function applyNotFound(
             return next()
           })
         }
-        app.get('*', (c) => {
-          c.status(404)
-          return notFoundHandler(c)
+        app.get('*', async (c, next) => {
+          await next()
+          if (processedNotFoundResponses.has(c.res)) {
+            c.status(404)
+            processedNotFoundResponses.add(c.res)
+            c.res = await notFoundHandler(c)
+          }
         })
       }
     }
diff --git a/test-integration/apps.test.ts b/test-integration/apps.test.ts
@@ -10,11 +10,15 @@ describe('Basic', () => {
   const NOT_FOUND = import.meta.glob('../mocks/app/routes/**/_404.(ts|tsx)', {
     eager: true,
   })
+  const RENDERER = import.meta.glob('../mocks/app/routes/**/_renderer.(ts|tsx)', {
+    eager: true,
+  })
 
   const app = createApp({
     root: '../mocks/app/routes',
     ROUTES: ROUTES as any,
     NOT_FOUND: NOT_FOUND as any,
+    RENDERER: RENDERER as any,
     init: (app) => {
       app.use('*', poweredBy())
     },
@@ -106,7 +110,7 @@ describe('Basic', () => {
         method: 'GET',
       },
     ]
-    expect(app.routes).toHaveLength(52)
+    expect(app.routes).toHaveLength(69)
     expect(app.routes).toEqual(
       expect.arrayContaining(
         routes.map(({ path, method }) => {
@@ -123,26 +127,30 @@ describe('Basic', () => {
   it('Should return 200 response - / with a Powered By header', async () => {
     const res = await app.request('/')
     expect(res.status).toBe(200)
-    expect(await res.text()).toBe('<h1>Hello</h1>')
+    expect(await res.text()).toContain('<h1>Hello</h1>')
     expect(res.headers.get('x-powered-by'), 'Hono')
   })
 
   it('Should return 404 response - /foo', async () => {
     const res = await app.request('/foo')
+    expect(res.headers.get('HeaderFrom404')).toBe('Hi')
+    expect(res.headers.get('HeaderFromRenderer')).toBe('Hi')
     expect(res.status).toBe(404)
   })
 
   it('Should return custom 404 response - /not-found', async () => {
     const res = await app.request('/not-found')
     expect(res.status).toBe(404)
-    expect(await res.text()).toBe('<h1>Not Found</h1>')
+    expect(res.headers.get('HeaderFrom404')).toBe('Hi')
+    expect(res.headers.get('HeaderFromRenderer')).toBe('Hi')
+    expect(await res.text()).toContain('<h1>Not Found</h1>')
   })
 
   it('Should return 200 response - /about/me', async () => {
     const res = await app.request('/about/me')
     expect(res.status).toBe(200)
     // hono/jsx escape a single quote to &#39;
-    expect(await res.text()).toBe('<p>It&#39;s me</p><b>My name is me</b>')
+    expect(await res.text()).toContain('<p>It&#39;s me</p><b>My name is me</b>')
   })
 
   it('Should return 200 response - POST /about/me', async () => {
@@ -155,19 +163,19 @@ describe('Basic', () => {
   it('Should return 200 response - GET /fc', async () => {
     const res = await app.request('/fc')
     expect(res.status).toBe(200)
-    expect(await res.text()).toBe('<h1>Function from /fc</h1>')
+    expect(await res.text()).toContain('<h1>Function from /fc</h1>')
   })
 
   it('Should not determined as an island component - GET /non-interactive', async () => {
     const res = await app.request('/non-interactive')
     expect(res.status).toBe(200)
-    expect(await res.text()).toBe('<p>Not Island</p>')
+    expect(await res.text()).toContain('<p>Not Island</p>')
   })
 
   it('Should render MDX content - /post', async () => {
     const res = await app.request('/post')
     expect(res.status).toBe(200)
-    expect(await res.text()).toBe('<h1>Hello MDX</h1>')
+    expect(await res.text()).toContain('<h1>Hello MDX</h1>')
   })
 
   it('Should return 500 response - /throw_error', async () => {
