Add production hardening scaffolds, docs, and CI gates#9
Conversation
…mobile, rollout docs and CI
Summary of ChangesHello @cjo93, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's readiness for production by integrating robust hardening measures. It covers critical areas such as security, performance, monitoring, user experience, and deployment processes, ensuring a more stable, secure, and maintainable system. The changes also lay the groundwork for future mobile and progressive web application development. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant step towards production readiness, introducing hardening across security, observability, and CI. The addition of documentation, CI gates, middleware for security, and stubs for background jobs is excellent. My review focuses on a few areas for improvement: CI efficiency, frontend robustness, backend correctness and typing, and logging consistency. Overall, these are great changes that lay a solid foundation for a production environment. The suggested changes will help improve the robustness and maintainability of the new additions.
| const mandala = await fetch(`${base}/api/mandala/preview`, { method: 'GET' }); | ||
| console.log('mandala preview', mandala.status); | ||
|
|
||
| // Voice synth check | ||
| const voice = await fetch(`${base}/api/voice/preview`, { method: 'GET' }); | ||
| console.log('voice preview', voice.status); | ||
|
|
||
| // If stripe test allowed, call checkout creation (requires test key) | ||
| if (process.env.STRIPE_SECRET_KEY) { | ||
| const checkout = await fetch(`${base}/api/payments/test-checkout`, { method: 'POST', headers: { 'content-type':'application/json' }, body: JSON.stringify({ test: true })}); | ||
| console.log('checkout', checkout.status); |
There was a problem hiding this comment.
The smoke test currently logs the status of the API endpoints but doesn't treat non-2xx responses as failures (except for /health). This could lead to a 'successful' smoke test run even if critical endpoints are down. You should check the response.ok property for each request and throw an error if it's not successful, causing the script to exit with a failure code.
| const mandala = await fetch(`${base}/api/mandala/preview`, { method: 'GET' }); | |
| console.log('mandala preview', mandala.status); | |
| // Voice synth check | |
| const voice = await fetch(`${base}/api/voice/preview`, { method: 'GET' }); | |
| console.log('voice preview', voice.status); | |
| // If stripe test allowed, call checkout creation (requires test key) | |
| if (process.env.STRIPE_SECRET_KEY) { | |
| const checkout = await fetch(`${base}/api/payments/test-checkout`, { method: 'POST', headers: { 'content-type':'application/json' }, body: JSON.stringify({ test: true })}); | |
| console.log('checkout', checkout.status); | |
| const mandala = await fetch(`${base}/api/mandala/preview`, { method: 'GET' }); | |
| console.log('mandala preview', mandala.status); | |
| if (!mandala.ok) throw new Error(`Mandala preview failed with status: ${mandala.status}`); | |
| // Voice synth check | |
| const voice = await fetch(`${base}/api/voice/preview`, { method: 'GET' }); | |
| console.log('voice preview', voice.status); | |
| if (!voice.ok) throw new Error(`Voice preview failed with status: ${voice.status}`); | |
| // If stripe test allowed, call checkout creation (requires test key) | |
| if (process.env.STRIPE_SECRET_KEY) { | |
| const checkout = await fetch(`${base}/api/payments/test-checkout`, { method: 'POST', headers: { 'content-type':'application/json' }, body: JSON.stringify({ test: true })}); | |
| console.log('checkout', checkout.status); | |
| if (!checkout.ok) throw new Error(`Test checkout failed with status: ${checkout.status}`); | |
| } |
| # ------------------------------- | ||
| unit_test: | ||
| stage: test | ||
| image: node:18 |
There was a problem hiding this comment.
To improve CI performance and reduce build times, consider adding caching for node_modules. This will prevent npm ci from re-downloading and re-installing all dependencies on every run.
This suggestion also applies to the integration_test and e2e_test jobs (with appropriate path adjustments for e2e_test, which would be frontend/node_modules/ and frontend/package-lock.json).
image: node:18
cache:
key:
files:
- package-lock.json
paths:
- node_modules/| self.addEventListener('fetch', (e) => { | ||
| // Basic network-first for API, cache-first for static | ||
| }); |
There was a problem hiding this comment.
This fetch event listener is currently a stub. An empty fetch handler can add overhead and potentially slow down requests, as they will still pass through the service worker even though no caching logic is performed. If you are not yet implementing caching logic, it's better to remove this listener until it's ready to be implemented to avoid any performance impact.
| useEffect(() => { | ||
| const prefersLowPower = window.matchMedia && window.matchMedia('(prefers-reduced-motion: reduce)').matches; | ||
| const fpsLow = false; // extend: detect device GPU | ||
| setLite(prefersLowPower || fpsLow); | ||
| }, []); |
There was a problem hiding this comment.
The useEffect hook runs only once on mount, so it won't react to changes in the user's prefers-reduced-motion setting if they change it while the app is running. To make this hook more robust, you should add an event listener to the media query and clean it up on unmount.
useEffect(() => {
if (!window.matchMedia) {
return;
}
const mediaQuery = window.matchMedia('(prefers-reduced-motion: reduce)');
const fpsLow = false; // extend: detect device GPU
const updateLiteMode = () => {
setLite(mediaQuery.matches || fpsLow);
};
updateLiteMode();
mediaQuery.addEventListener('change', updateLiteMode);
return () => {
mediaQuery.removeEventListener('change', updateLiteMode);
};
}, []);| next(); | ||
| }; | ||
|
|
||
| export function applySecurity(app: any) { |
There was a problem hiding this comment.
Using any for the app parameter defeats the purpose of TypeScript. You should use the Express type from the express package for better type safety. This also applies to other uses of any in this file (e.g., for req, res, next, and the CORS callback parameters).
| export function applySecurity(app: any) { | |
| export function applySecurity(app: import('express').Express) { |
| try { | ||
| event = stripe.webhooks.constructEvent(req.body, sig, process.env.STRIPE_WEBHOOK_SECRET); | ||
| } catch (err: any) { | ||
| console.error('⚠️ Webhook signature verification failed.', err.message); |
There was a problem hiding this comment.
This PR introduces a structured logger (pino) which should be used instead of console.error for consistent, structured logging. Please replace all console.* calls in this file with the appropriate logger methods (e.g., logger.error, logger.info). You'll need to import it via import { logger } from '../../utils/logger';.
| console.error('⚠️ Webhook signature verification failed.', err.message); | |
| logger.error({ err: err.message }, '⚠️ Webhook signature verification failed.'); |
| // handle checkout | ||
| // Example: update user tier in Firestore | ||
| try { | ||
| const session = event.data.object as any; |
There was a problem hiding this comment.
Casting event.data.object to any is not type-safe. The stripe library provides detailed types for webhook events. You can cast it to Stripe.Checkout.Session to get type checking and autocompletion for the session object.
| const session = event.data.object as any; | |
| const session = event.data.object as Stripe.Checkout.Session; |
| timestamp: pino.stdTimeFunctions.isoTime | ||
| }); | ||
|
|
||
| export function requestLogger(req: any, res: any, next: any) { |
There was a problem hiding this comment.
The middleware parameters are typed as any. For better type safety and code clarity, you should use the specific types from Express: Request, Response, and NextFunction. You'll need to import these at the top of the file.
| export function requestLogger(req: any, res: any, next: any) { | |
| export function requestLogger(req: import('express').Request, res: import('express').Response, next: import('express').NextFunction) { |
| }); | ||
|
|
||
| export function requestLogger(req: any, res: any, next: any) { | ||
| const rid = req.headers['x-request-id'] || `${Date.now()}-${Math.random().toString(36).slice(2,8)}`; |
There was a problem hiding this comment.
Generating a request ID with Date.now() and Math.random() is not guaranteed to be unique, especially under high load. Since uuid is already a dependency in this project, consider using a more robust method to generate a unique request ID, for example with crypto.randomUUID() (available in Node.js >= 15.6.0) or the uuid package.
| const rid = req.headers['x-request-id'] || `${Date.now()}-${Math.random().toString(36).slice(2,8)}`; | |
| const rid = req.headers['x-request-id'] || require('crypto').randomUUID(); |
| @@ -0,0 +1,7 @@ | |||
| // Worker that calls the LLM + image generator and stores the result | |||
| export async function createMandala({ prompt, seed, userId }: any) { | |||
There was a problem hiding this comment.
Using any for function parameters is not type-safe and misses an opportunity to enforce the data contract for this worker. You should define an interface for the job data to ensure correctness and improve maintainability.
interface CreateMandalaPayload {
prompt: string;
seed: number;
userId: string;
}
export async function createMandala({ prompt, seed, userId }: CreateMandalaPayload) {![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| script: | ||
| - npm ci --legacy-peer-deps | ||
| - npm run test:unit |
There was a problem hiding this comment.
The new unit_test gate runs npm run test:unit, but the root package.json only exposes the default test script. On every merge request this job will exit immediately with “missing script: test:unit”, so the unit gate never executes.
Useful? React with 👍 / 👎.
| script: | ||
| - npm ci --legacy-peer-deps | ||
| - npm run test:integration |
There was a problem hiding this comment.
The integration_test job invokes npm run test:integration, but there is no such script in the root package.json. Every merge request will see this stage fail before any integration tests can run.
Useful? React with 👍 / 👎.
| script: | ||
| - cd frontend | ||
| - npm ci --legacy-peer-deps | ||
| - npm run test:e2e |
There was a problem hiding this comment.
The e2e_test job expects npm run test:e2e inside frontend, but that package.json only defines dev/build/lint/preview scripts. As written, every merge request e2e gate will fail immediately with a missing script error.
Useful? React with 👍 / 👎.
| "nodemailer": "^7.0.11", | ||
| "helmet": "^7.1.0", | ||
| "passkit-generator": "^3.5.6", | ||
| "pdfkit": "^0.17.2", | ||
| "pino": "^9.5.0", |
There was a problem hiding this comment.
New runtime dependencies (e.g., helmet and pino) were added to package.json, but package-lock.json was not updated in this commit. The newly added npm ci --legacy-peer-deps steps in the CI jobs will fail with “package-lock.json is not in sync with package.json” before installing any packages.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR scaffolds production hardening infrastructure including security middleware, structured logging, background job processing, CI test gates, PWA support, and mobile readiness configurations. The changes introduce foundational patterns for observability, security, and deployment workflows to prepare the application for production launch.
- Adds security hardening with helmet, CORS whitelisting, and rate limiting middleware
- Implements structured logging with pino and request tracking
- Introduces BullMQ-based queue system for async AI operations (mandala generation)
- Extends CI/CD pipeline with unit, integration, e2e test gates and post-deploy smoke tests
- Adds PWA manifest, service worker, and Capacitor config for mobile deployment
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 20 comments.
Show a summary per file
| File | Description |
|---|---|
src/middleware/security.ts |
Implements helmet, CORS, rate limiting, and WAF placeholder for API security |
src/utils/logger.ts |
Adds pino-based structured logger with request ID tracking middleware |
src/routes/payments/webhook_secure.ts |
Implements Stripe webhook signature verification and payment event handling |
src/queues/mandalaQueue.ts |
Sets up BullMQ queue and worker for async mandala generation jobs |
src/workers/mandalaWorker.ts |
Stub worker function for mandala generation with AI models |
frontend/manifest.json |
PWA manifest for installable app experience |
frontend/service-worker.js |
Service worker shell for offline capability (stub) |
frontend/src/hooks/useLiteMode.ts |
React hook to detect and enable lite rendering mode |
frontend/design/tokens.css |
CSS design tokens for consistent theming |
mobile/capacitor.config.json |
Capacitor configuration for iOS/Android builds |
.gitlab-ci.yml |
Adds CI test gates (unit, integration, e2e) and post-deploy smoke test |
package.json |
Adds bullmq, helmet, and pino dependencies |
.env.example |
Documents CORS whitelist, rate limiting, and secret manager env vars |
config/feature-flags.json |
Feature flag configuration for gradual rollout |
docs/*.md |
Documentation for security, observability, AI performance, UX, rollout, and mobile |
scripts/post_deploy_smoke.js |
Post-deployment smoke test script for critical endpoints |
| const router = express.Router(); | ||
|
|
||
| // Expect STRIPE_WEBHOOK_SECRET in env/secret manager | ||
| const stripe = new Stripe(process.env.STRIPE_SECRET_KEY || '', { apiVersion: '2022-11-15' }); |
There was a problem hiding this comment.
The Stripe API version '2022-11-15' is outdated. Using an old API version may miss important security updates, bug fixes, and new features. Consider updating to a more recent version or using the latest stable API version.
| const stripe = new Stripe(process.env.STRIPE_SECRET_KEY || '', { apiVersion: '2022-11-15' }); | |
| const stripe = new Stripe(process.env.STRIPE_SECRET_KEY || ''); |
| await collections.users.doc(userId).update({ stripe_subscription_id: session.subscription }); | ||
| } | ||
| } catch (e) { | ||
| console.error('Error handling checkout.session.completed', e); |
There was a problem hiding this comment.
Missing error handling for the Firestore update operation. If the update fails, the webhook will still return a 200 success response to Stripe, causing the payment event to be marked as processed when it actually failed. The error should be thrown or the response should indicate failure so Stripe can retry the webhook.
| console.error('Error handling checkout.session.completed', e); | |
| console.error('Error handling checkout.session.completed', e); | |
| return res.status(500).send('Failed to process webhook'); |
| import { Queue, Worker, QueueScheduler } from 'bullmq'; | ||
| import IORedis from 'ioredis'; | ||
| import { createMandala } from '../workers/mandalaWorker'; | ||
|
|
||
| const connection = new IORedis(process.env.REDIS_URL || 'redis://127.0.0.1:6379'); | ||
|
|
||
| export const mandalaQueue = new Queue('mandala', { connection }); | ||
| export const mandalaScheduler = new QueueScheduler('mandala', { connection }); |
There was a problem hiding this comment.
QueueScheduler is deprecated in BullMQ v3+. Modern versions of BullMQ handle scheduling internally within the Queue class itself. Remove the QueueScheduler import and instantiation.
| import { Queue, Worker, QueueScheduler } from 'bullmq'; | |
| import IORedis from 'ioredis'; | |
| import { createMandala } from '../workers/mandalaWorker'; | |
| const connection = new IORedis(process.env.REDIS_URL || 'redis://127.0.0.1:6379'); | |
| export const mandalaQueue = new Queue('mandala', { connection }); | |
| export const mandalaScheduler = new QueueScheduler('mandala', { connection }); | |
| import { Queue, Worker } from 'bullmq'; | |
| import IORedis from 'ioredis'; | |
| import { createMandala } from '../workers/mandalaWorker'; | |
| const connection = new IORedis(process.env.REDIS_URL || 'redis://127.0.0.1:6379'); | |
| export const mandalaQueue = new Queue('mandala', { connection }); |
| { | ||
| "name": "Defrag", | ||
| "short_name": "Defrag", | ||
| "start_url": "/", | ||
| "scope": "/", | ||
| "display": "standalone", | ||
| "background_color": "#0b0b0f", | ||
| "theme_color": "#0b0b0f", | ||
| "icons": [ | ||
| { "src": "/icons/icon-192.png", "sizes": "192x192", "type": "image/png" }, | ||
| { "src": "/icons/icon-512.png", "sizes": "512x512", "type": "image/png" } | ||
| ] | ||
| } |
There was a problem hiding this comment.
Missing 'description' field in the PWA manifest. This field helps users understand what the app does when they're considering installing it, improving discoverability and accessibility.
| }); | ||
|
|
||
| export function requestLogger(req: any, res: any, next: any) { | ||
| const rid = req.headers['x-request-id'] || `${Date.now()}-${Math.random().toString(36).slice(2,8)}`; |
There was a problem hiding this comment.
The random request ID generation uses Math.random() which is not cryptographically secure. For better uniqueness and to avoid potential collisions in high-traffic scenarios, consider using the 'uuid' package (already in dependencies) or crypto.randomUUID() for generating request IDs.
| worker.on('completed', (job) => { | ||
| console.log(`Mandala job ${job.id} completed`); | ||
| }); | ||
|
|
||
| worker.on('failed', (job, err) => { | ||
| console.error(`Mandala job ${job?.id} failed`, err); |
There was a problem hiding this comment.
Using console.log and console.error instead of the structured logger. The codebase has a pino-based logger (src/utils/logger.ts) that should be used for consistency and proper log aggregation in production. Import and use the logger instead of console methods.
| // WAF placeholder: add WAF checks or integrate cloud WAF (Cloud Armor, Cloudflare) | ||
| app.use((req: any, res: any, next: any) => { | ||
| // Example: block suspicious UA or call pattern | ||
| const ua = req.get('user-agent') || ''; | ||
| if (ua.includes('sqlmap') || ua.includes('nikto')) { | ||
| return res.status(403).json({ error: 'Forbidden' }); | ||
| } | ||
| next(); | ||
| }); |
There was a problem hiding this comment.
Hardcoded user agent string checks for 'sqlmap' and 'nikto' are easily bypassed by attackers who can modify their user agent headers. This provides minimal security value while adding maintenance overhead. Consider removing this naive check and relying on proper Web Application Firewall (WAF) solutions like Cloud Armor or Cloudflare that are mentioned in the comment above.
| // WAF placeholder: add WAF checks or integrate cloud WAF (Cloud Armor, Cloudflare) | |
| app.use((req: any, res: any, next: any) => { | |
| // Example: block suspicious UA or call pattern | |
| const ua = req.get('user-agent') || ''; | |
| if (ua.includes('sqlmap') || ua.includes('nikto')) { | |
| return res.status(403).json({ error: 'Forbidden' }); | |
| } | |
| next(); | |
| }); | |
| // WAF integration should be configured at the infrastructure level (e.g., Cloud Armor, Cloudflare). |
| self.addEventListener('fetch', (e) => { | ||
| // Basic network-first for API, cache-first for static | ||
| }); |
There was a problem hiding this comment.
The service worker fetch event handler is empty, providing no actual caching logic. This means the service worker registers but doesn't improve performance or enable offline functionality. Either implement proper caching strategies (network-first for API calls, cache-first for static assets) as described in the comment, or remove the fetch event listener entirely.
| { "src": "/icons/icon-192.png", "sizes": "192x192", "type": "image/png" }, | ||
| { "src": "/icons/icon-512.png", "sizes": "512x512", "type": "image/png" } |
There was a problem hiding this comment.
Missing required 'purpose' field for PWA icons. Modern browsers expect the 'purpose' field to distinguish between maskable and any icons for better adaptive icon support. Add "purpose": "any" or "purpose": "maskable any" to each icon entry.
| { "src": "/icons/icon-192.png", "sizes": "192x192", "type": "image/png" }, | |
| { "src": "/icons/icon-512.png", "sizes": "512x512", "type": "image/png" } | |
| { "src": "/icons/icon-192.png", "sizes": "192x192", "type": "image/png", "purpose": "any" }, | |
| { "src": "/icons/icon-512.png", "sizes": "512x512", "type": "image/png", "purpose": "any" } |
| 1. Move secrets to your cloud Secret Manager (GCP/AWS) or GitLab CI variables (masked & protected). Rotate any sample/legacy keys. Do not store secrets in repo. | ||
| 2. Update `src/config/env.validator.ts` to validate presence of secrets only via env or secret manager client. Use short-lived credentials or service accounts. | ||
| 3. Add `src/middleware/security.ts` (helmet, CORS whitelist from env, express-rate-limit). Wire it into `src/index.ts`. | ||
| 4. Replace `node-fetch` usage with native `fetch` (Node 18+). Remove `node-fetch` and update code to `await fetch(...)`. |
There was a problem hiding this comment.
The comment instructs to "Replace node-fetch usage with native fetch (Node 18+). Remove node-fetch", but node-fetch is still present in the dependencies at line 31. For production hardening, this dependency should be removed and the codebase updated to use native fetch as documented.
| 4. Replace `node-fetch` usage with native `fetch` (Node 18+). Remove `node-fetch` and update code to `await fetch(...)`. | |
| 4. Replace all `node-fetch` imports/usages with the native `fetch` API (Node 18+), e.g. `const res = await fetch(...)`, and then remove `node-fetch` entirely from `package.json` dependencies. |
Summary
Testing
npm install helmet pino bullmq(fails with 403 from npm registry in this environment; dependencies recorded in package.json, package-lock.json unchanged)Codex Task