Skip to content
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

feat: Enhance Schema Error Visualization with New Icons #855

Merged
merged 21 commits into from
Jul 10, 2024

Conversation

Kevin101Zhang
Copy link
Contributor

@Kevin101Zhang Kevin101Zhang commented Jul 8, 2024

Optimized Editor Lifecycle methods and removed the 'janky' Alert that would insert itself between the developer tools and Editor component in favor of a small icon that is in the fileswitcher.

@Kevin101Zhang Kevin101Zhang linked an issue Jul 8, 2024 that may be closed by this pull request
@Kevin101Zhang Kevin101Zhang changed the title feat: fix: remove multiple page rerenders to QueryAPI, feat: optimized Editor life cycle methods, feat: added new icons to showcase Schema Error Jul 8, 2024
@Kevin101Zhang Kevin101Zhang linked an issue Jul 8, 2024 that may be closed by this pull request
@Kevin101Zhang Kevin101Zhang marked this pull request as ready for review July 8, 2024 20:22
@Kevin101Zhang Kevin101Zhang requested a review from a team as a code owner July 8, 2024 20:22
@Kevin101Zhang
Copy link
Contributor Author

Schema now no longer throws an Alert component in between the Editor and the DevelopMenu cause a 'janky experience'. Instead schema not showcases errors based on the file tab itself through small icons. Similarly the debugMode no longer has a Alert component but is now directs the user to open their console through a onHover tooltip. Indexer.js has a similar funtionality but is locked through a feature flag as it is dependent on Monaco having types of node, near-lake first before we can properly prompt the user there is an error in the Indexer.

@morgsmccauley
Copy link
Collaborator

There's a lot going on in this PR, could you please split it in to smaller ones, one for each of the fix/feat added in the title?

Smaller PRs are preferable because they:

  1. Make the review process easier, smaller diff to review.
  2. Reduce the likelihood of introducing bugs, as changes are more focused and easier to understand.
  3. Improve the ability to identify the source of issues, as smaller, isolated changes make debugging more straightforward.

@Kevin101Zhang Kevin101Zhang changed the title fix: remove multiple page rerenders to QueryAPI, feat: optimized Editor life cycle methods, feat: added new icons to showcase Schema Error Feat: optimized Editor life cycle methods and added new icons to showcase Schema Error Jul 9, 2024
@Kevin101Zhang Kevin101Zhang changed the title Feat: optimized Editor life cycle methods and added new icons to showcase Schema Error feat: optimized Editor life cycle methods and added new icons to showcase Schema Error Jul 9, 2024
Copy link
Contributor Author

@Kevin101Zhang Kevin101Zhang Jul 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small change in validators.ts, no functionality change.
Purpose is to make the error handling more consistent with others functions in the same file.

Copy link
Contributor Author

@Kevin101Zhang Kevin101Zhang Jul 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small change in index.js, no functionality change.
Purpose is to make component more readable

@Kevin101Zhang Kevin101Zhang changed the title feat: optimized Editor life cycle methods and added new icons to showcase Schema Error feat: Refactor Editor Lifecycle Methods and Enhance Schema Error Visualization with New Icons Jul 9, 2024
@Kevin101Zhang Kevin101Zhang changed the title feat: Refactor Editor Lifecycle Methods and Enhance Schema Error Visualization with New Icons feat: Enhance Schema Error Visualization with New Icons Jul 9, 2024
@Kevin101Zhang
Copy link
Contributor Author

Kevin101Zhang commented Jul 9, 2024

@morgsmccauley I removed the "fix: multiple rerenders" to a different PR.
The Editor optimizations heavily revolve around the the introduction of the new feature of icons in fileSwitcher to showcase error messages. All the other files here are tightly coupled are children component of Editor (fileSwitcher, developertools). There are the larger number of file changes due to each component having a view and viewContainer.

The exception are the 2 small more readability refactors in the files commented above that are trivial.

Copy link
Collaborator

@morgsmccauley morgsmccauley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a few minor comments

Indexer.js
<span>Indexer.js</span>
<div className="inline-flex items-center">
{/* {indexerError ? (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we just remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to add this the second I get monaco Types inserted. I attempted it with a little time but I was having trouble. Will retackle it in the next ticket so I would like to keep it

console.error(error.message);
return { data: code, error };
} else {
throw error;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand, there is a functional change here. Previously, if the error was not an Error it would be re-thrown. Are you sure we don't need that functionality?

Copy link
Contributor Author

@Kevin101Zhang Kevin101Zhang Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! Im fairly certain the error will be handled properly.

const debouncedValidateCode = useDebouncedCallback((_code: string) => {
  const { error } = validateJSCode(_code);
  console.log(error);
  indexerErrorHandler(error);
}, 500);

export function validateJSCode(code: string): { data: string | null; error: Error | null } {
  if (!code) return { data: null, error: null };

  try {
    const formattedCode = formatIndexingCode(code);
    return { data: formattedCode, error: null };
  } catch (error: any) {
    return {
      data: code,
      error: error.message,
    };
  }
}

export const formatIndexingCode = (code) => {
  try {
    return prettier.format(code, {
      parser: 'babel',
      plugins: [parserBabel],
    });
  } catch (error) {
    throw new Error(`Failed to format code: ${error.message}`);
  }
};

validateJSCode function will attempt to format the code. If an error occurs during formatting, it catches the error and returns it. formatIndexingCode function tries to format the code using Prettier. If Prettier fails, it throws an error with a clear message. This error will be caught by the validateJSCode function.

frontend/src/pages/query-api-editor/index.js Outdated Show resolved Hide resolved
@Kevin101Zhang Kevin101Zhang merged commit 1318cb6 into main Jul 10, 2024
4 checks passed
@Kevin101Zhang Kevin101Zhang deleted the 836-multiple-rerenders-on-editor-lifecycle branch July 10, 2024 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Monaco Editor Alert Jumps to top
2 participants