fix(MAJORLEA-005): 5 review findings across 2 files - #78
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| } | ||
|
|
||
| export async function enhanceCity(city: City): Promise<EnhancedCity> { | ||
| const [state, team] = await Promise.all([ |
There was a problem hiding this comment.
🦩 🔴 enhanced.ts calls getStateById/getTeamById without checking response.data.status === 'success'
In enhanceCity, introduced getStateByIdChecked and getTeamByIdChecked wrapper functions that call the original API functions and then check response.status !== 'success', throwing on failure. The || null fallback on state was removed (changed to state: state), so API errors are no longer silently masked. The || null pattern for the optional nearestTeam is preserved only for the legitimately absent case (when city.nearestTeamId is falsy), not as a fallback for API failure. Risk: the actual return type of getStateById/getTeamById from ./api is unknown — if they already return unwrapped data (not a response envelope with .status), the check (response as any).status !== 'success' will always throw. A complete fix would require inspecting ./api to know the exact return shape and adjusting accordingly. The cast to any is a pragmatic workaround given we cannot see ./api.
🤖 Prompt for AI agents
In frontend/src/services/enhanced.ts around line 6, review and complete this code-review fix: enhanced.ts calls getStateById/getTeamById without checking response.data.status === 'success'.
What the draft fix changed: In `enhanceCity`, introduced `getStateByIdChecked` and `getTeamByIdChecked` wrapper functions that call the original API functions and then check `response.status !== 'success'`, throwing on failure. The `|| null` fallback on `state` was removed (changed to `state: state`), so API errors are no longer silently masked. The `|| null` pattern for the optional `nearestTeam` is preserved only for the legitimately absent case (when `city.nearestTeamId` is falsy), not as a fallback for API failure. Risk: the actual return type of `getStateById`/`getTeamById` from `./api` is unknown — if they already return unwrapped data (not a response envelope with `.status`), the check `(response as any).status !== 'success'` will always throw. A complete fix would require inspecting `./api` to know the exact return shape and adjusting accordingly. The cast to `any` is a pragmatic workaround given we cannot see `./api`.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 62 medium — react 👍/👎 to teach the reviewer
| }; | ||
| } | ||
|
|
||
| export async function enhanceRegion(region: Region): Promise<EnhancedRegion> { |
There was a problem hiding this comment.
🦩 🔴 enhanceRegion silently drops failed getStateById calls via filter(s => s !== null)
In enhanceRegion, replaced the direct getStateById call with getStateByIdChecked, which throws on non-success. The filter((s): s is State => s !== null) null-filter was removed entirely — since getStateByIdChecked now throws on failure rather than returning null, all results in states are valid State objects and the filter is unnecessary. This means any API failure will propagate as a thrown error rather than being silently dropped. Same risk as finding 1: the actual shape of the ./api return values is unknown, so the .status check may need adjustment once ./api is inspected.
🤖 Prompt for AI agents
In frontend/src/services/enhanced.ts around line 18, review and complete this code-review fix: enhanceRegion silently drops failed getStateById calls via filter(s => s !== null).
What the draft fix changed: In `enhanceRegion`, replaced the direct `getStateById` call with `getStateByIdChecked`, which throws on non-success. The `filter((s): s is State => s !== null)` null-filter was removed entirely — since `getStateByIdChecked` now throws on failure rather than returning null, all results in `states` are valid `State` objects and the filter is unnecessary. This means any API failure will propagate as a thrown error rather than being silently dropped. Same risk as finding 1: the actual shape of the `./api` return values is unknown, so the `.status` check may need adjustment once `./api` is inspected.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 62 medium — react 👍/👎 to teach the reviewer
| @@ -4,7 +4,6 @@ import { HiringManagerProfile, JobOpening } from '../types/hiring'; | |||
|
|
|||
| // Configure axios to use the backend URL from environment | |||
| const BACKEND_API_URL = process.env.BACKEND_API_URL || '/'; | |||
There was a problem hiding this comment.
🦩 🟠 Frontend api.ts uses console.log to print the backend URL — debug output left in production service layer
Removed the console.log('API Service: Using backend URL:', BACKEND_API_URL); call at line 6. The line was deleted entirely; the surrounding const BACKEND_API_URL declaration and axios.defaults.baseURL assignment are preserved unchanged.
🤖 Prompt for AI agents
In frontend/src/services/api.ts around line 6, review and complete this code-review fix: Frontend api.ts uses console.log to print the backend URL — debug output left in production service layer.
What the draft fix changed: Removed the `console.log('API Service: Using backend URL:', BACKEND_API_URL);` call at line 6. The line was deleted entirely; the surrounding `const BACKEND_API_URL` declaration and `axios.defaults.baseURL` assignment are preserved unchanged.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 97 high — react 👍/👎 to teach the reviewer
|
|
||
| // Create a hidden link and click it to trigger the download | ||
| const link = document.createElement('a'); | ||
| link.href = `${BACKEND_API_URL}/api/contributors/export?${params.toString()}`; | ||
| link.href = `/api/contributors/export?${params.toString()}`; | ||
| link.download = 'contributors.csv'; | ||
| document.body.appendChild(link); | ||
| link.click(); |
There was a problem hiding this comment.
🦩 🟠 downloadContributors URL construction double-slash bug when BACKEND_API_URL ends with '/'
Changed link.href = \${BACKEND_API_URL}/api/contributors/export?${params.toString()}`tolink.href = `/api/contributors/export?${params.toString()}`indownloadContributors. This uses a root-relative path (same approach as all axios calls in the file) and avoids the double-slash protocol-relative URL bug when BACKEND_API_URLis'/'. Since axios.defaults.baseURLis already set toBACKEND_API_URL`, the browser will resolve the relative path correctly against the current origin in all deployment configurations where the frontend is served from the same host as the API proxy.
🤖 Prompt for AI agents
In frontend/src/services/api.ts around line 57, review and complete this code-review fix: downloadContributors URL construction double-slash bug when BACKEND_API_URL ends with '/'.
What the draft fix changed: Changed `link.href = \`${BACKEND_API_URL}/api/contributors/export?${params.toString()}\`` to `link.href = \`/api/contributors/export?${params.toString()}\`` in `downloadContributors`. This uses a root-relative path (same approach as all axios calls in the file) and avoids the double-slash protocol-relative URL bug when `BACKEND_API_URL` is `'/'`. Since `axios.defaults.baseURL` is already set to `BACKEND_API_URL`, the browser will resolve the relative path correctly against the current origin in all deployment configurations where the frontend is served from the same host as the API proxy.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| throw new Error(response.data.message); | ||
| } | ||
| return response.data.data; | ||
| }; // Force rebuild Sun Aug 31 19:49:51 EDT 2025 | ||
| }; |
There was a problem hiding this comment.
🦩 🔵 frontend/src/services/api.ts has a trailing comment '// Force rebuild Sun Aug 31 19:49:51 EDT 2025' that should not be committed
Removed the trailing // Force rebuild Sun Aug 31 19:49:51 EDT 2025 comment from the end of the getJobOpenings function closing };. The semicolon and closing brace are preserved; only the inline comment was deleted.
🤖 Prompt for AI agents
In frontend/src/services/api.ts around line 196, review and complete this code-review fix: frontend/src/services/api.ts has a trailing comment '// Force rebuild Sun Aug 31 19:49:51 EDT 2025' that should not be committed.
What the draft fix changed: Removed the trailing `// Force rebuild Sun Aug 31 19:49:51 EDT 2025` comment from the end of the `getJobOpenings` function closing `};`. The semicolon and closing brace are preserved; only the inline comment was deleted.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 99 high — react 👍/👎 to teach the reviewer
Closes 5 review findings across 2 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
frontend/src/services/enhanced.ts:6frontend/src/services/enhanced.ts:18frontend/src/services/api.ts:6frontend/src/services/api.ts:57frontend/src/services/api.ts:196What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.
Run: https://product-hub.flamingo.so/admin/code-review
Run id:
8f1c6ef6-6b61-4dcd-bb0e-59bc6a7d37e8Merging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.