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.

Sign up for GitHub

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

Jump to bottom

draft: hiding filters showing no results (not working) #1403

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

draft: hiding filters showing no results (not working) #1403

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a93a009
draft: hiding filters showing no results (not working)
mbacherycz Nov 27, 2024
2768f62
fix
mbacherycz Nov 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions packages/frontend/src/pages/Inbox/Inbox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,19 @@ export const Inbox = ({ viewType }: InboxProps) => {
return filterDialogs(dataSource, activeFilters, format);
}, [dataSource, activeFilters]);

const filteredDialogs = useMemo(() => {
return filterDialogs(dataSource, activeFilters, format);
}, [dataSource, activeFilters]);

Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Remove duplicate memoized variable

The filteredDialogs variable duplicates the exact same logic as itemsToDisplay defined above. This creates unnecessary memory overhead and complexity.

Remove this duplicate and use the existing itemsToDisplay variable instead:

- const filteredDialogs = useMemo(() => {
-   return filterDialogs(dataSource, activeFilters, format);
- }, [dataSource, activeFilters]);

// const filterBarSettings = useMemo(() => {
// return getFilterBarSettings(dataSource, filteredDialogs, format);
// }, [dataSource, filteredDialogs]);

const filterBarSettings = useMemo(
() => getFilterBarSettings(dataSource, filteredDialogs, format).filter((setting) => setting.options.length > 1),
[dataSource, format],
);

// biome-ignore lint/correctness/useExhaustiveDependencies: Full control of what triggers this code is needed
useEffect(() => {
setActiveFilters([]);
Expand Down Expand Up @@ -198,11 +211,6 @@ export const Inbox = ({ viewType }: InboxProps) => {
}));
};

const filterBarSettings = useMemo(
() => getFilterBarSettings(dataSource, format).filter((setting) => setting.options.length > 1),
[dataSource, format],
);

const savedSearchDisabled = !activeFilters?.length && !searchString;
const showFilterButton = filterBarSettings.length > 0;

Expand Down
32 changes: 24 additions & 8 deletions packages/frontend/src/pages/Inbox/filters.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,11 @@ export const filterDialogs = (
});
};

export const getFilterBarSettings = (dialogs: InboxItemInput[], format: FormatFunction): FilterSetting[] => {
export const getFilterBarSettings = (
dialogs: InboxItemInput[],
filteredDialogs: InboxItemInput[],
format: FormatFunction,
): FilterSetting[] => {
Comment on lines +77 to +81
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider using only filteredDialogs for consistency

The function now takes both dialogs and filteredDialogs, but this creates potential inconsistencies in the counts vs. available options. Consider using only filteredDialogs for both counting and option generation to ensure consistency.

 export const getFilterBarSettings = (
-  dialogs: InboxItemInput[],
   filteredDialogs: InboxItemInput[],
   format: FormatFunction,
 ): FilterSetting[] => {

Committable suggestion skipped: line range outside the PR's diff.

return [
{
id: 'sender',
Expand All @@ -85,7 +89,11 @@ export const getFilterBarSettings = (dialogs: InboxItemInput[], format: FormatFu
options: (() => {
const senders = dialogs.map((p) => p.sender.name);
const senderCounts = countOccurrences(senders);
return Array.from(new Set(senders)).map((sender) => ({

const validSenders = filteredDialogs.map((p) => p.sender.name);
const uniqueSenders = Array.from(new Set(validSenders));

return uniqueSenders.map((sender) => ({
displayLabel: `${t('filter_bar_fields.from')} ${sender}`,
value: sender,
count: senderCounts[sender],
Expand All @@ -101,7 +109,11 @@ export const getFilterBarSettings = (dialogs: InboxItemInput[], format: FormatFu
options: (() => {
const receivers = dialogs.map((p) => p.receiver.name);
const receiversCount = countOccurrences(receivers);
return Array.from(new Set(receivers)).map((receiver) => ({

const validReceivers = filteredDialogs.map((p) => p.receiver.name);
const uniqueReceivers = Array.from(new Set(validReceivers));

return uniqueReceivers.map((receiver) => ({
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

Fix inconsistent receiver counts and reduce code duplication

  1. The counts are calculated from the original dialogs array, causing the same inconsistency issue as the sender filter.
  2. The logic for generating options is duplicated between sender and receiver filters.

First, fix the counting:

-const receivers = dialogs.map((p) => p.receiver.name);
+const receivers = filteredDialogs.map((p) => p.receiver.name);
 const receiversCount = countOccurrences(receivers);

Then, consider extracting a helper function to reduce duplication:

const getParticipantOptions = (
  dialogs: InboxItemInput[],
  participantType: 'sender' | 'receiver',
  labelPrefix: string
) => {
  const participants = dialogs.map((p) => p[participantType].name);
  const counts = countOccurrences(participants);
  const unique = Array.from(new Set(participants));
  
  return unique.map((name) => ({
    displayLabel: `${t(labelPrefix)} ${name}`,
    value: name,
    count: counts[name],
  }));
};

displayLabel: `${t('filter_bar_fields.to')} ${receiver}`,
value: receiver,
count: receiversCount[receiver],
Expand All @@ -116,12 +128,16 @@ export const getFilterBarSettings = (dialogs: InboxItemInput[], format: FormatFu
operation: 'includes',
horizontalRule: true,
options: (() => {
const status = dialogs.map((p) => p.status);
const statusCount = countOccurrences(status);
return Array.from(new Set(status)).map((statusLabel) => ({
const statuses = dialogs.map((p) => p.status);
const statusCounts = countOccurrences(statuses);

const validStatuses = filteredDialogs.map((p) => p.status);
const uniqueStatuses = Array.from(new Set(validStatuses));

return uniqueStatuses.map((statusLabel) => ({
displayLabel: t(`status.${statusLabel.toLowerCase()}`),
value: statusLabel,
count: statusCount[statusLabel],
count: statusCounts[statusLabel],
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

Based on the results from our verification attempts, I can generate the final response:

Fix status counts and verify translation key case handling

The status counts issue needs to be fixed as proposed in the diff, but regarding the translation keys, we found evidence in the translation files (nb.json and en.json) that the lowercase status keys are correct. The translation files contain entries like "status.completed", "status.draft", etc., which match the lowercase transformation in the code.

  • Fix the counting inconsistency by using filteredDialogs instead of dialogs
  • ✓ Translation key case handling is correct as-is
🔗 Analysis chain

Fix status counts and verify translation key case handling

  1. The status counts have the same inconsistency issue as other filters.
  2. The translation key uses lowercase which might not match the actual translation keys.
-const statuses = dialogs.map((p) => p.status);
+const statuses = filteredDialogs.map((p) => p.status);
 const statusCounts = countOccurrences(statuses);

Let's verify the translation keys:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if translation keys exist in lowercase
rg -l "status\.: \{" . | xargs rg "status\.[A-Z]+" -A 1

Length of output: 57

Script:

#!/bin/bash
# Let's try a different approach to find translation files and keys
fd -e json -e yaml -e yml | xargs rg "status\." -B 1 -A 1

# Also search for translation related files
fd "i18n|translation|locale" --type f

# Look for actual status values in the code
rg "status: ['\"](.*?)['\"]" --type ts

Length of output: 4105

Script:

#!/bin/bash
# Let's find the actual status values used in the dialogs
rg "status: ['\"]" packages/frontend/src/pages/Inbox -B 2 -A 2

# Also check how these status values are used in the code
ast-grep --pattern 'status.$_'

Length of output: 99

Script:

#!/bin/bash
# Let's try to find the Dialog type definition to see status values
rg "type.*Dialog.*status" --type ts -A 5

# And check for enum definitions related to status
rg "enum.*Status" --type ts -A 10

# Also look for actual status assignments in the Inbox directory
rg "\.status\s*(=|:)" packages/frontend/src/pages/Inbox

Length of output: 136

}));
})(),
},
Expand All @@ -132,7 +148,7 @@ export const getFilterBarSettings = (dialogs: InboxItemInput[], format: FormatFu
unSelectedLabel: t('filter_bar.label.all_dates'),
operation: 'equals',
options: generateDateOptions(
dialogs.map((p) => new Date(p.updatedAt)),
filteredDialogs.map((p) => new Date(p.updatedAt)),
format,
),
},
Expand Down
Loading