feat: implement centralized configuration management#488
Open
Midoriya-w wants to merge 5 commits into
Open
Conversation
Conducted a technical debt audit across the codebase and documented findings, conclusions, and recommendations.
|
Someone is attempting to deploy a commit to the firefistisdead's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
📝 WalkthroughWalkthroughThis PR centralizes configuration by adding backend and frontend config modules, updates server and frontend code to consume those modules instead of reading process.env directly, updates ChangesConfiguration System Centralization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
frontend/src/services/api.jsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
backend
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.env.example:
- Line 47: The file ends without a final newline which triggers dotenv-linter's
EndingBlankLine; open the .env.example and add a single trailing newline after
the last entry (the NODE_ENV=development line) so the file ends with a blank
line/EOF newline character.
- Around line 31-47: The .env.example currently sets PORT=5000 which conflicts
with the RAG service default at RAG_SERVICE_URL=http://localhost:5000; update
the template so the gateway and RAG service do not share the same default
port—either change PORT to a different unused default (e.g., 3000) or change
RAG_SERVICE_URL to point to a different default port, and add a short comment
next to PORT and RAG_SERVICE_URL clarifying that they must not be identical to
avoid port conflicts; ensure you update references in docs or README if any
mention the old defaults.
In `@frontend/src/config/index.js`:
- Around line 1-5: The config export currently defaults apiBaseUrl to an empty
string which can silently break API calls in production; update the logic that
constructs/exporting const config (apiBaseUrl) to validate at startup: if
process.env.NODE_ENV === "production" and apiBaseUrl is falsy (empty string),
throw an Error (or call console.error and throw) with a clear message
instructing to set REACT_APP_API_URL, otherwise (for non-production) emit a
console.warn explaining that empty string implies same-origin behavior; keep the
exported symbol name config and its apiBaseUrl property unchanged so callers
continue to work.
In `@src/config/index.js`:
- Around line 1-27: The config module currently exports a plain config object
without validation; add startup validation in src/config/index.js that checks
required env vars (especially ragServiceUrl) for presence and well-formedness
(e.g., use a URL parse/regex) and throws a clear Error to fail fast if invalid,
log the resolved config values (masking secrets) using the existing logger or
console when nodeEnv !== "production", and ensure numeric parses (proxyCount,
uploadLimitMax, slowDownAfter, inferenceMax) are validated for NaN and within
expected ranges; update the module to export the validated config (references:
config object, ragServiceUrl, proxyCount, uploadLimitMax, slowDownAfter,
inferenceMax, nodeEnv).
- Line 5: The config entry for port currently sets port: process.env.PORT ||
4000 which yields mixed types and the wrong default; update the "port" property
in src/config/index.js to parse the environment value as an integer (e.g., using
parseInt or Number) and change the fallback default to 5000 to match
.env.example so config.port is always a number (ensure you handle NaN by falling
back to 5000).
- Around line 7-22: The parseInt calls that populate proxyCount, uploadLimitMax,
slowDownAfter, and inferenceMax can return NaN for invalid env values; update
the config creation to validate each parsed value (the results of parseInt for
proxyCount, uploadLimitMax, slowDownAfter, inferenceMax) and either throw a
clear error or fall back to a safe default when Number.isNaN(parsed) is true;
ensure the validation happens immediately after parsing (before exporting the
config object) so callers like server.js never receive NaN and can rely on
strict numeric semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f066d8f8-ad8a-40c3-97f0-c60bdb87902b
📒 Files selected for processing (5)
.env.examplefrontend/src/config/index.jsfrontend/src/services/api.jsserver.jssrc/config/index.js
Comment on lines
+31
to
+47
| # Express API Gateway settings | ||
|
|
||
| # ALLOWED_ORIGIN: Restricts CORS to this frontend origin only. | ||
| # In production, set this to your deployed frontend URL (e.g. https://yourapp.com). | ||
| # Requests from any other origin will be blocked by the browser. | ||
| ALLOWED_ORIGIN=http://localhost:3000 | ||
| PROXY_COUNT=0 | ||
|
|
||
| # ─ Semantic Chunking (RAG Service) ─────────────────────────────────────────── | ||
| RATE_LIMIT_UPLOAD_MAX=10 | ||
|
|
||
| # Soft maximum characters per chunk after Pass 1 (boundary-aware split). | ||
| # Chunks will NOT be cut mid-sentence just to hit this limit. | ||
| # Default: 1200 | ||
| SEMANTIC_CHUNK_SOFT_MAX=1200 | ||
| RATE_LIMIT_SLOWDOWN_AFTER=10 | ||
|
|
||
| # Chunks shorter than this (chars) become candidates for semantic merging in Pass 2. | ||
| # Default: 150 | ||
| SEMANTIC_CHUNK_MERGE_MIN=150 | ||
| RATE_LIMIT_INFERENCE_MAX=30 | ||
|
|
||
| # Hard upper bound on merged chunk length. Merging is skipped even when | ||
| # similarity >= threshold if the result would exceed this size. | ||
| # Default: 1400 | ||
| SEMANTIC_CHUNK_MERGE_MAX=1400 | ||
| # Frontend | ||
|
|
||
| # Cosine similarity threshold for merging adjacent tiny chunks (Pass 2). | ||
| # Range: 0.0 (always merge) to 1.0 (never merge). Recommended: 0.70–0.80. | ||
| # Default: 0.75 | ||
| SEMANTIC_CHUNK_SIMILARITY_THRESHOLD=0.75 | ||
| REACT_APP_API_URL= | ||
|
|
||
| # Wall-clock warning threshold for the Pass 2 embedding calls (seconds). | ||
| # A WARNING is logged if semantic merging for a single page exceeds this. | ||
| # Default: 5.0 | ||
| SEMANTIC_CHUNK_MERGE_WARN_SECS=5.0 | ||
| # Optional Node environment | ||
|
|
||
| # Enable hierarchical chunk storage (Pass 2b). | ||
| # Each chunk carries small_chunk (retrieval unit) and parent_chunk | ||
| # (small chunk + 1–2 neighbour sentences, sent to the LLM as context). | ||
| # Set to false to disable and save metadata memory. | ||
| # Default: true | ||
| SEMANTIC_CHUNK_HIERARCHICAL=true | ||
| NODE_ENV=development No newline at end of file |
Contributor
There was a problem hiding this comment.
Documented gateway config conflicts with existing PORT=5000 default.
After centralizing backend config, PORT now controls the Express gateway (src/config/index.js), but this template still sets PORT=5000 while RAG_SERVICE_URL also targets http://localhost:5000. Copying this file can make the gateway compete with the RAG service on the same port.
Suggested doc fix
-# Optional app host/port settings
+# Express API Gateway host/port settings
HOST=127.0.0.1
-PORT=5000
+PORT=4000
# URL of the RAG FastAPI service used by the Express API Gateway.
# Default: http://localhost:5000
RAG_SERVICE_URL=http://localhost:5000🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 47-47: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.env.example around lines 31 - 47, The .env.example currently sets PORT=5000
which conflicts with the RAG service default at
RAG_SERVICE_URL=http://localhost:5000; update the template so the gateway and
RAG service do not share the same default port—either change PORT to a different
unused default (e.g., 3000) or change RAG_SERVICE_URL to point to a different
default port, and add a short comment next to PORT and RAG_SERVICE_URL
clarifying that they must not be identical to avoid port conflicts; ensure you
update references in docs or README if any mention the old defaults.
Comment on lines
+1
to
+5
| const config = { | ||
| apiBaseUrl: process.env.REACT_APP_API_URL || "", | ||
| }; | ||
|
|
||
| export default config; No newline at end of file |
Contributor
There was a problem hiding this comment.
Validate or document that apiBaseUrl can be an empty string.
The empty string default for apiBaseUrl means API calls will use relative URLs (e.g., /upload). This only works if the frontend and backend are served from the same origin (e.g., via a dev proxy or same-domain production deployment).
Risks:
- In production, if
REACT_APP_API_URLis not set, API calls will silently fail or hit the wrong endpoint. - The error is not discoverable until runtime (when the first API call is made).
Recommendations:
- Option 1 (preferred): Add startup validation that throws an error if
apiBaseUrlis empty and the app is in production mode. - Option 2: Document in comments that empty string is intentional for same-origin deployments and add runtime logging.
🛡️ Option 1: Fail-fast validation
const config = {
apiBaseUrl: process.env.REACT_APP_API_URL || "",
};
+// Validate in production
+if (!config.apiBaseUrl && process.env.NODE_ENV === "production") {
+ throw new Error(
+ "Configuration error: REACT_APP_API_URL must be set in production"
+ );
+}
+
export default config;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const config = { | |
| apiBaseUrl: process.env.REACT_APP_API_URL || "", | |
| }; | |
| export default config; | |
| const config = { | |
| apiBaseUrl: process.env.REACT_APP_API_URL || "", | |
| }; | |
| // Validate in production | |
| if (!config.apiBaseUrl && process.env.NODE_ENV === "production") { | |
| throw new Error( | |
| "Configuration error: REACT_APP_API_URL must be set in production" | |
| ); | |
| } | |
| export default config; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/config/index.js` around lines 1 - 5, The config export currently
defaults apiBaseUrl to an empty string which can silently break API calls in
production; update the logic that constructs/exporting const config (apiBaseUrl)
to validate at startup: if process.env.NODE_ENV === "production" and apiBaseUrl
is falsy (empty string), throw an Error (or call console.error and throw) with a
clear message instructing to set REACT_APP_API_URL, otherwise (for
non-production) emit a console.warn explaining that empty string implies
same-origin behavior; keep the exported symbol name config and its apiBaseUrl
property unchanged so callers continue to work.
Comment on lines
+1
to
+27
| const config = { | ||
| ragServiceUrl: | ||
| process.env.RAG_SERVICE_URL || "http://localhost:5000", | ||
|
|
||
| port: process.env.PORT || 4000, | ||
|
|
||
| proxyCount: parseInt(process.env.PROXY_COUNT || "0", 10), | ||
|
|
||
| uploadLimitMax: parseInt( | ||
| process.env.RATE_LIMIT_UPLOAD_MAX || "10", | ||
| 10 | ||
| ), | ||
|
|
||
| slowDownAfter: parseInt( | ||
| process.env.RATE_LIMIT_SLOWDOWN_AFTER || "10", | ||
| 10 | ||
| ), | ||
|
|
||
| inferenceMax: parseInt( | ||
| process.env.RATE_LIMIT_INFERENCE_MAX || "30", | ||
| 10 | ||
| ), | ||
|
|
||
| nodeEnv: process.env.NODE_ENV || "development", | ||
| }; | ||
|
|
||
| module.exports = config; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Add startup validation for required environment variables.
The PR objectives explicitly state "Validates required environment variables at application startup," but this module performs no validation. At minimum, consider:
- Validating that critical values like
ragServiceUrlare reachable URLs (or at least well-formed). - Logging the loaded configuration values to aid debugging.
- Failing fast with a clear error message if required variables are missing or invalid.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/config/index.js` around lines 1 - 27, The config module currently exports
a plain config object without validation; add startup validation in
src/config/index.js that checks required env vars (especially ragServiceUrl) for
presence and well-formedness (e.g., use a URL parse/regex) and throws a clear
Error to fail fast if invalid, log the resolved config values (masking secrets)
using the existing logger or console when nodeEnv !== "production", and ensure
numeric parses (proxyCount, uploadLimitMax, slowDownAfter, inferenceMax) are
validated for NaN and within expected ranges; update the module to export the
validated config (references: config object, ragServiceUrl, proxyCount,
uploadLimitMax, slowDownAfter, inferenceMax, nodeEnv).
Comment on lines
+7
to
+22
| proxyCount: parseInt(process.env.PROXY_COUNT || "0", 10), | ||
|
|
||
| uploadLimitMax: parseInt( | ||
| process.env.RATE_LIMIT_UPLOAD_MAX || "10", | ||
| 10 | ||
| ), | ||
|
|
||
| slowDownAfter: parseInt( | ||
| process.env.RATE_LIMIT_SLOWDOWN_AFTER || "10", | ||
| 10 | ||
| ), | ||
|
|
||
| inferenceMax: parseInt( | ||
| process.env.RATE_LIMIT_INFERENCE_MAX || "30", | ||
| 10 | ||
| ), |
Contributor
There was a problem hiding this comment.
Validate that parseInt does not produce NaN.
If an environment variable like PROXY_COUNT is set to a non-numeric string (e.g., "abc"), parseInt will return NaN. This will silently corrupt configuration and cause unpredictable behavior downstream (e.g., if (PROXY_COUNT > 0) at server.js:25 will evaluate to false when it should error).
🛡️ Proposed fix with validation
Add validation after constructing the config object:
nodeEnv: process.env.NODE_ENV || "development",
};
+// Validate numeric config values
+const numericFields = ["port", "proxyCount", "uploadLimitMax", "slowDownAfter", "inferenceMax"];
+for (const field of numericFields) {
+ if (isNaN(config[field])) {
+ throw new Error(`Configuration error: ${field} must be a valid number`);
+ }
+}
+
module.exports = config;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/config/index.js` around lines 7 - 22, The parseInt calls that populate
proxyCount, uploadLimitMax, slowDownAfter, and inferenceMax can return NaN for
invalid env values; update the config creation to validate each parsed value
(the results of parseInt for proxyCount, uploadLimitMax, slowDownAfter,
inferenceMax) and either throw a clear error or fall back to a safe default when
Number.isNaN(parsed) is true; ensure the validation happens immediately after
parsing (before exporting the config object) so callers like server.js never
receive NaN and can rely on strict numeric semantics.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.env.example (1)
221-230:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove duplicated env keys to avoid silent shadowing.
PROXY_COUNTandRATE_LIMIT_SLOWDOWN_AFTERare already defined earlier (Line 123 and Line 163). Keeping duplicate definitions in.envtemplates makes later entries silently override earlier ones, which can unintentionally weaken trust-proxy and rate-limit behavior.Suggested cleanup
-# Express API Gateway settings - -PROXY_COUNT=0 - -RATE_LIMIT_UPLOAD_MAX=10 - -RATE_LIMIT_SLOWDOWN_AFTER=10 - -RATE_LIMIT_INFERENCE_MAX=30 +# Express API Gateway settings +# (defined above in their detailed sections)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.env.example around lines 221 - 230, The .env.example contains duplicated environment keys that will silently shadow earlier values; remove the duplicate PROXY_COUNT and RATE_LIMIT_SLOWDOWN_AFTER entries from this Express API Gateway block so only the original definitions remain (keep the first occurrences already defined earlier), and verify RATE_LIMIT_UPLOAD_MAX and RATE_LIMIT_INFERENCE_MAX remain unique; update any nearby comment to reflect the remaining keys if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.env.example:
- Around line 221-230: The .env.example contains duplicated environment keys
that will silently shadow earlier values; remove the duplicate PROXY_COUNT and
RATE_LIMIT_SLOWDOWN_AFTER entries from this Express API Gateway block so only
the original definitions remain (keep the first occurrences already defined
earlier), and verify RATE_LIMIT_UPLOAD_MAX and RATE_LIMIT_INFERENCE_MAX remain
unique; update any nearby comment to reflect the remaining keys if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 183b3695-2aac-4d72-9fa7-0990213566d1
📒 Files selected for processing (4)
.env.examplefrontend/src/services/api.jsserver.jsvalidators/schemas.js
💤 Files with no reviewable changes (2)
- validators/schemas.js
- server.js
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/services/api.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #482
This PR introduces centralized configuration management for both backend and frontend environment variables.
Changes
src/config/index.js)frontend/src/config/index.js).env.exampleto document all supported configuration valuesBenefits
Summary by CodeRabbit
Chores
Documentation
Chores