This repository was archived by the owner on Jul 30, 2026. It is now read-only.
refactor: extract handleBeforeSend keepAlive + popup closures for testability - #117
Merged
Merged
Conversation
…tability Closes #116. Lift two listener closures out from behind module-scoped singletons so the remaining 22 it.todo placeholders can drive real code instead of shape checks: - runBeforeSendEncryption() in src/background/encryption-flow.ts now owns the body of handleBeforeSend's keepAlive closure. background.ts passes the live browser.* / pg-js / popup deps in. - runEncryptInPopup() / runDecryptInPopup() / initCryptoPopup() in src/pages/yivi-popup/ own the popup's encrypt + decrypt + init/error flow. yivi-popup.ts collapses to ~60 lines of DOM + dep wire-up. - extractFromZip moved to src/pages/yivi-popup/zip.ts. Behaviour unchanged: composeTabs.has() guard, persistEncryptState, keepAlive wrap, fire-and-forget onUploadInit, and the attachment-size-cap arithmetic are all preserved on the call sites. Tests: 126 passed / 22 todo → 149 passed / 0 todo. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
rubenhensen
approved these changes
May 16, 2026
Contributor
Author
There was a problem hiding this comment.
Post-hoc review of the merged diff (the bot can't approve its own PR — recording findings as a comment).
Verified locally
npm test→ 149 passed / 0 todo (matches PR description; 22 previously-todo cases are now real)npm run typecheck→ cleannpm run build→ clean
Correctness
runBeforeSendEncryptionfaithfully reproduces the previouskeepAliveclosure body. Attachment removal still happens beforeopenCryptoPopup, the encrypted attachment is re-added only when ≤MAX_ATTACHMENT_SIZE, and any thrown error short-circuits to{ cancel: true }with nodetailsblock — so the plaintext-leak guard called out in #68 is preserved.handleBeforeSendkeeps all listener-site guards (composeTabs.has()race guard, persisted-state restore,keepAlive(\"onBeforeSend\", …)wrap, BCC / policy-editor early returns viaevaluateBeforeSendGuards). Afterif (guard || !state)every branch returns, so the proceed path is correctly narrowed.runEncryptInPopuppreserves the fire-and-forgetruntime.sendMessageinsideonUploadInit— a rejected send cannot abort the upload.initCryptoPopupreproduces the windowId-failure / no-pending-entry / encrypt-or-decrypt / error tail exactly, including theUploadSessionExpiredError-specific i18n message and the 750ms auto-close.runDecryptInPopupkeeps the uuid-mode unzip viaextractFromZip(postguard-js#39 workaround) and the data-mode path, with proper type narrowing on the result.
Test coverage
- The three failure-mode pins on
runBeforeSendEncryption(encryption-failure, popup-cancel, plaintext-leak guard) drive the function with a stubbedopenCryptoPopupthat throws and verify{ cancel: true }, nodetails, andnotifyError(\"encryptionError\"). Exactly the regressions a closure refactor could introduce silently. - Popup tests now cover init paths, encrypt mime decode + selector + windowId, decrypt uuid vs ciphertext branches, and error paths.
Style
- DI shape (
RunBeforeSendDeps,EncryptPopupDeps,DecryptPopupDeps,InitCryptoPopupDeps) is consistent across the four lifts and matches the pattern from #110–#115.
Minor / non-blocking
handleBeforeSend(tab, details: any)—details: anyis pre-existing, not introduced here.
The manual Thunderbird smoke-test items in the PR body still warrant attention before the next release tag, but the static surface and the lifted unit-test surface look clean. Approving in spirit.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #116.
Summary
handleBeforeSend'skeepAliveclosure intorunBeforeSendEncryption(state, details, tabId, deps)insrc/background/encryption-flow.ts.background.tspasses the livebrowser.*/ pg-js /openCryptoPopupsurfaces in.handleEncrypt/handleDecrypt/ the init+error flow intosrc/pages/yivi-popup/{encrypt-popup,decrypt-popup,init-flow}.ts.yivi-popup.tscollapses to a thin DOM + dep wire-up.extractFromZiptosrc/pages/yivi-popup/zip.ts(already pure).Test surface
npm test: 126 passed / 22 todo → 149 passed / 0 todo. The 22 placeholders #116 listed are now real tests:tests/encryption-flow.test.ts— encryption-failure, popup-cancel, and plaintext-leak guard now driverunBeforeSendEncryptionwith a stubbedopenCryptoPopupthat throws. Each pins{ cancel: true }, nodetailsblock,notifyError("encryptionError")fired.tests/crypto-popup.test.ts— init paths (windowId failure, no pending entry, PostGuard ctor call), encrypt paths (mime decode, Yivi element selector, attachment-size pass-through, windowId oncryptoPopupDone), decrypt paths (ciphertext decode, recipient, uuid-mode unzip), and error paths (cryptoPopupErrorsent, UI shown, no auto-close) now drive the extracted functions with stubbedpg+runtime.npm run typecheckandnpm run buildare both green.Preserved behaviour
This is the live production crypto path; the lift keeps every guard intact at the listener call site:
composeTabs.has()race guard (PR fix: prevent unencrypted send when encryption is enabled #68) — still inbackground.ts.keepAlive("onBeforeSend", ...)wrap — still inbackground.ts; the extracted function returns a plain promise.onUploadInitcallback inside pg-js's upload-stream start handler — still fire-and-forget so aruntime.sendMessagerejection cannot abort the upload.MAX_ATTACHMENT_SIZE) and theattachmentBase64 != nullshort-circuit — both preserved.Reviewer quickstart
Manual smoke-test recommended before merge
The bot ran unit tests + typecheck + production build, but does not have a Thunderbird in its workspace, so the encrypt and decrypt happy-paths were not click-tested live. Given this touches the production crypto pipeline (the closures inside
handleBeforeSend+handleEncrypt/handleDecrypt), a manual round-trip in Thunderbird against the staging PKG / Cryptify is recommended before merge — send an encrypted message and decrypt it on the other end. The state-lifecycle tests added in #110 pin the persistence invariants from #68, but the SDK call sites themselves are best exercised live.Test plan
npm test,npm run typecheck,npm run build)🤖 Generated with Claude Code