-
Notifications
You must be signed in to change notification settings - Fork 100
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor: Consolidate Editors + FileArtifact updates #4901
base: main
Are you sure you want to change the base?
Conversation
/review |
Code Review Agent Run Status
Code Review Overview
>>See detailed code suggestions<< See other commands you can run High-level FeedbackFocus 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. |
@@ -148,6 +120,7 @@ | |||
dimensions: Vector; | |||
}>, | |||
) { | |||
const parsedDocument = parseDocument($localContent ?? $remoteContent ?? ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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);
on:focus={() => { | ||
eventBus.emit("highlightSelection", [reference.reference]); | ||
}} | ||
on:mouseover={() => { | ||
eventBus.emit("highlightSelection", [reference.reference]); | ||
}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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])}
<Editor | ||
bind:autoSave | ||
bind:editor | ||
onSave={(content) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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);
}
}}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This cleanup will be a huge improvement!
UXQA –
- 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?
- The dashboard workspace header has no unsaved file indicator, like the other workspaces do
- 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).
- I ran into this type error:
Point 3 is true on |
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.
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.updateCodeEditor
helper function used during testingCloses #4688