Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
boneset-api/server.js
Outdated
| console.log(`[ColoredRegions API] Fetching colored regions for: ${boneId}`); | ||
| console.log("[ColoredRegions API] Files to try:", filenamesToTry); | ||
|
|
||
| // Try local first (working coordinate system) |
There was a problem hiding this comment.
We should not be using local files. The purpose of the issue being worked on here is to move away from the local files.
The reasoning behind this is because we want all of our data stored in one place. When all of our data is being fetched from one database, it will make our data structure clearer when we are working on it in the future. That is especially true because this is an open-source project and anybody can make a contribution. The structure of our data would be confusing and difficult to work with if some of it were being fetched from GitHub and some of it were being served locally by the server. Therefore, we want in this issue to move away from data being served locally by the server and towards all of our data being fetched from the database on GitHub.
The addition of trying local files is a regression from what was being done in the previous pull request you were working on for this issue. Why was it added in?
There was a problem hiding this comment.
The /api/colored-regions endpoint should now only fetch from GitHub.
There was a problem hiding this comment.
Why was it added in? I ask because I want to know the thought process and whether there was some error in or lack of communication on my part.
| @@ -104,464 +101,37 @@ async function fetchColoredRegionData(boneId, isBonesetSelection = false) { | |||
| const bonesWithColoredRegions = ["bony_pelvis", "iliac_crest", "anterior_iliac_spines", "posterior_iliac_spines", "posterior_superior_iliac_spines", "posterior_inferior_iliac_spines", "auricular_surface", "ramus", "ischial_tuberosity", "ischial_spine", "sciatic_notches", "pubic_rami", "pectineal_line", "symphyseal_surface", "pubic_tubercle"]; | |||
There was a problem hiding this comment.
In the other PR, this was amended to include "ilium", "ischium", "pubis", and the else if (boneId === "ilium" && !isBonesetSelection) condition was removed in the part above this line.
I don't really know the reasoning behind the initial if condition nor the reason why it was changed in the other PR, so I don't know whether it's better to have that change or not. Maybe the regions weren't loading right one way and that's fine, but I'd just like to ask why this change wasn't included in this PR?
Simplify the colored-regions API to dynamically construct filenames from boneId instead of using hardcoded mappings. Remove local file fallback to centralize all data in GitHub, making the codebase clearer for contributors.
… region data on GitHub
leandrumartin
left a comment
leandrumartin
left a comment
There was a problem hiding this comment.
Looks good, and no changes needed—just left some notes for the future. But before I approve and merge, I am still curious about the change in coloredRegionsOverlay.js, where an array was amended to include more things and an else-if was removed. Why was this change made in the other PR? If necessary, why was it at first not included here? I want to know whether this made any difference you noticed in the functionality, or why else you included that change in, because I want to know whether that change should be merged in.
boneset-api/server.js
Outdated
| const annotationData = JSON.parse(localAnnotationData); // Parse the local file | ||
|
|
||
| // 🛑 FIX: Define Full Slide Dimensions for Normalization 🛑 | ||
| // FIX: Define Full Slide Dimensions for Normalization |
There was a problem hiding this comment.
Please in the future do not make extraneous changes not relevant to the issue being resolved. It caused a merge conflict that, while simple to resolve, could have been avoided.
boneset-api/server.js
Outdated
| if (error.response?.status === 404) { | ||
| console.log(`[ColoredRegions API] Not found: ${filename}`); | ||
| return res.status(404).json({ | ||
| error: "Not Found", |
There was a problem hiding this comment.
As mentioned before, there is no "message" field that this method takes, only an "error" one that should contain the message. I'll make this change myself since it's a small one, but I'm letting you know for the future.
|
@leandrumartin The change in coloredRegionsOverlay.js was originally made to allow colored region overlays to display when selecting the parent bones individually, specifically the ilium, ischium, and pubis. Before that update, overlays only showed for bonesets or certain sub-bones like iliac_crest. There was an else if block that was unintentionally preventing colored regions from appearing when selecting something like the ilium on its own. Adding those bones to the bonesWithColoredRegions array and removing that conditional fixed that behavior. I did not notice any additional differences in behavior between the two implementations; I just tried a different approach. |
|
Ok, thanks for letting me know. This PR looks good to merge then. |
2 participants
Resolves Issue #244
I created a new PR because I wanted a fresh start.
This update fixes the /api/colored-regions endpoint so it correctly handles the GitHub data format for colored region annotations after those files were moved to the data branch. After merging the data PR that moved the colored region files to the data branch, the API was successfully fetching the files from GitHub. However, it was discarding them because the data validation logic was incorrect. The data on GitHub was already structured in the expected local format, using an images array. The transformation function, however, was expecting a different structure with a top-level colored_regions property. Because of this mismatch, valid data was being rejected.
Screenshots
The regions are showing up on my end.
