ReviewOS

stacks/stx

showMessageBox returns Craft's { buttonIndex } but is typed { response }, so result.response is undefined on desktop

#2040
Closed glennmichael123 opened this 9 days ago ·
9 days ago

Summary

showMessageBox() is typed as returning MessageBoxResult ({ response: number }), but on the native path it returns Craft's raw result, which is { buttonIndex: number }. Nothing reconciles the two, so result.response is undefined at runtime whenever a Craft bridge is present — while the web fallback in the same function does return { response }.

The result is a silent, type-clean wrong answer: TypeScript is satisfied, the browser works, and the packaged desktop app reads undefined off every dialog.

Evidence

The declared contract — @stacksjs/desktop/dist/dialogs.d.ts:

export declare function showMessageBox(options: MessageBoxOptions): Promise<MessageBoxResult>;

export declare interface MessageBoxResult {
  response: number
}

The implementation returns the bridge result untouched (from the bundled native-dialog build):

if (y()) {                       // craft bridge present?
  try {
    return await window.craft.dialog.showMessageBox(Z);   // <- raw, not normalised
  } catch (P) {
    console.warn("[stx-dialog] Failed to show native message box:", P);
  }
}
// ...web fallback below returns { response: confirm(z) ? X : G }
return { response: confirm(z) ? X : G };

Craft answers with buttonIndex — strings on the shipped craft-runtime (0.0.90):

{"canceled":false,"color":"#{"buttonIndex":

And the adapter never mentions it. grep -c buttonIndex over the whole bundled @stacksjs/desktop output: 0. The only buttonIndex in the package's types belongs to ModalResult (types.d.ts), a different API.

So the two branches of one function return different shapes, and the typed one is the branch that is wrong.

Impact

Any if (result.response === 0) / buttons[result.response] check silently takes the "not that button" path on desktop, no matter what the user pressed.

In our app (stacksjs/system-cleaner) this made every confirm dialog answer itself No: the sheet drew correctly, the user pressed the action button, and the answer was discarded on the way back. Clean Selected cleaned nothing, Empty Trash emptied nothing, and neither reported anything — there is no console in a packaged app, so it presented as "the button does nothing". It survived two rounds of fixes aimed at button ordering (defaultButton/cancelButton) before we traced it, because the ordering reasoning was sound and could never work while the index was read off a key the host never sets.

Suggested fix

Normalise in the adapter, so the declared type holds on both paths:

const raw = await window.craft.dialog.showMessageBox(opts)
const response = typeof raw.response === 'number' ? raw.response : raw.buttonIndex
return { response }

Alternatively widen MessageBoxResult to { response?: number, buttonIndex?: number } so the ambiguity is at least visible to callers — though normalising seems better, since response is the documented field and the one the examples in dialogs.d.ts use.

Worth a look at the sibling dialogs too (showOpenDialog, the color picker) in case they pass other Craft payloads through unnormalised.

Versions

  • @stacksjs/desktop 0.2.281
  • craft-native 0.0.90
  • macOS 26.6.2, arm64
9 days ago

Fixed in da5e222 — but the mechanism is not the one proposed here, and the difference matters for anyone who hits this.

The shape mismatch is real, but it was not what broke dialogs, because the native branch never returned a value at all.

isInCraftWindow() establishes only that craft.dialog exists. showMessageBox then called a method on it that was never checked for — and the shipped bridge's dialog namespace is:

var dialog = { openFile, openFolder, saveFile, showAlert, showConfirm, showPrompt };

No showMessageBox. craft-native's bridge/core.d.ts:289 declares one anyway (typed { response, checkboxChecked? }), which is why it read as available, while api/dialog.d.ts — the namespace it actually exports — does not have it.

So the call threw not a function, the catch swallowed it, and every native dialog fell through to the browser confirm() fallback. That is the worst place to land in a packaged app: no console to see the warning in, and a WebView that does not implement the confirm panel answers false without drawing anything.

Which means the theory that was deliberately not filed — "Craft doesn't implement the confirm panel, so window.confirm silently returns false" — was the right one. It just wasn't reached the way it looked; the bridge call falls into it first. Calling that one unproven was the correct judgement at the time, and it is now the load-bearing half of the explanation. It also explains why two rounds of defaultButton/cancelButton work could not have helped: nothing native was being asked.

Where { buttonIndex } does fit: showAlert is the method that exists, and craft-native's own implementation reads payload.buttonIndex off the host and resolves to a bare number. So the host really does speak buttonIndex — the strings evidence here is sound — it just reaches the app through a different method.

The fix:

  • Probe the method, not the namespace.
  • Use showAlert when showMessageBox is absent; it answers with the button index directly.
  • Still honour a host that implements showMessageBox, normalising all three currencies: a bare number, Electron's { response }, and Craft's { buttonIndex } — this issue's finding, kept.
  • An unreadable answer now resolves to the cancel button. It was previously whatever undefined === 0 made of it, and defaulting to 0 reads a dialog that failed to answer as consent to the destructive thing it asked about.

Two more in the same file: the generated window.stxDialog bridge script called the same absent method, and its confirm() still passed ['Cancel', 'OK'] with response === 1 — the ordering corrected in showConfirmDialog but never here, so it defaulted to Cancel.

Why the suite missed it: the shared bridge mock is a Proxy that answers every namespace and every method, so tests ran against a richer bridge than the one that ships. The new tests install exactly the real surface; six of seven fail against the previous code.

Sign in to comment on this issue.