feat: dataview timeline component - #860
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughIntroduces Sequence Diagram(s)sequenceDiagram
participant User
participant DataViewTimeline
participant OrderPage
participant OrdersAPI
User->>DataViewTimeline: scroll timeline
DataViewTimeline->>OrderPage: report visible date range
OrderPage->>OrdersAPI: fetch missing month ranges
OrdersAPI-->>OrderPage: return orders
OrderPage-->>DataViewTimeline: merge loaded rows
User->>DataViewTimeline: select Today
DataViewTimeline->>DataViewTimeline: scrollTo today
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@apps/www/src/app/examples/timeline/page.tsx`:
- Around line 573-612: Update the fetch handling in loadWindow so rejected
requests always decrement inflightRef and clear isLoading when no runs remain,
while removing each failed month range from requestedRef to allow retries. Add
rejection handling without changing the existing successful order-merging
behavior.
In `@apps/www/src/content/docs/components/dataview/props.ts`:
- Around line 178-197: Make DataViewTimelineProps generic as
DataViewTimelineProps<TData>, and update renderCard to receive the TanStack
Row<TData> wrapper rather than raw row data. Keep the existing
TimelineCardContext parameter and ensure the interface consistently uses the
generic type for the callback.
🪄 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: 99b881ad-cfdb-4cc2-867c-91dbff8f72f5
📒 Files selected for processing (15)
apps/www/src/app/examples/timeline/page.tsxapps/www/src/components/dataview-demo.tsxapps/www/src/components/demo/demo.tsxapps/www/src/content/docs/components/dataview/demo.tsapps/www/src/content/docs/components/dataview/index.mdxapps/www/src/content/docs/components/dataview/props.tspackages/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
| export interface DataViewTimelineProps { | ||
| /** Multi-view name. When set, the renderer gates itself on the active view. */ | ||
| name?: string; | ||
|
|
||
| /** Optional view-scoped field override (full replacement). */ | ||
| fields?: DataViewField[]; | ||
|
|
||
| /** Accessor key on the row yielding the start date. Rows with a missing/invalid value are skipped. (Required) */ | ||
| startField: string; | ||
|
|
||
| /** Accessor key for the end date. Omitted → point markers; present → variable-width span cards. */ | ||
| endField?: string; | ||
|
|
||
| /** | ||
| * Renders the card interior. The Timeline owns positioning (x from start, | ||
| * width from span, lane from packing, scroll); the consumer owns the card | ||
| * visual entirely. Compose `DataView.DisplayAccess` inside for Display | ||
| * Properties support. (Required) | ||
| */ | ||
| renderCard: (row: T, context: TimelineCardContext) => ReactNode; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
DataViewTimelineProps uses T but is declared non-generic; renderCard row type is also inaccurate.
Two issues here:
DataViewTimelineProps(Line 178) declares no generic, butrenderCard: (row: T, ...)(Line 197) referencesT— same unbound-name concern asDataViewListColumn. The canonical type isDataViewTimelineProps<TData>.- Per the tests and the real contract,
renderCard's first arg is a TanStackRow<T>(consumers readrow.original), not the row dataT. Documenting it asrow: Twill mislead consumers.
🤖 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/content/docs/components/dataview/props.ts` around lines 178 -
197, Make DataViewTimelineProps generic as DataViewTimelineProps<TData>, and
update renderCard to receive the TanStack Row<TData> wrapper rather than raw row
data. Keep the existing TimelineCardContext parameter and ensure the interface
consistently uses the generic type for the callback.
rohilsurana
left a comment
rohilsurana
left a comment
There was a problem hiding this comment.
Review: DataView.Timeline (in progress)
Reviewing this outside-in over a few passes. Covered so far: the public API/types, the pure utils (time-scale, pack-lanes), and the component itself. Styles, the 52 tests, and the docs/demo are still to come.
Overall this is strong, careful work. The renderer contract mirrors DataView.List and the actionsRef pattern from tour-beta closely, so the API stays consistent with the rest of the library. The scroll/pan/momentum machinery and the scroll-anchoring on domain shift are thoughtfully done, hooks are all called before the early returns, and rAF throttling plus cleanup are handled properly.
Confirmed the data scrub this PR exists for: the only change from the previous PR is the demo mock data in examples/timeline/page.tsx, real org/file names are gone, and no proprietary terms remain in any of the 15 files or in the branch's own commits. Note this only protects main going forward; the earlier data still lives in the previous PR's branch and commits on the remote if that matters.
Inline notes below, tagged by severity. The only one I would treat as a real gate is the unitWidth <= 0 guard in time-scale.tsx. The rest are API/doc/a11y polish. Nothing here blocks the concept.
rohilsurana
left a comment
rohilsurana
left a comment
There was a problem hiding this comment.
Step 4: styles (data-view.module.css)
Checked the Timeline styles against house conventions. Tokens all resolve to real definitions in styles/colors.css and are the same ones the existing data-view CSS uses, so light/dark theming comes for free. The 0.5px borders match the codebase convention (used in ~31 component stylesheets). Layering, sticky axis/footer, and overscroll-behavior: contain are all sensible. Two low-priority notes inline.
rohilsurana
left a comment
rohilsurana
left a comment
There was a problem hiding this comment.
Step 5: tests (52 cases, __tests__/timeline.test.tsx)
This is a genuinely strong suite, well above what most feature PRs ship. It does not just assert "renders" — it checks exact geometry (px positions, widths, lane indices) and it exercises the hard, stateful machinery I was most worried about:
- momentum glide after a fling, plus wheel-cancels-glide, plus hold-still-then-release does not glide
- drag-to-pan on both axes, and correctly not panning from a card or a touch pointer
- binary-search horizontal culling under
virtualized(overscan window) - scroll anchoring: the left-edge time stays put when the domain is extended leftward
onVisibleRangeChangededup on identity churn, and re-fire on a real scroll- scroll-position restore when a gated view is switched away and back
- the memoized card wrapper: a hover-cursor root re-render does not re-render card interiors
- the full
actionsRefsurface, including no-op-with-warning while hidden
Two gaps worth closing, both inline:
quarterscale is untested — and it is the only scale with hand-rolled (non-dayjs) logic, so it is the highest-value missing case.- rAF is flushed with real timers, which makes the momentum/timing tests the most likely to flake on CI and slows the suite.
One more, tied to my step-2 note: the unitWidth <= 0 path (the potential hang) has no test because the guard does not exist yet. If the guard is added, a one-line test should come with it.
Net: coverage is not the concern here. The two items above are quality/robustness polish.
rohilsurana
left a comment
rohilsurana
left a comment
There was a problem hiding this comment.
Step 6: docs and demo (final pass)
The guide (index.mdx) is genuinely excellent: accurate to the real API (scale set, unitWidth defaults, minCardWidth, estimatedPointWidth, actionsRef, the range-window loading walkthrough), well-structured, and its code examples correctly use row.original. The TimelineCardContext/TimelineMarker/TimelineActions tables are wired up via auto-type-table, and the demo exports (timelinePreview, timelinePointPreview) resolve.
I also verified the two existing CodeRabbit findings rather than take them at face value:
props.ts"unbound T" (Major): false positive.Tis a deliberate= anyplaceholder (line 5). The only real sliver is thatrenderCard's arg is aRowwrapper; minor, and the guide already teachesrow.original. Details inline.page.tsxunhandled rejection (Critical): the code observation is right, but the mock never rejects, so it cannot happen here. Worth fixing only because the guide sells this as the recommended pattern and examples get copied. Downgraded to example-quality. Details inline.
Also worth noting for the author: the estimatedRowHeight doc here (line 263) already has to add "cards render at exactly this height" to undo the misleading name, which reinforces the naming point from my earlier types comment.
Overall (steps 1-6 complete)
This is strong, careful work: a well-designed API consistent with DataView.List and tour-beta, solid scroll/pan/momentum and scroll-anchoring, correct and optimal lane packing, a comprehensive 52-test suite, on-pattern theming, and excellent docs. The data scrub this PR exists for is complete and clean.
Nothing I found blocks the concept. In priority order, before merge I would want:
- the
unitWidth <= 0guard intime-scale.tsx(only issue that can hang the render); - a
quarter-scale test (the one untested hand-rolled path).
Everything else (the estimatedRowHeight name, renderCard/memo stability docs, keyboard a11y on the scroll region, the CSS 52px de-dup, marker-line z-order, real-timer test flake, and the example's fetch error handling) is polish the author can take or leave.
|
Ordering and grouping doesn't work in the timeline mode so it should be disabled in that view |
| const from = timeScale.timeAt(viewport.left); | ||
| const to = timeScale.timeAt(viewport.left + viewport.width); | ||
| const prev = lastNotifiedRangeRef.current; | ||
| if (prev && prev.from === from && prev.to === to) return; |
There was a problem hiding this comment.
The comment above says this dedupes "by ms window," but it's comparing raw decimals. When the domain shifts, the browser rounds the scroll position, so these never quite line up and the callback fires again — an extra fetch each time. Rounding both sides to whole ms before comparing would fix it.
| * Initial horizontal scroll target. | ||
| * @defaultValue "today" | ||
| */ | ||
| defaultScrollTo?: TimelineDateInput | 'today' | 'start' | 'end'; |
There was a problem hiding this comment.
the rendered props table shows just TimelineDateInput for defaultScrollTo, so readers can't tell that 'today' | 'start' | 'end' are accepted — they only show up in the prose.
Cause: auto-type-table resolves the type through the compiler, and since TimelineDateInput includes string, the literals get folded into it. Same for scrollTo's target (L331).
Runtime type is fine — this is purely cosmetic.
Simplest fix is to name the values in the JSDoc, which survives extraction:
/**
* Initial horizontal scroll target: a date, or `'today'` | `'start'` | `'end'`.
* @defaultValue "today"
*/
defaultScrollTo?: TimelineDateInput | 'today' | 'start' | 'end';
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/raystack/components/data-view/__tests__/timeline.test.tsx (2)
1202-1217: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
renderGroupedletsprops.classNamessilently disable thebandmarker.
...propsis spread afterclassNames, so any caller passingclassNamesdrops thegroupHeader: 'band'hook andbands()returns[]. Combined with thetoHaveLength(0)assertion at Line 1291, a class-wiring regression would pass vacuously. Merge the marker last (or assert on the module class like the slot query does).♻️ Proposed merge-order fix
return renderTimeline( - { classNames: { groupHeader: 'band' }, ...props }, + { + ...props, + classNames: { ...props.classNames, groupHeader: 'band' } + }, data,Also applies to: 1289-1292
🤖 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 1202 - 1217, Update the test helper renderGrouped so the required classNames.groupHeader: 'band' marker is merged after props, preventing caller-provided classNames from overriding it. Preserve existing custom props while ensuring bands() always targets the group headers, so the related length assertion cannot pass vacuously.
1134-1160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese tests only cover mount-time sort, not a sort change on a live timeline.
Both cases unmount and mount a fresh tree with a different
defaultSort, so the "layout unchanged" assertion in the second test compares two independent mounts and is trivially satisfied. The regression worth guarding is lane reassignment (and its absence underautopacking) when the sort flips on an already-mounted timeline — drive that through the DataView sort API (or arerenderthat changes the active sort) on a single instance.🤖 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 1134 - 1160, Update the timeline tests to change the active sort on a single mounted instance instead of unmounting and remounting with a different defaultSort. In the one-per-row case, assert lanes are reassigned after flipping sort; in the packed-layout case, capture lane/top positions before the change and verify they remain unchanged afterward. Use the existing DataView sort API or rerender path to drive the live sort update.
🤖 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 `@apps/www/src/components/dataview-demo.tsx`:
- Around line 905-908: Disable grouping in timeline mode by adding hideGrouping
to DataView.DisplayControls at apps/www/src/components/dataview-demo.tsx:905-908
and using hideOrdering hideGrouping in the primary preview at
apps/www/src/content/docs/components/dataview/demo.ts:357-359. Remove the
grouped timeline demos at apps/www/src/components/dataview-demo.tsx:938-980 and
apps/www/src/content/docs/components/dataview/demo.ts:382-419, then remove their
import at apps/www/src/components/demo/demo.tsx:56 and default-scope entry at
apps/www/src/components/demo/demo.tsx:93-95.
---
Nitpick comments:
In `@packages/raystack/components/data-view/__tests__/timeline.test.tsx`:
- Around line 1202-1217: Update the test helper renderGrouped so the required
classNames.groupHeader: 'band' marker is merged after props, preventing
caller-provided classNames from overriding it. Preserve existing custom props
while ensuring bands() always targets the group headers, so the related length
assertion cannot pass vacuously.
- Around line 1134-1160: Update the timeline tests to change the active sort on
a single mounted instance instead of unmounting and remounting with a different
defaultSort. In the one-per-row case, assert lanes are reassigned after flipping
sort; in the packed-layout case, capture lane/top positions before the change
and verify they remain unchanged afterward. Use the existing DataView sort API
or rerender path to drive the live sort update.
🪄 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 Plus
Run ID: dd292c02-1a48-493c-ac6a-0161fe67705e
📒 Files selected for processing (10)
apps/www/src/app/examples/timeline/page.tsxapps/www/src/components/dataview-demo.tsxapps/www/src/components/demo/demo.tsxapps/www/src/content/docs/components/dataview/demo.tsapps/www/src/content/docs/components/dataview/index.mdxapps/www/src/content/docs/components/dataview/props.tspackages/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.types.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/raystack/components/data-view/data-view.types.tsx
- packages/raystack/components/data-view/data-view.module.css
- apps/www/src/content/docs/components/dataview/props.ts
- packages/raystack/components/data-view/components/timeline.tsx
- apps/www/src/content/docs/components/dataview/index.mdx
- apps/www/src/app/examples/timeline/page.tsx
| {/* Sort can't move a card horizontally (x is time) and `auto` | ||
| packing is chronological, so Ordering is hidden. Grouping is | ||
| left in: it renders swim-lane sections — try Team or Status. */} | ||
| <DataView.DisplayControls hideOrdering /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Disable grouping everywhere in timeline mode.
The stated review feedback says grouping is currently incorrect for timelines, but these changes leave it selectable and add demos/docs that present it as supported.
apps/www/src/components/dataview-demo.tsx#L905-L908: passhideGroupingalongsidehideOrdering.apps/www/src/components/dataview-demo.tsx#L938-L980: remove or defer the grouped timeline demo until renderer support is corrected.apps/www/src/components/demo/demo.tsx#L56-L56: remove the grouped timeline demo import if the demo is removed.apps/www/src/components/demo/demo.tsx#L93-L95: remove the grouped timeline demo from the default scope.apps/www/src/content/docs/components/dataview/demo.ts#L357-L359: usehideOrdering hideGroupingin the primary preview.apps/www/src/content/docs/components/dataview/demo.ts#L382-L419: remove the grouped timeline preview until grouping works correctly.
📍 Affects 3 files
apps/www/src/components/dataview-demo.tsx#L905-L908(this comment)apps/www/src/components/dataview-demo.tsx#L938-L980apps/www/src/components/demo/demo.tsx#L56-L56apps/www/src/components/demo/demo.tsx#L93-L95apps/www/src/content/docs/components/dataview/demo.ts#L357-L359apps/www/src/content/docs/components/dataview/demo.ts#L382-L419
🤖 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/components/dataview-demo.tsx` around lines 905 - 908, Disable
grouping in timeline mode by adding hideGrouping to DataView.DisplayControls at
apps/www/src/components/dataview-demo.tsx:905-908 and using hideOrdering
hideGrouping in the primary preview at
apps/www/src/content/docs/components/dataview/demo.ts:357-359. Remove the
grouped timeline demos at apps/www/src/components/dataview-demo.tsx:938-980 and
apps/www/src/content/docs/components/dataview/demo.ts:382-419, then remove their
import at apps/www/src/components/demo/demo.tsx:56 and default-scope entry at
apps/www/src/components/demo/demo.tsx:93-95.
3 participants
Summary
DataView.Timeline, a renderer that positions the shared DataView row model as cards on a continuous, horizontally scrollable time axis — the card interior is fully consumer-owned viarenderCard(row, context), while the Timeline owns the time scale, lane packing, sticky two-tier axis (ticks, month/year bands, today + custom markers, gridlines), and native x/y scrolling with drag-to-pan, momentum glide, and a snapped hover cursor.rangefor a stable coordinate space,onVisibleRangeChange(rAF-throttled, deduped by ms window), and time-anchored scrolling so the content never jumps when the domain shifts; imperative navigation ships viaactionsRef(scrollTo,getVisibleRange).virtualized.estimatedPointWidthso content-sized cards don't overlap;rowHeightis renamed toestimatedRowHeightto matchDataView.List.