Skip to content

Commit fce7166

Browse files
author
root
committed
docs: document review comments resolution workflow and mobile web share lessons
1 parent 9e06cf5 commit fce7166

2 files changed

Lines changed: 138 additions & 0 deletions

File tree

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
---
2+
name: pr-comments-resolution
3+
description: Comprehensive workflow for investigating, fixing, verifying, and resolving GitHub PR review comments and visual UI feedback.
4+
---
5+
6+
# PR Comments Resolution Skill
7+
8+
This skill captures the end-to-end operational workflow for addressing review comments on pull requests, particularly visual UI defects, platform-specific bugs, and automated comment resolution on GitHub.
9+
10+
## 1. Comment Discovery & Retrieval
11+
12+
### Fetch Comments via GitHub CLI
13+
Fetch both top-level PR comments and inline review comments:
14+
```bash
15+
# Fetch top-level issue comments with GraphQL IDs and minimization state
16+
gh pr view <PR_NUMBER> --json comments --jq '.comments[] | {id: .id, author: .author.login, isMinimized: .isMinimized, body: .body}'
17+
18+
# Fetch inline review diff comments
19+
gh api repos/<OWNER>/<REPO>/pulls/<PR_NUMBER>/comments
20+
```
21+
22+
### Inspect Attachments
23+
Download screenshots attached by reviewers (`https://github.com/user-attachments/assets/...`) directly:
24+
```bash
25+
curl -sL "<IMAGE_URL>" -o /tmp/review-issue.png
26+
```
27+
Inspect them using the `read` tool (`read(path="/tmp/review-issue.png")`) to understand the exact visual defect observed by the reviewer.
28+
29+
---
30+
31+
## 2. Resolving Comments on GitHub
32+
33+
GitHub has two distinct comment resolution mechanisms:
34+
35+
### A. Top-Level PR Issue Comments
36+
Top-level comments (e.g., general review notes with screenshots) do not have a review thread toggle. Instead, resolve them using GitHub's `minimizeComment` GraphQL mutation with classifier `RESOLVED`:
37+
```bash
38+
gh api graphql -f query='
39+
mutation {
40+
minimizeComment(input: { subjectId: "<COMMENT_NODE_ID>", classifier: RESOLVED }) {
41+
minimizedComment {
42+
isMinimized
43+
minimizedReason
44+
}
45+
}
46+
}'
47+
```
48+
This collapses the comment in the GitHub web UI and displays: *"This comment was marked as resolved by [user]"*.
49+
50+
### B. Inline Diff Review Comments
51+
Inline review comments belong to a review thread. Resolve them via `resolveReviewThread`:
52+
```bash
53+
gh api graphql -f query='
54+
mutation {
55+
resolveReviewThread(input: { threadId: "<THREAD_NODE_ID>" }) {
56+
thread {
57+
isResolved
58+
}
59+
}
60+
}'
61+
```
62+
63+
---
64+
65+
## 3. Visual & UI Verification Cycle
66+
67+
1. **Reproduce First:** Use Playwright or headless browser scripts to reproduce the exact state shown in the reviewer's screenshot. Match the device viewport (e.g., Pixel 5 for mobile, 1280x800 for desktop) and locale.
68+
2. **Implement Fix:** Modify components, CSS, and layout constraints.
69+
3. **Capture Deliverable Proof:** Re-run the browser screenshot script at the identical viewport and use `read` to compare the fix side-by-side with the reviewer's original image.
70+
4. **Mark Solved:** Once verified, mark the specific comment resolved via `minimizeComment` before moving to the next issue.
71+
72+
---
73+
74+
## 4. Mobile & Web Sharing Hard Lessons
75+
76+
### Transient User Activation on iOS / Safari
77+
- `navigator.share()` requires an active user activation token ($\approx 1000\text{ms}$).
78+
- Heavy DOM-crawling utilities (like `html2canvas`) take $> 1500\text{ms}$, causing user gesture activation to expire and making the first tap silently fail.
79+
- **Solution:** Render visual cards directly to an off-screen HTML5 `<canvas>` using 2D Canvas API. 2D Canvas operations take $< 10\text{ms}$, allowing `navigator.share()` to trigger instantly on the first tap.
80+
81+
### Eliminating Viewport Jumps
82+
- Avoid calling `window.scrollTo()` inside share handlers. In modern canvas snapshotting, off-screen rendering can be performed without scrolling the user's viewport.
83+
84+
### Target App Share Limitations (e.g. WhatsApp for iOS)
85+
- WhatsApp's iOS share extension (`net.whatsapp.WhatsApp.ShareExtension`) drops any accompanying text or URL when an image file attachment is present.
86+
- **Solution:**
87+
1. Burn all titles, labels, stats, and branding directly onto the exported image canvas.
88+
2. Silently write the prefilled text and URL to `navigator.clipboard.writeText(...)` during the click handler so users can easily paste the caption into WhatsApp or Instagram.
89+
90+
### iOS Safari Data URL Download Limitation
91+
- In iOS Safari, clicking `<a href="data:image/png;base64,..." download="...">` does not download the file; it displays a modal with title `/` where tapping "Download" does nothing.
92+
- **Solution:** Convert the canvas/data URL to a `Blob` (`canvas.toBlob(...)`), generate an Object URL (`URL.createObjectURL(blob)`), append the anchor to `document.body`, trigger `.click()`, and remove the anchor.
93+
94+
---
95+
96+
## 5. Local & CI Toolchain Parity
97+
98+
- **Pre-commit Hooks:** When using specialized environments (like Nix flakes), ensure `.husky/pre-commit` has an automatic fallback to execute tools inside the shell (e.g., `nix develop --command pnpm exec lint-staged`) if node/pnpm are not in the standard system PATH.
99+
- This prevents unformatted or failing code from being committed locally and failing CI pipelines (`format:check`, `lint`).

docs/modernization-runbook.md

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -388,3 +388,42 @@ does not install browsers or run Playwright.
388388
- The service worker precache manifest is easy to regress silently (see the
389389
PWA note above). Check `dist/sw.js` contains `/` in `precacheAndRoute` after
390390
any change to `pwa` or Nitro output configuration.
391+
392+
## Review Comments Resolution & Mobile Web Lessons
393+
394+
### Resolving GitHub PR Comments
395+
396+
- Top-level issue comments on pull requests do not support inline resolution threads.
397+
Resolve them via GitHub's GraphQL mutation:
398+
`minimizeComment(input: { subjectId: "<NODE_ID>", classifier: RESOLVED })`.
399+
This collapses the comment in the web UI and tags it as resolved.
400+
- Review comments on code diffs resolve via `resolveReviewThread(input: { threadId: "<NODE_ID>" })`.
401+
- Attached screenshots from `https://github.com/user-attachments/assets/...` can be
402+
downloaded and inspected locally with `read` for pixel-level ground truth.
403+
404+
### Mobile Web Share (`navigator.share`) & iOS Safari Realities
405+
406+
- **Transient User Activation:** Mobile WebKit (iOS Safari) requires an active user
407+
gesture token ($\approx 1000\text{ms}$). DOM-crawling tools like `html2canvas` take
408+
$> 1500\text{ms}$, expiring user activation and silently failing the first share tap.
409+
Direct 2D Canvas rendering takes $< 10\text{ms}$, keeping user activation alive and
410+
opening the share sheet instantly on the first press.
411+
- **Zero Viewport Jumps:** Avoid `window.scrollTo()` inside sharing or snapshot code.
412+
Off-screen canvas generation needs no scrolling and avoids viewport flickering/blanking.
413+
- **Target App Attachment Quirks (WhatsApp iOS):** WhatsApp's iOS share extension
414+
accepts only the image file and discards any accompanying text/URL strings. To ensure
415+
context is never lost:
416+
1. Render title, metrics, labels, and `WhatsAnalyze.com` branding directly onto the canvas.
417+
2. Silently write the share text to `navigator.clipboard.writeText(...)` during the click
418+
handler so users can easily paste the caption into WhatsApp or Instagram.
419+
- **iOS Safari Data URL Downloads:** Clicking `<a href="data:image/png;base64,..." download>`
420+
fails on iOS Safari (shows a modal with title `/` where "Download" does nothing).
421+
Always convert to a `Blob` and use `URL.createObjectURL(blob)` with a DOM-attached anchor
422+
or `saveAs` from `file-saver`.
423+
424+
### Local & CI Toolchain Parity
425+
426+
- When tools like Node and pnpm are managed through a Nix flake, ensure `.husky/pre-commit`
427+
auto-detects the environment and invokes `nix develop --command pnpm exec lint-staged`
428+
when pnpm is not in the system `/usr/bin` PATH. This prevents unformatted commits from
429+
bypassing local checks and failing CI workflows on GitHub Actions.

0 commit comments

Comments
 (0)