Repository navigation
fix(jira): render issue description/comment images with lightbox - #8938
Conversation
📝 WalkthroughWalkthroughJira ADF rendering now supports media resolvers, external image URLs, unresolved placeholders, and inline media. Jira attachment helpers parse metadata, extract referenced IDs, download eligible raster images, and map them to Markdown. Issue and comment loading passes attachment-aware options into Markdown conversion. Renderer markdown images now open in a portal lightbox, coordinate active-lightbox state, prevent underlying sheet dismissal, and include localized labels. Task-page Escape handling and related tests were updated. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 3
🧹 Nitpick comments (2)
src/renderer/src/components/sidebar/MarkdownImageLightbox.tsx (2)
78-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded colors instead of design tokens.
This portal uses raw Tailwind arbitrary-value classes (
bg-black/80,border-white/20,bg-black/55,bg-black/75) rather than tokens fromsrc/renderer/src/assets/main.css. As per coding guidelines, "All UI work must followdocs/STYLEGUIDE.md, use tokens fromsrc/renderer/src/assets/main.css, and use shadcn primitives fromsrc/renderer/src/components/ui/." The custom portal is understandable given the documented Radix pointer-events conflict, but the color/opacity values themselves could still reference existing tokens (or a themed overlay token) instead of hardcoded black/white mixes so dark/light theme changes propagate consistently.Source: Coding guidelines
93-103: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNo focus trap inside the modal.
Only the close button receives initial focus; Tab/Shift+Tab are never intercepted, so keyboard users can tab past the single focusable element into content hidden behind the opaque backdrop. Consider trapping focus on the close button (or a small focus-trap loop) while the overlay is mounted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5d51eb89-5ffc-4ab4-b44b-4c22d12bfefe
📥 Commits
Reviewing files that changed from the base of the PR and between 1ba71d2 and 9868d9b39b5c445ec3cda36354027be310db1b55.
📒 Files selected for processing (20)
src/main/jira/adf-markdown.test.tssrc/main/jira/adf-markdown.tssrc/main/jira/attachment-images.test.tssrc/main/jira/attachment-images.tssrc/main/jira/client.tssrc/main/jira/issues.test.tssrc/main/jira/issues.tssrc/renderer/src/components/JiraIssueWorkspace.tsxsrc/renderer/src/components/TaskPage.tsxsrc/renderer/src/components/sidebar/CommentMarkdown.test.tsxsrc/renderer/src/components/sidebar/MarkdownImageLightbox.test.tsxsrc/renderer/src/components/sidebar/MarkdownImageLightbox.tsxsrc/renderer/src/components/sidebar/comment-markdown-element-renderers.tsxsrc/renderer/src/components/sidebar/markdown-image-lightbox-state.tssrc/renderer/src/components/ui/sheet.tsxsrc/renderer/src/i18n/locales/en.jsonsrc/renderer/src/i18n/locales/es.jsonsrc/renderer/src/i18n/locales/ja.jsonsrc/renderer/src/i18n/locales/ko.jsonsrc/renderer/src/i18n/locales/zh.json
| return (attrs: JiraAdfMediaAttrs): string | null => { | ||
| const alt = attrs.alt?.trim() || 'Image' | ||
| if (attrs.url && /^https?:\/\//i.test(attrs.url)) { | ||
| return `` | ||
| } | ||
|
|
||
| if (attrs.alt?.trim()) { | ||
| const matches = byFilename.get(attrs.alt.trim().toLowerCase()) | ||
| if (matches && matches.length > 0) { | ||
| const stillQueued = matches.find((image) => queue.some((entry) => entry.id === image.id)) | ||
| const resolved = take(stillQueued ?? matches[0]) | ||
| if (resolved) { | ||
| return resolved | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Why: ADF media IDs are Media Service UUIDs, not attachment IDs. Pair remaining | ||
| // inline images to downloaded attachments in rendered/document order. | ||
| return take(queue.shift()) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Cache inferred attachments by ADF media UUID.
The resolver ignores attrs.id after consuming an attachment. If the same media UUID appears again without alt text, it consumes the next queued attachment or returns null, producing a wrong or missing image. Reuse the first resolution through a Map<string, string> keyed by attrs.id.
|
Taking a look! |
ef919e0 to
0ab218c
Compare
|
Thanks for the contribution — I rebased this onto current I replaced the custom lightbox portal and Sheet-wide dismissal state with the shared modal dialog primitive, which provides focus trapping, Escape ordering, and focus restoration while using design-system surfaces. Attachment resolution now scans the full metadata set before limiting, fetches only IDs referenced by rendered Jira content, reuses repeated media UUID mappings, respects an aggregate byte budget, uses same-origin provider content URLs for Cloud and Server/DC, and streams image-bearing detail/comment payloads in bounded chunks for remote-runtime/SSH use. Jira comments also keep their compact typography and opt into image expansion explicitly. Validation: 89 focused tests, full typecheck, touched-file lint, React checks, max-lines checks, and localization checks pass. The repo-wide lint still reports two existing switch-exhaustiveness errors outside this PR. |
Thanks a lot for the rebase and the maintainer follow-up — really appreciate you taking the time to clean this up! |
|
I wanted to ask when this PR can be merged — I’m waiting to use this Jira feature. If there’s anything I need to adjust, I can do it now. Thanks! |
|
Thanks for this — the ADF media parsing, the lightbox component, and the We did a deep review pass and found enough issues that round-tripping them as individual review comments would be slow. Summary: Correctness
Performance
Design note (not blocking this PR) Inlining bytes as Next step To keep this moving we'll push the fixes directly to this branch rather than sending you round after round of comments. Expect that within 24 hours, and happy to walk through any of it afterwards. (If branch permissions block a direct push, we'll open a PR into your branch instead.) One question in the meantime: has this been exercised against a real Jira issue with pasted screenshots? Everything routes through that |
|
Follow-up for protocol-handler transport rewrite: #10724 |
AmethystLiang
left a comment
AmethystLiang
left a comment
There was a problem hiding this comment.
Approved after plan + code review loops.
Implementation addresses the original review findings:
- media↔attachment pairing no longer re-emits consumed images
- Server/DC comment attachment lookup uses apiBasePath
- markdown-safe external URL destinations (not encodeURI alone)
- wider HTML discovery + Option A multi same-filename alt fallback
- downloads concurrent outside the Jira API semaphore
- main-process attachment data-URL cache with singleflight, lower caps, clear on disconnect/clearToken
- post-map resolution warn with attachment-only stats
Follow-up protocol-handler rewrite: #10724
AmethystLiang
left a comment
AmethystLiang
left a comment
There was a problem hiding this comment.
Approved after plan + code review loops.
Implementation addresses the original review findings:
- media↔attachment pairing no longer re-emits consumed images
- Server/DC comment attachment lookup uses apiBasePath
- markdown-safe external URL destinations (not encodeURI alone)
- wider HTML discovery + Option A multi same-filename alt fallback
- downloads concurrent outside the Jira API semaphore
- main-process attachment data-URL cache with singleflight, lower caps, clear on disconnect/clearToken
- post-map resolution warn with attachment-only stats
Follow-up protocol-handler rewrite: #10724
Jira ADF media nodes were dropped when converting descriptions/comments to Markdown, so screenshots never appeared in the Tasks drawer. Download image attachments with authenticated Jira API access, embed them as data URLs on issue/comment detail loads, and add a viewport-centered lightbox. Closing with X/Esc only dismisses the preview, not the issue sheet.
Jira issue comments still used compact markdown, so screenshots rendered but could not expand. Use the document renderer for comment bodies, add a regression test for the expand control, and sync MarkdownImageLightbox locale keys.
Address PR review findings: correct media-attachment pairing, Server/DC attachment lookup base path, markdown-safe external URLs, wider HTML discovery with gated alt fallback, concurrent downloads outside the API semaphore, and a main-process attachment data-URL cache with lower caps.
Fix discovery so repeated alts (image.png) get distinct attachment ids, flush resolution warns after ADF mapping using attachment-only stats, clear attachment cache on clearToken with epoch-guarded singleflight, and add Server comment path plus release-before-binary regression tests.
Only needingCount determines whether to skip the attachment metadata request — htmlIds alone cannot produce a download without needing media. Add type annotation for mediaAttrs for clarity.
d407be0 to
bb30356
Compare
Load Jira attachment images separately from issue Markdown so descriptions and comments stay small while screenshots load lazily and open in the existing lightbox. Downloads begin when an image mounts, capture its owning host, require negotiated image support, and use bounded authenticated byte streams and caches. Older clients retain their inline-image handlers. Keep legacy and cancellable downloads in separate in-flight domains so cancelling a new image reader cannot abort a legacy reader; completed bytes share the bounded cache. Preserve the landed assignee, board, transition/comment queue and request lifetimes. Move unchanged project metadata IPC handlers into their domain module and tighten two inherited focus-check ceilings without changing behavior. Validation:410 tests/54 suites; all changed-source quality stages, lint, format, routing and contract checks; exact final node/web types; independent source review;48/48 fresh hidden Electron checks with226 stable source hashes and nine inspected screenshots. Synthetic transports exercise production clients and UI. Current CI static checks, mobile bundle, wire compatibility, Linux/Windows packaging and Windows serve pass; five test shards were pending at review. Extends the existing attachment work credited to Frank (@lifefloating) and Brennan Benson in PR #8938; existing contribution credits and media remain in this PR.
…blyai#8938) * fix(jira): render issue images and open them in a lightbox Jira ADF media nodes were dropped when converting descriptions/comments to Markdown, so screenshots never appeared in the Tasks drawer. Download image attachments with authenticated Jira API access, embed them as data URLs on issue/comment detail loads, and add a viewport-centered lightbox. Closing with X/Esc only dismisses the preview, not the issue sheet. * fix(jira): open comment images in the same lightbox as description Jira issue comments still used compact markdown, so screenshots rendered but could not expand. Use the document renderer for comment bodies, add a regression test for the expand control, and sync MarkdownImageLightbox locale keys. * fix(jira): harden inline image handling * fix(jira): harden inline image discovery, escaping, and downloads Address PR review findings: correct media-attachment pairing, Server/DC attachment lookup base path, markdown-safe external URLs, wider HTML discovery with gated alt fallback, concurrent downloads outside the API semaphore, and a main-process attachment data-URL cache with lower caps. * fix(jira): Option A multi-same-name attachments and post-map media warns Fix discovery so repeated alts (image.png) get distinct attachment ids, flush resolution warns after ADF mapping using attachment-only stats, clear attachment cache on clearToken with epoch-guarded singleflight, and add Server comment path plus release-before-binary regression tests. * fix(jira): simplify comment media request skip condition Only needingCount determines whether to skip the attachment metadata request — htmlIds alone cannot produce a download without needing media. Add type annotation for mediaAttrs for clarity. --------- Co-authored-by: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Co-authored-by: Jinjing <6427696+AmethystLiang@users.noreply.github.com>
Summary
Jira issue screenshots no longer disappear in the Tasks drawer. Inline images in descriptions and comments are resolved from ADF media/attachments and can open in a viewport-centered lightbox (Esc/X closes only the preview, not the issue sheet).
Closes #8934
Screenshots
(Please attach captures from a real Jira issue with pasted screenshots.)
Testing
pnpm lint(0 errors; existing unrelated warnings only)pnpm typecheckpnpm test(branch-related suites green; full suite had one unrelated flakypty-connectioncase that passed on re-run)pnpm buildadf-markdown/attachment-images/issuesunit coverage for media → data URL conversionMarkdownImageLightbox+ Sheet interactionCommentMarkdownexpand-control regression for document variantAI Review Report
Reviewed the ADF media path, authenticated attachment download, data-URL embedding limits, and lightbox/Sheet dismissal interaction.
Main risks checked:
Cross-platform: no new shortcuts, shell commands, or path separators. Image fetch uses existing authenticated Jira REST client (main process). Lightbox is renderer UI over Sheets; behavior is the same on macOS/Linux/Windows Electron. No platform-specific keyboard accelerators added (Esc only).
Security Audit
data:image URLs for trusted markdown rendering only.Notes
variant="document"so they share the description lightbox path.MarkdownImageLightboxlocale keys were synced foren/es/ja/ko/zh.pnpm buildnot run in this pass; worth running before merge if CI does not cover it.ELI5
Screenshots in Jira issue descriptions and comments vanished in the Tasks drawer. Images load again and open in a lightbox without closing the whole issue panel.