Repository navigation
feat(jira): load attachment images outside issue Markdown - #28631
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Out of Scope Changes check |  |
The PR includes unrelated Jira project-metadata IPC changes. src/main/ipc/jira.ts removes four project-metadata handlers, while src/main/ipc/jira-project-metadata.ts and its tests add their regist… |
Remove the project-metadata IPC extraction and its dedicated tests from this PR, or provide a direct technical dependency that is required for the #10724 image-loading implementation. |
| Docstring Coverage | |
Docstring coverage is 7.69% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 130 functions across 77 files. (1 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check The PR meets the coding requirements in #10724. It registers the privilegedorca-jira-imagescheme and permits the required image sources in desktop and mobile CSP. Grants and main-process reads kee…Title check The title clearly and concisely describes the main change: loading Jira attachment images outside issue Markdown. Description check The description is comprehensive and covers the required sections, including user impact, implementation, rationale, linked issue, visual proof, testing, compatibility, security, and known limitations…
Full details: Out of Scope Changes check
-
Explanation
The PR includes unrelated Jira project-metadata IPC changes.
src/main/ipc/jira.tsremoves four project-metadata handlers, whilesrc/main/ipc/jira-project-metadata.tsand its tests add their registration in a separate module. These changes do not implement attachment references, authenticated image loading, image transfer, image caching, or image compatibility for#10724.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 7.69% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 130 functions across 77 files. (1 skipped: 1 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
e69aedc6-a48f-4ae6-9702-0d8a4bd58675
📥 Commits
Reviewing files that changed from the base of the PR and between 1c274cc and cfbc768f9eaf6433051026bbf2716020860b1689.
⛔ Files ignored due to path filters: 1
src/shared/rpc-contract/rpc-params-catalog.generated.tsis excluded by!**/*.generated.*
📒 Files selected for processing: 106
config/scripts/generate-bundled-skill-guides.mjsconfig/scripts/mobile-web-app-render.test.mjsconfig/scripts/mobile-web-app-source-control-render.test.mjsdocs/site/content/docs/review/jira.mdxmobile/modules/orca-mobile-web-shell/android/src/main/java/expo/modules/orcamobilewebshell/MobileWebShellCsp.ktmobile/modules/orca-mobile-web-shell/android/src/test/java/expo/modules/orcamobilewebshell/MobileWebShellCspTest.ktmobile/modules/orca-mobile-web-shell/ios/MobileWebShellCsp.swiftmobile/modules/orca-mobile-web-shell/tests/MobileWebShellChecks.swiftresources/skills/current-manifest.jsonskill-guides/jira-tickets.mdskill-guides/orca-jira.mdskill-stubs/jira-tickets.mdskill-stubs/orca-jira.mdskills/jira-tickets/SKILL.mdskills/orca-jira/SKILL.mdsrc/cli/args.tssrc/cli/bundled-skill-guides.tssrc/cli/command-body-input.tssrc/cli/handler-group-manifest.tssrc/cli/handlers/jira-write.tssrc/cli/handlers/jira.tssrc/cli/index-jira.test.tssrc/cli/jira-command-surface.test.tssrc/cli/jira-format.tssrc/cli/jira-handler-group.tssrc/cli/jira-target.tssrc/cli/linear-request-builders.tssrc/cli/reference-handler-group.tssrc/cli/specs/index.tssrc/cli/specs/jira.tssrc/main/ipc/jira.tssrc/main/jira/__mocks__/jira-client-module.tssrc/main/jira/attachment-discovery.tssrc/main/jira/attachment-image-bytes.tssrc/main/jira/attachment-image-cache.tssrc/main/jira/attachment-image-grants.tssrc/main/jira/attachment-image-protocol.tssrc/main/jira/attachment-image-read.test.tssrc/main/jira/attachment-image-read.tssrc/main/jira/attachment-image-references.test.tssrc/main/jira/attachment-images.tssrc/main/jira/attachment-meta.tssrc/main/jira/auth-http-fixture.tssrc/main/jira/authenticated-request-image-limit.test.tssrc/main/jira/authenticated-request.tssrc/main/jira/client-scoped-token.test.tssrc/main/jira/client.test.tssrc/main/jira/client.tssrc/main/jira/cloud-gateway.tssrc/main/jira/credential-verification.tssrc/main/jira/gateway-base-url.tssrc/main/jira/jira-agent-read.test.tssrc/main/jira/jira-api-error.tssrc/main/jira/jira-issue-comments.tssrc/main/jira/jira-issue-create-metadata.tssrc/main/jira/jira-issue-media.tssrc/main/jira/jira-issue-mutations.test.tssrc/main/jira/jira-issue-mutations.tssrc/main/jira/jira-issue-read.tssrc/main/jira/jira-issue-search.tssrc/main/jira/jira-project-queries.tssrc/main/jira/jira-read-failure.tssrc/main/jira/jira-transition-queries.tssrc/main/jira/site-credential-store.tssrc/main/jira/site-identity.tssrc/main/runtime/rpc/methods/jira-attachment-image.test.tssrc/main/runtime/rpc/methods/jira-attachment-image.tssrc/main/runtime/rpc/methods/jira.test.tssrc/main/runtime/rpc/methods/jira.tssrc/main/runtime/runtime-jira-commands.tssrc/main/runtime/runtime-rpc/runtime-rpc-mobile-paired-desktop-methods.tssrc/main/startup/cli-command-names.tssrc/main/startup/main-process-preflight.tssrc/main/startup/main-process-ready-foundation.tssrc/preload/api/jira-api.tssrc/preload/api/jira-bridge.tssrc/renderer/index.htmlsrc/renderer/src/components/JiraIssueWorkspace.tsxsrc/renderer/src/components/jira-attachment-image.test.tsxsrc/renderer/src/components/jira-attachment-image.tsxsrc/renderer/src/components/jira-connect-credential-guide.tsxsrc/renderer/src/components/jira-connect-dialog.tsxsrc/renderer/src/components/jira-connect-scoped-token.test.tsxsrc/renderer/src/components/jira-issue-markdown.tsxsrc/renderer/src/components/jira-issue-workspace-content.tsxsrc/renderer/src/components/sidebar/CommentMarkdown.tsxsrc/renderer/src/components/sidebar/MarkdownImageLightbox.tsxsrc/renderer/src/components/sidebar/comment-markdown-element-renderers.tsxsrc/renderer/src/hooks/ipc-events/app-lifetime-ipc-bridge.tssrc/renderer/src/i18n/locales/en.jsonsrc/renderer/src/runtime/runtime-jira-client.test.tssrc/renderer/src/runtime/runtime-jira-client.tssrc/renderer/src/runtime/runtime-jira-connection-status.test.tssrc/renderer/src/runtime/runtime-jira-connection-status.tssrc/renderer/src/runtime/runtime-jira-detail-client.tssrc/renderer/src/runtime/runtime-jira-image-cache.test.tssrc/renderer/src/runtime/runtime-jira-image-cache.tssrc/renderer/src/runtime/runtime-jira-image-client.test.tssrc/renderer/src/runtime/runtime-jira-image-client.tssrc/renderer/src/store/slices/jira-connection-actions.tssrc/shared/jira-agent-capability.tssrc/shared/jira-attachment-image.tssrc/shared/jira-image-stream-frame.tssrc/shared/jira-types.tssrc/shared/protocol-version.tssrc/shared/rpc-contract/jira-params.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.
nwparker
force-pushed
the
nwparker/jira-attachment-protocol
branch
from
October 14, 2026 10:17
8804a66 to
570a949
Compare
# Conflicts: # config/focus-setting-read-baseline.txt # src/main/runtime/rpc/methods/jira.ts # src/preload/api/jira-bridge.ts # src/renderer/src/components/JiraIssueWorkspace.tsx # src/renderer/src/components/jira-issue-workspace-request-lifetime.test.tsx # src/renderer/src/runtime/runtime-jira-client.ts # src/renderer/src/runtime/runtime-jira-detail-client.test.ts # src/renderer/src/runtime/runtime-jira-detail-client.ts # src/shared/protocol-version.ts # src/shared/rpc-contract/rpc-params-catalog.generated.ts
10 tasks done
nwparker
added a commit
that referenced
this pull request
Oct 14, 2026
## ELI5 Keep the test suite working with the Jira features that have just landed. There is no visible app change. ## What Changed The host-picker test now expects the host-qualified selection the parent already accepts. The startup fixture provides Electron's protocol handler and verifies that ready installs the Jira image scheme. Jira edit tests use exported wrappers with module-local setters that are cleared on unmount, preserving the issue/source switch tests without the banned mutable function export. The WSL storage inventory classifies Jira list columns, linked-view choice and prompt templates as device preferences, matching their keys and the existing Linear policy. Skill-guide tests check the actual generated discovery projection rather than assuming it is shorter than every full guide or uses one fixed phrase. The cache test follows the headless workflow's deliberate removal of its push trigger; cache-warmer checks remain intact. ## Why Final CI found six stale integration assumptions after the Jira batch. Updating the fixtures and inventory preserves the landed behavior and the underlying checks. Changing production code, padding the Jira guide, or restoring an intentionally removed workflow trigger would change the product to satisfy incorrect test assumptions. ## Linked Issue Follow-up to #28622, #28631 and #28592. The existing headless trigger policy was introduced in #28664. ## Visual Proof N/A — tests, test fixtures and storage inventory declarations only; no visual or interaction change. ## Testing - [x] Automated tests updated. - [x] 196 focused tests across nine files pass on macOS, covering every failed shard assertion, fixture consumers and the headless workflow policy. - [x] Node and web typechecks pass; all seven changed-code quality stages, formatting and diff checks pass. - Reproduced five failing assertions in startup/export/storage tests and the skill projection failure before the repairs. The host-picker owner also reproduced the stale selection failure before correcting it. - Final full CI will run on this PR. The prior Ubuntu 22.04 persistence job crashed in the native watcher build before persistence tests ran; this change does not claim a Linux release, live Jira or real SSH pass. ## Review Independent source review found no issues; all six reviewed file hashes and the complete diff match committed head `aa703eb50147ba95646cb82f10b6d86b4f890fc4`. Risk: low. No authentication, network, protocol implementation, workflow, renderer behavior, remote messages or migration rewrite logic changes. ## Agent skill upstream boundary - [x] Not applicable; no upstream skill-installer source or artifacts copied. ## Notes The three storage keys contain device-level preferences, without workspace paths or identifiers. Tests preserve host disambiguation and issue/source switching. Platform, SSH and provider behavior remains in the existing implementations. ## Checklist - [x] Small and focused. - [x] Explained the before/after, mechanism and choice of approach. - [x] Visual proof marked N/A with a reason. - [x] Self-reviewed for correctness and cleanup. - [x] Cross-platform and SSH impact considered. - [x] Focused tests, types, formatting and quality checks pass; full CI covers the broader build.
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.
ELI5
Jira screenshots load separately from issue text, so descriptions and comments stay small. Images that exceeded the previous inline limit can appear in the issue and open in the existing lightbox. Reconnecting the issue's host retries an image already on screen.
What Changed
Why
Embedding base64 images retained megabytes in issue caches and enlarged text transfers. Separate downloads begin when an image mounts and provide cancellation and byte limits. Plain attachment URLs would put authentication handling in the renderer and would not support remote execution hosts.
This extends Frank (@lifefloating)'s merged #8938, including its fixes with Brennan Benson, using the existing attachment discovery.
Linked Issue
Fixes #10724
Visual Proof
Final hidden macOS Electron/CDP proof at
91b449eec95, composed onto landed main624c4148cc88, passes 48 checks with nine screenshots. It runs production components, detail/image clients, cache, mutation queue and binary decoder with a synthetic store, RPC endpoint and subscriptions. Checks cover recovery, cancellation, stale pairing callbacks, lightbox, 48 description/comment images, byte bounds, title/hydration preservation, required transition comments and obsolete timeline reads.Recovered image after reconnect, retaining the saved title and hydrated fields:
Required transition comment immediately appears in the refreshed timeline:
The six existing images below retain the earlier fixture evidence. They are historical; the final branch is verified by the checks above. The first fixture exercised the authenticated local protocol and remote handler with a fixture token held in the main process. The later recovery fixture ran production renderer clients with synthetic transports.
Before: the 3,240,054-byte image exceeds the previous 2 MiB inline limit.
Local authenticated image:
Remote binary image:
Existing lightbox:
Mounted image before recovery:
After recovery, with saved title, hydrated fields and comment preserved:
Testing
I manually tested these changes locally
Automated tests added/updated, or explained why not below
Final
91b449eec95: 410 focused tests across 54 files pass, covering authentication, bounded transfer/cache behavior, legacy compatibility, cancellation, reconnect, captured owners, editing and timeline lifetimes.Exact branch node and renderer typechecks pass with unchanged source hashes.
All seven changed-code quality stages pass across 58 files; full-file lint passes for 98 files and formatting for 100. Anti-slop, the 1,106-read routing ratchet, regenerated RPC catalog and diff checks pass. Two inherited routing ceilings were lowered to match current main.
Fresh hidden Electron/CDP proof passes 48/48 checks. All 226 prepared/source hashes stayed unchanged, the renderer reported no page errors, and the owned background app processes were stopped with both ports clear.
Latest CI snapshot: static/typechecks, mobile verify and bundle, relay, wire compatibility, Linux/Windows packaging and Windows serve checks pass; five test shards remain pending.
Earlier headless mobile-web/CSP checks passed 32 tests on
354020e7d04; those are historical. Local checks do not claim live Jira/auth/socket/SSH, physical mobile, a full build or Linux/Windows execution. Image mounts trigger fetching; viewport laziness is not implemented. Padded PNGs verify encoded byte bounds, not decoded pixel memory.Review
Independent final source review finds no concrete defect and rates the scoped change Low risk. It covers authentication, transfer isolation, cancellation, bounded caches, capability exports/order, older-peer behavior and preservation of landed issue actions, transition comments, board filters and timeline lifetimes. All four addressed review threads remain resolved. Independent artifact review also verifies all 48 assertions, 226 source hashes, fixture/driver checksums, nine screenshots and cleanup; no blocking defect found.
Agent skill upstream boundary
Notes
CLI/scoped-auth prerequisites and the assignee/board changes have landed; this branch composes actual main
624c4148cc88and contains no separate CLI/auth feature changes. The workspace differs from that main only by its captured image-owner prop.Pending legacy inline reads and cancellable byte reads use separate request domains; completed image bytes share the bounded cache. Both request orders and completed-cache reuse are tested. Simultaneous cold legacy/new reads may download the same attachment twice. New binary frames require negotiated support, and the existing five mobile Jira read methods remain alongside attachment-image reads.
External token-file reload (#28594) does not immediately revoke cached host attachment bytes or rendered Blob URLs. Same-process disconnect/reconnect clears the host attachment cache.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (focused checks and exact node/web compilers pass; a full local suite/build was not run and CI test shards remain pending)