Website: API Service changed #2231
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to environment variable configurations and API URL handling across several files. The variable Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (3)
website2/src/app/api/proxy/route.ts (1)
Line range hint
63-73: Remove or configure production loggingThe extensive console logging in the POST route might expose sensitive information in production. Consider:
- Using a proper logging service
- Implementing environment-based logging levels
- // Log to check what the remaining body looks like - console.log('Received body (without endpoint):', body); - // Log the URL you're about to make the request to - console.log('API URL:', apiUrl);website2/src/services/apiService/index.tsx (1)
1-1: Consider architectural improvements for better type safety and error handlingWhile the current changes look good, here are some suggestions for future improvements:
- Add TypeScript interfaces for API responses
- Implement request timeout in axios configuration
- Add request interceptors for consistent error handling
- Consider implementing retry logic for failed requests
Example implementation:
// Response interfaces interface ApiResponse<T> { data: T; status: number; } // Axios configuration const apiClient = axios.create({ baseURL: API_BASE_URL, timeout: 10000, headers: { 'Content-Type': 'application/json', }, }); // Error interceptor apiClient.interceptors.response.use( response => response, error => { if (error.response?.status === 404) { console.warn('Resource not found:', error.config.url); } return Promise.reject(error); } );website2/src/components/sections/footer/CountrySelectorDialog.tsx (1)
83-104: Consider environment configuration improvementsThe current environment-based conditional logic could be better handled through a service layer abstraction.
Consider creating an API service layer that handles environment differences:
// services/api/gridsService.ts export class GridsService { private static async fetchGrids(): Promise<GridsSummaryResponse> { if (process.env.NODE_ENV === 'development') { return fetch('/api/proxy?endpoint=devices/grids/summary') .then(response => { if (!response.ok) throw new Error(response.statusText); return response.json(); }); } return getGridsSummary(); } public static async getGridsSummary(): Promise<GridsSummaryResponse> { try { const data = await this.fetchGrids(); if (!data?.grids) { throw new Error('Invalid response structure'); } return data; } catch (error) { throw new Error(`Failed to fetch grids: ${error.message}`); } } }This approach would:
- Encapsulate environment-specific logic
- Provide a consistent interface for components
- Make testing and maintenance easier
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
website2/.env.sample(1 hunks)website2/src/app/api/proxy/route.ts(1 hunks)website2/src/components/sections/footer/CountrySelectorDialog.tsx(2 hunks)website2/src/services/apiService/index.tsx(1 hunks)website2/src/services/externalService/index.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- website2/.env.sample
🔇 Additional comments (5)
website2/src/services/externalService/index.tsx (1)
52-52: Verify API endpoint configuration
Let's ensure the endpoint path is consistent with other usages in the codebase.
✅ Verification successful
API endpoint /devices/grids/summary is consistently used across the codebase
The endpoint is correctly used and appears in multiple services:
reports/src/services/api/index.tsxplatform/src/core/urls/deviceRegistry.jswebsite/frontend/config/urls.jswebsite2/src/services/externalService/index.tsx
The implementation aligns with existing usage patterns in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of this endpoint
rg -g '!*.{json,md}' '/devices/grids/summary'
# Check for any similar grid-related endpoints
rg -g '!*.{json,md}' '/devices/grids/'
Length of output: 1006
website2/src/app/api/proxy/route.ts (3)
Line range hint 8-45: LGTM: GET route implementation
The GET route implementation looks good with proper:
- URL construction
- Error handling
- Response handling
Line range hint 47-95: LGTM: POST route implementation
The POST route implementation looks good with proper:
- Request body handling
- Error handling
- Response handling
5-6: Verify environment variable updates across the application
The change from NEXT_PUBLIC_EXTERNAL_API_URL to NEXT_PUBLIC_API_URL needs to be consistently applied across all environments and documentation.
website2/src/components/sections/footer/CountrySelectorDialog.tsx (1)
19-19: Verify external service dependency implementation
Let's ensure the imported getGridsSummary function is properly implemented in the external service.
✅ Verification successful
External service dependency is properly implemented
The getGridsSummary function is correctly implemented in website2/src/services/externalService/index.tsx. It's an authenticated API endpoint that:
- Makes a GET request to '/devices/grids/summary'
- Uses the API token for authentication
- Handles errors appropriately
- Returns the response data or null on error
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of getGridsSummary in the external service
# Search for the function definition
ast-grep --pattern 'export const getGridsSummary = $_' || ast-grep --pattern 'export function getGridsSummary($_)'
# Search for any usage of this function
rg "getGridsSummary" --type ts --type tsx
Length of output: 1178
Script:
#!/bin/bash
# Search for getGridsSummary usage with proper file type options
rg "getGridsSummary" -t typescript -t tsx
# Verify the external service file structure and exports
rg "export" -t typescript -t tsx website2/src/services/externalService/
Length of output: 183
Script:
#!/bin/bash
# Search for getGridsSummary usage without file type restrictions
rg "getGridsSummary"
# Check the content of the external service file
cat website2/src/services/externalService/index.tsx
Length of output: 2630
| const API_BASE_URL = removeTrailingSlash(process.env.NEXT_PUBLIC_API_URL || ''); | ||
| const API_TOKEN = process.env.NEXT_PUBLIC_API_TOKEN || ''; |
There was a problem hiding this comment.
Review security implications of exposing API token
The NEXT_PUBLIC_ prefix makes this token available in the client-side bundle. Consider if this level of exposure is necessary, as client-side tokens can be extracted and potentially misused.
Consider these alternatives:
- Move token-dependent requests to API routes
- Implement request signing
- Add rate limiting and domain restrictions
| // Get grids summary endpoint it uses a api_token to authenticate the request | ||
| export const getGridsSummary = async (): Promise<any | null> => { | ||
| try { | ||
| const response = await apiClient.get('/devices/grids/summary', { | ||
| params: { | ||
| token: API_TOKEN, | ||
| }, | ||
| }); | ||
|
|
||
| return response.data; | ||
| } catch (error) { | ||
| console.error('Error fetching grids summary data:', error); | ||
| return null; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance type safety and error handling
The implementation could benefit from several improvements:
- The return type
Promise<any | null>is too permissive - Returning null on error might hide important error information
- Comment style differs from other functions (uses inline comment vs JSDoc)
Consider this improved implementation:
-// Get grids summary endpoint it uses a api_token to authenticate the request
+/**
+ * Fetches summary data for all grids
+ * @returns {Promise<GridSummary | Error>} The grids summary data or error
+ */
-export const getGridsSummary = async (): Promise<any | null> => {
+interface GridSummary {
+ // define your expected response structure here
+}
+
+export const getGridsSummary = async (): Promise<GridSummary> => {
try {
const response = await apiClient.get('/devices/grids/summary', {
params: {
token: API_TOKEN,
},
});
- return response.data;
+ return response.data as GridSummary;
} catch (error) {
console.error('Error fetching grids summary data:', error);
- return null;
+ throw error;
}
};Committable suggestion skipped: line range outside the PR's diff.
| const API_BASE_URL = removeTrailingSlash(process.env.NEXT_PUBLIC_API_URL || ''); | ||
| const API_TOKEN = process.env.NEXT_PUBLIC_API_TOKEN || ''; |
There was a problem hiding this comment.
Consider security implications of public environment variables
Using NEXT_PUBLIC_API_TOKEN exposes the token in the client-side bundle. Consider:
- Moving token handling to a secure backend endpoint
- Implementing proper authentication mechanisms
- Using runtime environment variables instead of build-time ones
| process.env.NEXT_PUBLIC_DJANGO_API_URL || 'http://127.0.0.1:8000/api'; | ||
| import { removeTrailingSlash } from '@/utils'; | ||
|
|
||
| const API_BASE_URL = `${removeTrailingSlash(process.env.NEXT_PUBLIC_API_URL || '')}/website`; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding URL validation and proper fallback
The current implementation might result in invalid URLs if NEXT_PUBLIC_API_URL is not set. Consider adding validation and a proper fallback URL.
Here's a suggested improvement:
-const API_BASE_URL = `${removeTrailingSlash(process.env.NEXT_PUBLIC_API_URL || '')}/website`;
+const DEFAULT_API_URL = 'http://localhost:8000';
+const apiUrl = process.env.NEXT_PUBLIC_API_URL || DEFAULT_API_URL;
+const API_BASE_URL = `${removeTrailingSlash(apiUrl)}/website`;
+
+if (!API_BASE_URL.startsWith('http')) {
+ console.warn('Invalid API URL configuration detected');
+}📝 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.
| const API_BASE_URL = `${removeTrailingSlash(process.env.NEXT_PUBLIC_API_URL || '')}/website`; | |
| const DEFAULT_API_URL = 'http://localhost:8000'; | |
| const apiUrl = process.env.NEXT_PUBLIC_API_URL || DEFAULT_API_URL; | |
| const API_BASE_URL = `${removeTrailingSlash(apiUrl)}/website`; | |
| if (!API_BASE_URL.startsWith('http')) { | |
| console.warn('Invalid API URL configuration detected'); | |
| } |
| import { removeTrailingSlash } from '@/utils'; | ||
|
|
||
| const API_BASE_URL = `${removeTrailingSlash(process.env.NEXT_PUBLIC_API_URL || '')}/website`; |
There was a problem hiding this comment.
💡 Codebase verification
Environment variable transition needs configuration updates
The switch from NEXT_PUBLIC_DJANGO_API_URL to NEXT_PUBLIC_API_URL requires the following actions:
- Update the Dockerfile in
website2/to useNEXT_PUBLIC_API_URLinstead ofNEXT_PUBLIC_DJANGO_API_URL - The new variable
NEXT_PUBLIC_API_URLis consistently used across multiple services (apiService, externalService, proxy route) - All URL constructions properly handle empty values with fallbacks
The verification revealed that while the codebase has mostly transitioned to the new environment variable, the Dockerfile still references the old one, which could cause deployment issues. The URL construction itself is safe with proper empty string fallbacks and trailing slash handling.
🔗 Analysis chain
Verify environment variable configuration across all environments
The switch from NEXT_PUBLIC_DJANGO_API_URL to NEXT_PUBLIC_API_URL requires verification of environment configurations.
Let's check the environment variable usage:
Let's get a more comprehensive view of the environment variable usage and configuration across the codebase, including hidden files and deployment configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify environment variable usage and configuration files
# Check for any remaining references to old env var
echo "Checking for old environment variable references..."
rg "NEXT_PUBLIC_DJANGO_API_URL"
# Look for .env files and environment configuration
echo "Checking environment configuration files..."
fd -t f "^\.env" -X cat
Length of output: 609
Script:
#!/bin/bash
# Description: Comprehensive check for environment variable usage and configuration
# Check for both old and new environment variable references
echo "Checking environment variable references..."
rg -A 2 "NEXT_PUBLIC_.*API_URL"
# Check Dockerfile and deployment configs with hidden files
echo "Checking configuration files..."
fd -H -t f "(Dockerfile|\.env.*|docker-compose.*\.yml|\.env$)" -X cat
# Check for any potential URL construction in the codebase
echo "Checking URL construction patterns..."
rg "process\.env\.NEXT_PUBLIC.*URL.*" -A 2
Length of output: 13072
| let data; | ||
|
|
||
| if (process.env.NODE_ENV === 'development') { | ||
| // Use proxy in development | ||
| const response = await fetch( | ||
| `/api/proxy?endpoint=devices/grids/summary`, | ||
| { | ||
| method: 'GET', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| signal: abortSignal, | ||
| }, | ||
| signal: abortSignal, | ||
| }, | ||
| ); | ||
| ); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(`Error: ${response.statusText}`); | ||
| } | ||
| if (!response.ok) { | ||
| throw new Error(`Error: ${response.statusText}`); | ||
| } | ||
|
|
||
| const data = await response.json(); | ||
| data = await response.json(); | ||
| } else { | ||
| // Use getGridsSummary in production | ||
| data = await getGridsSummary(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety and improve error handling
The environment-based conditional logic is well-structured, but there are a few improvements we can make:
- Add type safety for the API response
- Handle errors explicitly for getGridsSummary
- Ensure consistent response structure between environments
Consider applying these changes:
let data;
if (process.env.NODE_ENV === 'development') {
// Use proxy in development
const response = await fetch(
`/api/proxy?endpoint=devices/grids/summary`,
{
method: 'GET',
headers: {
'Content-Type': 'application/json',
},
signal: abortSignal,
},
);
if (!response.ok) {
throw new Error(`Error: ${response.statusText}`);
}
data = await response.json();
} else {
// Use getGridsSummary in production
- data = await getGridsSummary();
+ try {
+ data = await getGridsSummary();
+ if (!data?.grids) {
+ throw new Error('Invalid response structure from getGridsSummary');
+ }
+ } catch (error) {
+ throw new Error(`Error fetching grids summary: ${error.message}`);
+ }
}Also, consider adding a type interface for the API response:
interface GridsSummaryResponse {
grids: Array<{
_id: string;
name: string;
long_name: string;
admin_level: string;
numberOfSites: number;
}>;
}
Summary of Changes (What does this PR do?)
Status of maturity (all need to be checked before merging):
How should this be manually tested?
What are the relevant tickets?
Screenshots (optional)
Summary by CodeRabbit
New Features
Bug Fixes
Chores