feat: Obsidian-style image width and local image upload#19
Conversation
|
Warning Review limit reached
More reviews will be available in 10 minutes and 58 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. 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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds a ChangesLocal Image Upload and Obsidian Sizing
Sequence Diagram(s)sequenceDiagram
participant syncFile as syncFile (sync.ts)
participant preprocessImages as preprocessImages
participant disk as Disk (baseDir)
participant context as ConversionContext
participant imageConverter as image converter
syncFile->>context: init localImages = new Map()
syncFile->>preprocessImages: preprocessImages(ast, context, dirname)
loop each local image node in AST
preprocessImages->>disk: readFile(url)
disk-->>preprocessImages: bytes
preprocessImages->>preprocessImages: MD5 hash → hashedFilename
preprocessImages->>context: attachments.set(hashedFilename, {data, contentType})
preprocessImages->>context: localImages.set(url, hashedFilename)
end
preprocessImages-->>syncFile: done
syncFile->>imageConverter: convert image node
imageConverter->>context: localImages.get(url) → hashedFilename
imageConverter-->>syncFile: ac:image XML with ri:attachment + ac:width/ac:height
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/sync.ts (1)
54-67: 💤 Low valueComment could be more accurate.
Line 65's comment says "Upload local image files" but
preprocessImagesonly reads and registers them incontext.attachments. The actual upload happens later at line 132 viauploadAttachments.📝 Suggested comment fix
- // Upload local image files referenced by the markdown (populates context.attachments + localImages) + // Read and register local image files referenced by the markdown (populates context.attachments + localImages) await preprocessImages(ast, context, dirname(absolutePath))🤖 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/sync.ts` around lines 54 - 67, The comment for the preprocessImages function call is inaccurate. It states that preprocessImages "Upload local image files" but the function actually only reads and registers them in context.attachments and context.localImages. Update the comment above the preprocessImages call to clarify that it prepares and registers local image files rather than uploading them, and note that the actual upload happens later during the uploadAttachments call.test/images.test.ts (1)
88-101: ⚡ Quick winAdd converter output assertion for data: URIs.
This test verifies that preprocessing skips data: URIs, but it doesn't verify what the converter actually outputs for them. Given the data: URI handling issue in
src/elements/image.ts, this test should also assert the expected converter behavior.🧪 Suggested test extension
expect(context.attachments.size).toBe(0) expect(context.localImages.size).toBe(0) expect(warn).not.toHaveBeenCalled() + + // Verify converter output (should skip or handle data: URIs appropriately) + const xml = convert(ast, context) + // TODO: Update this assertion once data: URI handling is fixed in the converter + expect(xml).not.toContain('ri:attachment') } finally {🤖 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 `@test/images.test.ts` around lines 88 - 101, The test for skipping data: URIs in the test function starting at line 88 verifies that no attachments or warnings are created, but it doesn't assert what the converter actually outputs for data: URIs. After the preprocessImages call in this test, add an assertion that checks the actual converter output by examining the processed AST or the result to verify that data: URIs are handled correctly in the converted content, ensuring the converter properly preserves or processes these inline data URIs as expected.
🤖 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/elements/image.ts`:
- Around line 5-26: The image converter registered with registerConverter<Image>
does not handle data: URIs correctly. Currently, the isRemote check only detects
http:// and https://, so data: URIs fall through to the local file path logic,
which produces invalid markup like ri:attachment with the full data URI as a
filename. Add a check to detect data: URIs (starting with "data:") before or
within the isRemote condition to prevent them from reaching the local attachment
handling. Based on PR objectives that data: URIs should be "left untouched,"
either skip rendering them (return empty string or null) or return them as-is
rather than wrapping them in ri:attachment tags.
---
Nitpick comments:
In `@src/sync.ts`:
- Around line 54-67: The comment for the preprocessImages function call is
inaccurate. It states that preprocessImages "Upload local image files" but the
function actually only reads and registers them in context.attachments and
context.localImages. Update the comment above the preprocessImages call to
clarify that it prepares and registers local image files rather than uploading
them, and note that the actual upload happens later during the uploadAttachments
call.
In `@test/images.test.ts`:
- Around line 88-101: The test for skipping data: URIs in the test function
starting at line 88 verifies that no attachments or warnings are created, but it
doesn't assert what the converter actually outputs for data: URIs. After the
preprocessImages call in this test, add an assertion that checks the actual
converter output by examining the processed AST or the result to verify that
data: URIs are handled correctly in the converted content, ensuring the
converter properly preserves or processes these inline data URIs as expected.
🪄 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
Run ID: c755d280-cdcd-48f3-b876-20430ecc6e30
📒 Files selected for processing (7)
src/elements/image.tssrc/images/preprocess.tssrc/sync.tssrc/types.tstest/converter.test.tstest/images.test.tstest/sync.test.ts
pilat
force-pushed
the
feat/image-width-local-upload
branch
from
07f23a6 to
ab0aca9
Compare
pilat
force-pushed
the
feat/image-width-local-upload
branch
from
09a83f2 to
1132ef1
Compare
1 participant
Why
Local images referenced from markdown rendered as broken in Confluence. The converter emitted an
<ri:attachment>reference, but nothing ever uploaded the file — so only remote (http(s)) images and rendered mermaid diagrams actually worked end-to-end. There was also no way to control how large an image displays.What
sets width,sets width and height — emitted asac:width/ac:heighton<ac:image>. Only a trailing|<digits>or|<digits>x<digits>segment is treated as a size; anything else (e.g.) stays verbatim in the alt text, so legitimate pipes keep working. No parser extension needed — the data already lives innode.alt.references.How it behaves
<stem>-<md5>.<ext>). Confluence attachments are flat per page, so two different files both namedpic.pngwould otherwise overwrite each other; hashing also dedupes the same image referenced twice into a single upload.data:URIs are untouched.Notes
Summary by CodeRabbit
New Features
or).Tests