Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add support for viewing search results in context for text logs (clp-text). #489
Add support for viewing search results in context for text logs (clp-text). #489
Changes from 6 commits
0007fb4
c6d89d9
44c15eb
17bcd11
420126a
a6bf9d2
c1f6e19
18b9411
f684c09
2b6d83a
ba61aa6
302ac3a
7eed866
d8c6b42
8c860e5
c81aef8
d6a5e9b
fb6c7b6
ae526eb
6568b5a
3f6d40f
50b9f70
46d8a7a
db770f7
ec84389
af59797
8bb0e84
2b52fd5
a48588f
4cf54ad
a270ff4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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.
create a const for string "uiTheme"
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.
Unused?
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.
Let's use
logEventIdx
.logEventIx
is a mistake that's prevalent in clp, but we should avoid propagating it if we can.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 method has side-effects (setting query state, changing the URL) that (1) aren't obvious from its name and (2) don't really seem specific to the api "module". Perhaps its better to simplify this method to just submitting the POST request and do the rest in the
then
andcatch
parts of the caller's 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.
I missed this in the previous PR, but why are we using snake_case for these? I think the only place we actually need to use snake_case is when inserting the row into database, right?
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.
In a similar vein,
msg_ix
should really belogEventIdx
until we have to insert it into the database in the server.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.
Right, I was using the webui code as an example but forgot the search query args are directly insert into the DB. Will update accordingly.
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.
Should this be
innerLogEventNum
following our naming rules for the new log viewer?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 reads as if we're going to append to
errorMsg
, but really we're implementing an if-else. Can we rewrite it as an if-else or did you mean to append toerrorMsg
?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.
Right, depending on whether the request was sent successfully and whether the server returns a valid response, we pick the error message (from one of the 3 places) that best represents the error.
Let's continue the discussion with these as references:
Sure
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.
If we're obsessed enough with syntactic sugar, we can write it as
(only if it doesn't give us more "?" when reading the code; pun intended lol
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.
😛 I think in this case, fewer "?" is better, lol.
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 move this into the query.js and use the enums as indices (so that they don't go out of sync)?
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.
How about
QueryStatus.jsx
?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 name this the same as the file?
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.
Proxy added to avoid CORS issues during development.