-
Notifications
You must be signed in to change notification settings - Fork 316
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
Show Language Server Logs #9745
base: develop
Are you sure you want to change the base?
Conversation
}, [logsRaw]) | ||
|
||
return ( | ||
<Modal centered className="bg-dim"> |
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.
Can we use ariaComponents.Dialog type="fullscreen"
here?
const { getText } = textProvider.useText() | ||
|
||
React.useEffect(() => { | ||
if (logsRaw instanceof Promise) { |
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.
Use please useQuery
here fetching is useEffect
is a bad practice
{getText('logs')} | ||
</aria.Heading> | ||
<pre className="relative overflow-auto whitespace-pre-wrap"> | ||
<code>{logs}</code> |
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.
Btw what's the order of the logs?
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.
it's a single string here. but i the endpoint returns the logs from oldest to newest (i.e. the same orer as they would appear in a terminal)
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.
can we reverse scrolling then? I think showing the oldest logs isn't what users want(often they're looking for the recent error). We can either use bottom-to-top scrolling(using col-reverse order hack in flex) or stream logs in reverse order from lambda
Small question, do we limit the amount of logs to show simultaneously? |
for the time being no |
What'll happen if a user has more than, lets say 1 000 000 entries(50mb) of logs? Are we going to process and display all of them? |
@MrFlashAccount we're assuming that won't happen for the time being - we definitely want pagination and scrollback at some point, however i think it's not something we should be adding as an afterthought on a random PR |
code review should be addressed - not 100% sure the dialog still works properly, but we can't test it unless the logs PR is deployed |
Pull Request Description
Important Notes
None
Testing instructions
Just make sure that:
Screenshots
Project sessions list:
Logs modal:
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
The documentation has been updated, if necessary.Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
Unit tests have been written where possible.If GUI codebase was changed, the GUI was tested when built using./run ide build
.