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
40 changes: 38 additions & 2 deletions packages/editor/editableDocuments.ts
Original file line number Diff line number Diff line change
Expand Up @@ -633,7 +633,14 @@ export function useEditableDocuments() {
const activeDocument = useMemo(() => getActiveDocument(), [getActiveDocument, version]);
const fileEditStatuses = useMemo(() => getFileEditStatuses(), [getFileEditStatuses, version]);

return {
// The returned object must keep a stable identity across renders that did
// not change document state (`version`): consumers put it (and callbacks
// derived from it) in effect dep arrays, and a fresh object literal every
// render re-fires those effects unconditionally — which fed the skill-prime
// re-render loop in packages/editor/App.tsx. All members are useCallback/
// useMemo-stable, so this memo only produces a new object when `version`
// (and with it activeDocument/fileEditStatuses) actually changes.
return useMemo(() => ({
version,
activeDocument,
fileEditStatuses,
Expand Down Expand Up @@ -662,5 +669,34 @@ export function useEditableDocuments() {
getDraftDocuments,
getDraftSavedFileChanges,
getSourceDocuments,
};
}), [
version,
activeDocument,
fileEditStatuses,
openDocument,
setActiveKey,
getActiveKey,
getDocument,
getActiveDocument,
getActiveDocumentLive,
getCurrentText,
beginEdit,
updateActiveText,
markSaving,
markSaved,
markError,
markFileMissing,
clearDocument,
discardDocument,
reconcileDiskSnapshot,
reloadDiskConflict,
clearSavedFileChanges,
restoreDraftDocuments,
restoreSavedFileChanges,
getUnsavedDocuments,
getSavedFileChanges,
getDraftDocuments,
getDraftSavedFileChanges,
getSourceDocuments,
]);
}
40 changes: 40 additions & 0 deletions packages/editor/editableDocumentsHook.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -266,3 +266,43 @@ describe('useEditableDocuments conflict actions', () => {
await session.unmount();
});
});

describe('useEditableDocuments return identity', () => {
// App.tsx keys effects (via getLinkedDocumentMarkdown → getDocAnnotations →
// the skill-prime effect) on this object. A fresh literal every render made
// those effects re-fire unconditionally, which fed a re-render loop.
test.skipIf(!hasDom)('stable across unrelated re-renders, new only after a document mutation', async () => {
const container = document.createElement('div');
document.body.appendChild(container);
const root = createRoot(container);
roots.push(root);
containers.push(container);

const seen: EditableDocumentsApi[] = [];
let forceRender: () => void = () => {};
function Harness() {
const [, setTick] = React.useState(0);
forceRender = () => setTick((t) => t + 1);
seen.push(useEditableDocuments());
return null;
}

await act(async () => {
root.render(<Harness />);
});
await act(async () => {
forceRender();
});
expect(seen.length).toBeGreaterThanOrEqual(2);
// A re-render with no document state change keeps the same identity.
expect(seen[seen.length - 1]).toBe(seen[0]);

await act(async () => {
seen[seen.length - 1].openDocument({ key: KEY, text: 'a\n', sourceSave: SOURCE_A });
});
// A real state change still produces a new object so consumers react.
const after = seen[seen.length - 1];
expect(after).not.toBe(seen[0]);
expect(after.version).toBeGreaterThan(seen[0].version);
});
});
114 changes: 114 additions & 0 deletions packages/editor/skillPrimeRenderLoop.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
import { afterEach, beforeEach, describe, expect, test } from 'bun:test';
import React from 'react';
import { createRoot, type Root } from 'react-dom/client';
import { act } from 'react';
import {
primeSkillContentsForExport,
resetSkillCatalogCache,
resetSkillCatalogTransport,
resetSkillContentTransport,
setSkillCatalogTransport,
setSkillContentTransport,
} from '@plannotator/ui/utils/skillCatalog';
import type { SkillCatalogEntry } from '@plannotator/ui/utils/skillReferences';
import { useEditableDocuments } from './editableDocuments';

const hasDom = typeof document !== 'undefined';

// Regression for the skill-prime render loop: once a comment referenced a
// human-only skill, App.tsx's priming effect re-fired every render (its deps
// changed identity every render via useEditableDocuments' bare object
// literal) and primeSkillContentsForExport kept answering "changed" for
// content that had already landed — so every effect run bumped the
// generation, which re-rendered, which re-fired the effect, unbounded.
// This harness mirrors that exact wiring and asserts the commit count stays
// flat while idle.

const RENDER_CAP = 40; // keeps a regression from hanging the test run

let roots: Root[] = [];
let containers: HTMLElement[] = [];

beforeEach(() => {
resetSkillCatalogCache();
setSkillCatalogTransport(async () => [
{
name: 'plannotator-review',
root: 'claude',
humanOnly: true,
dir: '/skills/plannotator-review',
},
] as SkillCatalogEntry[]);
setSkillContentTransport(async (name) => ({
name,
dir: `/skills/${name}`,
path: `/skills/${name}/SKILL.md`,
content: `# Instructions for ${name}`,
truncated: false,
humanOnly: true,
}));
});

afterEach(async () => {
for (const root of roots.splice(0)) {
await act(async () => {
root.unmount();
});
}
for (const container of containers.splice(0)) container.remove();
resetSkillCatalogCache();
resetSkillCatalogTransport();
resetSkillContentTransport();
});

describe('skill-content priming effect', () => {
test.skipIf(!hasDom)('settles instead of re-render looping once a human-only skill is referenced', async () => {
const container = document.createElement('div');
document.body.appendChild(container);
const root = createRoot(container);
roots.push(root);
containers.push(container);

const renderCount = { current: 0 };

function Harness() {
renderCount.current++;
const editableDocuments = useEditableDocuments();
// Mirrors App.tsx's getLinkedDocumentMarkdown → getDocAnnotations
// chain: a callback keyed on the hook's returned object ends up in the
// priming effect's dep array.
const getDocAnnotations = React.useCallback(
() => new Map<string, never>(),
[editableDocuments],
);
const [, setSkillContentGeneration] = React.useState(0);
React.useEffect(() => {
if (renderCount.current > RENDER_CAP) return;
let cancelled = false;
void getDocAnnotations();
primeSkillContentsForExport(['See $plannotator-review']).then((changed) => {
if (changed && !cancelled) setSkillContentGeneration((g) => g + 1);
});
return () => {
cancelled = true;
};
}, [getDocAnnotations]);
return null;
}

await act(async () => {
root.render(<Harness />);
});
// Let several idle microtask/effect cycles pass; a looping app keeps
// committing here, a fixed one is already settled.
for (let i = 0; i < 5; i++) {
await act(async () => {
await Promise.resolve();
});
}

// Expected: initial render + the single generation bump when the skill
// content first lands. Anything near RENDER_CAP is the loop.
expect(renderCount.current).toBeLessThanOrEqual(4);
});
});
42 changes: 42 additions & 0 deletions packages/ui/utils/skillCatalog.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,48 @@ describe('primeSkillContentsForExport', () => {
expect(requested).toEqual(['plannotator-review']);
});

test('edge-triggered: a re-prime with already-registered content resolves false', async () => {
stubCatalogAndContent();
expect(await primeSkillContentsForExport(['$plannotator-review'])).toBe(true);
// The content stays registered — but it is no longer news, so re-priming
// must not signal "changed" again (a level-triggered true here re-render
// looped App.tsx's generation-bump effect).
expect(await primeSkillContentsForExport(['$plannotator-review'])).toBe(false);
expect(await primeSkillContentsForExport(['$plannotator-review again'])).toBe(false);

const block = skillReferenceExportBlock('Run $plannotator-review.');
expect(block).toContain('# Instructions for plannotator-review');
});

test('edge-triggered: a later prime that lands a NEW skill reports true exactly once', async () => {
setSkillCatalogTransport(async () => [
{ name: 'alpha', root: 'claude', humanOnly: true, dir: '/skills/alpha' },
{ name: 'beta', root: 'claude', humanOnly: true, dir: '/skills/beta' },
] as SkillCatalogEntry[]);
setSkillContentTransport(async (name) => ({
name,
dir: `/skills/${name}`,
path: `/skills/${name}/SKILL.md`,
content: `# ${name}`,
truncated: false,
humanOnly: true,
}));

expect(await primeSkillContentsForExport(['$alpha'])).toBe(true);
expect(await primeSkillContentsForExport(['$alpha'])).toBe(false);
// beta's content landing is one new edge; alpha stays silent.
expect(await primeSkillContentsForExport(['$alpha and $beta'])).toBe(true);
expect(await primeSkillContentsForExport(['$alpha and $beta'])).toBe(false);
});

test('a cache reset re-arms the changed signal for the next session', async () => {
stubCatalogAndContent();
expect(await primeSkillContentsForExport(['$plannotator-review'])).toBe(true);
resetSkillCatalogCache();
expect(await primeSkillContentsForExport(['$plannotator-review'])).toBe(true);
expect(await primeSkillContentsForExport(['$plannotator-review'])).toBe(false);
});

test('no human-only references → no requests, resolves false', async () => {
const requested = stubCatalogAndContent();
expect(await primeSkillContentsForExport(['Use /write-better only.'])).toBe(false);
Expand Down
32 changes: 28 additions & 4 deletions packages/ui/utils/skillCatalog.ts
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ export function resetSkillCatalogCache(): void {
cached = null;
inflight = null;
contentRequests.clear();
reportedContentNames.clear();
setSkillCatalogForExport([]);
resetSkillContentsForExport();
}
Expand Down Expand Up @@ -189,15 +190,26 @@ function normalizeSkillContent(raw: unknown): SkillExportContent | null {
// by resetSkillCatalogCache alongside the catalog itself.
const contentRequests = new Map<string, Promise<boolean>>();

// Names whose registered content has already been reported to a caller as
// "changed". A cached request stays resolved-true forever, so the "changed"
// signal must be edge-triggered: without this set, every re-prime re-reports
// the same landing, and callers that bump a re-render generation on `true`
// (packages/editor/App.tsx) spin into an unbounded render loop. Cleared by
// resetSkillCatalogCache alongside the requests.
const reportedContentNames = new Set<string>();

/**
* Fetch and register the SKILL.md contents for every HUMAN-ONLY skill the
* given comment texts reference, so skillReferenceExportBlock can inject them.
* Lazy by design: only referenced human-only skills are fetched — model-
* invocable skills export as names the agent can invoke itself, so shipping
* every body up front would be pure bloat.
*
* Resolves true when at least one awaited request registered content (callers
* use that to re-render memoized exports). Never rejects.
* Resolves true only when content newly landed in the registry — i.e. at
* least one referenced skill's content is registered and has not been
* reported by a previous call (callers use that to re-render memoized
* exports, so the signal must be edge-triggered, never level-triggered).
* Never rejects.
*/
export async function primeSkillContentsForExport(
texts: Array<string | undefined | null>,
Expand All @@ -215,8 +227,9 @@ export async function primeSkillContentsForExport(
}
if (names.size === 0) return false;

const nameList = [...names];
const results = await Promise.all(
[...names].map((name) => {
nameList.map((name) => {
let request = contentRequests.get(name);
if (!request) {
const startedIn = generation;
Expand All @@ -238,7 +251,18 @@ export async function primeSkillContentsForExport(
return request;
}),
);
return results.some(Boolean);
// Edge-triggered: only content that landed and has never been reported
// counts as a change. Concurrent callers awaiting the same request race
// for the report; exactly one wins, which is enough to bump the caller's
// generation once.
let changed = false;
for (let i = 0; i < nameList.length; i++) {
if (results[i] && !reportedContentNames.has(nameList[i])) {
reportedContentNames.add(nameList[i]);
changed = true;
}
}
return changed;
} catch {
return false;
}
Expand Down