Cleanup findings from #100 review (reuse, simplification, efficiency)#103
Merged
Conversation
The abort threshold (MAX_SEQUENTIAL_BOOK_ERRORS = 100) was serialized into every job-status snapshot while the admin UI also hardcoded its own || 100 fallback. Remove the serialized field and render the error streak as a plain count; the constant stays where it is used (the parser abort). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
AutoTokenTypeTextEmbeddingTranslator eagerly built both the with- and without-token-type variants but uses only one, fixed once the model is inspected. Memoize the single needed delegate on first prepare() with volatile double-checked locking; the shared instance is preserved so the per-thread predictors that share this translator stay safe. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
buildRawEmail built the whole message in a StringBuilder, encoded the attachment to a full-size base64 String, then copied the lot again via toString().toByteArray() — roughly 4-5x the file size in memory per send. Write directly to a ByteArrayOutputStream and stream the attachment through Base64.getMimeEncoder().wrap(). Output bytes are unchanged; a new round-trip test guards the base64 body. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The email path hand-rolled rfc2231FileName and its own filename/filename* branching, duplicating what Ktor's ContentDisposition (already used by the download route) does. Add attachmentContentDisposition(fileName, includeAsciiFallback) in util and use it from both the download route (fallback + filename*) and buildRawEmail (extended-only for Kindle), and delete rfc2231FileName. The email disposition now renders in Ktor's RFC-compliant form (lowercase utf-8'', unquoted token filenames); the percent-encoded bytes and the extended-only-for-non-ASCII behavior are unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The Kindle path re-derived the attachment filename from the raw book title (sanitizeBookTitle + format) instead of reusing the canonical MaterializedBook.fileName that BookFileService already computed for HTTP download, so the two channels delivered divergent filenames and blank titles lost the book_<id> uniqueness. Pass the materialized filename through sendBookToKindle as the attachment name; the Subject still uses the human-readable title. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The code-point-safe truncation and the whitespace/underscore-collapse regexes were duplicated across BookFileService.sanitizedTitle, EmailService.sanitizeBookTitle, and util.asciiFallbackFileName. Extract takeCodePointSafe, WHITESPACE_RUN, and UNDERSCORE_RUN into util/TextSanitization.kt and reuse them. No output changes for any input. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Addresses the five cleanup findings from the #100 review that were deferred from the bug-fix PR (#101, now merged). Branch is off fresh
main, so it builds on those fixes. Each change was delegated to codex and reviewed; behavior-preserving refactors are guarded by the existing suite (with characterization tests added where coverage was thin), and behavioral changes have their own tests. Full./gradlew :testpasses.Changes (one commit each)
Drop duplicated
maxSequentialBookErrors— the abort threshold (100) was serialized into every job-status snapshot while the admin UI also hardcoded its own|| 100. Removed the serialized field; the "Error Streak" metric now shows a plain count. The constant stays where it is used (the parser abort).Build the embedding translator delegate lazily —
AutoTokenTypeTextEmbeddingTranslatoreagerly built both the with/without-token-type variants but uses only one, fixed once the model is inspected. Now memoizes the single needed delegate on firstprepare()with volatile double-checked locking. (Kept the shared-instance semantics because per-thread predictors share this one translator — a naive per-prepare()rebuild would have been a concurrency hazard.)Stream the base64 attachment —
buildRawEmailheld the attachment ~4-5× in memory (raw bytes + base64 String + StringBuilder + finaltoByteArray). It now writes straight to aByteArrayOutputStreamviaBase64.getMimeEncoder().wrap(...). Output bytes are unchanged; a new round-trip test guards the base64 body.Share one Ktor-based
Content-Dispositionhelper — the email path hand-rolledrfc2231FileNameand its ownfilename/filename*branching, duplicating what Ktor'sContentDisposition(already used by the download route) does. AddedattachmentContentDisposition(fileName, includeAsciiFallback)inutil, used by both the download route (fallback +filename*) andbuildRawEmail(extended-only for Kindle), and deletedrfc2231FileName.Content-Dispositionnow renders in Ktor's RFC-compliant form: lowercasefilename*=utf-8''…(wasUTF-8'') and unquoted token filenames (filename=Clean_Title.epubinstead of quoted). The percent-encoded bytes and the extended-only-for-non-ASCII behavior that Fix mojibake Kindle titles and other book-delivery improvements #100 validated for Kindle are unchanged; only the RFC-5987 charset case and token quoting differ (both standards-compliant). Two email test assertions were updated to match.Consolidate filename handling (finding gradle(deps): bump org.gradle.toolchains.foojay-resolver-convention from 0.10.0 to 1.0.0 #6, two commits):
MaterializedBook.fileName) and blank titles lost thebook_<id>uniqueness.sendBookToKindlenow takes the canonicalattachmentFileName; the worker passesmaterialized.fileName. The Subject still uses the human-readable title.takeCodePointSafe,WHITESPACE_RUN, andUNDERSCORE_RUNmoved intoutil/TextSanitization.ktand reused byBookFileService.sanitizedTitle,EmailService.sanitizeBookTitle, andasciiFallbackFileName(previously each had its own copy). No output changes for any input.Tests
EmailServiceTest), Kindle attachment-filename assertion (KindleSendWorkerTest).EmailServiceTestdisposition assertions for the Ktor format (change pip(deps): bump aiofiles from 23.2.1 to 24.1.0 in /conversion-service #4)../gradlew :testpasses.🤖 Generated with Claude Code