Add group column, remove group from subject id #1215
Conversation
WalkthroughIntroduces a helper function Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (3)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
166-171: Apply consistent transformations to JSON export.The JSON export uses raw
params.subjectIdwithout theafterFirstDollartransformation and doesn't include theGroupIDfield. This is inconsistent with CSV/TSV exports.case 'JSON': { exportRecords.map((item) => { - item.subjectID = params.subjectId; + item.GroupID = currentGroup ? currentGroup.id : 'root'; + item.subjectID = afterFirstDollar(params.subjectId); }); void download(`${baseFilename}.json`, () => Promise.resolve(JSON.stringify(exportRecords, null, 2))); break; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/api/src/instrument-records/instrument-records.service.ts(3 hunks)apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts(4 hunks)apps/web/src/hooks/useInstrumentVisualization.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint-and-test
🔇 Additional comments (5)
apps/api/src/instrument-records/instrument-records.service.ts (1)
190-190: LGTM!The transformation is applied consistently to subject IDs in both export paths.
Also applies to: 213-213
apps/web/src/hooks/useInstrumentVisualization.ts (2)
77-94: LGTM!Wide-format export correctly adds GroupID and applies the subject ID transformation.
96-136: LGTM!Long-format export correctly includes GroupID and transformed SubjectID in both array and non-array paths.
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (2)
75-77: LGTM!Test expectations correctly reflect the new GroupID column in CSV and TSV exports (both wide and long formats).
Also applies to: 90-92, 106-108, 122-124
138-139: Update test after fixing JSON export.Once the JSON export in
useInstrumentVisualization.tsis updated to includeGroupID, this test should verify its presence.After applying the suggested fix in
apps/web/src/hooks/useInstrumentVisualization.ts(lines 166-171), update this assertion:const jsonContents = await getContentFn(); + expect(jsonContents).toContain('"GroupID": "testGroupId"'); expect(jsonContents).toContain('"someValue": "abc"'); expect(jsonContents).toContain('"subjectID": "testId"');
| function afterFirstDollar(str: string) { | ||
| const match = /\$(.*)/.exec(str); | ||
| if (!match) return str; | ||
| if (!match[1]) return str; | ||
| return match[1]; | ||
| } |
There was a problem hiding this comment.
Add input validation.
The function assumes str is a string but doesn't validate it. If called with null or undefined, the regex exec will throw.
function afterFirstDollar(str: string) {
+ if (!str) return str;
const match = /\$(.*)/.exec(str);
if (!match) return str;
if (!match[1]) return str;
return match[1];
}🤖 Prompt for AI Agents
In apps/api/src/instrument-records/instrument-records.service.ts around lines 41
to 46, the helper assumes `str` is a string and will throw if
`null`/`undefined`; add input validation by first checking `if (str == null)
return str;` then ensure a safe string by doing `const s = typeof str ===
'string' ? str : String(str);` and run the regex against `s`, returning early if
no match or empty capture — this prevents runtime errors for null/undefined and
handles non-string inputs safely.
| function afterFirstDollar(str: string) { | ||
| const match = /\$(.*)/.exec(str); | ||
| if (!match) return str; | ||
| if (!match[1]) return str; | ||
| return match[1]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract duplicated helper to shared utility.
This function is duplicated in apps/api/src/instrument-records/instrument-records.service.ts (lines 41-46). Extract it to a shared utility module to maintain a single source of truth.
Consider creating a shared utility file like packages/utils/src/subject-id.ts:
export function afterFirstDollar(str: string): string {
if (!str) return str;
const match = /\$(.*)/.exec(str);
if (!match) return str;
if (!match[1]) return str;
return match[1];
}🤖 Prompt for AI Agents
In apps/web/src/hooks/useInstrumentVisualization.ts around lines 39 to 44, the
helper function afterFirstDollar is duplicated in
apps/api/src/instrument-records/instrument-records.service.ts; extract it into a
shared utility (e.g., packages/utils/src/subject-id.ts) and import it from both
locations. Create the new utility file exporting afterFirstDollar with the same
behavior, update both files to remove the local function and import the shared
function, and run type checks to ensure imports/exports and any path aliases are
correct.
2 participants
This pr add a group column to the subject download table containing the group id of the subject.
the group id is also removed from the subjectids in the datahub export as well.
closes issue #1005
Summary by CodeRabbit
New Features
Enhancements