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

fix: updated row key in triggered alert list table #6154

Merged
merged 3 commits into from
Oct 14, 2024
Merged

Conversation

rahulkeswani101
Copy link
Contributor

@rahulkeswani101 rahulkeswani101 commented Oct 10, 2024

Summary

The key for each row was the same, duplicating the row if we did the sorting in the triggered alert list. To solve this issue we have to update the key to be unique.

Related Issues / PR's

#6148

Screenshots

Screen.Recording.2024-10-10.at.3.23.23.PM.mov

Affected Areas and Manually Tested Areas

  • Tested the triggered alert list view and checked if the current implementation doesn't affect other functionalities on this page.
  • Tested that duplicate rows are not created.

Important

Update rowKey in NoFilterTable to ensure unique keys and prevent duplicate rows when sorting.

  • Behavior:
    • Update rowKey in NoFilterTable to ${record.startsAt}-${record.fingerprint} to ensure unique keys and prevent duplicate rows when sorting.

This description was created by Ellipsis for 875c796. It will automatically update as commits are pushed.

@github-actions github-actions bot added the bug Something isn't working label Oct 10, 2024
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 875c796 in 18 seconds

More details
  • Looked at 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. frontend/src/container/TriggeredAlerts/NoFilterTable.tsx:101
  • Draft comment:
    Using startsAt as part of the rowKey can lead to non-unique keys if multiple alerts have the same startsAt and fingerprint. Consider using a more unique identifier if available.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is directly related to the change made in the diff, which is the modification of the rowKey. The concern raised is about the uniqueness of the key, which is a valid consideration for React keys. If fingerprint is not unique across alerts with the same startsAt, this could indeed lead to issues. However, without additional context on the uniqueness of fingerprint, it's hard to definitively say if this is an issue.
    I might be missing information about the fingerprint field and whether it is guaranteed to be unique. Without this information, it's difficult to assess the validity of the comment fully.
    The comment raises a valid point about key uniqueness, which is crucial for React components. Even without full context, it's better to err on the side of caution and consider the potential issue.
    Keep the comment as it raises a valid concern about the uniqueness of the rowKey, which is directly related to the change made in the diff.

Workflow ID: wflow_slivUkgqKNsVK2Qg


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@rahulkeswani101 rahulkeswani101 merged commit 90ae552 into develop Oct 14, 2024
13 checks passed
@rahulkeswani101 rahulkeswani101 deleted the SIG-6148 branch October 14, 2024 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants