Fix data explorer queries for single tree registrations #2273
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
mohitb35
changed the title
mohitb35
force-pushed
the
hotfix/data-explorer-queries
branch
3 times, most recently
from
764c275 to
ee616fa
Compare
WalkthroughThe pull request includes significant modifications across several API endpoints related to data retrieval for plant and species information. Key changes involve restructuring SQL queries to improve readability and robustness, particularly through the use of Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
- excludes deleted data and interventions that are not single or multi tree registrations
- also excludes deleted interventions from results - restricts results to single and multi tree registrations
- also excludes deleted interventions from results - restricts results to single and multi tree registrations - Handles `null` value for name by grouping with 'Unknown'
mohitb35
force-pushed
the
hotfix/data-explorer-queries
branch
from
421fa8b to
ea14a67
Compare
- also excludes deleted interventions from results - restricts results to single and multi tree registrations
mohitb35
force-pushed
the
hotfix/data-explorer-queries
branch
from
f711f2b to
7227467
Compare
…istrations - also excludes deleted interventions from results - restricts results to single and multi tree registrations
- updates API to return correct data for single tree registration - displays only relevant data for single tree registration - add header to display selected plant location hid
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (15)
pages/api/data-explorer/export.ts (1)
Line range hint
15-57: Improve error handling and connection management.The current implementation has several issues:
- Redundant database connection termination
- Missing error response to the client
- No validation of request body parameters
Consider applying these improvements:
handler.post(async (req, response) => { const { projectId, startDate, endDate } = req.body; + + // Validate required parameters + if (!projectId || !startDate || !endDate) { + return response.status(400).json({ + error: 'Missing required parameters', + }); + } + try { const query = `...`; const res = await db.query<IExportData[]>(query, [ projectId, startDate, `${endDate} 23:59:59.999`, ]); - await db.end(); response.status(200).json({ data: res }); } catch (err) { console.error('Error fetching export data:', err); + response.status(500).json({ + error: 'Failed to fetch export data', + }); } finally { await db.quit(); } });pages/api/data-explorer/total-trees-planted.ts (1)
Line range hint
66-68: Add proper error response handling.The catch block only logs the error without sending an error response to the client. This could lead to hanging requests as no response is sent in error scenarios.
Apply this fix:
} catch (err) { console.error('Error fetching total trees planted:', err); + response.status(500).json({ error: 'Failed to fetch total trees planted' }); } finally {src/features/common/types/dataExplorer.d.ts (1)
133-137: LGTM! Improved null safety for optional data.The nullable fields appropriately handle cases where sample plant locations might not be available, which is common for single tree registrations.
Consider adding JSDoc comments to document when these fields might be null, helping other developers understand the expected behavior. For example:
/** * Details about a plant location including its properties and samples * @property samplePlantLocations - Array of sample locations. Null for single tree registrations * @property totalSamplePlantLocations - Count of sample locations. Null for single tree registrations */src/features/user/TreeMapper/Analytics/components/Map/components/PlantLocationDetails/index.module.scss (2)
73-84: LGTM! Consider accessibility improvements.The new header section is well-structured and maintains consistency with the existing design system. The implementation properly handles the layout requirements for displaying the HID.
Consider these accessibility and maintainability improvements:
.header { width: 100%; padding: 0 5px; - font-size: 12px; + font-size: 0.75rem; display: flex; justify-content: space-between; margin-bottom: -8px; .hid { - font-weight: 500; + font-weight: var(--font-weight-medium, 500); + // Ensure sufficient color contrast for accessibility + color: var(--text-color-primary, #000); } }This refactor:
- Uses relative units (rem) for better accessibility when users modify their browser's font size
- Introduces CSS variables for better maintainability and theme support
- Ensures proper color contrast for accessibility
Line range hint
28-39: Consider improving responsive design and accessibility.The current implementation uses fixed dimensions and absolute positioning which might cause issues on different screen sizes. Additionally, hiding scrollbars can impact accessibility.
Consider these improvements:
- Replace fixed dimensions with responsive units:
.plantLocationDetailsContainer { position: absolute; top: 15px; left: 20px; z-index: 2; - width: 300px; + width: min(300px, 90vw); - height: 450px; + max-height: 80vh; border-radius: 16px; background-color: #fff; font-size: 10px; - ::-webkit-scrollbar { - display: none; - } + // Allow native scrollbar for better accessibility + overflow-y: auto; }
- Consider using CSS Grid or Flexbox for a more flexible layout that adapts to different screen sizes.
- Add media queries for better mobile support.
pages/api/data-explorer/trees-planted.ts (2)
Line range hint
144-157: Improve error handling in the API handler.The current error handling only logs the error but doesn't send an appropriate response to the client. Consider:
- Sending proper error responses with status codes
- Adding different handling for various error types
- Including more detailed error information in logs
} catch (err) { console.error('Error fetching trees planted:', err); + const status = err.code === 'ER_NO_SUCH_TABLE' ? 404 : 500; + const message = process.env.NODE_ENV === 'development' + ? err.message + : 'An error occurred while fetching data'; + response.status(status).json({ error: message }); } finally { await db.quit(); }
Line range hint
17-52: Enhance cache handling robustness.While the caching implementation is good, consider:
- Adding a cache invalidation strategy when data is updated
- Handling Redis connection errors gracefully
- Adding cache versioning for easier invalidation
+const CACHE_VERSION = '1'; const CACHE_KEY = `${cacheKeyPrefix}_TREES_PLANTED__${getCachedKey( + CACHE_VERSION, projectId, startDate, endDate, timeFrame as string )}`; -const cacheHit = await redisClient.get(CACHE_KEY); +let cacheHit; +try { + cacheHit = await redisClient.get(CACHE_KEY); +} catch (err) { + console.warn('Redis cache error:', err); + // Continue without cache on Redis errors +}pages/api/data-explorer/map/distinct-species/[projectId].ts (2)
Line range hint
36-39: Fix: Parse cached data before sending responseThe
cachedDistinctSpeciesretrieved from Redis is a JSON string. Before returning it in the response, it should be parsed into an array to maintain consistency with the uncached response.Apply this diff to parse the cached data:
if (cachedDistinctSpecies) { - return response.status(200).json({ data: cachedDistinctSpecies }); + return response.status(200).json({ data: JSON.parse(cachedDistinctSpecies) }); }
Line range hint
70-71: Ensure error response is sent to client in the catch blockIn the
catchblock, the error is logged, but no response is sent to the client. This can cause the client to hang or timeout. Consider sending an appropriate error response to inform the client about the failure.Apply this diff to send an error response:
} catch (err) { console.error(`Error fetching distinct species for ${projectId}`, err); + response.status(500).json({ error: 'Internal server error' }); } finally { db.quit(); }pages/api/data-explorer/total-species-planted.ts (1)
Line range hint
24-27: Parse cached data before returning in the responseWhen retrieving the cached data,
cacheHitis a JSON string. Returning it directly in the response will send the data as a string rather than as an object. Consider parsingcacheHitbefore returning it to ensure the client receives the expected data format.Apply this diff to fix the issue:
if (cacheHit) { - response.status(200).json({ data: cacheHit }); + response.status(200).json({ data: JSON.parse(cacheHit) }); return; }pages/api/data-explorer/map/plant-location/index.ts (5)
Line range hint
42-43: Avoid Premature Closing of Database ConnectionThe call to
await db.end();before sending the response may lead to issues if the response processing requires an active database connection.Consider removing
await db.end();here and ensure the database connection is properly closed in thefinallyblock.Apply this diff:
- await db.end(); response.setHeader( 'Cache-Control', 's-maxage=7200, stale-while-revalidate' );
Line range hint
59-61: Use Consistent Method for Closing Database ConnectionIn the
finallyblock,db.quit();is used to close the database connection. However, earlier in the code,db.end();is used. For consistency and to prevent unexpected behavior, use the same method throughout.Replace
db.quit();withawait db.end();in thefinallyblock.Apply this diff:
} catch (err) { console.error('Error fetching plant location data:', err); response.status(500).json({ error: 'Internal Server Error' }); } finally { - db.quit(); + await db.end(); } });
Line range hint
46-50: Set Proper Cache-Control Headers Only on Successful ResponsesThe
Cache-Controlheader is being set regardless of whether the data retrieval is successful or not. If an error occurs, the response should not be cached.Move the
setHeadercall to occur only when the data retrieval is successful.Apply this diff:
const plantLocations: SinglePlantLocationApiResponse[] = qRes.map( (plantLocation) => ({ type: 'Feature', properties: { guid: plantLocation.guid, treeCount: plantLocation.treeCount, }, geometry: JSON.parse(plantLocation.geometry) as Geometry, }) ); - await db.end(); response.setHeader( 'Cache-Control', 's-maxage=7200, stale-while-revalidate' ); + await db.end();
Line range hint
52-54: Enhance Error Logging for Better DebuggingThe current error logging simply outputs a generic message along with the error. Including more context, such as input parameters, can aid in debugging.
Consider enhancing the error log:
} catch (err) { - console.error('Error fetching plant location data:', err); + console.error('Error fetching plant location data for projectId:', projectId, 'with error:', err); response.status(500).json({ error: 'Internal Server Error' }); } finally {
Line range hint
22-50: Consider Using a Query Builder for Improved MaintainabilityBuilding SQL queries using string concatenation can be error-prone and hard to maintain, especially as the complexity grows.
Consider using a query builder like Knex.js to construct SQL queries more safely and readably.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
public/static/locales/en/treemapperAnalytics.jsonis excluded by!**/*.json
📒 Files selected for processing (12)
- pages/api/data-explorer/export.ts (1 hunks)
- pages/api/data-explorer/map/distinct-species/[projectId].ts (1 hunks)
- pages/api/data-explorer/map/plant-location/[plantLocationId].ts (3 hunks)
- pages/api/data-explorer/map/plant-location/index.ts (1 hunks)
- pages/api/data-explorer/map/sites/[projectId].ts (1 hunks)
- pages/api/data-explorer/species-planted.ts (1 hunks)
- pages/api/data-explorer/total-species-planted.ts (1 hunks)
- pages/api/data-explorer/total-trees-planted.ts (1 hunks)
- pages/api/data-explorer/trees-planted.ts (1 hunks)
- src/features/common/types/dataExplorer.d.ts (1 hunks)
- src/features/user/TreeMapper/Analytics/components/Map/components/PlantLocationDetails/index.module.scss (1 hunks)
- src/features/user/TreeMapper/Analytics/components/Map/components/PlantLocationDetails/index.tsx (3 hunks)
✅ Files skipped from review due to trivial changes (1)
- pages/api/data-explorer/map/sites/[projectId].ts
🔇 Additional comments (15)
pages/api/data-explorer/export.ts (1)
18-43: Consider performance optimizations for the query.The query involves multiple joins and filtering conditions which might impact performance for large datasets. Additionally, there's no pagination which could lead to memory issues with large result sets.
Recommendations:
- Add pagination using LIMIT and OFFSET
- Consider adding indexes on:
intervention.intervention_start_dateintervention.typeintervention.plant_project_idproject.guid- Use EXPLAIN ANALYZE to verify query performance
Let's check if appropriate indexes exist:
src/features/common/types/dataExplorer.d.ts (1)
127-130: LGTM! Well-defined type-safe interface.The
PlantLocationPropertiesinterface correctly defines the structure for plant location types and unique identifiers.Let's verify the usage of this interface across the codebase:
✅ Verification successful
Based on the search results, I can now generate the final response as we have comprehensive evidence of the interface usage across the codebase.
Type definition is correctly implemented and consistently used
The
PlantLocationPropertiesinterface's registration types are consistently used throughout the codebase:
- Frontend components properly handle both registration types with type guards
- API endpoints correctly filter by both registration types
- Type-safe discriminated unions are properly implemented in related files (e.g.,
plantLocation.d.ts)- No instances of typos or inconsistent string literals were found
Reasoning: The search results show that both
'single-tree-registration'and'multi-tree-registration'are used consistently across the codebase, with proper type checking in components, correct database queries in API endpoints, and consistent type definitions in related interfaces. The implementation aligns perfectly with the type definition, and there are no deviations from the specified literals.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistent usage of registration types # Test: Search for hardcoded registration type strings to ensure they match the interface rg -g '*.{ts,tsx}' "'single-tree-registration'|'multi-tree-registration'"Length of output: 6955
pages/api/data-explorer/trees-planted.ts (1)
72-89: Verify week calculations across different MySQL versions.The
WEEK()function behavior can vary based on MySQL version and server settings. Ensure that the week calculations produce consistent results across all environments.src/features/user/TreeMapper/Analytics/components/Map/components/PlantLocationDetails/index.tsx (2)
6-6: LGTM: Import addition is appropriateThe PlantLocationProperties type import aligns with the new component requirements.
138-148:⚠️ Potential issueAdd validation for hid formatting
The hid formatting assumes the string is always longer than 3 characters. Consider adding validation to handle edge cases:
Consider this safer implementation:
- const formattedHid = hid.substring(0, 3) + '-' + hid.substring(3); + const formattedHid = hid.length > 3 + ? hid.substring(0, 3) + '-' + hid.substring(3) + : hid;pages/api/data-explorer/map/distinct-species/[projectId].ts (1)
47-47: Verify the order ofCOALESCEargumentsEnsure that the order of the arguments in the
COALESCEfunction reflects the intended priority for species names. Currently, it checksss.name, thenps.other_species, theniv.other_species, and finally defaults to'Unknown'. Confirm that this order aligns with the desired business logic.pages/api/data-explorer/total-species-planted.ts (3)
44-60: LGTMThe SQL query modifications align with the PR objectives and correctly handle the necessary filters for deleted interventions and specific intervention types.
44-60:⚠️ Potential issueEnsure error responses are sent to the client
Currently, if an error occurs during query execution, the server logs the error but does not send any response to the client. This could result in the client hanging or not receiving proper feedback. Consider sending an appropriate error response in the
catchblock.Apply this diff to send an error response:
} catch (err) { console.error('Error fetching total species planted:', err); + response.status(500).json({ error: 'Internal Server Error' }); } finally { await db.quit(); }Likely invalid or redundant comment.
59-59: Confirm date handling forendDateparameterAppending
'23:59:59.999'toendDateassumes thatendDatedoes not already include a time component. IfendDateincludes a time, this could lead to incorrect query results. Verify thatendDateis always provided without a time component.Run the following script to check the usage of
endDatein the codebase:✅ Verification successful
Let me try a different approach to verify the date handling by searching for all usages of
endDatewith more context.This will help us understand:
- How endDate is being used across the codebase
- Any type definitions that might constrain its format
- The context around where it's being set or processed
Based on the search results, I can now provide a conclusive response about the date handling:
Appending time to
endDateis correct and consistent with codebase usageThe codebase consistently handles
endDateas a date-only parameter and explicitly appends the time component23:59:59.999when querying the database. This is evidenced by:
- The
getCachedKeyutility formats dates using only 'MM-dd-yyyy' format- Multiple API endpoints (
trees-planted.ts,total-trees-planted.ts,species-planted.ts,export.ts) all follow the same pattern of appending23:59:59.999toendDate- Frontend components pass
endDatewithout time components- Type definitions and interfaces show
endDatebeing handled as a string or Date object without time specificationsThe current implementation is correct and follows the established pattern across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of `endDate` to confirm it doesn't include a time component. # Test: Find all assignments to `endDate`. Expect: `endDate` is provided without time. rg "endDate\s*=\s*['\"]\d{4}-\d{2}-\d{2}['\"]"Length of output: 52
Script:
#!/bin/bash # Search for all usages of endDate with surrounding context rg -A 3 -B 3 "endDate" # Also check for date-related type definitions or interfaces ast-grep --pattern 'interface $_ { $$$ endDate $$$ }' ast-grep --pattern 'type $_ = { $$$ endDate $$$ }'Length of output: 16118
pages/api/data-explorer/species-planted.ts (1)
51-51: Confirm the aggregation logic for tree countsThe aggregation uses
SUM(COALESCE(ps.tree_count, iv.trees_planted, 0))to calculatetotal_tree_count. Verify that this logic accurately sums the tree counts, especially in scenarios where bothps.tree_countandiv.trees_plantedareNULL. This ensures the total reflects the correct number of trees planted.To verify, check for records where both
ps.tree_countandiv.trees_plantedmight beNULLand ensure the sum correctly accounts for such cases.pages/api/data-explorer/map/plant-location/index.ts (2)
34-35: Ensure Consistency in Intervention Type FiltersThe
WHEREclause filtersiv.typeto include only'multi-tree-registration'and'single-tree-registration'. Verify that these are the correct types to include and that no other relevant types are excluded.Run the following script to list all distinct intervention types:
#!/bin/bash # Description: List all distinct intervention types in the database. # Test: Retrieve and display all unique intervention types. # Expected: Ensure that only the desired types are included or adjust the query accordingly. sqlite3 your_database.db "SELECT DISTINCT type FROM intervention;"
27-30: Clarify COALESCE Logic for Species Name RetrievalThe current
COALESCEfunction in the SELECT statement may lead to unexpected species names due to the order of evaluation. Specifically, it prioritizesss.name, thenps.other_species, theniv.other_species, before defaulting to'Unknown'. Given thativ.scientific_species_idandps.scientific_species_idare both used in the JOIN withscientific_species ss, it's possible thatss.namecould beNULLeven ifiv.other_speciesorps.other_speciescontain valid data.Please ensure that the species names are retrieved as intended. You can verify the
COALESCElogic with the following script:✅ Verification successful
Let me analyze the SQL query structure and related files to verify the concern.
The goal is to:
- Find similar COALESCE patterns to understand if this is a common pattern
- Locate schema definitions to understand the data model
- Find related species handling code to verify the logic
This will help us understand if the COALESCE order is intentional and follows the established pattern in the codebase.
Let me analyze the schema to better understand the data model relationships.
This will help us understand:
- The data model relationships between intervention, planted_species, and scientific_species
- Any constraints or documentation about how species data should be handled
- Whether the COALESCE order aligns with the data model design
Based on the analysis of the codebase, I can now provide a final response:
COALESCE Logic for Species Name is Consistently Implemented
The COALESCE pattern
COALESCE(ss.name, ps.other_species, iv.other_species, 'Unknown')is consistently used across multiple files in the codebase:
- pages/api/data-explorer/species-planted.ts
- pages/api/data-explorer/map/distinct-species/[projectId].ts
- pages/api/data-explorer/map/plant-location/index.ts
- pages/api/data-explorer/export.ts
The order of precedence is intentional and follows a clear pattern:
- Scientific species name (
ss.name) from the scientific_species table- Other species from planted_species (
ps.other_species)- Other species from intervention (
iv.other_species)- 'Unknown' as fallback
This hierarchy makes sense because:
- It prioritizes standardized scientific names over custom entries
- When scientific names aren't available, it uses species information from more specific (planted_species) to more general (intervention) sources
- The 'Unknown' fallback ensures no null values are returned
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the COALESCE order for species names. # Test: Check if the COALESCE function retrieves the correct species names. # Expected: The species name should reflect the correct priority. # Sample query to test the COALESCE logic cat <<EOF | sqlite3 your_database.db SELECT COALESCE(ss.name, ps.other_species, iv.other_species, 'Unknown') AS name, ss.name AS scientific_name, ps.other_species AS planted_species_other, iv.other_species AS intervention_other_species FROM intervention iv LEFT JOIN planted_species ps ON iv.id = ps.intervention_id LEFT JOIN scientific_species ss ON COALESCE(iv.scientific_species_id, ps.scientific_species_id) = ss.id WHERE iv.id = 'sample_intervention_id'; EOFLength of output: 362
Script:
#!/bin/bash # Let's check for any similar COALESCE patterns in the codebase # and understand the data model better # Search for similar COALESCE patterns rg -l "COALESCE.*species" --type sql --type ts # Look for table definitions or schema files fd -e sql -e prisma schema # Search for related species handling rg "scientific_species|planted_species" -A 3 -B 3Length of output: 9134
Script:
#!/bin/bash # Let's examine the Prisma schema to understand the relationships cat prisma/schema.prisma # Also check if there are any comments or documentation about the species handling rg -B 2 -A 2 "species.*id|other_species" prisma/schema.prismaLength of output: 4270
pages/api/data-explorer/map/plant-location/[plantLocationId].ts (3)
29-29: Verify the precedence in theCOALESCEfunction fortreeCountThe
COALESCEfunction on line 29 prioritizesps.tree_countoveriv.trees_planted. Please confirm thatps.tree_countshould indeed take precedence. This ensures that whenps.tree_countis available, it is used; otherwise, it falls back toiv.trees_planted.
90-90: Ensure the number of placeholders matches the parametersAn additional
plantLocationIdparameter is added on line 90. Verify that the number of?placeholders in your SQL query matches the number of parameters provided. There should be five placeholders to correspond with the fiveplantLocationIdentries.
69-69:⚠️ Potential issueChanging
LEFT JOINtoINNER JOINmay omit records without samplesLines 69 and 77 change the join from
LEFT JOINtoINNER JOINwithintervention siv. This modification will excludeivrecords that do not have correspondingsiventries (sample interventions). If there are plant locations without samples, they will be omitted fromsamplePlantLocationsandtotalSamplePlantLocations.Apply this diff if you intend to include all plant locations, even those without samples:
- INNER JOIN intervention siv ON iv.id = siv.parent_id + LEFT JOIN intervention siv ON iv.id = siv.parent_idTo confirm the impact of this change, you can run the following script:
This script checks for interventions with the given
plantLocationIdthat have no associated samples. If any are found, they would be excluded by theINNER JOIN.Also applies to: 77-77
| pp.guid=? AND | ||
| iv.type IN ('multi-tree-registration','single-tree-registration') AND | ||
| iv.deleted_at IS NULL AND | ||
| iv.intervention_start_date BETWEEN ? AND ? |
There was a problem hiding this comment.
Ensure consistent date-time handling.
The date range filtering is inconsistent - the start date doesn't include a time component while the end date adds it during parameter binding. This could lead to unexpected results at the day boundaries.
Consider modifying the query to handle both dates consistently:
- iv.intervention_start_date BETWEEN ? AND ?
+ iv.intervention_start_date >= ?
+ AND iv.intervention_start_date < DATE_ADD(?, INTERVAL 1 DAY)Then update the parameter binding:
- `${endDate} 23:59:59.999`,
+ endDate,Committable suggestion was skipped due to low confidence.
| const query = ` | ||
| SELECT | ||
| COALESCE(SUM(iv.trees_planted), 0) AS totalTreesPlanted | ||
| FROM intervention iv | ||
| JOIN project pp ON iv.plant_project_id = pp.id | ||
| WHERE | ||
| iv.deleted_at IS NULL | ||
| AND iv.type IN ('single-tree-registration', 'multi-tree-registration') | ||
| AND pp.guid = ? | ||
| AND iv.intervention_start_date BETWEEN ? AND ?`; |
There was a problem hiding this comment.
💡 Codebase verification
Query changes need to be applied consistently across data explorer endpoints
Based on the search results, I found inconsistencies in how deleted records and intervention types are handled across data explorer endpoints:
- Some files properly filter with both conditions:
trees-planted.tsmap/distinct-species/[projectId].ts
- Missing both filters:
map/plant-location/index.tsmap/plant-location/[plantLocationId].ts
- Missing intervention type filter:
total-species-planted.tsspecies-planted.tsexport.ts
The changes in total-trees-planted.ts are correct, but similar changes should be applied to other data explorer endpoints to maintain consistency in data filtering.
🔗 Analysis chain
LGTM! Query changes align with PR objectives.
The SQL query improvements look good:
- Properly filters out deleted records
- Correctly handles single/multi tree registrations
- Uses COALESCE for null handling
- Improved readability with proper formatting
Let's verify the intervention types across the codebase:
Let me try a different approach to verify the intervention types usage across data explorer endpoints.
This should help us:
- Find all SQL queries in data explorer endpoints that use the intervention table
- Find any type definitions or constants that define these intervention types to ensure consistency
Let me try one final search with a simpler pattern to find any SQL queries in the data explorer endpoints:
This will help us understand the structure of data explorer endpoints and their SQL usage patterns.
Let's check the specific SQL queries in these intervention-related files to verify consistency:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistency of intervention type filtering across data explorer endpoints
# Expected: All data explorer endpoints should consistently filter deleted records and intervention types
rg -l "intervention.*type.*IN.*('single-tree-registration'|'multi-tree-registration')" pages/api/data-explorer/
Length of output: 125
Script:
#!/bin/bash
# Search for SQL queries in data explorer endpoints
rg -A 5 -B 5 "SELECT.*FROM.*intervention.*" pages/api/data-explorer/
# Also check for any other files that might define these intervention types
ast-grep --pattern 'type.*=.*("single-tree-registration"|"multi-tree-registration")'
Length of output: 157
Script:
#!/bin/bash
# List all files in data explorer directory first
fd . pages/api/data-explorer/
# Then search for any SQL queries with less restrictive pattern
rg -l "SELECT.*FROM" pages/api/data-explorer/
# And specifically look for intervention table usage
rg -l "intervention" pages/api/data-explorer/
Length of output: 1108
Script:
#!/bin/bash
# Check SQL queries in files that use intervention table
echo "=== trees-planted.ts ==="
rg -A 10 "SELECT" pages/api/data-explorer/trees-planted.ts
echo "=== total-species-planted.ts ==="
rg -A 10 "SELECT" pages/api/data-explorer/total-species-planted.ts
echo "=== species-planted.ts ==="
rg -A 10 "SELECT" pages/api/data-explorer/species-planted.ts
echo "=== export.ts ==="
rg -A 10 "SELECT" pages/api/data-explorer/export.ts
echo "=== map/plant-location/index.ts ==="
rg -A 10 "SELECT" pages/api/data-explorer/map/plant-location/index.ts
echo "=== map/plant-location/[plantLocationId].ts ==="
rg -A 10 "SELECT" pages/api/data-explorer/map/plant-location/\[plantLocationId\].ts
echo "=== map/distinct-species/[projectId].ts ==="
rg -A 10 "SELECT" pages/api/data-explorer/map/distinct-species/\[projectId\].ts
Length of output: 8201
| properties: PlantLocationProperties; | ||
| plantedSpecies: { | ||
| treeCount: number; | ||
| scientificName: string; | ||
| }[]; | ||
| totalPlantedTrees: number; | ||
| samplePlantLocations: { | ||
| tag: string; | ||
| guid: string; | ||
| species: string; | ||
| geometry: { | ||
| type: string; | ||
| coordinates: number[]; | ||
| }; | ||
| measurements: { | ||
| width: string; | ||
| height: string; | ||
| }; | ||
| }[]; | ||
| totalSamplePlantLocations: number; | ||
| samplePlantLocations: | ||
| | null | ||
| | { | ||
| tag: string; | ||
| guid: string; | ||
| species: string; | ||
| geometry: { | ||
| type: string; | ||
| coordinates: number[]; | ||
| }; | ||
| measurements: { | ||
| width: string; | ||
| height: string; | ||
| }; | ||
| }[]; | ||
| totalSamplePlantLocations: null | number; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting the sample location type to improve maintainability.
The nested type definition for samplePlantLocations could be extracted into a separate interface to improve code organization and reusability.
Consider refactoring like this:
interface SampleLocationResponse {
tag: string;
guid: string;
species: string;
geometry: {
type: string;
coordinates: number[];
};
measurements: {
width: string;
height: string;
};
}
export interface PlantLocationDetailsApiResponse {
res: {
properties: PlantLocationProperties;
plantedSpecies: {
treeCount: number;
scientificName: string;
}[];
totalPlantedTrees: number;
samplePlantLocations: null | SampleLocationResponse[];
totalSamplePlantLocations: null | number;
};
}| query = ` | ||
| SELECT | ||
| LEFT(MONTHNAME(iv.intervention_start_date), 3) AS month, | ||
| YEAR(iv.intervention_start_date) AS year, | ||
| SUM(iv.trees_planted) AS treesPlanted | ||
| FROM intervention iv | ||
| JOIN project pp ON iv.plant_project_id = pp.id | ||
| WHERE | ||
| iv.deleted_at IS NULL AND | ||
| iv.type IN ('single-tree-registration', 'multi-tree-registration') AND | ||
| pp.guid = ? AND | ||
| iv.intervention_start_date BETWEEN ? AND ? | ||
| GROUP BY month, year | ||
| ORDER BY iv.intervention_start_date | ||
| `; | ||
| break; | ||
|
|
||
| case TIME_FRAME.YEARS: | ||
| query = | ||
| 'SELECT \ | ||
| YEAR(iv.intervention_start_date) AS year, \ | ||
| SUM(iv.trees_planted) AS treesPlanted \ | ||
| FROM intervention iv \ | ||
| JOIN project pp ON iv.plant_project_id = pp.id \ | ||
| WHERE pp.guid = ? AND iv.intervention_start_date BETWEEN ? AND ? \ | ||
| GROUP BY year \ | ||
| ORDER BY iv.intervention_start_date'; | ||
| query = ` | ||
| SELECT | ||
| YEAR(iv.intervention_start_date) AS year, | ||
| SUM(iv.trees_planted) AS treesPlanted | ||
| FROM intervention iv | ||
| JOIN project pp ON iv.plant_project_id = pp.id | ||
| WHERE | ||
| iv.deleted_at IS NULL AND | ||
| iv.type IN ('single-tree-registration', 'multi-tree-registration') AND | ||
| pp.guid = ? AND | ||
| iv.intervention_start_date BETWEEN ? AND ? | ||
| GROUP BY year | ||
| ORDER BY iv.intervention_start_date | ||
| `; | ||
| break; | ||
|
|
||
| default: | ||
| query = | ||
| 'SELECT \ | ||
| YEAR(iv.intervention_start_date) AS year, \ | ||
| SUM(iv.trees_planted) AS treesPlanted \ | ||
| FROM intervention iv \ | ||
| JOIN project pp ON iv.plant_project_id = pp.id \ | ||
| WHERE pp.guid = ? AND iv.intervention_start_date BETWEEN ? AND ? \ | ||
| GROUP BY year \ | ||
| ORDER BY iv.intervention_start_date'; | ||
| query = ` | ||
| SELECT | ||
| YEAR(iv.intervention_start_date) AS year, | ||
| SUM(iv.trees_planted) AS treesPlanted | ||
| FROM intervention iv | ||
| JOIN project pp ON iv.plant_project_id = pp.id | ||
| WHERE | ||
| iv.deleted_at IS NULL AND | ||
| iv.type IN ('single-tree-registration', 'multi-tree-registration') AND | ||
| pp.guid = ? AND | ||
| iv.intervention_start_date BETWEEN ? AND ? | ||
| GROUP BY year | ||
| ORDER BY iv.intervention_start_date | ||
| `; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to reduce query duplication.
The SQL queries share common conditions and structures. Consider extracting these into reusable constants or a query builder function.
const COMMON_WHERE_CONDITIONS = `
iv.deleted_at IS NULL AND
iv.type IN ('single-tree-registration', 'multi-tree-registration') AND
pp.guid = ? AND
iv.intervention_start_date BETWEEN ? AND ?
`;
const buildQuery = (selectClause: string, groupByClause: string) => `
SELECT ${selectClause}
FROM intervention iv
JOIN project pp ON iv.plant_project_id = pp.id
WHERE ${COMMON_WHERE_CONDITIONS}
GROUP BY ${groupByClause}
ORDER BY iv.intervention_start_date
`;| query = ` | ||
| SELECT | ||
| iv.intervention_start_date AS plantedDate, | ||
| SUM(iv.trees_planted) AS treesPlanted | ||
| FROM intervention iv | ||
| JOIN project pp ON iv.plant_project_id = pp.id | ||
| WHERE | ||
| iv.deleted_at IS NULL AND | ||
| iv.type IN ('single-tree-registration', 'multi-tree-registration') AND | ||
| pp.guid = ? AND | ||
| iv.intervention_start_date BETWEEN ? AND ? | ||
| GROUP BY iv.intervention_start_date | ||
| ORDER BY iv.intervention_start_date | ||
| `; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding an index hint for better query performance.
The query looks correct and well-structured. However, for optimal performance, consider adding an index hint for the intervention_start_date column since it's used in both filtering and ordering.
SELECT
iv.intervention_start_date AS plantedDate,
SUM(iv.trees_planted) AS treesPlanted
-FROM intervention iv
+FROM intervention FORCE INDEX (idx_intervention_start_date) iv
JOIN project pp ON iv.plant_project_id = pp.idCommittable suggestion was skipped due to low confidence.
| const plantLocationType = plantLocationDetails?.properties.type; | ||
|
|
||
| const hasData = | ||
| plantLocationDetails !== null && | ||
| Boolean( | ||
| selectedLayer.treeCount || | ||
| selectedLayer?.density || | ||
| (plantLocationType === 'multi-tree-registration' && | ||
| (selectedLayer.treeCount || selectedLayer.density)) || | ||
| (plantLocationDetails?.plantedSpecies?.length || 0) > 0 || | ||
| (plantLocationDetails?.samplePlantLocations?.length || 0) > 0 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting complex conditions and using type constants
The hasData condition is complex and uses magic strings. Consider these improvements for better maintainability:
+ const PLANT_LOCATION_TYPES = {
+ MULTI_TREE: 'multi-tree-registration'
+ } as const;
+
+ const hasMultiTreeData = (
+ type: string,
+ treeCount?: number,
+ density?: number
+ ) => type === PLANT_LOCATION_TYPES.MULTI_TREE && (treeCount || density);
+
+ const hasSpeciesData = (species?: any[]) => (species?.length || 0) > 0;
+ const hasSampleData = (samples?: any[]) => (samples?.length || 0) > 0;
+
const hasData =
plantLocationDetails !== null &&
Boolean(
- (plantLocationType === 'multi-tree-registration' &&
- (selectedLayer.treeCount || selectedLayer.density)) ||
- (plantLocationDetails?.plantedSpecies?.length || 0) > 0 ||
- (plantLocationDetails?.samplePlantLocations?.length || 0) > 0
+ hasMultiTreeData(plantLocationType, selectedLayer.treeCount, selectedLayer.density) ||
+ hasSpeciesData(plantLocationDetails?.plantedSpecies) ||
+ hasSampleData(plantLocationDetails?.samplePlantLocations)
);Committable suggestion was skipped due to low confidence.
| SUM(COALESCE(ps.tree_count, iv.trees_planted, 0)) AS total_tree_count | ||
| FROM intervention iv | ||
| LEFT JOIN planted_species ps ON iv.id = ps.intervention_id | ||
| LEFT JOIN scientific_species ss ON COALESCE(iv.scientific_species_id, ps.scientific_species_id) = ss.id |
There was a problem hiding this comment.
Adjust the JOIN condition to handle NULL species IDs properly
The LEFT JOIN on scientific_species ss uses COALESCE(iv.scientific_species_id, ps.scientific_species_id) = ss.id. If both iv.scientific_species_id and ps.scientific_species_id are NULL, the condition becomes NULL = ss.id, which will not match any records. This could lead to missing species names in the results.
Consider modifying the JOIN condition to accommodate cases where either species ID is NULL:
- LEFT JOIN scientific_species ss ON COALESCE(iv.scientific_species_id, ps.scientific_species_id) = ss.id
+ LEFT JOIN scientific_species ss ON (iv.scientific_species_id = ss.id OR ps.scientific_species_id = ss.id)This change ensures that the join will succeed if either of the species IDs matches ss.id.
📝 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.
| LEFT JOIN scientific_species ss ON COALESCE(iv.scientific_species_id, ps.scientific_species_id) = ss.id | |
| LEFT JOIN scientific_species ss ON (iv.scientific_species_id = ss.id OR ps.scientific_species_id = ss.id) |
| const query = ` | ||
| SELECT | ||
| ps.other_species, | ||
| COALESCE(iv.scientific_species_id, | ||
| ps.scientific_species_id) AS scientific_species_id, | ||
| COALESCE(ss.name, | ||
| ps.other_species, | ||
| iv.other_species, 'Unknown') AS name, | ||
| SUM(COALESCE(ps.tree_count, iv.trees_planted, 0)) AS total_tree_count | ||
| FROM intervention iv | ||
| LEFT JOIN planted_species ps ON iv.id = ps.intervention_id | ||
| LEFT JOIN scientific_species ss ON COALESCE(iv.scientific_species_id, ps.scientific_species_id) = ss.id | ||
| JOIN project pp ON iv.plant_project_id = pp.id | ||
| WHERE | ||
| iv.deleted_at IS NULL | ||
| AND iv.type IN ('single-tree-registration', 'multi-tree-registration') | ||
| AND pp.guid = ? | ||
| AND iv.intervention_start_date BETWEEN ? AND ? | ||
| GROUP BY | ||
| COALESCE(iv.scientific_species_id, ps.scientific_species_id), | ||
| COALESCE(ss.name, ps.other_species, iv.other_species, 'Unknown') | ||
| ORDER BY total_tree_count DESC`; |
There was a problem hiding this comment.
Include ps.other_species in the GROUP BY clause to avoid SQL errors
The query selects ps.other_species without aggregating it or including it in the GROUP BY clause. Some SQL dialects require that all non-aggregated columns in the SELECT clause be included in the GROUP BY clause. Omitting it may lead to SQL errors or unexpected results.
Apply this diff to include ps.other_species in the GROUP BY clause:
GROUP BY
COALESCE(iv.scientific_species_id, ps.scientific_species_id),
COALESCE(ss.name, ps.other_species, iv.other_species, 'Unknown')
+ , ps.other_species📝 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 query = ` | |
| SELECT | |
| ps.other_species, | |
| COALESCE(iv.scientific_species_id, | |
| ps.scientific_species_id) AS scientific_species_id, | |
| COALESCE(ss.name, | |
| ps.other_species, | |
| iv.other_species, 'Unknown') AS name, | |
| SUM(COALESCE(ps.tree_count, iv.trees_planted, 0)) AS total_tree_count | |
| FROM intervention iv | |
| LEFT JOIN planted_species ps ON iv.id = ps.intervention_id | |
| LEFT JOIN scientific_species ss ON COALESCE(iv.scientific_species_id, ps.scientific_species_id) = ss.id | |
| JOIN project pp ON iv.plant_project_id = pp.id | |
| WHERE | |
| iv.deleted_at IS NULL | |
| AND iv.type IN ('single-tree-registration', 'multi-tree-registration') | |
| AND pp.guid = ? | |
| AND iv.intervention_start_date BETWEEN ? AND ? | |
| GROUP BY | |
| COALESCE(iv.scientific_species_id, ps.scientific_species_id), | |
| COALESCE(ss.name, ps.other_species, iv.other_species, 'Unknown') | |
| ORDER BY total_tree_count DESC`; | |
| const query = ` | |
| SELECT | |
| ps.other_species, | |
| COALESCE(iv.scientific_species_id, | |
| ps.scientific_species_id) AS scientific_species_id, | |
| COALESCE(ss.name, | |
| ps.other_species, | |
| iv.other_species, 'Unknown') AS name, | |
| SUM(COALESCE(ps.tree_count, iv.trees_planted, 0)) AS total_tree_count | |
| FROM intervention iv | |
| LEFT JOIN planted_species ps ON iv.id = ps.intervention_id | |
| LEFT JOIN scientific_species ss ON COALESCE(iv.scientific_species_id, ps.scientific_species_id) = ss.id | |
| JOIN project pp ON iv.plant_project_id = pp.id | |
| WHERE | |
| iv.deleted_at IS NULL | |
| AND iv.type IN ('single-tree-registration', 'multi-tree-registration') | |
| AND pp.guid = ? | |
| AND iv.intervention_start_date BETWEEN ? AND ? | |
| GROUP BY | |
| COALESCE(iv.scientific_species_id, ps.scientific_species_id), | |
| COALESCE(ss.name, ps.other_species, iv.other_species, 'Unknown') | |
| , ps.other_species | |
| ORDER BY total_tree_count DESC`; |
| LEFT JOIN planted_species ps ON iv.id = ps.intervention_id | ||
| LEFT JOIN scientific_species ss ON COALESCE(iv.scientific_species_id, ps.scientific_species_id) = ss.id |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential Performance Issue with COALESCE in JOIN Condition
Using COALESCE in the LEFT JOIN condition may lead to inefficient query execution because it prevents the database from using indexes on iv.scientific_species_id and ps.scientific_species_id.
Consider restructuring the JOIN to improve performance:
-LEFT JOIN scientific_species ss ON COALESCE(iv.scientific_species_id, ps.scientific_species_id) = ss.id
+LEFT JOIN scientific_species ss
+ ON iv.scientific_species_id = ss.id
+ OR ps.scientific_species_id = ss.idThis change allows the database to utilize indexes on scientific_species_id, potentially improving query performance.
📝 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.
| LEFT JOIN planted_species ps ON iv.id = ps.intervention_id | |
| LEFT JOIN scientific_species ss ON COALESCE(iv.scientific_species_id, ps.scientific_species_id) = ss.id | |
| LEFT JOIN planted_species ps ON iv.id = ps.intervention_id | |
| LEFT JOIN scientific_species ss | |
| ON iv.scientific_species_id = ss.id | |
| OR ps.scientific_species_id = ss.id |
| FROM intervention iv | ||
| LEFT JOIN intervention siv ON iv.id = siv.parent_id | ||
| INNER JOIN intervention siv ON iv.id = siv.parent_id | ||
| LEFT JOIN scientific_species ss ON iv.scientific_species_id = ss.id |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent usage of COALESCE in JOIN conditions
On line 70, the join condition uses iv.scientific_species_id = ss.id without COALESCE, whereas elsewhere, COALESCE(iv.scientific_species_id, ps.scientific_species_id) is used. For consistency and to handle potential null values in iv.scientific_species_id, consider updating the join condition.
Apply this diff for consistency:
- LEFT JOIN scientific_species ss ON iv.scientific_species_id = ss.id
+ LEFT JOIN scientific_species ss ON COALESCE(iv.scientific_species_id, ps.scientific_species_id) = ss.idCommittable suggestion was skipped due to low confidence.
hidto identify the selected plant location