Add --expand-links flag to fetch linked Confluence pages inline - #8
Add --expand-links flag to fetch linked Confluence pages inline#8calin-marian wants to merge 4 commits into
Conversation
Rewrite HTML tag stripper to preserve links (as "text (url)") and images (as "[image: url]") instead of stripping all tags. Add Atlassian Document Format (ADF) renderer for Jira Cloud descriptions, handling text with link marks, inlineCard/embedCard smart links, mentions, hard breaks, and media nodes. Fix formatJiraIssue to handle ADF object descriptions in addition to plain string descriptions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a great new feature for expanding Confluence links within Jira issues. The implementation is comprehensive, covering ADF and HTML parsing, recursive fetching with depth limiting, and deduplication.
My review has identified a few areas for improvement. The most critical issue is a recurring incorrect usage of std.ArrayList.append, which passes an extra allocator argument. This will likely cause compilation errors with current Zig versions. I've left several comments pointing out where this occurs.
Additionally, I've noted that in stripHtmlTags, potential allocation errors are silently ignored, which could lead to truncated output. In expandConfluenceLink, network and parsing errors are also handled silently, which could make debugging difficult for users. Improving the error handling in these areas would make the new feature more robust.
Finally, I've included a minor suggestion to improve code readability in one of the new utility functions.
Overall, this is a solid contribution. Addressing these points will ensure the new functionality is both correct and reliable.
| if (std.mem.eql(u8, entity, "nbsp")) { | ||
| result.append(allocator, ' ') catch {}; | ||
| } else if (std.mem.eql(u8, entity, "lt")) { | ||
| result.append(allocator, '<') catch {}; | ||
| } else if (std.mem.eql(u8, entity, "gt")) { | ||
| result.append(allocator, '>') catch {}; | ||
| } else if (std.mem.eql(u8, entity, "amp")) { | ||
| result.append(allocator, '&') catch {}; | ||
| } else if (std.mem.eql(u8, entity, "quot")) { | ||
| result.append(allocator, '"') catch {}; | ||
| } else { |
There was a problem hiding this comment.
There are a couple of issues with how result.append is being used in this function:
- The
appendmethod forstd.ArrayListdoes not take anallocatoras its first argument. The list already knows about its allocator from initialization. The calls should be in the formresult.append(item). - Using
catch {}silences potential allocation errors. Since this function can return an error (![]u8), it's better to usetry result.append(...)to propagate any memory allocation failures. This will prevent silent truncation of the output.
These issues are present on lines 92, 109, 111, 113, 115, 117, and 126 within this function.
if (std.mem.eql(u8, entity, "nbsp")) {
try result.append(' ');
} else if (std.mem.eql(u8, entity, "lt")) {
try result.append('<');
} else if (std.mem.eql(u8, entity, "gt")) {
try result.append('>');
} else if (std.mem.eql(u8, entity, "amp")) {
try result.append('&');
} else if (std.mem.eql(u8, entity, "quot")) {
try result.append('"');
} else {
There was a problem hiding this comment.
This is a false positive. Zig 0.15 uses an unmanaged ArrayList API where the allocator is passed per-call rather than stored in the struct. result.append(allocator, item) is the correct signature. See the Zig 0.15 std.ArrayList docs.
However, the related point about catch {} silently swallowing errors is valid — fixed in the base branch (1e12dd1), these now use try to propagate allocation failures.
| if (attrs == .object) { | ||
| if (attrs.object.get("url")) |url| { | ||
| if (url == .string) { | ||
| try urls.append(allocator, try allocator.dupe(u8, url.string)); |
There was a problem hiding this comment.
The append method for std.ArrayList does not take an allocator as its first argument. The list already knows about its allocator from initialization. The call should be try urls.append(...).
try urls.append(try allocator.dupe(u8, url.string));
There was a problem hiding this comment.
Same as above — append(allocator, item) is the correct Zig 0.15 unmanaged ArrayList API. No change needed.
| if (attrs == .object) { | ||
| if (attrs.object.get("href")) |href| { | ||
| if (href == .string) { | ||
| try urls.append(allocator, try allocator.dupe(u8, href.string)); |
There was a problem hiding this comment.
The append method for std.ArrayList does not take an allocator as its first argument. The list already knows about its allocator from initialization. The call should be try urls.append(...).
try urls.append(try allocator.dupe(u8, href.string));
There was a problem hiding this comment.
Same as above — append(allocator, item) is the correct Zig 0.15 unmanaged ArrayList API. No change needed.
| (trimmed.len == 1 or trimmed[1] == ' ' or trimmed[1] == '\t' or trimmed[1] == '\n')) | ||
| { | ||
| if (extractHtmlAttr(tag_inner, "href")) |href| { | ||
| try urls.append(allocator, try allocator.dupe(u8, href)); |
There was a problem hiding this comment.
The append method for std.ArrayList does not take an allocator as its first argument. The list already knows about its allocator from initialization. The call should be try urls.append(...).
try urls.append(try allocator.dupe(u8, href));
There was a problem hiding this comment.
Same as above — append(allocator, item) is the correct Zig 0.15 unmanaged ArrayList API. No change needed.
| visited.append(allocator, owned_id) catch { | ||
| allocator.free(owned_id); | ||
| return; | ||
| }; |
There was a problem hiding this comment.
The append method for std.ArrayList does not take an allocator as its first argument. The list already knows about its allocator from initialization. The call should be visited.append(owned_id).
visited.append(owned_id) catch {
allocator.free(owned_id);
return;
};
There was a problem hiding this comment.
Same Zig 0.15 API — visited.append(allocator, owned_id) is correct. No change needed.
| }; | ||
|
|
||
| // Fetch and display | ||
| const page_response = confluence.getPage(page_id) catch return; |
There was a problem hiding this comment.
This function uses catch return to handle errors, which means failures will be silent. This can be confusing for the user when a link fails to expand. It would be more user-friendly to log an error message to stderr before returning, so the user knows why an operation failed. This applies to all catch return and if...else return error handling patterns in this function.
const page_response = confluence.getPage(page_id) catch |err| {
std.debug.print("Error: Failed to fetch Confluence page ID {s}: {any}\n", .{page_id, err});
return;
};
There was a problem hiding this comment.
Fixed in 9d86c3f. expandConfluenceLink now logs warnings with the page ID and error before returning on fetch or format failures.
| const pos = std.mem.indexOf(u8, url, marker) orelse return null; | ||
| const start = pos + marker.len; | ||
| var end = start; | ||
| while (end < url.len and url[end] >= '0' and url[end] <= '9') : (end += 1) {} |
There was a problem hiding this comment.
For better readability and to make the intent clearer, you could use std.ascii.isDigit to check if a character is a digit.
while (end < url.len and std.ascii.isDigit(url[end])) : (end += 1) {}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 490f1173b6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (content_val == .array and depth < max_stack) { | ||
| stack[depth] = .{ .items = content_val.array.items, .idx = 0 }; | ||
| depth += 1; | ||
| continue; // process children before emitting newline |
There was a problem hiding this comment.
The unconditional continue after pushing content onto the stack skips the block-node newline logic for any node that has children, which is the common shape for paragraph, heading, and list nodes in Jira ADF. As a result, multi-paragraph descriptions are flattened into a single run-on line (e.g., two paragraphs render as FirstSecond), making issue output significantly less readable whenever the description is an ADF object.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the base branch (1e12dd1, rebased into this PR). The DFS stack now tracks is_block per frame and emits newlines when block-node frames are exhausted.
Fix off-by-one in extractHtmlAttr loop bound and use std.ascii.isWhitespace for consistent whitespace checks. Propagate allocation errors in stripHtmlTags instead of silently swallowing them. Fix writeAdfText to emit newlines after block-level nodes by tracking is_block in the DFS stack, so paragraphs and headings are properly separated. Add [image] fallback for external media nodes missing a URL. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When viewing a Jira issue with --expand-links[=N], extract URLs from the ADF description and fetch the linked Confluence pages inline. Supports recursive depth and deduplication of visited pages. Add URL extraction utilities for both ADF and HTML content.
Use std.ascii.isDigit for readability in extractConfluencePageId. Log warnings when Confluence page fetch or format fails instead of silently returning.
490f117 to
9d86c3f
Compare
1 participant
Summary
--expand-links[=N]flag tojira issuethat extracts URLs from the ADF description and recursively fetches linked Confluence pages inlineDepends on #7.
Test plan
jira issue <key> --expand-linksfetches and displays linked Confluence pages after the issue--expand-links=2recurses into child page links🤖 Generated with Claude Code