Merge pull request #18 from oslabs-beta/jaime/code-review-and-suggestions

[review] jaime's input / insights

Author: eodafoo

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();

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,

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';

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() === '') {

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;

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:

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);

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 = {