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

feat: implement new table design #189

Merged
merged 20 commits into from
Jan 27, 2025
Merged

feat: implement new table design #189

merged 20 commits into from
Jan 27, 2025

Conversation

kantord
Copy link
Member

@kantord kantord commented Jan 24, 2025

This PR implements the new design for the alerts table.

There is an ongoing discussion to include all messages, not just alerts in this table and just pre-filter them to hide non-problematic sessions. This PR does not implement this idea at all, but it changes the columns of the table in a way that will prepare the ground for that change as well

Screenshot_select-area_20250124183049

@coveralls
Copy link
Collaborator

coveralls commented Jan 24, 2025

Pull Request Test Coverage Report for Build 12988563640

Details

  • 13 of 17 (76.47%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 68.232%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/components/AlertsTable.tsx 9 13 69.23%
Totals Coverage Status
Change from base Build 12985395565: -0.2%
Covered Lines: 766
Relevant Lines: 1020

💛 - Coveralls

@kantord kantord force-pushed the alerts-table-refactor branch from 89a78e7 to f22e138 Compare January 24, 2025 11:20
@kantord kantord force-pushed the alerts-table-refactor branch from f22e138 to f7c9c73 Compare January 24, 2025 11:28
@kantord kantord changed the title Alerts table refactor feat: implement new table design Jan 24, 2025
@kantord kantord marked this pull request as ready for review January 24, 2025 18:13
@kantord kantord enabled auto-merge (squash) January 24, 2025 18:13
@kantord kantord force-pushed the alerts-table-refactor branch from 1931241 to 7d55f72 Compare January 24, 2025 18:16
const maliciousPackage = getMaliciousPackage(alert.trigger_string);

if (maliciousPackage !== null && typeof maliciousPackage === "object") {
return "malicious_package";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for malicious package we can show the link to insights, it was already in place, I guess it could be very useful to show.

@jtenniswood maybe you didn't have an real example on that:
Screenshot 2025-01-27 at 09 45 05

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, if the entire table row is clickable, does it make sense to put another link to it?

I think that we should create an alert detail page 🤔 then we could put all these additional information there.

Actually I think that we can convert the current conversation page into an alert detail page? Basically keeping the current content but adding a header or sth similar where we can add details about the alert (I guess this could be hidden for conversations that don't have any alerts?)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, if the entire table row is clickable, does it make sense to put another link to it?

I agree, but I also see the value to show the detail about a malicious package, maybe instead of a clickable row we could add a column e.g. go to chat
I guess that James is not aware of this malicious package detail

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I totally agree, I prefer that over a clickable row any day. It's much clearer user experience in my opinion

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will mention this to James

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if that sub content should be on the sub page? I have also heard people wanting to be linked to the place where it happens in the conversation (also for secrets leaking).

@kantord kantord merged commit 97bc4ea into main Jan 27, 2025
7 of 8 checks passed
@kantord kantord deleted the alerts-table-refactor branch January 27, 2025 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants