feat: Add CSS chunking support and unify CSS file format - #190
Conversation
Port blob URL / magic-byte media detection into the pipeline architecture so Code Styler and similar plugins no longer cause 415 upload errors.
📝 WalkthroughWalkthroughCSS metadata now supports single and chunked stylesheet references through upload, sharing, and note payload construction. Media uploads add URL, MIME, and signature-based type detection with blob handling. Share icons respond to metadata and layout changes, and mise configures Node.js 23. ChangesCSS publishing
Media type detection
Share interface synchronization
Tooling configuration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ShareService
participant uploadCss
participant buildPayload
participant NotePayload
ShareService->>uploadCss: upload or reuse CSS files
uploadCss-->>ShareService: return CSS file metadata
ShareService->>buildPayload: provide cssFiles
buildPayload->>NotePayload: embed stylesheet references
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
mise.toml (1)
1-2: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAlign the Node.js major version across local tooling and CI.
mise.tomluses Node.js 23, while.github/workflows/release.ymlruns 20.x. Pick one supported major and keep both environments on the same version so local behavior matches release builds.🤖 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 `@mise.toml` around lines 1 - 2, Align the Node.js version configured by the [tools] section in mise.toml with the major version used by the release workflow’s Node setup. Choose one supported major version and update both configuration points consistently, preserving the existing tooling and CI setup.src/pipeline/upload-css.ts (2)
189-244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
hashChanged/needsResplit/hasExistingChunksnever affect control flow here.Line 115-117 already returns early whenever
existingCssis present and!options.isForceUpload. So by the time execution reaches line 198/228, eitherexistingCssis falsy (makinghashChangedtriviallytrue) oroptions.isForceUploadistrue(making both||conditions triviallytrueregardless ofhashChanged). The comparison itself is also semantically off for the reachable-but-forced case:cssHash(whole-file hash) is compared againstexistingCss[0]?.hash, which for a previously-chunked upload is a single chunk's content hash, not a whole-file hash — comparing apples to oranges if this logic is ever wired up to actually gate behavior later.Recommend simplifying to remove the dead computation, or wiring an actual whole-content hash comparison if automatic re-upload-on-change (without force) is desired in the future.
🤖 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 `@src/pipeline/upload-css.ts` around lines 189 - 244, Remove the dead hashChanged, needsResplit, and hasExistingChunks computations from the upload flow and simplify their dependent conditions in the surrounding CSS chunk/single-file upload logic. Preserve the existing early-return and force-upload behavior, without introducing a whole-content hash comparison or changing automatic re-upload semantics.
33-85: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPer-character
TextEncoder.encode()calls are wasteful for the large files this feature targets, and UTF-16 iteration risks splitting surrogate pairs.
encoder.encode(char)allocates a newUint8Arrayfor every character in the loop (line 48) — for multi-hundred-KB/MB CSS this is a large number of allocations on what is effectively the hot path this PR is meant to optimize. Additionally, iteratingcss[i]by UTF-16 code unit means a chunk boundary can land between the two halves of a surrogate pair (e.g., an emoji in acontent:value), corrupting that character in the resulting chunks.Consider iterating by code point (
for (const char of css), which iterates by Unicode code point and never splits a surrogate pair) and computing UTF-8 byte length arithmetically from the code point instead of re-encoding each character:♻️ Suggested approach
- for (let i = 0; i < css.length; i++) { - const char = css[i] - const charBytes = encoder.encode(char).length + let prevChar = '' + for (const char of css) { + const codePoint = char.codePointAt(0) || 0 + const charBytes = codePoint <= 0x7F ? 1 : codePoint <= 0x7FF ? 2 : codePoint <= 0xFFFF ? 3 : 4 if (!inString && (char === '"' || char === "'")) { inString = true stringChar = char - } else if (inString && char === stringChar && css[i - 1] !== '\\') { + } else if (inString && char === stringChar && prevChar !== '\\') { inString = false stringChar = '' } ... + prevChar = char }🤖 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 `@src/pipeline/upload-css.ts` around lines 33 - 85, Update splitCssIntoChunks to iterate over css by Unicode code point rather than UTF-16 index, preserving surrogate pairs in every chunk. Replace per-character TextEncoder.encode calls with arithmetic UTF-8 byte-length calculation from each code point, while retaining the existing brace, string, and chunk-boundary behavior.src/pipeline/upload-css.test.ts (1)
1-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage for
splitCssIntoChunks; consider adding tests foruploadCssitself.The chunking helper is well tested, but there's no test coverage here (or elsewhere in this cohort) for
uploadCss's new behavior: the early-return-on-existing-CSS path, single-vs-multi-chunk output construction, or the multi-chunk partial-failure scenario. Given the amount of new branching logic inuploadCss, tests for at least the reuse-without-force and chunked-upload-success paths would meaningfully increase confidence.🤖 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 `@src/pipeline/upload-css.test.ts` around lines 1 - 30, The test file currently covers only splitCssIntoChunks; add focused uploadCss tests covering reuse of existing CSS without force, successful single- and multi-chunk uploads, and preferably partial failure during multi-chunk upload. Mock the upload and lookup dependencies, assert the existing-CSS path avoids uploading, and verify chunked uploads produce the expected output and failure behavior.
🤖 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 `@src/pipeline/upload-media.ts`:
- Around line 82-87: Update the media-type detection logic around the MP4 ftyp
check to inspect the major and compatible brands, returning mp4 only for
supported MP4 brands rather than every ISO-BMFF container. Ensure AVIF, HEIC,
and other unsupported brands do not fall through as mp4 and therefore remain
eligible for unknown-blob handling. Add regression coverage for AVIF and HEIC
detection.
- Around line 95-100: Update the SVG detection logic around the sample marker
check so an XML declaration alone is not sufficient; after an optional <?xml
...?> declaration and surrounding whitespace, require the next root element to
be <svg>. Preserve detection for samples beginning directly with an SVG root and
continue returning 'svg' only for actual SVG content.
---
Nitpick comments:
In `@mise.toml`:
- Around line 1-2: Align the Node.js version configured by the [tools] section
in mise.toml with the major version used by the release workflow’s Node setup.
Choose one supported major version and update both configuration points
consistently, preserving the existing tooling and CI setup.
In `@src/pipeline/upload-css.test.ts`:
- Around line 1-30: The test file currently covers only splitCssIntoChunks; add
focused uploadCss tests covering reuse of existing CSS without force, successful
single- and multi-chunk uploads, and preferably partial failure during
multi-chunk upload. Mock the upload and lookup dependencies, assert the
existing-CSS path avoids uploading, and verify chunked uploads produce the
expected output and failure behavior.
In `@src/pipeline/upload-css.ts`:
- Around line 189-244: Remove the dead hashChanged, needsResplit, and
hasExistingChunks computations from the upload flow and simplify their dependent
conditions in the surrounding CSS chunk/single-file upload logic. Preserve the
existing early-return and force-upload behavior, without introducing a
whole-content hash comparison or changing automatic re-upload semantics.
- Around line 33-85: Update splitCssIntoChunks to iterate over css by Unicode
code point rather than UTF-16 index, preserving surrogate pairs in every chunk.
Replace per-character TextEncoder.encode calls with arithmetic UTF-8 byte-length
calculation from each code point, while retaining the existing brace, string,
and chunk-boundary behavior.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 90fb17e2-5a09-439c-8baa-c9ef7a803a4e
📒 Files selected for processing (10)
mise.tomlsrc/NotePayload.tssrc/api.test.tssrc/api.tssrc/pipeline/build-payload.tssrc/pipeline/share-service.tssrc/pipeline/upload-css.test.tssrc/pipeline/upload-css.tssrc/pipeline/upload-media.test.tssrc/pipeline/upload-media.ts
| // MP4: ftyp box at offset 4 | ||
| if ( | ||
| bytes.length >= 8 && | ||
| bytes[4] === 0x66 && bytes[5] === 0x74 && bytes[6] === 0x79 && bytes[7] === 0x70 | ||
| ) { | ||
| return 'mp4' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not classify every ISO-BMFF container as MP4.
ftyp is also used by AVIF, HEIC, and other ISO-BMFF formats. An image/avif blob has no MIME mapping here, then falls through to mp4, bypassing the unknown-blob skip and uploading with the wrong type. Inspect the major/compatible brands and return mp4 only for supported MP4 brands; add an AVIF/HEIC regression case.
🤖 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 `@src/pipeline/upload-media.ts` around lines 82 - 87, Update the media-type
detection logic around the MP4 ftyp check to inspect the major and compatible
brands, returning mp4 only for supported MP4 brands rather than every ISO-BMFF
container. Ensure AVIF, HEIC, and other unsupported brands do not fall through
as mp4 and therefore remain eligible for unknown-blob handling. Add regression
coverage for AVIF and HEIC detection.
| // SVG: text markers near the start | ||
| const sample = new TextDecoder('utf-8', { fatal: false }) | ||
| .decode(new Uint8Array(content, 0, Math.min(content.byteLength, 256))) | ||
| .trimStart() | ||
| if (sample.startsWith('<?xml') || sample.startsWith('<svg')) { | ||
| return 'svg' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Require an SVG root element after an XML declaration.
Any generic XML payload beginning with <?xml is labeled svg, so an otherwise unknown blob is uploaded instead of skipped. Match an actual <svg> root after an optional declaration.
Proposed fix
- if (sample.startsWith('<?xml') || sample.startsWith('<svg')) {
+ if (/^(?:<\?xml\b[^>]*\?>\s*)?<svg(?:\s|>)/i.test(sample)) {
return 'svg'
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // SVG: text markers near the start | |
| const sample = new TextDecoder('utf-8', { fatal: false }) | |
| .decode(new Uint8Array(content, 0, Math.min(content.byteLength, 256))) | |
| .trimStart() | |
| if (sample.startsWith('<?xml') || sample.startsWith('<svg')) { | |
| return 'svg' | |
| // SVG: text markers near the start | |
| const sample = new TextDecoder('utf-8', { fatal: false }) | |
| .decode(new Uint8Array(content, 0, Math.min(content.byteLength, 256))) | |
| .trimStart() | |
| if (/^(?:<\?xml\b[^>]*\?>\s*)?<svg(?:\s|>)/i.test(sample)) { | |
| return 'svg' |
🤖 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 `@src/pipeline/upload-media.ts` around lines 95 - 100, Update the SVG detection
logic around the sample marker check so an XML declaration alone is not
sufficient; after an optional <?xml ...?> declaration and surrounding
whitespace, require the next root element to be <svg>. Preserve detection for
samples beginning directly with an SVG root and continue returning 'svg' only
for actual SVG content.
Newer Obsidian property DOM no longer matches the old div.external-link exact-text check, so update/copy/delete icons never injected. Match more URL render shapes and re-inject on metadata/layout changes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/main.ts`:
- Around line 278-296: Update tryInjectShareIcons to set injected = true when a
matching div.share-note-icons element already exists before skipping that
property, so the method reports that the icons are present and addShareIcons
does not create redundant MutationObservers.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ffb0e289-6a48-4903-9620-67e2e61fb595
📒 Files selected for processing (2)
src/main.tsstyles.css
| private tryInjectShareIcons (activeFile: TFile, fieldKey: string, shareLink: string): boolean { | ||
| let injected = false | ||
| activeDocument.querySelectorAll(`div.metadata-property[data-property-key="${fieldKey}"]`) | ||
| .forEach(propertyEl => { | ||
| const valueEl = propertyEl.querySelector('div.metadata-property-value') | ||
| const linkEl = valueEl?.querySelector('div.external-link') as HTMLElement | ||
| if (linkEl?.innerText !== shareLink) return | ||
| if (!valueEl || valueEl.querySelector('div.share-note-icons')) return | ||
|
|
||
| // `data-property-key` already identifies the share_link row. Do not | ||
| // require a brittle `div.external-link` exact text match - newer | ||
| // Obsidian builds render URL properties as anchors/inputs/plain text. | ||
| // If a value is present and clearly not our link, skip; otherwise inject. | ||
| const hasAnyValue = !!(valueEl.textContent || '').trim() || !!valueEl.querySelector('input, a, div.external-link') | ||
| if (hasAnyValue && !this.propertyValueMatchesShareLink(valueEl, shareLink)) { | ||
| // Still allow injection when the row is our property key and the | ||
| // displayed text is only the URL without hash, icons, etc. The | ||
| // matcher already accepts base-URL prefixes; if it still fails the | ||
| // property is showing unrelated content. | ||
| return | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
tryInjectShareIcons never reports success once icons are already injected — causes repeated MutationObserver churn.
Line 283 skips the row when div.share-note-icons already exists, but doesn't set injected = true. So tryInjectShareIcons() (and therefore addShareIcons()'s inject()) returns false even when the icons are correctly in place, and addShareIcons() falls through to spin up a brand-new MutationObserver on activeDocument.body (subtree-wide) that lives for up to 5s (Line 247, increased from the previous shorter timeout).
This matters more now because addShareIcons() is triggered far more often than before: active-leaf-change plus the new metadataCache.on('changed') (fires on note edits) and workspace.on('layout-change') (Lines 144-151). While a note is already shared, every edit/layout event can spawn another full-document observer that re-scans the DOM on each mutation for up to 5 seconds, and these can stack concurrently during active editing.
🛠️ Proposed fix
.forEach(propertyEl => {
const valueEl = propertyEl.querySelector('div.metadata-property-value')
- if (!valueEl || valueEl.querySelector('div.share-note-icons')) return
+ if (!valueEl) return
+ if (valueEl.querySelector('div.share-note-icons')) {
+ injected = true
+ return
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private tryInjectShareIcons (activeFile: TFile, fieldKey: string, shareLink: string): boolean { | |
| let injected = false | |
| activeDocument.querySelectorAll(`div.metadata-property[data-property-key="${fieldKey}"]`) | |
| .forEach(propertyEl => { | |
| const valueEl = propertyEl.querySelector('div.metadata-property-value') | |
| const linkEl = valueEl?.querySelector('div.external-link') as HTMLElement | |
| if (linkEl?.innerText !== shareLink) return | |
| if (!valueEl || valueEl.querySelector('div.share-note-icons')) return | |
| // `data-property-key` already identifies the share_link row. Do not | |
| // require a brittle `div.external-link` exact text match - newer | |
| // Obsidian builds render URL properties as anchors/inputs/plain text. | |
| // If a value is present and clearly not our link, skip; otherwise inject. | |
| const hasAnyValue = !!(valueEl.textContent || '').trim() || !!valueEl.querySelector('input, a, div.external-link') | |
| if (hasAnyValue && !this.propertyValueMatchesShareLink(valueEl, shareLink)) { | |
| // Still allow injection when the row is our property key and the | |
| // displayed text is only the URL without hash, icons, etc. The | |
| // matcher already accepts base-URL prefixes; if it still fails the | |
| // property is showing unrelated content. | |
| return | |
| } | |
| private tryInjectShareIcons (activeFile: TFile, fieldKey: string, shareLink: string): boolean { | |
| let injected = false | |
| activeDocument.querySelectorAll(`div.metadata-property[data-property-key="${fieldKey}"]`) | |
| .forEach(propertyEl => { | |
| const valueEl = propertyEl.querySelector('div.metadata-property-value') | |
| if (!valueEl) return | |
| if (valueEl.querySelector('div.share-note-icons')) { | |
| injected = true | |
| return | |
| } | |
| // `data-property-key` already identifies the share_link row. Do not | |
| // require a brittle `div.external-link` exact text match - newer | |
| // Obsidian builds render URL properties as anchors/inputs/plain text. | |
| // If a value is present and clearly not our link, skip; otherwise inject. | |
| const hasAnyValue = !!(valueEl.textContent || '').trim() || !!valueEl.querySelector('input, a, div.external-link') | |
| if (hasAnyValue && !this.propertyValueMatchesShareLink(valueEl, shareLink)) { | |
| // Still allow injection when the row is our property key and the | |
| // displayed text is only the URL without hash, icons, etc. The | |
| // matcher already accepts base-URL prefixes; if it still fails the | |
| // property is showing unrelated content. | |
| return | |
| } |
🤖 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 `@src/main.ts` around lines 278 - 296, Update tryInjectShareIcons to set
injected = true when a matching div.share-note-icons element already exists
before skipping that property, so the method reports that the icons are present
and addShareIcons does not create redundant MutationObservers.
1 participant
Summary
This PR implements CSS file chunking for large files and unifies the CSS data structure to use an array format for both single files and multiple chunks.
Changes
splitCssIntoChunks()method to split large CSS files at rule boundaries (>500KB threshold)Array<{ url: string, hash: string }>for both single files and multiple chunksTextEncoderfor accurate UTF-8 encoding (fixesERR_CONTENT_LENGTH_MISMATCH)processCss()to properly handle emptycssResultarraysFiles modified:
src/api.ts: UpdateCheckFilesResultinterface to use array formatsrc/NoteTemplate.ts: Updatecssfield to use array formatsrc/note.ts: Add chunking logic and update CSS processingBenefits
Testing
Related Issues
Fixes issues with:
ERR_CONTENT_LENGTH_MISMATCHfor large CSS filesTechnical Details
CSS Chunking Algorithm
}) to avoid breaking CSS syntaxTextEncoderfor accurate byte size calculationsha1(chunkIndex-chunkContentHash-totalChunks)