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

Feature: Audit Log - Add Filter Option for Event Type #312

Merged
merged 16 commits into from
Feb 19, 2025

Conversation

mindmonk
Copy link
Contributor

@mindmonk mindmonk commented Feb 6, 2025

This PR implements the feature requested in #286, adding a filter option for Audit Log Event Types.

Changes:

  • Added a dropdown in the filter popup listing all available event types.
  • Allows selecting multiple event types, displaying them as removable buttons inside the input field.
  • Entries are sorted alphabetically.
  • Added a clear button (trash icon) to reset the event type filter selection.
  • Unified the border styling of the sorting dropdown and result list with other components for consistency.
Event Type Filter closed Event Type Filter selection

@mindmonk mindmonk requested a review from SailReal February 6, 2025 13:14
Copy link

coderabbitai bot commented Feb 6, 2025

Walkthrough

This pull request introduces enhanced filtering for audit log events by adding support for multiple event types. The AuditLog.vue component on the frontend now includes a multi-select dropdown using a new Listbox component, an updated popover panel width, and a new removeEventType method to manage filter selections. The logic for applying and resetting filters has been extended to consider the selected event types, and the fetchData and applyFilter functions have been updated accordingly. Additionally, new internationalization entries are added to support the UI changes. On the backend, the getAllEvents method in AuditLogResource now accepts a type query parameter (a list of strings) with associated validation checks. The repository method findAllInPeriod in the AuditEvent class has been updated to use this parameter for event filtering. The AuditLogService in the frontend is also updated to pass the event type filters when fetching data.

Possibly related PRs

  • Feature: User Audit Log #310: Updates the getAllEvents method in AuditLogResource to support enhanced filtering by event types, aligning closely with the current parameter modifications.
  • Web of Trust Setup #305: Involves modifications to the audit log system, specifically the introduction of a new event type for logging updates to Web of Trust settings, which relates to the filtering enhancements in this PR.

Suggested reviewers

  • overheadhunter

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (5)
frontend/src/components/AuditLog.vue (5)

51-51: Consider making the filter popup width responsive.

The fixed width of w-96 might not be optimal for all screen sizes. Consider using responsive width classes.

-              <PopoverPanel class="absolute right-0 z-10 mt-2 origin-top-right rounded-md bg-white p-4 shadow-2xl ring-1 ring-black/5 ring-opacity-5 focus:outline-none w-96">
+              <PopoverPanel class="absolute right-0 z-10 mt-2 origin-top-right rounded-md bg-white p-4 shadow-2xl ring-1 ring-black/5 ring-opacity-5 focus:outline-none w-full sm:w-96">

66-68: Make the event type label more descriptive.

The current label "Event" could be more descriptive to better indicate it's for filtering.

-                      {{ t('auditLog.type') }}
+                      {{ t('auditLog.filter.eventType') }}

83-83: Review the color scheme for selected event types.

The green color scheme (bg-green-50, text-green-700, ring-green-600/20) might not align with the application's primary color scheme which uses the primary class elsewhere.

-                              class="inline-flex items-center rounded-md bg-green-50 px-2 py-1 text-xs font-medium text-green-700 ring-1 ring-inset ring-green-600/20">
+                              class="inline-flex items-center rounded-md bg-primary/10 px-2 py-1 text-xs font-medium text-primary ring-1 ring-inset ring-primary/20">

325-325: Enhance type safety and maintainability of event type handling.

Consider these improvements:

  1. Define an enum or union type for event types
  2. Move event type options to a constant
  3. Add JSDoc documentation
+// Define event type enum
+enum AuditEventType {
+  ALL = 'ALL',
+  DEVICE_REGISTER = 'DEVICE_REGISTER',
+  DEVICE_REMOVE = 'DEVICE_REMOVE',
+  // ... add other event types
+}
+
+// Define event type options type
+type EventTypeOption = {
+  label: string;
+  value: AuditEventType;
+};
+
-const selectedEventTypes = ref<string[]>(['ALL']);
+const selectedEventTypes = ref<AuditEventType[]>([AuditEventType.ALL]);

+/**
+ * Maps event types to their localized labels.
+ * The entries are sorted alphabetically with 'ALL' always first.
+ */
const eventTypeOptions = Object.fromEntries(
  Object.entries({
    [AuditEventType.ALL]: t('auditLog.details.all'),
    [AuditEventType.DEVICE_REGISTER]: t('auditLog.details.device.register'),
    // ... rest of the mappings
  }).sort((a, b) => {
    if (a[0] === AuditEventType.ALL) return -1;
    if (b[0] === AuditEventType.ALL) return 1;
    return a[1].localeCompare(b[1]);
  })
);

Also applies to: 328-348


361-371: Consider server-side filtering for better performance.

The current implementation filters event types client-side after fetching all data. For large datasets, this could impact performance.

Consider:

  1. Adding event type filter to the backend API
  2. Implementing loading state during filtering
  3. Adding error handling
async function applyFilter() {
  if (filterIsValid.value) {
+    state.value = State.Loading;
+    try {
      startDate.value = beginOfDate(new Date(startDateFilter.value));
      endDate.value = endOfDate(new Date(endDateFilter.value));
-      await fetchData();
-
-      if (!selectedEventTypes.value.includes('ALL')) {
-        auditEvents.value = auditEvents.value.filter(event => selectedEventTypes.value.includes(event.type));
-      }
+      const eventTypes = selectedEventTypes.value.includes('ALL') ? undefined : selectedEventTypes.value;
+      await fetchData(0, eventTypes);
+    } catch (error) {
+      console.error('Failed to apply filter:', error);
+      onFetchError.value = error instanceof Error ? error : new Error('Failed to apply filter');
+    }
  }
}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c18af8c and 527fd75.

📒 Files selected for processing (3)
  • frontend/src/components/AuditLog.vue (5 hunks)
  • frontend/src/i18n/de-DE.json (1 hunks)
  • frontend/src/i18n/en-US.json (1 hunks)
🔇 Additional comments (2)
frontend/src/i18n/en-US.json (1)

81-81: LGTM!

The addition of the "All" translation entry is correct and consistent with the feature requirements.

frontend/src/i18n/de-DE.json (1)

81-81: LGTM!

The addition of the "Alle" translation entry is correct and provides proper German localization.

Copy link
Member

@SailReal SailReal left a comment

Choose a reason for hiding this comment

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

Good start!

I think there were a problem regarding merging the Tailwind 4 changes into this branch, that's the reason for all my "Revert it back" comments.

Here are some further findings:

  • Change the date (but do not touch the events filter), the apply button does not close the modal and starts the filter

    image

  • We should filter the events in the backend because when you create e.g. 5 vaults, add a lot of users to it and remove them again, all create events are e.g. on page 2, then the result for the event list shows "Showing entries 0 to 0" when you filter for "create vault".

  • Clicking on the next button clears the events filter.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (3)
frontend/src/components/AuditLog.vue (3)

327-348: Consider extracting 'ALL' as a constant.

While the implementation is good, the 'ALL' string is still hardcoded in multiple places. This could lead to maintenance issues if the value needs to change.

+const ALL_EVENT_TYPE = 'ALL';
+
-const selectedEventTypes = ref<string[]>(['ALL']);
+const selectedEventTypes = ref<string[]>([ALL_EVENT_TYPE]);
 const eventTypeOptions = Object.fromEntries(
   Object.entries({
-    ALL: t('auditLog.details.all'),
+    [ALL_EVENT_TYPE]: t('auditLog.details.all'),
     DEVICE_REGISTER: t('auditLog.details.device.register'),
     // ... rest of the options
   }).sort((a, b) => {
-    if (a[0] === 'ALL') return -1;
-    if (b[0] === 'ALL') return 1;
+    if (a[0] === ALL_EVENT_TYPE) return -1;
+    if (b[0] === ALL_EVENT_TYPE) return 1;
     return a[1].localeCompare(b[1]);
   })
 );

361-371: Consider implementing server-side filtering for event types.

The current implementation filters event types client-side after fetching all events. This approach might not scale well with large datasets. Consider:

  1. Adding event type filter parameters to the backend API
  2. Implementing the filtering logic on the server side
  3. Only fetching the filtered events

373-377: Simplify removeEventType function.

Since the button is already disabled for 'ALL' type, the check in the function is redundant.

 function removeEventType(type: string): void {
-  if (type !== 'ALL') {
-    selectedEventTypes.value = selectedEventTypes.value.filter(t => t !== type);
-  }
+  selectedEventTypes.value = selectedEventTypes.value.filter(t => t !== type);
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cbc75d9 and 69f4a01.

📒 Files selected for processing (1)
  • frontend/src/components/AuditLog.vue (3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Build and Test
🔇 Additional comments (2)
frontend/src/components/AuditLog.vue (2)

51-51: Well-implemented UI for event type filtering!

The implementation follows best practices with:

  • Proper accessibility attributes
  • Clear visual feedback for selected items
  • Consistent styling with other components

Also applies to: 65-112


350-359: Add error handling for invalid event types.

The watch function should validate that only valid event types are selected.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (2)
frontend/src/components/AuditLog.vue (2)

70-112: Consider extracting color-specific classes into variables.

The implementation is solid, but the hardcoded color classes could be extracted into variables for better maintainability and theme consistency.

Consider refactoring the color classes into variables:

-              class="inline-flex items-center rounded-md bg-green-50 px-2 py-1 text-xs font-medium text-green-700 ring-1 ring-inset ring-green-600/20"
+              :class="[
+                'inline-flex items-center rounded-md px-2 py-1 text-xs font-medium',
+                'bg-tag-bg text-tag-text ring-1 ring-inset ring-tag-border'
+              ]"

Then define these variables in your CSS:

:root {
  --tag-bg: rgb(240 253 244); /* green-50 */
  --tag-text: rgb(21 128 61); /* green-700 */
  --tag-border: rgb(22 163 74 / 0.2); /* green-600/20 */
}

328-350: Add TypeScript types for better type safety.

Consider adding explicit types for the event types to improve type safety and maintainability.

Add these type definitions:

type EventType = 'ALL' | 'DEVICE_REGISTER' | 'DEVICE_REMOVE' | /* ... other types */;
const ALL: EventType = 'ALL';
const selectedEventTypes = ref<EventType[]>([ALL]);
const eventTypeOptions: Record<EventType, string> = Object.fromEntries(/* ... */);
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 69f4a01 and fdfa1a0.

📒 Files selected for processing (1)
  • frontend/src/components/AuditLog.vue (3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Build and Test
🔇 Additional comments (3)
frontend/src/components/AuditLog.vue (3)

51-51: LGTM!

The increased width of the PopoverPanel provides better accommodation for the new event type filter.


352-361: Add validation for event type selection.

The watch function should validate that only valid event types can be selected.


375-379: LGTM!

The removeEventType function is well-implemented with proper safety checks.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

♻️ Duplicate comments (1)
frontend/src/components/AuditLog.vue (1)

370-372: Move filtering logic to the backend.

Client-side filtering can lead to pagination issues when the filtered results don't match the expected page size. Consider moving this logic to the backend.

🧹 Nitpick comments (2)
backend/src/main/java/org/cryptomator/hub/api/AuditLogResource.java (2)

57-57: Enhance the parameter documentation.

The current description "the list of type of events to return. Empty list is all events." could be more specific about valid event types and the behavior of the ALL option.

-@Parameter(name = "type", description = "the list of type of events to return. Empty list is all events.", in = ParameterIn.QUERY)
+@Parameter(name = "type", description = "List of event types to filter by (e.g., DEVICE_REGISTER, VAULT_CREATE). An empty list or omitting this parameter returns all event types.", in = ParameterIn.QUERY)

75-76: Consider allowing null type parameter.

The current implementation throws a BadRequestException if type is null, but it might be more user-friendly to treat null as an empty list, consistent with the behavior for retrieving all events.

-} else if (type == null) {
-    throw new BadRequestException("type must be specified");
+} else if (type == null) {
+    type = List.of(); // Treat null as empty list to get all events
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fdfa1a0 and c4ec06a.

📒 Files selected for processing (4)
  • backend/src/main/java/org/cryptomator/hub/api/AuditLogResource.java (2 hunks)
  • backend/src/main/java/org/cryptomator/hub/entities/events/AuditEvent.java (4 hunks)
  • frontend/src/common/auditlog.ts (1 hunks)
  • frontend/src/components/AuditLog.vue (4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Build and Test
🔇 Additional comments (2)
backend/src/main/java/org/cryptomator/hub/entities/events/AuditEvent.java (1)

35-35: Verify the SQL query performance with type filtering.

The added type filtering condition (:allTypes = true OR ae.type IN :types) might benefit from an index on the type column to optimize query performance, especially with large datasets.

Also applies to: 45-45

❌ Verification inconclusive

🏁 Script executed:

#!/bin/bash
# Check if an index exists on the type column
rg -A 5 'CREATE.*INDEX.*type' 

Length of output: 31


I'll gather a bit more information by checking if any JPA index annotations are present on the type field in the AuditEvent.java entity. This could shed more light on whether an index exists at the ORM level. Please see the following shell script for further verification:


🏁 Script executed:

#!/bin/bash
# Check for inline JPA index annotations within AuditEvent.java regarding "type" column or similar.
rg -i '@Index' backend/src/main/java/org/cryptomator/hub/entities/events/AuditEvent.java

Length of output: 88


SQL Query Performance – Missing Index on Type Column

Our investigation did not reveal any index defined for the type column through SQL migration scripts or JPA annotations in the AuditEvent.java entity. This suggests that the filter condition (:allTypes = true OR ae.type IN :types) might not be optimized for large datasets, and adding an index on the type column could improve performance.

  • No SQL index definition for type was found in migration files.
  • No JPA index annotation (e.g., via @Index or @Table) exists on the type field in AuditEvent.java.

It is recommended to review the expected dataset sizes and consider adding an index on the type column if performance becomes an issue.

frontend/src/components/AuditLog.vue (1)

353-362: Add error handling for invalid event types.

The watch function should validate that only valid event types are selected.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (1)
backend/src/main/java/org/cryptomator/hub/api/AuditLogResource.java (1)

58-58: Enhance parameter documentation with allowed values.

The description should list the valid event types to help API consumers. Consider updating it to include the allowed values from lines 79-82.

-	@Parameter(name = "type", description = "the list of type of events to return. Empty list is all events.", in = ParameterIn.QUERY)
+	@Parameter(name = "type", description = "List of event types to return. Valid types are: DeviceRegistered, DeviceRemoved, SettingWotUpdate, SignedWotId, VaultCreated, VaultUpdated, VaultAccessGranted, VaultKeyRetrieved, VaultMemberAdded, VaultMemberRemoved, VaultMemberUpdated, VaultOwnershipClaimed. Empty list returns all events.", in = ParameterIn.QUERY)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c4ec06a and 0aca5ee.

📒 Files selected for processing (1)
  • backend/src/main/java/org/cryptomator/hub/api/AuditLogResource.java (3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Build and Test
🔇 Additional comments (2)
backend/src/main/java/org/cryptomator/hub/api/AuditLogResource.java (2)

63-63: LGTM!

The method signature update is well-structured, using appropriate types and annotations.


90-90: LGTM!

The repository method call is correctly updated to include the new type parameter while maintaining existing functionality.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (3)
frontend/src/components/AuditLog.vue (3)

71-71: Fix non-standard width class.

The width class w-88 appears to be non-standard in Tailwind CSS. Consider using a standard width class like w-96 or defining a custom width in your Tailwind configuration if needed.

-      <div class="relative w-88">
+      <div class="relative w-96">

302-304: Add validation for event types before sending to backend.

While the filtering logic is correct, it's recommended to validate event types before sending them to the backend to prevent potential issues with invalid types.

-    const filterEventTypes = selectedEventTypes.value.includes(ALL) ? [] : selectedEventTypes.value;
+    const filterEventTypes = selectedEventTypes.value.includes(ALL) ? [] : selectedEventTypes.value.filter(type => 
+      type !== ALL && Object.keys(eventTypeOptions).includes(type)
+    );

339-361: Enhance type safety for event types.

Consider using a TypeScript enum or union type for event types to improve type safety and maintainability.

type EventType = 'ALL' | 'DEVICE_REGISTER' | 'DEVICE_REMOVE' | /* ... other types ... */;

const ALL = 'ALL' as const;
const selectedEventTypes = ref<EventType[]>([ALL]);
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0aca5ee and 433dc95.

📒 Files selected for processing (5)
  • backend/src/main/java/org/cryptomator/hub/api/AuditLogResource.java (3 hunks)
  • frontend/src/common/auditlog.ts (1 hunks)
  • frontend/src/components/AuditLog.vue (6 hunks)
  • frontend/src/i18n/de-DE.json (1 hunks)
  • frontend/src/i18n/en-US.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
  • frontend/src/i18n/en-US.json
  • frontend/src/i18n/de-DE.json
  • frontend/src/common/auditlog.ts
  • backend/src/main/java/org/cryptomator/hub/api/AuditLogResource.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Build and Test
🔇 Additional comments (2)
frontend/src/components/AuditLog.vue (2)

255-260: LGTM! Well-integrated filter reset logic.

The updated filterIsReset computed property correctly handles the new event type filter state alongside existing date filters.


363-372: Add error handling in the watch function.

The watch function should validate event types to prevent invalid selections.

@mindmonk mindmonk requested a review from SailReal February 14, 2025 16:39
SailReal and others added 2 commits February 17, 2025 11:55
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🔭 Outside diff range comments (1)
frontend/src/components/AuditLog.vue (1)

332-363: 💡 Verification agent

🧩 Analysis chain

Verify error handling for invalid event types.

Ensure that the backend API properly validates the event types and that the frontend handles any validation errors gracefully.


🏁 Script executed:

#!/bin/bash
# Search for error handling in the backend API
rg -A 5 "getAllEvents.*type.*" --type java

Length of output: 964


Action Required: Add Explicit Validation for Event Types

  • Backend: In the getAllEvents() method of AuditLogResource.java, the API accepts a List<String> for the event types but does not appear to perform any explicit validation on these values. Please ensure that invalid or unsupported event types are caught early, with the API returning clear, descriptive error messages.
  • Frontend: Confirm that the fetchData() function in frontend/src/components/AuditLog.vue can gracefully handle any validation errors (e.g., improper type values) propagated from the backend, distinguishing them from other error types where applicable.
🧹 Nitpick comments (3)
frontend/src/components/AuditLog.vue (3)

79-119: Add keyboard navigation for removing selected event types.

While the implementation is solid, consider improving keyboard accessibility for removing selected event types.

 <button 
   v-for="type in selectedEventTypes" 
   :key="type"
   class="inline-flex items-center rounded-md bg-green-50 px-2 py-1 text-xs font-medium text-green-700 ring-1 ring-inset ring-green-600/20"
+  @keydown.delete="removeEventType(type)"
+  @keydown.backspace="removeEventType(type)"
   @click.stop="removeEventType(type)"
+  tabindex="0"
 >

297-318: Memoize event type options for better performance.

The event type options object is recreated on every component render. Consider using computed to memoize it.

-const eventTypeOptions = Object.fromEntries(
+const eventTypeOptions = computed(() => Object.fromEntries(
   Object.entries({
     DEVICE_REGISTER: t('auditLog.details.device.register'),
     // ... other entries
   }).sort(([,valueA], [,valueB]) => valueA.localeCompare(valueB))
-);
+));
-const allEventTypes = Object.keys(eventTypeOptions);
+const allEventTypes = computed(() => Object.keys(eventTypeOptions.value));

328-330: Optimize the watch handler for selectedEventTypes.

The current implementation sorts the array on every selection change. Consider sorting only when necessary.

 watch(selectedEventTypes, (newSelection, oldSelection) => {
+  if (newSelection.length !== oldSelection.length) {
     selectedEventTypes.value.sort((a, b) => eventTypeOptions[a].localeCompare(eventTypeOptions[b]));
+  }
 });
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 250cc2f and 2bd907d.

📒 Files selected for processing (2)
  • frontend/src/components/AuditLog.vue (7 hunks)
  • frontend/src/i18n/en-US.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • frontend/src/i18n/en-US.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Build and Test
🔇 Additional comments (3)
frontend/src/components/AuditLog.vue (3)

65-78: LGTM! Clean implementation of the event type filter header.

The implementation provides a clear visual hierarchy and includes a convenient clear button with proper disabled state handling.


120-127: LGTM! Proper handling of async filter application.

The implementation correctly handles the asynchronous nature of filter application and maintains UI consistency during the operation.


379-381: LGTM! Clean implementation of event type removal.

The removeEventType function is concise and effective.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (3)
frontend/src/components/AuditLog.vue (3)

85-92: Enhance accessibility of the event type remove button.

The close button (×) in the selected event type pills needs proper ARIA attributes and a more descriptive label for screen readers.

 <button 
   v-for="type in selectedEventTypes" 
   :key="type"
   class="inline-flex items-center rounded-md bg-green-50 px-2 py-1 text-xs font-medium text-green-700 ring-1 ring-inset ring-green-600/20"
   @click.stop="removeEventType(type)"
+  :aria-label="t('auditLog.filter.removeEventType', { type: eventTypeOptions[type] })"
 >
   <span class="mr-1">{{ eventTypeOptions[type] }}</span>
-  <span class="text-green-800 font-bold">&times;</span>
+  <span class="text-green-800 font-bold" aria-hidden="true">&times;</span>
 </button>

297-315: Consider memoizing the eventTypeOptions object.

The eventTypeOptions object is recreated on every component mount. Consider using a computed property or moving it outside the component to improve performance.

-const eventTypeOptions = Object.fromEntries(
+const eventTypeOptions = computed(() => Object.fromEntries(
   Object.entries({
     DEVICE_REGISTER: t('auditLog.details.device.register'),
     // ... other entries
   }).sort(([,valueA], [,valueB]) => valueA.localeCompare(valueB))
-);
+));

379-381: Add type validation in removeEventType function.

The removeEventType function should validate the type parameter before removing it.

 function removeEventType(type: string): void {
+  if (!Object.keys(eventTypeOptions.value).includes(type)) {
+    console.error(`Invalid event type: ${type}`);
+    return;
+  }
   selectedEventTypes.value = selectedEventTypes.value.filter(t => t !== type);
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2bd907d and 6d042e6.

📒 Files selected for processing (1)
  • frontend/src/components/AuditLog.vue (7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Build and Test
🔇 Additional comments (2)
frontend/src/components/AuditLog.vue (2)

124-124: LGTM! Good improvement in filter application handling.

The async click handler ensures that the filter is fully applied before closing the popover, preventing potential race conditions.


328-330: Add validation in the watch handler for selectedEventTypes.

The watch handler should validate that only valid event types are selected.

Copy link
Member

@SailReal SailReal left a comment

Choose a reason for hiding this comment

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

Very good result 👍

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
frontend/src/components/AuditLog.vue (1)

329-331: Consider adding validation for event types.

The watch handler should validate that only valid event types are selected.

 watch(selectedEventTypes, (newSelection, oldSelection) => {
+  // Validate all event types are valid
+  const invalidTypes = newSelection.filter(type => !allEventTypes.includes(type));
+  if (invalidTypes.length > 0) {
+    console.error(`Invalid event types selected: ${invalidTypes.join(', ')}`);
+    selectedEventTypes.value = oldSelection;
+    return;
+  }
   selectedEventTypes.value.sort((a, b) => eventTypeOptions[a].localeCompare(eventTypeOptions[b]));
 });
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cf52bf8 and 80aca9f.

📒 Files selected for processing (4)
  • CHANGELOG.md (1 hunks)
  • backend/src/main/java/org/cryptomator/hub/api/AuditLogResource.java (3 hunks)
  • frontend/src/common/auditlog.ts (1 hunks)
  • frontend/src/components/AuditLog.vue (7 hunks)
✅ Files skipped from review due to trivial changes (1)
  • CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
  • frontend/src/common/auditlog.ts
  • backend/src/main/java/org/cryptomator/hub/api/AuditLogResource.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Build and Test
🔇 Additional comments (4)
frontend/src/components/AuditLog.vue (4)

65-78: LGTM! Well-implemented clear button with proper accessibility.

The clear button implementation follows best practices with:

  • Proper disabled state handling
  • Accessibility attributes (title and aria-hidden)
  • Consistent styling with the design system

79-120: LGTM! Well-structured multi-select implementation.

The Listbox implementation is well done with:

  • Proper keyboard navigation support via Headless UI
  • Clear visual feedback for selected items
  • Proper ARIA attributes for accessibility
  • Consistent styling with other dropdowns

298-316: LGTM! Well-structured event type options with proper sorting.

The implementation:

  • Uses i18n translations for internationalization
  • Sorts options alphabetically for better UX
  • Maintains type safety with proper TypeScript usage

380-382: LGTM! Clean implementation of event type removal.

The removeEventType function is concise and effective.

@SailReal SailReal merged commit a36ef63 into develop Feb 19, 2025
6 checks passed
@SailReal SailReal deleted the feature/audit-log-event-filter branch February 19, 2025 15:17
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.

2 participants