New features #1253
New features #1253
Conversation
WalkthroughAdds API and frontend support for viewing individual instrument records: new GET /instrument-records/:id endpoint with access control, React query hooks, a record detail route with preloading, table navigation to records, QR code component for assignments, and InstrumentSummary option to show all measures. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as Client UI (Router)
participant Route as Record Detail Route
participant Hooks as Query Hooks
participant API as API Client
participant Controller as InstrumentRecordsController
participant Service as InstrumentRecordsService
participant DB as Database
User->>UI: Click table row (contains __id__)
UI->>Route: Navigate to /datahub/$subjectId/$recordId
Route->>Hooks: useInstrumentRecordQuery(recordId)
Hooks->>API: GET /v1/instrument-records/:id
API->>Controller: findById(id, ability)
Controller->>Service: findById(id, { ability })
Service->>DB: accessibleQuery + find by id
DB-->>Service: InstrumentRecord (or null)
Service-->>Controller: InstrumentRecord or NotFound
Controller-->>API: Return record
API-->>Hooks: Parse record ($InstrumentRecord.parse)
Hooks-->>Route: Record resolved
Route->>Hooks: useSubjectQuery(record.subjectId)
Hooks->>API: GET /v1/subjects/:id
API-->>Hooks: Subject data
Hooks-->>Route: Subject resolved
Route->>Route: Resolve instrument via useInstrument
Route->>UI: Render InstrumentSummary(displayAllMeasures)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/api/src/instrument-records/instrument-records.service.ts (1)
257-265: findById access control and error handling look good; consider tightening the where-clause for consistencyThe method correctly:
- Applies
accessibleQuery(ability, 'read', 'InstrumentRecord')so only readable records can be fetched.- Returns a 404 (
NotFoundException) when the record is missing or inaccessible, which is a reasonable behavior.For readability and consistency with other methods (e.g.,
count,find), you might want to fold theidinto the sameANDarray instead of mixing top-level andANDfilters:- const record = await this.instrumentRecordModel.findFirst({ - where: { AND: [accessibleQuery(ability, 'read', 'InstrumentRecord')], id } - }); + const record = await this.instrumentRecordModel.findFirst({ + where: { + AND: [ + accessibleQuery(ability, 'read', 'InstrumentRecord'), + { id } + ] + } + });Optional, but makes the intent a bit clearer and mirrors existing patterns in this service.
apps/web/src/routes/_app/datahub/index.tsx (1)
175-178: Subject selection now correctly lands on the table viewRouting subject row clicks to
./${subject.id}/tablematches the new “table-first” UX. You might also consider updating the lookup flow (Line 130) to navigate to the table instead of assignments for consistency, unless you intentionally want lookup to prioritize assignments.apps/web/src/routes/_app/datahub/$subjectId/table.tsx (1)
10-66: Be explicit aboutsubjectIdwhen navigating to the record routeThe row click navigation to
/datahub/$subjectId/$recordIdis correct conceptually, but you currently only passrecordIdinparams. To avoid relying on any implicit parent param inference, it’s safer to also passsubjectIdexplicitly:- const navigate = Route.useNavigate(); + const navigate = Route.useNavigate(); @@ - onEntryClick={(row) => { - void navigate({ params: { recordId: row.__id__ }, to: '/datahub/$subjectId/$recordId' }); - }} + onEntryClick={(row) => { + void navigate({ + to: '/datahub/$subjectId/$recordId', + params: { subjectId: params.subjectId, recordId: row.__id__ } + }); + }}This keeps the route params explicit and avoids subtle routing issues if the router stops inheriting parent params.
apps/web/src/routes/_app/datahub/$subjectId/assignments.tsx (1)
24-81: Tightening slider open condition is good; refine URL handling and QR fallbackRequiring
assignmentandinstrumentin theopencondition is a solid improvement. Two small follow‑ups:
- For the input, avoid passing a null/undefined URL directly; normalizing to an empty string is safer:
- <Input readOnly className="h-9" id="link" value={assignment?.url} /> + <Input readOnly className="h-9" id="link" value={assignment?.url ?? ''} />
- For the QR code, encoding
'javascript:void(0)'when no URL is available is confusing for users scanning it. It’s cleaner to only render the QR code when a real URL exists:- <QRCode url={assignment?.url ?? 'javascript:void(0)'} /> + {assignment?.url && <QRCode url={assignment.url} />}This keeps the UI honest and avoids meaningless or odd QR targets.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
apps/api/src/instrument-records/instrument-records.controller.ts(1 hunks)apps/api/src/instrument-records/instrument-records.service.ts(1 hunks)apps/web/package.json(2 hunks)apps/web/src/components/QRCode.tsx(1 hunks)apps/web/src/hooks/useInstrumentRecordQuery.ts(1 hunks)apps/web/src/hooks/useInstrumentVisualization.ts(2 hunks)apps/web/src/hooks/useSubjectQuery.ts(1 hunks)apps/web/src/route-tree.ts(10 hunks)apps/web/src/routes/_app/datahub/$subjectId/$recordId.tsx(1 hunks)apps/web/src/routes/_app/datahub/$subjectId/assignments.tsx(2 hunks)apps/web/src/routes/_app/datahub/$subjectId/route.tsx(1 hunks)apps/web/src/routes/_app/datahub/$subjectId/table.tsx(2 hunks)apps/web/src/routes/_app/datahub/index.tsx(1 hunks)packages/react-core/src/components/InstrumentSummary/InstrumentSummary.tsx(2 hunks)packages/react-core/src/index.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
packages/react-core/src/components/InstrumentSummary/InstrumentSummary.tsx (2)
packages/runtime-core/src/types/instrument.core.ts (1)
AnyUnilingualInstrument(48-48)packages/react-core/src/types.ts (1)
SubjectDisplayInfo(23-23)
apps/web/src/components/QRCode.tsx (2)
apps/outreach/src/scripts/theme-init.js (1)
theme(9-9)packages/runtime-internal/src/interactive/worker.js (1)
url(61-61)
apps/web/src/hooks/useSubjectQuery.ts (1)
packages/schemas/src/subject/subject.ts (1)
$Subject(20-27)
apps/web/src/hooks/useInstrumentRecordQuery.ts (1)
packages/schemas/src/instrument-records/instrument-records.ts (1)
$InstrumentRecord(42-51)
apps/web/src/routes/_app/datahub/$subjectId/table.tsx (3)
apps/web/src/routes/_app/datahub/$subjectId/route.tsx (1)
Route(73-75)apps/web/src/routes/_app/datahub/index.tsx (1)
Route(185-191)apps/web/src/routes/_app/datahub/$subjectId/graph.tsx (1)
Route(168-170)
apps/web/src/routes/_app/datahub/$subjectId/$recordId.tsx (5)
apps/web/src/routes/_app/datahub/index.tsx (1)
Route(185-191)apps/web/src/hooks/useInstrumentRecordQuery.ts (2)
useInstrumentRecordQuery(15-17)instrumentRecordQueryOptions(5-13)apps/web/src/hooks/useSubjectQuery.ts (2)
useSubjectQuery(19-21)subjectQueryOptions(9-17)apps/web/src/hooks/useInstrument.ts (1)
useInstrument(10-32)packages/react-core/src/components/InstrumentSummary/InstrumentSummary.tsx (1)
InstrumentSummary(23-217)
apps/api/src/instrument-records/instrument-records.controller.ts (3)
apps/api/src/core/decorators/route-access.decorator.ts (1)
RouteAccess(18-20)apps/web/src/store/types.ts (1)
CurrentUser(8-10)apps/api/src/auth/auth.types.ts (1)
AppAbility(23-23)
apps/api/src/instrument-records/instrument-records.service.ts (2)
apps/api/src/core/types.ts (1)
EntityOperationOptions(3-5)apps/api/src/auth/ability.utils.ts (1)
accessibleQuery(26-35)
⏰ 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 (11)
apps/web/src/hooks/useInstrumentVisualization.ts (3)
21-21: Type addition looks good.The
__id__field follows the existing metadata naming convention.
80-80: Consider whether__id__should be excluded from exports.Line 80 omits
__time__but not__id__. The new__id__field will appear in CSV/Excel/JSON exports. Verify this is intentional—record IDs may be useful for traceability but could clutter analytical datasets.
233-233: Verifyrecord.idis always defined and typed as string.Ensure the API guarantees
record.idexists and is a string before assigning it to__id__.apps/api/src/instrument-records/instrument-records.controller.ts (1)
96-101: GET /instrument-records/:id endpoint is wired correctlyThe new route cleanly mirrors the existing patterns:
- Uses
ValidObjectIdPipeand@CurrentUser('ability')consistently.- Applies
RouteAccess({ action: 'read', subject: 'InstrumentRecord' })and delegates to the service method that also enforcesaccessibleQuery.No issues from my side here.
apps/web/src/hooks/useInstrumentRecordQuery.ts (1)
1-17: Instrument record query hook looks correctQuery key, axios call, and
$InstrumentRecordparsing are consistent and should integrate cleanly with suspense queries.apps/web/package.json (1)
49-69: QR code dependencies align with new component usageAdding
qrcodeand@types/qrcodematches the newQRCodecomponent; nothing else stands out here. Please just ensure lockfile and builds/tests are updated and passing.apps/web/src/routes/_app/datahub/$subjectId/route.tsx (1)
55-65: Tab ordering change matches requirementsPutting the table and graph tabs before assignments aligns with the requested DataHub tab order, with no functional regressions apparent.
packages/react-core/src/index.ts (1)
1-11: Publicly exportingInstrumentSummaryis appropriateRe‑exporting
InstrumentSummaryfrom the package index matches the existing pattern and allows clean consumption from the web app.apps/web/src/components/QRCode.tsx (1)
1-32: QRCode canvas wrapper is straightforward and fits the use caseHooking into
useThemeand rendering viaqrcode.toCanvasin an effect is a clean, focused implementation; nothing problematic stands out here.packages/react-core/src/components/InstrumentSummary/InstrumentSummary.tsx (1)
17-17: LGTM! Clean implementation of the displayAllMeasures feature.The optional prop is properly typed and the short-circuit logic correctly bypasses visibility filtering when enabled. This aligns with the PR objective to re-view instrument summaries with all measures visible.
Also applies to: 23-29, 38-40
apps/web/src/routes/_app/datahub/$subjectId/$recordId.tsx (1)
1-41: LGTM! Well-structured data flow.The component correctly chains data fetching (record → subject → instrument) and the loader appropriately prefetches both instrumentRecord and subject data. The use of
displayAllMeasuresaligns with the PR objective to re-view completed instruments with all measures visible.
| export const subjectQueryOptions = ({ params }: { params: SubjectQueryParams }) => { | ||
| return queryOptions({ | ||
| queryFn: async () => { | ||
| const response = await axios.get(`/v1/subjects/${params.id}`, { params }); | ||
| return $Subject.parseAsync(response.data); | ||
| }, | ||
| queryKey: ['subjects', `id-${params.id}`] | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Remove redundant params from axios config.
Line 12 passes params as query parameters, resulting in /v1/subjects/123?id=123. The ID is already in the URL path and shouldn't be duplicated as a query parameter.
Apply this diff:
export const subjectQueryOptions = ({ params }: { params: SubjectQueryParams }) => {
return queryOptions({
queryFn: async () => {
- const response = await axios.get(`/v1/subjects/${params.id}`, { params });
+ const response = await axios.get(`/v1/subjects/${params.id}`);
return $Subject.parseAsync(response.data);
},
queryKey: ['subjects', `id-${params.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.
| export const subjectQueryOptions = ({ params }: { params: SubjectQueryParams }) => { | |
| return queryOptions({ | |
| queryFn: async () => { | |
| const response = await axios.get(`/v1/subjects/${params.id}`, { params }); | |
| return $Subject.parseAsync(response.data); | |
| }, | |
| queryKey: ['subjects', `id-${params.id}`] | |
| }); | |
| }; | |
| export const subjectQueryOptions = ({ params }: { params: SubjectQueryParams }) => { | |
| return queryOptions({ | |
| queryFn: async () => { | |
| const response = await axios.get(`/v1/subjects/${params.id}`); | |
| return $Subject.parseAsync(response.data); | |
| }, | |
| queryKey: ['subjects', `id-${params.id}`] | |
| }); | |
| }; |
🤖 Prompt for AI Agents
In apps/web/src/hooks/useSubjectQuery.ts around lines 9 to 17, the axios.get
call incorrectly passes the entire params object as query parameters causing
duplicate id in the URL (e.g. /v1/subjects/123?id=123); remove the second
argument ({ params }) from axios.get so the request uses only the path
parameter, leaving the axios call as axios.get(`/v1/subjects/${params.id}`) and
keep the rest (parsing and queryKey) unchanged.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/web/src/hooks/useInstrumentVisualization.ts (2)
18-23: New required__id__field may be a breaking type changeMaking
__id__a required property onInstrumentVisualizationRecordwill force all manual constructions of this type (tests, mocks, ad‑hoc objects) to supply an ID. If there are any such call sites, this will surface as TS errors.Two options you might consider:
- Keep it required but add a short doc comment clarifying it’s the backing instrument‑record ID used for navigation and not included in exports.
- Or, make it optional (
__id__?: string) if you expect partially populated records in other contexts.
80-80: Confirm intent to strip__id__from all export formats
exportRecordsnow omits both__time__and__id__, so the internal record identifier is never present in CSV/TSV/JSON/Excel outputs. That’s a reasonable privacy/cleanliness choice and matches the server‑side export shape that doesn’t include record IDs, but it does mean downstream consumers cannot link exported rows back to specific instrument records.Please confirm this is the desired behavior for consumers of JSON/CSV exports; if not, you may want to keep
__id__for JSON only, or expose a more explicitrecordIdfield.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/hooks/useInstrumentVisualization.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/hooks/useInstrumentVisualization.ts (2)
apps/api/src/instrument-records/instrument-records.controller.ts (1)
exportRecords(70-72)apps/api/src/instrument-records/instrument-records.service.ts (1)
exportRecords(135-221)
⏰ 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 (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
225-238: Populating__id__fromrecord.idlooks correctUsing
record.idto set__id__at materialization time cleanly ties visualization rows back to their underlying instrument records and keeps the rest of the shaping logic unchanged.Assuming
record.idis always a defined string, this is sound and aligns with the new type.
1 participant
closes #1247
closes #1246
closes #1245
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.