Conversation
… by using optional chaining, preventing potential errors when accessing results.
…ays for undefined results, preventing potential runtime errors during entity type data processing.
WalkthroughAdds defensive fallbacks in the reports service to avoid reading properties from possibly undefined query results by using Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/services/reports.js (3)
462-467: Silent error swallowing — consider logging when the entity type lookup fails.The
|| []fallback correctly prevents the runtime crash, but ifgetEntityTypeWithEntitiesBasedOnOrgreturns{ success: false, message }, that failure is silently ignored. This can mask real issues (e.g., misconfigured orgs, DB connectivity problems) and make debugging difficult.For consistency with
getFilterList(line 92), consider checking.successexplicitly and logging a warning on failure:Suggested improvement
- // Fallback to empty array if result is undefined (when getEntityTypeWithEntitiesBasedOnOrg returns {success: false, message} on error) const transformedEntityData = await utils.mapEntityTypeToData( result, - entityTypesDataWithPagination.result || [] + entityTypesDataWithPagination.success ? entityTypesDataWithPagination.result : (() => { + console.warn('Entity type lookup failed for report data:', entityTypesDataWithPagination.message) + return [] + })() )Or more readably, extract the guard before the call:
+ if (!entityTypesDataWithPagination.success) { + console.warn('Entity type lookup failed:', entityTypesDataWithPagination.message) + } + const entityTypes = entityTypesDataWithPagination.result || [] const transformedEntityData = await utils.mapEntityTypeToData( result, - entityTypesDataWithPagination.result || [] + entityTypes )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/reports.js` around lines 462 - 467, The current code silently falls back to an empty array when getEntityTypeWithEntitiesBasedOnOrg fails, masking errors; update the logic around the entityTypesDataWithPagination result (the output of getEntityTypeWithEntitiesBasedOnOrg) to explicitly check entityTypesDataWithPagination.success and, on false, log a warning/error (similar to getFilterList) including the returned message or error, then only pass entityTypesDataWithPagination.result to utils.mapEntityTypeToData when success is true (otherwise pass an empty array), so you both avoid the crash and surface the underlying failure for debugging.
494-500: Same silent failure pattern — add a warning log here too.Same concern as above. When
entityTypeFiltersreturns a failure, the.reduceover an empty array produces{}, which meansreportDataResult.filterswill silently have missing entity-type filter options. Users would see empty filter dropdowns with no indication of why.Suggested improvement
- // FIX: Mentee reports have isEntityType columns (categories, recommended_for) which triggers direct DB query path. - // If query fails, getEntityTypeWithEntitiesBasedOnOrg returns {success: false, message} without 'result' property. - // Fallback to empty array prevents "Cannot read properties of undefined (reading 'reduce')" error. - const filtersEntity = (entityTypeFilters.result || []).reduce((acc, item) => { + if (!entityTypeFilters.success) { + console.warn('Entity type filter lookup failed:', entityTypeFilters.message) + } + const filtersEntity = (entityTypeFilters.result || []).reduce((acc, item) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/reports.js` around lines 494 - 500, When computing filtersEntity from entityTypeFilters.result, add a warning log when the call to getEntityTypeWithEntitiesBasedOnOrg failed or returned no result so the silent fallback to an empty object is visible; check if entityTypeFilters.success === false or !entityTypeFilters.result and then call the module's logger.warn (or existing logger) including entityTypeFilters.message and/or the entityTypeFilters object before using (entityTypeFilters.result || []) to build filtersEntity so operators see why filter options are missing.
528-532: Consistent with the other two sites — same logging suggestion applies.The defensive fallback is correct. Same recommendation: log a warning when
entityTypesData.successis false so CSV exports with missing entity resolution are diagnosable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/reports.js` around lines 528 - 532, Before calling utils.mapEntityTypeToData, check entityTypesData.success and if it's false emit a warning (using the existing logger in this module, e.g., processLogger or logger) indicating entity type resolution failed and that entityTypesData.result will be used as a fallback; then proceed to call utils.mapEntityTypeToData with entityTypesData.result and assign to transformedData as before. Ensure the warning includes enough context (e.g., mention "entityTypesData.success false" and any available error/details from entityTypesData) so CSV export issues are diagnosable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/services/reports.js`:
- Around line 462-467: The current code silently falls back to an empty array
when getEntityTypeWithEntitiesBasedOnOrg fails, masking errors; update the logic
around the entityTypesDataWithPagination result (the output of
getEntityTypeWithEntitiesBasedOnOrg) to explicitly check
entityTypesDataWithPagination.success and, on false, log a warning/error
(similar to getFilterList) including the returned message or error, then only
pass entityTypesDataWithPagination.result to utils.mapEntityTypeToData when
success is true (otherwise pass an empty array), so you both avoid the crash and
surface the underlying failure for debugging.
- Around line 494-500: When computing filtersEntity from
entityTypeFilters.result, add a warning log when the call to
getEntityTypeWithEntitiesBasedOnOrg failed or returned no result so the silent
fallback to an empty object is visible; check if entityTypeFilters.success ===
false or !entityTypeFilters.result and then call the module's logger.warn (or
existing logger) including entityTypeFilters.message and/or the
entityTypeFilters object before using (entityTypeFilters.result || []) to build
filtersEntity so operators see why filter options are missing.
- Around line 528-532: Before calling utils.mapEntityTypeToData, check
entityTypesData.success and if it's false emit a warning (using the existing
logger in this module, e.g., processLogger or logger) indicating entity type
resolution failed and that entityTypesData.result will be used as a fallback;
then proceed to call utils.mapEntityTypeToData with entityTypesData.result and
assign to transformedData as before. Ensure the warning includes enough context
(e.g., mention "entityTypesData.success false" and any available error/details
from entityTypesData) so CSV export issues are diagnosable.
…y and maintainability, focusing on clarity in entity type data processing.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/services/reports.js (3)
286-292:⚠️ Potential issue | 🟠 Major
replacements.sort_typeisundefinedin the BAR_CHART path — injects"undefined"literal into SQLThe
replacementsobject built for the BAR_CHART branch (lines 279–286) has nosort_typekey. Whenquery.replace(/:sort_type/g, replacements.sort_type)is called on line 288, JavaScript coercesundefinedto the string"undefined", which gets substituted verbatim into the query template if it contains:sort_type. This produces a malformed SQL clause.🐛 Proposed fix
const replacements = { userId: userId || null, session_type: sessionType ? utils.convertToTitleCase(sessionType) : null, start_date: dateRange.start_date || null, end_date: dateRange.end_date || null, tenantCode: tenantCode, } - let query = reportQuery[0].query.replace(/:sort_type/g, replacements.sort_type) + let query = reportQuery[0].query.replace(/:sort_type/g, '')If BAR_CHART queries never need a
sort_type, strip the placeholder entirely. If they do, addsort_typeto thereplacementsobject with a safe default (e.g.'ASC').🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/reports.js` around lines 286 - 292, The BAR_CHART branch builds a replacements object that lacks sort_type so the subsequent call to query.replace(/:sort_type/g, replacements.sort_type) injects the literal "undefined" into SQL; update the BAR_CHART replacements creation (where replacements is assembled for reportQuery and BAR_CHART) to either add a safe default like replacements.sort_type = 'ASC' or conditionally strip the :sort_type placeholder from query when BAR_CHART does not use sorting (i.e., detect BAR_CHART path and replace the token with ''), ensuring you touch the same reportQuery/replacements logic that feeds into the query.replace call.
340-350:⚠️ Potential issue | 🟠 Major
sortType.toUpperCase()throws ifsortTypeis null/undefined
sortTypehas no default in the function signature, so it may arrive asundefined. When it does,.toUpperCase()throws aTypeErrorbefore|| 'ASC'can kick in — the fallback only applies to a falsy string result, not to a null/undefined receiver. The same faulty pattern appears on both line 340 and line 349.🐛 Proposed fix
- sort_type: sortType.toUpperCase() || 'ASC', + sort_type: (sortType || 'ASC').toUpperCase(),Apply to both line 340 and line 349.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/reports.js` around lines 340 - 350, The code calls sortType.toUpperCase() inside the objects (see sort_type assignments in the main replacements and noPaginationReplacements) which throws if sortType is null/undefined; change both occurrences to guard first by converting/falling back before uppercasing, e.g. use (sortType || 'ASC').toUpperCase() or String(sortType || 'ASC').toUpperCase() so the fallback is applied before .toUpperCase(); update the sort_type fields in both replacements and noPaginationReplacements accordingly.
213-255:⚠️ Potential issue | 🟠 MajorFallback queries are identical to the primary queries — dead code
Both the
reportConfigfallback (lines 223–231) and thereportQueryfallback (lines 247–254) are copy-pastes of their respective primary queries with no parameter change. The comments indicate the intent was to fall back to the default org, but the else-branches still includeorgCodeinorganization_codeand the same[tenantCode, defaults.tenantCode]tenant filter. If the first call returns an empty result, the second will too — the fallback is effectively dead.🐛 Proposed fix — use only default-org parameters in the fallback branches
if (reportConfigWithOrgId.length > 0) { reportConfig = reportConfigWithOrgId } else { const reportConfigWithDefaultOrgId = await reportsQueries.findReport( { code: reportCode, - organization_code: { [Op.in]: [orgCode, defaults.orgCode] }, + organization_code: defaults.orgCode, }, - { [Op.in]: [tenantCode, defaults.tenantCode] } + defaults.tenantCode ) reportConfig = reportConfigWithDefaultOrgId }Apply the same correction to the
reportQueryfallback (lines 247–254).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/reports.js` around lines 213 - 255, The fallback branches for reportsQueries.findReport (reportConfigWithDefaultOrgId) and reportQueryQueries.findReportQueries (reportQueryWithDefaultOrgId) are identical to the primary calls so the fallback never changes the scope; update those fallback calls so they query only the default organization and default tenant: use organization_code set to only defaults.orgCode (e.g. { organization_code: { [Op.in]: [defaults.orgCode] } }) and the tenant param to only defaults.tenantCode (instead of including orgCode/tenantCode), keeping reportCode, reportConfigWithDefaultOrgId and reportQueryWithDefaultOrgId as the result variables.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/reports.js`:
- Around line 463-466: The code is accessing .result directly on the return
value of getEntityTypeWithEntitiesBasedOnOrg (used before calling
utils.mapEntityTypeToData) which can throw if that call returns null/undefined;
update the three spots in getReportData where you do something like
entityTypesDataWithPagination.result to use optional chaining on the parent
(e.g., entityTypesDataWithPagination?.result) so the || [] fallback actually
works; locate references to getEntityTypeWithEntitiesBasedOnOrg and
mapEntityTypeToData in getReportData and replace direct .result accesses with
?.result in all three changed locations (around the lines that call
utils.mapEntityTypeToData).
---
Outside diff comments:
In `@src/services/reports.js`:
- Around line 286-292: The BAR_CHART branch builds a replacements object that
lacks sort_type so the subsequent call to query.replace(/:sort_type/g,
replacements.sort_type) injects the literal "undefined" into SQL; update the
BAR_CHART replacements creation (where replacements is assembled for reportQuery
and BAR_CHART) to either add a safe default like replacements.sort_type = 'ASC'
or conditionally strip the :sort_type placeholder from query when BAR_CHART does
not use sorting (i.e., detect BAR_CHART path and replace the token with ''),
ensuring you touch the same reportQuery/replacements logic that feeds into the
query.replace call.
- Around line 340-350: The code calls sortType.toUpperCase() inside the objects
(see sort_type assignments in the main replacements and
noPaginationReplacements) which throws if sortType is null/undefined; change
both occurrences to guard first by converting/falling back before uppercasing,
e.g. use (sortType || 'ASC').toUpperCase() or String(sortType ||
'ASC').toUpperCase() so the fallback is applied before .toUpperCase(); update
the sort_type fields in both replacements and noPaginationReplacements
accordingly.
- Around line 213-255: The fallback branches for reportsQueries.findReport
(reportConfigWithDefaultOrgId) and reportQueryQueries.findReportQueries
(reportQueryWithDefaultOrgId) are identical to the primary calls so the fallback
never changes the scope; update those fallback calls so they query only the
default organization and default tenant: use organization_code set to only
defaults.orgCode (e.g. { organization_code: { [Op.in]: [defaults.orgCode] } })
and the tenant param to only defaults.tenantCode (instead of including
orgCode/tenantCode), keeping reportCode, reportConfigWithDefaultOrgId and
reportQueryWithDefaultOrgId as the result variables.
| const transformedEntityData = await utils.mapEntityTypeToData( | ||
| result, | ||
| entityTypesDataWithPagination.result | ||
| entityTypesDataWithPagination.result || [] | ||
| ) |
There was a problem hiding this comment.
Defensive fallback is still incomplete — use optional chaining on the parent
In all three changed locations, .result is accessed directly on the return value of getEntityTypeWithEntitiesBasedOnOrg before the || [] guard can apply. If that call ever returns null or undefined, a TypeError is thrown before the fallback activates. The getFilterList method (line 92) consistently checks .success && .result before using the result; getReportData should be equally cautious.
🛡️ Proposed fix using optional chaining
- entityTypesDataWithPagination.result || []
+ entityTypesDataWithPagination?.result || []- (entityTypeFilters.result || []).reduce((acc, item) => {
+ (entityTypeFilters?.result || []).reduce((acc, item) => {- entityTypesData.result || []
+ entityTypesData?.result || []Also applies to: 493-496, 523-526
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/reports.js` around lines 463 - 466, The code is accessing
.result directly on the return value of getEntityTypeWithEntitiesBasedOnOrg
(used before calling utils.mapEntityTypeToData) which can throw if that call
returns null/undefined; update the three spots in getReportData where you do
something like entityTypesDataWithPagination.result to use optional chaining on
the parent (e.g., entityTypesDataWithPagination?.result) so the || [] fallback
actually works; locate references to getEntityTypeWithEntitiesBasedOnOrg and
mapEntityTypeToData in getReportData and replace direct .result accesses with
?.result in all three changed locations (around the lines that call
utils.mapEntityTypeToData).
2 participants
Release Notes
|| []) in map/reduce paths used for report generation and CSV exportContribution Summary