Repository navigation
refactor(import): project CSV previews without retaining raw row matrices - #201
Merged
Merged
Conversation
johnny4young
marked this pull request as ready for review
October 6, 2026 03:47
johnny4young
commented
Oct 6, 2026
johnny4young
left a comment
Owner
Author
There was a problem hiding this comment.
Summary: ClipImporter.readCSV now runs header interpretation and candidate projection inside a row visitor (forEachCSVRow), so it no longer keeps the full [[String]] matrix plus a formula-guard-mapped copy in memory.
Verdict: ready. Nothing blocking.
What I checked against main:
- Header selection is the same: it is the first non-empty row, because empty rows are still filtered in
endRow.textstill wins overcontenttext, the first duplicate column still wins, andisGanchoExportstill keys oncontenttext. - Error precedence is the same. The old code parsed everything before validating the header, and the new code finishes the scan before the
emptyCSV/missingTextColumnguards, sounclosedQuotedFieldstill wins over a bad header. No partialDocumentcan escape, because the visitor only appends to a local array and the function throws before returning. - Short-row and blank-text handling (
unsupportedCount), and the pinned valuestrue/1/yes, are unchanged. - The deleted
parseCSVhad no other callers. - CI is green: lint, package tests, app builds.
🟡 Nit, optional: when the header has no text column, the remaining rows are scanned only to keep syntax-error precedence. That is the right call, and the closure already returns early, so this is fine as is.
Generated by Claude Code
…w tests - Strip Gancho formula guards from the text, title and pinned fields only, through one bounds-checked helper, instead of copying every column per row - Drop a malformed-tail assertion that could never fail and compare the large preview against one expected array - State the real column precedence in the architecture notes
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.
Review follow-ups
Reviewed; no changes needed. Header selection, error precedence (
unclosedQuotedFieldover an invalid header), short-row/blank-text counting and pinned parsing are identical tomain, and no other caller used the removedparseCSV.Merge order
Independent of every other open PR; merge before the v0.10.0 release PR #193.
Original description:
Finding
CSV preview first collected the entire raw row matrix, shifted off its header, and for Gancho exports created another complete formula-decoded matrix before constructing candidates. Projection did not need either retained intermediary collection.
Refactor
The full input string and final candidate document still remain in memory. This is not a bounded-memory file-streaming API, and no measured memory or latency improvement is claimed.
Regression coverage
Six new native tests cover BOM/normalized and duplicate headers, Gancho versus foreign apostrophes, quoted CRLF/multiline and escaped quotes, short optional fields and unsupported rows, malformed-tail atomicity/error precedence, and ordered repeatable 5,000-row previews. Existing importer/export roundtrip and migration tests remain intact.
Verification
Published source head: ca93a55
Uploaded tree: f51b961003bc89fee3b20c3a5bd4c9ced1a23f22
Explicit base: main at ccc4403
Scope and integration
Independent main-based PR; no dependency on the open Keychain, license, archive, recovery, ownership or MCP repairs. Storage ownership/actual-connection lifetime protocols are untouched. Physical iOS/App Group lifecycle, quiescent upgraded cohort and combined native MCP qualification remain outside this refactor and unresolved. No merge, release, deployment, live library or credential access.
Exact-head hosted evidence
Corrected source head: ca93a55
Verified remote/local tree: f51b961003bc89fee3b20c3a5bd4c9ced1a23f22
Verified synthetic PR merge checkout: cf4d528
Ready for human review; remains main-based and mergeable. No gates were relaxed, and no merge, release or deployment occurred.
Human-review readiness
Scoped CSV implementation and regression coverage are complete at unchanged head ca93a55. Fresh verification confirms both required native jobs and GitGuardian successful, with the expected non-blocking beta-canary skip, and no pending discussion comments, submitted reviews or unresolved inline threads. No physical lifecycle or combined-integration qualification is inferred for other open PRs.
🤖 Generated with Claude Code
https://claude.ai/code/session_01ExmnfUngrDYjAheGvYwMnn
Generated by Claude Code