Verified data exists on GitHub, created new API endpoint, rewrote fro…#258
Verified data exists on GitHub, created new API endpoint, rewrote fro…#258
Conversation
…ntend, removed any references to local path, deleted local folder
leandrumartin
left a comment
leandrumartin
left a comment
There was a problem hiding this comment.
Running the code with npm start causes the colored regions to not load, because it does not fetch the correct URLs, and the code interacts with the fetching function wrong. Make sure to test that your code works every time before you submit it for review.
boneset-api/server.js
Outdated
| }); | ||
|
|
||
| // 🌟 FINALIZED ENDPOINT: Annotation Data (Fetches & Combines Scaling Data) 🌟 | ||
| // New endpoint: Get colored region annotations for a bone |
There was a problem hiding this comment.
This comment is unnecessary, and will only create confusion in the future when the endpoint is no longer new.
boneset-api/server.js
Outdated
| } | ||
|
|
||
| // Build GitHub URL for the colored regions JSON | ||
| const GITHUB_COLORED_REGIONS_URL = `${GITHUB_REPO}annotations/ColoredRegions/${boneId}_colored_regions.json`; |
There was a problem hiding this comment.
This does not match the file names in the data branch. Currently, running this code results in a 404 error every time.
boneset-api/server.js
Outdated
| } | ||
| }); | ||
|
|
||
| // FINALIZED ENDPOINT: Annotation Data (Fetches & Combines Scaling Data) |
There was a problem hiding this comment.
This comment is also unnecessary and annotates an endpoint that was not changed. There are a few changes to comments made below that also are unnecessary. It's ok for now, but in the future generally avoid making changes that do not address the issue being tackled, as it clogs up the PR and makes it more tedious to review and parse out which changes are important.
| // Network error or parsing error - try next variation | ||
| console.debug(`[ColoredRegions] Error with ${filename}:`, error.message); | ||
| try { | ||
| console.log(`[ColoredRegions] Fetching from API endpoint: /api/colored-regions?boneId=${boneId}`); |
There was a problem hiding this comment.
I would actually recommend getting rid of this log. When looking at the JavaScript console on the running website, there are end up being so many logs that it gets difficult to find actual bugs. This particular log duplicates information that is already available in the Network tab of your browser's dev tools, so I would get rid of this line.
| const response = await fetch(`/api/colored-regions?boneId=${encodeURIComponent(boneId)}`); | ||
|
|
||
| if (!response.ok) { | ||
| console.log(`[ColoredRegions] Failed to fetch colored regions from API: ${response.status}`); |
There was a problem hiding this comment.
This line is ok to keep, but make it a console.error instead of console.log so we can more easily find errors when looking through the console.
leandrumartin
left a comment
leandrumartin
left a comment
There was a problem hiding this comment.
Sorry for not clarifying earlier what the best approach was. I will try to be more up-front about these things in future reviews.
There are still some other comments that have not yet been addressed.
| }); | ||
| } | ||
|
|
||
| // Map bone IDs to actual GitHub filenames |
There was a problem hiding this comment.
This mapping would be difficult and tedious to maintain when the information in the database is updated. When files are added later as new data is extracted from more PowerPoints, this will have to be updated too and things may be missed in the process. This is the kind of structure we are trying to move away from in order to support updating the data in our database.
Rather than this, I think a better approach would be to update the data branch to have the filenames that match the bone IDs, as was already done with the local files that were removed in this PR. Those file names generally matched the scheme of <boneId>_colored_regions.json, so we should be able to dynamically build the filename to fetch from the ID instead of having the filenames hardcoded. So I think the best approach would be to make another PR to data to replace the colored regions annotations files there with the ones that were removed in this PR. (If some mapping between ID and filename is still necessary after that change, then that's fine.)
I should have clarified that that was the correct approach in my previous review—sorry for not clarifying earlier.
2 participants
Working on Issue #244
This PR adds a new GET /api/colored-regions?boneId= endpoint that retrieves colored region annotation data directly from the GitHub data branch instead of relying on local files. It also simplifies the frontend by updating it to call this new API endpoint without hardcoding filenames. Previously, the frontend and backend were tightly coupled, which made the codebase rigid and harder to maintain. It also created confusion when trying to understand the project structure. This update introduces a new GET /api/colored-regions route in server.js that validates the boneId parameter and dynamically fetches data from DataPelvis/annotations/ColoredRegions/{boneId}_colored_regions.json on GitHub. On the frontend, fetchColoredRegionData() in coloredRegionsOverlay.js was rewritten to call the API endpoint using only the boneId query parameter, eliminating all hardcoded filenames. All LOCAL_PATH configuration and references to local file paths were removed from both the code and documentation. The data_extraction/annotations/color_regions/ folder has been deleted because the data is now fully retrieved from GitHub.