Subject table download #1210
Subject table download #1210
Conversation
WalkthroughAdds CSV/TSV wide and long export formats and JSON enhancements to instrument exports; introduces a Jest-style zustand test mock with automatic resets and tests for exports; updates Vitest aliasing; adds testing-library devDependencies to the web app and removes a root devDependency. (34 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as ActionDropdown
participant Hook as useInstrumentVisualization
participant API as instrument/info & records
participant Formatter as makeWideRows / makeLongRows
participant Parser as parseHelper / JSON
participant Download as download util
Note right of Hook #eef6ff: export selection triggers flow
UI->>Hook: select subjectId + format
Hook->>API: fetch instrument info & records
API-->>Hook: instrument + records
alt wide format (CSV/TSV)
Hook->>Formatter: makeWideRows(records)
Formatter-->>Parser: rows + delimiter
else long format (CSV Long/TSV Long)
Hook->>Formatter: makeLongRows(records)
Formatter-->>Parser: rows + delimiter
else JSON
Hook->>Parser: attach subjectID + pretty stringify
end
Parser->>Download: download(filename, content, mime)
Download-->>UI: browser file saved
alt error (missing instrument/data)
Hook->>Hook: notify error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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 |
david-roper
force-pushed
the
subject-table-download
branch
from
8cc7f6f to
b607bd8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
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/routes/_app/datahub/$subjectId/table.tsx (1)
18-26: Fix crash when records is empty (records[0] is undefined).Guard before iterating or derive from keys safely.
Apply this diff:
- const fields: { field: string; label: string }[] = []; - for (const subItem in records[0]) { - if (!subItem.startsWith('__')) { - fields.push({ - field: subItem, - label: camelToSnakeCase(subItem).toUpperCase() - }); - } - } + const fields: { field: string; label: string }[] = + records.length === 0 + ? [] + : Object.keys(records[0]!) + .filter((k) => !k.startsWith('__')) + .map((k) => ({ field: k, label: camelToSnakeCase(k).toUpperCase() }));
🧹 Nitpick comments (7)
apps/web/src/__mocks__/zustand.ts (1)
37-49: Simplify redundant typeof checks.StateCreator is a function; the branches can be collapsed.
Apply:
-export function create<T>(stateCreator: StateCreator<T>): UseBoundStore<StoreApi<T>> { - if (typeof stateCreator === 'function') { - return createUncurried(stateCreator); - } - return createUncurried as unknown as UseBoundStore<StoreApi<T>>; -} +export function create<T>(stateCreator: StateCreator<T>): UseBoundStore<StoreApi<T>> { + return createUncurried(stateCreator); +} -export function createStore<T>(stateCreator: StateCreator<T>): StoreApi<T> { - if (typeof stateCreator === 'function') { - return createStoreUncurried(stateCreator); - } - return createStoreUncurried as unknown as StoreApi<T>; -} +export function createStore<T>(stateCreator: StateCreator<T>): StoreApi<T> { + return createStoreUncurried(stateCreator); +}apps/web/src/hooks/useInstrumentVisualization.ts (4)
70-84: Unify SubjectID casing across formats (wide vs long).Long format uses SubjectID; wide uses subjectId. Standardize to SubjectID for consistency.
Apply:
- return exportRecords.map((item) => { - const obj: { [key: string]: any } = { subjectId: params.subjectId }; + return exportRecords.map((item) => { + const obj: { [key: string]: any } = { SubjectID: params.subjectId }; for (const key of columnNames) { const val = item[key]; if (key === '__date__') { obj.Date = toBasicISOString(val as Date); continue; } obj[key] = typeof val === 'object' ? JSON.stringify(val) : val; } return obj; });
152-156: Avoid map for side-effects; consider consistent ID key.Use forEach; optionally align to SubjectID for JSON too.
Apply:
- exportRecords.map((item) => { - item.subjectID = params.subjectId; - }); + exportRecords.forEach((item) => { + (item as any).SubjectID = params.subjectId; + });
64-66: Sanitize timestamp in filenames (avoid ':').Use toBasicISOString (compact ISO) to avoid OS‑unsafe chars.
Apply:
- const baseFilename = `${currentUser!.username}_${instrument.internal.name}_${ - instrument.internal.edition - }_${new Date().toISOString()}`; + const baseFilename = `${currentUser!.username}_${instrument.internal.name}_${ + instrument.internal.edition + }_${toBasicISOString(new Date())}`;
145-151: Disambiguate long-format filenames.Add a “-long” suffix to prevent confusion.
Apply:
- void download(`${baseFilename}.csv`, () => { + void download(`${baseFilename}-long.csv`, () => { ... - void download(`${baseFilename}.tsv`, () => { + void download(`${baseFilename}-long.tsv`, () => {Also applies to: 168-174
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (2)
95-95: Recommended: RemoveuseEffectspy or clarify intent.Spying on
React.useEffectand immediately invoking it doesn't match React's actual lifecycle (effects run after render, asynchronously). If you're trying to force effects to run synchronously for testing, consider usingwaitFororrenderHookfrom@testing-library/reactinstead.If using
renderHook:- vi.spyOn(React, 'useEffect').mockImplementation((fn) => fn()); - const { dl, records } = useInstrumentVisualization({ + const { result } = renderHook(() => useInstrumentVisualization({ params: { subjectId: 'testId' } - }); + })); + const { dl, records } = result.current;Also applies to: 111-111, 127-127, 143-143, 159-159
88-174: Optional: Expand test coverage with edge cases.Current tests use minimal mock data (single record, simple values). Consider adding tests for:
- Multiple records
- Records with nested objects and arrays (tested in
makeLongRows)- Empty records array (error handling)
- Missing instrument (error handling)
This would increase confidence that the export logic handles real-world data correctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/web/package.json(1 hunks)apps/web/src/__mocks__/zustand.ts(1 hunks)apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts(1 hunks)apps/web/src/hooks/useInstrumentVisualization.ts(3 hunks)apps/web/src/routes/_app/datahub/$subjectId/table.tsx(1 hunks)apps/web/vitest.config.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
useInstrumentVisualization(27-203)
🔇 Additional comments (2)
apps/web/src/routes/_app/datahub/$subjectId/table.tsx (1)
41-41: Download options expansion looks good.Matches hook union types and aligns with PR goals.
apps/web/vitest.config.ts (1)
8-12: Alias addition is correct.Clean, conventional '@' -> src mapping. No issues.
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts
Outdated
Show resolved
Hide resolved
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts
Outdated
Show resolved
Hide resolved
| const makeLongRows = () => { | ||
| const longRecord: { [key: string]: any }[] = []; | ||
|
|
||
| exportRecords.forEach((item) => { | ||
| let date: Date; | ||
|
|
||
| Object.entries(item).forEach(([objKey, objVal]) => { | ||
| if (objKey === '__date__') { | ||
| date = objVal as Date; | ||
| return; | ||
| } | ||
|
|
||
| if (Array.isArray(objVal)) { | ||
| objVal.forEach((arrayItem) => { | ||
| Object.entries(arrayItem as object).forEach(([arrKey, arrItem]) => { | ||
| longRecord.push({ | ||
| Date: toBasicISOString(date), | ||
| SubjectID: params.subjectId, | ||
| Variable: `${objKey}-${arrKey}`, | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, perfectionist/sort-objects | ||
| Value: arrItem | ||
| }); | ||
| }); | ||
| }); | ||
| } else { | ||
| longRecord.push({ | ||
| Date: toBasicISOString(date), | ||
| SubjectID: params.subjectId, | ||
| Value: objVal, | ||
| Variable: objKey | ||
| }); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| return longRecord; | ||
| }; |
There was a problem hiding this comment.
Bug: date depends on object iteration order; can be undefined.
Don’t rely on encountering 'date' first. Read it once, then iterate the rest.
Apply:
- const makeLongRows = () => {
- const longRecord: { [key: string]: any }[] = [];
-
- exportRecords.forEach((item) => {
- let date: Date;
-
- Object.entries(item).forEach(([objKey, objVal]) => {
- if (objKey === '__date__') {
- date = objVal as Date;
- return;
- }
- if (Array.isArray(objVal)) {
- objVal.forEach((arrayItem) => {
- Object.entries(arrayItem as object).forEach(([arrKey, arrItem]) => {
- longRecord.push({
- Date: toBasicISOString(date),
- SubjectID: params.subjectId,
- Variable: `${objKey}-${arrKey}`,
- // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, perfectionist/sort-objects
- Value: arrItem
- });
- });
- });
- } else {
- longRecord.push({
- Date: toBasicISOString(date),
- SubjectID: params.subjectId,
- Value: objVal,
- Variable: objKey
- });
- }
- });
- });
-
- return longRecord;
- };
+ const makeLongRows = () => {
+ return exportRecords.flatMap((item) => {
+ const date = (item as any).__date__ as Date;
+ const base = { Date: toBasicISOString(date), SubjectID: params.subjectId };
+ return Object.entries(item)
+ .filter(([k]) => k !== '__date__')
+ .flatMap(([objKey, objVal]) => {
+ if (Array.isArray(objVal)) {
+ return (objVal as any[]).flatMap((arrayItem) =>
+ Object.entries(arrayItem as object).map(([arrKey, arrItem]) => ({
+ ...base,
+ Variable: `${objKey}-${arrKey}`,
+ // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, perfectionist/sort-objects
+ Value: arrItem
+ }))
+ );
+ }
+ return [
+ {
+ ...base,
+ Variable: objKey,
+ // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, perfectionist/sort-objects
+ Value: objVal
+ }
+ ];
+ });
+ });
+ };📝 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 makeLongRows = () => { | |
| const longRecord: { [key: string]: any }[] = []; | |
| exportRecords.forEach((item) => { | |
| let date: Date; | |
| Object.entries(item).forEach(([objKey, objVal]) => { | |
| if (objKey === '__date__') { | |
| date = objVal as Date; | |
| return; | |
| } | |
| if (Array.isArray(objVal)) { | |
| objVal.forEach((arrayItem) => { | |
| Object.entries(arrayItem as object).forEach(([arrKey, arrItem]) => { | |
| longRecord.push({ | |
| Date: toBasicISOString(date), | |
| SubjectID: params.subjectId, | |
| Variable: `${objKey}-${arrKey}`, | |
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, perfectionist/sort-objects | |
| Value: arrItem | |
| }); | |
| }); | |
| }); | |
| } else { | |
| longRecord.push({ | |
| Date: toBasicISOString(date), | |
| SubjectID: params.subjectId, | |
| Value: objVal, | |
| Variable: objKey | |
| }); | |
| } | |
| }); | |
| }); | |
| return longRecord; | |
| }; | |
| const makeLongRows = () => { | |
| return exportRecords.flatMap((item) => { | |
| const date = (item as any).__date__ as Date; | |
| const base = { Date: toBasicISOString(date), SubjectID: params.subjectId }; | |
| return Object.entries(item) | |
| .filter(([k]) => k !== '__date__') | |
| .flatMap(([objKey, objVal]) => { | |
| if (Array.isArray(objVal)) { | |
| return (objVal as any[]).flatMap((arrayItem) => | |
| Object.entries(arrayItem as object).map(([arrKey, arrItem]) => ({ | |
| ...base, | |
| Variable: `${objKey}-${arrKey}`, | |
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, perfectionist/sort-objects | |
| Value: arrItem | |
| })) | |
| ); | |
| } | |
| return [ | |
| { | |
| ...base, | |
| Variable: objKey, | |
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, perfectionist/sort-objects | |
| Value: objVal | |
| } | |
| ]; | |
| }); | |
| }); | |
| }; |
🤖 Prompt for AI Agents
In apps/web/src/hooks/useInstrumentVisualization.ts around lines 86 to 122, the
code assumes '__date__' will be encountered before other keys while iterating an
object, which can leave the date undefined; first read item['__date__'] into a
local Date variable (with proper typing and a fallback/guard if missing), then
iterate the rest of the entries skipping '__date__' to build longRecord; ensure
you don't rely on iteration order and handle the case where the date is absent
by either continuing/throwing/logging as appropriate.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (3)
16-22: Past critical issue resolved.The previous review flagged
__date__usinggetDate(), but the code now correctly usesnew Date()to return a Date object. This issue has been addressed.
28-35: Past major issue not resolved.The previous review identified that the mock structure is inconsistent and uses wrong types. The current code still has:
useAppStorereturning{ store: { ... } }instead of being called with a selectorcurrentGroupas a string instead of an object withidpropertycurrentUseras a string instead of an object withusernamepropertyThis mismatch will cause the hook to fail when accessing
currentGroup?.idorcurrentUser!.username.Also applies to: 56-58
66-74: Past major issue not resolved.The previous review identified that mocking React internals is fragile and recommended using
renderHookfrom@testing-library/react. The current code still mocksuseState,useEffect, anduseMemo, and line 72 returns a string'setRecords'instead of a function. This prevents proper testing of state management and lifecycle behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/src/__mocks__/zustand.ts(1 hunks)apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/mocks/zustand.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
useInstrumentVisualization(27-203)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (1)
56-64: Major: Avoid mocking React internals; use @testing-library/react utilities.This issue was previously flagged but remains unaddressed. Mocking
useState,useEffect, anduseMemodirectly is fragile and bypasses actual hook behavior. Line 62 returns'setRecords'(a string) instead of a function, and the mock prevents proper state updates. This approach hides bugs related to state management and lifecycle.Instead, use
renderHookfrom@testing-library/reactto test the hook in a realistic environment:+import { renderHook } from '@testing-library/react'; + // Remove the React mocking entirely (lines 56-64) -vi.mock('react', async (importOriginal) => { - const actual = await importOriginal<typeof React>(); - return { - ...actual, - useEffect: vi.fn(), - useMemo: vi.fn(), - useState: vi.fn(() => [mockRecords, 'setRecords']) - }; -}); // In each test, replace direct hook calls with renderHook: -const { dl, records } = useInstrumentVisualization({ - params: { subjectId: 'testId' } -}); +const { result } = renderHook(() => useInstrumentVisualization({ + params: { subjectId: 'testId' } +})); -act(() => dl('CSV')); +act(() => result.current.dl('CSV')); -expect(records).toBeDefined(); +expect(result.current.records).toBeDefined();This tests actual hook behavior including state updates and effects.
🧹 Nitpick comments (2)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (2)
78-159: Consider parameterizing repetitive test cases.The five test suites (CSV, TSV, CSV Long, TSV Long, JSON) follow an identical pattern and differ only in format names, file extensions, delimiters, and expected content. A parameterized test structure would reduce duplication and improve maintainability.
Example refactor using
it.each:describe('Download formats', () => { it.each([ { format: 'CSV', extension: '.csv', expectedContent: 'subjectId,Date,someValue\r\ntestId,2025-04-30,abc' }, { format: 'TSV', extension: '.tsv', expectedContent: 'subjectId\tDate\tsomeValue\r\ntestId\t2025-04-30\tabc' }, { format: 'CSV Long', extension: '.csv', expectedContent: 'Date,SubjectID,Value,Variable\r\n2025-04-30,testId,abc,someValue' }, { format: 'TSV Long', extension: '.tsv', expectedContent: 'Date\tSubjectID\tValue\tVariable\r\n2025-04-30\ttestId\tabc\tsomeValue' } ])('should download $format format', ({ format, extension, expectedContent }) => { const { dl, records } = useInstrumentVisualization({ params: { subjectId: 'testId' } }); act(() => dl(format as any)); expect(records).toBeDefined(); expect(mockUseDownload).toHaveBeenCalledTimes(1); const [filename, getContentFn] = mockUseDownload.mock.calls[0]; expect(filename).toContain(extension); const contents = getContentFn(); expect(contents).toMatch(expectedContent); }); it('should download JSON format', async () => { // JSON test remains separate due to async content retrieval // ... existing JSON test logic }); });
78-159: Consider adding error path tests.The current tests only verify successful downloads. The hook includes error handling for missing instruments and empty records (notifications at lines 57 and 60 in useInstrumentVisualization.ts), but these paths aren't tested.
Add tests for error cases:
describe('Error handling', () => { it('should notify when no instrument is selected', () => { const mockAddNotification = vi.fn(); vi.mocked(useNotificationsStore).mockReturnValue({ addNotification: mockAddNotification }); // Mock useInstrument to return null mockUseInstrument.mockReturnValueOnce(null); const { dl } = useInstrumentVisualization({ params: { subjectId: 'testId' } }); act(() => dl('CSV')); expect(mockAddNotification).toHaveBeenCalledWith({ message: 'errors.noInstrumentSelected', type: 'error' }); expect(mockUseDownload).not.toHaveBeenCalled(); }); it('should notify when records are empty', () => { // Mock useState to return empty records // ... similar test for empty records case }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
useInstrumentVisualization(27-203)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts
Outdated
Show resolved
Hide resolved
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts
Outdated
Show resolved
Hide resolved
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts
Outdated
Show resolved
Hide resolved
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (4)
1-3: Optional: Fix import ordering per ESLint rules.Static analysis flags multiple sorting violations. Apply this diff:
-import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { renderHook, act } from '@testing-library/react'; +import { act, renderHook } from '@testing-library/react'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + import { useInstrumentVisualization } from '../useInstrumentVisualization';
28-33: Optional: Reorder object keys per ESLint rules.Static analysis flags key ordering. Apply this diff:
data: [ { - date: new Date(), computedMeasures: {}, + date: new Date(), data: { someValue: 'abc' } } ]
79-79: Optional: Use.toBe()for exact string matching.Lines 79, 93, 107, and 121 use
.toMatch()with exact strings. Since.toMatch()is for regex patterns,.toBe()is more semantically appropriate for deterministic exact matches.Example for line 79:
- expect(csvContents).toMatch('subjectId,Date,someValue\r\ntestId,2025-04-30,abc'); + expect(csvContents).toBe('subjectId,Date,someValue\r\ntestId,2025-04-30,abc');Apply similar changes to lines 93, 107, and 121.
Also applies to: 93-93, 107-107, 121-121
63-138: Optional: Consider adding error case tests.Tests currently cover only happy paths. Consider adding tests for error scenarios:
- No instrument selected (instrument is null)
- No records available (records.length === 0)
This would verify that the hook properly calls
notifications.addNotification()with appropriate error messages.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
useInstrumentVisualization(27-203)
🪛 ESLint
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts
[error] 1-1: Expected "expect" to come before "it".
(perfectionist/sort-named-imports)
[error] 1-1: Expected "beforeEach" to come before "vi".
(perfectionist/sort-named-imports)
[error] 2-2: Expected "@testing-library/react" to come before "vitest".
(perfectionist/sort-imports)
[error] 2-2: Expected "act" to come before "renderHook".
(perfectionist/sort-named-imports)
[error] 3-3: Missed spacing between "@testing-library/react" and "../useInstrumentVisualization".
(perfectionist/sort-imports)
[error] 31-31: Expected "computedMeasures" to come before "date".
(perfectionist/sort-objects)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
useInstrumentVisualization(27-203)
🪛 ESLint
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts
[error] 1-1: Expected "expect" to come before "it".
(perfectionist/sort-named-imports)
[error] 1-1: Expected "beforeEach" to come before "vi".
(perfectionist/sort-named-imports)
[error] 2-2: Expected "@testing-library/react" to come before "vitest".
(perfectionist/sort-imports)
[error] 2-2: Expected "act" to come before "renderHook".
(perfectionist/sort-named-imports)
[error] 3-3: Missed spacing between "@testing-library/react" and "../useInstrumentVisualization".
(perfectionist/sort-imports)
[error] 4-4: Expected "@douglasneuroinformatics/libjs" (external) to come before "../useInstrumentVisualization" (parent).
(perfectionist/sort-imports)
[error] 20-20: 'mockBasicIsoString' is assigned a value but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
[error] 32-32: Expected "computedMeasures" to come before "date".
(perfectionist/sort-objects)
🔇 Additional comments (1)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (1)
60-140: Good test coverage for all export formats.The suite systematically tests all 5 download formats with consistent structure. Each test verifies the download is triggered, correct file extension is used, and content matches expected format. Mock cleanup in
beforeEachensures test isolation.
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (1)
1-4: Optional: Fix import ordering.Static analysis flags several import ordering issues per the
perfectionistrule. Consider running auto-fix if this matters to the project.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
useInstrumentVisualization(27-203)
🪛 ESLint
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts
[error] 1-1: Expected "expect" to come before "it".
(perfectionist/sort-named-imports)
[error] 1-1: Expected "beforeEach" to come before "vi".
(perfectionist/sort-named-imports)
[error] 2-2: Expected "@testing-library/react" to come before "vitest".
(perfectionist/sort-imports)
[error] 2-2: Expected "act" to come before "renderHook".
(perfectionist/sort-named-imports)
[error] 3-3: Missed spacing between "@testing-library/react" and "../useInstrumentVisualization".
(perfectionist/sort-imports)
[error] 4-4: Expected "@douglasneuroinformatics/libjs" (external) to come before "../useInstrumentVisualization" (parent).
(perfectionist/sort-imports)
[error] 30-30: Expected "computedMeasures" to come before "date".
(perfectionist/sort-objects)
🔇 Additional comments (2)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (2)
36-56: LGTM on mock implementations.All mocks are properly structured and return the correct shapes expected by the hook. The selector pattern for
useAppStoreand the return values for notification/translation hooks are correct.
58-138: Excellent test coverage and structure.The test suite comprehensively covers all five download formats with proper use of
renderHookandact. Each test correctly validates both filename extension and content format. ThebeforeEachhook ensures proper test isolation.
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (2)
1-4: Fix import ordering per project ESLint rules.The
perfectionist/sort-importsandperfectionist/sort-named-importsrules are enabled in your project. Reorder imports alphabetically.Apply this diff:
-import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { renderHook, act } from '@testing-library/react'; +import { act, renderHook } from '@testing-library/react'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + import { useInstrumentVisualization } from '../useInstrumentVisualization'; + import { toBasicISOString } from '@douglasneuroinformatics/libjs';
59-139: Test coverage looks solid for the happy path.All five download formats are tested with correct assertions. The use of
FIXED_TEST_DATEensures deterministic results.Optional: Consider adding tests for error paths (no instrument selected, no data to export) in a future PR to increase coverage of the notification logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
useInstrumentVisualization(27-203)
🪛 ESLint
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts
[error] 1-1: Expected "expect" to come before "it".
(perfectionist/sort-named-imports)
[error] 1-1: Expected "beforeEach" to come before "vi".
(perfectionist/sort-named-imports)
[error] 2-2: Expected "@testing-library/react" to come before "vitest".
(perfectionist/sort-imports)
[error] 2-2: Expected "act" to come before "renderHook".
(perfectionist/sort-named-imports)
[error] 3-3: Missed spacing between "@testing-library/react" and "../useInstrumentVisualization".
(perfectionist/sort-imports)
[error] 4-4: Expected "@douglasneuroinformatics/libjs" (external) to come before "../useInstrumentVisualization" (parent).
(perfectionist/sort-imports)
[error] 31-31: Expected "computedMeasures" to come before "date".
(perfectionist/sort-objects)
2 participants
Works on issue #1206
inserts user ID and experiment date as columns in download
properly parses exportRecords with papa parse
CSV and TSV long format downloads
tests to confirm download contents in all formats completed
Summary by CodeRabbit
New Features
Tests
Chores