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/editor error logging #480

Merged
merged 5 commits into from Jan 3, 2024
Merged

Feat/editor error logging #480

merged 5 commits into from Jan 3, 2024

Conversation

jl-santana
Copy link
Contributor

Changes Made:

  • Implemented a function to validate types and format from SQL schema, enhancing the schema editor's error logging capabilities.
  • Reduced error states and removed unnecessary Alerts for improved code clarity.
  • Added debounce functionality to schema validation for enhanced performance.
  • Created a dedicated tests folder to improve project folder structure.
Screen.Recording.2024-01-03.at.11.44.48.AM.mov

@jl-santana jl-santana requested a review from a team as a code owner January 3, 2024 16:48
Copy link
Contributor

@roshaans roshaans left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this PR @jl-santana

Nice addition on the debounce logic!

We recently had one state variable to keep track of our Editor error states and that caused a few issues:

  1. If there are issues in multiple files (indexerFunction.js, schema, or type generation from schema), the user would only see one of these errors. So we made them explicit in order to show the full exhaustive list of errors. You can check out his PR here for more details or this issue

I think we should leave these as exhaustive even if they seem redundant as without it, its really difficult to track down which of the following is causing the issue (code, schema, typegeneration).

Open to hear about other suggestions on how to improve this!

@jl-santana
Copy link
Contributor Author

@roshaans and I synced offline, tested the new flow, and found that using only one error state suffices for user communication. Additionally, the new flow prevents indexer registration in case of a schema error. I'll further test it on dev before deploying to prod.

# Conflicts:
#	frontend/yarn.lock
@jl-santana jl-santana merged commit 314dc2c into main Jan 3, 2024
4 checks passed
@jl-santana jl-santana deleted the feat/editor-error-logging branch January 3, 2024 21:07
@gabehamilton
Copy link
Collaborator

Additionally, the new flow prevents indexer registration in case of a schema error.

That sounds worrisome since our client side schema validation often hasn't supported valid SQL features that we need in the schema (for example, DEFAULT '')

@jl-santana
Copy link
Contributor Author

@gabehamilton valid point, can we meet about this?

@jl-santana
Copy link
Contributor Author

@gabehamilton, @darunrs and I synced offline to discuss this issue.

  • We decided to block the user from register a function if there is a formatting error on the schema, in the case of a type generation error, we will let the user know that a contextDB object wont be generated but he can choose whether to proceed

jl-santana added a commit that referenced this pull request Jan 11, 2024
More info about this can be found on this
[issue](#483)

Changes on this PR:
- Validate code & schema before registering the indexer
Logic: If formatting either the code or the schema fails, the `Publish`
button is disabled. Only if type generation errors or no errors are
detected, the `Publish` button will be enabled. More about this on this
[discussion](#480 (comment))


https://github.com/near/queryapi/assets/15988846/68f89cb4-f561-4e8a-9fa7-81c1a38e548c

Additionally 
- Created a reusable Modal with a global context to manage it, so we can
trigger it from any component to display some info
- Updated schema with granular error types for improved clarity
- Created a custom error class to filter by type Error. We can add more
fields on it if needed

---------

Co-authored-by: Juan Luis Santana <juanluis@near.org>
Co-authored-by: Roshaan Siddiqui <siddiqui.roshaan@gmail.com>
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.

None yet

3 participants