Skip to content

Commit 2724145

Browse files
committed
Fix extract-grid embed crashing CAML editor preview
The "Insert Extract Grid" picker in CamlArticleEditor wraps each marker in a CAML fence so the article renderer can intercept it and mount the ExtractGridEmbed component. The previous fence used `::: prose`, but @os-legal/caml's parseBlock has no `case "prose"` — the resulting block carried `body` instead of `content`, and ProseBlock crashed inside splitPullquotes(undefined). The crash unmounted the editor modal, which is why "ArrowDown then Enter inserts the extract-grid component marker" failed in CT. Fence type is now `::: oc-component`, dispatched via CamlArticle's `customBlocks` slot in both CamlArticleEditor and CamlDirectiveRenderer. The customBlocks renderer hands the fence body back to the existing [component:...] resolver path so behavior for inline markers is unchanged. Also tightened the picker keyboard handler to read activeExtractIndex from a useRef mirror — back-to-back ArrowDown/Enter keystrokes were able to fire before React re-ran useCallback, so Enter saw a stale `-1` and bailed out before insertion. Address PR #1327 review (Claude bot): captured the SYNC_CONTENT fallback messageId in a single local so the visible chat entry and the ChatSourceAtom record share an id when the server omits message_id. CHANGELOG entries updated.
1 parent d220d2b commit 2724145

7 files changed

Lines changed: 139 additions & 35 deletions

File tree

CHANGELOG.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
5353

5454
### Fixed
5555

56-
- **`CorpusChat` dropped `SYNC_CONTENT` messages from the visible chat** (Issue #1276, `frontend/src/components/corpuses/CorpusChat.tsx:468-495`): The `SYNC_CONTENT` WebSocket frame is a standalone, non-streaming assistant reply used for synchronous server responses. `ChatTray` (document chat) appends these directly to its `chat` state; the corpus-level chat only forwarded the content to `handleCompleteMessage`, which stores sources in `ChatSourceAtom` but never pushes a message to the visible list. As a result, any `SYNC_CONTENT` the backend sent over the corpus socket rendered nothing. Fixed by mirroring the `ChatTray` pattern — push a new complete assistant message into `chat` before persisting sources/timeline. New regression test in `frontend/tests/CorpusChat.ct.tsx` ("SYNC_CONTENT renders a complete message immediately") pins the behavior.
56+
- **`CorpusChat` dropped `SYNC_CONTENT` messages from the visible chat** (Issue #1276, `frontend/src/components/corpuses/CorpusChat.tsx:468-505`): The `SYNC_CONTENT` WebSocket frame is a standalone, non-streaming assistant reply used for synchronous server responses. `ChatTray` (document chat) appends these directly to its `chat` state; the corpus-level chat only forwarded the content to `handleCompleteMessage`, which stores sources in `ChatSourceAtom` but never pushes a message to the visible list. As a result, any `SYNC_CONTENT` the backend sent over the corpus socket rendered nothing. Fixed by mirroring the `ChatTray` pattern — push a new complete assistant message into `chat` before persisting sources/timeline. The fallback `crypto.randomUUID()` is also now captured in a single local variable so the visible chat entry and the `ChatSourceAtom` record share the same id when the server omits `message_id`. New regression test in `frontend/tests/CorpusChat.ct.tsx` ("SYNC_CONTENT renders a complete message immediately") pins the behavior.
57+
- **CAML article preview crashed when inserting an extract grid embed** (`frontend/src/utils/camlComponents.ts`, `frontend/src/hooks/useCamlComponentRenderer.tsx`, `frontend/src/components/corpuses/CamlArticleEditor.tsx`, `frontend/src/components/corpuses/caml/CamlDirectiveRenderer.tsx`): the editor wrapped each newly-inserted `[component:TYPE ...]` marker in a `::: prose` fence, but `@os-legal/caml`'s parser has no `case "prose"` in `parseBlock`, so the resulting block carried `body` instead of `content`. `ProseBlock` then crashed inside `splitPullquotes(undefined)`, which unmounted the entire editor modal and made the "ArrowDown then Enter inserts the extract-grid component marker" CT test fail. Switched the fence to a project-specific `::: oc-component` block and routed it through `CamlArticle`'s `customBlocks` slot, where the marker text is handed back to the existing `[component:...]` resolver. The keyboard handler in `CamlArticleEditor` was also tightened to read the active picker index from a `useRef` mirror so back-to-back ArrowDown/Enter keystrokes don't observe a stale closure value of `-1` and bail out before insertion.
5758
- **PR #1177 follow-up: CAML extract embed polish** (Issue #1227):
5859
- **`fullDatacellList` payload now bounded server-side**: `ExtractType.full_datacell_list` accepts optional `limit` / `offset` arguments and the resolver clamps `limit` to `MAX_FULL_DATACELL_LIST_LIMIT` (`opencontractserver/constants/extracts.py`, currently `500`) after permission filtering (`config/graphql/extract_types.py`). `GET_EXTRACT_GRID_EMBED` passes `limit: EXTRACT_GRID_EMBED_CELL_LIMIT` (mirrored at `500` in `frontend/src/assets/configurations/constants.ts`) so pathological extracts no longer transmit thousands of cells just to trigger the too-many-rows guard (`frontend/src/graphql/queries.ts`, `frontend/src/components/extracts/ExtractGridEmbed.tsx`). Full server-side pagination is still tracked in #1204.
5960
- **`resolveComponentMarker` now receives a stable React key from both call sites**: `useCamlComponentRenderer` and `CamlDirectiveRenderer` pass the marker string as the `key` argument so multiple `[component:...]` blocks in a single article reconcile correctly without React's "missing key prop" warnings (`frontend/src/hooks/useCamlComponentRenderer.tsx`, `frontend/src/components/corpuses/caml/CamlDirectiveRenderer.tsx`). Added regression tests in `frontend/src/utils/__tests__/camlComponents.test.ts`.

frontend/src/components/corpuses/CamlArticleEditor.tsx

Lines changed: 40 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -373,6 +373,11 @@ export const CamlArticleEditor: React.FC<CamlArticleEditorProps> = ({
373373
// Index of the keyboard-focused option within the extract picker dropdown.
374374
// `-1` means no option is focused (initial state when the dropdown opens).
375375
const [activeExtractIndex, setActiveExtractIndex] = useState<number>(-1);
376+
// Mirror of `activeExtractIndex` in a ref so that the keyboard handler can
377+
// read the latest value when consecutive keystrokes (e.g. ArrowDown then
378+
// Enter) arrive faster than React schedules a re-render. Without this, the
379+
// Enter handler would observe a stale closure value of `-1` and bail out.
380+
const activeExtractIndexRef = useRef<number>(-1);
376381
const textareaRef = useRef<HTMLTextAreaElement>(null);
377382
const extractPickerRef = useRef<HTMLDivElement>(null);
378383
const extractPickerTriggerRef = useRef<HTMLButtonElement>(null);
@@ -545,9 +550,11 @@ export const CamlArticleEditor: React.FC<CamlArticleEditorProps> = ({
545550
}, [showExtractPicker]);
546551

547552
// Reset the keyboard-focused option whenever the picker closes so the next
548-
// open starts in a clean state.
553+
// open starts in a clean state. The ref mirror is reset alongside state so
554+
// the next ArrowDown reads the fresh `-1` value synchronously.
549555
useEffect(() => {
550556
if (!showExtractPicker) {
557+
activeExtractIndexRef.current = -1;
551558
setActiveExtractIndex(-1);
552559
}
553560
}, [showExtractPicker]);
@@ -600,43 +607,57 @@ export const CamlArticleEditor: React.FC<CamlArticleEditorProps> = ({
600607
if (!showExtractPicker) return;
601608
const count = corpusExtracts.length;
602609

610+
// Helper: update both state (drives render) and ref (drives next
611+
// synchronous keystroke). The ref read in the Enter case below would
612+
// otherwise observe a stale `-1` if Enter arrives in the same tick as
613+
// a preceding ArrowDown.
614+
const updateActiveIndex = (next: number) => {
615+
activeExtractIndexRef.current = next;
616+
setActiveExtractIndex(next);
617+
};
618+
603619
switch (event.key) {
604620
case "Escape":
605621
event.preventDefault();
606622
setShowExtractPicker(false);
607623
extractPickerTriggerRef.current?.focus();
608624
break;
609-
case "ArrowDown":
625+
case "ArrowDown": {
610626
if (count === 0) return;
611627
event.preventDefault();
612-
setActiveExtractIndex((prev) => (prev + 1 >= count ? 0 : prev + 1));
628+
const prev = activeExtractIndexRef.current;
629+
updateActiveIndex(prev + 1 >= count ? 0 : prev + 1);
613630
break;
614-
case "ArrowUp":
631+
}
632+
case "ArrowUp": {
615633
if (count === 0) return;
616634
event.preventDefault();
617635
// `prev <= 0` covers both `0` (first item → wrap to last) and `-1`
618636
// (no item focused → jump to last). This is intentional per WAI-ARIA
619637
// Authoring Practices for listbox keyboard interaction.
620-
setActiveExtractIndex((prev) => (prev <= 0 ? count - 1 : prev - 1));
638+
const prev = activeExtractIndexRef.current;
639+
updateActiveIndex(prev <= 0 ? count - 1 : prev - 1);
621640
break;
641+
}
622642
case "Home":
623643
if (count === 0) return;
624644
event.preventDefault();
625-
setActiveExtractIndex(0);
645+
updateActiveIndex(0);
626646
break;
627647
case "End":
628648
if (count === 0) return;
629649
event.preventDefault();
630-
setActiveExtractIndex(count - 1);
650+
updateActiveIndex(count - 1);
631651
break;
632652
case "Enter": {
633653
if (count === 0) return;
634654
// Only act when a menu option is focused — otherwise let the
635655
// default button behaviour on the trigger toggle the picker.
636-
if (activeExtractIndex < 0 || activeExtractIndex >= count) return;
656+
const current = activeExtractIndexRef.current;
657+
if (current < 0 || current >= count) return;
637658
event.preventDefault();
638659
event.stopPropagation();
639-
const selected = corpusExtracts[activeExtractIndex];
660+
const selected = corpusExtracts[current];
640661
if (selected) {
641662
// handleInsertComponent already calls setShowExtractPicker(false)
642663
// internally, so the picker is closed as part of the insertion.
@@ -649,16 +670,14 @@ export const CamlArticleEditor: React.FC<CamlArticleEditorProps> = ({
649670
break;
650671
}
651672
},
652-
[
653-
showExtractPicker,
654-
corpusExtracts,
655-
activeExtractIndex,
656-
handleInsertComponent,
657-
]
673+
[showExtractPicker, corpusExtracts, handleInsertComponent]
658674
);
659675

660676
// Markdown renderer with generic component marker interception
661-
const renderMarkdownPreview = useCamlComponentRenderer(CAML_COMPONENTS);
677+
const {
678+
renderMarkdown: renderMarkdownPreview,
679+
customBlocks: previewCustomBlocks,
680+
} = useCamlComponentRenderer(CAML_COMPONENTS);
662681

663682
const handleClose = () => {
664683
if (hasChanges) {
@@ -744,7 +763,10 @@ export const CamlArticleEditor: React.FC<CamlArticleEditorProps> = ({
744763
key={ext.id}
745764
$active={index === activeExtractIndex}
746765
aria-selected={false}
747-
onMouseEnter={() => setActiveExtractIndex(index)}
766+
onMouseEnter={() => {
767+
activeExtractIndexRef.current = index;
768+
setActiveExtractIndex(index);
769+
}}
748770
onClick={() =>
749771
handleInsertComponent("extract-grid", {
750772
extractId: ext.id,
@@ -778,6 +800,7 @@ export const CamlArticleEditor: React.FC<CamlArticleEditorProps> = ({
778800
<CamlArticle
779801
document={parsedDocument}
780802
renderMarkdown={renderMarkdownPreview}
803+
customBlocks={previewCustomBlocks}
781804
/>
782805
</CamlThemeProvider>
783806
)}

frontend/src/components/corpuses/CorpusChat.tsx

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -470,10 +470,16 @@ export const CorpusChat: React.FC<CorpusChatProps> = ({
470470
// ASYNC path, it must be appended to `chat` directly or it will never render.
471471
// No setIsProcessing(false) is needed: ASYNC_START is the only setter for
472472
// isProcessing(true), and SYNC_CONTENT arrives without a preceding ASYNC_START.
473+
//
474+
// Capture the message id ONCE so the visible chat entry and the
475+
// ChatSourceAtom record agree even if the server omits message_id
476+
// (otherwise each `crypto.randomUUID()` call produces a different
477+
// value, leaving citations unable to find their parent message).
478+
const messageId = data?.message_id ?? crypto.randomUUID();
473479
setChat((prev) => [
474480
...prev,
475481
{
476-
messageId: data?.message_id ?? crypto.randomUUID(),
482+
messageId,
477483
user: "Assistant",
478484
content,
479485
timestamp: new Date().toLocaleString(),
@@ -493,7 +499,7 @@ export const CorpusChat: React.FC<CorpusChatProps> = ({
493499
handleCompleteMessage(
494500
content,
495501
sourcesToPass,
496-
data?.message_id,
502+
messageId,
497503
undefined,
498504
timelineToPass
499505
);

frontend/src/components/corpuses/caml/CamlDirectiveRenderer.tsx

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import {
2626
DirectiveHandlerContext,
2727
} from "./directiveRegistry";
2828
import {
29+
OC_COMPONENT_FENCE,
2930
resolveComponentMarker,
3031
type CamlComponentRegistry,
3132
} from "../../../utils/camlComponents";
@@ -187,13 +188,30 @@ export const CamlDirectiveRenderer: React.FC<CamlDirectiveRendererProps> = ({
187188
componentRegistry,
188189
]);
189190

191+
// Custom block dispatch for the project's `::: oc-component` fence used by
192+
// the editor to embed components (e.g. extract grids). The fence wraps a
193+
// plain `[component:TYPE ...]` marker, so we delegate to the same
194+
// renderMarkdown path that handles inline markers.
195+
const customBlocks = useMemo(
196+
() => ({
197+
[OC_COMPONENT_FENCE]: (block: unknown) => {
198+
const body = (
199+
(block as { body?: string } | undefined)?.body ?? ""
200+
).trim();
201+
return renderMarkdown(body);
202+
},
203+
}),
204+
[renderMarkdown]
205+
);
206+
190207
return (
191208
<CamlThemeProvider>
192209
<CamlArticle
193210
document={cleanedDocument}
194211
stats={stats}
195212
renderMarkdown={renderMarkdown}
196213
resolveImageSrc={resolveImageSrc}
214+
customBlocks={customBlocks}
197215
/>
198216
</CamlThemeProvider>
199217
);

frontend/src/hooks/useCamlComponentRenderer.tsx

Lines changed: 48 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3,31 +3,57 @@
33
* for CamlArticle which intercepts `[component:TYPE ...]` markers and
44
* renders registered React components in their place.
55
*
6+
* Also returns a `customBlocks` map that handles the project-specific
7+
* `::: oc-component` fence used by the editor's "Insert Extract Grid"
8+
* action. The fence is needed because the library's parser does not handle
9+
* a `::: prose` fence (block.content is undefined → ProseBlock crashes).
10+
*
611
* Usage:
712
* const registry = { "extract-grid": ExtractGridEmbed };
8-
* const renderMarkdown = useCamlComponentRenderer(registry);
9-
* <CamlArticle document={doc} renderMarkdown={renderMarkdown} />
13+
* const { renderMarkdown, customBlocks } = useCamlComponentRenderer(registry);
14+
* <CamlArticle
15+
* document={doc}
16+
* renderMarkdown={renderMarkdown}
17+
* customBlocks={customBlocks}
18+
* />
1019
*/
11-
import React, { useCallback } from "react";
20+
import React, { useCallback, useMemo } from "react";
1221
import { MarkdownMessageRenderer } from "../components/threads/MarkdownMessageRenderer";
1322
import { ErrorBoundary } from "../components/widgets/ErrorBoundary";
1423
import { ComponentEmbedErrorFallback } from "../components/widgets/ComponentEmbedErrorFallback";
15-
import { resolveComponentMarker } from "../utils/camlComponents";
24+
import {
25+
OC_COMPONENT_FENCE,
26+
resolveComponentMarker,
27+
} from "../utils/camlComponents";
1628
export type { CamlComponentRegistry } from "../utils/camlComponents";
1729

30+
interface OcComponentBlock {
31+
type: string;
32+
body?: string;
33+
attrs?: Record<string, string>;
34+
}
35+
36+
export interface CamlComponentRendererBindings {
37+
/** Pass to `<CamlArticle renderMarkdown={...}>`. */
38+
renderMarkdown: (md: string) => React.ReactNode;
39+
/** Pass to `<CamlArticle customBlocks={...}>`. */
40+
customBlocks: Record<string, (block: unknown) => React.ReactNode>;
41+
}
42+
1843
/**
19-
* Returns a stable `renderMarkdown` callback that checks each prose block
20-
* for a `[component:TYPE ...]` marker. If it matches a registered component,
21-
* renders that component with the parsed props; otherwise falls back to the
22-
* standard markdown renderer.
44+
* Returns the `renderMarkdown` and `customBlocks` callbacks needed by
45+
* `CamlArticle` to resolve `[component:TYPE ...]` markers. `renderMarkdown`
46+
* intercepts standalone markers in prose blocks (legacy / inline use); the
47+
* `oc-component` custom block intercepts markers wrapped in the project's
48+
* dedicated fence.
2349
*/
2450
export function useCamlComponentRenderer(
2551
registry: Record<
2652
string,
2753
React.ComponentType<Record<string, string | undefined>>
2854
>
29-
): (md: string) => React.ReactNode {
30-
return useCallback(
55+
): CamlComponentRendererBindings {
56+
const renderMarkdown = useCallback(
3157
(md: string) => {
3258
// Use the marker string as the React key so that multiple
3359
// `[component:...]` blocks in a single article each get a stable,
@@ -48,4 +74,16 @@ export function useCamlComponentRenderer(
4874
},
4975
[registry]
5076
);
77+
78+
const customBlocks = useMemo(
79+
() => ({
80+
[OC_COMPONENT_FENCE]: (block: unknown) => {
81+
const body = ((block as OcComponentBlock)?.body ?? "").trim();
82+
return renderMarkdown(body);
83+
},
84+
}),
85+
[renderMarkdown]
86+
);
87+
88+
return { renderMarkdown, customBlocks };
5189
}

frontend/src/utils/__tests__/camlComponents.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -212,12 +212,12 @@ describe("buildComponentMarker()", () => {
212212
});
213213

214214
describe("buildComponentProseFence()", () => {
215-
it("wraps marker in a CAML prose block fence", () => {
215+
it("wraps marker in the project's oc-component fence", () => {
216216
const result = buildComponentProseFence("extract-grid", {
217217
extractId: "abc",
218218
});
219219
expect(result).toBe(
220-
"\n::: prose\n[component:extract-grid extractId=abc]\n:::\n"
220+
"\n::: oc-component\n[component:extract-grid extractId=abc]\n:::\n"
221221
);
222222
});
223223
});

frontend/src/utils/camlComponents.ts

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -126,14 +126,32 @@ export function buildComponentMarker(
126126
}
127127

128128
/**
129-
* Wrap a marker in a CAML prose block fence, ready for insertion into
130-
* the editor source.
129+
* Name of the custom CAML fence used to embed an OpenContracts component.
130+
*
131+
* The library's parser does not have a dedicated case for `::: prose`, so a
132+
* `::: prose` fence ends up as `{type: "prose", body, ...}` *without* a
133+
* `content` field — and `ProseBlock` then crashes inside `splitPullquotes`.
134+
* Using a project-specific block type sidesteps the missing case: unknown
135+
* types fall through to the renderer's `customBlocks` lookup, where we own
136+
* the rendering and can simply pass the marker text to our resolver.
137+
*/
138+
export const OC_COMPONENT_FENCE = "oc-component";
139+
140+
/**
141+
* Wrap a marker in a CAML fence ready for insertion into the editor source.
142+
*
143+
* The marker text is preserved verbatim inside the fence body so existing
144+
* marker parsers (`parseComponentMarker`, `resolveComponentMarker`) keep
145+
* working against the same `[component:TYPE ...]` shape.
131146
*/
132147
export function buildComponentProseFence(
133148
type: string,
134149
props: CamlComponentProps
135150
): string {
136-
return `\n::: prose\n${buildComponentMarker(type, props)}\n:::\n`;
151+
return `\n::: ${OC_COMPONENT_FENCE}\n${buildComponentMarker(
152+
type,
153+
props
154+
)}\n:::\n`;
137155
}
138156

139157
// ---------------------------------------------------------------------------

0 commit comments

Comments
 (0)