Skip to content

feat(results): add native result charts - #2222

Merged
datlechin merged 4 commits into
TableProApp:mainfrom
sophiathedev:feat/result-charts
Aug 19, 2026
Merged

feat(results): add native result charts#2222
datlechin merged 4 commits into
TableProApp:mainfrom
sophiathedev:feat/result-charts

Conversation

@sophiathedev

@sophiathedev sophiathedev commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add a native Chart result mode with bar, line, area, and scatter views
  • provide typed X/Y and optional series controls over the active loaded result buffer
  • show exact raw X, Y, and series values on hover without changing chart scope
  • adapt chart controls to narrow result panes and preserve readable plot space
  • bound projection work and reject values Swift Charts cannot represent without precision loss
  • keep data-grid-only commands and licensed metadata outside Chart mode

Preview

Bar
Bar chart full-window preview
Line
Line chart full-window preview
Area
Area chart full-window preview
Scatter
Scatter chart full-window preview

All previews are full-window captures using the bundled Chinook sample database. The query charts invoice date, revenue, and billing country from 40 loaded rows.

Scope

Charts visualize loaded rows only. They do not fetch, aggregate, sample, or persist configuration. Pivot tables remain follow-up work.

Related to #2107. This implements the result-chart slice of item 4; pivot tables remain follow-up work.

Validation

  • generated TablePro.xcodeproj from project.yml
  • passed chart, selection, toolbar, projection, license, result-mode, status-bar, and pinning regression suites
  • rendered categorical and numeric X data across all four chart types in light and dark appearances
  • passed strict SwiftLint on all 13 changed Swift files
  • verified the string catalog and chart localization placeholders for Turkish, Vietnamese, Simplified Chinese, and Traditional Chinese
  • passed an unsigned Debug TablePro build
  • manually verified all four chart types in full-window Chinook flows

The chart UI test now uses stable accessibility identifiers for the Chart segment and license gate. It compiles locally; the local UI runner was killed before bootstrapping, so GitHub Actions remains the authoritative runtime result for that test.

@sophiathedev
sophiathedev marked this pull request as ready for review August 19, 2026 07:32
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Repo admins can enable using credits for code reviews in their settings.

@datlechin

Copy link
Copy Markdown
Member

Pushed eb5a9d3 onto this branch: it fixes the 15 review findings plus 4 more that came out of investigating them. Nothing was rewritten, it is one commit on top of your work.

Three root causes account for most of it, so the fix is a refactor at three boundaries rather than 19 patches.

1. The chart's vocabulary was narrower than Swift Charts'. AxisKind had two cases where Charts has three primitive plottables (String, Double, Date).

  • .date / .timestamp / .datetime now resolve to a real time axis instead of unordered string bands. Cell text is parsed by a new DatabaseDateParser, extracted from DateFormattingService so there is one grammar, plus three measured patterns it was missing (PostgreSQL timestamptz with a space before the offset, and microsecond timestamps).
  • y and numeric x are now Double rather than Decimal. This was the worst finding and it was not in the original review: NSDecimalNumber(decimal:).doubleValue is not correctly rounded, so isExactlyPlottable was rejecting 2806 of the 10000 values of the form d.dd (measured, reproduced twice). A DECIMAL(10,2) price column silently lost over a quarter of its bars: 19.99, 0.07, 1.07, 0.11, 0.21 were all dropped. Charts converts to Double anyway, so parsing with strtod and validating against the value Charts will actually plot removes the second, differently-rounded conversion entirely.
  • Bar grouping passes a discrete slot to .position(by:) instead of an Int. Charts reads an Int as a continuous offset, so a repeated category's second bar rendered inside the next category's band at a different width. Measured with an ImageRenderer pixel harness: for (paid,100),(paid,60),(pending,30) the second paid bar centred at x=398 with the band boundary at x=363. The two-series grouped bar chart in this PR's own preview was affected. Also measured that Charts orders a discrete position scale by first appearance, not alphabetically, so the ordinal's string needs no zero padding.
  • Resolved.xAxisKind is now carried on the projection and used, instead of the canvas sniffing points.first.

2. A bound was treated as a terminal error. Every cap returned points: [], so a result one row over any limit showed an error card instead of a chart.

  • Caps now truncate and report: limits: [Limit] replaces the all-or-nothing Issue, and the toolbar says "Showing the first 2,000 points of 8,431 loaded rows". Every other tool does this (Metabase's "Showing first 2000 rows", DBeaver's row-count cap); none refuses.
  • showsDataChrome split into per-affordance predicates on ResultsViewMode, so Chart mode keeps the row count, Fetch All and the pagination controls. The toolbar's "More rows available" warning is gone: it had no remedy on screen, and hasUnloadedRows was forward-looking only, so on the last page of a 5-page table it reported the chart complete while 4,000 of 5,000 rows were missing. The status bar's row range already tells the truth, so the second, wrong signal was removed rather than corrected.
  • A skipped row now records a line break on every path. When the series cell is the unreadable one the row cannot be attributed, so every series breaks.
  • ResultChartView has an explicit state enum, so no combination renders a blank pane. project() is async throws(CancellationError), which makes "cancellation is the only failure" a compiler guarantee rather than a comment, so no unreachable retry UI was added.

3. A three-way router was collapsed into a bool. GridSelectionOwner already existed; dataGridOwnsSelection flattened it.

  • Every row command switches over the owner exhaustively. This fences resolvedRowSelection() itself, so a stale grid selection carried into Chart mode can no longer delete rows, while the sidebar's table-deletion path still works.
  • Structure mode gets Copy back. Copy Rows with Headers and Copy Rows as JSON are gated separately on hasDataGridRowSelection, because the structure grid has no handler for them and feeding it structure-grid indices is the display-position bug from Faulty Details for Json column #1837.
  • Ownership only counts where a handler exists, so a Create Table tab no longer swallows Cmd+Delete and Cmd+C into nil handlers.
  • JSON mode keeps its row commands. That change was unrelated to charts and undocumented, so it was reverted rather than kept.
  • Chart configuration moved from ResultSet to QueryTab, beside sortState and pagination, and is keyed by column name rather than index. It now survives a page turn, a sort and a re-run, and a re-ordered SELECT cannot silently chart a different column. A choice whose column is missing is kept, not erased.
  • The default Y axis skips the primary key. On the bundled Chinook Track table the first chart was Row Number vs TrackId: a straight diagonal.

Smaller ones: per-series dashes come from .chartLineStyleScale(range:) (the constant .lineStyle was overriding .lineStyle(by:), so series differed by colour alone); hover uses an index built once instead of two O(n) scans per frame, and the deep onChange(of: projection) comparison is gone; the accessibility summary no longer reads "with 1 points"; docs/features/licensing.mdx gained the tier row.

On finding 2 (the segment accessibility identifier): it does not reproduce. A measured AX probe shows a segmented Picker passes a per-segment identifier through to the AXSegment; the container does not clobber it. The UI test now addresses the segment by its title anyway, matching the two CI-green precedents in QueryPlanResultUITests and OpenQuicklyCommandUITests, and it passes locally: the runner logs Click "chart.xyaxis.line" RadioButton.

Verified: Debug build passes. 160 unit tests pass across every touched suite, including new ones for the money-value regression, the chronological date axis, cap truncation, config survival across a result replacement, the default-Y rule, the bar banding, and the plural. ResultChartUITests passes. swiftlint --strict is clean on every changed file.

One pre-existing lint error is untouched and not from this branch: ExecutionAuditLog.swift:40 (storage_environment_directory), which is already on main at the merge base.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants