fix: preserve line continuity at zoom viewport edges - #53
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe zoomable time-window case separates dot and line data. Line data includes interpolated rows at fractional viewport boundaries, while dots remain tied to visible observations. Scene clipping, geometry tests, screenshots, and preview metadata were updated. ChangesZoom window rendering
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ChartDefinition
participant visibleZoomLineData
participant GeometryDriver
ChartDefinition->>visibleZoomLineData: compute lineRows for the window
visibleZoomLineData-->>ChartDefinition: return boundary-interpolated rows
GeometryDriver->>visibleZoomLineData: compute linePoints for the window
GeometryDriver-->>ChartDefinition: render clipped line geometry
Suggested reviewers: Merge Risk: ⚪ Minimal · up to Zoomed and panned charts now extend continuous lines to viewport boundaries while retaining dots only for observed data. The changed behavior is covered by model and rendering geometry tests, with no remaining merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 5 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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 `@benchmarks/conformance/cases/90-zoomable-time-window/model.ts`:
- Around line 56-60: The neighbor scan in visibleZoomDataWithNeighbors must not
break when a timestamp exceeds end, because rows may be out of order. Remove the
early termination and scan every row, preserving selection of the first visible
row and collecting neighbors based on each row’s timestamp within the window;
alternatively, explicitly enforce and document an ascending Date invariant
before retaining the break.
In `@benchmarks/conformance/cases/90-zoomable-time-window/view.tsx`:
- Around line 73-86: Update the line geometry bounds in the chart definition
around lineY and visibleZoomDataWithNeighbors to use the rendered lineRows data,
while keeping visibleRows for the dot mark. Build the role: 'line' bounds from
lineRows and clip the resulting rectangle to the chart area so it remains within
the geometry contract.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: acf98fd1-a1c1-4cdb-a976-6eb880e9f732
📒 Files selected for processing (2)
benchmarks/conformance/cases/90-zoomable-time-window/model.tsbenchmarks/conformance/cases/90-zoomable-time-window/view.tsx
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
View your CI Pipeline Execution ↗ for commit 27459be
☁️ Nx Cloud last updated this comment at |
* Allow line marks to configure caps and joins * Add Cartesian axis title styling * Allow configuring focus ring appearance * Preserve focus ring defaults for undefined options * Reserve the axis gutter in a right-to-left container A y scale with `side: 'right'` took its tick-label anchor from the physical side alone. That anchor reaches the DOM as SVG `text-anchor`, which resolves against inline base direction, so an RTL container painted the labels leftward into the plot. The margin pass then read those bounds correctly and reserved nothing on the right, widening the plot until it ran under its own labels. Anchor the far side with `end` once the container reads right to left, and mirror the same relation in the text estimator so a host without a DOM measurer resolves the identical layout. * Relock the difference-mark bundle increment The layout fix costs 73 B raw in shared code, which puts the difference-mark increment 0.02 kB over its ceiling. Gzip falls on most entries. * Harden focus styling and RTL text anchors * Complete RTL text direction support * Update combined release baselines * Address final review findings * Refresh PR 124 evidence on v0.17.0 * Sanitize XML-invalid SVG characters * Reduce XML sanitizer footprint * Escape custom SVG paths * Refresh PR 124 evidence after XML sanitization * docs: use the dynamic README header endpoint (#54) * docs: use the dynamic README header endpoint * docs: fix package README banner metadata --------- Co-authored-by: Tanner Linsley <tannerlinsley@gmail.com> * docs: complete the React custom tooltip example (#83) * Fixed typo in react/quick-start.md for Chart component code example * Complete React tooltip example --------- Co-authored-by: Tanner Linsley <tannerlinsley@gmail.com> * docs: clarify React definition ownership (#111) * Align react docs language to be similar to homepage "what does TanStack own" * Clarify React definition ownership * docs: describe React definition updates precisely --------- Co-authored-by: Tanner Linsley <tannerlinsley@gmail.com> * fix: preserve line continuity at zoom viewport edges (#53) * fix: preserve neighboring zoom rows * fix: preserve line continuity at zoom viewport edges * fix: preserve zoom line continuity at viewport edges --------- Co-authored-by: Tanner Linsley <tannerlinsley@gmail.com> --------- Co-authored-by: Wojciech Maj <kontakt@wojtekmaj.pl> Co-authored-by: 3li7alaki <alihalaki@outlook.com> Co-authored-by: Alem Tuzlak <t.zlak97@gmail.com> Co-authored-by: NEDIE.YASSIN <67947969+nedieyassin@users.noreply.github.com> Co-authored-by: Vikrant Pogula <vikrant.pogula@gmail.com> Co-authored-by: Ferréol DUBOIS COLI <80718477+Fefedu973@users.noreply.github.com>
Summary
Fix the zoomable time-window example so continuous lines reach the viewport boundaries correctly while zooming and panning.
Problem
The example filtered the line to observations strictly inside the accepted time window. When a boundary fell between observations, the crossing segment disappeared and the line stopped short of the plot edge.
Using raw offscreen neighbors restores the segment, but it also lets offscreen values distort the inferred y domain.
Changes
Verification
Result
The line now reaches both viewport edges without offscreen extrema compressing the visible y scale, dots remain limited to real visible observations, and exact-boundary and gap behavior remains stable.
Summary by CodeRabbit
Bug Fixes
Tests