feat: dataview timeline component#860
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughIntroduces Sequence Diagram(s)sequenceDiagram
participant User
participant DataViewTimeline
participant TimelineActions
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->>TimelineActions: select Today
TimelineActions->>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
| 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); | ||
| // Coalesce consecutive missing months into contiguous [first, last] runs | ||
| // (jump-scrolling can leave already-fetched holes between them). | ||
| const runs: [string, string][] = []; | ||
| for (const key of missing) { | ||
| const last = runs[runs.length - 1]; | ||
| if ( | ||
| last && | ||
| dayjs(`${last[1]}-01`).add(1, 'month').format('YYYY-MM') === key | ||
| ) { | ||
| last[1] = key; | ||
| } else { | ||
| runs.push([key, key]); | ||
| } | ||
| } | ||
| inflightRef.current += runs.length; | ||
| setIsLoading(true); | ||
| for (const [firstKey, lastKey] of runs) { | ||
| fetchOrdersApi( | ||
| dayjs(`${firstKey}-01`).valueOf(), | ||
| dayjs(`${lastKey}-01`).endOf('month').valueOf() | ||
| ).then(rows => { | ||
| setOrders(prev => { | ||
| const seen = new Set(prev.map(order => order.id)); | ||
| const fresh = rows.filter(order => !seen.has(order.id)); | ||
| return fresh.length > 0 ? [...prev, ...fresh] : prev; | ||
| }); | ||
| inflightRef.current -= 1; | ||
| if (inflightRef.current === 0) setIsLoading(false); | ||
| }); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Unhandled fetch rejection permanently poisons requestedRef and stalls isLoading.
fetchOrdersApi(...).then(...) has no .catch()/.finally(). If the request rejects (the realistic case once this mock is swapped for a real API), the months were already added to requestedRef at line 581 before the fetch settles, so they're never retried, and inflightRef.current never decrements — leaving isLoading stuck true forever for that window.
🛡️ Proposed fix
for (const [firstKey, lastKey] of runs) {
fetchOrdersApi(
dayjs(`${firstKey}-01`).valueOf(),
dayjs(`${lastKey}-01`).endOf('month').valueOf()
- ).then(rows => {
- setOrders(prev => {
- const seen = new Set(prev.map(order => order.id));
- const fresh = rows.filter(order => !seen.has(order.id));
- return fresh.length > 0 ? [...prev, ...fresh] : prev;
- });
- inflightRef.current -= 1;
- if (inflightRef.current === 0) setIsLoading(false);
- });
+ )
+ .then(rows => {
+ setOrders(prev => {
+ const seen = new Set(prev.map(order => order.id));
+ const fresh = rows.filter(order => !seen.has(order.id));
+ return fresh.length > 0 ? [...prev, ...fresh] : prev;
+ });
+ })
+ .catch(error => {
+ console.error('Failed to load orders', error);
+ // Un-mark so a future scroll into this window retries the fetch.
+ for (const key of monthKeysBetween(
+ dayjs(`${firstKey}-01`).valueOf(),
+ dayjs(`${lastKey}-01`).endOf('month').valueOf()
+ )) {
+ requestedRef.current.delete(key);
+ }
+ })
+ .finally(() => {
+ inflightRef.current -= 1;
+ if (inflightRef.current === 0) setIsLoading(false);
+ });
}📝 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 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); | |
| // Coalesce consecutive missing months into contiguous [first, last] runs | |
| // (jump-scrolling can leave already-fetched holes between them). | |
| const runs: [string, string][] = []; | |
| for (const key of missing) { | |
| const last = runs[runs.length - 1]; | |
| if ( | |
| last && | |
| dayjs(`${last[1]}-01`).add(1, 'month').format('YYYY-MM') === key | |
| ) { | |
| last[1] = key; | |
| } else { | |
| runs.push([key, key]); | |
| } | |
| } | |
| inflightRef.current += runs.length; | |
| setIsLoading(true); | |
| for (const [firstKey, lastKey] of runs) { | |
| fetchOrdersApi( | |
| dayjs(`${firstKey}-01`).valueOf(), | |
| dayjs(`${lastKey}-01`).endOf('month').valueOf() | |
| ).then(rows => { | |
| setOrders(prev => { | |
| const seen = new Set(prev.map(order => order.id)); | |
| const fresh = rows.filter(order => !seen.has(order.id)); | |
| return fresh.length > 0 ? [...prev, ...fresh] : prev; | |
| }); | |
| inflightRef.current -= 1; | |
| if (inflightRef.current === 0) setIsLoading(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); | |
| // Coalesce consecutive missing months into contiguous [first, last] runs | |
| // (jump-scrolling can leave already-fetched holes between them). | |
| const runs: [string, string][] = []; | |
| for (const key of missing) { | |
| const last = runs[runs.length - 1]; | |
| if ( | |
| last && | |
| dayjs(`${last[1]}-01`).add(1, 'month').format('YYYY-MM') === key | |
| ) { | |
| last[1] = key; | |
| } else { | |
| runs.push([key, key]); | |
| } | |
| } | |
| inflightRef.current += runs.length; | |
| setIsLoading(true); | |
| for (const [firstKey, lastKey] of runs) { | |
| fetchOrdersApi( | |
| dayjs(`${firstKey}-01`).valueOf(), | |
| dayjs(`${lastKey}-01`).endOf('month').valueOf() | |
| ) | |
| .then(rows => { | |
| setOrders(prev => { | |
| const seen = new Set(prev.map(order => order.id)); | |
| const fresh = rows.filter(order => !seen.has(order.id)); | |
| return fresh.length > 0 ? [...prev, ...fresh] : prev; | |
| }); | |
| }) | |
| .catch(error => { | |
| console.error('Failed to load orders', error); | |
| // Un-mark so a future scroll into this window retries the fetch. | |
| for (const key of monthKeysBetween( | |
| dayjs(`${firstKey}-01`).valueOf(), | |
| dayjs(`${lastKey}-01`).endOf('month').valueOf() | |
| )) { | |
| requestedRef.current.delete(key); | |
| } | |
| }) | |
| .finally(() => { | |
| 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 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.
| 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.
| padUnits = 0, | ||
| minWidth = 0 | ||
| } = params; | ||
| const pxPerMs = unitWidth / TIMELINE_UNIT_MS[scale]; |
There was a problem hiding this comment.
[robustness] unitWidth is consumer-controllable (the unitWidth prop). If it is 0 or negative, pxPerMs becomes 0, and the viewport-fill step below computes Math.ceil(deficitPx / unitWidth) = Infinity, which feeds addUnits(end, scale, Infinity) and a while loop that can never exit. A single bad prop value hangs the render. Suggest clamping at the boundary, e.g. const unitWidth = Math.max(1, params.unitWidth). This is the one point I would treat as a real gate, because the failure mode is a hang rather than a wrong pixel.
| let cursor = startOfUnit(dayjs(timeScale.t0), scale); | ||
| if (cursor.valueOf() < timeScale.t0) cursor = addUnits(cursor, scale, 1); | ||
| let index = 0; | ||
| while (cursor.valueOf() <= timeScale.t1) { |
There was a problem hiding this comment.
[scalability, non-blocking] buildAxis materializes one tick object per unit across the whole domain. The virtualized path culls what renders but not what gets built here, so a day scale over a multi-year range allocates tens of thousands of ticks on every domain rebuild. Fine for the shipped use cases (weeks/months). Worth a code comment so nobody points it at a decade of days and wonders why it stalls.
| laneCount: number; | ||
| } | ||
|
|
||
| const DEFAULT_LANE_GAP_PX = 8; |
There was a problem hiding this comment.
[minor] Two things: (1) the horizontal in-lane gap defaults to 8px and the component never passes a value, so it is not configurable, while the vertical gap (laneGap) is a public prop defaulting to 16. Worth confirming that asymmetry is intentional. (2) DEFAULT_LANE_GAP_PX (8, horizontal) reads almost identically to DEFAULT_LANE_GAP (16, vertical) in timeline.tsx but they are different axes; a rename like DEFAULT_CARD_GAP_PX would remove the trap for the next reader. For the record, the first-fit greedy packing itself is correct and optimal in lane count.
| */ | ||
| lanePacking?: 'auto' | 'one-per-row'; | ||
| /** Card height in px (cards render at exactly this height; named to match `DataView.List`). Default 66. */ | ||
| estimatedRowHeight?: number; |
There was a problem hiding this comment.
[api / naming] rowHeight was renamed to estimatedRowHeight to match DataView.List, but the semantics differ. In List, estimatedRowHeight is a true estimate: rows auto-measure after paint and it is only used until the first measurement. In Timeline, cards render at exactly this height (applied as a hard height style; nothing measures), so content taller than this value clips or overflows. The shared name promises auto-measurement that does not happen here. Options: keep rowHeight (honest), or keep the name but make the doc explicit that cards do not auto-measure unlike List. Since the rename came from earlier review feedback, flagging the mismatch rather than deciding it.
| * visual entirely — chrome, states, truncation, and the collapsed variant. | ||
| * Compose `<DataView.DisplayAccess>` inside for Display Properties support. | ||
| */ | ||
| renderCard: ( |
There was a problem hiding this comment.
[perf / docs] The Timeline memoizes its card wrapper (TimelineCardView) so per-frame cursor/viewport state does not re-render card interiors. That optimization only holds if renderCard is referentially stable. If a consumer passes an inline renderCard={(row, ctx) => ...}, its identity changes every render, the memo never hits, and every visible card re-renders on every scroll frame. Nothing here tells consumers to wrap it in useCallback. Suggest adding that guidance to this doc (the same applies to onRowClick).
| /** Pixel width of the time span (0 when `endField` is omitted). */ | ||
| width: number; | ||
| /** True when the span is narrower than `minCardWidth`. */ | ||
| collapsed: boolean; |
There was a problem hiding this comment.
[minor doc] collapsed is documented as true when the span is narrower than minCardWidth, but point cards (no endField) are never collapsed regardless of width (see collapsed={item.endTime !== null && ...} in timeline.tsx). One clause here would close the gap.
| ); | ||
| } | ||
|
|
||
| const TimelineCardView = memo( |
There was a problem hiding this comment.
[perf] The value of this memo depends on row, renderCard, and onRowClick being referentially stable across renders (the comment above says as much). row is stable from TanStack, but renderCard/onRowClick come straight from consumer props with no memoization enforced. If a consumer passes them inline, the shallow compare fails every frame and this memo becomes pure overhead. Pairs with the renderCard doc note in data-view.types.tsx. No change needed in this file; calling it out so the consumer-facing docs cover it.
| ); | ||
|
|
||
| return ( | ||
| <div |
There was a problem hiding this comment.
[a11y] The scroll container is the primary way to navigate the timeline, but it is not keyboard operable: no tabIndex, so keyboard-only users cannot focus it to scroll horizontally with arrow keys, and drag-to-pan is mouse-only by design. The role="list" canvas also has no accessible name. Focusable content inside renderCard lets tab reach individual cards, but panning the axis itself is unreachable without a pointer. Consider tabIndex={0} + an aria-label on the scroll region, or documenting the keyboard story. Medium priority.
| // Panning, not selecting — suppress native text-selection drag. | ||
| event.preventDefault(); | ||
| }, | ||
| [] |
There was a problem hiding this comment.
[nit] handleDragPointerDown uses stopMomentum but its dependency array is []. It works because stopMomentum is a stable useCallback([]), but it is inconsistent with the rest of this file, which is otherwise careful with exhaustive deps (and would trip react-hooks/exhaustive-deps if that rule is on). Either add stopMomentum to the array or leave a note.
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.
| position: sticky; | ||
| top: 0; | ||
| z-index: 3; | ||
| height: 52px; |
There was a problem hiding this comment.
[minor / maintainability] The 52px axis height is a magic literal duplicated here and in .timelineCanvas (min-height: calc(100% - 52px)), plus the explaining comment. Change the axis height and you must remember all three. A custom property scoped to .timelineRoot (e.g. --timeline-axis-height: 52px) referenced in both rules would keep them in sync. Nice that it is not also duplicated in the JS.
| pointer-events: none; | ||
| } | ||
|
|
||
| .timelineMarkerLine { |
There was a problem hiding this comment.
[design question, low] Marker lines, gridlines, and the cursor line render before the cards in DOM order and none carry a z-index, so cards paint on top of them. A today line reading as a subtle background is fine, but a custom danger marker (a deadline) would be hidden wherever a card overlaps its x. Is occluding marker lines behind cards intended? If milestone lines should stay visible, they would need to sit above cards (a higher z-index, still pointer-events: none).
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.
| }); | ||
| }); | ||
|
|
||
| describe('buildAxis', () => { |
There was a problem hiding this comment.
[test coverage] Every scale-level and renderer-level test uses day or month. The quarter scale has zero coverage, and it is the riskiest path because its logic is hand-rolled: startOfUnit (date.startOf('month').subtract(date.month() % 3, ...)), addUnits (add(3 * n, 'month')), and the Q${...} branches in tickLabel and cursorLabel all exist precisely because dayjs has no quarter support without a plugin. A single quarter-scale buildAxis case (assert Q1 2025/Q2 ticks and year bands) would cover the one part of the math that is not just delegating to dayjs. A week case would be nice too, though it mostly delegates.
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.
| fetchOrdersApi( | ||
| dayjs(`${firstKey}-01`).valueOf(), | ||
| dayjs(`${lastKey}-01`).endOf('month').valueOf() | ||
| ).then(rows => { |
There was a problem hiding this comment.
[example quality] Confirming CodeRabbit's finding here, with a calibrated severity. The observation is correct: this .then() has no .catch()/.finally(), and because the months are added to requestedRef at line 581 before the fetch settles, a rejection would strand isLoading at true and never retry that window.
But I would not call it Critical: fetchOrdersApi (line 161) only ever resolves via setTimeout, so it cannot reject in this demo. The real reason to fix it is that the guide presents this block as "the recommended pattern" for range-window loading, and example code gets copied against a real API that can reject. So: worth fixing as a teaching example (add .catch that deletes the affected keys from requestedRef so a later scroll retries, plus .finally for the inflight decrement), but it is not a bug in the shipped library and does not block the feature.
2 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.