-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
fix[FE]: fix the update view button not visible on changes to Not Updating in Logs and Traces List View. #6454
Open
Aarif5435
wants to merge
1
commit into
SigNoz:develop
Choose a base branch
from
Aarif5435:fix(FE)/update-view-btn
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+42
−16
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
👍 Looks good to me! Reviewed everything up to eb4e88d in 42 seconds
More details
- Looked at
190
lines of code in5
files - Skipped
0
files when reviewing. - Skipped posting
7
drafted comments based on config settings.
1. frontend/src/container/ExplorerOptions/ExplorerOptions.tsx:256
- Draft comment:
The functiongetUpdatedExtraData
is defined twice in this file. Consider defining it once and reusing it to avoid redundancy. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment claims a duplicate function definition, but the full file content shows only one definition ofgetUpdatedExtraData
. This suggests the comment is incorrect. The comment should be removed as it does not reflect the actual state of the code.
I might have missed a subtle duplication or a similar function with a different name that serves the same purpose. However, the evidence strongly suggests there is no duplication.
The full file content shows only one instance ofgetUpdatedExtraData
, supporting the decision to remove the comment.
The comment about the duplicate function definition is incorrect and should be removed.
2. frontend/src/components/ExplorerCard/utils.ts:100
- Draft comment:
Remove trailing whitespace on this line for cleaner code. - Reason this comment was not posted:
Confidence changes required:10%
TheisQueryUpdatedInView
function has trailing whitespace on line 100, which is unnecessary and should be removed for cleaner code.
3. frontend/src/components/ExplorerCard/utils.ts:106
- Draft comment:
Remove trailing whitespace on this line for cleaner code. - Reason this comment was not posted:
Confidence changes required:10%
TheisQueryUpdatedInView
function is used in multiple files, and the logic seems consistent with the PR description. However, the trailing whitespace should be removed for cleaner code.
4. frontend/src/container/ExplorerOptions/ExplorerOptions.tsx:246
- Draft comment:
Avoid using inline styles. Move styles to an external stylesheet or styled components. This applies to other inline styles in this file as well. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is about inline styles, but the diff does not introduce any new inline styles. The comment is not about a change made in the diff, so it should be removed.
I might be missing some context about whether the inline styles were modified in a way not visible in the diff, but based on the information provided, it seems they were not.
The diff should show any changes to inline styles if they were made. Since it doesn't, the comment is likely not relevant to the changes.
The comment is not about a change made in the diff, so it should be deleted.
5. frontend/src/container/ExplorerOptions/ExplorerOptions.tsx:231
- Draft comment:
Use design tokens or predefined color constants instead of hardcoding color values. This applies to other instances of hardcoded colors in this file as well. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment seems to be addressing the use of hardcoded colors, but the specific line in question uses a predefined color constant, which aligns with best practices. The comment might be more relevant to other parts of the code, but it is not applicable to the specific change in the diff.
I might be missing other instances of hardcoded colors in the file that the comment could be referring to. However, the comment should be specific to the changes in the diff, and the line in question does not have hardcoded colors.
The comment should be specific to the changes in the diff, and since the line in question uses a predefined color constant, the comment is not applicable here.
The comment is not relevant to the specific change in the diff, as the line uses a predefined color constant. Therefore, the comment should be deleted.
6. frontend/src/container/ExplorerOptions/ExplorerOptions.tsx:232
- Draft comment:
Use design tokens or predefined color constants instead of hardcoding color values. This applies to other instances of hardcoded colors in this file as well. - Reason this comment was not posted:
Marked as duplicate.
7. frontend/src/container/ExplorerOptions/ExplorerOptions.tsx:247
- Draft comment:
Use design tokens or predefined color constants instead of hardcoding color values. This applies to other instances of hardcoded colors in this file as well. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_7wCV7aMVXiXOWvu1
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
… in logs and traces list view
YounixM
force-pushed
the
fix(FE)/update-view-btn
branch
from
November 18, 2024 05:37
eb4e88d
to
2dcd375
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fix: Update View Button Not Visible and Columns Not Updating in Logs and Traces List View
Description
This PR resolves two issues related to the "Update View" functionality in the Logs and Traces list view:
extraData
(i.e., columns) and failed to account for the newly added or removed columns.updateView
API was being called with outdatedextraData
(i.e., columns), overwriting the new changes with old data.Changes Made
extraData
(columns) with the updated columns to accurately detect changes.updateView
API call to include the newextraData
(columns) so that changes persist correctly after clicking the "Update View" button.Issue
Testing
Screenshot
update-view-btn.mp4
Notes
This fix ensures a smoother user experience when managing columns in the Logs and Traces list view, allowing users to reliably update and save their preferred view configurations.
Important
Fixes 'Update View' button visibility and column update issues in Logs and Traces list view by correcting comparison logic and ensuring API calls use updated data.
ExplorerOptions.tsx
by correcting comparison logic inisQueryUpdatedInView()
inutils.ts
.extraData
inupdateViewAsync
inExplorerOptions.tsx
.isQueryUpdatedInView()
inutils.ts
to compareoptions.selectColumns
withextraData.selectColumns
.onUpdateQueryHandler()
inExplorerOptions.tsx
to useupdatedExtraData
.options
toIsQueryUpdatedInViewProps
intypes.ts
andQueryBuilderContextType
inqueryBuilder.ts
.This description was created by for eb4e88d. It will automatically update as commits are pushed.