Repository navigation
fix review findings from lui extension replacement - #32
Conversation
- decode_picked: tolerate extensionless picks via file_extension fallback\n- stage_copy: stream the temp copy in 64KiB chunks instead of reading the whole file into memory\n- staged_completion: only echo completion for request tokens that already produced a staged pick, so a newly armed picker request isn't prematurely completed\n- clear_graph_surface: drop media_preview so a previous graph's preview can't persist across a graph switch\n- journal_media_view: restore tif/avif to the inline image list (parity with the retired extension)
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15f478b6c2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let ic = open_in_bin path in | ||
| let length = in_channel_length ic in | ||
| let contents = really_input_string ic length in | ||
| close_in ic; | ||
| let oc = open_out_bin dest in |
There was a problem hiding this comment.
If open_out_bin dest raises—for example because the temporary directory is full, unwritable, or the process has reached its descriptor limit—the outer handler returns Error without closing the already-open ic. Repeated attachment attempts under that condition leak one source-file descriptor each and can eventually exhaust descriptors for the entire app; wrap both channels in exception-safe cleanup so failures at destination creation and final close cannot leak handles or partial files.
Useful? React with 👍 / 👎.
1 participant
Summary
Address the 5 Devin Review findings on #31 (
e8751e7), applied on top of the merged content ondevin/1790580543-composer-assets.decode_picked:String.sub "" 1 0on extensionless filenames raised and silently discarded the pick → derivefile_typethrough the existingfile_extensionhelper ("bin"fallback) moved above the decoder.stage_copy:really_input_stringread the entire pick into one string → stream copy in 64 KiB chunks; on failure the partial temp file is removed.staged_completion: echoed the armed request token wheneverpending <> [], so a newly armed pick request was completed before its pick could land → trackspent_requests(armed tokens that produced an accepted pick) and only echo completion for those.clear_graph_surface:media_previewsurvived graph switches, leaving the prior graph's QuickLook preview mounted → reset it with the rest of the graph surface.journal_media_view.is_image_type:tif/avifwere dropped versus the retired extension'sisImage(JournalMedia.swift) → restored;file_image's ImageIO thumbnail path decodes both.dune build/dune runtestgreen except the pre-existingV.progressboundary failure (identical on main).Link to Devin session: https://app.devin.ai/sessions/55c3d31f1c6d4ee4beda2f832725d7b7
Open in Devin Desktop: https://app.devin.ai/desktop/session/55c3d31f1c6d4ee4beda2f832725d7b7?variant=devin
Requested by: @RCmerci