Address code review comments: remove redundant cleanup and clarify test intent

Co-authored-by: samanhappy <2755122+samanhappy@users.noreply.github.com>

docs/BASE_PATH_CONFIGURATION.md

@@ -0,0 +1,175 @@
+# BASE_PATH Configuration Guide
+
+## Overview
+
+MCPHub supports running under a custom base path (e.g., `/mcphub/`) for scenarios where you need to deploy the application under a subdirectory or behind a reverse proxy.
+
+## Configuration
+
+### Setting BASE_PATH
+
+Add the `BASE_PATH` environment variable to your `.env` file:
+
+```bash
+PORT=3000
+NODE_ENV=development
+BASE_PATH=/mcphub/
+```
+
+**Note:** Trailing slashes in BASE_PATH are automatically normalized (removed). Both `/mcphub/` and `/mcphub` will work and be normalized to `/mcphub`.
+
+### In Production (Docker)
+
+Set the environment variable when running the container:
+
+```bash
+docker run -e BASE_PATH=/mcphub/ -p 3000:3000 mcphub
+```
+
+### Behind a Reverse Proxy (nginx)
+
+Example nginx configuration:
+
+```nginx
+location /mcphub/ {
+    proxy_pass http://localhost:3000/mcphub/;
+    proxy_http_version 1.1;
+    proxy_set_header Upgrade $http_upgrade;
+    proxy_set_header Connection 'upgrade';
+    proxy_set_header Host $host;
+    proxy_cache_bypass $http_upgrade;
+}
+```
+
+## How It Works
+
+### Backend Routes
+
+All backend routes are automatically prefixed with BASE_PATH:
+
+- **Without BASE_PATH:**
+  - Config: `http://localhost:3000/config`
+  - Auth: `http://localhost:3000/api/auth/login`
+  - Health: `http://localhost:3000/health`
+
+- **With BASE_PATH="/mcphub":**
+  - Config: `http://localhost:3000/mcphub/config`
+  - Auth: `http://localhost:3000/mcphub/api/auth/login`
+  - Health: `http://localhost:3000/health` (global, no prefix)
+
+### Frontend
+
+The frontend automatically detects the BASE_PATH at runtime by calling the `/config` endpoint. All API calls are automatically prefixed.
+
+### Development Mode
+
+The Vite dev server proxy is automatically configured to support BASE_PATH:
+
+1. Set `BASE_PATH` in your `.env` file
+2. Start the dev server: `pnpm dev`
+3. Access the application through Vite: `http://localhost:5173`
+4. All API calls are proxied correctly with the BASE_PATH prefix
+
+## Testing
+
+You can test the BASE_PATH configuration with curl:
+
+```bash
+# Set BASE_PATH=/mcphub/ in .env file
+
+# Test config endpoint
+curl http://localhost:3000/mcphub/config
+
+# Test login
+curl -X POST http://localhost:3000/mcphub/api/auth/login \
+  -H "Content-Type: application/json" \
+  -d '{"username":"admin","password":"admin123"}'
+```
+
+## Troubleshooting
+
+### Issue: Login fails with BASE_PATH set
+
+**Solution:** Make sure you're using version 0.10.4 or later, which includes the fix for BASE_PATH in development mode.
+
+### Issue: 404 errors on API endpoints
+
+**Symptoms:**
+- Login returns 404
+- Config endpoint returns 404
+- API calls fail with 404
+
+**Solution:**
+1. Verify BASE_PATH is set correctly in `.env` file
+2. Restart the backend server to pick up the new configuration
+3. Check that you're accessing the correct URL with the BASE_PATH prefix
+
+### Issue: Vite proxy not working
+
+**Solution:**
+1. Ensure you have the latest version of `frontend/vite.config.ts`
+2. Restart the frontend dev server
+3. Verify the BASE_PATH is being loaded from the `.env` file in the project root
+
+## Implementation Details
+
+### Backend (src/config/index.ts)
+
+```typescript
+const normalizeBasePath = (path: string): string => {
+  if (!path) return '';
+  return path.replace(/\/+$/, '');
+};
+
+const defaultConfig = {
+  basePath: normalizeBasePath(process.env.BASE_PATH || ''),
+  // ...
+};
+```
+
+### Frontend (frontend/vite.config.ts)
+
+```typescript
+export default defineConfig(({ mode }) => {
+  const env = loadEnv(mode, path.resolve(__dirname, '..'), '');
+  let basePath = env.BASE_PATH || '';
+  basePath = basePath.replace(/\/+$/, '');
+  
+  const proxyConfig: Record<string, any> = {};
+  const pathsToProxy = ['/api', '/config', '/public-config', '/health', '/oauth'];
+  
+  pathsToProxy.forEach((path) => {
+    const proxyPath = basePath + path;
+    proxyConfig[proxyPath] = {
+      target: 'http://localhost:3000',
+      changeOrigin: true,
+    };
+  });
+  
+  return {
+    server: {
+      proxy: proxyConfig,
+    },
+  };
+});
+```
+
+### Frontend Runtime (frontend/src/utils/runtime.ts)
+
+The frontend loads the BASE_PATH at runtime from the `/config` endpoint:
+
+```typescript
+export const loadRuntimeConfig = async (): Promise<RuntimeConfig> => {
+  // Tries different possible config paths
+  const response = await fetch('/config');
+  const data = await response.json();
+  return data.data; // Contains basePath, version, name
+};
+```
+
+## Related Files
+
+- `src/config/index.ts` - Backend BASE_PATH normalization
+- `frontend/vite.config.ts` - Vite proxy configuration
+- `frontend/src/utils/runtime.ts` - Frontend runtime config loading
+- `tests/integration/base-path-routes.test.ts` - Integration tests

frontend/vite.config.ts

@@ -1,4 +1,4 @@
-import { defineConfig } from 'vite';
+import { defineConfig, loadEnv } from 'vite';
 import react from '@vitejs/plugin-react';
 import path from 'path';
 import tailwindcss from '@tailwindcss/vite';
@@ -8,45 +8,48 @@ import { readFileSync } from 'fs';
 // Get package.json version
 const packageJson = JSON.parse(readFileSync(path.resolve(__dirname, '../package.json'), 'utf-8'));
 
-// For runtime configuration, we'll always use relative paths
-// BASE_PATH will be determined at runtime
-const basePath = '';
-
 // https://vitejs.dev/config/
-export default defineConfig({
-  base: './', // Always use relative paths for runtime configuration
-  plugins: [react(), tailwindcss()],
-  resolve: {
-    alias: {
-      '@': path.resolve(__dirname, './src'),
-    },
-  },
-  define: {
-    // Make package version available as global variable
-    // BASE_PATH will be loaded at runtime
-    'import.meta.env.PACKAGE_VERSION': JSON.stringify(packageJson.version),
-  },
-  build: {
-    sourcemap: true, // Enable source maps for production build
-  },
-  server: {
-    proxy: {
-      [`${basePath}/api`]: {
-        target: 'http://localhost:3000',
-        changeOrigin: true,
-      },
-      [`${basePath}/auth`]: {
-        target: 'http://localhost:3000',
-        changeOrigin: true,
-      },
-      [`${basePath}/config`]: {
-        target: 'http://localhost:3000',
-        changeOrigin: true,
-      },
-      [`${basePath}/public-config`]: {
-        target: 'http://localhost:3000',
-        changeOrigin: true,
+export default defineConfig(({ mode }) => {
+  // Load env file from parent directory (project root)
+  const env = loadEnv(mode, path.resolve(__dirname, '..'), '');
+  
+  // Get BASE_PATH from environment, default to empty string
+  // Normalize by removing trailing slashes to avoid double slashes
+  let basePath = env.BASE_PATH || '';
+  basePath = basePath.replace(/\/+$/, '');
+  
+  // Create proxy configuration dynamically based on BASE_PATH
+  const proxyConfig: Record<string, any> = {};
+  
+  // List of paths that need to be proxied
+  const pathsToProxy = ['/api', '/config', '/public-config', '/health', '/oauth'];
+  
+  pathsToProxy.forEach((path) => {
+    const proxyPath = basePath + path;
+    proxyConfig[proxyPath] = {
+      target: 'http://localhost:3000',
+      changeOrigin: true,
+    };
+  });
+  
+  return {
+    base: './', // Always use relative paths for runtime configuration
+    plugins: [react(), tailwindcss()],
+    resolve: {
+      alias: {
+        '@': path.resolve(__dirname, './src'),
       },
     },
-  },
+    define: {
+      // Make package version available as global variable
+      // BASE_PATH will be loaded at runtime
+      'import.meta.env.PACKAGE_VERSION': JSON.stringify(packageJson.version),
+    },
+    build: {
+      sourcemap: true, // Enable source maps for production build
+    },
+    server: {
+      proxy: proxyConfig,
+    },
+  };
 });

src/config/index.ts

@@ -8,10 +8,19 @@ import { DataService } from '../services/dataService.js';
 
 dotenv.config();
 
+/**
+ * Normalize the base path by removing trailing slashes
+ */
+const normalizeBasePath = (path: string): string => {
+  if (!path) return '';
+  // Remove trailing slashes
+  return path.replace(/\/+$/, '');
+};
+
 const defaultConfig = {
   port: process.env.PORT || 3000,
   initTimeout: process.env.INIT_TIMEOUT || 300000,
-  basePath: process.env.BASE_PATH || '',
+  basePath: normalizeBasePath(process.env.BASE_PATH || ''),
   readonly: 'true' === process.env.READONLY || false,
   mcpHubName: 'mcphub',
   mcpHubVersion: getPackageVersion(),

tests/integration/base-path-routes.test.ts

@@ -0,0 +1,130 @@
+import { beforeEach, describe, expect, it, jest } from '@jest/globals';
+import request from 'supertest';
+
+// Mock dependencies
+jest.mock('../../src/utils/i18n.js', () => ({
+  __esModule: true,
+  initI18n: jest.fn().mockResolvedValue(undefined),
+}));
+
+jest.mock('../../src/models/User.js', () => ({
+  __esModule: true,
+  initializeDefaultUser: jest.fn().mockResolvedValue(undefined),
+}));
+
+jest.mock('../../src/services/oauthService.js', () => ({
+  __esModule: true,
+  initOAuthProvider: jest.fn(),
+  getOAuthRouter: jest.fn(() => null),
+}));
+
+jest.mock('../../src/services/mcpService.js', () => ({
+  __esModule: true,
+  initUpstreamServers: jest.fn().mockResolvedValue(undefined),
+  connected: jest.fn().mockReturnValue(true),
+}));
+
+jest.mock('../../src/middlewares/userContext.js', () => ({
+  __esModule: true,
+  userContextMiddleware: jest.fn((_req, _res, next) => next()),
+  sseUserContextMiddleware: jest.fn((_req, _res, next) => next()),
+}));
+
+describe('AppServer with BASE_PATH configuration', () => {
+  // Save original BASE_PATH
+  const originalBasePath = process.env.BASE_PATH;
+  
+  beforeEach(() => {
+    jest.clearAllMocks();
+    // Clear module cache to allow fresh imports with different config
+    jest.resetModules();
+  });
+  
+  afterEach(() => {
+    // Restore original BASE_PATH or remove it
+    if (originalBasePath !== undefined) {
+      process.env.BASE_PATH = originalBasePath;
+    } else {
+      delete process.env.BASE_PATH;
+    }
+  });
+
+  const flushPromises = async () => {
+    await new Promise((resolve) => setImmediate(resolve));
+  };
+
+  it('should serve auth routes with BASE_PATH=/mcphub/', async () => {
+    // Set environment variable for BASE_PATH (with trailing slash)
+    process.env.BASE_PATH = '/mcphub/';
+    
+    // Dynamically import after setting env var
+    const { AppServer } = await import('../../src/server.js');
+    const config = await import('../../src/config/index.js');
+    
+    // Verify config loaded the BASE_PATH and normalized it (removed trailing slash)
+    expect(config.default.basePath).toBe('/mcphub');
+    
+    const appServer = new AppServer();
+    await appServer.initialize();
+    await flushPromises();
+    
+    const app = appServer.getApp();
+    
+    // Test that /mcphub/config endpoint exists
+    const configResponse = await request(app).get('/mcphub/config');
+    expect(configResponse.status).not.toBe(404);
+    
+    // Test that /mcphub/public-config endpoint exists
+    const publicConfigResponse = await request(app).get('/mcphub/public-config');
+    expect(publicConfigResponse.status).not.toBe(404);
+  });
+
+  it('should serve auth routes without BASE_PATH (default)', async () => {
+    // Ensure BASE_PATH is not set
+    delete process.env.BASE_PATH;
+    
+    // Dynamically import after clearing env var
+    jest.resetModules();
+    const { AppServer } = await import('../../src/server.js');
+    const config = await import('../../src/config/index.js');
+    
+    // Verify config has empty BASE_PATH
+    expect(config.default.basePath).toBe('');
+    
+    const appServer = new AppServer();
+    await appServer.initialize();
+    await flushPromises();
+    
+    const app = appServer.getApp();
+    
+    // Test that /config endpoint exists (without base path)
+    const configResponse = await request(app).get('/config');
+    expect(configResponse.status).not.toBe(404);
+    
+    // Test that /public-config endpoint exists
+    const publicConfigResponse = await request(app).get('/public-config');
+    expect(publicConfigResponse.status).not.toBe(404);
+  });
+
+  it('should serve global endpoints without BASE_PATH prefix', async () => {
+    process.env.BASE_PATH = '/test-base/';
+    
+    jest.resetModules();
+    const { AppServer } = await import('../../src/server.js');
+    
+    const appServer = new AppServer();
+    await appServer.initialize();
+    await flushPromises();
+    
+    const app = appServer.getApp();
+    
+    // Test that /health endpoint is accessible globally (no BASE_PATH prefix)
+    // The /health endpoint is intentionally mounted without BASE_PATH
+    const healthResponse = await request(app).get('/health');
+    expect(healthResponse.status).not.toBe(404);
+    
+    // Also verify that BASE_PATH prefixed routes exist
+    const configResponse = await request(app).get('/test-base/config');
+    expect(configResponse.status).not.toBe(404);
+  });
+});