fix: bound artifact previews and stabilize preview lifecycle - #375
Merged
Conversation
alwaysmavs
marked this pull request as ready for review
September 6, 2026 15:11
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (31)
Included review availability: 4 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. Summary by CodeRabbit
WalkthroughThe change adds bounded ZIP, ZIP64, TAR, and TGZ archive previews with cancellation, truncation, and malformed-input handling. It separates archive parsing from general artifact preview code. Chat previews now use keyed caches, bounded retries, separate scheduler pools, stale-result protection, retry controls, resource lifecycle callbacks, persistent spreadsheet hosts, and synchronized navigation state. Tests cover archive limits, cache behavior, renderer lifecycles, scheduler behavior, and navigation races. Sequence Diagram(s)sequenceDiagram
participant ArtifactSelection
participant PreviewCache
participant PreviewRenderer
participant RetryControl
ArtifactSelection->>PreviewCache: request keyed artifact preview
PreviewCache->>PreviewRenderer: provide cached or loaded preview
PreviewRenderer-->>PreviewCache: report resource loaded or failed
RetryControl->>PreviewCache: invalidate failed preview
PreviewCache->>PreviewRenderer: load replacement preview
Merge Risk: ⚪ Minimal · up to No concrete merge-blocking risk remains; the identified concerns are non-material documentation and refactoring suggestions. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches✨ Simplify code
Comment |
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
1 participant
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
Artifact previews could retain transient read failures, reuse cancelled queued requests, and let an older request overwrite refreshed cache data. Returning from a folder could also restore the initial file selection, while spreadsheet switches rebuilt the full Univer runtime. Archive listings displayed only 300 entries but still copied/read entire ZIPs or scanned TARs without a processing budget.
This change fixes the preview lifecycle and bounds archive work:
read_failed/missingresults; reject reuse of aborted requests and verify request identity before cache writes. Subscribe by semantic file identity, including thumbnails.Verification
All implementation checks below ran against commit
5d8b8bfe; lint and changed-file formatting were rerun before opening this PR.corepack pnpm run ts-check— passed, also included in the production build.corepack pnpm run lint— passed, freshly rerun.corepack pnpm run format— full-repository formatting was not rerun;oxfmt --checkpassed for all 31 files changed by this PR, preserving unrelated local artifacts.corepack pnpm test— 3,137 passed / 4 skipped; 389 test files passed / 2 skipped.corepack pnpm run build— equivalentcorepack pnpm run build:apppassed (renderer, main and preload). Existing large-chunk warnings remain; Univer stays lazy-loaded.Regression tests cover cancellation/reacquisition, stale cache writes, transient failure recovery, retry limits, navigation races, thumbnail consumers, runtime reuse, archive read/scan budgets, ZIP64/Unicode metadata, and trusted snapshot identity. Runtime evidence is retained locally; screenshots and unrelated local audit artifacts are not included in this PR.
Safety and Compatibility
nullfor incomplete scans.Active native thumbnail work and filesystem reads are still not forcibly interruptible through renderer IPC. Separate pools, bounded parsing and stale-result isolation address the audited impact without adding a generic cancellation framework. The parsing deadline is cooperative and excludes snapshot copying (bounded by file size). Split ZIP archives remain unsupported; exactly 300 TAR entries conservatively yield an unknown total. No global PDF/DOCX viewer cache or large-document performance benchmark is introduced.