Skip to content

OUT-4012: warn before changing the bank-deposit flag - #272

Merged
SandipBajracharya merged 3 commits into
feature/payout-reconciliationfrom
OUT-4012
Jul 31, 2026
Merged

OUT-4012: warn before changing the bank-deposit flag#272
SandipBajracharya merged 3 commits into
feature/payout-reconciliationfrom
OUT-4012

Conversation

@SandipBajracharya

@SandipBajracharya SandipBajracharya commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Problem

Changing bankDepositFeeFlag has consequences users can't see: the deposit behavior is frozen per invoice at creation, so a toggle only affects new invoices, and a Stripe payout that mixes pre/post-change invoices won't get its automatic bank deposit and needs manual reconciliation.

Shipped

A confirmation modal on save (not on toggle), shown only when the flag differs from its saved value — covers both enable and disable, and never fires when unchanged. UX safeguard only; correctness lives in the frozen-intent + mixed-guard work (OUT-4010/4009).

Copy:

Change bank deposit setting?
This applies only to invoices created from now on; existing invoices are unaffected. If a Stripe payout mixes invoices from before and after the change, you'll need to reconcile that payout manually in QuickBooks.

Code sites

  • src/components/ui/ConfirmModal.tsx (new) — reusable portal modal (Escape/backdrop dismiss, role="dialog" + aria-labelledby/aria-describedby via useId).
  • src/hook/useSettings.tsuseInvoiceDetailSettings gains the save-intercept (requestInvoiceSettingsSave / confirmBankDepositChange / cancelBankDepositChange); raw submitInvoiceSettings no longer exported.
  • src/components/dashboard/settings/SettingAccordion.tsx — invoice save button routes through the guard and renders the modal. InvoiceDetail.tsx unchanged (checkbox stays a free local toggle).

Notes

  • Stacked on OUT-4011 (base branch), not master.
  • No component-test infra exists in the repo (both Vitest projects run environment: 'node'); verified via tsc, lint, prettier, yarn build, and yarn test (357/357).
  • Known follow-up: on a brand-new portal's first-time setup, enabling the flag trips the modal (saved value genuinely changed) — cosmetic, suppression deferred pending a product call.

Testing Criteria

image

🤖 Generated with Claude Code

@linear-code

linear-code Bot commented Jul 29, 2026

Copy link
Copy Markdown

OUT-4012

@vercel

vercel Bot commented Jul 29, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
quickbooks-sync Building Building Jul 31, 2026 10:29am
quickbooks-sync (dev) Ready Ready Preview Jul 31, 2026 10:29am

Request Review

@SandipBajracharya SandipBajracharya changed the title feat(OUT-4012): warn before changing the bank-deposit flag OUT-4012: warn before changing the bank-deposit flag Jul 29, 2026
Comment thread src/components/ui/ConfirmModal.tsx
@greptile-apps

greptile-apps Bot commented Jul 29, 2026

Copy link
Copy Markdown

Greptile Summary

Adds a confirmation safeguard when saving a changed bank-deposit setting.

  • Routes invoice-setting saves through a change-detection guard.
  • Adds a reusable accessible confirmation modal with Escape/backdrop dismissal, initial focus, keyboard focus containment, and focus restoration.
  • Connects confirmation and cancellation actions to the existing settings submission flow.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
src/components/ui/ConfirmModal.tsx Adds the modal and fully addresses the prior focus-management finding by moving focus inside, trapping keyboard navigation, and restoring prior focus on close.
src/components/dashboard/settings/SettingAccordion.tsx Routes invoice-setting saves through the warning guard and renders the confirmation modal.
src/hook/useSettings.ts Detects changes to the saved bank-deposit flag and coordinates request, confirmation, cancellation, and submission state.

Reviews (2): Last reviewed commit: "fix(OUT-4012): add focus management to t..." | Re-trigger Greptile

@SandipBajracharya

Copy link
Copy Markdown
Collaborator Author

@greptileai review PR again

Comment thread src/components/ui/ConfirmModal.tsx Outdated

@priosshrsth priosshrsth left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

lgtm

@SandipBajracharya
SandipBajracharya changed the base branch from OUT-4011 to feature/payout-reconciliation July 31, 2026 10:26
SandipBajracharya and others added 3 commits July 31, 2026 16:12
Show a confirmation modal when saving invoice settings that flip
bankDepositFeeFlag, so users acknowledge that the change applies only to
new invoices and that a payout mixing pre/post-change invoices may need
manual reconciliation. UX safeguard only; fires on save and only when the
flag differs from its saved value, both directions.

- add reusable ConfirmModal (portal, Escape/backdrop dismiss, a11y ids)
- gate the invoice save behind requestInvoiceSettingsSave in useSettings
- render the modal from SettingAccordion; InvoiceDetail toggle unchanged

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address Greptile P2: the aria-modal dialog left focus on the background
save button with no trap or restoration. On open, move focus into the
dialog, trap Tab/Shift+Tab between its buttons, and restore focus to the
previously focused element on close. Keying the effect on `open` via an
onCancel ref also stops it re-subscribing on every render.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the onCancelRef workaround with two focused effects: one keyed on
`open` for focus-in/restore (runs once), one keyed on `open`+`onCancel`
for the Escape + Tab-trap listener. Clearer, honest dependency arrays.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@SandipBajracharya
SandipBajracharya merged commit bbd57c4 into feature/payout-reconciliation Jul 31, 2026
4 checks passed
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