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
new-log-viewer: Add event cursor, refactor state context, and redesign load page methods to support discontinuous log event numbers in preparation for log level filtering. #76
new-log-viewer: Add event cursor, refactor state context, and redesign load page methods to support discontinuous log event numbers in preparation for log level filtering. #76
Changes from 15 commits
370af54
96927bf
fcd0ee9
d88bc3d
ceb8a4d
479c1a8
4d1e70a
662619b
ed2d9c4
26a0d02
b32c5bd
c6f1f5f
d2ed5b9
0050180
a523d18
cf096fa
99f4003
78a1f8f
a11295a
18599f1
829ba10
1572a91
1e6ad3f
31012ff
6200bd4
6f43e7f
d266ce7
0419552
3887f76
808a7e1
d8b32ea
a55a3c8
c26459d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
🛠️ Refactor suggestion
Consider refactoring for maintainability and fix lint error
The
StateContextProvider
component and its associated functions have grown quite complex. While the current implementation is functional, it might become harder to maintain as the codebase evolves.Consider breaking down the
StateContextProvider
into smaller, more focused components or hooks. This could improve readability and make it easier to test individual pieces of functionality.Also, there's a minor lint error:
On line 323, there are trailing spaces that should be removed to comply with the project's linting rules.
Addressing these points will improve the overall code quality and maintainability of this file.
Also applies to: 323-323
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:
loadPage
->loadPageByCursor
loadPageAction
->loadPageByAction
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.
done
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.
Instead of passing specificPageNum separately, can we create a type like CursorType that includes args? I know we already have
ActionType
, so we'll probably need to rename that to something likeUiActionType
.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 tried something similar to worker req/response type, but slightly more complex. See in actions.ts
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 still useful to clamp the log event number, right? The idea being if we clamp the event number and it ends up being on the current page, then we save a round-trip to the worker. And in the case that we don't need to change pages, we should still update the URL with the clamped log event number.
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.
Yes I believe this can save a request. I updated so we have the feature.
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 Junhao intends to have two newlines separating imports/exports from code, but it's not easy to enforce with eslint.
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.
done
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.
We don't need to export this any more, 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.
done