mirror of
https://github.com/vxcontrol/pentagi.git
synced 2026-08-25 12:36:30 +00:00
fix: repair what the review found in the ledger-closure commits
Five defects, each verified before being accepted: The dialog focus fix was keyed to the wrapper's mount instead of the dialog's open state. Most dialogs here — ConfirmationDialog among them — render DialogContent unconditionally, so the "opener" was whatever had focus when the PAGE mounted and the restore fired on navigation. Moving it into a component rendered inside the content ties it to open/close, and a second spec covers that family, which the first test could not distinguish. Eleven more search-clear buttons were still unnamed: two regex sweeps missed them because an arrow handler contains `=>` and their attribute lists never matched. The 1001-file upload is refused by Go's multipart part cap, not by the handler's own count check — confirmed in the backend log — so the comment claimed a guard the test cannot exercise. Its cleanup also deleted one file of a thousand, because Object.fromEntries keeps only the last of repeated `paths[]` keys. The provider list-delete operated the first row, where a menu wired to a fixed index would look correct; it now deletes the second. And the PDF helper's comment claimed the markdown export is asserted byte for byte, which it is not. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Sergey Kozyrenko
co-authored by
Claude Opus 4.8
Claude Opus 4.8
parent
cbbee09eda
commit
115f90d968
@@ -45,8 +45,9 @@ const inflateStreams = (pdf: Buffer): Buffer[] => {
|
||||
|
||||
/**
|
||||
* Structural read of a generated PDF. It deliberately does not decode the text: each font subset
|
||||
* carries its own glyph map, so a merged decode garbles the result. What the report's text says is
|
||||
* pinned by the markdown export, which is asserted byte for byte from the same source string.
|
||||
* carries its own glyph map, so a merged decode garbles the result. The report's wording is checked
|
||||
* on the markdown export instead, which is built from the same string and whose bytes are searched
|
||||
* for the task and result text.
|
||||
*/
|
||||
export const inspectPdf = (pdf: Buffer): PdfShape => {
|
||||
const head = pdf.subarray(0, 5).toString();
|
||||
|
||||
@@ -187,13 +187,18 @@ export const providersList = (...userDefined: ProviderConfigFragmentFragment[])
|
||||
}),
|
||||
});
|
||||
|
||||
/** A second row so a delete spec can prove the table survived rather than merely emptied. */
|
||||
/** A second row so a delete spec can operate a row that is not index 0, and prove the table survived. */
|
||||
export const OTHER_PROVIDER = {
|
||||
id: 'custom-2',
|
||||
name: 'Second Endpoint',
|
||||
};
|
||||
|
||||
export const OTHER_PROVIDER_ROW = (): ProviderConfigFragmentFragment =>
|
||||
entity('ProviderConfig', {
|
||||
agents: agentsConfig(),
|
||||
createdAt: T,
|
||||
id: 'custom-2',
|
||||
name: 'Second Endpoint',
|
||||
id: OTHER_PROVIDER.id,
|
||||
name: OTHER_PROVIDER.name,
|
||||
type: ProviderType.Custom,
|
||||
updatedAt: T,
|
||||
});
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
import { expect, test } from '../../fixtures/test.ts';
|
||||
import { scanA11y, waiversForScan } from '../../helpers/a11y.ts';
|
||||
import { expectCleanPage } from '../../helpers/errors.ts';
|
||||
import { resourcesCassette } from '../../mocks/cassettes/resources.ts';
|
||||
import { FILE_RESOURCE, resourcesCassette } from '../../mocks/cassettes/resources.ts';
|
||||
import { populatedSettingsProvidersCassette } from '../../mocks/cassettes/settings-providers.ts';
|
||||
import { loginJourneyCassette } from '../../mocks/cassettes/smoke.ts';
|
||||
import { ROUTE_MANIFEST } from '../../routes.ts';
|
||||
@@ -93,4 +93,23 @@ test.describe('dialog keyboard contract', { tag: '@cross' }, () => {
|
||||
await expect(opener).toBeFocused();
|
||||
expectCleanPage(pageErrorLog);
|
||||
});
|
||||
|
||||
// The other dialog family: ConfirmationDialog renders its DialogContent unconditionally, so a
|
||||
// focus hook living on the wrapper would capture whatever had focus when the PAGE mounted and
|
||||
// restore it on navigation instead. This case is what tells the two apart.
|
||||
test('Escape returns focus for a dialog whose content is always mounted', async ({ page, pageErrorLog }) => {
|
||||
await page.goto('/resources');
|
||||
await page.getByRole('checkbox', { name: `Select ${FILE_RESOURCE.name}` }).click();
|
||||
|
||||
const opener = page.getByRole('button', { exact: true, name: 'Delete' });
|
||||
|
||||
await opener.click();
|
||||
await expect(page.getByRole('dialog')).toBeVisible();
|
||||
|
||||
await page.keyboard.press('Escape');
|
||||
await expect(page.getByRole('dialog')).toBeHidden();
|
||||
|
||||
await expect(opener).toBeFocused();
|
||||
expectCleanPage(pageErrorLog);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -35,7 +35,11 @@ test.describe('resources upload limits at the endpoint', { tag: '@real' }, () =>
|
||||
|
||||
const tooMany = await upload(request, many);
|
||||
|
||||
expect(tooMany.status(), 'one file past the 1000-file cap').toBe(400);
|
||||
// 400, but not from the handler's own count check: Go's multipart reader caps a form at 1000
|
||||
// parts and answers `multipart: message too large` first (confirmed in the backend log), which
|
||||
// makes `resources.MaxUploadFiles` unreachable for this endpoint. What this pins is the
|
||||
// user-visible contract — 1001 is refused and nothing is written.
|
||||
expect(tooMany.status(), 'a batch past 1000 files is refused').toBe(400);
|
||||
|
||||
const afterReject = await request.get('/api/v1/resources/', { params: { recursive: 'true' } });
|
||||
const rejectedPaths = ((await afterReject.json()).data.items ?? []).map((item: { path: string }) => item.path);
|
||||
@@ -51,9 +55,15 @@ test.describe('resources upload limits at the endpoint', { tag: '@real' }, () =>
|
||||
|
||||
expect(atCap.status(), 'a batch exactly on the cap').toBe(200);
|
||||
|
||||
await request.delete('/api/v1/resources/', {
|
||||
params: Object.fromEntries(many.slice(0, 1000).map(({ name }) => ['paths[]', name])),
|
||||
});
|
||||
// Repeated keys, not an object: `Object.fromEntries` keeps only the last of 1000 identical
|
||||
// `paths[]` entries and would delete one file while leaving 999 on the stand.
|
||||
const cleanup = new URLSearchParams();
|
||||
|
||||
many.slice(0, 1000).forEach(({ name }) => cleanup.append('paths[]', name));
|
||||
|
||||
const deleted = await request.delete(`/api/v1/resources/?${cleanup}`);
|
||||
|
||||
expect(deleted.status(), 'the seeded batch is removed again').toBe(200);
|
||||
});
|
||||
|
||||
// The server sanitises rather than rejects, which is a legitimate choice — what must hold is that
|
||||
|
||||
@@ -4,6 +4,7 @@ import { expect, test } from '../../fixtures/test.ts';
|
||||
import { expectCleanPage } from '../../helpers/errors.ts';
|
||||
import {
|
||||
agentTestResult,
|
||||
OTHER_PROVIDER,
|
||||
OTHER_PROVIDER_ROW,
|
||||
populatedSettingsProvidersCassette,
|
||||
providersList,
|
||||
@@ -160,8 +161,6 @@ test.describe('settings provider edit paths', { tag: '@coverage' }, () => {
|
||||
|
||||
expect(variables.providerId).toBe(SEEDED_PROVIDER.id);
|
||||
expect(variables.name).toBe(RENAMED);
|
||||
// The mutation has no `type` argument, so an edit can never move a provider between types.
|
||||
expect(variables.type).toBeUndefined();
|
||||
expect(Object.keys(variables.agents), 'the whole agents map is resubmitted').toHaveLength(13);
|
||||
await expect(page, 'a saved provider returns to the list').toHaveURL(/\/settings\/providers$/);
|
||||
expectCleanPage(pageErrorLog);
|
||||
@@ -210,14 +209,14 @@ test.describe('settings provider edit paths', { tag: '@coverage' }, () => {
|
||||
{
|
||||
data: { deleteProvider: 'success' } as never,
|
||||
setFlag: 'provider-deleted',
|
||||
variables: { providerId: SEEDED_PROVIDER.id },
|
||||
variables: { providerId: OTHER_PROVIDER.id },
|
||||
},
|
||||
],
|
||||
},
|
||||
queries: {
|
||||
settingsProviders: [
|
||||
{ data: providersList(seededProviderRow(), OTHER_PROVIDER_ROW()) },
|
||||
{ data: providersList(OTHER_PROVIDER_ROW()), whenFlag: 'provider-deleted' },
|
||||
{ data: providersList(seededProviderRow()), whenFlag: 'provider-deleted' },
|
||||
],
|
||||
},
|
||||
}),
|
||||
@@ -226,7 +225,9 @@ test.describe('settings provider edit paths', { tag: '@coverage' }, () => {
|
||||
test('the row menu deletes the row it belongs to', async ({ page, pageErrorLog }) => {
|
||||
await page.goto('/settings/providers');
|
||||
|
||||
const row = page.getByRole('row', { name: new RegExp(SEEDED_PROVIDER.name) });
|
||||
// The second row on purpose: a menu wired to a fixed index would still send the first
|
||||
// row's id, and a spec that operated row one could not tell the two apart.
|
||||
const row = page.getByRole('row', { name: new RegExp(OTHER_PROVIDER.name) });
|
||||
|
||||
await expect(row).toBeVisible();
|
||||
await row.hover();
|
||||
@@ -237,11 +238,11 @@ test.describe('settings provider edit paths', { tag: '@coverage' }, () => {
|
||||
|
||||
await page.getByRole('dialog').getByRole('button', { name: 'Delete' }).click();
|
||||
|
||||
expect((await request).postDataJSON().variables).toEqual({ providerId: SEEDED_PROVIDER.id });
|
||||
expect((await request).postDataJSON().variables).toEqual({ providerId: OTHER_PROVIDER.id });
|
||||
// The row goes only because the refetch answers without it, and the sibling proves the
|
||||
// table itself survived — a vanished table would satisfy a bare toBeHidden() too.
|
||||
await expect(row).toBeHidden();
|
||||
await expect(page.getByRole('row', { name: /Second Endpoint/ })).toBeVisible();
|
||||
await expect(page.getByRole('row', { name: new RegExp(SEEDED_PROVIDER.name) })).toBeVisible();
|
||||
expectCleanPage(pageErrorLog);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -23,22 +23,6 @@ function DialogClose({ ...props }: React.ComponentProps<typeof DialogPrimitive.C
|
||||
}
|
||||
|
||||
function DialogContent({ children, className, ...props }: React.ComponentProps<typeof DialogPrimitive.Content>) {
|
||||
// Radix hands focus back to its own DialogTrigger, and almost every dialog here is a controlled
|
||||
// `<Dialog open>` with no trigger — and its content is unmounted by the page before Radix's
|
||||
// close sequence runs at all, so `onCloseAutoFocus` never fires. Without this, closing a dialog
|
||||
// drops focus on <body> and a keyboard user restarts from the top of the page. Captured during
|
||||
// the first render, because by the time effects run focus is already inside the dialog.
|
||||
const [opener] = React.useState(() => document.activeElement as HTMLElement | null);
|
||||
|
||||
React.useEffect(
|
||||
() => () => {
|
||||
if (opener?.isConnected) {
|
||||
opener.focus();
|
||||
}
|
||||
},
|
||||
[opener],
|
||||
);
|
||||
|
||||
return (
|
||||
<DialogPortal>
|
||||
<DialogOverlay />
|
||||
@@ -50,6 +34,10 @@ function DialogContent({ children, className, ...props }: React.ComponentProps<t
|
||||
data-slot="dialog-content"
|
||||
{...props}
|
||||
>
|
||||
{/* Radix returns focus to its own DialogTrigger; these dialogs are opened from state,
|
||||
and their content is often unmounted before Radix's close sequence runs, so its
|
||||
`onCloseAutoFocus` never fires. */}
|
||||
<DialogFocusReturn />
|
||||
{children}
|
||||
<DialogPrimitive.Close
|
||||
className="ring-offset-background focus:ring-ring data-[state=open]:bg-accent data-[state=open]:text-muted-foreground absolute top-4 right-4 rounded-sm opacity-70 transition-opacity hover:opacity-100 focus:ring-2 focus:ring-offset-2 focus:outline-hidden disabled:pointer-events-none"
|
||||
@@ -74,6 +62,28 @@ function DialogDescription({ className, ...props }: React.ComponentProps<typeof
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Rendered inside the content, so it mounts when the dialog opens and unmounts when it closes —
|
||||
* a wrapper-level hook would instead run when the PAGE mounts, because most dialogs here render
|
||||
* `<DialogContent>` unconditionally and let Radix decide whether it is on screen.
|
||||
*/
|
||||
function DialogFocusReturn() {
|
||||
// Captured during this component's first render: by the time effects run, Radix has already
|
||||
// moved focus inside the dialog.
|
||||
const [opener] = React.useState(() => document.activeElement as HTMLElement | null);
|
||||
|
||||
React.useEffect(
|
||||
() => () => {
|
||||
if (opener?.isConnected) {
|
||||
opener.focus();
|
||||
}
|
||||
},
|
||||
[opener],
|
||||
);
|
||||
|
||||
return null;
|
||||
}
|
||||
|
||||
function DialogFooter({ className, ...props }: React.ComponentProps<'div'>) {
|
||||
return (
|
||||
<div
|
||||
|
||||
@@ -157,6 +157,7 @@ function FlowAgents() {
|
||||
{field.value && (
|
||||
<InputGroupAddon align="inline-end">
|
||||
<InputGroupButton
|
||||
aria-label="Clear agent search"
|
||||
onClick={() => {
|
||||
form.reset({ search: '' });
|
||||
setDebouncedSearchValue('');
|
||||
|
||||
@@ -339,6 +339,7 @@ export function FlowForm({
|
||||
{templateSearch && (
|
||||
<InputGroupAddon align="inline-end">
|
||||
<InputGroupButton
|
||||
aria-label="Clear template search"
|
||||
onClick={(event) => {
|
||||
event.stopPropagation();
|
||||
setTemplateSearch('');
|
||||
@@ -393,6 +394,7 @@ export function FlowForm({
|
||||
{resourceSearch && (
|
||||
<InputGroupAddon align="inline-end">
|
||||
<InputGroupButton
|
||||
aria-label="Clear resource search"
|
||||
onClick={(event) => {
|
||||
event.stopPropagation();
|
||||
setResourceSearch('');
|
||||
@@ -577,6 +579,7 @@ export function FlowForm({
|
||||
{providerSearch && (
|
||||
<InputGroupAddon align="inline-end">
|
||||
<InputGroupButton
|
||||
aria-label="Clear provider search"
|
||||
onClick={(event) => {
|
||||
event.stopPropagation();
|
||||
setProviderSearch('');
|
||||
|
||||
@@ -526,6 +526,7 @@ function FlowAssistantMessages({ className }: FlowAssistantMessagesProps) {
|
||||
{field.value && (
|
||||
<InputGroupAddon align="inline-end">
|
||||
<InputGroupButton
|
||||
aria-label="Clear message search"
|
||||
disabled={isAssistantCreating}
|
||||
onClick={() => {
|
||||
form.reset({ search: '' });
|
||||
|
||||
@@ -216,6 +216,7 @@ function FlowAutomationMessages({ className }: FlowAutomationMessagesProps) {
|
||||
{field.value && (
|
||||
<InputGroupAddon align="inline-end">
|
||||
<InputGroupButton
|
||||
aria-label="Clear message search"
|
||||
onClick={() => {
|
||||
form.reset({ search: '' });
|
||||
setDebouncedSearchValue('');
|
||||
|
||||
@@ -99,6 +99,7 @@ function FlowScreenshots() {
|
||||
{field.value && (
|
||||
<InputGroupAddon align="inline-end">
|
||||
<InputGroupButton
|
||||
aria-label="Clear screenshot search"
|
||||
onClick={() => {
|
||||
form.reset({ search: '' });
|
||||
setDebouncedSearchValue('');
|
||||
|
||||
@@ -115,6 +115,7 @@ function FlowTasks() {
|
||||
{field.value && (
|
||||
<InputGroupAddon align="inline-end">
|
||||
<InputGroupButton
|
||||
aria-label="Clear task search"
|
||||
onClick={() => {
|
||||
form.reset({ search: '' });
|
||||
setDebouncedSearchValue('');
|
||||
|
||||
@@ -192,6 +192,7 @@ function FlowTerminal() {
|
||||
)}
|
||||
{field.value && (
|
||||
<InputGroupButton
|
||||
aria-label="Clear terminal search"
|
||||
onClick={handleClearSearch}
|
||||
size="icon-xs"
|
||||
title="Clear search"
|
||||
|
||||
@@ -158,6 +158,7 @@ function FlowTools() {
|
||||
{field.value && (
|
||||
<InputGroupAddon align="inline-end">
|
||||
<InputGroupButton
|
||||
aria-label="Clear tool search"
|
||||
onClick={() => {
|
||||
form.reset({ search: '' });
|
||||
setDebouncedSearchValue('');
|
||||
|
||||
@@ -159,6 +159,7 @@ function FlowVectorStores() {
|
||||
{field.value && (
|
||||
<InputGroupAddon align="inline-end">
|
||||
<InputGroupButton
|
||||
aria-label="Clear vector store search"
|
||||
onClick={() => {
|
||||
form.reset({ search: '' });
|
||||
setDebouncedSearchValue('');
|
||||
|
||||
Reference in New Issue
Block a user