Adds date / time data type - #527
Conversation
project_bootstrap / desktop_index import ./all_css, which Rolldown emits as all_css.css; keep Flask asset naming consistent with mdv.css. Co-authored-by: Cursor <cursoragent@cursor.com>
✅ Deploy Preview for mdv-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Review limit reached
Next review available in: 33 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughDate-valued columns are ingested as UTC epoch-day doubles with ChangesDate column support
CSS asset mapping
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Source
participant MDVProject
participant DataStore
participant SelectionDialog
participant Chart
Source->>MDVProject: submit datetime columns
MDVProject->>DataStore: store epoch-day doubles and date metadata
DataStore->>SelectionDialog: provide date column and numeric range
SelectionDialog->>DataStore: apply parsed date bounds
DataStore->>Chart: provide date column values
Chart->>Chart: render ISO date ticks and labels
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![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
🧹 Nitpick comments (1)
src/charts/SVGChart.js (1)
129-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
isDateColumnhelper to centralize date detection.All three files manually check
is_date || date_unit === "days". To prevent divergence, import and use the sharedisDateColumnhelper fromsrc/lib/dateFormat.ts.
src/charts/SVGChart.js#L129-L131: replace the manual check withif (isDateColumn(col)) { return col; }.src/charts/WGLScatterPlot.js#L274-L275: replace withconst xIsDate = isDateColumn(xCol);andconst yIsDate = isDateColumn(yCol);.src/utilities/Color.js#L243-L243: replace the ternary condition withisDateColumn(c).🤖 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 `@src/charts/SVGChart.js` around lines 129 - 131, Replace manual date detection with the shared isDateColumn helper and import it from src/lib/dateFormat.ts: update SVGChart.js at lines 129-131 to use isDateColumn(col), WGLScatterPlot.js at lines 274-275 to derive xIsDate and yIsDate through isDateColumn, and Color.js at line 243 to use isDateColumn(c) in the ternary condition.
🤖 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 `@src/charts/WGLScatterPlot.js`:
- Around line 505-522: Remove the entire updateAxis() override from
WGLScatterPlot. Rely on the inherited SVGChart.js implementation for date tick
formatting and axis updates, preserving smooth transitions for non-date X and Y
axes.
- Around line 270-289: Extract the date-specific axis logic from the constructor
into a _applyDateAxisDefaults() helper, preserving the existing date detection,
log-scale disabling, and minimum axis sizes. Call this helper from both the
constructor and drawChart(), after the dynamic x/y parameters are assigned, so
Settings-driven column changes apply the defaults before rendering.
---
Nitpick comments:
In `@src/charts/SVGChart.js`:
- Around line 129-131: Replace manual date detection with the shared
isDateColumn helper and import it from src/lib/dateFormat.ts: update SVGChart.js
at lines 129-131 to use isDateColumn(col), WGLScatterPlot.js at lines 274-275 to
derive xIsDate and yIsDate through isDateColumn, and Color.js at line 243 to use
isDateColumn(c) in the ternary condition.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: beb45690-7222-42dd-9102-c2eb76a810c3
📒 Files selected for processing (30)
docs/TABLE_CHART_REACT.mddocs/extradocs/datasource.mddocs/jsdocs/extradocs/datasource.mdpython/mdvtools/mdvproject.pypython/mdvtools/tests/test_date_columns.pysrc/charts/BaseChart.tssrc/charts/SVGChart.jssrc/charts/WGLScatterPlot.jssrc/charts/charts.d.tssrc/charts/schemas/DataSourceSchema.tssrc/datastore/DataStore.jssrc/lib/dateFormat.tssrc/react/components/AxisComponent.tsxsrc/react/components/HistogramWidget.tsxsrc/react/components/SelectionDialogComponent.tsxsrc/react/components/legend/ColorLegend.tsxsrc/react/components/legend/LegendContinuousSvg.tsxsrc/react/legend/color_legend/buildColorLegendSpec.tssrc/react/legend/color_legend/types.tssrc/react/legend/shared/legendTypes.tssrc/react/legend/shared/legendUtils.tssrc/react/utils/valueReplacementUtil.tssrc/tests/dateAxisTicks.spec.tssrc/tests/dateColumnDisplay.spec.tssrc/tests/dateFormat.spec.tssrc/tests/histogramBrushRange.spec.tssrc/tests/react/legend/legendUtils.test.tssrc/tests/table_react/utils/valueReplacementUtils.test.tsxsrc/utilities/Color.jsvite.config.mts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/charts/WGLScatterPlot.js`:
- Around line 367-383: Update _applyDateAxisDefaults() to cache each axis’s
pre-date x_log_scale/y_log_scale and size values when first switching to a date
column, then restore those cached settings when the corresponding column becomes
numeric again. Keep date-axis defaults and setAxisSize behavior intact, and add
a regression test covering a date-to-numeric switch that restores logarithmic
scaling and the original axis size.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c1ad610-497d-4de8-b381-34b784db29c3
📒 Files selected for processing (2)
python/mdvtools/mdvproject.pysrc/charts/WGLScatterPlot.js
🚧 Files skipped from review as they are similar to previous changes (1)
- python/mdvtools/mdvproject.py
| * Categorical columns return category strings; `is_date` numerics return ISO `YYYY-MM-DD`; | ||
| * other numerics return numbers (or `"missing"` for NaN). | ||
| */ | ||
| getValue: (i: number) => string | number; |
There was a problem hiding this comment.
we've lost some fidelity in the type here :(
There was a problem hiding this comment.
Slight shame that we keep putting more into legacy vanilla charts that I'd rather see in react, but this looks mostly reasonable.
edit: it's not like the relevant changes in AxisComponent.tsx etc are missing, so I should probably stop whinging about nothing. LGTM, not having actually tried it...
2 participants
Adds real date-column support end to end. Datetimes ingested from pandas/polars are stored as numeric days since the Unix epoch, tagged with is_date, and shown as YYYY-MM-DD in tables, tooltips, axes, color legends, Selection Dialog filters, and Find/Replace — while charts and filters keep using the underlying day numbers so sorting and brushing stay chronological.
Summary by CodeRabbit
YYYY-MM-DDacross tables, axes, legends, filters, and histogram/brush labels.is_date,date_unit) and behavior.