Improve mobile pull request UI#899
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
arul28
changed the title
📝 WalkthroughWalkthroughChangesPR detail UI
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 (1)
apps/ios/ADE/Views/PRs/PrGitHubDescriptionParser.swift (1)
20-111: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache parsed PR description blocks and compiled regexes.
PrThreadDescriptionCardrebuildsblocks = parsePrGitHubDescriptionBlocks(text)from its initializer after SwiftUI state changes, so scrolling/other detail-screen updates can re-parse the description. Store the parsed blocks in the card instead, and hoist the repeatedNSRegularExpressionpatterns (details,summary, code blocks, fenced code, script/style, tag normalization, blank-line collapse) into static constants so body updates don’t recompile them.🤖 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 `@apps/ios/ADE/Views/PRs/PrGitHubDescriptionParser.swift` around lines 20 - 111, Cache the result of parsePrGitHubDescriptionBlocks in PrThreadDescriptionCard rather than rebuilding it during state-driven updates. Hoist the detailsRegex and summaryRegex patterns in parsePrGitHubDescriptionBlocks, along with the code-block, fenced-code, script/style, tag-normalization, and blank-line-collapse regexes used by prProtectMarkdownCode and normalization helpers, into reusable static constants so they are compiled once.
🤖 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 `@apps/ios/ADE/Views/PRs/PrDetailScreen.swift`:
- Around line 619-622: Restore the matched-geometry transition in PrDetailView
by applying adeNavigationZoomTransition with the PR’s transition identifier and
transitionNamespace, matching the PrRowCard source configuration. If the
transition is intentionally removed instead, remove transitionNamespace from
PrDetailView’s initializer and the PR navigation route, and remove the
corresponding PrRowCard transition parameters.
---
Nitpick comments:
In `@apps/ios/ADE/Views/PRs/PrGitHubDescriptionParser.swift`:
- Around line 20-111: Cache the result of parsePrGitHubDescriptionBlocks in
PrThreadDescriptionCard rather than rebuilding it during state-driven updates.
Hoist the detailsRegex and summaryRegex patterns in
parsePrGitHubDescriptionBlocks, along with the code-block, fenced-code,
script/style, tag-normalization, and blank-line-collapse regexes used by
prProtectMarkdownCode and normalization helpers, into reusable static constants
so they are compiled once.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 456e37a6-96e0-4a59-916f-303160a827e6
⛔ Files ignored due to path filters (3)
apps/ios/ADE.xcodeproj/project.pbxprojis excluded by!**/*.xcodeproj/project.pbxprojdocs/features/pull-requests/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**
📒 Files selected for processing (14)
apps/ios/ADE/Models/RemoteModels.swiftapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/PRs/PrAiSummaryCard.swiftapps/ios/ADE/Views/PRs/PrDetailHeaderComponents.swiftapps/ios/ADE/Views/PRs/PrDetailOverviewPreviews.swiftapps/ios/ADE/Views/PRs/PrDetailOverviewTab.swiftapps/ios/ADE/Views/PRs/PrDetailScreen.swiftapps/ios/ADE/Views/PRs/PrGitHubDescriptionParser.swiftapps/ios/ADE/Views/PRs/PrModels.swiftapps/ios/ADE/Views/PRs/PrRowCard.swiftapps/ios/ADE/Views/PRs/PrRowCardPreviews.swiftapps/ios/ADE/Views/PRs/PrsRootScreen.swiftapps/ios/ADETests/ADETests.swiftapps/ios/ADETests/PrGitHubDescriptionParserTests.swift
💤 Files with no reviewable changes (5)
- apps/ios/ADETests/ADETests.swift
- apps/ios/ADE/Views/PRs/PrAiSummaryCard.swift
- apps/ios/ADE/Views/PRs/PrModels.swift
- apps/ios/ADE/Models/RemoteModels.swift
- apps/ios/ADE/Services/SyncService.swift
| } | ||
| .listStyle(.plain) | ||
| .listRowSpacing(12) | ||
| .contentMargins(.bottom, 88, for: .scrollContent) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm whether transitionNamespace is consumed anywhere in PrDetailScreen.swift
# or a subview it constructs, and whether the zoom transition was intentionally dropped.
rg -n 'transitionNamespace|adeNavigationZoomTransition|matchedGeometryEffect' apps/ios/ADE/Views/PRs/PrDetailScreen.swift apps/ios/ADE/Views/PRs/PrsRootScreen.swift apps/ios/ADE/Views/PRs/PrRowCard.swiftRepository: arul28/ADE
Length of output: 1363
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant files around the reported identifiers and modifier chains.
for f in apps/ios/ADE/Views/PRs/PrDetailScreen.swift apps/ios/ADE/Views/PRs/PrsRootScreen.swift apps/ios/ADE/Views/PRs/PrRowCard.swift apps/ios/ADE/Views/PRs/PrDetailScreen.swift; do
echo "===== $f ====="
wc -l "$f"
done
echo "===== PrDetailScreen init/body outline ====="
ast-grep outline apps/ios/ADE/Views/PRs/PrDetailScreen.swift || true
echo "===== PrDetailScreen relevant sections ====="
sed -n '1,120p' apps/ios/ADE/Views/PRs/PrDetailScreen.swift
echo "--------------------------------------------------"
sed -n '580,665p' apps/ios/ADE/Views/PRs/PrDetailScreen.swift
echo "===== PrsRootScreen relevant section ====="
sed -n '940,970p' apps/ios/ADE/Views/PRs/PrsRootScreen.swift
echo "===== Search all refs ====="
rg -n 'transitionNamespace|adeNavigationZoomTransition|matchedGeometryEffect|isSelectedTransitionSource|adeMatchedGeometry' apps/ios/ADE/Views/PRs/ apps/ios/ADE -g '*.swift' | head -200Repository: arul28/ADE
Length of output: 28518
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant files around the reported identifiers and modifier chains.
for f in apps/ios/ADE/Views/PRs/PrDetailScreen.swift apps/ios/ADE/Views/PRs/PrsRootScreen.swift apps/ios/ADE/Views/PRs/PrRowCard.swift; do
echo "===== $f ====="
wc -l "$f"
done
echo "===== PrDetailScreen init/body outline ====="
ast-grep outline apps/ios/ADE/Views/PRs/PrDetailScreen.swift || true
echo "===== PrDetailScreen relevant sections ====="
sed -n '1,120p' apps/ios/ADE/Views/PRs/PrDetailScreen.swift
echo "--------------------------------------------------"
sed -n '580,665p' apps/ios/ADE/Views/PRs/PrDetailScreen.swift
echo "===== PrsRootScreen relevant section ====="
sed -n '940,970p' apps/ios/ADE/Views/PRs/PrsRootScreen.swift
echo "===== Search all refs ====="
rg -n 'transitionNamespace|adeNavigationZoomTransition|matchedGeometryEffect|isSelectedTransitionSource|adeMatchedGeometry' apps/ios/ADE/Views/PRs/ apps/ios/ADE -g '*.swift' | head -200Repository: arul28/ADE
Length of output: 28413
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the PR detail route/rendering code around PrDetailView construction.
rg -n 'PrDetailView\(|prTransitionNamespace|TransitionType|open.*pr|OpenSheet|Presentation|navigationDestination|PrStackSheet|PrsRootScreen' apps/ios/ADE/Views apps/ios/ADE -g '*.swift' | head -220
echo "\n===== Candidate files likely containing route/rendering ====="
git ls-files apps/ios/ADE/Views | sed -n '1,220p'
echo "===== PrsRootScreen lines 450-920 ====="
sed -n '450,920p' apps/ios/ADE/Views/PRs/PrsRootScreen.swift
echo "===== Search for PR presentation/destination constructs ====="
rg -n 'PrDetailView\(|open.*pr|openPR|navigate.*pr|transitionNamespace|selectedPrTransitionId|Selection' apps/ios/ADE/Views apps/ios/ADE -g '*.swift' | head -260Repository: arul28/ADE
Length of output: 50366
Restore the PR matched-geometry transition or drop the transition params.
PrDetailView(prId:, transitionNamespace:) is constructed from the PR row navigation destination with the shared prTransitionNamespace, and PrRowCard uses isSelectedTransitionSource / transitionNamespace for .adeMatchedGeometry. Without .adeNavigationZoomTransition(id: ..., in: transitionNamespace) in PrDetailView, the tap→detail zoom completes as a plain navigation push instead of the matched-geometry animation. Clean up transitionNamespace from both the route and the row if this is intentional.
🤖 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 `@apps/ios/ADE/Views/PRs/PrDetailScreen.swift` around lines 619 - 622, Restore
the matched-geometry transition in PrDetailView by applying
adeNavigationZoomTransition with the PR’s transition identifier and
transitionNamespace, matching the PrRowCard source configuration. If the
transition is intentionally removed instead, remove transitionNamespace from
PrDetailView’s initializer and the PR navigation route, and remove the
corresponding PrRowCard transition parameters.
|
@codex review |
![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: 7d2fce344c
ℹ️ 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".
| value = prReplacingHtmlTag(value, regex: PrGitHubDescriptionRegex.blockquoteOpen, replacement: "\n> ") | ||
| value = prReplacingHtmlTag(value, regex: PrGitHubDescriptionRegex.blockquoteClose, replacement: "\n") | ||
| value = prReplacingHtmlTag(value, regex: PrGitHubDescriptionRegex.horizontalRule, replacement: "\n---\n") | ||
| value = prReplacingHtmlTag(value, regex: PrGitHubDescriptionRegex.anyTag, replacement: "") |
There was a problem hiding this comment.
When a PR body contains a standard Markdown autolink such as <https://github.com/arul28/ADE> or <dev@example.com>, the broad anyTag replacement treats the entire autolink as an HTML tag and deletes both its address and visible text. Protect Markdown autolinks before this pass, or restrict the residual-tag expression to actual HTML tag names.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe19d25c2d
ℹ️ 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".
|
|
||
| private enum PrGitHubDescriptionRegex { | ||
| static let details = expression( | ||
| #"<details\b[^>]*>(.*?)</details\s*>"#, |
There was a problem hiding this comment.
When a GitHub description contains valid nested <details> elements, this non-greedy regex ends the outer match at the inner </details>. The parser consequently drops the inner disclosure structure and emits the remainder of the outer body as a separate Markdown block; for example, an outer disclosure containing an inner disclosure renders outer end outside the outer disclosure. Use balanced tag scanning or an HTML parser so each outer block consumes its matching closing tag.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad30672fd8
ℹ️ 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".
| static let blockquoteOpen = expression(#"<blockquote\b[^>]*>"#, options: [.caseInsensitive]) | ||
| static let blockquoteClose = expression(#"</blockquote\s*>"#, options: [.caseInsensitive]) | ||
| static let horizontalRule = expression(#"<hr\s*/?>"#, options: [.caseInsensitive]) | ||
| static let anyTag = expression(#"<[^>]+>"#, options: [.caseInsensitive]) |
There was a problem hiding this comment.
Restrict this matcher to actual HTML tags. Every PR description now passes through this normalizer, and <[^>]+> deletes ordinary Markdown prose between comparison operators or placeholder brackets; for example, Keep x < y and y > z here renders as Keep x z here, while Use <T> as a placeholder loses T entirely.
Useful? React with 👍 / 👎.
| value = value | ||
| .split(separator: "\n", omittingEmptySubsequences: false) | ||
| .map { $0.trimmingCharacters(in: .whitespaces) } | ||
| .joined(separator: "\n") |
There was a problem hiding this comment.
Avoid trimming leading whitespace from every normalized line. This function also processes descriptions containing no HTML, so a nested list such as - parent\n - child is flattened into two top-level items, and four-space indented code blocks become ordinary prose before reaching PrMarkdownRenderer.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 801c43eab9
ℹ️ 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".
| } | ||
|
|
||
| let nsSource = source as NSString | ||
| let matches = prGitHubDetailsRanges(in: nsSource) |
There was a problem hiding this comment.
When a PR body contains <details> inside an HTML comment, four-space-indented code, or an unclosed fenced code block, this global scan still treats it as live HTML because protection only covers inline code and closed fences. GitHub keeps those regions hidden or literal, but the iOS view removes the markup from its original context and renders a visible DisclosureGroup, exposing template instructions or corrupting code examples; skip comments and all Markdown code ranges before scanning for disclosures.
Useful? React with 👍 / 👎.
| let item = prDecodeHtmlEntities(prStripHtmlTags(prHtmlCapture(match, in: original, index: 1))) | ||
| .trimmingCharacters(in: .whitespacesAndNewlines) | ||
| return item.isEmpty ? "" : "\n- \(item)" |
There was a problem hiding this comment.
When a GitHub description embeds an <ol>, every <li> is unconditionally converted to -, so ordered instructions become bullets and any numbering or start semantics are lost. Track whether each item belongs to an ordered or unordered container and emit numbered Markdown for ordered lists.
Useful? React with 👍 / 👎.
1 participant
Summary
Validation
Open in ADE: https://ade-app.dev/open?type=pr&repo=arul28%2FADE&number=899
Summary by CodeRabbit
New Features
<details>now render as expandable sections with extracted titles.Improvements
Tests
Greptile Summary
This PR updates the iOS pull request experience for a more compact mobile flow. The main changes are:
Confidence Score: 5/5
Safe to merge with low risk.
The changes are mostly UI refactoring plus a focused parser covered by tests. No blocking correctness or security issues were identified.
No files require special attention.
What T-Rex did
Important Files Changed
Sequence Diagram
sequenceDiagram participant User participant PRList as iOS PR List participant Detail as PrDetailView participant Sync as SyncService participant Host as ADE Host User->>PRList: Open GitHub or ADE PR PRList->>Detail: Navigate with PR id or repo coordinates Detail->>Sync: Load list item, snapshot, and sidecars alt Unmapped GitHub route Sync->>Host: prs.getMobileGithubDetail Host-->>Sync: item, snapshot, threads, runs, activity, unavailable parts Sync-->>Detail: Aggregate detail payload Detail->>Detail: Merge snapshot and preserve cached deployments/capabilities else Mapped ADE route Sync->>Host: prs.getSnapshot and sidecar commands Host-->>Sync: snapshot, threads, runs, deployments, capabilities Sync-->>Detail: Hydrated detail state end Detail->>Detail: Render summary metrics and parsed description disclosures User->>Detail: Expand unmapped actions or commit listReviews (6): Last reviewed commit: "ship: iteration 5 — preserve PR Markdown..." | Re-trigger Greptile