mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-08 12:33:05 +01:00
fix(skills): keep initial file editor drafts clean (#19241)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - People inspect and edit skill files from the Skills library. > - The Markdown editor receives the body without YAML frontmatter. > - The skill page compared that body with the entire saved file. > - Opening Edit therefore showed unsaved changes before the person typed. > - This pull request uses the same saved body for initialization, comparison and Discard. > - Clean files stay clean, while real edits retain the existing save path. ## Linked Issues or Issue Description **What happened?** Open an editable SKILL.md with YAML frontmatter in the Skills library. Select Files, then Edit. The page immediately shows Unsaved changes without input. The draft contains only the body, while the dirty check compares it with the full file. **Expected behavior** Opening Edit leaves the file clean and Save disabled. Real input enables Save. Discard restores the original body. Saving an edit keeps the existing frontmatter. **Steps to reproduce** 1. Open a managed skill whose SKILL.md has YAML frontmatter and a Markdown body. 2. Select Files, then Edit. 3. Make no editor input. 4. Observe the dirty indicator and Save state. **Paperclip version or commit** Reproduced locally on mastere8163fc860. **Deployment mode** Rendered local page tests for both Skills-library interface variants. API data and the rich-editor boundary are mocked. Fixed live-browser acceptance remains unverified. Related: #13915 suppresses initial rich-editor normalization, and #9356 improves Skill Studio edit tracking. This defect occurs in the Skills-library page before any editor change event. ## What Changed - Compare the draft with the same body used by Markdown initialization. - Restore that body on Discard, without inserting frontmatter into the body editor. - Disable Save when the file is clean. - Cover both page variants through actual page hydration and the Edit button. - Preserve real edits, exact fixture frontmatter, custom metadata and file-selection behavior. ## Verification - Before the fix, both rendered page variants fail after Edit shows Unsaved changes without an editor change event. - The same regressions pass after the four-line production correction. - 137 focused tests pass across CompanySkills, MarkdownEditor and company-skill-routes. - The page regression checks genuine input, Discard and re-entry, an explicit metadata-preserving save, and switching away from an unsaved prior-file draft without another save. - UI typecheck and production build pass. Token gates and diff checks pass. - The first typecheck lacked ignored generated Storybook catalog files. The existing generator supplied them; the failed prerequisite attempt remains recorded. - Independent local Sol review passed the candidate. - Full-workspace LOCAL typecheck, tests and build were not run. - Hosted CI passes on head8336145: typecheck, build, selected general tests, all eight E2E shards and aggregate gates. Four scope-based checks are skipped: Storybook visual, Storybook deployment, Runner matrix and serialized server suites. This is affected-scope coverage. - Greptile reviewed the exact head with a 5/5 score and no actionable findings. - Paperclip native review was filtered by the configured Draft and Ready event policy. This is not a completed native review or a native 5/5 result. The review route uses independent local Sol review and hosted Greptile. Repository-required checks and reviews still govern merge. - No fixed live-browser, deployed behavior or canonical-file mutation is claimed. ## Risks The change affects the Skills-library page baseline in both existing interface variants. It keeps the existing frontmatter merge and file-write API. Rich-editor normalization, Skill Studio, settings, permissions and selection lifecycle remain unchanged. The tests isolate the page baseline with a controlled editor boundary; they do not prove a live rich-editor or server write journey. ## Model Used OpenAI gpt-6.1-sol through Codex, with reasoning, source inspection, code execution and local tests. The runtime did not expose context-window size. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge Co-authored-by: Paperclip <paperclip@paperclipai.com>
This commit is contained in:
1 parent
03abe1f1a1
commit
7fd05e1354
3 files changed
+67
-7
No files matched your search
@@ -2987,7 +2987,7 @@ export function SkillDetailPage({
|
||||
|
||||
// Track unsaved edits so we can float a save bar and warn before the page is
|
||||
// unloaded with a dirty draft (PAP-10907 J).
|
||||
const savedFileContent = file?.content ?? "";
|
||||
const savedFileContent = file?.markdown ? splitFrontmatter(file.content).body : file?.content ?? "";
|
||||
const isDirty = editMode && Boolean(file?.editable) && draft !== savedFileContent;
|
||||
useEffect(() => {
|
||||
if (!isDirty) return;
|
||||
@@ -3086,7 +3086,7 @@ export function SkillDetailPage({
|
||||
editMode ? (
|
||||
<>
|
||||
<Button variant="ghost" size="sm" onClick={() => setEditMode(false)} disabled={savePending}>Cancel</Button>
|
||||
<Button size="sm" onClick={onSave} disabled={savePending}>
|
||||
<Button size="sm" onClick={onSave} disabled={savePending || !isDirty}>
|
||||
<Save className="mr-1.5 h-3.5 w-3.5" />
|
||||
{savePending ? "Saving..." : "Save"}
|
||||
</Button>
|
||||
|
||||
@@ -23,6 +23,7 @@ import { CompanySkills as ProductionCompanySkills } from "./CompanySkills.produc
|
||||
import { queryKeys } from "../lib/queryKeys";
|
||||
|
||||
const readVersion = vi.hoisted(() => vi.fn());
|
||||
const editorRoute = vi.hoisted(() => ({ path: undefined as string | undefined, search: "" }));
|
||||
const mockCompanySkillsApi = vi.hoisted(() => ({
|
||||
version: readVersion,
|
||||
list: vi.fn(),
|
||||
@@ -34,6 +35,7 @@ const mockCompanySkillsApi = vi.hoisted(() => ({
|
||||
catalogDetail: vi.fn(),
|
||||
catalogFile: vi.fn(),
|
||||
browseProject: vi.fn(),
|
||||
updateFile: vi.fn(),
|
||||
}));
|
||||
const mockFoldersApi = vi.hoisted(() => ({ list: vi.fn() }));
|
||||
const mockAgentsApi = vi.hoisted(() => ({ list: vi.fn() }));
|
||||
@@ -63,8 +65,8 @@ vi.mock("@/lib/router", () => ({
|
||||
<a href={to} {...props}>{children}</a>
|
||||
),
|
||||
useNavigate: () => vi.fn(),
|
||||
useParams: () => ({}),
|
||||
useSearchParams: () => [new URLSearchParams(), vi.fn()],
|
||||
useParams: () => ({ "*": editorRoute.path }),
|
||||
useSearchParams: () => [new URLSearchParams(editorRoute.search), vi.fn()],
|
||||
}));
|
||||
|
||||
vi.mock("@/components/ui/button", () => ({
|
||||
@@ -134,7 +136,7 @@ vi.mock("../components/MarkdownBody", () => ({
|
||||
}));
|
||||
|
||||
vi.mock("../components/MarkdownEditor", () => ({
|
||||
MarkdownEditor: ({ value }: { value: string }) => <textarea readOnly value={value} />,
|
||||
MarkdownEditor: ({ value, onChange }: { value: string; onChange: (value: string) => void }) => <textarea aria-label="Skill markdown" value={value} onInput={event => onChange(event.currentTarget.value)} onChange={() => {}} />,
|
||||
}));
|
||||
|
||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
||||
@@ -151,6 +153,8 @@ async function act(callback: () => void | Promise<void>) {
|
||||
}
|
||||
|
||||
afterEach(() => {
|
||||
editorRoute.path = undefined;
|
||||
editorRoute.search = "";
|
||||
root?.unmount();
|
||||
root = null;
|
||||
queryClient?.clear();
|
||||
@@ -1278,6 +1282,62 @@ describe("CompanySkills page read resilience", () => {
|
||||
vi.unstubAllGlobals();
|
||||
});
|
||||
|
||||
describe.each([["current", CompanySkills], ["production", ProductionCompanySkills]] as const)("%s file editor baseline", (_name, Page) => {
|
||||
it("opens frontmatter markdown clean without an editor change, preserves edits and resets selection", async () => {
|
||||
const frontmatter = "---\nname: demo-skill\ndescription: Keep this metadata.\ncustom: stable\n---\n\n";
|
||||
const body = "# Demo Skill\n\nOriginal body.\n";
|
||||
const content = frontmatter + body;
|
||||
const detail = makeDetail(makeVersion(1, content));
|
||||
detail.fileInventory.push({ path: "references/notes.md", kind: "reference" });
|
||||
editorRoute.path = "demo-skill";
|
||||
editorRoute.search = "tab=files";
|
||||
const files: Record<string, string> = { "SKILL.md": content, "references/notes.md": "Reference body.\n" };
|
||||
const readFile = (path: string) => ({ path, content: files[path], markdown: true, editable: true, encoding: "utf-8" });
|
||||
mockCompanySkillsApi.detail.mockResolvedValue(detail);
|
||||
mockCompanySkillsApi.file.mockImplementation(async (_company, _skill, path) => readFile(path));
|
||||
mockCompanySkillsApi.versions.mockResolvedValue([]);
|
||||
mockCompanySkillsApi.updateStatus.mockResolvedValue({ supported: false });
|
||||
mockCompanySkillsApi.updateFile.mockReset().mockImplementation(async (_company, _skill, path, next) => {
|
||||
files[path] = next;
|
||||
return readFile(path);
|
||||
});
|
||||
const node = await renderPage(Page);
|
||||
await vi.waitFor(() => expect(buttonsNamed(node, "Edit")).toHaveLength(1));
|
||||
await click(buttonsNamed(node, "Edit")[0]!);
|
||||
expect(node.textContent).not.toContain("Unsaved changes");
|
||||
expect((buttonsNamed(node, "Save")[0] as HTMLButtonElement).disabled).toBe(true);
|
||||
const editor = () => node.querySelector<HTMLTextAreaElement>('[aria-label="Skill markdown"]')!;
|
||||
expect(editor().value).toBe(body);
|
||||
expect(mockCompanySkillsApi.updateFile).not.toHaveBeenCalled();
|
||||
const change = async (value: string) => act(() => {
|
||||
Object.getOwnPropertyDescriptor(HTMLTextAreaElement.prototype, "value")!.set!.call(editor(), value);
|
||||
editor().dispatchEvent(new Event("input", { bubbles: true }));
|
||||
});
|
||||
await change(body + "A real edit.\n");
|
||||
expect(node.textContent).toContain("Unsaved changes");
|
||||
expect((buttonsNamed(node, "Save")[0] as HTMLButtonElement).disabled).toBe(false);
|
||||
await click(buttonsNamed(node, "Discard")[0]!);
|
||||
await click(buttonsNamed(node, "Edit")[0]!);
|
||||
expect(editor().value).toBe(body);
|
||||
expect(node.textContent).not.toContain("Unsaved changes");
|
||||
await change(body + "Saved edit.\n");
|
||||
await click(buttonsNamed(node, "Save")[0]!);
|
||||
await vi.waitFor(() => expect(mockCompanySkillsApi.updateFile).toHaveBeenCalledWith("company-1", "skill-1", "SKILL.md", frontmatter + body + "Saved edit.\n"));
|
||||
expect(detail.description).toBe("A demo skill.");
|
||||
await settle();
|
||||
await click(buttonsNamed(node, "Edit")[0]!);
|
||||
await change(body + "Unsaved prior-file edit.\n");
|
||||
editorRoute.path = "demo-skill/files/references/notes.md";
|
||||
await act(() => root?.render(<QueryClientProvider client={queryClient}><Page /></QueryClientProvider>));
|
||||
await settle();
|
||||
await click(buttonsNamed(node, "Edit")[0]!);
|
||||
expect(editor().value).toBe("Reference body.\n");
|
||||
expect(node.textContent).not.toContain("Unsaved changes");
|
||||
expect((buttonsNamed(node, "Save")[0] as HTMLButtonElement).disabled).toBe(true);
|
||||
expect(mockCompanySkillsApi.updateFile).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
});
|
||||
|
||||
describe.each([["current", CompanySkills], ["production", ProductionCompanySkills]] as const)("%s discovery errors", (_name, Page) => {
|
||||
it.each([["skills", 403], ["skills", 429], ["catalog", 403], ["catalog", 429]] as const)(
|
||||
"shows a %s %s failure while the other read pauses and keeps its own Retry usable",
|
||||
|
||||
@@ -2897,7 +2897,7 @@ export function SkillDetailPage({
|
||||
|
||||
// Track unsaved edits so we can float a save bar and warn before the page is
|
||||
// unloaded with a dirty draft (PAP-10907 J).
|
||||
const savedFileContent = file?.content ?? "";
|
||||
const savedFileContent = file?.markdown ? splitFrontmatter(file.content).body : file?.content ?? "";
|
||||
const isDirty = editMode && Boolean(file?.editable) && draft !== savedFileContent;
|
||||
useEffect(() => {
|
||||
if (!isDirty) return;
|
||||
@@ -3001,7 +3001,7 @@ export function SkillDetailPage({
|
||||
editMode ? (
|
||||
<>
|
||||
<Button variant="ghost" size="sm" onClick={() => setEditMode(false)} disabled={savePending}>Cancel</Button>
|
||||
<Button size="sm" onClick={onSave} disabled={savePending}>
|
||||
<Button size="sm" onClick={onSave} disabled={savePending || !isDirty}>
|
||||
<Save className="mr-1.5 h-3.5 w-3.5" />
|
||||
{savePending ? "Saving..." : "Save"}
|
||||
</Button>
|
||||
|
||||
Reference in new issue
Block a user