Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 65 additions & 0 deletions e2e/app.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,15 @@ function toolText(payload: unknown): string {

test.skip(!url, 'KOZOU_TEST_DATABASE_URL not set');

/** Read through `getPropertyValue` rather than a camelCase field, and typed as
* the one method that is called. Both because this suite typechecks under the
* node tsconfig, which has no DOM globals - and because a hand-written record of
* camelCase fields makes the type authoritative over the real API: a misspelled
* or renamed field then reads `undefined`, and `undefined !== '0px'` is true, so
* the assertion passes forever. `getPropertyValue` answers '' for a property it
* does not know, which fails these comparisons instead of passing them. */
type StyleReader = { getPropertyValue: (property: string) => string };

test('two profiles: map, detail pane, and AI view end-to-end', async () => {
const userData = mkdtempSync(join(tmpdir(), 'kozou-desktop-e2e-'));
const app = await electron.launch({
Expand Down Expand Up @@ -182,12 +191,68 @@ test('two profiles: map, detail pane, and AI view end-to-end', async () => {
await expect(page.getByTestId('overview-cards')).toHaveCount(0);
await expect(page.getByTestId('rowaccess-badge-beta')).toHaveCount(1);

// The way to grant row access is a control, and on this surface it is a real
// one. Two attempts at the discoverability of this thing changed only the
// wording; it was a bare run of blue words beside a bordered status badge, with
// no border, background or padding of its own. Asserted as the box it now draws
// rather than as a class name, because a class says nothing about what is on
// screen.
const grant = page.getByTestId('rowaccess-enable-beta');
const box = async (loc: typeof grant): Promise<{ tag: string; h: number; w: number; border: string; bg: string }> =>
loc.evaluate((el) => {
const s = (globalThis as unknown as { getComputedStyle: (e: unknown) => StyleReader })
.getComputedStyle(el);
const r = el.getBoundingClientRect();
return {
tag: el.tagName,
h: Math.round(r.height),
w: Math.round(r.width),
border: s.getPropertyValue('border-top-color'),
bg: s.getPropertyValue('background-color'),
};
});
const barGrant = await box(grant);
// A real <button> here, so Enter and Space work without this app reimplementing
// them. The card cannot have one (it is itself a <button>) and keeps the span.
expect(barGrant.tag).toBe('BUTTON');
// The box, not "has some border and some background": a bare <button> with this
// app's rule deleted still carries Chromium's own 2px border, #efefef fill and
// 6px padding, so three inequalities on those pass while the control is back to
// a run of text. Measured here: 109x22 against the 92x14 it replaced, and the
// app's own blue rather than the platform's grey.
expect(barGrant.h).toBeGreaterThanOrEqual(20);
expect(barGrant.w).toBeGreaterThanOrEqual(100);
expect(barGrant.border).toBe('rgb(47, 111, 237)');
expect(barGrant.bg).toBe('rgb(238, 243, 255)');

// F1: the all-databases view puts every profile side by side, which is what
// makes coverage comparable; the cards carry counts and annotation coverage.
await page.getByTestId('rail-all').click();
await expect(page.getByTestId('overview-cards')).toBeVisible();
await expect(page.getByTestId('profile-bar')).toHaveCount(0);
await expect(page.getByTestId('rowaccess-badge-beta')).toHaveCount(1);

// On the card it is a span carrying role=button, and it has to stay one: nesting
// a <button> in the card's own <button> is invalid HTML, and the browser's repair
// for it is to close the outer element early - which would take the rest of the
// card out of the button that selects the profile. Same presentation either way.
const cardSpan = page.getByTestId('rowaccess-enable-beta');
const cardGrant = await box(cardSpan);
const cardRole = await cardSpan.evaluate((el) => ({
role: el.getAttribute('role'),
focusable: el.getAttribute('tabindex'),
}));
expect(cardGrant.tag).toBe('SPAN');
expect(cardRole).toEqual({ role: 'button', focusable: '0' });
// Same presentation on both surfaces - measured against the bar's own numbers
// rather than restated, since "they match" is the property and two separate
// property sets would not notice them drifting apart.
expect({ h: cardGrant.h, w: cardGrant.w, border: cardGrant.border, bg: cardGrant.bg }).toEqual({
h: barGrant.h,
w: barGrant.w,
border: barGrant.border,
bg: barGrant.bg,
});
await expect(page.getByTestId('card-alpha')).toContainText('tables');
await expect(page.getByTestId('card-alpha')).toContainText('annotated');

Expand Down
3 changes: 3 additions & 0 deletions src/renderer/src/OverviewCards.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,9 @@
onkeydown={(e) => e.key === 'Enter' && (e.stopPropagation(), ondelete(p.name))}>delete</span
>
</div>
<!-- No `asButton`: the card is itself a <button> that selects the profile, so
the controls in here stay spans. That is also the default, so a host that
says nothing cannot get this wrong. -->
<RowAccessBlock
profile={p.name}
level={p.rowAccess}
Expand Down
4 changes: 4 additions & 0 deletions src/renderer/src/ProfileBar.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,14 @@
>
</span>
</div>
<!-- `asButton`: nothing here is an interactive host (the bar's own `delete` has
been a real button since this component existed), so the grant can be one
too and take Enter and Space from the browser. -->
<RowAccessBlock
profile={profile.name}
level={profile.rowAccess}
pending={rowAccessPending === profile.name}
asButton
{onrowaccess}
/>
<McpBlock
Expand Down
150 changes: 111 additions & 39 deletions src/renderer/src/RowAccessBlock.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
profile,
level,
pending,
asButton = false,
onrowaccess,
}: {
profile: string;
Expand All @@ -23,6 +24,23 @@
* The dialog is modal to the window, so at most one request is ever in
* flight app-wide. */
pending: boolean;
/** Whether this host can hold a real <button>. Only the profile bar can: the
* overview card is itself a <button> that selects the profile, and a <button>
* inside a <button> is invalid HTML — the accessibility tree of a nested
* interactive element is not defined, and screen readers disagree about what
* to announce.
*
* (What it is NOT: a rendering failure. Svelte builds the card and this block
* as separate templates and joins them by DOM insertion, so the parser's
* repair — which would close the card's <button> early — never runs, and the
* nesting simply survives. An earlier version of this comment claimed
* otherwise. Measured, the card keeps all its children either way.)
*
* Declared by the host and OFF by default, so that the answer a host gets by
* forgetting is the one that renders correctly anywhere. The default used to
* be spelled the other way round, `insideButton = false`, which read as the
* safe default and was the opposite of one. */
asButton?: boolean;
onrowaccess: (name: string, level: RowAccess) => void;
} = $props();

Expand Down Expand Up @@ -52,56 +70,81 @@

const ra = $derived(rowAccessBadge(level));

/** Keyboard/click helper for the linkish span-buttons. `stopPropagation` is
* unconditional: on the overview card these sit inside the card's own
* <button> (which selects the profile), and a click that enables browsing
* must not also select. In the profile bar there is nothing above them to
* stop, so it costs nothing there. Spans rather than buttons for the same
* reason — a <button> inside a <button> is not valid HTML. */
/** Keyboard/click helper for the controls. `stopPropagation` is unconditional:
* on the overview card these sit inside the card's own <button> (which selects
* the profile), and a click that enables browsing must not also select. In the
* profile bar there is nothing above them to stop, so it costs nothing there.
*
* The span branch adds a keydown handler for Enter only. A real button answers
* Enter AND Space; this does not, and on the card Space is worse than inert —
* the key reaches the card's own <button> and selects the profile instead, and
* Enter fires both, because Chromium's default handlers walk the event path and
* are gated on preventDefault rather than on propagation. That is pre-existing
* (the bar's own spans behaved the same before this control had a box) and it is
* recorded rather than fixed here. It is also the sharper reason the bar takes a
* real button: on that surface the keyboard simply works. */
const act = (e: Event, fn: () => void): void => {
e.stopPropagation();
fn();
};

/** The grants this level offers, in the order they are offered. One list so the
* two element kinds above cannot drift into offering different things. */
const controls = $derived.by((): { testid: string; cls: string; label: string; run: () => void }[] =>
level === 'off'
? [
{
testid: `rowaccess-enable-${profile}`,
cls: 'grant',
label: 'enable browsing',
run: () => onrowaccess(profile, 'read'),
},
]
: [
...(level === 'read'
? [
{
testid: `rowaccess-edit-${profile}`,
cls: 'grant danger',
label: 'enable editing',
run: () => onrowaccess(profile, 'readwrite'),
},
]
: []),
{
testid: `rowaccess-off-${profile}`,
cls: 'grant danger',
label: 'turn off',
run: () => onrowaccess(profile, 'off'),
},
],
);
</script>

<div class="row rowaccess" data-testid={`rowaccess-${profile}`}>
<span class={`ra-badge ${ra.cls}`} data-testid={`rowaccess-badge-${profile}`}>{ra.text}</span>
{#if pending}
<span class="ra-wait">waiting for approval...</span>
{:else if level === 'off'}
{:else}
<!-- Browsing first, editing as its own step. Each level is a separate
native approval, so nobody arrives at write access by clicking
once — and the dialog for editing says what editing costs. -->
<span
class="linkish"
role="button"
tabindex="0"
data-testid={`rowaccess-enable-${profile}`}
onclick={(e) => act(e, () => onrowaccess(profile, 'read'))}
onkeydown={(e) => e.key === 'Enter' && act(e, () => onrowaccess(profile, 'read'))}
>enable browsing</span
>
{:else}
{#if level === 'read'}
<span
class="linkish danger"
role="button"
tabindex="0"
data-testid={`rowaccess-edit-${profile}`}
onclick={(e) => act(e, () => onrowaccess(profile, 'readwrite'))}
onkeydown={(e) => e.key === 'Enter' && act(e, () => onrowaccess(profile, 'readwrite'))}
>enable editing</span
>
{/if}
<span
class="linkish danger"
role="button"
tabindex="0"
data-testid={`rowaccess-off-${profile}`}
onclick={(e) => act(e, () => onrowaccess(profile, 'off'))}
onkeydown={(e) => e.key === 'Enter' && act(e, () => onrowaccess(profile, 'off'))}
>turn off</span
>
{#each controls as c (c.testid)}
{#if !asButton}
<span
class={c.cls}
role="button"
tabindex="0"
data-testid={c.testid}
onclick={(e) => act(e, c.run)}
onkeydown={(e) => e.key === 'Enter' && act(e, c.run)}>{c.label}</span
>
{:else}
<button type="button" class={c.cls} data-testid={c.testid} onclick={(e) => act(e, c.run)}
>{c.label}</button
>
{/if}
{/each}
{/if}
</div>
<!-- Stated at every level, 'off' included: the second approval and the place
Expand Down Expand Up @@ -130,13 +173,42 @@
border-top: 1px dashed #eee;
padding-top: 0.3rem;
}
.linkish {
/* These look like controls, which they had not until now. What they were: blue
0.75rem text with `cursor: pointer` and nothing else — no border, no
background, no padding — sitting next to a status badge that DID have a
border. An operator reading that row sees one bordered thing (the state) and
some coloured words, and the words are the only way to grant anything.
Two attempts at this failed, and both changed the wording; the settled scope
for the discoverability of this control is "wording and presentation", and
presentation had never been touched. So: a box, a background, and room to be
pressed, the same shape as every other control in the app.
Written to cover both element kinds, since the card cannot use a <button>. */
.grant {
border: 1px solid #2f6fed;
border-radius: 6px;
background: #eef3ff;
padding: 0.15rem 0.55rem;
color: #2f6fed;
/* `font: inherit` is for the button, which would otherwise take the platform's
own family and size. Nothing else about the type is set here: an earlier
version added `font-size: 0.72rem` "for the button", which the button was
already inheriting from `.rowaccess` — so all it did was shrink the label
from 12px to 11.52px on both kinds. Making the words 4% smaller is the
wrong direction for a control two people could not find. */
font: inherit;
cursor: pointer;
}
.linkish.danger {
.grant:hover {
background: #dfe9ff;
}
.grant.danger {
border-color: #a00;
background: #fff2f2;
color: #a00;
}
.grant.danger:hover {
background: #ffe6e6;
}
.ra-badge {
border: 1px solid #ccc;
border-radius: 999px;
Expand Down
Loading