[hold] Add "Edit filters" to saved views#94547
Conversation
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
melvin-bot
Bot
requested review from
FitseTLT and
trjExpensify
and removed request for
a team
|
@trjExpensify can you pls trigger translations here?
|
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08f70e2061
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Is this ready for my review? @mukhrr |
@FitseTLT I think no #92770 (comment) |
|
Running translations for you. I'm also going to switch @flaviadefaria on for the product review here, as she's been running this one. 👍 |
trjExpensify
requested review from
flaviadefaria
and removed request for
trjExpensify
🦜 Polyglot Parrot! 🦜Squawk! Looks like you added some shiny new English strings. Allow me to parrot them back to you in other tongues: View the translation diffdiff --git a/src/languages/de.ts b/src/languages/de.ts
index 72935d43be9..28965d95e44 100644
--- a/src/languages/de.ts
+++ b/src/languages/de.ts
@@ -8431,9 +8431,9 @@ Fügen Sie weitere Ausgabelimits hinzu, um den Cashflow Ihres Unternehmens zu sc
groupColumns: 'Spalten gruppieren',
expenseColumns: 'Spalten für Ausgaben',
saveView: 'Ansicht speichern',
- editFilters: 'Edit filters',
- saveEdits: 'Save edits',
- saveAsNewView: 'Save as new view',
+ editFilters: 'Filter bearbeiten',
+ saveEdits: 'Änderungen speichern',
+ saveAsNewView: 'Als neue Ansicht speichern',
deleteSavedSearch: 'Gespeicherte Suche löschen',
deleteSavedSearchConfirm: 'Möchtest du diese Suche wirklich löschen?',
searchName: 'Namen suchen',
diff --git a/src/languages/fr.ts b/src/languages/fr.ts
index 3b55b9c0cd9..87a1834c74f 100644
--- a/src/languages/fr.ts
+++ b/src/languages/fr.ts
@@ -8465,9 +8465,9 @@ Ajoutez davantage de règles de dépenses pour protéger la trésorerie de l’e
groupColumns: 'Regrouper les colonnes',
expenseColumns: 'Colonnes de dépenses',
saveView: 'Enregistrer la vue',
- editFilters: 'Edit filters',
- saveEdits: 'Save edits',
- saveAsNewView: 'Save as new view',
+ editFilters: 'Modifier les filtres',
+ saveEdits: 'Enregistrer les modifications',
+ saveAsNewView: 'Enregistrer comme nouvelle vue',
deleteSavedSearch: 'Supprimer la recherche enregistrée',
deleteSavedSearchConfirm: 'Voulez-vous vraiment supprimer cette recherche ?',
searchName: 'Rechercher un nom',
diff --git a/src/languages/it.ts b/src/languages/it.ts
index db83018ccba..268f26c5c9e 100644
--- a/src/languages/it.ts
+++ b/src/languages/it.ts
@@ -8420,9 +8420,9 @@ Aggiungi altre regole di spesa per proteggere il flusso di cassa aziendale.`,
groupColumns: 'Raggruppa colonne',
expenseColumns: 'Colonne spese',
saveView: 'Salva vista',
- editFilters: 'Edit filters',
- saveEdits: 'Save edits',
- saveAsNewView: 'Save as new view',
+ editFilters: 'Modifica filtri',
+ saveEdits: 'Salva modifiche',
+ saveAsNewView: 'Salva come nuova vista',
deleteSavedSearch: 'Elimina ricerca salvata',
deleteSavedSearchConfirm: 'Sei sicuro di voler eliminare questa ricerca?',
searchName: 'Cerca nome',
diff --git a/src/languages/ja.ts b/src/languages/ja.ts
index c9d5b2813e4..3c5c971ee73 100644
--- a/src/languages/ja.ts
+++ b/src/languages/ja.ts
@@ -8319,9 +8319,9 @@ ${reportName}`,
groupColumns: '列をグループ化',
expenseColumns: '経費列',
saveView: 'ビューを保存',
- editFilters: 'Edit filters',
- saveEdits: 'Save edits',
- saveAsNewView: 'Save as new view',
+ editFilters: 'フィルターを編集',
+ saveEdits: '編集を保存',
+ saveAsNewView: '新しいビューとして保存',
deleteSavedSearch: '保存した検索を削除',
deleteSavedSearchConfirm: 'この検索を削除してもよろしいですか？',
searchName: '名前を検索',
diff --git a/src/languages/nl.ts b/src/languages/nl.ts
index 7d2f38818ec..6787d830231 100644
--- a/src/languages/nl.ts
+++ b/src/languages/nl.ts
@@ -8387,9 +8387,9 @@ er bestedingsregels toe om de kasstroom van het bedrijf te beschermen.`,
groupColumns: 'Kolommen groeperen',
expenseColumns: 'Onkostencolommen',
saveView: 'Weergave opslaan',
- editFilters: 'Edit filters',
- saveEdits: 'Save edits',
- saveAsNewView: 'Save as new view',
+ editFilters: 'Filters bewerken',
+ saveEdits: 'Wijzigingen opslaan',
+ saveAsNewView: 'Opslaan als nieuwe weergave',
deleteSavedSearch: 'Opgeslagen zoekopdracht verwijderen',
deleteSavedSearchConfirm: 'Weet je zeker dat je deze zoekopdracht wilt verwijderen?',
searchName: 'Zoeknaam',
diff --git a/src/languages/pl.ts b/src/languages/pl.ts
index 0c3a8051c93..bc9c740e638 100644
--- a/src/languages/pl.ts
+++ b/src/languages/pl.ts
@@ -8380,9 +8380,9 @@ Dodaj więcej zasad wydatków, żeby chronić płynność finansową firmy.`,
groupColumns: 'Grupuj kolumny',
expenseColumns: 'Kolumny wydatków',
saveView: 'Zapisz widok',
- editFilters: 'Edit filters',
- saveEdits: 'Save edits',
- saveAsNewView: 'Save as new view',
+ editFilters: 'Edytuj filtry',
+ saveEdits: 'Zapisz zmiany',
+ saveAsNewView: 'Zapisz jako nowy widok',
deleteSavedSearch: 'Usuń zapisaną wyszukiwanie',
deleteSavedSearchConfirm: 'Na pewno chcesz usunąć to wyszukiwanie?',
searchName: 'Wyszukaj nazwę',
diff --git a/src/languages/pt-BR.ts b/src/languages/pt-BR.ts
index d590a9b5202..6800db8f7b0 100644
--- a/src/languages/pt-BR.ts
+++ b/src/languages/pt-BR.ts
@@ -8378,9 +8378,9 @@ Adicione mais regras de gasto para proteger o fluxo de caixa da empresa.`,
groupColumns: 'Agrupar colunas',
expenseColumns: 'Colunas de despesas',
saveView: 'Salvar visualização',
- editFilters: 'Edit filters',
- saveEdits: 'Save edits',
- saveAsNewView: 'Save as new view',
+ editFilters: 'Editar filtros',
+ saveEdits: 'Salvar edições',
+ saveAsNewView: 'Salvar como nova visualização',
deleteSavedSearch: 'Excluir pesquisa salva',
deleteSavedSearchConfirm: 'Tem certeza de que deseja excluir esta pesquisa?',
searchName: 'Pesquisar nome',
diff --git a/src/languages/zh-hans.ts b/src/languages/zh-hans.ts
index f5fa8793aba..f2f9129f285 100644
--- a/src/languages/zh-hans.ts
+++ b/src/languages/zh-hans.ts
@@ -8149,9 +8149,9 @@ ${reportName}`,
groupColumns: '分组列',
expenseColumns: '报销列',
saveView: '保存视图',
- editFilters: 'Edit filters',
- saveEdits: 'Save edits',
- saveAsNewView: 'Save as new view',
+ editFilters: '编辑筛选条件',
+ saveEdits: '保存编辑',
+ saveAsNewView: '另存为新视图',
deleteSavedSearch: '删除已保存的搜索',
deleteSavedSearchConfirm: '确定要删除此搜索吗？',
searchName: '搜索名称',
Note You can apply these changes to your branch by copying the patch to your clipboard, then running |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02a3327616
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| * edits change the query we pass the old hash as previousHash and the backend overrides it, keeping a single LHN entry. | ||
| */ | ||
| function saveSavedViewEdits({queryJSON, name, previousHash}: {queryJSON: Readonly<SearchQueryJSON>; name: string; previousHash: number}) { | ||
| saveSearch({queryJSON, newName: name, previousHash}); |
There was a problem hiding this comment.
When the edited view was auto-named (name === original query) and the filters change, this always sends the original query string as newName. saveSearch then stores {name: oldQuery, query: newQuery}, and SavedSearchList only derives a readable title when item.name === item.query, so the saved view starts displaying the stale old query as a custom name after Save edits. Keep the auto-name case by using the edited query/default name when the original name matched the original query.
Useful? React with 👍 / 👎.
| <HeaderWithBackButton title={translate('search.filtersHeader')} /> | ||
| <HeaderWithBackButton | ||
| title={translate('search.filtersHeader')} | ||
| onBackButtonPress={isEditingSavedView ? cancelEdits : undefined} |
There was a problem hiding this comment.
On narrow screens this only wires the header back button to cancelEdits. If the fullscreen filters route is dismissed via Android hardware back, browser back, or an iOS swipe gesture, neither this handler nor the footer Cancel runs, leaving SEARCH_EDITING_SAVED_VIEW set; subsequent search navigation can keep the old saved view highlighted and reopen filters in edit mode. Add an unmount/beforeRemove cleanup for the edit state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is wrong. I tested. BUt I think it's harmless and safe to implement this
|
@FitseTLT I think this is ready. can you pls review?! thanks! |
|
What are the failings? |
|
@FitseTLT It's okay now |
|
@mukhrr can u pls give me an explanation on your recent changes? one the opening mechanism and the other pop state mechanism. What problems you are trying to fix, as some of the code looks a bit more complex and few timeout workarounds. |
| handleRef?: RefObject<FilterPopupButtonHandle | null>; | ||
|
|
||
| /** Called whenever the popover closes (click-outside, Cancel, Save or browser back) so the caller can leave any associated edit mode */ | ||
| onOverlayClose?: (reason?: 'browserBack') => void; |
There was a problem hiding this comment.
So we can replace reason with a boolean param right?
There was a problem hiding this comment.
Yes, it's binary (for now) — used a literal so the call site reads onOverlayClose?.('browserBack') instead of (true), and a new close reason wouldn't change the signature.
There was a problem hiding this comment.
want me update to boolean?
There was a problem hiding this comment.
We are fixing a specific case here, we don't know the future cases. It should definitely be an is... boolean
@FitseTLT Recent changes came after this bug #94547 (comment). Both fix the same root problem: opening the popup was driven by persisted state (onyx flag + Opening: each Edit filters click now leaves one consumable open request. Once the view's search is active, the button consumes it (returns true exactly once) and opens the popup through the same path as pressing the Filters button. A consumed request can't replay, so Popstate: browser back should close the popup, otherwise it stays open over a different search. We can't use The |
|
#94547 (comment) - on this, @FitseTLT can you just report it in the #expensify-bugs channel please? |
|
| // Each "Edit filters" click requests ONE open of the filters UI. Consumed exactly once (read-and-clear), so nothing | ||
| // can replay it on remounts, browser back or stale Onyx emissions; the TTL discards a request that never became openable. | ||
| let pendingOpenEditFiltersAt: number | undefined; | ||
| const PENDING_OPEN_EDIT_FILTERS_TTL_MS = 10000; |
There was a problem hiding this comment.
Do you really think this this is necessary? If we are consuming it why should we check with ttl have you faced any real problem
There was a problem hiding this comment.
hmm not actually, it was just defensive (a click whose navigation never lands could fire the open later when you visit the view manually).
There was a problem hiding this comment.
I think we are good to remove it. tested
|
lint failures @mukhrr |
|
@FitseTLT resolved |
|
@mukhrr your test steps are outdated and also fill all tests. |
|
@FitseTLT thanks for catching 👍 updated it |
|
how is 2 still correct? |
|
oh sorry 🙏 now it should be all good |
|
Discussing this w/ tom |
JS00001
changed the title
|
Discussed with @trjExpensify , and we'd like to hold these changes on this feature: https://github.com/Expensify/Expensify/issues/651173#issuecomment-4906136563 Basing this PR on those changes will drastically simplify the changes that need to be made here. We will also need to discuss changing the behavior here, based on those changes Those changes should get moving tomorrow, so shouldn't be too long edit: sorry, that issue is private, but I'll create an e/app issue for it tomorrow when we move to implementation |
|
Makes sense to me to simplify the implementation 👍 |
8 participants
Explanation of Change
Fixed Issues
$ #92770
PROPOSAL: #92770 (comment)
Tests
Preconditions:
Offline tests
QA Steps
The same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android_app.mp4
Android: mWeb Chrome
android_web.mp4
iOS: Native
ios_app.mp4
iOS: mWeb Safari
ios_web.mp4
MacOS: Chrome / Safari
REC-20260625182235.mp4