Fix detail page error handling and improve loading states - #380
Merged
Conversation
…found" The template and knowledge detail pages dropped `error` from their query and inferred "this record does not exist" from the absence of data. A real load failure — a network drop, a 5xx, a cold-cache backend error on a deep link — therefore rendered "Template not found" / bounced to the list with a toast, offering no way back in short of retyping the URL. Only a genuine 404 should do that; a transient failure should keep the user on the route behind Retry. Both now split the two outcomes the way flow already does: a real error → in-page ErrorState + Retry; a settled-empty result or a not-found error → the existing redirect/not-found card. The `no rows`/`not found` predicate that flow-provider kept privately becomes the shared `lib/errors.ts#isNotFoundError` now that three call sites need it, and flow-provider moves onto it. Proven by a runtime repro, not by reading: knowledge.test.tsx asserts the in-page error + no redirect on a real failure and the redirect on a genuine not-found — reverting the fix drops it to a failure. errors.test.ts pins the predicate's two sides. e2e repros on both detail routes drive it through the production bundle for CI. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
On a cold Tier-2 stack the new-flow form stays invalid — and Submit disabled — until the providers query lands, so clicking it straight away burned the whole 240s test timeout waiting for a disabled control. Wait for it to enable first. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…fetch The five settings surfaces guard their loading and error branches inconsistently. Their queries are cache-and-network, so a subscription- or mutation-driven refetch flips `loading` (and, on a failure, `error`) to true while the cached data is still on screen. Where the guard omits `&& !data`, that refetch replaces a populated list — or a provider/prompt edit form with unsaved changes — with the full-page spinner or error screen for the duration of the round-trip. Each branch now matches the one beside it in the same file, which already carried the guard and the comment "a failed background refetch must not blank a working list": - api-tokens / providers / prompts lists: `if (isLoading)` -> `&& !data` - prompt / provider detail: `if (error)` -> `&& !data` Proven by runtime repro, one per class: settings-provider.test asserts the form survives an error arriving with cached data (revert -> red), and settings-providers.test asserts the populated table survives loading:true with cached rows (revert -> red). The other three are the identical one-line guard against the same cache-and-network behaviour. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The B1-B3/B6/B7 pass guarded the settings detail pages' error branch with `&& !data`, but each detail page has an `if (loading)` branch that runs FIRST, and it was left unguarded — so the fix it was meant to deliver never applied. The queries are cache-and-network, so a background revalidation (a list→detail navigation into a warm cache, or a post-save refetchQueries) reports loading true with cached data present and blanks the edit form to the full-page spinner before the guarded error branch is ever reached. - settings-prompt.tsx / settings-provider.tsx: `if (loading)` -> `&& !data` - template.tsx spinner: `if (!isNew && isLoadingTemplate)` -> `&& !template`, which also realigns it with knowledge.tsx (fixed in 28ab3d2 to gate on the entity, not raw loading) — the two had silently diverged. - docs/list_detail_pages.md: the "canonical render gate" recipe still taught the unguarded `if (isLoading)` it tells new pages to copy; both branches now gate. settings-provider.test gains the loading-with-cached-data case (revert -> red); the detail loading branch had zero coverage before. Found by the adversarial review of the previous fix pass — the guard I applied was one line short. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…record Two ways a detail page redirected the user off to the list when it should not have, both surfaced by an A/B review of this branch: - isNotFoundError matched /not found/i, but the backend's authz failure "requested permission '<perm>' not found" (graph/context.go) also contains "not found". A user who merely lacked a permission was silently bounced to the list instead of seeing the denial. Authz strings now read as real failures. - flow-provider's isFlowMissing dropped the `!flow` guard that its two siblings (flowLoadError and the not-found toast) apply: under errorPolicy:'all' a partial not-found error rides alongside a flow that loaded fine, so the redirect fired on a flow that had rendered correctly. The disjunct is gated on `!flowData?.flow` again, extracted to a pure `deriveFlowMissing` so the regression is unit-tested. errors.test gains the real authz strings (revert the predicate -> red); flow-provider.test covers the partial-error-with-loaded-flow case (revert -> red). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
isTemplatePending ORed the raw Apollo `loading` flag, gating Save/Rename/Delete. Commit 3d5fc75 fixed the sibling *render* gate (`isLoadingTemplate && !template`) but left this `disabled` gate one level down still keyed on raw loading, so a background revalidation greyed out the actions on a form the user had already edited. flow.tsx uses the entity-guarded `isFlowLoading`; template was the only detail page reading raw loading here. The render gates above (both branches carry `!template`) already make the form unreachable without a loaded template, so the loading term only added a dead disabled window — dropped it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… asserts Two e2e gaps surfaced by an A/B review of this branch: - knowledges detail gains an authz-denial case: the backend denial string "requested permission '<perm>' not found" contains "not found", and a naive not-found match would bounce a user who merely lacks access to the list. The spec asserts it stays on the route behind Retry. Proven: reverting errors.ts to the pre-fix predicate turns this red (page bounces to the list). - template-detail and pager header-order specs used findIndex, which returns -1 for an absent label; -1 < any real index, so "Save left of Previous" passed even if the Save/Next button had vanished. Both now assert every referenced button is present before ordering them, so a missing-button regression fails the spec. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…pload The stand job runs against a URL, user and password held as repo secrets, and Playwright's results.json embeds the resolved page URL (baseURL) in navigation and toHaveURL error messages, plus the user in locator text, on any failing run. The `if: always()` upload then publishes results.json as a public-repo artifact for 3 days. GitHub masks secrets in logs but never in artifacts, so a red stand run leaked the stand URL and user. The comment beside the upload claimed results.json carried none of those — false exactly when the upload matters. Adds a redact step (node split/join, literal — safe for password metacharacters) that replaces each secret with <redacted> before upload; proven locally to strip a URL + user from a sample results.json while keeping it valid JSON. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e, not npx The version guard compared @playwright/test against `npx playwright --version`, but the pinned playwright container ships no global playwright package and the step runs before pnpm install — so npx fetched the registry latest and compared the package against that, not against the container. Green only while latest == the pin; the next Playwright release fails every run telling you to bump the tag to the value it already is, and a pin bump without a re-tag passes despite real drift. Reads driverVersion from the image's own /ms-playwright/.docker-info instead. Verified first-hand inside v1.61.1-noble: no global playwright, .docker-info reports 1.61.1, and the fixed check reads pkg=container=1.61.1 with the repo mounted. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… view
The palette gate scanned only each route's default view, so off-palette colours
behind a tab (which Radix unmounts while inactive) were never checked — the a11y
gate iterates tabs, this one did not, and they had drifted. The flow Files tab
carries a live off-palette node (file-manager's expand-all control,
hover:text-blue-400) that went green purely because the panel was unmounted.
Adds a per-tab scan mirroring the a11y gate, with tab-scoped waivers keyed
`${path} [${tab}]`. The file-manager control is waived on the Files tab under the
same "goes with the design pass" rationale it already carries on /resources.
Proven: the Files-tab scan passes against the exact waived offender (a non-empty
`toEqual`), so the scan reaches the panel — the old default-view scan could not.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The mock PR gate runs retries:0, but trace was 'on-first-retry' — so it never
recorded a trace on that tier, while docs/e2e.md ("Debugging a red CI run") and
the auto-posted PR comment both tell you to open trace.zip from the mock tier's
e2e-report artifact. Every red gate run dead-ended the advertised debug path.
Switches trace to 'retain-on-failure' (matching video on the same line), keeping
the stand tier at 'off' for the session-cookie privacy reason. Proven on a clean
host: a failing mock test (retries:0) now writes trace.zip + video.webm.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t reconnect graphql-ws resubscribes active sinks in a microtask after the retry connect, so the reconnect test gated on the wrong signal: after `retries == [false, true]` it raised the flag immediately, and a poll landing in the gap delivered seq:2 to an empty subscriber set — the no-replay contract then lost the frame, timing out. The report flagged it as CI-load-dependent (0/25 local repro); the mechanism is a real ordering gap regardless. Adds MockWorld.subscriberCount(streamKey) and waits for the resubscribe to re-register the sink before raising the flag. Not a repro of the flake (it does not reproduce locally), but it closes the ordering gap the flake rides on. 4/4 green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The lazy-mount check used toBeHidden(), which passes for a detached node AND for a mounted-but-hidden one — so it did not actually verify the "not mounted while Analytics is active" claim it documents; an eager mount (all overview queries firing behind a hidden panel) would still pass. Switches to not.toBeAttached(). Confirmed green: the panel is genuinely unmounted (Radix drops the inactive tab). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…bel set The job triggers on `pull_request: [labeled]`, but the guard tested `contains(labels.*.name, 'e2e:stand')` — the label *set*, not the label that fired the event. So adding ANY label to a PR that already carries `e2e:stand` re-triggered the run, and `cancel-in-progress` then killed the approved, in-flight stand run and re-pinged the environment reviewers. Gates on `github.event.label.name` instead; workflow_dispatch is unchanged, and the job only listens to labeled + dispatch so the event always carries a label name. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The local tier fell back to https://localhost:8443 when E2E_BASE_URL was unset, while stand fail-fasts. Both tiers bake the real (paid) flow-run specs against a real backend, so a bare `E2E_TIER=local pnpm e2e` silently ran a real flow against the developer's dev stack — a paid LLM call, a junk flow, and a sandbox container/volume the wrapper cleanup never removes. Requires E2E_BASE_URL for local too; run-local-tier.sh already supplies it, so the legitimate path is unaffected. Verified: bare local now throws, local + E2E_BASE_URL loads clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The malformed-body guard only rejects non-object top-level payloads, so a body like
{"tools":5} passed it and reached `(payload.tools ?? []).map(...)` — `.map` on a
number throws outside the try/catch and kills the process, dropping any in-flight SSE
streams and violating the guard's stated contract. Guards on Array.isArray before
mapping. Proven: the old expression throws on {"tools":5}, the new one yields "" and
still maps a real tools array.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…meouts test.setTimeout(240_000) was below the sum of the sequential step ceilings (60+30+90+90+90+60 = 420s), so a legitimately slow-but-passing real run was killed mid-step with a generic timeout that masked the real failure — the exact reason the config's own globalTimeout comment gives. Raised to 450_000. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The a11y and palette sweeps clicked a tab and scanned immediately, so the two round-trip panels could be scanned while still skeletons. Skeletons carry no axe or palette violations, so those scans passed on an empty panel instead of the content they exist to check. Tabs now carry a readiness locator beside the name and both sweeps wait for it. Ordering is part of the same defect: the flow auto-opens the Assistant panel when it has no message logs, so an Assistant-first sweep clicked a tab that was already open and asserted a marker that predated the click. Dashboard leads the left-hand pair, and the sweep asserts each panel is absent before its own click so a future reordering that makes an iteration a no-op fails loudly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The pager keeps FlowProvider mounted and only swaps subscription variables, which is the one path where a superseded stream can stay open. Asserting the leaked message is absent from the DOM cannot catch that: messageLogs is keyed by flowId, so a frame carrying the old flow's id is written to the old flow's cache slot and is never rendered under the new one, leaked or not. Assert the mock's live subscriber count for the superseded stream instead, and export the stream-key builder so the spec cannot drift from the mock's format. Verified by holding a stale subscription open in the provider: the new assert fails while every DOM assert stays green. Also restores the class member ordering the linter requires. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sampling window.location on a timer measured the host's speed: on an unloaded machine the sibling landed before the third sample, so the guard that the samples spanned the switch failed 3 runs in 4. Weakening it to a length check made it vacuous instead — the sampling loop always runs its full count. Record every pushState/replaceState the app makes and assert the exact sequence, which is what "without passing through the list" claims. Verified by routing the pager through the list: the trail assert names the detour. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e it to non-forks Concurrency is evaluated for the whole run before the job's `if`, so a run started by any other label joined the same group and cancelled an approved, in-flight stand run — then skipped its own job, leaving nothing in its place. Key the group on the label as well. The file's header promises fork PRs get Tier 1 only, but nothing enforced it. GitHub withholds secrets from fork `pull_request` runs, so a labelled fork PR held reviewers for an environment approval and then failed on empty credentials. Require a non-fork head. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`maxDiffPixelRatio: 0.01` allows 9,216 differing pixels on a 1280x720 baseline — more than the area any single foreground token covers — so the gate could not fail on the palette regression it was tightened for. Verified: swapping the primary token from blue to green passed all 20 baselines before this change and fails 6 of them after. Both baselines and runs render in the pinned container, so the remaining noise is glyph anti-aliasing; 200 pixels covers it with 20/20 still matching. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The dashboard laziness check asserted the Overview heading was unattached straight after goto. The route is lazy, so that is satisfied by "nothing has rendered yet": mounting the panel eagerly with forceMount still passed. Wait for the Analytics panel first — with the gate in place forceMount now fails it. The flow-detail a11y waivers matched `button[aria-label`, which waives button-name and target-size for every labelled button on the route rather than the file-manager controls that actually violate them. Anchored both on the offending nodes; the real violations stay waived and a nameless icon button or an undersized labelled button no longer does. The rejected-login smoke test asserted only the disabled half of the behaviour its name describes. Change a field and assert Sign in comes back. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`DetailNavigationToolbar` was documented as returning `null` when `controller.itemsEmpty`. It has no such branch — it always renders the fragment, and `itemsEmpty` is read nowhere outside the module's own tests. Describe what it actually does: mount straight away and let the buttons show their disabled/"–/0" state until items land. "Prev/Next runs the same matcher the list filter uses" is also untrue. The toolbar's `createTextMatcher` normalizes NFKD and strips combining marks; `DataTable`'s `globalFilterFn` only lowercases. Searching `cafe` hides a `café` row from the table while Prev/Next still steps onto it. Record the divergence rather than the intent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The array guard added earlier covers the container but not its elements:
`tool.function?.name` chains off `function`, not off `tool`, so a body of
{"tools":[null]} throws a TypeError outside the request try/catch and takes the
process down mid-suite. Reproduced: the request returned nothing and the server
stopped answering; every payload now answers 200 with the process still up.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The resolver builds default provider configs without an ID, so gqlgen marshals the bare number 0 for every one of them. The cassette handed each a distinct string id instead, which left `ProviderConfig.keyFields`'s id-0 branch — the one that stops those un-normalisable defaults collapsing onto a single cache entry — unexercised by any test. Serve id 0 and the plain provider name, give each default a distinguishable model so a collapse is observable, and pin the cache contract directly: with the keyFields branch removed the new test reads anthropic's default as openai's. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…roviders The gate sanctioned the flat union of every badge and button variant, so any variant's hue passed on any element — precisely the wrong-variant reuse it was built for. Match each element's palette tokens against a single variant's set, by equality rather than containment: a lone borrowed token is a subset of the variant it came from, and containment waved it through (verified — an outline badge carrying the blue variant's `text-blue-800` passed until the switch). The route sweep also reaches /settings/providers only with the empty seed, so the provider cards were never palette-scanned, unlike in the a11y and visual gates. Add the same dedicated populated sweep. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codegen freshness watched the four inputs but not the generated types.ts, so a push editing only the generated file skipped the check and left the drift to fail someone else's unrelated codegen push. Watch the output too. Stand redaction split results.json on the raw secret values only. auth.setup builds a locator name from the regex-escaped user, and the file it scrubs is JSON, where that backslash is encoded again — so a failed stand login published the login address in a public artifact. Verified against a synthesised results.json: the old pass leaves `qa\\.bot@…` intact, the new one removes it. Also covers the URL-encoded form. The visual-diffs artifact was gated on the snapshot step failing, which a webServer or build failure inside that step also satisfies while still leaving results.json behind — the report then announced "snapshots differ" for an infra failure. Gate on diff images actually existing. Dispatching `tier: all` did nothing: e2e-stand never read its input, and e2e.yml's matching arm could not be selected. Drop both. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…r-broad claims `pageErrorLog` was opt-in with nothing asserting it on teardown, so a spec that never destructured the fixture collected nothing and an uncaught exception on that surface failed nothing. Make it auto and assert on teardown. Verified by throwing from an init script in the populated-providers a11y test, which does not take the fixture: green before, red after. Console errors stay opt-in — several specs drive genuine 4xx paths. The remaining flow-detail waivers keyed on bare utility classes: `bg-primary` matched every Progress root through `bg-primary/20`, and the other two were similarly unanchored. Pin all three to the offending nodes, taken from a run with the waivers disabled. /templates and /knowledges claimed whole page dirs, so a diff touching only the unswept detail page scoped to the list route instead of the run-everything fallback. Name the list files; leave `src/features/knowledges` unowned, since its only importer is that unswept route. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Deleting `eligible`'s second line — the one that makes a newly-enabled flagged entry outrank the unflagged candidates — left the whole unit tier green. The existing cases use two entries, where the consumed-index cursor lands on the flagged entry anyway, so sequencing alone reproduced every asserted progression. Add a three-candidate case where the two disagree: it passes as written and fails with that line removed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ing the run `vitest run src/components/shared/markdown-editor` exits 1 at HEAD while all 472 tests read green. Radix closes a still-open layer asynchronously, so a test that unmounts with the list menu open lands `onCloseAutoFocus` on a torn-down editor; `editor.commands` throws off any test's stack, three times, and only the process exit code carries it. The same path is reachable in the product whenever the editor unmounts while a menu or popover is open — a route change from an open Link popover does it. Seven popovers and dropdowns carried the same two-line handler, so the guard lives in one `returnFocusToEditor` they now share rather than in seven copies. I reported this suite green yesterday. The run was piped through `grep`, which returned the exit code of `grep` — the mistake the repo's own pipe-exit-code note describes. The gate is checked by exit code here.
…line backslashes
Two halves of the same escape/unescape pair, both wrong, and they had to move
together.
**Save side.** `escapeLineLeadingBlockMarkers` demanded a literal space after the
marker (`#{1,6} ` / `> `), but marked 18 makes it optional: the heading rule is
`/^ {0,3}(#{1,6})(?=\s|$)…/` — a lookahead — and the blockquote rule is `> ?`.
So a typed paragraph shipped bytes that read back as a different block:
`>foo` reloads as a blockquote, and the next save writes `> foo`
`#` reloads as an EMPTY heading, and the next save writes nothing at all
`###` the same — the line is gone
`#<TAB>` reloads as a heading
**Load side.** The `\#`/`\>` unescape lived in the inline `escape` tokenizer,
which has no notion of position, so it fired mid-line where nothing had escaped
anything:
`grep '\<root\>'` loads as `grep '\<root>'` — the word-boundary operator
`cat file \> out` loads as `cat file > out`
It cannot simply move to the `lex` pre-pass the way the table-pipe escape did:
`lex` runs before block tokenization, so unescaping there hands marked a live
`#`/`>` and re-creates exactly the reinterpretation the save side is preventing —
measured, `\# not a heading` becomes a heading token. It belongs in
`inlineTokens`, which runs after blocks are settled and still sees line starts.
Tests start from a paragraph NODE, not from markdown: `>foo` as stored markdown
is legitimately a quote, so a markdown-in/markdown-out oracle would have pinned
the bug as correct. Five rows fail on the old save regex, four on the old
tokenizer placement.
Shift+Enter then a line of `=` or `-` turned the whole paragraph into a heading and destroyed the run: `foo` + hard break + `===` saved as `foo \n===`, which marked reads as an H1, and the next save wrote `# foo`. The trigger is wider than a three-character rule — marked's lheading run is `(=+|-+) *`, so a single `-` after a hard break was enough, and trailing spaces are allowed. All six shapes are covered. Only continuation lines are escaped. A paragraph's first line is preceded by a blank line, where lheading cannot fire, so escaping it would add a backslash for nothing. The load side unescapes exactly the shape the serializer writes — a backslash before a run that FILLS the line — so an author's `\-` inside `[a-z\-_]` and a `\=` inside a sed expression keep their backslash. Extending the old inline escape tokenizer to `=`/`-` instead would have eaten both; the tests pin them.
…nce opener
marked's fence rule carries a lookahead on the backtick branch only —
`` `{3,}(?=[^`\n]*(?:\n|$)) `` — so a prose line like ```` ```pnpm run dev``` starts it ````
is an inline code span, not a fence. The pipe scanner had no such lookahead, so
it opened a fence marked never opened and the two ran out of step in both
directions:
* a table after that line lost pipe protection entirely — a cell holding
`` `x | y` `` was torn in two and the trailing cell silently dropped;
* once the phantom fence closed on the next bare ```, the parity was inverted
and the scanner escaped pipes INSIDE a real code block, writing a backslash
into code content that then persisted through save.
The regex is now a literal mirror of marked's; tilde fences stay unrestricted,
as they are there. The same constant is element [1] of ENDS_TABLE_BODY, whose
marked counterpart uses the same lookahead-carrying sub-pattern, so that use
becomes more faithful too.
Tests cover both directions — the report described only the dropped cell.
A list item indents its content to its own column. Once that column reaches 4 —
which is simply where a table under a nested bullet sits — the pipe scanner read
the delimiter row as indented code and skipped the whole table, while marked
dedents the item and re-lexes, where the table is live. A cell holding
`` `x | y` `` was torn in two and the trailing cell silently dropped.
The report framed this as an over-indented top-level bullet, which reads like a
typo. The reachable shape is the ordinary one:
- outer
- inner
| Op | Meaning |
| --- | --- |
| `x | y` | KEEP |
Mirroring marked means dedenting by the item's content column and recursing, not
relaxing the leading-space cap: the indented-code rule has to keep applying
RELATIVE to the item, so a line four columns past the content start still goes
unprotected, and a four-space-indented table at TOP level still does — both
pinned. The content column follows CommonMark, including the 5+-spaces case
where the item opens with indented code.
Recursion composes with the existing blockquote branch in both directions, so a
list inside a quote is covered; that case is pinned too.
Note on the run that produced this: eight unrelated tests timed out mid-way and
looked like a regression. They were host saturation from my own leftover vitest
processes (load average 120). `escapeTablePipes` on a real 14 KB template
measures 0.56 ms/call, and the corpus suite passes 39/39 in 3.7 s with this
change in place.
…scape
`| {{.Host | urlquery}} |` was saved as `{{.Host \| urlquery}}`. That is not a
cosmetic backslash: Go text/template rejects the file outright — verified by
parsing both forms with the real package —
"| a | {{.Host | urlquery}} |" -> nil
"| a | {{.Host \| urlquery}} |" -> unexpected "\" in operand
so the prompt could not be saved at all, and every save from rich mode re-added
the backslash, leaving no way out. The server's ValidatePrompt is what caught it,
which is why this shows up as a hard save failure rather than a silently broken
prompt reaching the agents.
Dropping the escape alone would have traded a backslash for losing the table: an
unescaped pipeline in the HEADER row counts one cell more than the delimiter,
detection bails, and marked degrades the whole table to a paragraph. So the
loader's cell count masks action pipes first — that is the load-bearing half of
this change, and the header case is pinned.
The load-side escape inside `escapeRowPipes` stays: it hides the pipe from
marked's row splitter, which unescapes it back into the cell text. Save no longer
adds one of its own, so the two sides are symmetric.
Two tests pinned the escaped form as expected output; both now pin the form Go
actually accepts.
…s and alt text Four popover defects that all end the same way — a value the user entered reaches the document in a form markdown cannot read back. **Whitespace (F5).** `new URL()` accepts an inner space, so `example.com/my page` persisted as `[hello](https://example.com/my page)`, which reloads with the link gone and the brackets leaking into the text. Both normalizers now reject whitespace. NOT routed through `new URL().href` as the report suggested: measured against the inputs the field accepts today, that also rewrites `example.com` to `https://example.com/`, percent-encodes `{{HOST}}` placeholders this project puts in template content, and percent-encodes non-ASCII paths. **Unknown schemes (F21).** `https://` was prefixed onto anything unrecognised, so `ftp://host/file` became `https://ftp://host/file` — host `ftp`, no validation error, a dead link persisted. Rejected on `scheme://` rather than a bare `scheme:`, because a bare-colon pattern also matches `localhost:3000` and `example.com:8080/path`, which are ordinary scheme-less input and stay accepted. **Image alt (F8).** An unbalanced bracket broke `` outright: the reload parsed ZERO image nodes. Escaped at serialization, where the parse side's outputLink pass unescapes it back. **Link label (F7).** Same class, but it cannot be fixed at the same layer: @tiptap/markdown hands a mark's renderMarkdown a placeholder and splices the text in afterwards, so the label's own bytes are never visible there — verified before settling for the insert path. The Link popover, which is where the reported case comes from, no longer puts brackets into the label it synthesises from the URL; the href keeps them. Typing `]` inside an existing link is the residual, recorded rather than papered over. Tests pin both directions of each guard, including the values that must stay byte-identical, and the convergence property rather than the regex itself.
react-hook-form merges the form-level `resetOptions` into every manual `reset()`, so `keepDirtyValues: true` — set so a subscription resync cannot wipe an unsaved body — was silently inherited by post-save resets and by cross-entity navigation. Three symptoms, one cause. **Post-save reset (both call sites).** The reset that runs after a successful save kept the pre-save values and their dirty flags, so editing a question, saving, then renaming the document from the header inline-edit sent the stale question back on the next save and reverted the rename server-side. Both resets now opt out explicitly. **Templates.** One React element serves every `/templates/:templateId`, so the form instance survived the param change: edit A, go to next, discard, and B's form still held A's edited body — Save then wrote it to B. `/templates/new` inherited the same abandoned draft. Keyed by id, which is what `knowledge.tsx` already does for `<KnowledgeForm>` and why that page never had the leak. **Prompts.** Same shape on `/settings/prompts/:promptId` via a POP across prompt URLs. Keying also fixes the stranded `activeTab`: `human` carried onto a tool prompt renders no TabsContent, and the header Save then submitted a form id that no longer existed and did nothing. Keying is preferred over resetting on param change, which the report offered as an alternative: the same effect is the deliberate keepDirtyValues resync, so a blanket opt-out there would reintroduce the wipe it exists to prevent. Suite note: on this host `pnpm run test` reports 12 files failing under full worker parallelism and passes 1345/1345 with `--maxWorkers=2`. Every failure is a timeout; the machine is CPU-bound from outside this repo.
A cell serialises inline, so from a caret inside one these controls silently
degraded the document on a single click: a code block saved as
`| ``` alpha ``` |` and reloaded as an INLINE span, a blockquote came back as
literal `\> alpha` text, and a horizontal rule applied mid-word split the word
permanently. The whole-document path already refuses tables — the plain-caret
path had no guard at all.
The report's fix was to make the commands report false so the existing `can()`
wiring disables the buttons. That only reaches Blockquote: Code block, Horizontal
rule and the list and heading menu items are deliberately NOT can()-driven,
because their dry run is unfaithful (see the previous commit and its test), so
five of the six controls would have stayed enabled with their clicks turned into
silent no-ops. The guard is structural instead — `isActive('tableCell') ||
isActive('tableHeader')` in the selector, threaded to the controls that corrupt.
Paragraph stays enabled in the heading menu: it is the cell's own block type and
selecting it is how you undo a heading, not a way to break the cell.
… table guard
`toggleHeading` was the one block toggle with no whole-document handling, so
Ctrl+A + "Heading 2" converted every table-cell paragraph into a heading. That
saves as `| ## h | ## i |`, reloads as literal cell text, and escalates on each
further save (`## h` becomes `\## h`).
Reusing the existing block-type family unchanged — the report's instruction —
produces two new wrong outputs, both measured: every choice lands as H1, because
the family calls `setBlockType` with no attrs and had nowhere to receive the
command's `{ level }`; and an all-H1 document reads as "already Heading 2" and
gets demoted to paragraphs, because "applied" compared node type only. So the
family now threads the command's attrs through both the predicate and the write.
`codeBlock`, which shares the family, passes no attrs and is unaffected — the
comparison degenerates to the old type-only check.
GFM cannot represent a table without a header, and renderTableToMarkdown answers that by emitting an EMPTY header row above the demoted rows. So switching the header off and saving grew the table by one blank row per cycle (2 → 3 → 4) and the switch silently flipped back on, since the reloaded table has a header again. Promote the first row instead. The row count and every cell survive, the header switch keeps its in-session meaning, and a second save is a no-op because the promoted table already has a header — where the old behaviour was unbounded. The report also suggested simply disabling switch-off as "honest about the format". Promotion loses no data and keeps the control useful, so it wins.
…miss stick **Shortcut labels.** prosemirror-keymap resolves `Mod` per platform, so `⌘B` and `⇧⌘Z` named keys that do not exist on Windows or Linux. Derived from `isMac()` now, the way `input-search.tsx` already does it. Redo is labelled `Ctrl+Shift+Z` rather than `Ctrl+Y`: the history extension registers both, and the shifted form matches the Mac label. **On-link popover.** `close()` only cleared the target, and the `selectionUpdate` handler re-targeted unconditionally whenever the caret sat in a link — so Escape bought exactly one keystroke before the popover came back. The dismissed link is remembered by its start position (already the popover's key, and stable while the caret stays inside), and cleared when the caret leaves the mark or an edit shifts that start. Not live-confirmed: the popover did not open under programmatic selection or synthesised mouse events in this consumer, so the behaviour change rests on a source read plus the type and suite gates, not on a runtime repro.
Three findings from the CSS/highlight review, all in surfaces the token system never reached. **Gap cursor.** tiptap injects `border-top: 1px solid black` for it and nothing overrode that, so the caret you get between two block nodes — press left or up at the start of a leading table or fence, which prompts routinely open with — was a black line on the dark theme's near-black background. Verified live: it now resolves to `--foreground`. **Node selection.** Only `img.ProseMirror-selectednode` was styled, and neither tiptap's injected CSS nor prosemirror-view's stylesheet (imported nowhere) fills the gap, so clicking a horizontal rule produced a NodeSelection with no visual at all — `.ProseMirror-hideselection` blanks the native highlight at the same time, so the next keystroke replaced a node the user could not see was selected. Verified live: the selected `hr` now carries a 2px `--primary` outline. **Viewer code comments.** The editor gave itself an hljs comment override; the read-only viewer kept stock atom-one-dark. Measured on the ground `.prose` actually paints, in a real browser in both themes: `#5c6370` is 2.43 light and 3.25 dark — below AA in both, on the token most present in agent-generated code. The editor's value clears it at 5.24 / 7.03. Only the comment is mirrored. The report also flagged stock red as marginal; measured 4.59 light and 6.15 dark, it passes, so it stays as it is. Contrast gate: the editor-surface and in-fence specs pass in both themes. The badge and button specs time out on this host at load average 40 with no assertion failures, and neither touches a surface this commit changes.
…save
Two tables separated by one blank line never reached a fixed point. The table
renderer emits a newline of its own on top of the block separator the manager
already adds, so the extra line reloaded as an empty paragraph — which then
serialised to another blank line on the next save, and so on:
table,table -> table,paragraph,table -> table,paragraph,paragraph,table
68 bytes -> 88 -> 90 -> 92 -> 94 …
Found while building a kitchen-sink fixture for manual testing, not by either
review — both looked at what a single table does.
The renderer's surrounding blank lines are trimmed; spacing between blocks is the
manager's job. Every existing table test still passes, so the trimmed newline was
never load-bearing for a table on its own.
A paragraph whose first text node is a code span starting with `# ` was promoted to a heading: the autoformat plugin matched the marker on the first child without checking its marks, so `# ` was deleted out of the code span and the block type changed. `` `# Title` trailing `` loaded back as an H1 reading `Title trailing`, and stayed that way, since the corrupted form is itself a fixed point. Skip the promotion when the marker carries the `code` mark — inside a span the hash is content, not markup. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Code fences were painted by the stock atom-one-dark stylesheet, imported for its side effect by the editor and the viewer. That gave every fence a hard-coded #282c34 in BOTH themes: on a light page the editor showed a black slab whose own inline-code chips, which do use a token, were light. A side-effect CSS import cannot be scoped to a theme, so no amount of overriding could make the fence adapt while the stylesheet supplied it. Drop both imports and own the palette. The surface is --editor-code-bg, already declared and already theme-aware, so a block and an inline chip now sit on the same ground. The syntax colours become tokens: the dark set keeps atom-one-dark's hues, and the light set holds atom-one-light's hues and chroma but lowers lightness, because stock one-light fails AA on 6 of its 8 colours against this surface. The viewer's Tailwind Typography internals are re-pointed at the same tokens, so a document reads the same edited and viewed. The fence contrast spec measured only two token classes and pinned the surface to a literal rgb(40, 44, 52); it now covers one class per token group and resolves the expected surface from the token, and a new gate holds that list to the stylesheet so a future token cannot go unmeasured. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Delete 134 comments across the backend that only narrate what the adjacent line already says — "check cache first" above a cache Load, "load from database" above the query, "extract privilege names" above the loop that builds them, and the like. Doc comments, domain-rule notes, and every non-obvious invariant are kept; only pure restatements go. No behaviour change: build, vet, and test compilation are green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`formatName` was defined identically inside both the agent-table and tool-table builders; hoist it to one module-level helper. Remove the `type AgentPrompt = AgentPrompts` alias in both settings-prompt files and collapse the `AgentPrompt | AgentPrompts` casts it fed — a union of a type with its own alias, which is just `AgentPrompts`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The switch over resetType already handles every case the else branch can
see ('all' | 'human' | 'system', 'tool' having returned earlier), and all
14 call sites pass a string literal from that union, so the trailing
`return false` could never run — the compiler flagged it as unreachable.
Remove it; TypeScript proves the function still returns on every path.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…l headers Flow: right after the Report dropdown; template and knowledge: first in the actions group, ahead of Anonymize/Save. Render conditions are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…monMark An inline code span whose content holds a backtick — `` `XSS` ``, `` `key` = value ``, shell substitution, common in pentest write-ups — was written with a fixed single-backtick fence, producing invalid markdown that collapsed further on every save. A real-data sweep found 5 of 393 dev knowledge documents drifting on each save, losing the backticks that were the point (template literals, JSFuck), confirmed end-to-end through the real save mutation. Root cause is architectural in @tiptap/markdown: it derives a mark's delimiter by rendering the mark against a placeholder, so the code fence is blind to the content and can neither widen past an internal backtick run nor add the protective space padding CommonMark requires. Fix it at the seam we already own. serializeCodeSpan sizes the fence one longer than the longest internal run and pads a leading/trailing backtick (mirrors prosemirror-markdown's backticksFor), applied in the text encoder we replace for the code mark only — code BLOCKS keep their own fence. The code mark's renderMarkdown is overridden to emit no fence, so the placeholder-derived delimiter is empty and the content-sized one is the only fence. Verified: 18/18 marked round-trips exact, the corrupting dev docs now converge, table-pipe escaping composes correctly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The prev/next pager was moved left of the fixed actions, so the two header order assertions no longer matched the DOM. Update them to the current layout: on a flow detail, Report → pager → Toggle favorite → Flow actions; on a template detail, pager → Save → Template actions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ager The prev/next pager moved left of the fixed header actions, so the flow detail full-page snapshot no longer matched. Regenerated in the pinned Playwright container; the diff is confined to the header action cluster, every other route baseline is byte-identical. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The prev/next pager moved left of the fixed actions on all three detail headers, but only the flow and template detail headers asserted their order — the knowledge detail header had no order test at all. Add one (pager → Save → Knowledge actions), plus the knowledgeDetailCassette its detail route needs to load. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Updated the git command in the CI code generation tests to include user identity configuration for commits. This change addresses issues with CI runners lacking a global git user setup, ensuring that commits can be made successfully during automated tests.
Simplified the formatting of the git command in the CI code generation tests by consolidating the arguments into a single line. This change enhances readability without altering functionality.
36 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of the Change
This PR merges
feature/frontend(149 commits) intofeature/next-release. It is a quality pass: it makes the end-to-end suite able to falsify the app, closes the CI gates that could pass without checking anything, and fixes the defects that pass then found — mostly in the markdown editor, accessibility, and detail/settings error handling — plus four small backend correctness fixes.Problem
The e2e suite covered the happy paths of list and CRUD pages, but the report route, assistants, flow interruption, session expiry, uploads and downloads had no coverage at all, and several gates (visual, palette, contrast, a11y, page errors) were written so that they could not fail. CI compounded this: the lint/codegen/test gate ran on
pushonly — so a fork PR could edit the schema without regeneratingtypes.tsand still merge green — the codegen freshness check diffed a range that no longer contained the change, and the sticky e2e comment reported a pass it had never read.Once the gates could fail, they exposed real defects: the markdown editor corrupted documents on round trip (tables gaining a blank row per save, invalid code fences, mis-escaped table pipes and line-leading markers), dialogs and icon-only controls were unusable with a screen reader, several syntax-highlight tokens failed WCAG AA, and detail/settings pages misreported an authorization denial as "not found", blanked a working view on a background refetch, or silently discarded unsaved edits.
Solution
E2E coverage and gate integrity. New mock-tier specs for the assistant lifecycle, flow interrupt/answer, live panels, tab deep links, the report route and its four exports, session expiry, and resource uploads; new real-tier specs for the account/password boundary, real file downloads (asserting the exported PDF is a real document), and upload limits from both sides of every boundary. Spec cases grew 59 → 125. Gates that could not fail were closed — the visual gate has an absolute pixel budget and fails on a missing baseline, the palette and contrast gates sweep tab panels and composite text onto its own background, a11y waivers are scoped to the tab that owns the debt, and uncaught page errors are asserted everywhere. Mocks are re-armed per browser context (so a report popup is mocked like the page that opened it), and the cassette matcher can pin a query string. A new
e2e/tools/review-sandbox.shgives review agents a throwaway git worktree, so verifying a gate by breaking it never touches the live checkout.CI. The lint/codegen/test gate now runs on
pull_request(fork-safe, no secrets), the codegen range comes from the PR base via a unit-tested script, the sticky comment is routed by PR identity and never claims a result it did not read, the stand run is gated on repo identity plus its own label, and stand secrets are redacted from the public report artifact.Markdown editor (~20 fixes). Correct CommonMark on save: table pipe escaping (nested in a list item, around a Go template pipeline, in a headerless table), valid fences for labelled code blocks and backtick-containing info strings, setext and line-leading escapes, inline code containing a backtick. Correct toolbar behaviour: block toggles are reversible under a select-all selection, controls are disabled when their command reports unavailable or a table cell cannot hold the block, URLs are rejected when unusable, shortcut labels are platform-correct. Plus caret/selection visibility, a theme-aware code surface, and two perf fixes (the list tokenizers no longer re-split the document per block; unlabelled code blocks no longer trigger language auto-detection).
Accessibility and UI correctness. Focus returns to the control that opened a dialog, sheet or menu; icon-only buttons are named; data-table columns announce their sort state; the highlight palette was retuned so every token clears AA in both themes; link text gets its own token instead of reusing the fill colour. Detail and settings pages now distinguish an authorization denial or a partial error from a missing record (retryable error state instead of a false "not found"), background refetches no longer blank a working view or discard unsaved provider/agent edits, attached resources are cleared after a submit, exported PDFs are no longer named
*.pdf.pdf, and 0-byte uploads are accepted (the API stores them happily).Backend. Password validation is capped at the 72 bytes bcrypt can hash — the old
max=100let a longer password pass validation and then fail insidebcrypt.GenerateFromPassword. Four agent options (minP,n,json,responseMimeType) that the GraphQL round trip silently dropped are now carried through the schema and converter. A flow whose worker fails to start is dropped instead of being left in the listing. The Graphiti startup health check retries three times before permanently disabling the client for the process lifetime.Infrastructure. Compose mounts the host-side
PENTAGI_BEDROCK_CONFIG_PATH(it named the in-container path);.env.localand Playwright output stay out of the docker build context.Closes #
Type of Change
Areas Affected
Testing and Verification
Test Configuration
Test Steps
cd frontend && pnpm run test— Vitest unit suite.cd frontend && pnpm run lintandpnpm typescript.cd backend && go build ./... && go test ./....pnpm run e2e): assistants, interrupt, live panels, tab deep links, report exports, session expiry, uploads, plus the cross-cutting a11y/contrast/palette/visual gates.Test Results
pnpm run lintclean (0 warnings).go build ./...andgo vet ./...clean,gofmtclean on every touched file; the affected packages (controller,database/converter,graphiti,server/models) pass, with new tests for the flow-start cleanup, the agent-option round trip, the health-check retry and the password length rule.Security Considerations
CLAUDE.mdupdated to match).ifgate keeps a mislabeled or fork PR from ever reaching the environment secrets..env.localand any.env.*other than.env.exampleare excluded from the docker build context.Performance Impact
Documentation Updates
CLAUDE.mdpassword policy,frontend/docs/e2e.md(mock scope, downloads, stand gating, review sandbox),frontend/docs/list_detail_pages.mdDeployment Notes
No migrations, no new environment variables.
One compose change requires attention: the Bedrock provider config mount now reads
PENTAGI_BEDROCK_CONFIG_PATH(host path) instead ofBEDROCK_CONFIG_PATH(container path). Deployments that set onlyBEDROCK_CONFIG_PATHand relied on the old mount must addPENTAGI_BEDROCK_CONFIG_PATH; the documented pair inREADME.mdis unchanged in meaning.Passwords longer than 72 bytes are now rejected at registration/change time. No stored credential is affected — such a password could never have been hashed successfully in the first place.
Deployment steps:
docker compose build.docker compose up -d.Checklist
Code Quality
go fmtandgo vet(for Go code)npm run lint(for TypeScript/JavaScript code)Security
Compatibility
Documentation
Additional Notes
Review focus areas:
frontend/src/components/shared/markdown-editor/— the largest single surface; the round-trip fixes are covered bymarkdown-editor-extensions.test.tsandmarkdown-editor-table-pipes.test.ts..github/workflows/and.github/scripts/codegen-inputs-changed.sh— gate semantics, unit-tested bye2e/ci-codegen-gate.unit.test.ts.backend/pkg/controller/flow.go— the deferred cleanup is armed before the first fallible call and disarmed only once the worker owns the flow.frontend/src/lib/errors.ts— the authz-denial carve-out is what keeps a permission error from redirecting as a missing record.