fix(fonts): address PR #190 code review findings#193
Merged
sharkAndshark merged 1 commit intofeat/font-service-resources-uifrom Mar 30, 2026
Merged
fix(fonts): address PR #190 code review findings#193sharkAndshark merged 1 commit intofeat/font-service-resources-uifrom
sharkAndshark merged 1 commit intofeat/font-service-resources-uifrom
Conversation
…red helpers, improve robustness - Use CURRENT_TIMESTAMP for font created_at and published_at instead of Rust-side Utc::now(), consistent with the rest of the codebase - Extract test-mode workspace bootstrap from font_handlers.rs and upload.rs into workspace::ensure_test_mode_workspace(), eliminating ~100 lines of duplicated code - Extract read_font_row() helper to deduplicate the 13-column FontItem row mapping used in list_fonts and get_font - Log DB errors in update_font_error instead of silently discarding them - Save uploaded font with original file extension (e.g. original.ttf) for easier filesystem inspection
3045ba7
into
feat/font-service-resources-ui
7 checks passed
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 code review findings from PR #190.
Fixes
Timestamp consistency — Use
CURRENT_TIMESTAMPforcreated_atandpublished_atin SQL instead of Rust-sideUtc::now().to_rfc3339(), matching the rest of the codebase and eliminating DuckDB implicit RFC3339 to TIMESTAMP parsing fragility.Deduplicated test-mode workspace bootstrap — Extract
workspace::ensure_test_mode_workspace()shared function, eliminating ~100 lines of identical bootstrap code duplicated betweenfont_handlers.rsandupload.rs.Deduplicated FontItem row mapping — Extract
read_font_row()helper for the 13-column row mapping previously duplicated inlist_fontsandget_font.Error logging —
update_font_errornow logs DB errors viawarn!instead of silently discarding them withlet _ =.Font file extension — Save uploaded fonts with their original extension (e.g.,
original.ttfinstead oforiginal) for easier filesystem inspection.Note on race condition (Item 1 from review)
The review flagged a TOCTOU race in
publish_fontbetween theSELECT EXISTScheck and theUPDATE. This race is already prevented by the application-levelMutex<Connection>- the lock is held continuously from the conflict check through the update. No additional fix needed.Test results