feat(desktop): reference worksheets and dashboards by stable id, not display name

[lauren-jacksonSFDC]

Decision

Sheets are referenced by their stable id (the sheet's simple-id uuid); the display name is
changeable UI text. The rule the codebase now keeps: id identifies, name displays.

• id identifies: every worksheet/dashboard lookup matches the caller's argument against the
  element's own <simple-id uuid> first, then its display name — so a renamed sheet (or a cached
  title) no longer produces a "not found" error.
• name displays: every user-facing label, cache-file name, and edit-buffer key now uses the
  resolved name or the stable id — never the raw id-or-name string the caller happened to pass.

Why

Error report: tools were called with a worksheet display name that no longer existed (renamed, or
a cached title), producing "not found" errors that listed many valid worksheet ids. The id is
stable across renames; the name is not. The mirror bug: when a caller passed an id, that raw id
leaked back into user-facing labels and cache filenames — the "name displays" half fixes that.

How the id relates to the .twb (verified)

The External Client API worksheet/dashboard id is in the persisted .twb: it is the
element's own <simple-id uuid='...'> child, braces included (verified against a saved workbook —
inventory id {5804EDA1-…} equals that worksheet's <simple-id uuid>). Because it is in the XML
we already hold, we match an id straight out of the workbook/fragment — no extra list-worksheets
round-trip.

But the .twb still wires a worksheet to its <window> and to dashboard zones / story points by
the sheet's name — each element carries its own distinct simple-id, and Tableau enforces
unique sheet names. So name stays the internal join key for the XML surgery: we resolve id →
current name from the XML, then perform the name-keyed operations the format requires. No .twb
format change.

Changes — the two halves of the one rule

id identifies (lookups match the stable id first):

1. Surgery layer (metadata/sheets.ts, dashboards.ts) — resolveWorksheetRef /
   resolveDashboardRef match <simple-id uuid> first, then display name, returning the sheet's
   current name; worksheetFragmentSimpleId reads the id off a standalone worksheet fragment.
2. Per-sheet apply (loadWorksheetXml.ts, perSheetDocumentApply.ts) — targets the live sheet
   by the fragment's own simple-id, so an apply lands on the right sheet even if it was renamed
   after the fragment was read; preserves the fragment's original bytes when it only refreshes the
   name; falls back to the name when the fragment carries no id.
3. bind-template target_worksheet — resolves an id (or name) against the already-loaded
   workbook XML before the replace-gate and title, and hands the id to Call-2. No extra API call.
4. apply-worksheet cached worksheetName — derives the canonical name from the cached
   fragment's own <simple-id>, no live-inventory lookup.
5. Workbook-document read fallback (workbookReadGateway.ts) — the route-missing fallback
   resolves reads by id (resolveWorksheetRef / resolveDashboardRef) and surfaces each sheet's id
   in the list (listWorksheetRefs / listDashboardRefs), so a host without the live
   worksheets/dashboards route is id-first too.
6. Worksheet edit buffer — keyed by the worksheet's simple-id, not its display name, so
   add-field / remove-field edits stay attached to the right sheet across a rename mid-session.

name displays (user-facing text uses the resolved value, never the raw argument):
7. list-worksheets / list-dashboards return the full External Client API item payload (id, name,
and the rest), so a caller captures both the stable id and the real display name up front.
8. get-worksheet-xml / get-dashboard-xml return { xml, name } — the resolved display name flows
into the response label and the cache-file prefix, so passing an id no longer leaks that id
into labels or cache filenames.
9. list-storyboards description aligned with list-worksheets / list-dashboards.

Testing

npm test (5792 pass), npm run lint clean, npm run build:desktop clean. Desktop tools/list byte
budget respected.

🤖 Generated with Claude Code

[mattcfilbert]

MattGPT update: stable IDs now survive a rename between read and apply.

The first live smoke exposed one Desktop constraint: the route was correctly addressed by worksheet ID, but Desktop rejected the cached fragment because its root still carried the old display name (C655862E). The branch now resolves the live object by ID, changes only that root name attribute before POST, and uses the live name for focus, verification, and receipts. The rest of the fragment stays byte-for-byte intact, including formulas and numeric entities.

This is the shared rule for worksheet, dashboard, and storyboard writes—not a worksheet-only patch. The worksheet case is live-proven: read by GUID, rename by GUID, apply the stale cached fragment by GUID, clean readback, same GUID, no duplicate. Dashboard and storyboard use the same seam and have regression coverage; I could not run the dashboard leg live because Desktop API 0.2.6 on this build returns 404 for dashboards:new.

Grok review found and prevented a whole-fragment serialization bug. Final local gate: 376 test files / 5,792 tests, lint, typecheck, desktop build, and lockstep all green.

[mattcfilbert]

MattGPT review: Approved. Stable ID selects the object; the MCP updates only the current root display name required by Desktop import. The worksheet rename/apply path passed live with clean readback, and the full local gate passed. Dashboard and storyboard share the regression-tested write seam; live dashboard proof remains unavailable on this API 0.2.6 build because dashboards:new is not served.

[mattcfilbert]

Re-reviewed the six commits since my approval at cea5a73. One blocking item, and it's a regression in the exact code that approval covered — so I'm switching to request-changes until it's fixed.

Blocking — src/tools/desktop/api/getWorksheetXml.ts:87

worksheetId: worksheetFragmentSimpleId(result.value.xml)!,

worksheetFragmentSimpleId returns string | null, and clearStickyWorksheetFile calls worksheetId.trim() on its first line. When the id is null, the ! lets it through and .trim() throws a TypeError — the whole get-worksheet-xml call crashes.

It's reachable. The workbook-document fallback (getWorksheetXmlViaWorkbookDocument) resolves a sheet by name through resolveWorksheetRef (where id is optional) and serializes the fragment with extractSheetXml, which returns a <worksheet> that has no <simple-id>. The comment above the call says a document-route fragment "always carries one," but listWorksheetRefs in this same file drops id-less sheets (id && name ? [...] : []) — so the code already treats a missing simple-id as a real case, which contradicts the invariant this ! leans on.

An earlier commit in this PR had the guard:

const bufferWorksheetId = worksheetFragmentSimpleId(result.value.xml);
if (bufferWorksheetId) {
  clearStickyWorksheetFile({ session: resolvedSession, worksheetId: bufferWorksheetId });
}

Restore that — it's the fix. The tests miss the regression because every fixture worksheet carries a simple-id; a fragment-without-simple-id case on the fallback path would go red and lock this down.

One question, your call — same root premise

listWorksheetRefs / listDashboardRefs drop any sheet without a <simple-id>, and a test enshrines "a Desktop document always has one." That's the same assumption the bug above disproves. If any Desktop build you support has document XML that predates simple-id, the workbook-document fallback returns an empty worksheet/dashboard list where the old name-only path returned names. Worth one probe against the oldest supported build, or a version note.

The rest of the re-review is clean — the id-vs-name resolver tests, the retitle numeric-entity test, and the null-simple-id guards already in add-field/remove-field/apply-worksheet are the right coverage. The list-shape change (string[] to {id,name,...}[]) is the point of the PR and fine on feature/desktop.

---

Posted by MattGPT on Matt Filbert's behalf.

[tableaukyler]

KylerGPT review

What this does: switches worksheet/dashboard lookup across the desktop tool layer to match a caller's ref against the sheet's stable <simple-id uuid> first, then its display name — so a rename between an agent's list-worksheets call and its next add-field/apply-worksheet call doesn't produce a false "not found." list-*/get-*-xml now return the resolved id alongside the name, and XML surgery (perSheetDocumentApply.ts) patches a stale cached fragment's name= attribute to the sheet's current name before POSTing, closing the specific race the PR calls out.

Good fix for the problem it targets, and perSheetDocumentApply.ts's rename-at-apply-time patch is solid. Two 🟠 gaps worth fixing before merge — both are cases where the diff's own new fallback logic quietly reintroduces the exact class of bug the PR sets out to remove.

🟠 1. add-field / remove-field's id resolution falls back to keying the buffer by display name — the bug this PR fixes

src/tools/desktop/authoring/fields/addField.ts:181-198 and the identical block in removeField.ts:94-111: when resolveWorksheetSimpleId can't resolve a ref to a simple-id (which happens exactly when the caller's cached name no longer matches — i.e. the rename-mid-session case this PR is about), the code falls back to bufferWorksheetId = trimmedWorksheetName and logs a warning. The comment above it says outright: "Keying the buffer on the display name... is a latent bug."

That contradicts worksheetEditBuffer.ts's own module doc a few lines away: "Display names are never the key: a rename would strand the buffer... Callers resolve the id before opening the buffer." This caller doesn't — it ships the known-bad key with a log line instead of failing loudly, which is exactly what ~/.claude/CLAUDE.md's Exceptions rule ("fail loudly... don't create fallbacks just so your code never throws") flags as a smell. The practical effect: the sticky buffer silently isn't reused (falls back to a fresh fetch) in precisely the scenario the fix targets. Since the fallback is identical in both files, worth fixing once in a shared helper that raises an actionable error (or otherwise doesn't proceed with a key it knows is wrong) instead of duplicating the known-bad fallback twice.

🟠 2. get-worksheet-xml's non-null assertion can turn a successful read into a hard failure

src/tools/desktop/api/getWorksheetXml.ts:85-88:

clearStickyWorksheetFile({
  session: resolvedSession,
  worksheetId: worksheetFragmentSimpleId(result.value.xml)!,
});

worksheetFragmentSimpleId returns string | null. If a fragment ever lacks a <simple-id>, the ! lies to the compiler and clearStickyWorksheetFile throws on worksheetId.trim() — after the XML was already read successfully, so the agent gets a generic tool-execution error instead of the sheet it asked for. Contrast with the same pattern done safely two files over, in applyWorksheet.ts:397-400:

const appliedBufferId = worksheetFragmentSimpleId(worksheetXml);
if (appliedBufferId) {
  clearStickyWorksheetFile({ session: resolvedSession, worksheetId: appliedBufferId });
}

Same PR, same helper, one call site guards the null and one doesn't. Worth matching the guarded form here too.

Nit

Several of the new one-line comments on resolveWorksheetRef/resolveDashboardRef/worksheetFragmentSimpleId (sheets.ts, dashboards.ts) genuinely explain a non-obvious invariant (why name, not id, stays the .twb surgery key) and match the codebase's existing docblock style (e.g. worksheetEditBuffer.ts's pre-existing module doc) — those are fine. The two comments flagged above admitting a shipped bug are the ones that should go away once the fallback itself is fixed, per ~/.claude/CLAUDE.md's "make a local note for the PR description, not an inline comment" rule.

---

🤖 Posted by KylerGPT — an AI reviewer trained on Kyler's review history. Kyler reviewed and approved this before posting.

[tableaukyler]

KylerGPT follow-up

Wider review pass turned up an item Matt's review and my first pass didn't cover.

---

🤖 Posted by KylerGPT — an AI reviewer trained on Kyler's review history. Kyler reviewed and approved this before posting.

[tableaukyler]

KylerGPT follow-up (2)

One more, a design nit this time, not a bug.

---

🤖 Posted by KylerGPT — an AI reviewer trained on Kyler's review history. Kyler reviewed and approved this before posting.

[tableaukyler]

🟡 resolveWorksheetRef here and resolveDashboardRef (dashboards.ts:197) are byte-for-byte identical aside from the collection accessor (worksheets?.worksheet vs dashboards?.dashboard). Same twinning for listWorksheetRefs/listDashboardRefs (sheets.ts:144 / dashboards.ts:185) and worksheetFragmentSimpleId/dashboardFragmentSimpleId (sheets.ts:176 / dashboards.ts:177). Same duplication shape for the resolve-buffer-id-or-fall-back-to-name block, copy-pasted across addField.ts, removeField.ts, and twice more in applyWorksheet.ts. Six near-identical bodies for one matching policy means a future change to the id/name resolution rule (or a fix to the name-fallback bug flagged elsewhere in this review) has to be hand-synced six times and can silently drift between worksheets and dashboards. Worth one shared helper parameterized on the accessor, and one shared resolveBufferWorksheetId for the buffer-key block.

[lauren-jacksonSFDC]

✨ Lauren's code review bot: ✨

Fair point — they're near-identical. I've kept them as separate typed functions: each returns a distinct ref type over a different collection, and a shared generic would thread the accessor and element type through and read less clearly than the two short twins. If you'd prefer the shared helper, I'll collapse them.

[lauren-jacksonSFDC]

✨ Lauren's code review bot: ✨

Re @mattcfilbert's review: Good catch on the !. Not reachable in practice: the per-sheet /document route ships in every build, and the whole-workbook fallback fragment also carries a <simple-id> (verified on a real .twb). Fixed anyway in 2d2def66 — an id-less fragment now returns a clear error instead of crashing on null.trim(), with a test.

[tableaukyler]

Re-review: fresh full pass since last review

Context: this covers a fresh full pass since my last review (against fff86a83). All 3 previously flagged issues are fixed correctly:

• getWorksheetXml.ts null-.trim() crash → now a malformed-worksheet-fragment error (2d2def6) ✓
• addField.ts name-keyed buffer fallback → now hard-errors via resolveWorksheetBufferId (a83b42c) ✓
• refineWorksheet.ts stale readback ref → now targets worksheetFragmentSimpleId(sourceXml) ?? canonicalWorksheetName (a83b42c) ✓

Ran conventions, bug-hunt, blast-radius, comment-policy, design-pattern, and premortem passes, plus adversarial verification on the 🟠. One confirmed regression risk, three minor design nits. Nothing blocking.

🤖 Posted by KylerGPT — an AI reviewer trained on Kyler's review history. Kyler reviewed and approved this before posting.

[tableaukyler]

🟠 Sticky-buffer clear is inconsistent — silently no-ops instead of erroring or falling back.

Confirmed reachable: loadWorksheetXml.ts:473 falls back to name-based targeting (worksheetFragmentSimpleId(xml) ?? canonicalName) when the input fragment has no <simple-id>, so a cached-file apply can succeed on an id-less fragment. But here, worksheetFragmentSimpleId(worksheetXml) re-parses that same id-less input, gets null, and the buffer clear is skipped — silently, no error. A later name-only add-field/remove-field resumes the stale pre-apply buffer and can quietly revert the change that was just applied — the exact bug class the sticky buffer exists to prevent.

Same best-effort-skip shape at the other two clear sites (applyWorksheet.ts:171, :266), and worksheetCache.ts:26-27's docstring on resolveWorksheetSimpleId claims callers "fall back to the name only as a last resort" — none do; they just skip the clear. Worth a loud failure or a real name-keyed fallback here, not a silent skip.

[tableaukyler]

🟡 Identical 11-line block resolving bufferWorksheetId via resolveWorksheetBufferId, duplicated verbatim in removeField.ts:93-103. Both files already share helpers from worksheetCache.ts — this should be one too, or a future tweak to error wording/retry behavior has to land in two places in lockstep.

[tableaukyler]

🟡 PerSheetApplyOutcome's 'applied' object variant is reached by elimination — typeof x === 'object' && 'type' in x then typeof x === 'string' — not a direct discriminant check. Same shape in loadWorksheetXml.ts. A future object-shaped outcome without a type key would silently fall through as "applied" and crash on .name/.id. Give "applied" its own check up front in both files.

[tableaukyler]

🟡 This comment just restates the ?? fallback order. Its worksheet twin (sheets.ts:154-157) has the same comment plus the actual reason (the .twb still cross-references sheets by name, so name has to stay the fallback join key). Copy that WHY over or drop the comment.