feat: Dataview.Timeline component#844
Open
rohanchkrabrty wants to merge 3 commits into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughAdds a reusable Sequence Diagram(s)sequenceDiagram
participant Page
participant DataViewTimeline
participant OrdersAPI
participant DetailsDialog
Page->>DataViewTimeline: Render orders for visible range
DataViewTimeline->>Page: Emit visible date bounds
Page->>OrdersAPI: Fetch missing month buckets
OrdersAPI-->>Page: Return generated orders
Page->>DataViewTimeline: Render accumulated orders
DataViewTimeline->>DetailsDialog: Open selected order
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
apps/www/src/app/examples/timeline/page.tsx (1)
531-562: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard
loadWindow's async callback against unmount.
fetchOrdersApi(...).then(...)callssetOrders/setIsLoadingunconditionally. If the page unmounts while the simulated 500ms latency is in flight, these fire post-unmount. Harmless in React 18+ (silently no-ops), but worth the idiomatic guard since this page is a public usage example other engineers will copy.♻️ Suggested guard
+ const mountedRef = useRef(true); + useEffect(() => () => { mountedRef.current = false; }, []); + const loadWindow = useCallback((fromMs: number, toMs: number) => { const clampedFrom = Math.max(fromMs, Date.parse(RANGE[0])); const clampedTo = Math.min(toMs, Date.parse(RANGE[1])); if (clampedFrom >= clampedTo) return; const missing = monthKeysBetween(clampedFrom, clampedTo).filter( key => !requestedRef.current.has(key) ); if (missing.length === 0) return; for (const key of missing) requestedRef.current.add(key); inflightRef.current += 1; setIsLoading(true); fetchOrdersApi(missing).then(rows => { + if (!mountedRef.current) return; setOrders(prev => [...prev, ...rows]); inflightRef.current -= 1; if (inflightRef.current === 0) setIsLoading(false); }); }, []);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/www/src/app/examples/timeline/page.tsx` around lines 531 - 562, Guard the async callback in loadWindow with a mounted-state ref that is set false during component cleanup, and skip setOrders/setIsLoading after unmount while preserving inflight bookkeeping as appropriate. Apply the guard specifically to the fetchOrdersApi(...).then(...) path and initialize the ref within Page.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/raystack/components/data-view/__tests__/timeline.test.tsx`:
- Around line 654-667: Update the `fires onVisibleRangeChange with the visible
time window` test to assert that `to.getTime()` also equals the Jan 1, 2025
timestamp, matching the existing comment that both viewport edges resolve to t0;
leave the existing `from` assertion unchanged.
In `@packages/raystack/components/data-view/components/timeline.tsx`:
- Around line 275-287: The lane-packing input in the useMemo block must not
treat auto-width point cards as MIN_RENDER_WIDTH intervals when endField is
absent. Provide measurable fixed-width geometry for point cards or constrain
their wrapper before calling packLanes, ensuring packed items cannot overlap
later cards on the same lane.
- Around line 567-573: Update the visible-range useEffect callback to first
verify that the timeline is the active view before invoking
onVisibleRangeChange. Preserve the existing viewport and callback guards and
date-range calculation, but prevent retained viewport changes from triggering
callbacks while the timeline is hidden.
- Around line 175-186: Make the initial day anchor deterministic across SSR and
hydration: update todayTime’s today === true path and the fallback anchor below
to use a fixed server snapshot or defer timezone-dependent date calculation
until after hydration. Preserve the existing today === false behavior and
explicit today timestamp handling.
- Around line 752-765: The timeline card wrapper in the render path around
renderCard must be keyboard-activatable whenever onRowClick is provided. Add
appropriate focusability and Enter/Space key handling that invokes onRowClick
with item.row.original, while preserving non-clickable behavior and ensuring
nested controls remain usable.
- Around line 135-142: Update the row extraction in the timeline component to
use rowModel.flatRows instead of rowModel.rows before filtering leaf rows.
Preserve the existing subRows-based filter so group headers remain excluded
while nested leaf rows are included.
---
Nitpick comments:
In `@apps/www/src/app/examples/timeline/page.tsx`:
- Around line 531-562: Guard the async callback in loadWindow with a
mounted-state ref that is set false during component cleanup, and skip
setOrders/setIsLoading after unmount while preserving inflight bookkeeping as
appropriate. Apply the guard specifically to the fetchOrdersApi(...).then(...)
path and initialize the ref within Page.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8fb8ac3a-c71b-4a24-927b-7cd65bf0eab9
📒 Files selected for processing (10)
apps/www/src/app/examples/timeline/page.tsxpackages/raystack/components/data-view/__tests__/timeline.test.tsxpackages/raystack/components/data-view/components/timeline.tsxpackages/raystack/components/data-view/data-view.module.csspackages/raystack/components/data-view/data-view.tsxpackages/raystack/components/data-view/data-view.types.tsxpackages/raystack/components/data-view/index.tspackages/raystack/components/data-view/utils/pack-lanes.tsxpackages/raystack/components/data-view/utils/time-scale.tsxpackages/raystack/index.tsx
Comment on lines
+654
to
+667
| it('fires onVisibleRangeChange with the visible time window', async () => { | ||
| const onVisibleRangeChange = vi.fn(); | ||
| renderTimeline({ onVisibleRangeChange }); | ||
| await act(async () => { | ||
| await new Promise(resolve => setTimeout(resolve, 0)); | ||
| }); | ||
| expect(onVisibleRangeChange).toHaveBeenCalled(); | ||
| const calls = onVisibleRangeChange.mock.calls; | ||
| const [from, to] = calls[calls.length - 1][0]; | ||
| expect(from).toBeInstanceOf(Date); | ||
| expect(to).toBeInstanceOf(Date); | ||
| // jsdom viewport is 0-wide at scrollLeft 0 → both edges sit at t0 (Jan 1). | ||
| expect(from.getTime()).toBe(dayjs('2025-01-01').valueOf()); | ||
| }); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert to as well, per the test's own comment.
The comment states "both edges sit at t0 (Jan 1)" but only from.getTime() is checked; to.getTime() is left unverified despite being just as cheap to assert.
✅ Suggested addition
expect(from.getTime()).toBe(dayjs('2025-01-01').valueOf());
+ expect(to.getTime()).toBe(dayjs('2025-01-01').valueOf());📝 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.
Suggested change
| it('fires onVisibleRangeChange with the visible time window', async () => { | |
| const onVisibleRangeChange = vi.fn(); | |
| renderTimeline({ onVisibleRangeChange }); | |
| await act(async () => { | |
| await new Promise(resolve => setTimeout(resolve, 0)); | |
| }); | |
| expect(onVisibleRangeChange).toHaveBeenCalled(); | |
| const calls = onVisibleRangeChange.mock.calls; | |
| const [from, to] = calls[calls.length - 1][0]; | |
| expect(from).toBeInstanceOf(Date); | |
| expect(to).toBeInstanceOf(Date); | |
| // jsdom viewport is 0-wide at scrollLeft 0 → both edges sit at t0 (Jan 1). | |
| expect(from.getTime()).toBe(dayjs('2025-01-01').valueOf()); | |
| }); | |
| it('fires onVisibleRangeChange with the visible time window', async () => { | |
| const onVisibleRangeChange = vi.fn(); | |
| renderTimeline({ onVisibleRangeChange }); | |
| await act(async () => { | |
| await new Promise(resolve => setTimeout(resolve, 0)); | |
| }); | |
| expect(onVisibleRangeChange).toHaveBeenCalled(); | |
| const calls = onVisibleRangeChange.mock.calls; | |
| const [from, to] = calls[calls.length - 1][0]; | |
| expect(from).toBeInstanceOf(Date); | |
| expect(to).toBeInstanceOf(Date); | |
| // jsdom viewport is 0-wide at scrollLeft 0 → both edges sit at t0 (Jan 1). | |
| expect(from.getTime()).toBe(dayjs('2025-01-01').valueOf()); | |
| expect(to.getTime()).toBe(dayjs('2025-01-01').valueOf()); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/raystack/components/data-view/__tests__/timeline.test.tsx` around
lines 654 - 667, Update the `fires onVisibleRangeChange with the visible time
window` test to assert that `to.getTime()` also equals the Jan 1, 2025
timestamp, matching the existing comment that both viewport edges resolve to t0;
leave the existing `from` assertion unchanged.
Comment on lines
+135
to
+142
| const rowModel = table?.getRowModel(); | ||
| const { rows = [] } = rowModel || {}; | ||
|
|
||
| // Timeline bypasses group headers (RFC §Grouping) — position leaf rows only. | ||
| const leafRows = useMemo( | ||
| () => rows.filter(row => !(row.subRows && row.subRows.length > 0)), | ||
| [rows] | ||
| ); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the target file and inspect the relevant section.
git ls-files 'packages/raystack/components/data-view/components/timeline.tsx'
wc -l packages/raystack/components/data-view/components/timeline.tsx
sed -n '1,240p' packages/raystack/components/data-view/components/timeline.tsx
# Search for row-model usage and TanStack table version hints.
rg -n "flatRows|getRowModel|subRows|grouped" packages/raystack/components -S
rg -n "tanstack|`@tanstack/react-table`|`@tanstack/table-core`" package.json packages -SRepository: raystack/apsara
Length of output: 23245
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- timeline.tsx around row selection ---'
sed -n '120,220p' packages/raystack/components/data-view/components/timeline.tsx
echo '--- data-view.tsx table setup ---'
sed -n '120,260p' packages/raystack/components/data-view/data-view.tsx
echo '--- utils row-count comment and helpers ---'
sed -n '290,340p' packages/raystack/components/data-view/utils/index.tsx
echo '--- any explicit use of filterFromLeafRows / getExpandedRowModel / grouping setup ---'
rg -n "filterFromLeafRows|getExpandedRowModel|getGroupedRowModel|getFilteredRowModel|getPaginationRowModel|manualGrouping|grouping:" packages/raystack/components/data-view packages/raystack/components/data-table -SRepository: raystack/apsara
Length of output: 11308
Use rowModel.flatRows here packages/raystack/components/data-view/components/timeline.tsx:135-142
rowModel.rows only contains the top-level grouped rows, so filtering it drops nested leaf rows and grouped timelines can render nothing. flatRows keeps the leaves while still letting you skip group headers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/raystack/components/data-view/components/timeline.tsx` around lines
135 - 142, Update the row extraction in the timeline component to use
rowModel.flatRows instead of rowModel.rows before filtering leaf rows. Preserve
the existing subRows-based filter so group headers remain excluded while nested
leaf rows are included.
Comment on lines
+175
to
+186
| const todayTime = useMemo(() => { | ||
| if (today === false) return null; | ||
| if (today === true) { | ||
| // Day precision: aligns the line with its day tick and keeps SSR and | ||
| // client renders consistent (no per-ms Date.now() drift → no hydration | ||
| // mismatch). | ||
| const now = new Date(); | ||
| now.setHours(0, 0, 0, 0); | ||
| return now.getTime(); | ||
| } | ||
| return toTimestamp(today); | ||
| }, [today]); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/raystack/components/data-view/components/timeline.tsx"
echo "== outline =="
ast-grep outline "$FILE" --view expanded || true
echo
echo "== lines 150-240 =="
sed -n '150,240p' "$FILE" | cat -nRepository: raystack/apsara
Length of output: 4513
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/raystack/components/data-view/components/timeline.tsx"
echo "== top of file =="
sed -n '1,80p' "$FILE" | cat -n
echo
echo "== lines 210-235 =="
sed -n '210,235p' "$FILE" | cat -n
echo
echo "== search for 'use client' and hydration-related logic =="
rg -n --fixed-strings "'use client'" "$FILE" || true
rg -n "hydrate|hydration|client render|server render|window|document" "$FILE" || trueRepository: raystack/apsara
Length of output: 4799
Derive the initial day anchor deterministically
new Date().setHours(0, 0, 0, 0) runs during SSR too, so a server in a different timezone can render a different domain and shift the timeline on hydration. Use a fixed server snapshot or defer the anchor until after hydration here, and in the fallback anchor below.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/raystack/components/data-view/components/timeline.tsx` around lines
175 - 186, Make the initial day anchor deterministic across SSR and hydration:
update todayTime’s today === true path and the fallback anchor below to use a
fixed server snapshot or defer timezone-dependent date calculation until after
hydration. Preserve the existing today === false behavior and explicit today
timestamp handling.
Comment on lines
+275
to
+287
| const { lanes, laneCount } = useMemo(() => { | ||
| if (lanePacking === 'one-per-row') { | ||
| return { | ||
| lanes: positioned.map((_, i) => i), | ||
| laneCount: positioned.length | ||
| }; | ||
| } | ||
| return packLanes( | ||
| positioned.map(item => ({ | ||
| x: item.x, | ||
| width: item.renderWidth ?? MIN_RENDER_WIDTH | ||
| })) | ||
| ); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not pack auto-width point cards as 24px intervals.
Without endField, the wrapper is content-sized, but lane collisions assume MIN_RENDER_WIDTH. A point card wider than 24px can therefore overlap a later card assigned to the same lane. Supply measurable/fixed point-card width geometry or constrain the wrapper before packing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/raystack/components/data-view/components/timeline.tsx` around lines
275 - 287, The lane-packing input in the useMemo block must not treat auto-width
point cards as MIN_RENDER_WIDTH intervals when endField is absent. Provide
measurable fixed-width geometry for point cards or constrain their wrapper
before calling packLanes, ensuring packed items cannot overlap later cards on
the same lane.
Comment on lines
+567
to
+573
| useEffect(() => { | ||
| if (!onVisibleRangeChange || !viewport) return; | ||
| onVisibleRangeChange([ | ||
| new Date(timeScale.timeAt(viewport.left)), | ||
| new Date(timeScale.timeAt(viewport.left + viewport.width)) | ||
| ]); | ||
| }, [onVisibleRangeChange, viewport, timeScale]); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Gate visible-range callbacks on the active view.
After a timeline has been active, its retained viewport can trigger callbacks while hidden whenever its scale or callback identity changes—potentially initiating pagination for an inactive view.
Proposed fix
useEffect(() => {
- if (!onVisibleRangeChange || !viewport) return;
+ if (!isActive || !onVisibleRangeChange || !viewport) return;
onVisibleRangeChange([
new Date(timeScale.timeAt(viewport.left)),
new Date(timeScale.timeAt(viewport.left + viewport.width))
]);
- }, [onVisibleRangeChange, viewport, timeScale]);
+ }, [isActive, onVisibleRangeChange, viewport, timeScale]);📝 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.
Suggested change
| useEffect(() => { | |
| if (!onVisibleRangeChange || !viewport) return; | |
| onVisibleRangeChange([ | |
| new Date(timeScale.timeAt(viewport.left)), | |
| new Date(timeScale.timeAt(viewport.left + viewport.width)) | |
| ]); | |
| }, [onVisibleRangeChange, viewport, timeScale]); | |
| useEffect(() => { | |
| if (!isActive || !onVisibleRangeChange || !viewport) return; | |
| onVisibleRangeChange([ | |
| new Date(timeScale.timeAt(viewport.left)), | |
| new Date(timeScale.timeAt(viewport.left + viewport.width)) | |
| ]); | |
| }, [isActive, onVisibleRangeChange, viewport, timeScale]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/raystack/components/data-view/components/timeline.tsx` around lines
567 - 573, Update the visible-range useEffect callback to first verify that the
timeline is the active view before invoking onVisibleRangeChange. Preserve the
existing viewport and callback guards and date-range calculation, but prevent
retained viewport changes from triggering callbacks while the timeline is
hidden.
Comment on lines
+752
to
+765
| return ( | ||
| <div | ||
| role='listitem' | ||
| key={item.row.id} | ||
| className={cx( | ||
| styles.timelineCard, | ||
| onRowClick && styles.clickable, | ||
| classNames.card | ||
| )} | ||
| style={style} | ||
| data-collapsed={context.collapsed || undefined} | ||
| onClick={() => onRowClick?.(item.row.original)} | ||
| > | ||
| {renderCard(item.row, context)} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file structure first
ast-grep outline packages/raystack/components/data-view/components/timeline.tsx --view expanded
# Show the target area with line numbers
sed -n '700,830p' packages/raystack/components/data-view/components/timeline.tsx
# Search for any keyboard handlers or interactive wrappers in this file
rg -n "onKeyDown|onKeyUp|tabIndex|role='button'|role=\"button\"|button" packages/raystack/components/data-view/components/timeline.tsxRepository: raystack/apsara
Length of output: 3638
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the click/interaction context around the timeline cards
sed -n '430,475p' packages/raystack/components/data-view/components/timeline.tsx
# Find the prop types / callback definition for onRowClick and renderCard
rg -n "onRowClick|renderCard|TimelineCardContext|clickable" packages/raystack/components/data-view/components/timeline.tsxRepository: raystack/apsara
Length of output: 2238
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant lines around the interaction helpers
sed -n '430,475p' packages/raystack/components/data-view/components/timeline.tsx
# Locate the component's interaction-related identifiers
rg -n "onRowClick|renderCard|TimelineCardContext|clickable" packages/raystack/components/data-view/components/timeline.tsxRepository: raystack/apsara
Length of output: 2238
Add keyboard activation to clickable cards
When onRowClick is set, this wrapper is still a plain div, so the card is mouse-only and unreachable by keyboard. Add focusability plus Enter/Space handling, or switch to an interactive element that preserves nested controls.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/raystack/components/data-view/components/timeline.tsx` around lines
752 - 765, The timeline card wrapper in the render path around renderCard must
be keyboard-activatable whenever onRowClick is provided. Add appropriate
focusability and Enter/Space key handling that invokes onRowClick with
item.row.original, while preserving non-clickable behavior and ensuring nested
controls remain usable.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.