From 96cdbec6af0a8ab61939532f2eb7ce3394ddcfce Mon Sep 17 00:00:00 2001 From: Jaime de Venecia Date: Thu, 6 Nov 2025 19:52:40 -0800 Subject: [PATCH 1/2] review(server): add comments to server.js --- server/server.js | 69 +++++++++++++++++++++++++++++++++++------------- 1 file changed, 51 insertions(+), 18 deletions(-) diff --git a/server/server.js b/server/server.js index 29118c8..3d7884d 100644 --- a/server/server.js +++ b/server/server.js @@ -1,3 +1,27 @@ +/* + i'd recommend organizing your imports at the top of files (here and in other files), + perhaps by sections separated with spaces, i.e.: + + // library dependencies + import express from 'express' + import cors from 'cors' + import helmet from 'helmet' + import { z } from 'zod' + ... + + // routes + import mcpRoutes from './routes/mcp.js' + import agentRoutes from './routes/agent.js' + ... + + // helper functions / constants / other data / etc. + import { healthCheck } from './db.js' + import { query } from './db.js' + ... + + all up to you how you want to do this. but i find it helps with readability and organization. +*/ + import 'dotenv/config'; import express from 'express'; import cors from 'cors'; @@ -27,13 +51,17 @@ app.use(morgan('dev')); app.use(cookieParser()); // --- Request Logging Middleware --- -app.use((req, res, next) => { + +// a convention you can choose to follow is prefixing unused parameters with an underscore +app.use((req, _res, next) => { const user = req.headers['x-user-id'] || 'anonymous'; console.log( `[${new Date().toISOString()}] ${req.method} ${ req.originalUrl } | user=${user}` ); + // ^ nice logging; this is great. + next(); }); @@ -50,17 +78,21 @@ app.get('/db/ping', async (_req, res) => { } }); +// Mount users route at /users +// ^ imo, this kind of comment is a bit useless: it's obvious to other devs what it does :) +app.use('/', userRouter); + +// i'd probably put the other routes here as well. + /** Users */ const UserBody = z.object({ email: z.string().email(), github_username: z.string().min(1).optional(), }); -// Mount users route at /users -app.use('/', userRouter); // Create or upsert user by email app.post('/users', async (req, res) => { - const parse = UserBody.safeParse(req.body); + const parse = UserBody.safeParse(req.body); // love that you are doing this. great. if (!parse.success) return res.status(400).json({ error: parse.error.message }); const { email, github_username } = parse.data; @@ -82,6 +114,9 @@ app.post('/users', async (req, res) => { } }); +// you definitely want to minimize commented-out code like below +// if you don't need it, just remove it. + // app.get('/users', async (_req, res) => { // try { // const rows = await query( @@ -125,30 +160,28 @@ app.get('/connections', async (_req, res) => { } }); -// // --- Request Logging Middleware --- -// app.use((req, res, next) => { -// const user = req.headers['x-user-id'] || 'anonymous'; -// console.log( -// `[${new Date().toISOString()}] ${req.method} ${ -// req.originalUrl -// } | user=${user}` -// ); -// next(); -// }); - // -- Agent entry point + +/* +you should keep your router names consistent: + - deploymentsRouter + - agentRouter (not agentRoutes) + - authAwsRouter (not authAws) + - authGoogleRouter (not authGoogle) + etc. +*/ + +// also, i'd probably move these routes closer to the top of the file, so they're easier to find. + app.use('/deployments', deploymentsRouter); app.use('/agent', agentRoutes); app.use('/mcp/v1', pipelineCommitRouter); app.use('/mcp/v1', mcpRoutes); -// Mount GitHub OAuth routes at /auth/github app.use('/auth/github', githubAuthRouter); app.use(authRoutes); -// Mount AWS SSO routes app.use('/auth/aws', authAws); -// Mount Google OAuth routes app.use('/auth/google', authGoogle); app.use('/jenkins', jenkinsRouter); From 4dae7871d53b458541c816bf978756772656acf6 Mon Sep 17 00:00:00 2001 From: Jaime de Venecia Date: Thu, 6 Nov 2025 20:06:42 -0800 Subject: [PATCH 2/2] chore(review): add comments/insights --- server/db.js | 12 +++++++++--- server/lib/github-oauth.js | 2 ++ server/routes/agent.js | 4 ++++ server/routes/auth.aws.js | 7 ++++++- server/routes/auth.github.js | 2 ++ server/routes/pipelineCommit.js | 2 ++ server/src/config/env.js | 1 + 7 files changed, 26 insertions(+), 4 deletions(-) diff --git a/server/db.js b/server/db.js index 8e6e322..44d1845 100644 --- a/server/db.js +++ b/server/db.js @@ -1,9 +1,13 @@ +// definitely periodically run Prettier or some other formatter on your codebase! + import 'dotenv/config'; import pkg from 'pg'; const { Pool } = pkg; - -console.log("๐Ÿ” DB SSL rejectUnauthorized:", process.env.DB_SSL_REJECT_UNAUTHORIZED); +console.log( + '๐Ÿ” DB SSL rejectUnauthorized:', + process.env.DB_SSL_REJECT_UNAUTHORIZED +); export const pool = new Pool({ connectionString: process.env.DATABASE_URL, max: parseInt(process.env.DB_POOL_MAX || '8', 10), @@ -17,7 +21,9 @@ export const pool = new Pool({ }, }); -pool.on('error', (err) => console.error('[DB] Unexpected error on idle client', err)); +pool.on('error', (err) => + console.error('[DB] Unexpected error on idle client', err) +); export async function query(sql, params = []) { const start = Date.now(); diff --git a/server/lib/github-oauth.js b/server/lib/github-oauth.js index afc8b30..9c97eb9 100644 --- a/server/lib/github-oauth.js +++ b/server/lib/github-oauth.js @@ -1,3 +1,5 @@ +// this file is great, and organized. + export function buildAuthorizeUrl({ clientId, redirectUri, scopes, state }) { const params = new URLSearchParams({ client_id: clientId, diff --git a/server/routes/agent.js b/server/routes/agent.js index 1ae34da..ac39901 100644 --- a/server/routes/agent.js +++ b/server/routes/agent.js @@ -1,3 +1,7 @@ +/* just commenting on this file to say: +in your /routes folder, make sure to keep your router names consistent! +*/ + import express from 'express'; import { runWizardAgent } from '../agent/wizardAgent.js'; import { pipeline_generator } from '../tools/pipeline_generator.js'; diff --git a/server/routes/auth.aws.js b/server/routes/auth.aws.js index 4f566ef..76aa2de 100644 --- a/server/routes/auth.aws.js +++ b/server/routes/auth.aws.js @@ -13,12 +13,17 @@ import { } from "@aws-sdk/client-sso-oidc"; const router = Router(); + +// i'd set up some linting to ensure that you're catching unused variables, like the one below +// this will be important in terms of keeping your codebase organized & professional down the line const SESSION_SECRET = process.env.SESSION_SECRET; // โœ… Start AWS connect flow router.post("/connect", requireSession, async (req, res) => { const { sso_start_url, sso_region, account_id, role_to_assume } = req.body; - const userId = req.user.id; + + // if you want to get fancy with destructuring, i'd do this personally: + const { user: { id: userId } } = req; // Validate required parameters if (!sso_start_url || typeof sso_start_url !== 'string' || sso_start_url.trim() === '') { diff --git a/server/routes/auth.github.js b/server/routes/auth.github.js index 55ce52a..3281dc6 100644 --- a/server/routes/auth.github.js +++ b/server/routes/auth.github.js @@ -41,6 +41,8 @@ router.get('/start', (req, res) => { return res.redirect(url); }); +// remove commented-out code like this if you're not gonna use it + // router.get('/callback', async (req, res) => { // try { // const { code, state } = req.query; diff --git a/server/routes/pipelineCommit.js b/server/routes/pipelineCommit.js index 3310982..d270989 100644 --- a/server/routes/pipelineCommit.js +++ b/server/routes/pipelineCommit.js @@ -5,6 +5,8 @@ import { upsertWorkflowFile } from '../tools/github_adapter.js'; const router = Router(); +// this is nice. consider using JSDoc at some point in the future to document your routes/functions/etc. +// it really elevates a codebase's professionalism /** * POST /mcp/v1/pipeline_commit * Body: diff --git a/server/src/config/env.js b/server/src/config/env.js index 523a941..9d2adbd 100644 --- a/server/src/config/env.js +++ b/server/src/config/env.js @@ -1,6 +1,7 @@ import dotenv from "dotenv"; dotenv.config(); +// definitely be sure to remove these kinds of logs in production! console.log("๐Ÿงพ MCP_API_KEY from .env:", process.env.MCP_API_KEY); export const config = {