Migrate preview rendering and parsers to Rust - #14
Conversation
|
Warning Review limit reached
Next review available in: 27 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (27)
WalkthroughThis change replaces Go and Swift preview parsers with a Rust ChangesPreviewCore migration
Estimated code review effort: 5 (Critical) | ~120 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: b718e743f4
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
This PR migrates Quick Look preview rendering and untrusted parsing (Markdown/code/Jupyter + TSV/ZIP/TAR/7z metadata) from the prior Go + Swift package stack to a bounded Rust static “PreviewCore”, and updates the Swift host to prepare previews off the main actor with cancellation-aware execution.
Changes:
- Introduces
PreviewCore(Rust staticlib + C header/modulemap) providing rendering and bounded archive/TSV parsing via an FFI JSON payload interface. - Refactors Swift preview implementations to
asyncand runs parsing/rendering/directory enumeration off-main using a sharedPreviewExecutor. - Updates build/test/release tooling (mise tasks, Xcode build phase, CI workflows) and docs/credits/release notes to reflect the new Rust core and removed Go/Swift parser dependencies.
Reviewed changes
Copilot reviewed 50 out of 55 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| RELEASE_NOTES.md | Adds 1.6.0 release notes describing Rust preview core migration and constraints. |
| README.md | Updates version/build metadata and rendering/parser implementation notes to Rust core. |
| QLPlugin/Views/PreviewVC.swift | Makes the Preview creation API async for off-main preparation. |
| QLPlugin/Views/Previews/ZIPPreview.swift | Switches ZIP metadata scanning to Rust core + Swift tree adapter. |
| QLPlugin/Views/Previews/TSVPreview.swift | Switches TSV parsing to Rust core payload decoding. |
| QLPlugin/Views/Previews/TARPreview.swift | Switches TAR/TGZ scanning to Rust core + updates truncation/ratio labeling. |
| QLPlugin/Views/Previews/SevenZipPreview.swift | Switches 7z scanning to Rust core + Swift tree adapter. |
| QLPlugin/Views/Previews/MarkdownPreview.swift | Moves Markdown rendering to background execution and new renderer backend. |
| QLPlugin/Views/Previews/JupyterPreview.swift | Moves notebook rendering to background execution and new renderer backend. |
| QLPlugin/Views/Previews/DirectoryPreview.swift | Moves directory enumeration off-main with cancellation checks and bounded retention. |
| QLPlugin/Views/Previews/CodePreview.swift | Moves code rendering to background execution and new renderer backend. |
| QLPlugin/Views/NestedPreviewProvider.swift | Updates nested preview creation to async and awaits preview building. |
| QLPlugin/Utils/PreviewExecutor.swift | Adds shared detached executor that checks/discards results on cancellation. |
| QLPlugin/Utils/PreviewCoreBridge.swift | Adds Swift bridge for calling Rust FFI and decoding JSON payloads. |
| QLPlugin/Utils/HTMLRenderer.swift | Replaces Go/HTMLConverter calls with Rust FFI renderer calls and buffer handling. |
| QLPlugin/Resources/shared/shared-chroma.css | Updates syntax-highlighting CSS to syntect semantic classes while keeping .chroma. |
| QLPlugin/MainVC.swift | Makes preview preparation async/cancellable and adds task tracking for nested previews. |
| QLPlugin/Info.plist | Adds UTI support for recent .tar.gz identifier. |
| PreviewCore/THIRD_PARTY_LICENSES.md | Documents PreviewCore dependency licenses and references Cargo.lock as authoritative. |
| PreviewCore/src/zip.rs | Implements bounded ZIP metadata scanning with central-directory preflight and limits. |
| PreviewCore/src/tsv.rs | Implements bounded TSV parsing with row/column/file-size limits and tests. |
| PreviewCore/src/tar.rs | Implements TAR/TGZ metadata scanning with PAX/long-name support and gzip scan limits. |
| PreviewCore/src/sevenzip.rs | Implements bounded 7z metadata scanning with header preflight and encryption rejection. |
| PreviewCore/src/notebook.rs | Implements Jupyter Notebook rendering with sanitization and safe output handling. |
| PreviewCore/src/model.rs | Defines JSON-serializable payload types for TSV and archive metadata. |
| PreviewCore/src/markdown.rs | Implements Markdown rendering with front-matter rewriting and syntax highlighting hooks. |
| PreviewCore/src/lib.rs | Wires PreviewCore modules and re-exports FFI surface. |
| PreviewCore/src/highlight.rs | Implements syntect/two-face highlighting with classed HTML output and lexer selection. |
| PreviewCore/src/ffi.rs | Adds C ABI entry points, buffer ownership/freeing, status mapping, and panic containment. |
| PreviewCore/src/error.rs | Defines core error taxonomy and render error types used across FFI. |
| PreviewCore/include/glance_preview_core.h | Adds the C header for the Rust core FFI API. |
| PreviewCore/Cargo.toml | Adds Rust crate config (staticlib) and pinned dependencies/profiles. |
| PreviewCore/build-xcode.sh | Adds Xcode-friendly Rust build script using pinned toolchain via mise. |
| module.modulemap | Renames module to GlancePreviewCore and points to the new C header + archive. |
| mise.toml | Replaces Go tooling with pinned Rust + adds Rust test/audit and app verification tasks. |
| HTMLConverter/htmlconverter.go | Removes the legacy Go HTMLConverter implementation. |
| HTMLConverter/htmlconverter_test.go | Removes legacy Go converter tests. |
| HTMLConverter/go.sum | Removes Go dependency lockfile for removed converter. |
| HTMLConverter/go.mod | Removes Go module definition for removed converter. |
| HTMLConverter/.golangci.yml | Removes Go lint configuration no longer used. |
| GlanceTests/PlistCoverageTests.swift | Updates coverage assertions for new Rust toolchain/build script and version/build bump. |
| GlanceTests/NestedPreviewTests.swift | Migrates tests to async nested preview creation and adds timing helpers. |
| GlanceTests/DirectoryPreviewTests.swift | Migrates directory preview tests to async preview creation. |
| Glance/Shared/Utils/SupportedPreviewRegistry.swift | Updates lexer documentation to reflect syntect/two-face style naming. |
| Glance/Credits.rtf | Updates credits from Chroma/goldmark/nbtohtml and old Swift parsers to Rust core deps. |
| Glance.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved | Removes SwiftPM resolved file now that parser packages are removed. |
| Glance.xcodeproj/project.pbxproj | Rewires build phases and link inputs to libglance_preview_core.a, removes SwiftPM deps, bumps version/build. |
| AppStore/Listing/Description.txt | Updates app store listing version text to 1.6.0. |
| .gitignore | Updates ignored artifacts for Rust build outputs and removes Go converter outputs. |
| .github/workflows/verify.yml | Adds macOS CI workflow running mise run verify and verify:app gates. |
| .github/workflows/release.yml | Updates release pipeline to use production verification and release notes file, adds DMG checksum verification. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (20)
PreviewCore/src/ffi.rs (1)
207-227: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the unbounded lifetime on the input helpers.
byte_input<'a>andutf8_input<'a>return a reference whose lifetime no argument constrains. The caller can choose any'a, including'static. Every current call site consumes the slice inside the sameffi_callclosure, so the code is sound today. A later change that stores or returns the slice would still compile and would create a dangling reference.Add a
# Safetynote that the returned reference must not outlive the FFI call.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PreviewCore/src/ffi.rs` around lines 207 - 227, Add Rust documentation with a # Safety section to both utf8_input and byte_input, stating that the returned reference must not outlive the enclosing FFI call because its lifetime is not tied to the input pointer. Keep the existing validation and conversion behavior unchanged.PreviewCore/src/model.rs (1)
4-8: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftConsider positional rows instead of a map per row.
Vec<BTreeMap<String, String>>repeats every header string in every row and re-sorts keys, so the JSON key order does not followheaders. Duplicate header names also collapse to a single key, whileheadersstill lists the duplicate. In that case one column value is lost and another is rendered twice.A positional representation keeps column order, keeps duplicate headers distinct, and removes the per-row key allocations.
♻️ Proposed model change
#[derive(Debug, PartialEq, Serialize)] pub(crate) struct TsvPayload { pub headers: Vec<String>, - pub rows: Vec<BTreeMap<String, String>>, + pub rows: Vec<Vec<String>>, }This change also requires updates in
PreviewCore/src/tsv.rsand in the Swift decoder that consumes the TSV payload.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PreviewCore/src/model.rs` around lines 4 - 8, Change TsvPayload.rows from Vec<BTreeMap<String, String>> to a positional row representation aligned with headers, preserving column order and duplicate header values without repeated key storage. Update the TSV construction logic in tsv.rs and the Swift decoder to consume positional rows while keeping the existing headers and payload behavior intact.PreviewCore/Cargo.toml (2)
29-31: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDeclare the unwind panic strategy explicitly.
ffi_callinPreviewCore/src/ffi.rsusescatch_unwindto convert panics intoSTATUS_INTERNAL_ERROR. That conversion works only with the unwind strategy. The profile relies on the default today. If a future change setspanic = "abort", the Quick Look host process aborts instead of reporting an error.♻️ Proposed change to pin the panic strategy
[profile.release] codegen-units = 1 lto = "thin" +# `ffi_call` in src/ffi.rs converts panics into STATUS_INTERNAL_ERROR via catch_unwind. +panic = "unwind"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PreviewCore/Cargo.toml` around lines 29 - 31, Update the [profile.release] configuration in Cargo.toml to explicitly set the panic strategy to unwind, ensuring ffi_call can continue converting panics into STATUS_INTERNAL_ERROR instead of aborting the host process.
11-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider one consistent version-pinning policy.
Five dependencies use exact
=pins. Six use caret ranges.Cargo.lockplus--lockedinbuild-xcode.shalready fixes the resolved versions, so the exact pins add no reproducibility. Choose one style, or add a comment that explains whycsv,flate2,sevenz-rust2,two-face, andzipneed exact pins.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PreviewCore/Cargo.toml` around lines 11 - 27, Normalize the dependency version policy in the [dependencies] section: either remove the exact “=” pins from csv, flate2, sevenz-rust2, two-face, and zip to match the caret ranges, or document a concrete reason for retaining each exact pin. Preserve the existing feature flags and rely on Cargo.lock with --locked for reproducibility.PreviewCore/build-xcode.sh (1)
36-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider positional parameters and an explicit target check.
$PROFILE_ARGUMENTis an unquoted expansion, which relies on word splitting and triggers shellcheck SC2086. Positional parameters express the same intent without the warning. The script also does not confirm theaarch64-apple-darwintarget is installed, so a missing target produces a raw cargo error inside the Xcode build log instead of an actionable message like themisecheck on line 30.♻️ Proposed change
PROFILE="debug" -PROFILE_ARGUMENT="" +set -- case "${CONFIGURATION:-Debug}" in Release|Profile) PROFILE="release" - PROFILE_ARGUMENT="--release" + set -- --release ;; esac +if ! "$MISE_BIN" exec -- rustc --print target-list >/dev/null 2>&1; then + echo "The pinned Rust toolchain is not available through mise" >&2 + exit 1 +fi + "$MISE_BIN" exec -- cargo build \ --locked \ --manifest-path "$CORE_ROOT/Cargo.toml" \ --target "$TARGET" \ - $PROFILE_ARGUMENT + "$@"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PreviewCore/build-xcode.sh` around lines 36 - 49, Update the build command in the case-driven profile setup to pass the optional release flag via positional parameters instead of the unquoted PROFILE_ARGUMENT expansion, eliminating shellcheck SC2086 while preserving debug and release behavior. Before invoking cargo build, add an explicit check that the aarch64-apple-darwin target is installed and emit an actionable error consistent with the existing mise validation when it is missing.PreviewCore/include/glance_preview_core.h (1)
14-30: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPublish the status codes and buffer-ownership rule in the header.
The status values are part of this public ABI, but they exist only as private Rust constants in
PreviewCore/src/ffi.rs(lines 10-16). Any Swift or C consumer must hardcode0-6, and a later renumbering in Rust breaks consumers silently. The header also does not state that a non-NULLdatabuffer must be released withglance_render_buffer_free, or that error payloads carry a UTF-8 message.♻️ Proposed header additions
typedef struct GlanceRenderResult { uint8_t *data; size_t length; int32_t status; } GlanceRenderResult; + +/// Status values returned in `GlanceRenderResult.status`. +typedef enum GlanceStatus { + GlanceStatusOK = 0, + GlanceStatusInvalidInput = 1, + GlanceStatusParseError = 2, + GlanceStatusInternalError = 3, + GlanceStatusIOError = 4, + GlanceStatusResourceLimit = 5, + GlanceStatusUnsupported = 6, +} GlanceStatus; + +/// On `GlanceStatusOK`, `data` holds the payload bytes. On any other status, `data` holds a +/// UTF-8 error message. Release every non-NULL `data` exactly once with +/// `glance_render_buffer_free`, using the returned `length` unchanged.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PreviewCore/include/glance_preview_core.h` around lines 14 - 30, Publish the `GlanceRenderResult` status-code constants in `glance_preview_core.h`, matching the private Rust values in `ffi.rs` so C and Swift consumers use named ABI-stable symbols instead of numeric literals. Document that non-NULL result data is owned by the caller and must be released with `glance_render_buffer_free`, and that error payloads contain UTF-8 messages.QLPlugin/Utils/PreviewCoreBridge.swift (1)
119-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the decoding failure detail.
DecodingError.localizedDescriptionproduces a generic message and drops the coding path and the mismatched key. Use the debug description so the log identifies the payload field that failed.♻️ Proposed change
} catch { - throw PreviewCoreBridgeError.invalidPayload(error.localizedDescription) + throw PreviewCoreBridgeError.invalidPayload(String(describing: error)) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@QLPlugin/Utils/PreviewCoreBridge.swift` around lines 119 - 129, Update the catch block in the generic decode method to preserve detailed JSON decoding diagnostics by using the decoding error’s debug description when constructing PreviewCoreBridgeError.invalidPayload, including the coding path and mismatched key.PreviewCore/src/notebook.rs (1)
101-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReport a clearer error when
nbformatis absent.
nbformatdefaults to0when the key is missing. A notebook without the field then fails with a message about an old format. Distinguish the missing field from a real version below 4 so the user sees the correct cause.♻️ Proposed change
- #[serde(default)] - nbformat: i64, + nbformat: Option<i64>,- if notebook.nbformat < 4 { - return Err(RenderError::new( - "The provided Jupyter Notebook uses an old format; version 4 or newer is required", - )); - } + match notebook.nbformat { + None => { + return Err(RenderError::new( + "The provided Jupyter Notebook does not declare an nbformat version", + )); + } + Some(version) if version < 4 => { + return Err(RenderError::new( + "The provided Jupyter Notebook uses an old format; version 4 or newer is required", + )); + } + Some(_) => {} + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PreviewCore/src/notebook.rs` around lines 101 - 108, Update render_notebook to detect whether nbformat was absent before applying the old-format check, and return a clear missing-nbformat error in that case. Preserve the existing old-format error for notebooks that explicitly provide a version below 4.GlanceTests/PreviewSmokeTests.swift (2)
235-251: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the fixed sleep from the completion assertion.
startPreviewPreparationcalls the completion handler after the preparation task finishes, and the preparation installs the controller before it returns. The extra 50 ms sleep therefore adds wall-clock time and can still be too short on a loaded machine. Poll the state instead so the test stays deterministic.♻️ Proposed change
await fulfillment(of: [completion], timeout: 5) - try await Task.sleep(for: .milliseconds(50)) - XCTAssertTrue(mainVC.currentPreviewController is WebPreviewVC) + let deadline = ContinuousClock().now.advanced(by: .seconds(5)) + while !(mainVC.currentPreviewController is WebPreviewVC), ContinuousClock().now < deadline { + try await Task.sleep(for: .milliseconds(10)) + } + XCTAssertTrue(mainVC.currentPreviewController is WebPreviewVC)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@GlanceTests/PreviewSmokeTests.swift` around lines 235 - 251, The test testMainVCAsyncPreparationCompletesExactlyOnce should remove the fixed 50 ms Task.sleep and deterministically wait for currentPreviewController to become a WebPreviewVC after the completion callback. Poll or use an appropriate asynchronous expectation for that state, preserving the existing completion over-fulfillment assertion.
518-528: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFail the wait helper on timeout with a clear message.
If the web view is still loading at the deadline, the helper reports only
XCTAssertFalse(webView.isLoading). The following JavaScript assertions then fail with unrelated messages. Add an explicit failure message that names the timeout.♻️ Proposed change
while webView.isLoading, clock.now < deadline { try await Task.sleep(for: .milliseconds(10)) } - XCTAssertFalse(webView.isLoading) + XCTAssertFalse(webView.isLoading, "Web view did not finish loading within \(timeout)")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@GlanceTests/PreviewSmokeTests.swift` around lines 518 - 528, Update waitForWebViewToFinishLoadingAsync so the final assertion includes a clear message indicating that the web view failed to finish loading before the specified timeout, while preserving the existing loading-state check.QLPlugin/Views/Previews/DirectoryPreview.swift (1)
88-107: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the general log category for directory tree failures.
makeFileTreerecords node insertion failures underLog.parse. This code path enumerates a directory and does not parse a file format. The rest of this file usesLog.generalfor directory errors. UseLog.generalso the categories stay consistent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@QLPlugin/Views/Previews/DirectoryPreview.swift` around lines 88 - 107, Update the error logging in makeFileTree so node insertion failures use Log.general instead of Log.parse, while preserving the existing localized error message and privacy setting.QLPlugin/Utils/PreviewExecutor.swift (1)
3-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winState that cancellation does not interrupt the running operation.
onCancelcancels the detached task, buttry await task.valuestill waits for the synchronousoperationto return. The Rust FFI parsers do not pollTask.isCancelled, so a cancelled preview releases the caller only after the parser finishes. Bounded parser limits keep this finite. Record the behavior in the doc comment so callers do not assume prompt cancellation.♻️ Proposed change
-/// Runs preview preparation outside the main actor and discards results after cancellation. +/// Runs preview preparation outside the main actor and discards results after cancellation. +/// +/// Cancellation does not interrupt `operation`. A synchronous operation that does not check +/// `Task.isCancelled` runs to completion, and the result is discarded. Bounded parser limits +/// keep the wait finite.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@QLPlugin/Utils/PreviewExecutor.swift` around lines 3 - 19, Update the documentation comment for PreviewExecutor.run to state that cancellation does not interrupt the synchronous operation or release the caller until the operation finishes, while results are discarded afterward. Keep the existing cancellation handling and implementation unchanged.QLPlugin/Views/Previews/CodePreview.swift (1)
27-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCancellation is logged as a render failure in the converted previews.
PreviewExecutor.runthrowsCancellationErrorwhenever Quick Look replaces or dismisses a preview. Both catch blocks record that normal case at error level with a "Could not generate ... HTML" message.
QLPlugin/Views/Previews/CodePreview.swift#L27-L38: add acatch is CancellationErrorclause that rethrows without logging.QLPlugin/Views/Previews/JupyterPreview.swift#L89-L94: add the samecatch is CancellationErrorclause before the general catch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@QLPlugin/Views/Previews/CodePreview.swift` around lines 27 - 38, Add a CancellationError-specific catch before the general catch in the preview execution flows: QLPlugin/Views/Previews/CodePreview.swift lines 27-38 and QLPlugin/Views/Previews/JupyterPreview.swift lines 89-94. Rethrow cancellation without logging, while preserving the existing error logging and rethrow behavior for other failures.QLPlugin/Views/Previews/TARPreview.swift (1)
6-6: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDerive the entry limit from PreviewCore instead of duplicating it.
TARPreview.maxEntryCountduplicatesPreviewCore/src/tar.rs:MAX_ENTRY_COUNT. If the native limit changes, thetruncatedpayload can produce an incorrect"Preview truncated after 50000 entries"label. Export the limit through the payload or C header, or display the count only when the bridge reports it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@QLPlugin/Views/Previews/TARPreview.swift` at line 6, Remove the duplicated maxEntryCount constant from TARPreview and use the entry limit reported or exported by PreviewCore through the existing bridge. Update the truncated payload and its “Preview truncated after … entries” label to consume that native value, preserving accurate behavior when MAX_ENTRY_COUNT changes.PreviewCore/src/markdown.rs (1)
3-19: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSet Comrak’s sanitization flag explicitly.
The plugin paths are valid for Comrak 0.54.0. Use
options.render.r#unsafe = false; the field is not namedunsafe_.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PreviewCore/src/markdown.rs` around lines 3 - 19, Update render_markdown to explicitly set options.render.r#unsafe = false after creating the Comrak Options, using the exact r#unsafe field name and preserving the existing plugin configuration.PreviewCore/src/tar.rs (1)
442-456: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
tar_stringandmetadata_stringare identical.Both functions truncate at the first NUL byte and decode lossily. Keep one function and call it from both sites.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PreviewCore/src/tar.rs` around lines 442 - 456, Remove the duplicate implementation between tar_string and metadata_string by retaining one shared helper and updating both call sites to invoke it. Preserve the existing first-NUL truncation and lossy UTF-8 decoding behavior.PreviewCore/src/tsv.rs (1)
51-52: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚖️ Poor tradeoffRow truncation is silent.
take(max_rows)drops rows above the limit.TsvPayloadcarries no truncation flag, so the preview shows partial data and the user gets no indication.ArchivePayloadalready exposestruncatedfor archives.Consider adding an equivalent flag to
TsvPayloadand surfacing it in the TSV preview. This spansPreviewCore/src/model.rs, the FFI payload, and the Swift consumer, so it can be deferred.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PreviewCore/src/tsv.rs` around lines 51 - 52, Add a truncation indicator to TsvPayload and propagate it through the FFI model to the Swift TSV preview, setting it whenever records exceed max_rows. Update the consumer to surface the indicator while preserving existing row rendering.PreviewCore/src/zip.rs (1)
88-95: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueThe EOCD predicate rejects archives with trailing data.
The finder accepts a candidate only when
index + EOCD_MIN_SIZE + comment_length == tail.len(). Archives that carry appended bytes after the end-of-central-directory record, such as some self-extracting or concatenated files, fail with a parse error even though thezipcrate locates the record.If the previous parser previewed such files, relax the predicate to accept a comment length that does not exceed the remaining tail.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PreviewCore/src/zip.rs` around lines 88 - 95, The EOCD search predicate currently requires the record and comment to end exactly at tail.len(), rejecting archives with trailing data. In the eocd_index finder, change the length validation to accept comment lengths whose end is at or before the remaining tail, while preserving the existing signature and bounds checks.PreviewCore/src/sevenzip.rs (2)
154-159: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
system_time_to_unixnever returnsNone.Both match arms return
Some. TheOptionreturn type forces the.then(...).flatten()chain at lines 63-66. Returnf64and drop theflattencall.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PreviewCore/src/sevenzip.rs` around lines 154 - 159, Change system_time_to_unix to return f64 directly, preserving the existing positive and pre-UNIX_EPOCH conversions from both match arms. Update its caller’s .then(...).flatten() chain to remove flatten and use the direct numeric result.
161-179: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueBuild the CRC table once, or use an existing CRC implementation.
crc32rebuilds the 256-entry table on every call.preflight_headercalls it twice, and the second call covers up to 8 MiB. AOnceLocktable removes the repeated setup. Thecrc32fastcrate is already in the dependency graph throughflate2andzip, and a hand-rolled CRC is also present in thezip.rstest helpers.♻️ Proposed refactor to build the table once
+use std::sync::OnceLock; + fn crc32(bytes: &[u8]) -> u32 { - let mut table = [0_u32; 256]; - for (index, value) in table.iter_mut().enumerate() { - let mut crc = index as u32; - for _ in 0..8 { - crc = if crc & 1 == 1 { - 0xedb8_8320 ^ (crc >> 1) - } else { - crc >> 1 - }; - } - *value = crc; - } + static TABLE: OnceLock<[u32; 256]> = OnceLock::new(); + let table = TABLE.get_or_init(|| { + let mut table = [0_u32; 256]; + for (index, value) in table.iter_mut().enumerate() { + let mut crc = index as u32; + for _ in 0..8 { + crc = if crc & 1 == 1 { + 0xedb8_8320 ^ (crc >> 1) + } else { + crc >> 1 + }; + } + *value = crc; + } + table + }); let mut crc = u32::MAX; for byte in bytes { crc = table[((crc ^ u32::from(*byte)) & 0xff) as usize] ^ (crc >> 8); } !crc }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PreviewCore/src/sevenzip.rs` around lines 161 - 179, Update crc32 to avoid rebuilding its 256-entry lookup table on every call: initialize the table once with a suitable static mechanism such as OnceLock, or reuse an existing CRC implementation like crc32fast. Preserve the current CRC-32 output and call behavior used by preflight_header.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AppStore/Listing/Description.txt`:
- Line 5: Update the product name capitalization from “Apple silicon” to “Apple
Silicon” in AppStore/Listing/Description.txt lines 5-5 and README.md lines
35-35.
In `@PreviewCore/build-xcode.sh`:
- Around line 51-54: Update build-xcode.sh to install the generated archive into
a configuration-specific output path, such as separate Debug and Release
locations, instead of the shared "$OUTPUT_DIRECTORY/libglance_preview_core.a"
path. Keep the "$PROFILE" selection used by the Cargo archive input and ensure
the Xcode phase output path matches the selected configuration.
In `@PreviewCore/src/tar.rs`:
- Around line 328-344: Update the checksum validation near stored_checksum and
computed_checksum to also calculate the signed-byte header checksum, treating
each header byte as a signed 8-bit value while preserving spaces for the
checksum field. Accept the header when the stored checksum matches either the
existing unsigned sum or the signed sum; retain the current parse error when
neither matches.
In `@PreviewCore/src/tsv.rs`:
- Around line 55-59: Disambiguate duplicate header names before the row-building
logic in the TSV parsing flow, preserving the original name for the first
occurrence and assigning unique names to subsequent occurrences. Use the
resulting unique headers both for the returned headers collection and the
BTreeMap construction in the visible row-mapping code, so each column retains
its own value and TablePreviewVC receives matching keys.
In `@PreviewCore/src/zip.rs`:
- Around line 41-51: In the ZIP entry-processing loop, move the __MACOSX path
filtering in front of the compressed_size and uncompressed_size checked_add
operations. Ensure filtered entries continue immediately without affecting
totals, while non-filtered entries retain the existing overflow handling and are
added to entries.
In `@PreviewCore/THIRD_PARTY_LICENSES.md`:
- Around line 7-13: Add license references for MIT-0 and Unicode-DFS-2016 to the
introductory list, and replace the bzip2 homepage URL with a direct link to the
bzip2-1.0.6 license terms. Verify that every listed URL resolves successfully,
retaining the existing license names and formatting.
In `@QLPlugin/Info.plist`:
- Line 47: Update the Quick Look handling associated with the
org.gnu.gnu-zip-archive declaration so bare .gz files are rejected with an error
that triggers system fallback, while .tar.gz files continue to use the existing
preview path. Ensure the unsupported case propagates through previewFile and
does not complete successfully when PreviewVCFactory returns nil.
In `@QLPlugin/MainVC.swift`:
- Around line 214-222: In previewFile, add a cancellation check immediately
after await previewInitializer.createPreviewVC(file: file) and before installing
the top-level preview. Use the same Task.checkCancellation() guard as the nested
preview path, preserving the existing behavior for non-cancelled requests.
In `@QLPlugin/Views/Previews/SevenZipPreview.swift`:
- Around line 16-24: Update the compression ratio interpolation in
SevenZipPreview’s labelText to format compressionRatio with one decimal place,
matching the "%.1f" formatting used by ZIPPreview and TARPreview.
In `@QLPlugin/Views/Previews/TSVPreview.swift`:
- Around line 8-11: Update the Data(contentsOf:) call in the PreviewExecutor.run
closure to perform a plain read without .mappedIfSafe before passing the data to
PreviewCoreBridge.parseTSV; preserve the existing parsing and error propagation
flow.
In `@QLPlugin/Views/Previews/ZIPPreview.swift`:
- Around line 28-45: Update makeFileTree to catch ZIPPreviewError, including
metadataSizeLimitExceeded thrown by checkedInt, before the generic catch.
Propagate that ZIPPreviewError instead of logging and skipping the entry, while
preserving the existing handling for other errors.
In `@RELEASE_NOTES.md`:
- Around line 15-16: Update the macOS release signing description in
RELEASE_NOTES.md to state that the DMG is ad hoc signed, or explicitly not
Developer ID-signed, instead of calling it unsigned; preserve the existing
notarization and checksum guidance.
---
Nitpick comments:
In `@GlanceTests/PreviewSmokeTests.swift`:
- Around line 235-251: The test testMainVCAsyncPreparationCompletesExactlyOnce
should remove the fixed 50 ms Task.sleep and deterministically wait for
currentPreviewController to become a WebPreviewVC after the completion callback.
Poll or use an appropriate asynchronous expectation for that state, preserving
the existing completion over-fulfillment assertion.
- Around line 518-528: Update waitForWebViewToFinishLoadingAsync so the final
assertion includes a clear message indicating that the web view failed to finish
loading before the specified timeout, while preserving the existing
loading-state check.
In `@PreviewCore/build-xcode.sh`:
- Around line 36-49: Update the build command in the case-driven profile setup
to pass the optional release flag via positional parameters instead of the
unquoted PROFILE_ARGUMENT expansion, eliminating shellcheck SC2086 while
preserving debug and release behavior. Before invoking cargo build, add an
explicit check that the aarch64-apple-darwin target is installed and emit an
actionable error consistent with the existing mise validation when it is
missing.
In `@PreviewCore/Cargo.toml`:
- Around line 29-31: Update the [profile.release] configuration in Cargo.toml to
explicitly set the panic strategy to unwind, ensuring ffi_call can continue
converting panics into STATUS_INTERNAL_ERROR instead of aborting the host
process.
- Around line 11-27: Normalize the dependency version policy in the
[dependencies] section: either remove the exact “=” pins from csv, flate2,
sevenz-rust2, two-face, and zip to match the caret ranges, or document a
concrete reason for retaining each exact pin. Preserve the existing feature
flags and rely on Cargo.lock with --locked for reproducibility.
In `@PreviewCore/include/glance_preview_core.h`:
- Around line 14-30: Publish the `GlanceRenderResult` status-code constants in
`glance_preview_core.h`, matching the private Rust values in `ffi.rs` so C and
Swift consumers use named ABI-stable symbols instead of numeric literals.
Document that non-NULL result data is owned by the caller and must be released
with `glance_render_buffer_free`, and that error payloads contain UTF-8
messages.
In `@PreviewCore/src/ffi.rs`:
- Around line 207-227: Add Rust documentation with a # Safety section to both
utf8_input and byte_input, stating that the returned reference must not outlive
the enclosing FFI call because its lifetime is not tied to the input pointer.
Keep the existing validation and conversion behavior unchanged.
In `@PreviewCore/src/markdown.rs`:
- Around line 3-19: Update render_markdown to explicitly set
options.render.r#unsafe = false after creating the Comrak Options, using the
exact r#unsafe field name and preserving the existing plugin configuration.
In `@PreviewCore/src/model.rs`:
- Around line 4-8: Change TsvPayload.rows from Vec<BTreeMap<String, String>> to
a positional row representation aligned with headers, preserving column order
and duplicate header values without repeated key storage. Update the TSV
construction logic in tsv.rs and the Swift decoder to consume positional rows
while keeping the existing headers and payload behavior intact.
In `@PreviewCore/src/notebook.rs`:
- Around line 101-108: Update render_notebook to detect whether nbformat was
absent before applying the old-format check, and return a clear missing-nbformat
error in that case. Preserve the existing old-format error for notebooks that
explicitly provide a version below 4.
In `@PreviewCore/src/sevenzip.rs`:
- Around line 154-159: Change system_time_to_unix to return f64 directly,
preserving the existing positive and pre-UNIX_EPOCH conversions from both match
arms. Update its caller’s .then(...).flatten() chain to remove flatten and use
the direct numeric result.
- Around line 161-179: Update crc32 to avoid rebuilding its 256-entry lookup
table on every call: initialize the table once with a suitable static mechanism
such as OnceLock, or reuse an existing CRC implementation like crc32fast.
Preserve the current CRC-32 output and call behavior used by preflight_header.
In `@PreviewCore/src/tar.rs`:
- Around line 442-456: Remove the duplicate implementation between tar_string
and metadata_string by retaining one shared helper and updating both call sites
to invoke it. Preserve the existing first-NUL truncation and lossy UTF-8
decoding behavior.
In `@PreviewCore/src/tsv.rs`:
- Around line 51-52: Add a truncation indicator to TsvPayload and propagate it
through the FFI model to the Swift TSV preview, setting it whenever records
exceed max_rows. Update the consumer to surface the indicator while preserving
existing row rendering.
In `@PreviewCore/src/zip.rs`:
- Around line 88-95: The EOCD search predicate currently requires the record and
comment to end exactly at tail.len(), rejecting archives with trailing data. In
the eocd_index finder, change the length validation to accept comment lengths
whose end is at or before the remaining tail, while preserving the existing
signature and bounds checks.
In `@QLPlugin/Utils/PreviewCoreBridge.swift`:
- Around line 119-129: Update the catch block in the generic decode method to
preserve detailed JSON decoding diagnostics by using the decoding error’s debug
description when constructing PreviewCoreBridgeError.invalidPayload, including
the coding path and mismatched key.
In `@QLPlugin/Utils/PreviewExecutor.swift`:
- Around line 3-19: Update the documentation comment for PreviewExecutor.run to
state that cancellation does not interrupt the synchronous operation or release
the caller until the operation finishes, while results are discarded afterward.
Keep the existing cancellation handling and implementation unchanged.
In `@QLPlugin/Views/Previews/CodePreview.swift`:
- Around line 27-38: Add a CancellationError-specific catch before the general
catch in the preview execution flows: QLPlugin/Views/Previews/CodePreview.swift
lines 27-38 and QLPlugin/Views/Previews/JupyterPreview.swift lines 89-94.
Rethrow cancellation without logging, while preserving the existing error
logging and rethrow behavior for other failures.
In `@QLPlugin/Views/Previews/DirectoryPreview.swift`:
- Around line 88-107: Update the error logging in makeFileTree so node insertion
failures use Log.general instead of Log.parse, while preserving the existing
localized error message and privacy setting.
In `@QLPlugin/Views/Previews/TARPreview.swift`:
- Line 6: Remove the duplicated maxEntryCount constant from TARPreview and use
the entry limit reported or exported by PreviewCore through the existing bridge.
Update the truncated payload and its “Preview truncated after … entries” label
to consume that native value, preserving accurate behavior when MAX_ENTRY_COUNT
changes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 62f428ab-46dc-4953-8958-6fb2b467ee91
⛔ Files ignored due to path filters (5)
Glance.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolvedis excluded by!**/Package.resolvedGlanceTests/TestFiles/archives/encrypted.7zis excluded by!**/*.7zGlanceTests/TestFiles/archives/example.7zis excluded by!**/*.7zHTMLConverter/go.sumis excluded by!**/*.sumPreviewCore/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (50)
.github/workflows/release.yml.github/workflows/verify.yml.gitignoreAppStore/Listing/Description.txtGlance.xcodeproj/project.pbxprojGlance/Credits.rtfGlance/Shared/Utils/SupportedPreviewRegistry.swiftGlanceTests/DirectoryPreviewTests.swiftGlanceTests/NestedPreviewTests.swiftGlanceTests/PlistCoverageTests.swiftGlanceTests/PreviewSmokeTests.swiftHTMLConverter/.golangci.ymlHTMLConverter/go.modHTMLConverter/htmlconverter.goHTMLConverter/htmlconverter_test.goPreviewCore/Cargo.tomlPreviewCore/THIRD_PARTY_LICENSES.mdPreviewCore/build-xcode.shPreviewCore/include/glance_preview_core.hPreviewCore/src/error.rsPreviewCore/src/ffi.rsPreviewCore/src/highlight.rsPreviewCore/src/lib.rsPreviewCore/src/markdown.rsPreviewCore/src/model.rsPreviewCore/src/notebook.rsPreviewCore/src/sevenzip.rsPreviewCore/src/tar.rsPreviewCore/src/tsv.rsPreviewCore/src/zip.rsQLPlugin/Info.plistQLPlugin/MainVC.swiftQLPlugin/Resources/shared/shared-chroma.cssQLPlugin/Utils/HTMLRenderer.swiftQLPlugin/Utils/PreviewCoreBridge.swiftQLPlugin/Utils/PreviewExecutor.swiftQLPlugin/Views/NestedPreviewProvider.swiftQLPlugin/Views/PreviewVC.swiftQLPlugin/Views/Previews/CodePreview.swiftQLPlugin/Views/Previews/DirectoryPreview.swiftQLPlugin/Views/Previews/JupyterPreview.swiftQLPlugin/Views/Previews/MarkdownPreview.swiftQLPlugin/Views/Previews/SevenZipPreview.swiftQLPlugin/Views/Previews/TARPreview.swiftQLPlugin/Views/Previews/TSVPreview.swiftQLPlugin/Views/Previews/ZIPPreview.swiftREADME.mdRELEASE_NOTES.mdmise.tomlmodule.modulemap
💤 Files with no reviewable changes (4)
- HTMLConverter/go.mod
- HTMLConverter/htmlconverter_test.go
- HTMLConverter/htmlconverter.go
- HTMLConverter/.golangci.yml
|
Review follow-up on d38fcae: Implemented the remaining defensive nits: deterministic async test waits and timeout messages; positional shell arguments plus an installed-target check; explicit unwind panic policy and exact-pin rationale; public ABI status/ownership documentation; FFI lifetime safety notes; explicit unsafe-Markdown disablement; clearer missing-nbformat errors; one-time 7z CRC initialization; simplified 7z timestamps and TAR string parsing; detailed Swift JSON decode errors; cancellation behavior documentation and quiet cancellation propagation; and the correct directory log category. Local Three suggestions were intentionally not applied:
|
|
@codex review |
|
CI failure triage: run 31603857202 never reached project verification. |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Glance currently uses Go and several Swift packages to parse untrusted preview formats on the Quick Look path. This moves rendering and TSV/archive parsing into a bounded Rust static core while keeping the native Swift/AppKit/WebKit shell.
Preview preparation is now cancellation-aware and runs rendering, parsing, JSON decoding, and directory enumeration off the main actor. The old Go runtime and Swift parser dependencies are removed. This also prepares the unsigned Apple-silicon v1.6.0 release.
Validation:
mise run verifymise run verify:appSummary by CodeRabbit
New Features
.tar.gzQuick Look support.Bug Fixes
Documentation