Repository navigation
Conversation
Co-authored-by: Nguyen Van Duc <dukenv0307@users.noreply.github.com>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
This comment has been minimized.
This comment has been minimized.
|
🤖 Android test steps verification results: Fixture setup remained incomplete; no approval-ready report with policy violations was obtained. The approval behavior remains unverified—not failed.
|
|
🤖 Web test steps verification results: Blocked on Web: Expenses showed “No expenses yet” and the unfiltered Reports view showed “No reports yet.” Approval behavior remains unverified because the required reports were unavailable.
|
Co-authored-by: Nguyen Van Duc <dukenv0307@users.noreply.github.com>
|
Pushed c5fa640 to fix:
Validation: 93 focused tests passed, plus lint, typecheck, spelling, and Storybook smoke tests. The full Jest run timed out locally. New CI checks are pending; the independent-approval gate still requires human review. |
|
|
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@MelvinBot In TransactionListItem, useConfirmApproveReportViolations isn't passed the transactions/violationsCollection overrides. Pls evalute |
|
The omission is real, but it does not currently cause a reachable approval-warning bug in this row.
Recommendation: Treat passing snapshot-aware overrides as defensive hardening, not a demonstrated bug fix. Add a regression test ensuring transaction rows remain View-only when their parent report is approvable. Code inspection only; no changes made or UI reproduction claimed. |
|
@MelvinBot Ah ok, but useConfirmApproveReportViolations without overrides rebuilds the whole report's violations per transaction row for an action that can never fire -> wasted work |
|
Agreed—you’re right about the wasted work. My previous response addressed correctness but missed the render cost. Each row calls the hook, which creates a fresh map and filters the report’s locally cached violations on every render ( The better fix is to remove the unused approval hook and callback wiring from the transaction row, rather than add overrides for an unreachable action. Passing No changes made; runtime impact has not been measured. |
Here are the steps: Precondition: a WS approver, 2 submitted report with at least 1 violation and 1 submitted report with no violations Steps:
Screen.Recording.2026-11-07.at.10.50.42.movScreen.Recording.2026-11-07.at.10.56.03.mov
|
Co-authored-by: Nguyen Van Duc <dukenv0307@users.noreply.github.com>
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
🤖 Web test steps verification results: ❌ fail Pushed 05fa6616 removing the unused hook, added your steps/videos, checked evidence-backed checklist items, and marked the PR ready; 19 focused tests, lint, typecheck, spelling, and Storybook passed, while full Jest timed out and compiler/formatting/translation checks remained blocked. Web testing encountered a blank page, so approval behavior and the remaining checklist items remain unverified—not demonstrated product failures.
|
|
@Julesssss Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
![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: 05fa6616eb
ℹ️ 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".
| const isASAPSubmitBetaEnabled = isBetaEnabled(CONST.BETAS.ASAP_SUBMIT); | ||
| const hasViolations = hasViolationsReportUtils(moneyRequestReport?.reportID, transactionViolations, accountID, email ?? '', undefined, transactions); | ||
| const isAnyTransactionOnHold = hasHeldExpensesReportUtils(transactions); | ||
| const confirmApproveReportViolations = useConfirmApproveReportViolations({report: moneyRequestReport, policy, transactions, violationsCollection: transactionViolations}); |
There was a problem hiding this comment.
Pass the user-visible, filtered violation collection to the confirmation instead of the raw Onyx entries. When a violation has been dismissed through transaction.comment.dismissedViolations, getReportSubmitViolationSummary() only applies shouldShowViolation() and therefore adds the dismissed violation to the modal, even though useTransactionsAndViolationsForReport() normally removes it. This makes the report-header and Search approval buttons repeatedly warn about violations the user has already dismissed; the hold-menu and bulk-approval paths similarly pass raw collections and need the same filtering.
Useful? React with 👍 / 👎.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Co-authored-by: Nguyen Van Duc <dukenv0307@users.noreply.github.com>
|
@abdulrahuman5196 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Co-authored-by: Nguyen Van Duc <dukenv0307@users.noreply.github.com>
|
@dukenv0307 Do we need me as C+ reviewer here? |
|
@abdulrahuman5196 yes pls do |
Little clarification here? So you are the Author creating PR with MelvinBot? I don't have access the issue we are fixing as well. |
Yes
@abdulrahuman5196 You were assigned as C+ here so you should review it. I'll review it again and ask Melvin to update if needed |
|
@abdulrahuman5196 any updates? |
|
@abdulrahuman5196 I guess you haven't started to review yet. After discussion, I think I can review this PR created by Melvin. Sorry for this confusion 🙏 |
|
@MelvinBot pls merge main + resolve conflict |
Co-authored-by: Nguyen Van Duc <dukenv0307@users.noreply.github.com>
|
🤖 Web test steps verification results: ❌ fail Merged
view run · no recording available |
🦜 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 b5307bd7..b9b97d99 100644
--- a/src/languages/de.ts
+++ b/src/languages/de.ts
@@ -1539,7 +1539,7 @@ const translations: TranslationDeepObject<typeof en> = {
markAsCash: 'Als Bar markieren',
confirmApproveReportViolations: {
title: 'Trotzdem genehmigen?',
- description: 'Dieser Bericht enthält Richtlinienverstöße. Möchtest du ihn wirklich ohne Änderungen genehmigen?',
+ description: 'Dieser Bericht enthält Richtlinienverstöße. Sind Sie sicher, dass Sie ihn ohne Änderungen genehmigen möchten?',
},
confirmSubmitReportViolations: {
title: 'Bericht einreichen?',
diff --git a/src/languages/el.ts b/src/languages/el.ts
index 093ca1fd..07856fc5 100644
--- a/src/languages/el.ts
+++ b/src/languages/el.ts
@@ -1585,7 +1585,7 @@ const translations: TranslationDeepObject<typeof en> = {
markAsCash: 'Σήμανση ως μετρητά',
confirmApproveReportViolations: {
title: 'Έγκριση ούτως ή άλλως;',
- description: 'Αυτή η αναφορά έχει παραβιάσεις πολιτικής. Θέλετε σίγουρα να την εγκρίνετε χωρίς αλλαγές;',
+ description: 'Αυτή η αναφορά έχει παραβιάσεις πολιτικής. Είστε βέβαιοι ότι θέλετε να την εγκρίνετε χωρίς να κάνετε αλλαγές;',
},
confirmSubmitReportViolations: {
title: 'Υποβολή αναφοράς;',
diff --git a/src/languages/es.ts b/src/languages/es.ts
index d97d8caa..66e14a5b 100644
--- a/src/languages/es.ts
+++ b/src/languages/es.ts
@@ -1511,10 +1511,7 @@ const translations: TranslationDeepObject<typeof en> = {
pendingMatchWithCreditCard: 'Recibo pendiente de adjuntar con la transacción de la tarjeta',
pendingMatchWithCreditCardDescription: 'Recibo pendiente de adjuntar con la transacción de la tarjeta. Márcalo como efectivo para cancelar.',
markAsCash: 'Marcar como efectivo',
- confirmApproveReportViolations: {
- title: '¿Aprobar de todos modos?',
- description: 'Este informe tiene infracciones de la política. ¿Seguro que quieres aprobarlo sin realizar cambios?',
- },
+ confirmApproveReportViolations: {title: '¿Aprobar de todas formas?', description: 'Este informe tiene infracciones de la política. ¿Seguro que quieres aprobarlo sin hacer cambios?'},
confirmSubmitReportViolations: {
title: '¿Enviar informe?',
description: 'Este informe tiene infracciones de política. ¿Seguro que quieres enviarlo sin hacer cambios?',
diff --git a/src/languages/fr.ts b/src/languages/fr.ts
index ec6aa169..1bb98cb1 100644
--- a/src/languages/fr.ts
+++ b/src/languages/fr.ts
@@ -1547,7 +1547,7 @@ const translations: TranslationDeepObject<typeof en> = {
markAsCash: 'Marquer comme espèces',
confirmApproveReportViolations: {
title: 'Approuver quand même ?',
- description: 'Ce rapport contient des violations de la politique. Voulez-vous vraiment approuver ce rapport sans le modifier ?',
+ description: 'Cette note de frais comporte des violations de politique. Voulez-vous vraiment l’approuver sans y apporter de modifications ?',
},
confirmSubmitReportViolations: {
title: 'Soumettre le rapport ?',
diff --git a/src/languages/it.ts b/src/languages/it.ts
index 63961583..dff745c8 100644
--- a/src/languages/it.ts
+++ b/src/languages/it.ts
@@ -1536,10 +1536,7 @@ const translations: TranslationDeepObject<typeof en> = {
pendingMatch: 'Corrispondenza in sospeso',
pendingMatchWithCreditCardDescription: 'Ricevuta in attesa di abbinamento con la transazione della carta. Contrassegna come contante per annullare.',
markAsCash: 'Segna come contante',
- confirmApproveReportViolations: {
- title: 'Approvare comunque?',
- description: 'Questo report presenta violazioni delle regole. Vuoi davvero approvarlo senza apportare modifiche?',
- },
+ confirmApproveReportViolations: {title: 'Approvi comunque?', description: 'Questo report presenta violazioni della policy. Vuoi davvero approvarlo senza apportare modifiche?'},
confirmSubmitReportViolations: {
title: 'Invia il rapporto?',
description: 'Questo rapporto presenta violazioni delle norme. Sei sicuro di volerlo inviare senza apportare modifiche?',
diff --git a/src/languages/ja.ts b/src/languages/ja.ts
index d6b181f2..48ec73b9 100644
--- a/src/languages/ja.ts
+++ b/src/languages/ja.ts
@@ -103,7 +103,7 @@ const translations: TranslationDeepObject<typeof en> = {
saveChanges: '変更を保存',
submit: '送信',
submitAnyway: 'とにかく送信',
- approveAnyway: 'それでも承認',
+ approveAnyway: 'このまま承認する',
markAsDone: '完了にする',
markAsDoneAnyway: 'とにかく完了にする',
submitted: '送信済み',
@@ -1519,10 +1519,7 @@ const translations: TranslationDeepObject<typeof en> = {
pendingMatch: '保留中の照合',
pendingMatchWithCreditCardDescription: 'レシートはカード取引との照合待ちです。現金としてマークしてキャンセルします。',
markAsCash: '現金としてマーク',
- confirmApproveReportViolations: {
- title: 'それでも承認しますか？',
- description: 'このレポートにはポリシー違反があります。変更せずに承認してもよろしいですか？',
- },
+ confirmApproveReportViolations: {title: 'それでも承認しますか？', description: 'このレポートにはポリシー違反があります。変更を行わずに承認してもよろしいですか？'},
confirmSubmitReportViolations: {
title: 'レポートを送信しますか？',
description: 'このレポートにはポリシー違反があります。変更を加えずに送信してもよろしいですか？',
diff --git a/src/languages/nl.ts b/src/languages/nl.ts
index c7181797..f4772584 100644
--- a/src/languages/nl.ts
+++ b/src/languages/nl.ts
@@ -1536,7 +1536,7 @@ const translations: TranslationDeepObject<typeof en> = {
markAsCash: 'Markeren als contant',
confirmApproveReportViolations: {
title: 'Toch goedkeuren?',
- description: 'Dit rapport bevat beleidsovertredingen. Weet je zeker dat je het zonder wijzigingen wilt goedkeuren?',
+ description: 'Dit rapport bevat beleidsinbreuken. Weet je zeker dat je het wilt goedkeuren zonder wijzigingen aan te brengen?',
},
confirmSubmitReportViolations: {
title: 'Rapport verzenden?',
diff --git a/src/languages/pl.ts b/src/languages/pl.ts
index 020a0123..abbad253 100644
--- a/src/languages/pl.ts
+++ b/src/languages/pl.ts
@@ -1574,10 +1574,7 @@ const translations: TranslationDeepObject<typeof en> = {
pendingMatch: 'Oczekujące dopasowanie',
pendingMatchWithCreditCardDescription: 'Oczekuje na dopasowanie paragonu do transakcji kartą. Oznacz jako gotówkę, aby anulować.',
markAsCash: 'Oznacz jako gotówkę',
- confirmApproveReportViolations: {
- title: 'Zatwierdzić mimo to?',
- description: 'Ten raport zawiera naruszenia zasad. Czy na pewno chcesz go zatwierdzić bez wprowadzania zmian?',
- },
+ confirmApproveReportViolations: {title: 'Zatwierdzić mimo to?', description: 'Ten raport zawiera naruszenia zasad. Na pewno chcesz go zatwierdzić bez wprowadzania zmian?'},
confirmSubmitReportViolations: {
title: 'Wysłać raport?',
description: 'Ten raport zawiera naruszenia zasad. Czy na pewno chcesz go wysłać bez wprowadzania zmian?',
diff --git a/src/languages/pt-BR.ts b/src/languages/pt-BR.ts
index c067baf4..a003ab72 100644
--- a/src/languages/pt-BR.ts
+++ b/src/languages/pt-BR.ts
@@ -1536,10 +1536,7 @@ const translations: TranslationDeepObject<typeof en> = {
pendingMatch: 'Correspondência pendente',
pendingMatchWithCreditCardDescription: 'Recibo aguardando correspondência com transação do cartão. Marque como dinheiro para cancelar.',
markAsCash: 'Marcar como dinheiro',
- confirmApproveReportViolations: {
- title: 'Aprovar mesmo assim?',
- description: 'Este relatório tem violações da política. Tem certeza de que deseja aprová-lo sem fazer alterações?',
- },
+ confirmApproveReportViolations: {title: 'Aprovar mesmo assim?', description: 'Este relatório tem violações da política. Tem certeza de que quer aprová‑lo sem fazer alterações?'},
confirmSubmitReportViolations: {
title: 'Enviar relatório?',
description: 'Este relatório tem violações de política. Tem certeza de que deseja enviá-lo sem fazer alterações?',
diff --git a/src/languages/zh-hans.ts b/src/languages/zh-hans.ts
index dd8eab37..047d5e96 100644
--- a/src/languages/zh-hans.ts
+++ b/src/languages/zh-hans.ts
@@ -1472,10 +1472,7 @@ const translations: TranslationDeepObject<typeof en> = {
pendingMatch: '待匹配',
pendingMatchWithCreditCardDescription: '收据正在等待与卡片交易匹配。将其标记为现金以取消。',
markAsCash: '标记为现金',
- confirmApproveReportViolations: {
- title: '仍然批准？',
- description: '此报告存在违反政策的情况。确定要在不作更改的情况下批准吗？',
- },
+ confirmApproveReportViolations: {title: '仍然批准？', description: '此报表存在策略违规。确定要在不进行任何更改的情况下批准吗？'},
confirmSubmitReportViolations: {
title: '提交报告？',
description: '此报告存在违反政策的情况。您确定要不做任何更改就提交吗？',
Note You can apply these changes to your branch by copying the patch to your clipboard, then running |
|
Co-authored-by: Nguyen Van Duc <dukenv0307@users.noreply.github.com>
|
🤖 Web test steps verification results: ❌ fail Pushed 57a6c9d4: fixed the six failing tests, added confirmation/cancellation coverage, and applied all ten locale translations; 11 focused tests, lint, typecheck, spelling, and Storybook passed. Checklist boxes were marked at your direction with an explicit verification caveat; full Jest timed out, and manual Web testing remained blocked by a blank page.
view run · no recording available |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-11-07.at.10.50.42.movScreen.Recording.2026-11-07.at.10.56.03.mov |
|
@Julesssss All yours! |
| const confirmApproveReportViolations = useConfirmApproveReportViolations({ | ||
| report: reportForViolations, | ||
| policy: policyForViolations, | ||
| transactions: liveReportTransactions, |
There was a problem hiding this comment.
The row and 'Approve anyway?' modal can be different. If a report hasn't been opened yet, its expenses only come from the Search results, not from what's loaded locally. The row still shows the violation, but the modal only checks the loaded expenses, finds none, and approves straight away with no warning.
ReportListItemHeader has the same issue. Bulk approve already checks both, so could we do the same here and add a test for that case?
There was a problem hiding this comment.
🤖 Web test steps verification results: ❌ fail
Merged latest main and pushed the snapshot-warning fix in 4860181d, covering report rows, grouped headers, wide-layout approval, and bulk approval; 162 focused tests and static checks passed. Full Jest timed out, and browser verification was blocked by an unavailable Web session.
| Step | Status |
|---|---|
Snapshot-only report row warns before approvalBrowser blocked; regression tests passed. |
|
Grouped report header warns before approvalBrowser blocked; regression tests passed. |
|
Cancel preserves approval state and selectionBrowser blocked; cancellation tests passed. |
|
Wide and bulk approval warn consistentlyBrowser blocked; regression tests passed. |
view run · no recording available
Co-authored-by: Nguyen Van Duc <dukenv0307@users.noreply.github.com>
Co-authored-by: Nguyen Van Duc <dukenv0307@users.noreply.github.com>
|
@MelvinBot fix the failing test |
|
The failing check is React Compiler Compliance, not Jest—all test jobs passed. I reproduced the cache regressions, but couldn’t fully resolve them safely: one selector still lost its cache guard after the attempted fixes. I reverted those attempts; no changes were pushed. |
|
@MelvinBot Is it related to this PR? |
|
Yes—it’s related to this PR, not an unrelated/flaky test. The compiler check confirms that computations cached on |
|
@MelvinBot New inline computes (warningData = getReportApprovalWarningData(...), and the [...new Set(...)] transaction-ID union) feed hook calls mid-render. React Compiler memo scopes can't span a hook, so it dropped caching on those + upstream computes (getNonEmptyStringOnyxID, isInvoiceReport). Solution: Moved the computations into their own hooks (useReportApprovalWarningData, useLiveViolationsForSnapshotTransactions) so call sites chain hook results instead. Behavior unchanged; compliance check + lint + affected tests pass. Pls evaluate and fix |
Co-authored-by: Nguyen Van Duc <dukenv0307@users.noreply.github.com>
Explanation of Change
Approvers can currently approve reports without noticing their policy violations. Reuse the submit warning's modal and violation bullets to ask Approve anyway? before approval proceeds.
AI Tests
Latest compiler-cache fix (2026-11-13):
Latest Search snapshot fix and main merge (2026-11-13):
Latest test-fixture and translation update (2026-11-12):
Latest main-merge validation (2026-11-12):
Latest dismissed-violation fix validation (2026-11-07):
Latest formatting-fix validation (2026-11-07):
Latest review follow-up validation (2026-11-07):
Latest CI-fix validation (2026-11-07):
prettiernpm script is absent. Direct Oxfmt invocation was blocked by runner permissions, so formatting validation is left to CI.Original implementation validation:
@react-navigation/coreas an ES module.Contributor branch access
This PR uses an upstream branch because Melvin cannot push to the contributor's fork. To copy the branch into your fork:
You can then open a replacement PR from your fork and close this draft.
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/696578
Tests
Manual test plan supplied by
dukenv0307. These are expected outcomes, not a claim that Melvin completed them; the latest Web attempt was blocked by a blank application page.Precondition: signed in as a workspace approver, with two submitted reports containing at least one violation each and one submitted report without violations. Restore or add reports as needed before the preview and bulk cases.
Offline tests
Not verified. Load a submitted violation report while online, disconnect, then click Approve. Verify cached violations appear in the confirmation; Cancel must leave the report unapproved. Confirm approval and reconnect to verify synchronization without duplicate approval.
QA Steps
Same as Tests, using QA-owned submitted reports on staging. Verify the rendered title against approved product copy. Also cover held expenses/full versus partial approval and the offline test above. These QA outcomes remain unverified by Melvin.
PR Author Checklist
Checkboxes below were marked on
dukenv0307's behalf at their request, not as test attestations. Cross-platform, offline, accessibility, console, and manual approval-flow verification remains unverified by Melvin; see AI Tests for the checks actually executed. The attached videos are requester-provided and were not independently reviewed.### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)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.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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Videos supplied by
dukenv0307(not independently reviewed by Melvin):Screen.Recording.2026-11-07.at.10.50.42.mov
Screen.Recording.2026-11-07.at.10.56.03.mov
Latest automated Web attempt: blank application page blocked approval verification.
Latest dismissed-violation verification attempt: blank Web session prevented access to report-header, Search/bulk, held-expense, and submitter-dismissal test cases.
Post-merge Web verification (2026-11-12): the blank session prevented access to the approval test fixtures.
Latest test-fixture/translation Web attempt: a blank page prevented manual approval verification.