Skip to content

Add inline chart editing to the V2 interface - #1418

Merged
Paul Lizer (paullizer) merged 2 commits into
paullizer-react-v2-uifrom
paullizer-chart-editing
Sep 4, 2026
Merged

Add inline chart editing to the V2 interface#1418
Paul Lizer (paullizer) merged 2 commits into
paullizer-react-v2-uifrom
paullizer-chart-editing

Conversation

@paullizer

@paullizer Paul Lizer (paullizer) commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

What this adds

Inline editing for simplechart blocks in the V2 chat, completing the set alongside diagram and image editing.

A generated chart used to be final. If the model picked a pie where a bar was wanted, put a scale on the axis that flattened the story, left the axes unnamed, or got one number wrong, the only remedy was to ask again — which left a second near-duplicate chart below the first with nothing to say which one was current.

Every chart now has an Edit button opening a modal with a live preview and six tabs:

Tab What's in it
Data A grid of the chart's labels and series — edit any value, rename/add/remove series, add/remove rows. Scatter and bubble charts get their x/y/size pairs instead.
Design Chart type, title/subtitle/caption, legend and position, bar width, horizontal, stacked, line smoothing/fill/thickness/point size, doughnut hole, gridlines, data-table toggle
Axes Axis names, explicit min/max, start-at-zero, linear or logarithmic, category label angle and thinning
Source The raw payload, validated as it is typed, with a "Lay it out" button
Ask AI A scoped instruction, same as diagrams
History Every version, with restore

How it works

The block revision framework was already written kind-agnostic, so storage, addressing, conflict handling, export resolution and the shared-conversation path needed no changes — simplechart was simply admitted as an editable kind.

Every control is a pure source-to-source transform of the chart's own payload. That is what makes a control change a revision like any other: undoable, present in the history, honoured by the server-side export, and visible to a reader in the classic interface. A parallel "chart settings" store would have had none of those properties.

Two rules protect a payload while it is rewritten: a transform mutates the raw parsed JSON rather than the normalised ChartSpec (so fields this client does not read survive), and the serialised form follows the source it came from (so a compact payload is not inflated towards the size cap).

Unlike the diagram editor, the whole panel edits a draft and saves once. The diagram editor saves on each layout click, which is right for two buttons; a chart has dozens of controls, and one revision per click would file twenty documents and make the history unreadable. The saved revision carries an auto-written note naming what changed — "Bar width, Value axis" rather than "Edited".

Payload additions

All optional, all defaulting to what charts already did: yMin, yMax, yScale, xTickRotation, xTickLimit, barWidth, lineWidth, pointRadius, showGridX, showGridY.

Existing charts render byte-identically — verified by diffing PNG hashes from the matplotlib export against the previously committed renderer across every chart kind.

Bugs fixed along the way

  • smooth, fill and showDataTable did nothing. All three were parsed by every renderer and applied by none. A chart asking for straight line segments was drawn curved; one asking to keep its numbers private still offered them. Controls were added for them, so they are now wired up in all three renderers.
  • Horizontal bar charts scaled the wrong axis. A bar chart on its side draws its values along the bottom, but start-at-zero was applied to the axis carrying the category names — so the setting did nothing on exactly the charts where a truncated scale misleads most.
  • matplotlib silently re-enables a grid when line properties are passed with False, which would have made "hide the gridlines" do nothing in exports.
  • Code review caught three more in this PR's own new code: set_xlim(bottom=) raising a TypeError into a swallowed guard; a NaN in an AI reply storing a chart no browser can parse; and — worst — no text field could accept a space, because trimmed-on-parse values made React revert the keystroke.

Refactors

The classic client's revision resolution is extracted to chat-block-revisions.js and shared with the diagram renderer, rather than copied. The rule is subtle enough — find by position, confirm by fingerprint, fall back to an unambiguous fingerprint match, otherwise leave the original alone — that two copies would eventually disagree.

Chart drawing is extracted to ChartCanvas.tsx so the editor's preview and the chart in the reply cannot diverge.

Testing

  • functional_tests/test_v2_chart_editor.py — 23 checks, including real matplotlib renders proving the new options reach the export, that a value range moves the axis on a horizontal bar chart, and that a hostile payload cannot take the export down
  • functional_tests/test_v2_chart_editor_logic.ts — 76 behavioural checks on the transforms
  • Existing suites re-run green: test_v2_diagram_editor.py (18), test_message_block_revisions.py (18), test_v2_rich_rendering.py (13), test_v2_visual_style_controls.py (16), test_docs_app_surface_coverage.py, test_docs_site_quality.py, test_conversation_export_mermaid_tex_images.py, test_v2_collaboration_visual_style_fix.py
  • npm run build and tsc --noEmit clean

No new settings, no new admin surface, no new npm or CDN dependency — Chart.js is already vendored and loaded on demand.

Version 0.261.0600.261.061 (developed as 0.261.059, renumbered on merging the updated base). Documented in V2 Inline Chart Editing.

A generated chart used to be final. If the model picked a pie where a bar was
wanted, put a scale on the axis that flattened the story, left the axes unnamed,
or got one number wrong, the only remedy was to ask again -- which left a second
near-duplicate chart below the first with nothing to say which was current.

Charts now use the block revision framework diagrams already use. The framework
was written kind-agnostic for exactly this, so storage, addressing, conflict
handling, export resolution and the shared-conversation path needed no changes;
`simplechart` was simply admitted as an editable kind.

The editor has six tabs -- Data, Design, Axes, Source, Ask AI and History --
beside a live preview. Every control is a pure source-to-source transform of the
chart's own payload, so a control change is a revision like any other: undoable,
in the history, honoured by the export, and visible in the classic client.
Unlike the diagram editor, the whole panel edits a draft and saves once, because
a chart has dozens of controls and one revision per click would make the history
unreadable; the saved revision carries an auto-written note naming what changed.

Payload additions, all optional and all defaulting to prior behaviour: yMin,
yMax, yScale, xTickRotation, xTickLimit, barWidth, lineWidth, pointRadius,
showGridX and showGridY. Verified byte-identical rendering for existing charts
by diffing PNG hashes against the previously committed matplotlib renderer.

Also fixed along the way:

- `smooth`, `fill` and `showDataTable` were parsed by every renderer and applied
  by none. Controls were added for them, so they are now wired up.
- A horizontal bar chart draws its values along the bottom, but begin-at-zero was
  applied to the axis carrying the category names -- so the setting did nothing on
  exactly the charts where a truncated scale misleads most. All three renderers
  now swap the value and category axes, as the export has always done for the
  axis titles.
- matplotlib treats `grid(False, alpha=...)` as a styling request and enables the
  grid anyway, which would have made "hide the gridlines" silently do nothing.
- The classic client's revision resolution is extracted to chat-block-revisions.js
  and shared with the diagram renderer, rather than copied.

Tests: functional_tests/test_v2_chart_editor.py (23 checks, including real
matplotlib renders) and test_v2_chart_editor_logic.ts (76 checks).

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The base branch reached 0.261.060 while the chart editor was being built, so the
feature is renumbered from 0.261.059 in config.py, its release note section, its
feature documentation and both of its test headers.

Only two files conflicted, both the usual version-bump collisions: the VERSION
line and the top of the release notes. No code conflicted — the merged work is
in the V2 admin settings surface and does not touch the chart or block revision
paths.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@paullizer
Paul Lizer (paullizer) merged commit 64fd305 into paullizer-react-v2-ui Sep 4, 2026
2 checks passed
Paul Lizer (paullizer) added a commit that referenced this pull request Sep 4, 2026
Merging Workflow (#1416), Workspaces (#1417) and chart editing (#1418) into this
branch brought three latent defects into range of checks added here.

ADMIN_SETTINGS_FIELDS had two duplicate section keys. workflow-settings-section
was declared twice with different contents, so the later declaration silently won
and the earlier one was dead -- a reordering would have swapped which applied and
dropped six settings. chat-file-uploads-section was also declared twice, and
there the later declaration was actively overriding the Chat group's, hiding
`enable_chat_file_uploads`. Both are reduced to the single declaration that
already carries every field.

Three group assignment pickers were gated only on the switch that selects them,
not on the capability that owns it. Because visibility is evaluated per field
rather than recursively, turning the capability off left the picker on screen
assigning access for a disabled feature. Affects group and public workspace file
downloads and group workflows; each now repeats its gate's own condition.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Paul Lizer (paullizer) added a commit to paullizer/simplechat that referenced this pull request Sep 4, 2026
Phase 1 (microsoft#1415) landed and this PR was retargeted from
`paullizer-admin-ai-models-settings` onto `paullizer-react-v2-ui`, which had
also absorbed microsoft#1418 (chart editing) and microsoft#1421 (Security group). Five files
conflicted; every one is additive, so both sides are kept throughout.

`config.py`
    0.261.061 is now below the base, which reached 0.261.070 when phase 1
    merged, so keeping it would be a version regression. The merge queue
    assigned 0.261.073.

`docs/explanation/release_notes.md`
    The 0.261.061 heading collided with the number microsoft#1418 had already been
    given, so this PR's entries move to 0.261.073. The 0.261.060 heading is
    dropped rather than kept: it named phase 1's fix section, which the base
    collapsed into 0.261.070, and that section's body arrived as common
    context. Verified as base + exactly one new section, no duplicate headings.

`admin_settings_fields.py`
    The conflict cut through an unterminated field dict on both sides, so a
    keep-both would not have parsed. Rebuilt from the base instead, with this
    branch's three additions re-applied: the NON_PATCHABLE_TYPES rationale, the
    gpt-config section, and the default_model_selection legacy field name.
    Verified by parsing both sides and asserting the merged ADMIN_SETTINGS_FIELDS
    is exactly the union of their section keys -- 37 from the base plus
    gpt-config -- with no key lost, invented, or newly duplicated.

`route_backend_v2.py`
    Interleaved with microsoft#1421's group directory route because the two share
    incidental JSON-response structure. Rebuilt from the base with the
    default-model block appended and its import added; all ten of the base's
    route functions verified present. The settings PATCH keeps the deliberate
    order established by phase 1 and microsoft#1421 -- secret resolution, then
    `_seed_connections_on_first_enable`, then the write -- so endpoints seeded
    from the classic configuration are not run back through the secret pass.

`AdminSettingsPage.tsx`
    Imports and the component switch each gained arms from three PRs, and both
    sides were cut mid-JSX sharing one closing tail. Kept whole, giving each
    side its own copy of the tail; all eleven component arms verified present.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Paul Lizer (paullizer) added a commit to paullizer/simplechat that referenced this pull request Sep 4, 2026
Phase 2's branch brought microsoft#1418, microsoft#1421 and microsoft#1422 with it. Three of the things
this branch had built independently already existed there in a more complete
form, so this merge deletes more of its own code than it keeps.

Secrets
-------

The base declares a `secret` field type with a `SecretControl`, a shared
`admin_settings_secret_utils` module, and a settings route that already redacts
on read and resolves the placeholder on write. This branch had arrived at the
same design under the name `password`, with its own control and its own
resolution pass.

Carrying both would have been two mechanisms for one idea -- the thing the
review asked to avoid on the redaction side. So `password` is gone entirely:
the four AI Models credentials are re-declared as `secret`, `_normalize_password`
and `_resolve_redacted_secrets` are deleted, and `PasswordControl`,
`hasStoredSecret` and `readSecretValue` go with them.

The base's control is better than the one it replaces. Rather than a masked box
that starts empty, it shows "Stored" with an explicit Replace, and its comments
explain why it stages nothing until then: the control can unmount when a search,
a group change or a dependency switch drops it, and a queued empty value would
take the escape hatch with it while leaving the deletion behind.

Note the two designs disagree about the empty string, and the base's is
self-consistent. Here blank means clear, which is safe precisely because the
control cannot emit blank by accident; the earlier design needed blank to mean
"keep" because its box was always editable. Adopting half of either would have
produced a credential that could not be deleted.

Visibility
----------

The base generalized `depends_on` to accept a list of conditions, all of which
must hold, and its `_dependency_is_satisfied` already compares strings by value.
That supersedes this branch's string comparison and its transitive parent walk.

The declarative form is the better answer to the problem the walk was solving:
the schema is flat where the panes are nested, so a control inside two blocks
now names both gates rather than inheriting one. The Azure OpenAI keys declare
the direct-connection route and the authentication type; the image fields also
declare the capability toggle above them.

That last part was a real defect this merge surfaced. Every image field was
gated only on the APIM switch, which is itself gated, so turning image
generation off would have left the whole section on screen. The base's
`test_gated_fields_inherit_their_gate_s_own_conditions` caught it.

Registry
--------

`document-intelligence-section` existed on both sides, so the relocated
`enable_office_embedded_image_analysis` is folded into the base's block rather
than added as a second key -- a duplicate would have silently discarded whichever
block came first. The AST check reports 51 sections and no duplicates: the base's
49 plus `embeddings-config` and `image-config`.

Version moves to 0.261.083; microsoft#1425 took 082.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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.

1 participant