From d46118812d6c72aab17b3b9462d1dff4ee823daf Mon Sep 17 00:00:00 2001 From: Adib Hanna Date: Mon, 19 Oct 2026 17:04:57 -0500 Subject: [PATCH] =?UTF-8?q?fix(mobile):=20Settings,=20Vault=20shows=20your?= =?UTF-8?q?=20vaults=20and=20Manage=E2=80=A6=20again?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since core 2.58.0 the phone Settings, Vault, Location card showed only the core "Change…" row. The vault list, New Vault… and Manage… were gone, so Settings had no way to reach Add Remote Vault… (the circle, More, Vaults still did). The card is a React island that useVaultSettingsRows mounts into core's Vault Location row, before the row's first button. Core 2.58.0 (ZenNotes/zennotes#871) moved those buttons into one wrapping group, so the first button became a grandchild of the row, insertBefore threw NotFoundError inside the MutationObserver callback, and the island never attached. The CSS that hid the row's own buttons in the island's favor matched bare buttons only. On iPad the Manage… button still mounted, but as a flex item of its own, so it wrapped onto its own line instead of sitting beside Change…. placeVaultSettingsIsland (vault-settings-access.ts, the same file as on Android) now anchors on the row's direct child that is or holds a button, which covers both shapes: the phone island goes before that group, and the iPad Manage… joins it. The observer's already-mounted check is host.contains, because the iPad button no longer sits directly in the row and re-placing it would fire the observer again. mobile.css hides whatever follows the island and is or holds a button, so the row's own Change… stays hidden on the phone, as it was before 2.58.0. The new tests run on a small stand-in DOM whose insertBefore throws for a reference that is not a direct child; the old placement fails four of them, two with that NotFoundError. Reproduced first in the browser pane on this build at 375x812: only Change…, and the NotFoundError in the console. After the fix: the vault list with the current vault checked, New Vault… and Manage…; switching vaults from the list keeps the card; Manage… opens the Vaults sheet with Add Remote Vault…. At 768x1024 Change… and Manage… sit side by side and Manage… opens the sheet. 278 tests, typecheck and boundary check pass. --- src/ui-mobile/MobileShell.tsx | 18 +-- src/ui-mobile/mobile.css | 34 ++--- src/ui-mobile/vault-settings-access.test.ts | 149 +++++++++++++++++++- src/ui-mobile/vault-settings-access.ts | 35 +++++ 4 files changed, 207 insertions(+), 29 deletions(-) diff --git a/src/ui-mobile/MobileShell.tsx b/src/ui-mobile/MobileShell.tsx index 8e6c86c..5011489 100644 --- a/src/ui-mobile/MobileShell.tsx +++ b/src/ui-mobile/MobileShell.tsx @@ -55,7 +55,7 @@ import { installNoteRowGestures, NOTE_ROW_SELECTOR } from './note-row-gestures' import { NoteActionSheet } from './note-actions' import { installEditorKeyboardScroll } from './editor-keyboard-scroll' import { installEditorNativeTyping } from './editor-native-typing' -import { vaultSettingsAccessForLayout } from './vault-settings-access' +import { placeVaultSettingsIsland, vaultSettingsAccessForLayout } from './vault-settings-access' // Phone-only behaviours gate on this. Smallest-side based, so rotating a phone // into landscape no longer disables the whole mobile shell (Android #12 — the // same failure existed here: an iPhone in landscape is wider than 768 pt). @@ -2305,7 +2305,10 @@ function useVaultSettingsRows(): void { // which reads as the whole card blinking. Moving the same container // back in preserves the live React tree and its state. if (!host) return - if (container?.parentElement === host) return + // contains, not parentElement: on a tablet the container lives in the + // row's button group, and re-placing it on every mutation would fire + // this observer again forever. + if (container && host.contains(container)) return if (!container) { container = document.createElement('div') // The phone wrapper claims the full stacked row. On iPad, display: @@ -2323,16 +2326,7 @@ function useVaultSettingsRows(): void { ) ) } - if (access === 'quick-switch') { - // The phone stylesheet wraps this desktop row into a stack. Mount the - // switcher between its label and actions; CSS replaces those actions - // with the switcher's own action group. - const firstBtn = host.querySelector('button') - if (firstBtn) host.insertBefore(container, firstBtn) - else host.appendChild(container) - } else { - host.appendChild(container) - } + placeVaultSettingsIsland(host, container, access) } const observer = new MutationObserver(() => sync()) observer.observe(document.body, { childList: true, subtree: true }) diff --git a/src/ui-mobile/mobile.css b/src/ui-mobile/mobile.css index be7b164..dbd12d9 100644 --- a/src/ui-mobile/mobile.css +++ b/src/ui-mobile/mobile.css @@ -2112,35 +2112,32 @@ /* ---- Settings vault quick-switch (useVaultSettingsRows) ------------------- */ /* Sits inside the location row (stacked on the phone), between the label and - the row's own buttons — the row's px-5 padding is already around it. */ + the row's own actions; the row's px-5 padding is already around it. */ .zn-settings-vaults-host { flex-basis: 100%; min-width: 100%; } -/* In the tablet settings row, keep only the compact manager entry point. The - wrapper disappears from flex layout so its button sits beside Change… and - Quick Connect… instead of forcing the phone quick-switch card into the row. */ +/* In the tablet settings row, keep only the compact manager entry point. + placeVaultSettingsIsland puts the wrapper at the end of the row's button + group, and the wrapper disappears from flex layout so its button sits + beside Change… and Quick Connect… and wraps with them (core wraps that + group under the label on its own since 2.58.0). */ .zn-settings-vaults-manage-host { display: contents; } -[data-settings-search-id='vault-location']:has(> .zn-settings-vaults-manage-host) { - flex-wrap: wrap; - row-gap: 0.5rem; -} - .zn-settings-vaults { padding-top: 0.625rem; border-top: 0.5px solid rgb(var(--z-bg-4) / 0.55); } -/* The card's own actions ("Change…", and in a remote workspace its siblings) - are hidden while the island is mounted — the island's action group renders - its own Change… (same store flows) so all actions live in ONE clearly - separated button row under the picker list (Adib: buttons must read apart - from the list). Scoped with :has() so this only applies where the island - actually mounted (phone width). */ +/* The row's own actions (Change…, and in a remote workspace its siblings) + are hidden while the island is mounted: the island's action group carries + New Vault… and Manage…, so all actions live in ONE clearly separated + button row under the picker list (Adib: buttons must read apart from the + list). Scoped with :has() so this only applies where the island actually + mounted (phone layout). */ .zn-mobile [data-zn-settings] div.flex.items-center.justify-between[class*='px-']:has(> .zn-settings-vaults-host) { @@ -2152,7 +2149,12 @@ padding-bottom: 0.625rem; } -[data-settings-search-id='vault-location']:has(> .zn-settings-vaults-host) > button { +/* Everything after the island that is or holds a button is the row's own + actions: bare buttons before core 2.58.0, one wrapping group since (#871). + A rule for bare buttons alone stopped matching when core wrapped them. */ +[data-settings-search-id='vault-location'] + > .zn-settings-vaults-host + ~ :is(button, :has(button)) { display: none; } diff --git a/src/ui-mobile/vault-settings-access.test.ts b/src/ui-mobile/vault-settings-access.test.ts index 02cfd2d..a2daa51 100644 --- a/src/ui-mobile/vault-settings-access.test.ts +++ b/src/ui-mobile/vault-settings-access.test.ts @@ -1,6 +1,10 @@ import assert from 'node:assert/strict' import { describe, it } from 'node:test' -import { vaultSettingsAccessForLayout } from './vault-settings-access.ts' +import { + placeVaultSettingsIsland, + vaultRowActionGroup, + vaultSettingsAccessForLayout +} from './vault-settings-access.ts' describe('vaultSettingsAccessForLayout', () => { it('keeps the full quick-switch controls in the phone settings layout', () => { @@ -11,3 +15,146 @@ describe('vaultSettingsAccessForLayout', () => { assert.equal(vaultSettingsAccessForLayout(false), 'manage-button') }) }) + +// The tests run under plain node, so the row is a small stand-in for the DOM +// that keeps the rule that broke the island: insertBefore throws unless the +// reference node is a direct child. +class FakeElement { + readonly tagName: string + readonly label: string + parentElement: FakeElement | null = null + private kids: FakeElement[] = [] + + constructor(tagName: string, label: string, kids: FakeElement[] = []) { + this.tagName = tagName.toUpperCase() + this.label = label + for (const kid of kids) this.appendChild(kid) + } + + get children(): FakeElement[] { + return [...this.kids] + } + + querySelector(selector: string): FakeElement | null { + assert.equal(selector, 'button') + for (const kid of this.kids) { + if (kid.tagName === 'BUTTON') return kid + const found = kid.querySelector(selector) + if (found) return found + } + return null + } + + appendChild(node: FakeElement): FakeElement { + return this.insertBefore(node, null) + } + + insertBefore(node: FakeElement, ref: FakeElement | null): FakeElement { + if (ref && ref.parentElement !== this) { + throw new Error( + 'NotFoundError: The node before which the new node is to be inserted is not a child of this node.' + ) + } + node.parentElement?.removeChild(node) + const at = ref ? this.kids.indexOf(ref) : this.kids.length + this.kids.splice(at, 0, node) + node.parentElement = this + return node + } + + removeChild(node: FakeElement): void { + this.kids = this.kids.filter((kid) => kid !== node) + node.parentElement = null + } +} + +const el = (tag: string, label: string, kids: FakeElement[] = []): FakeElement => + new FakeElement(tag, label, kids) +const labels = (node: FakeElement): string[] => node.children.map((kid) => kid.label) +const place = (row: FakeElement, island: FakeElement, phone: boolean): void => + placeVaultSettingsIsland( + row as unknown as Element, + island as unknown as Element, + vaultSettingsAccessForLayout(phone) + ) + +// SettingsModal's Vault Location row since core 2.58.0 (#871): the label, +// then ONE wrapping group that holds the buttons. +const coreRow = (buttons: string[]): FakeElement => + el('div', 'row', [ + el('div', 'label', [el('div', 'title'), el('div', 'path')]), + el( + 'div', + 'actions', + buttons.map((name) => el('button', name)) + ) + ]) + +// The same row before 2.58.0: the buttons sat bare in the row. +const bareRow = (buttons: string[]): FakeElement => + el('div', 'row', [el('div', 'label'), ...buttons.map((name) => el('button', name))]) + +const phoneIsland = (): FakeElement => + el('div', 'island', [el('button', 'My Vault'), el('div', 'btns', [el('button', 'New Vault…')])]) + +describe('placeVaultSettingsIsland on the phone', () => { + it('mounts between the label and the wrapped button group', () => { + const row = coreRow(['Change…']) + const island = phoneIsland() + place(row, island, true) + assert.deepEqual(labels(row), ['label', 'island', 'actions']) + }) + + it('mounts the same way in a remote workspace, with four buttons', () => { + const row = coreRow([ + 'Change Remote Vault…', + 'Return to Local Vault', + 'Open Local Vault…', + 'Quick Connect…' + ]) + const island = phoneIsland() + place(row, island, true) + assert.deepEqual(labels(row), ['label', 'island', 'actions']) + }) + + it('still mounts before bare buttons', () => { + const row = bareRow(['Change…']) + const island = phoneIsland() + place(row, island, true) + assert.deepEqual(labels(row), ['label', 'island', 'Change…']) + }) + + it('appends when the row has no buttons at all', () => { + const row = el('div', 'row', [el('div', 'label')]) + const island = phoneIsland() + place(row, island, true) + assert.deepEqual(labels(row), ['label', 'island']) + }) +}) + +describe('placeVaultSettingsIsland on a tablet', () => { + it('joins the button group so Manage… sits beside Change…', () => { + const row = coreRow(['Change…']) + const island = el('div', 'manage', [el('button', 'Manage…')]) + place(row, island, false) + assert.deepEqual(labels(row), ['label', 'actions']) + assert.deepEqual(labels(row.children[1]), ['Change…', 'manage']) + }) + + it('appends to the row itself when the buttons are bare', () => { + const row = bareRow(['Change…']) + const island = el('div', 'manage', [el('button', 'Manage…')]) + place(row, island, false) + assert.deepEqual(labels(row), ['label', 'Change…', 'manage']) + }) +}) + +describe('vaultRowActionGroup', () => { + it("never mistakes the island's own buttons for the row's", () => { + const row = coreRow(['Change…']) + const island = phoneIsland() + row.insertBefore(island, row.children[1]) + const group = vaultRowActionGroup(row as unknown as Element, island as unknown as Element) + assert.equal((group as unknown as FakeElement | null)?.label, 'actions') + }) +}) diff --git a/src/ui-mobile/vault-settings-access.ts b/src/ui-mobile/vault-settings-access.ts index 6779f53..3b4d683 100644 --- a/src/ui-mobile/vault-settings-access.ts +++ b/src/ui-mobile/vault-settings-access.ts @@ -5,3 +5,38 @@ export type VaultSettingsAccess = 'quick-switch' | 'manage-button' export function vaultSettingsAccessForLayout(isPhoneLayout: boolean): VaultSettingsAccess { return isPhoneLayout ? 'quick-switch' : 'manage-button' } + +/** The Vault Location row's own actions: the first direct child of the row + * that is, or holds, a button. Core has shipped both shapes, bare buttons in + * the row before 2.58.0 and one wrapping button group since (#871). Looking + * for a direct child, not the first button, is what keeps the island + * mountable: a nested button is not a child of the row, insertBefore throws + * on it, and the island silently never appeared on the phone. The island is + * skipped so its own buttons never count as the row's. */ +export function vaultRowActionGroup(row: Element, island: Element): Element | null { + for (const child of Array.from(row.children)) { + if (child === island) continue + if (child.tagName === 'BUTTON' || child.querySelector('button')) return child + } + return null +} + +/** Mount a vault island into the Vault Location row. + * + * Phone: between the label and the row's actions, so the card reads + * location, picker list, actions, and mobile.css hides the row's actions + * after the island in favor of the island's own group. + * + * Tablet: at the end of the row's button group, so Manage… sits beside + * Change… instead of wrapping onto a line of its own. With bare buttons the + * row itself is the group. */ +export function placeVaultSettingsIsland( + row: Element, + island: Element, + access: VaultSettingsAccess +): void { + const actions = vaultRowActionGroup(row, island) + if (access === 'quick-switch') row.insertBefore(island, actions) + else if (actions && actions.tagName !== 'BUTTON') actions.appendChild(island) + else row.appendChild(island) +}