From 2879f6ed1717663d6cd627936effb79ba8c621c8 Mon Sep 17 00:00:00 2001 From: Jakob Wennberg <149234542+jakobwennberg@users.noreply.github.com> Date: Thu, 21 May 2026 14:00:49 +0200 Subject: [PATCH] fix(inbox): sync matched_transaction_id on MCP-staged document attach (#549) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(inbox): sync matched_transaction_id on MCP-staged document attach The REST attach route (app/api/transactions/[id]/attach-document) already updates invoice_inbox_items.matched_transaction_id so the inbox list shows the "Kopplad till transaktion" indicator. The MCP-staged path through commitAttachDocumentToTransaction did not, leaving inbox rows looking unprocessed after a /pending approval. Mirror the REST behaviour with a best-effort UPDATE (idempotent, gated on both FK columns being NULL). Co-Authored-By: Claude Opus 4.7 (1M context) * fix(review): inspect supabase error on best-effort inbox link The Supabase client resolves with { error } rather than rejecting on RLS/DB failures, so the previous try/catch wrapping the inbox UPDATE never fired for the documented failure modes — the console.error was dead code. Switch to destructured { error } + if-block, matching the pattern used elsewhere in commit.ts. Applied to both the new MCP-staged path (lib/pending-operations/commit.ts) and the pre-existing REST route (app/api/transactions/[id]/attach-document/ route.ts) it was mirrored from. Both tests now assert console.error fires with the expected payload, so the swallow-and-log path is exercised for real instead of passing for the wrong reason. Addresses Greptile P1 + P2 on PR #549. Co-Authored-By: Claude Opus 4.7 (1M context) --------- Co-authored-by: Claude Opus 4.7 (1M context) --- .../attach-document/__tests__/route.test.ts | 7 +++ .../[id]/attach-document/route.ts | 22 ++++---- .../__tests__/executors.test.ts | 53 +++++++++++++++++++ lib/pending-operations/commit.ts | 19 +++++++ 4 files changed, 91 insertions(+), 10 deletions(-) diff --git a/app/api/transactions/[id]/attach-document/__tests__/route.test.ts b/app/api/transactions/[id]/attach-document/__tests__/route.test.ts index c0cbdf9b..1bff87ea 100644 --- a/app/api/transactions/[id]/attach-document/__tests__/route.test.ts +++ b/app/api/transactions/[id]/attach-document/__tests__/route.test.ts @@ -126,6 +126,13 @@ describe('POST /api/transactions/[id]/attach-document', () => { // Side-effect failure must not roll back the (compliant) document attach. expect(status).toBe(200) expect(body.data.transaction_id).toBe('tx-1') + // The Supabase client resolves with { error } rather than rejecting, so + // we additionally assert that the error was actually inspected and logged + // (not silently dropped by a try/catch that never fires). + expect(spy).toHaveBeenCalledWith( + '[attach-document] Failed to link inbox item:', + expect.objectContaining({ message: 'rls denied' }), + ) spy.mockRestore() }) }) diff --git a/app/api/transactions/[id]/attach-document/route.ts b/app/api/transactions/[id]/attach-document/route.ts index f2d41313..1bd75d51 100644 --- a/app/api/transactions/[id]/attach-document/route.ts +++ b/app/api/transactions/[id]/attach-document/route.ts @@ -87,16 +87,18 @@ export async function POST( // as matched so the inbox UI can show it as "Kopplad" + link back to the // transaction. Best-effort: a failure here must not roll back the // (compliant) document attach. - try { - await supabase - .from('invoice_inbox_items') - .update({ matched_transaction_id: transactionId }) - .eq('document_id', document_id) - .eq('company_id', companyId) - .is('matched_transaction_id', null) - .is('created_supplier_invoice_id', null) - } catch (linkErr) { - console.error('[attach-document] Failed to link inbox item:', linkErr) + // + // The Supabase client resolves with { error } rather than rejecting on + // RLS/DB errors, so we destructure rather than try/catch. + const { error: inboxLinkErr } = await supabase + .from('invoice_inbox_items') + .update({ matched_transaction_id: transactionId }) + .eq('document_id', document_id) + .eq('company_id', companyId) + .is('matched_transaction_id', null) + .is('created_supplier_invoice_id', null) + if (inboxLinkErr) { + console.error('[attach-document] Failed to link inbox item:', inboxLinkErr) } // Rättelse audit trail (BFL 5 kap 5 §): record swaps where a non-null doc diff --git a/lib/pending-operations/__tests__/executors.test.ts b/lib/pending-operations/__tests__/executors.test.ts index f4a9c453..6db1a934 100644 --- a/lib/pending-operations/__tests__/executors.test.ts +++ b/lib/pending-operations/__tests__/executors.test.ts @@ -493,6 +493,7 @@ describe('commitPendingOperation: attach_document_to_transaction', () => { enqueue({ data: { id: 'tx-1', document_id: null, journal_entry_id: null }, error: null }) enqueue({ data: { id: 'doc-1' }, error: null }) // doc fetch enqueue({ data: { journal_entry_id: null }, error: null }) // UPDATE returning + enqueue({ data: null, error: null }) // invoice_inbox_items best-effort link enqueue({ data: null, error: null }) // dispatcher commit update const result = await commitPendingOperation( @@ -510,6 +511,7 @@ describe('commitPendingOperation: attach_document_to_transaction', () => { enqueue({ data: { id: 'tx-1', document_id: null, journal_entry_id: null }, error: null }) enqueue({ data: { id: 'doc-1' }, error: null }) // doc fetch enqueue({ data: { journal_entry_id: 'je-7' }, error: null }) // UPDATE returning post-state + enqueue({ data: null, error: null }) // invoice_inbox_items best-effort link enqueue({ data: null, error: null }) // doc propagation update enqueue({ data: null, error: null }) // dispatcher commit update @@ -521,4 +523,55 @@ describe('commitPendingOperation: attach_document_to_transaction', () => { ) expect(result.status).toBe('committed') }) + + it('still commits when the inbox-link best-effort update errors', async () => { + // Inbox sync is best-effort — a failure to mark the inbox row as matched + // must not roll back the (compliant) doc→tx attach. Mirrors the REST + // route's swallow-and-log behaviour. The Supabase client resolves with + // { error } rather than rejecting, so we both confirm the op commits AND + // that the error was actually inspected and logged (not silently dropped). + const { supabase, enqueue } = createQueuedMockSupabase() + enqueue({ data: { id: 'op-1' }, error: null }) // CAS claim + enqueue({ data: { id: 'tx-1', document_id: null, journal_entry_id: null }, error: null }) + enqueue({ data: { id: 'doc-1' }, error: null }) // doc fetch + enqueue({ data: { journal_entry_id: null }, error: null }) // tx UPDATE returning + enqueue({ data: null, error: { message: 'inbox row missing or RLS-blocked' } }) // inbox link — errors + enqueue({ data: null, error: null }) // dispatcher commit update + + const spy = vi.spyOn(console, 'error').mockImplementation(() => {}) + const result = await commitPendingOperation( + supabase as never, + 'user-1', + 'company-1', + makePendingOp(baseOp), + ) + expect(result.status).toBe('committed') + expect(spy).toHaveBeenCalledWith( + '[commitAttach] Failed to link inbox item:', + expect.objectContaining({ message: 'inbox row missing or RLS-blocked' }), + ) + spy.mockRestore() + }) + + it('touches the invoice_inbox_items table to sync matched_transaction_id', async () => { + // Argument-level check that the executor actually reaches into + // invoice_inbox_items to keep the inbox UI in sync with the staged-write + // path. Otherwise the inbox row stays in "Behöver åtgärd" forever. + const { supabase, enqueue } = createQueuedMockSupabase() + enqueue({ data: { id: 'op-1' }, error: null }) // CAS claim + enqueue({ data: { id: 'tx-1', document_id: null, journal_entry_id: null }, error: null }) + enqueue({ data: { id: 'doc-1' }, error: null }) // doc fetch + enqueue({ data: { journal_entry_id: null }, error: null }) // tx UPDATE returning + enqueue({ data: null, error: null }) // invoice_inbox_items link + enqueue({ data: null, error: null }) // dispatcher commit update + + await commitPendingOperation( + supabase as never, + 'user-1', + 'company-1', + makePendingOp(baseOp), + ) + const tablesTouched = (supabase.from as ReturnType).mock.calls.map(c => c[0]) + expect(tablesTouched).toContain('invoice_inbox_items') + }) }) diff --git a/lib/pending-operations/commit.ts b/lib/pending-operations/commit.ts index 00af5168..e22a2ac5 100644 --- a/lib/pending-operations/commit.ts +++ b/lib/pending-operations/commit.ts @@ -1059,6 +1059,25 @@ async function commitAttachDocumentToTransaction( } if (!postUpdate) return { error: 'Transaction not found', status: 404 } + // If the attached doc came from an invoice_inbox_items row, mark that row + // as matched so the inbox UI shows "Kopplad till transaktion". Best-effort: + // a failure must not roll back the (compliant) attach. Mirrors the REST + // route in app/api/transactions/[id]/attach-document/route.ts so MCP-staged + // and REST attaches converge on the same inbox state. + // + // The Supabase client resolves with { error } rather than rejecting on + // RLS/DB errors, so we destructure rather than try/catch. + const { error: inboxLinkErr } = await supabase + .from('invoice_inbox_items') + .update({ matched_transaction_id: txId }) + .eq('document_id', documentId) + .eq('company_id', companyId) + .is('matched_transaction_id', null) + .is('created_supplier_invoice_id', null) + if (inboxLinkErr) { + console.error('[commitAttach] Failed to link inbox item:', inboxLinkErr) + } + const journalEntryId = postUpdate.journal_entry_id as string | null if (journalEntryId) { const { error: linkErr } = await supabase