Skip to content

Conversation

@hib4
Copy link
Member

@hib4 hib4 commented May 11, 2025

PR Type

Bug fix, Enhancement


Description

  • Use environment variables for Firebase config

  • Downgrade firebase-admin dependency to 12.7.0

  • Apply consistent code formatting and semicolons

  • Update ESLint rules and plugin ordering


Changes walkthrough 📝

Relevant files
Enhancement
1 files
firebase.ts
Use env vars for Firebase initialization                                 
+3/-2     
Formatting
4 files
index.ts
Reformat `onRequest` invocation syntax                                     
+7/-4     
application.ts
Add semicolons to route definitions                                           
+6/-5     
server.ts
Enforce consistent formatting and logging                               
+16/-10 
fake_data_populator.ts
Normalize object literals with trailing commas                     
+27/-27 
Configuration changes
1 files
.eslintrc.js
Reorder ESLint plugins and adjust rules                                   
+8/-8     
Dependencies
1 files
package.json
Downgrade `firebase-admin` to v12.7.0                                       
+1/-1     

Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @hib4 hib4 self-assigned this May 11, 2025
    @hib4 hib4 merged commit bfe9d20 into main May 11, 2025
    1 check passed
    @github-actions
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Missing Import

    express is used to create a router but not imported, causing a runtime error.

    import express from "express";
    import {
      getApplicationStatus,
      getApplicationQuestion,
      getApplicationQuestions,
      patchApplication,
      uploadFile,
      setApplicationStatusToSubmitted,
    } from "../controllers/application_controller";
    
    const router = express.Router();
    Env Var Validation

    Added process.env.PROJECT_ID and process.env.STORAGE_BUCKET without validation; missing fallback or error handling can lead to undefined configuration.

    projectId: process.env.PROJECT_ID,
    credential: admin.credential.cert(serviceAccount as admin.ServiceAccount),
    storageBucket: process.env.STORAGE_BUCKET,
    Dependency Compatibility

    Downgraded firebase-admin to v12.7.0; verify compatibility with other Firebase dependencies and API usage.

    "firebase-admin": "^12.7.0",

    @github-actions
    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Validate required env variables

    Validate that PROJECT_ID and STORAGE_BUCKET environment variables are defined before
    initializing Firebase to prevent runtime misconfiguration. Throw a clear error or
    provide a sensible default to fail fast if they are missing.

    functions/src/config/firebase.ts [8-10]

    -projectId: process.env.PROJECT_ID,
    -storageBucket: process.env.STORAGE_BUCKET,
    +const projectId = process.env.PROJECT_ID;
    +if (!projectId) throw new Error("PROJECT_ID env var is not defined");
    +const storageBucket = process.env.STORAGE_BUCKET;
    +if (!storageBucket) throw new Error("STORAGE_BUCKET env var is not defined");
     
    +admin.initializeApp({
    +  projectId,
    +  credential: admin.credential.cert(serviceAccount as admin.ServiceAccount),
    +  storageBucket,
    +});
    +
    Suggestion importance[1-10]: 8

    __

    Why: Adding explicit checks for PROJECT_ID and STORAGE_BUCKET prevents silent misconfiguration by failing fast when env vars are missing, significantly improving robustness.

    Medium
    Security
    Use structured logging

    Switch to structured logging by passing the logData object directly to
    functions.logger.info instead of stringifying it. This preserves data types,
    improves readability, and avoids logging sensitive fields in raw form.

    functions/src/server.ts [53-59]

    -functions.logger.info(
    -  `[${timestamp}] Incoming Request Details: ${JSON.stringify(
    -    logData,
    -    null,
    -    2
    -  )}`
    -);
    +functions.logger.info("Incoming Request Details", {
    +  timestamp,
    +  ...logData,
    +});
    Suggestion importance[1-10]: 6

    __

    Why: Logging the logData object directly enables structured logging for clearer, typed output and avoids manual string formatting, improving log quality.

    Low

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    2 participants