Refactor: Consolidate Editors + FileArtifact updates

[briangregoryholmes]

This PR consolidates editors across the application to use the single generic Editor component.

Additionally, it moves the file mutation functions and local/remote content caches to the FileArtifact class.

Because of this update, it is no longer necessary to warn a user that unsaved changes will be lost when navigating. While product/design input is necessary to make this fully featured, this PR adjusts the FileExplorer to display unsaved files with 50% opacity and adds a save all key binding.

• Removes the queryHighlight writable and instead uses the global event bus to achieve the same functionality
• Resolves some quirks around asynchronous updates the Editor by using queueMicrotask. May address this in a future PR by trying to integrate our linting with CodeMirror in a more canonical way. This is skipped when running unit tests for the component.
• Adds a confirmation popup when a user has unsaved local changes but then receives an update to the remote content
• Adds a 500ms timeout the updateCodeEditor helper function used during testing

Closes #4688

[ericpgreen2]

/review

[rill-dev]

Code Review Agent Run Status

• AI Based Review: Successful

Code Review Overview

• Summary: This PR introduces significant enhancements across editor functionalities, file management, and event handling. Key changes include improved asynchronous operations handling, streamlined state management, and the introduction of new editor components for better configuration and state control.
• Code change type: Refactoring, Feature Addition, Performance Improvement
• Unit tests added: False
• Estimated effort to review (1-5, lower is better): 2

>>See detailed code suggestions<<
The Bito AI Code Review Agent successfully reviewed 37 files and discovered 6 issues. Please review these issues along with suggested fixes in the Changed Files.

See other commands you can run

High-level Feedback

Focus on ensuring robust error handling and asynchronous operations to prevent race conditions. Consider further modularizing components to enhance maintainability and testability. Review new functionalities for potential security implications and ensure comprehensive unit testing to cover new changes.

[rill-dev]

Issue: The use of nullish coalescing with empty string fallback in 'parseDocument' might lead to unexpected behavior if both '$localContent' and '$remoteContent' are null or undefined. This could result in parsing an empty string which might not be the intended behavior and could lead to errors downstream if the parsed document structure is expected to have specific properties or structure.
Fix: Ensure that 'parseDocument' is called with valid data. Consider handling cases where both '$localContent' and '$remoteContent' are null or undefined more explicitly, possibly by not calling 'parseDocument' at all, or by initializing '$localContent' and '$remoteContent' to a valid default state elsewhere in your application.
Code Suggestion:

- const parsedDocument = parseDocument($localContent ?? $remoteContent ?? "");
+ if (!$localContent && !$remoteContent) return; // Handle or log error appropriately
+ const parsedDocument = parseDocument($localContent ?? $remoteContent);

[rill-dev]

Issue: The new event handlers for focus and mouseover events emit a "highlightSelection" event without checking if the reference is valid. This could potentially lead to emitting events with undefined or null values, which might cause runtime errors or unexpected behavior in other parts of the application that listen to this event.
Fix: Add null checks before emitting the event to ensure that the reference is valid. This will prevent potential errors in other parts of the application.
Code Suggestion:

@@ -77,5 +77,5 @@
-                on:focus={() => {
-                  eventBus.emit("highlightSelection", [reference.reference]);
-                }}
-                on:mouseover={() => {
-                  eventBus.emit("highlightSelection", [reference.reference]);
+                on:focus={() => reference && eventBus.emit("highlightSelection", [reference.reference])}
+                on:mouseover={() => reference && eventBus.emit("highlightSelection", [reference.reference])}

[rill-dev]

Issue: The onSave callback function does not handle asynchronous operations properly, which can lead to race conditions or unhandled promise rejections if the operations inside it fail.
Fix: Modify the onSave callback to properly handle asynchronous operations using async/await to ensure that errors are caught and handled properly.
Code Suggestion:

@@ -49,7 +49,11 @@
onSave={(content) => {
-  metricsExplorerStore.remove(metricViewName);
-  createPersistentDashboardStore(metricViewName).reset();
+  (async () => {
+    try {
+      await metricsExplorerStore.remove(metricViewName);
+      await createPersistentDashboardStore(metricViewName).reset();
+    } catch (error) {
+      console.error('Failed to save metrics:', error);
+    }
+  })();
  if (!content?.length) {
    setLineStatuses([], editor);
  }
}}

[ericpgreen2]

This cleanup will be a huge improvement!

UXQA –

1. Because we also gray-out dotfiles, it’s hard to tell the semantic difference between a dotfile and an unsaved file. Maybe we could use a gray dot in the sidebar, like we do in the Workspace Header?
   
   
2. The dashboard workspace header has no unsaved file indicator, like the other workspaces do
3. Here’s a dashboard YAML that’s errored because it doesn’t have any dimensions. The editor is not showing a message with the error (and the file name in the sidebar is not red, and the preview button in the top-right is not disabled).
4. I ran into this type error:
   

[briangregoryholmes]

Point 3 is true on main as well. Looks like this isn't considered a parse error. Not really sure if it should be. @begelundmuller