🧪 fix: Editor Styling, Incomplete Artifact Editing, Optimize Artifact Context (#8953)

* refactor: optimize artifacts context for improved performance * fix: layout classes for artifacts editor * chore: linting * fix: enhance artifact mutation handling in CodeEditor to prevent infinite retries * fix: handle incomplete artifacts in replaceArtifactContent and add regression tests
2025-09-22 06:00:56 +02:00 · 2025-08-08 15:49:58 -04:00 · 2025-08-08 15:49:58 -04:00 · 5d0bc95193
commit 5d0bc95193
parent e7d6100fe4
8 changed files with 311 additions and 45 deletions
--- a/api/server/services/Artifacts/update.js
+++ b/api/server/services/Artifacts/update.js
@ -60,7 +60,14 @@ const replaceArtifactContent = (originalText, artifact, original, updated) => {

  // Find boundaries between ARTIFACT_START and ARTIFACT_END
  const contentStart = artifactContent.indexOf('\n', artifactContent.indexOf(ARTIFACT_START)) + 1;
-  const contentEnd = artifactContent.lastIndexOf(ARTIFACT_END);
+  let contentEnd = artifactContent.lastIndexOf(ARTIFACT_END);
+
+  // Special case: if contentEnd is 0, it means the only ::: found is at the start of :::artifact
+  // This indicates an incomplete artifact (no closing :::)
+  // We need to check that it's exactly at position 0 (the beginning of artifactContent)
+  if (contentEnd === 0 && artifactContent.indexOf(ARTIFACT_START) === 0) {
+    contentEnd = artifactContent.length;
+  }

  if (contentStart === -1 || contentEnd === -1) {
    return null;
@ -72,12 +79,20 @@ const replaceArtifactContent = (originalText, artifact, original, updated) => {

  // Determine where to look for the original content
  let searchStart, searchEnd;
-  if (codeBlockStart !== -1 && codeBlockEnd !== -1) {
-    // If code blocks exist, search between them
+  if (codeBlockStart !== -1) {
+    // Code block starts
    searchStart = codeBlockStart + 4; // after ```\n
-    searchEnd = codeBlockEnd;
+
+    if (codeBlockEnd !== -1 && codeBlockEnd > codeBlockStart) {
+      // Code block has proper ending
+      searchEnd = codeBlockEnd;
+    } else {
+      // No closing backticks found or they're before the opening (shouldn't happen)
+      // This might be an incomplete artifact - search to contentEnd
+      searchEnd = contentEnd;
+    }
  } else {
-    // Otherwise search in the whole artifact content
+    // No code blocks at all
    searchStart = contentStart;
    searchEnd = contentEnd;
  }
--- a/api/server/services/Artifacts/update.spec.js
+++ b/api/server/services/Artifacts/update.spec.js
@ -89,9 +89,9 @@ describe('replaceArtifactContent', () => {
  };

  test('should replace content within artifact boundaries', () => {
-    const original = 'console.log(\'hello\')';
+    const original = "console.log('hello')";
    const artifact = createTestArtifact(original);
-    const updated = 'console.log(\'updated\')';
+    const updated = "console.log('updated')";

    const result = replaceArtifactContent(artifact.text, artifact, original, updated);
    expect(result).toContain(updated);
@ -317,4 +317,182 @@ console.log(greeting);`;
    expect(result).not.toContain('\n\n```');
    expect(result).not.toContain('```\n\n');
  });
+
+  describe('incomplete artifacts', () => {
+    test('should handle incomplete artifacts (missing closing ::: and ```)', () => {
+      const original = `<!DOCTYPE html>
+<html lang="en">
+<head>
+<meta charset="utf-8" />
+<meta name="viewport" content="width=device-width, initial-scale=1" />
+<title>Pomodoro</title>
+<meta name="description" content="A single-file Pomodoro timer with logs, charts, sounds, and dark mode." />
+<style>
+  :root{`;
+
+      const prefix = `Awesome idea! I'll deliver a complete single-file HTML app called "Pomodoro" with:
+- Custom session/break durations
+
+You can save this as pomodoro.html and open it directly in your browser.
+
+`;
+
+      // This simulates the real incomplete artifact case - no closing ``` or :::
+      const incompleteArtifact = `${ARTIFACT_START}{identifier="pomodoro-single-file-app" type="text/html" title="Pomodoro — Single File App"}
+\`\`\`
+${original}`;
+
+      const fullText = prefix + incompleteArtifact;
+      const message = { text: fullText };
+      const artifacts = findAllArtifacts(message);
+
+      expect(artifacts).toHaveLength(1);
+      expect(artifacts[0].end).toBe(fullText.length);
+
+      const updated = original.replace('Pomodoro</title>', 'Pomodoro</title>UPDATED');
+      const result = replaceArtifactContent(fullText, artifacts[0], original, updated);
+
+      expect(result).not.toBeNull();
+      expect(result).toContain('UPDATED');
+      expect(result).toContain(prefix);
+      // Should not have added closing markers
+      expect(result).not.toMatch(/:::\s*$/);
+    });
+
+    test('should handle incomplete artifacts with only opening code block', () => {
+      const original = 'function hello() { console.log("world"); }';
+      const incompleteArtifact = `${ARTIFACT_START}{id="test"}\n\`\`\`\n${original}`;
+
+      const message = { text: incompleteArtifact };
+      const artifacts = findAllArtifacts(message);
+
+      expect(artifacts).toHaveLength(1);
+
+      const updated = 'function hello() { console.log("UPDATED"); }';
+      const result = replaceArtifactContent(incompleteArtifact, artifacts[0], original, updated);
+
+      expect(result).not.toBeNull();
+      expect(result).toContain('UPDATED');
+    });
+
+    test('should handle incomplete artifacts without code blocks', () => {
+      const original = 'Some plain text content';
+      const incompleteArtifact = `${ARTIFACT_START}{id="test"}\n${original}`;
+
+      const message = { text: incompleteArtifact };
+      const artifacts = findAllArtifacts(message);
+
+      expect(artifacts).toHaveLength(1);
+
+      const updated = 'Some UPDATED text content';
+      const result = replaceArtifactContent(incompleteArtifact, artifacts[0], original, updated);
+
+      expect(result).not.toBeNull();
+      expect(result).toContain('UPDATED');
+    });
+  });
+
+  describe('regression tests for edge cases', () => {
+    test('should still handle complete artifacts correctly', () => {
+      // Ensure we didn't break normal artifact handling
+      const original = 'console.log("test");';
+      const artifact = createArtifactText({ content: original });
+
+      const message = { text: artifact };
+      const artifacts = findAllArtifacts(message);
+
+      expect(artifacts).toHaveLength(1);
+
+      const updated = 'console.log("updated");';
+      const result = replaceArtifactContent(artifact, artifacts[0], original, updated);
+
+      expect(result).not.toBeNull();
+      expect(result).toContain(updated);
+      expect(result).toContain(ARTIFACT_END);
+      expect(result).toMatch(/```\nconsole\.log\("updated"\);\n```/);
+    });
+
+    test('should handle multiple complete artifacts', () => {
+      // Ensure multiple artifacts still work
+      const content1 = 'First artifact';
+      const content2 = 'Second artifact';
+      const text = `${createArtifactText({ content: content1 })}\n\n${createArtifactText({ content: content2 })}`;
+
+      const message = { text };
+      const artifacts = findAllArtifacts(message);
+
+      expect(artifacts).toHaveLength(2);
+
+      // Update first artifact
+      const result1 = replaceArtifactContent(text, artifacts[0], content1, 'First UPDATED');
+      expect(result1).not.toBeNull();
+      expect(result1).toContain('First UPDATED');
+      expect(result1).toContain(content2);
+
+      // Update second artifact
+      const result2 = replaceArtifactContent(text, artifacts[1], content2, 'Second UPDATED');
+      expect(result2).not.toBeNull();
+      expect(result2).toContain(content1);
+      expect(result2).toContain('Second UPDATED');
+    });
+
+    test('should not mistake ::: at position 0 for artifact end in complete artifacts', () => {
+      // This tests the specific fix - ensuring contentEnd=0 doesn't break complete artifacts
+      const original = 'test content';
+      // Create an artifact that will have ::: at position 0 when substring'd
+      const artifact = `${ARTIFACT_START}\n\`\`\`\n${original}\n\`\`\`\n${ARTIFACT_END}`;
+
+      const message = { text: artifact };
+      const artifacts = findAllArtifacts(message);
+
+      expect(artifacts).toHaveLength(1);
+
+      const updated = 'updated content';
+      const result = replaceArtifactContent(artifact, artifacts[0], original, updated);
+
+      expect(result).not.toBeNull();
+      expect(result).toContain(updated);
+      expect(result).toContain(ARTIFACT_END);
+    });
+
+    test('should handle empty artifacts', () => {
+      // Edge case: empty artifact
+      const artifact = `${ARTIFACT_START}\n${ARTIFACT_END}`;
+
+      const message = { text: artifact };
+      const artifacts = findAllArtifacts(message);
+
+      expect(artifacts).toHaveLength(1);
+
+      // Trying to replace non-existent content should return null
+      const result = replaceArtifactContent(artifact, artifacts[0], 'something', 'updated');
+      expect(result).toBeNull();
+    });
+
+    test('should preserve whitespace and formatting in complete artifacts', () => {
+      const original = `  function test() {
+    return {
+      value: 42
+    };
+  }`;
+      const artifact = createArtifactText({ content: original });
+
+      const message = { text: artifact };
+      const artifacts = findAllArtifacts(message);
+
+      const updated = `  function test() {
+    return {
+      value: 100
+    };
+  }`;
+      const result = replaceArtifactContent(artifact, artifacts[0], original, updated);
+
+      expect(result).not.toBeNull();
+      expect(result).toContain('value: 100');
+      // Should preserve exact formatting
+      expect(result).toMatch(
+        /```\n {2}function test\(\) \{\n {4}return \{\n {6}value: 100\n {4}\};\n {2}\}\n```/,
+      );
+    });
+  });
 });
--- a/client/src/Providers/ArtifactsContext.tsx
+++ b/client/src/Providers/ArtifactsContext.tsx
@ -0,0 +1,46 @@
+import React, { createContext, useContext, useMemo } from 'react';
+import type { TMessage } from 'librechat-data-provider';
+import { useChatContext } from './ChatContext';
+import { getLatestText } from '~/utils';
+
+interface ArtifactsContextValue {
+  isSubmitting: boolean;
+  latestMessageId: string | null;
+  latestMessageText: string;
+  conversationId: string | null;
+}
+
+const ArtifactsContext = createContext<ArtifactsContextValue | undefined>(undefined);
+
+export function ArtifactsProvider({ children }: { children: React.ReactNode }) {
+  const { isSubmitting, latestMessage, conversation } = useChatContext();
+
+  const latestMessageText = useMemo(() => {
+    return getLatestText({
+      messageId: latestMessage?.messageId ?? null,
+      text: latestMessage?.text ?? null,
+      content: latestMessage?.content ?? null,
+    } as TMessage);
+  }, [latestMessage?.messageId, latestMessage?.text, latestMessage?.content]);
+
+  /** Context value only created when relevant values change */
+  const contextValue = useMemo<ArtifactsContextValue>(
+    () => ({
+      isSubmitting,
+      latestMessageText,
+      latestMessageId: latestMessage?.messageId ?? null,
+      conversationId: conversation?.conversationId ?? null,
+    }),
+    [isSubmitting, latestMessage?.messageId, latestMessageText, conversation?.conversationId],
+  );
+
+  return <ArtifactsContext.Provider value={contextValue}>{children}</ArtifactsContext.Provider>;
+}
+
+export function useArtifactsContext() {
+  const context = useContext(ArtifactsContext);
+  if (!context) {
+    throw new Error('useArtifactsContext must be used within ArtifactsProvider');
+  }
+  return context;
+}
--- a/client/src/Providers/index.ts
+++ b/client/src/Providers/index.ts
@ -23,4 +23,5 @@ export * from './SetConvoContext';
 export * from './SearchContext';
 export * from './BadgeRowContext';
 export * from './SidePanelContext';
+export * from './ArtifactsContext';
 export { default as BadgeRowProvider } from './BadgeRowContext';
--- a/client/src/components/Artifacts/ArtifactCodeEditor.tsx
+++ b/client/src/components/Artifacts/ArtifactCodeEditor.tsx
@ -1,5 +1,5 @@
 import debounce from 'lodash/debounce';
-import React, { memo, useEffect, useMemo, useCallback } from 'react';
+import React, { memo, useEffect, useMemo, useCallback, useState } from 'react';
 import {
  useSandpack,
  SandpackCodeEditor,
@ -34,13 +34,16 @@ const CodeEditor = ({
  editorRef: React.MutableRefObject<CodeEditorRef>;
 }) => {
  const { sandpack } = useSandpack();
+  const [currentUpdate, setCurrentUpdate] = useState<string | null>(null);
  const { isMutating, setIsMutating, setCurrentCode } = useEditorContext();
  const editArtifact = useEditArtifact({
-    onMutate: () => {
+    onMutate: (vars) => {
      setIsMutating(true);
+      setCurrentUpdate(vars.updated);
    },
    onSuccess: () => {
      setIsMutating(false);
+      setCurrentUpdate(null);
    },
    onError: () => {
      setIsMutating(false);
@ -71,8 +74,14 @@ const CodeEditor = ({
    }

    const currentCode = (sandpack.files['/' + fileKey] as SandpackBundlerFile | undefined)?.code;
+    const isNotOriginal =
+      currentCode && artifact.content != null && currentCode.trim() !== artifact.content.trim();
+    const isNotRepeated =
+      currentUpdate == null
+        ? true
+        : currentCode != null && currentCode.trim() !== currentUpdate.trim();

-    if (currentCode && artifact.content != null && currentCode.trim() !== artifact.content.trim()) {
+    if (artifact.content && isNotOriginal && isNotRepeated) {
      setCurrentCode(currentCode);
      debouncedMutation({
        index: artifact.index,
@ -92,8 +101,9 @@ const CodeEditor = ({
    artifact.messageId,
    readOnly,
    isMutating,
-    sandpack.files,
+    currentUpdate,
    setIsMutating,
+    sandpack.files,
    setCurrentCode,
    debouncedMutation,
  ]);
--- a/client/src/components/Chat/Presentation.tsx
+++ b/client/src/components/Chat/Presentation.tsx
@ -4,7 +4,7 @@ import { FileSources, LocalStorageKeys } from 'librechat-data-provider';
 import type { ExtendedFile } from '~/common';
 import { useDeleteFilesMutation } from '~/data-provider';
 import DragDropWrapper from '~/components/Chat/Input/Files/DragDropWrapper';
-import { EditorProvider, SidePanelProvider } from '~/Providers';
+import { EditorProvider, SidePanelProvider, ArtifactsProvider } from '~/Providers';
 import Artifacts from '~/components/Artifacts/Artifacts';
 import { SidePanelGroup } from '~/components/SidePanel';
 import { useSetFilesToDelete } from '~/hooks';
@ -66,9 +66,11 @@ export default function Presentation({ children }: { children: React.ReactNode }
          defaultCollapsed={defaultCollapsed}
          artifacts={
            artifactsVisibility === true && Object.keys(artifacts ?? {}).length > 0 ? (
-              <EditorProvider>
-                <Artifacts />
-              </EditorProvider>
+              <ArtifactsProvider>
+                <EditorProvider>
+                  <Artifacts />
+                </EditorProvider>
+              </ArtifactsProvider>
            ) : null
          }
        >
--- a/client/src/hooks/Artifacts/useArtifacts.ts
+++ b/client/src/hooks/Artifacts/useArtifacts.ts
@ -1,14 +1,15 @@
 import { useMemo, useState, useEffect, useRef } from 'react';
 import { Constants } from 'librechat-data-provider';
 import { useRecoilState, useRecoilValue, useResetRecoilState } from 'recoil';
-import { getLatestText, logger } from '~/utils';
-import { useChatContext } from '~/Providers';
+import { logger } from '~/utils';
+import { useArtifactsContext } from '~/Providers';
 import { getKey } from '~/utils/artifacts';
 import store from '~/store';

 export default function useArtifacts() {
  const [activeTab, setActiveTab] = useState('preview');
-  const { isSubmitting, latestMessage, conversation } = useChatContext();
+  const { isSubmitting, latestMessageId, latestMessageText, conversationId } =
+    useArtifactsContext();

  const artifacts = useRecoilValue(store.artifactsState);
  const resetArtifacts = useResetRecoilState(store.artifactsState);
@ -31,26 +32,23 @@ export default function useArtifacts() {
    const resetState = () => {
      resetArtifacts();
      resetCurrentArtifactId();
-      prevConversationIdRef.current = conversation?.conversationId ?? null;
+      prevConversationIdRef.current = conversationId;
      lastRunMessageIdRef.current = null;
      lastContentRef.current = null;
      hasEnclosedArtifactRef.current = false;
    };
-    if (
-      conversation?.conversationId !== prevConversationIdRef.current &&
-      prevConversationIdRef.current != null
-    ) {
+    if (conversationId !== prevConversationIdRef.current && prevConversationIdRef.current != null) {
      resetState();
-    } else if (conversation?.conversationId === Constants.NEW_CONVO) {
+    } else if (conversationId === Constants.NEW_CONVO) {
      resetState();
    }
-    prevConversationIdRef.current = conversation?.conversationId ?? null;
+    prevConversationIdRef.current = conversationId;
    /** Resets artifacts when unmounting */
    return () => {
      logger.log('artifacts_visibility', 'Unmounting artifacts');
      resetState();
    };
-  }, [conversation?.conversationId, resetArtifacts, resetCurrentArtifactId]);
+  }, [conversationId, resetArtifacts, resetCurrentArtifactId]);

  useEffect(() => {
    if (orderedArtifactIds.length > 0) {
@ -66,7 +64,7 @@ export default function useArtifacts() {
    if (orderedArtifactIds.length === 0) {
      return;
    }
-    if (latestMessage == null) {
+    if (latestMessageId == null) {
      return;
    }
    const latestArtifactId = orderedArtifactIds[orderedArtifactIds.length - 1];
@ -78,7 +76,6 @@ export default function useArtifacts() {
    setCurrentArtifactId(latestArtifactId);
    lastContentRef.current = latestArtifact?.content ?? null;

-    const latestMessageText = getLatestText(latestMessage);
    const hasEnclosedArtifact =
      /:::artifact(?:\{[^}]*\})?(?:\s|\n)*(?:```[\s\S]*?```(?:\s|\n)*)?:::/m.test(
        latestMessageText.trim(),
@ -95,15 +92,22 @@ export default function useArtifacts() {
        hasAutoSwitchedToCodeRef.current = true;
      }
    }
-  }, [setCurrentArtifactId, isSubmitting, orderedArtifactIds, artifacts, latestMessage]);
+  }, [
+    artifacts,
+    isSubmitting,
+    latestMessageId,
+    latestMessageText,
+    orderedArtifactIds,
+    setCurrentArtifactId,
+  ]);

  useEffect(() => {
-    if (latestMessage?.messageId !== lastRunMessageIdRef.current) {
-      lastRunMessageIdRef.current = latestMessage?.messageId ?? null;
+    if (latestMessageId !== lastRunMessageIdRef.current) {
+      lastRunMessageIdRef.current = latestMessageId;
      hasEnclosedArtifactRef.current = false;
      hasAutoSwitchedToCodeRef.current = false;
    }
-  }, [latestMessage]);
+  }, [latestMessageId]);

  const currentArtifact = currentArtifactId != null ? artifacts?.[currentArtifactId] : null;

--- a/client/src/mobile.css
+++ b/client/src/mobile.css
@ -313,20 +313,30 @@
  background-color: transparent;  /* Color of the tracking area */
 }

-.sp-preview-container {
-  @apply flex h-full w-full grow flex-col justify-center;
-}
-
-.sp-preview {
-  @apply flex h-full w-full grow flex-col justify-center;
-}
-
-.sp-preview-iframe {
-  @apply grow;
-}
-
+/* Base wrapper for both preview and editor */
 .sp-wrapper {
-  @apply flex h-full w-full grow flex-col justify-center;
+  @apply flex h-full w-full grow flex-col;
+}
+
+/* Stack containers (sp-preview and sp-editor) */
+.sp-preview,
+.sp-editor {
+  @apply flex h-full w-full grow flex-col;
+}
+
+/* Inner containers */
+.sp-preview-container,
+.sp-code-editor {
+  @apply flex h-full w-full grow flex-col;
+}
+
+/* Content elements */
+.sp-preview-iframe {
+  @apply h-full w-full grow;
+}
+
+.sp-cm {
+  @apply h-full w-full grow;
 }

@keyframes shake {