fix(desktop): resolve datasource captions before authoring - #845
Conversation
myu404
left a comment
There was a problem hiding this comment.
🤖 Reviewed by MichaelGPT — Recommendation: Approve
This centralizes datasource-caption resolution for the four datasource authoring tools (author-action set-mode, author-calc, author-parameter, author-set) plus bind-template inline calcs, so a caller-supplied caption (human display name) is resolved to the canonical top-level internal datasource name before mutation and readback instead of being matched raw. The change is correct and fails closed where it matters:
- Resolution is centralized and fail-closed.
selectTargetDatasource(authorCalcCore.ts:303) resolves viaresolveUniqueDatasourceName(exact internal name → normalized/bracket-equivalent name → a caption mapping to exactly one datasource). A caption shared by ≥2 datasources returns an ambiguity error listing the internal names (authorCalcCore.ts:320), and an unmatched selector returns a not-found error with candidates (:324) — neither silently picks the first candidate or a default. Exact internal names correctly win over colliding captions. - The de-dup is strictly safer, not a regression. The removed
authorSet(−90) /authorAction(−47) copies matched only exact names (or name-or-caption with no ambiguity check); routing them through the shared helper adds the ambiguity guard rather than dropping one. All five call sites now share one resolution path. - Readback is now scoped to the resolved datasource (
hasColumnNameAndCaptionInDatasource), closing the prior any-datasource match. - Tests are strong where they exist:
author-calcandauthor-actiondirectly exercise unique-caption resolution, exact-name-over-caption precedence, ambiguous-caption rejection, nested-connection-ID (textscan.*) rejection, and cross-datasource readback rejection.
No correctness or fail-open issues found. Two non-blocking observations inline (one minor, one nit).
|
@mattcfilbert : what do we need to do to move this one forward? |
|
via Claude:
1. Leading/trailing whitespace on
|
|
@matthewmarkmillersf The latest head, 64be8ad, is refreshed against feature/desktop and passes the full local check. author-parameter now has the same datasource description as the other four tools; duplicate-caption rejection is tested at both the parameter and set boundaries. Calculation readback also checks the intended datasource. Your whitespace report remains unaddressed: the shared resolver still preserves exact names and does not trim. I did not add the suggested unconditional trim because it would change an exact identifier containing surrounding spaces. A follow-up should preserve exact matching first, then allow a unique whitespace-tolerant match with ambiguity tests. This PR does not claim to fix the reported trailing-space case. — MattGPT |
Use one datasource-selection rule across Desktop authoring: accept an exact top-level internal name or a unique visible caption, then use the internal name for the write and calculation readback. Exact names take precedence; duplicate captions fail before mutation. Nested connection IDs are not datasource IDs.
This covers
author-calc, inline calculations inbind-template,author-parameter,author-set, and set-modeauthor-action. Future changes must preserve that identity through the full operation.Review changes
feature/desktopat601021e1, preserving formula validation, partial-retry behavior, and URL actions. Package version: 2.68.1.Validation
At
64be8adc,scripts/agent-checkpassed: 397 files / 6,913 tests, lint, typecheck, Desktop build, and lockstep checks. The focused datasource/binder run passed 345 tests. Sol adversarial review found no actionable P0–P2 issues. Exact measured schema assertions are 2,576 bytes forbind-templateand 1,433 forauthor-action.A live Desktop mutation was not repeated on this revision. Leading/trailing whitespace matching remains unchanged in the shared resolver; that review request is not implemented here.
— MattGPT