Implement JSON formatting and syntax highlighting by AmerMesanovic · Pull Request #443 · astarte-platform/astarte-dashboard

AmerMesanovic · 2024-07-05T11:25:05Z

Enhance UX in Dashboard with JSON text editor using Monaco Editor (@monaco-editor/react) for improved formatting, syntax highlighting, and validation error reporting.

src/components/InterfaceEditor.tsx

src/components/JsonEditor.tsx

package-lock.json

src/components/TriggerEditor/index.tsx

src/schemas/jsonSchemas.tsx

cypress/e2e/interface_builder.cy.js

cypress/e2e/trigger_builder.cy.js

src/components/JsonEditor.tsx

cypress/e2e/trigger_builder.cy.js

eddbbt · 2024-12-02T15:19:07Z

cypress/e2e/interface_builder.cy.js

-      databaseTTL,
-    } = parseMappingOptions(_.get(iface.mappings, '0'));
+    const { reliability, explicitTimestamp, retention, expiry, databaseRetention, databaseTTL } =
+      parseMappingOptions(_.get(iface.mappings, '0'));


if there are no changes, please avoid to change code format as this will introduce undesired noise to the pr.
Same goes for unnecessary ; at the end of a line

there are multiple occurrences in this PR that only changes how the code is styled but add nothing, it's better to move a restyling in another PR

eddbbt · 2024-12-02T15:22:57Z

cypress/e2e/interface_builder.cy.js

    .within(() => {
      cy.contains(mapping.endpoint);
-      cy.get('button').contains('Edit...').click();
+      cy.get('button').contains('Edit...').should('have.length', 1).click({ force: true });


we could possible have more that one edit button? There is a necessity to check that?

eddbbt · 2024-12-02T15:45:05Z

cypress/e2e/interface_builder.cy.js

          cy.fixture(interfaceFixture).then(({ data: iface }) => {
            setupInterfaceEditorFromUI(iface);
            checkInterfaceEditorUIValues(iface);
+


as above, avoid newlines if not necessary

eddbbt · 2024-12-02T16:11:15Z

cypress/e2e/trigger_builder.cy.js

+        if (win.monaco && win.monaco.editor) {
+          const editor = win.monaco.editor.getModels()[0];
+          const editorContent = editor.getValue();
+          const editorJson = JSON.parse(editorContent);
+
+          expect(editorJson.simple_triggers[0].interface_name).to.equal(iface);
+        } else {
+          throw new Error('Monaco Editor instance is not available');
+        }
+      });
+      cy.window().then((win) => {
+        if (win.monaco && win.monaco.editor) {
+          const editor = win.monaco.editor.getModels()[0];
+          const editorContent = editor.getValue();
+          const editorJson = JSON.parse(editorContent);
+
+          expect(editorJson.simple_triggers[0].interface_major).to.equal(simpleTrigger.interface_major);
+        } else {
+          throw new Error('Monaco Editor instance is not available');
+        }
+      });
+      cy.window().then((win) => {
+        if (win.monaco && win.monaco.editor) {
+          const editor = win.monaco.editor.getModels()[0];
+          const editorContent = editor.getValue();
+          const editorJson = JSON.parse(editorContent);
+
+          expect(editorJson.simple_triggers[0].match_path).to.equal(simpleTrigger.match_path);
+        } else {
+          throw new Error('Monaco Editor instance is not available');
+        }


Here there is a repetition of

cy.window().then((win) => { if (win.monaco && win.monaco.editor) { const editor = win.monaco.editor.getModels()[0]; const editorContent = editor.getValue(); const editorJson = JSON.parse(editorContent);

all of that can be merged in a single if clause

eddbbt · 2024-12-02T16:12:06Z

cypress/e2e/trigger_builder.cy.js

        .scrollIntoView()
        .should('be.visible')
-        .contains(simpleTrigger.interface_major)
+        .contains(triggerOperatorToLabel[simpleTrigger.value_match_operator])


good to check for more than one property, but

.contains(simpleTrigger.interface_major)

got removed

eddbbt · 2024-12-02T16:12:10Z

cypress/e2e/trigger_builder.cy.js

        .scrollIntoView()
        .should('be.visible')
-        .contains(simpleTrigger.interface_major)
+        .contains(triggerOperatorToLabel[simpleTrigger.value_match_operator])


good to check for more than one property, but

.contains(simpleTrigger.interface_major)

got removed entirely

eddbbt · 2024-12-02T16:14:43Z

cypress/e2e/trigger_builder.cy.js

+        cy.get('#triggerSource', { timeout: 1000 }).scrollIntoView().should('be.visible');
+        cy.get('button').contains('Hide source', { timeout: 1000 }).scrollIntoView().click();
+        cy.get('#triggerSource', { timeout: 1000 }).should('not.exist');
+        cy.get('button').contains('Show source', { timeout: 1000 }).scrollIntoView().click();
+        cy.get('#triggerSource', { timeout: 1000 }).scrollIntoView().should('be.visible');


if not required, all of those timeouts would only prolong the duration of this already long tests

This apply for the entire PR, check and be sure the timeouts/cy.wait functions are really needed

Enhance UX in Dashboard with JSON text editor using Monaco Editor (@monaco-editor/react) for improved formatting, syntax highlighting, and validation error reporting. Signed-off-by: AmerMesanovic <amer.mesanovic@secomind.com>

utilizing MonacoEditor. - Updated test selectors to target JSONEditor component. - Used force option for clicks to handle overlays in the editor. - Increased timeouts where necessary to account for MonacoEditor's loading times Signed-off-by: AmerMesanovic <amer.mesanovic@secomind.com>

eddbbt

@Annopaolo can we relaunch the CI? imho it's just cypress flakiness

davidebriani · 2025-02-10T23:45:15Z

src/components/MonacoContext.tsx

+import React, { createContext, useContext, useEffect, useState } from 'react';
+import monaco from '@monaco-editor/react';
+
+interface MonacoContextType {
+  monaco: typeof monaco | null;
+}
+
+const MonacoContext = createContext<MonacoContextType | undefined>(undefined);
+
+export const MonacoProvider: React.FC<{ children: React.ReactNode }> = ({ children }) => {
+  const [monacoInstance, setMonacoInstance] = useState<typeof monaco | null>(null);
+
+  useEffect(() => {
+    if (!monacoInstance) {
+      setMonacoInstance(monaco);
+    }
+  }, [monacoInstance]);
+
+  return (
+    <MonacoContext.Provider value={{ monaco: monacoInstance }}>{children}</MonacoContext.Provider>
+  );
+};
+
+export const useMonacoContext = (): MonacoContextType => {
+  const context = useContext(MonacoContext);
+  if (!context) {
+    throw new Error('useMonacoContext must be used within a MonacoProvider');
+  }
+  return context;
+};


This file defines a MonacoContext and a useMonacoContext, but it seems they are not really used anywhere (besides wrapping the App with MonacoProvider).
What is their purpose? Can we get rid of them?
From the documentation of the editor it should be enough to import its React component and render it where needed, as you already did.

davidebriani · 2025-02-10T23:47:46Z

cypress/e2e/interface_builder.cy.js

+  cy.get('.monaco-editor')
+    .should('be.visible')
+    .then(() => {
+      cy.window().then((win) => {
+        const editor = win.monaco.editor.getModels()[0];
+        editor.setValue(JSON.stringify(iface, null, 4));
+      });
+    });


It would be ideal to stick to the basic Cypress commands, thus simulating an actual user interacting and writing (or pasting) into the editor.
The main idea when doing e2e tests is to avoid thinking about the implementation and just test the expected user interaction.
Do you think it's feasible, or did you encounter some issues?

davidebriani · 2025-02-10T23:53:48Z

cypress/e2e/interface_builder.cy.js

        interfaceFixtures.forEach((interfaceFixture) => {
          cy.fixture(interfaceFixture).then(({ data: iface }) => {
            setupInterfaceEditorFromSource(iface);
+            setupInterfaceEditorFromUI(iface);


This looks like cheating :)

This test correctly loads interface from its source should validate that when the user writes into the editor (setupInterfaceEditorFromSource(iface)) then the UI form fields are automatically updated and correspond to the provided source.

If we call setupInterfaceEditorFromUI(iface) to setup the UI form fields, of course they will correspond: it defies the purpose of the test.

If we want to maintain the same behavior, we can update the implementation instead of the test, making sure that when the user changes the editor's content then the UI fields are updated.

Pavinati requested changes Jul 5, 2024

View reviewed changes

src/components/InterfaceEditor.tsx Show resolved Hide resolved

src/components/JsonEditor.tsx Outdated Show resolved Hide resolved

AmerMesanovic force-pushed the json-editor branch 2 times, most recently from 2148e57 to e493080 Compare July 11, 2024 13:12

AmerMesanovic requested a review from Pavinati July 11, 2024 13:30

Pavinati requested changes Jul 11, 2024

View reviewed changes

package-lock.json Outdated Show resolved Hide resolved

AmerMesanovic force-pushed the json-editor branch from e493080 to b761066 Compare July 12, 2024 06:48

AmerMesanovic requested a review from Pavinati July 12, 2024 06:58

Pavinati requested changes Jul 12, 2024

View reviewed changes

src/components/TriggerEditor/index.tsx Show resolved Hide resolved

src/schemas/jsonSchemas.tsx Show resolved Hide resolved

AmerMesanovic force-pushed the json-editor branch from b761066 to 896bc9b Compare July 12, 2024 10:07

AmerMesanovic requested a review from Pavinati July 12, 2024 10:08

Pavinati approved these changes Jul 12, 2024

View reviewed changes

AmerMesanovic force-pushed the json-editor branch from 896bc9b to 1d73438 Compare August 1, 2024 08:44

AmerMesanovic requested a review from Pavinati August 1, 2024 11:08

Pavinati requested changes Aug 5, 2024

View reviewed changes

AmerMesanovic force-pushed the json-editor branch from 1d73438 to db10ac3 Compare August 7, 2024 08:07

Pavinati requested changes Aug 8, 2024

View reviewed changes

cypress/e2e/trigger_builder.cy.js Outdated Show resolved Hide resolved

cypress/e2e/trigger_builder.cy.js Outdated Show resolved Hide resolved

AmerMesanovic force-pushed the json-editor branch from db10ac3 to 339313b Compare August 8, 2024 09:29

AmerMesanovic requested a review from Pavinati August 8, 2024 09:29

AmerMesanovic force-pushed the json-editor branch 2 times, most recently from 00f7438 to a095ea6 Compare August 9, 2024 06:52

Pavinati requested changes Aug 9, 2024

View reviewed changes

cypress/e2e/trigger_builder.cy.js Outdated Show resolved Hide resolved

cypress/e2e/trigger_builder.cy.js Show resolved Hide resolved

cypress/e2e/trigger_builder.cy.js Outdated Show resolved Hide resolved

cypress/e2e/trigger_builder.cy.js Show resolved Hide resolved

AmerMesanovic force-pushed the json-editor branch from a095ea6 to 2e2ccbe Compare August 9, 2024 10:33

AmerMesanovic requested a review from Pavinati August 9, 2024 10:38

AmerMesanovic force-pushed the json-editor branch from 2e2ccbe to 44e9eb8 Compare September 2, 2024 11:33

eddbbt reviewed Dec 2, 2024

View reviewed changes

Implement JSON formatting and syntax highlighting

b103c12

Enhance UX in Dashboard with JSON text editor using Monaco Editor (@monaco-editor/react) for improved formatting, syntax highlighting, and validation error reporting. Signed-off-by: AmerMesanovic <amer.mesanovic@secomind.com>

AmerMesanovic force-pushed the json-editor branch from 44e9eb8 to 03aa1da Compare February 4, 2025 12:27

AmerMesanovic requested a review from eddbbt February 4, 2025 12:44

AmerMesanovic force-pushed the json-editor branch 2 times, most recently from 0c6eadb to 1563051 Compare February 5, 2025 12:17

AmerMesanovic force-pushed the json-editor branch 9 times, most recently from 82336bc to 2e397f2 Compare February 7, 2025 13:48

AmerMesanovic force-pushed the json-editor branch from 2e397f2 to 393030d Compare February 7, 2025 14:07

eddbbt approved these changes Feb 7, 2025

View reviewed changes

davidebriani requested changes Feb 10, 2025

View reviewed changes

Conversation

AmerMesanovic commented Jul 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddbbt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AmerMesanovic commented Jul 5, 2024 •

edited

Loading