Add GitHub Pull Request block for viewing PRs in-app#3886
Conversation
Add a `pr` block that renders a GitHub pull request inside Macro using the
standard main-content + right-side-panel layout (matching the call and
markdown blocks). The main area shows the PR title, status, line changes, a
prominent "Open in GitHub" button, and the conversation; the side panel shows
Details (status, repo, number, changes) and CI checks.
Clicking a PR entity in Soup now opens this block instead of opening GitHub in
a new tab.
- Register `pr` in the block registry and all exhaustive block-name maps
(split combinations, nesting, method registry, entity icons, list views,
file colors).
- Fetch PR metadata via a new storageServiceClient.getForeignEntity wrapper
over the existing GET /foreign_entity/{id} endpoint, keyed by the foreign
entity id Soup already uses.
- Route github_pull_request foreign entities to { type: 'pr', id } in Soup.
Diff rendering is intentionally deferred; the "Open in GitHub" button links out
to the full diff for now.
https://claude.ai/code/session_013CcbMaiGmVBDRDv5qWigQo
📝 WalkthroughSummary by CodeRabbitNew Features
WalkthroughThis pull request introduces a new GitHub pull request block type for viewing PR data natively within the application. It creates a new 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches⚔️ Resolve 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 |
PR conversation comments were shown as raw, pre-wrapped text, so GitHub markdown (and machine-generated comments like CodeRabbit's) rendered as literal source. Render them through StaticMarkdown with the external (GitHub-flavored) target, sharing one editor via StaticMarkdownContext like the call block's summary. Also adds a small, code-fence-safe sanitizer that drops HTML comments and unwraps a few structural HTML tags (details/summary/div/etc.) GitHub renders but our markdown renderer doesn't, so they don't appear as literal tags. https://claude.ai/code/session_013CcbMaiGmVBDRDv5qWigQo
| video: 'documents', | ||
| write: 'documents', | ||
| automation: 'agents', | ||
| pr: 'inbox', |
There was a problem hiding this comment.
remove this once you've rebased onto origin/main when i merge this pr #3894
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@js/app/packages/block-pr/utils.ts`:
- Around line 21-46: The code assumes m.checks is an array and calls .filter
which can throw if the payload shape drifts; update the normalization to guard
against non-arrays by replacing checks: (m.checks ?? []).filter(Boolean) with a
safe check such as checks: Array.isArray(m.checks) ? m.checks.filter(Boolean) :
[] (or null if you prefer) so malformed m.checks won't crash the function;
locate the return object in utils.ts and change the checks normalization
accordingly.
In `@js/app/packages/queries/pull-request/pull-request.ts`:
- Around line 13-19: The hook builds query options with a dynamic id() call
twice which can cause a queryKey/queryFn mismatch; modify
usePullRequestEntityQuery so it captures the id once into a local const (e.g.,
const capturedId = id()) and then use capturedId for
pullRequestKeys.entity(capturedId).queryKey, for the
storageServiceClient.getForeignEntity({ id: capturedId }) call inside the
queryFn (and for enabled), ensuring useQuery receives the same stable id for key
and fetch.
🪄 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
Run ID: 20348f1e-11fa-4fd6-8643-5d85b95baf08
⛔ Files ignored due to path filters (1)
js/bun.lockis excluded by!**/*.lock,!**/bun.lock
📒 Files selected for processing (22)
js/app/packages/app/component/next-soup/utils.tsjs/app/packages/app/constants/list-views.tsjs/app/packages/block-pr/component/Block.tsxjs/app/packages/block-pr/component/PrChecks.tsxjs/app/packages/block-pr/component/PrComment.tsxjs/app/packages/block-pr/component/PrStatusBadge.tsxjs/app/packages/block-pr/component/PullRequestBody.tsxjs/app/packages/block-pr/component/PullRequestSplitHeader.tsxjs/app/packages/block-pr/component/sidepanel/PullRequestSidePanelSections.tsxjs/app/packages/block-pr/definition.tsjs/app/packages/block-pr/package.jsonjs/app/packages/block-pr/tsconfig.jsonjs/app/packages/block-pr/utils.tsjs/app/packages/core/block.tsjs/app/packages/core/blockMethodRegistry.tsjs/app/packages/core/component/EntityIcon.tsxjs/app/packages/core/component/FileList/constants.tsxjs/app/packages/entity/src/utils/buildEntityData.tsjs/app/packages/queries/pull-request/keys.tsjs/app/packages/queries/pull-request/pull-request.tsjs/app/packages/service-clients/service-storage/client.tsjs/app/tsconfig.json
| const m = raw as Partial<GithubPullRequest>; | ||
| if (typeof m.number !== 'number' || !m.owner || !m.repo || !m.url) { | ||
| return null; | ||
| } | ||
|
|
||
| let status: PullRequestStatus = 'open'; | ||
| if (m.status === 'merged') { | ||
| status = 'merged'; | ||
| } else if (m.status === 'closed') { | ||
| status = 'closed'; | ||
| } | ||
|
|
||
| const displayName = m.displayName ?? `${m.owner}/${m.repo}#${m.number}`; | ||
|
|
||
| return { | ||
| number: m.number, | ||
| name: m.name ?? displayName, | ||
| owner: m.owner, | ||
| repo: m.repo, | ||
| url: m.url, | ||
| status, | ||
| additions: m.additions ?? 0, | ||
| deletions: m.deletions ?? 0, | ||
| comments: m.comments ?? [], | ||
| checks: (m.checks ?? []).filter(Boolean), | ||
| }; |
There was a problem hiding this comment.
Harden normalization against malformed payload shapes.
Line 45 assumes m.checks is always an array and can throw at runtime when metadata shape drifts. This breaks the PR block instead of gracefully returning normalized defaults/null.
Suggested fix
export function normalizePullRequestMetadata(
raw: unknown
): PullRequestMetadata | null {
if (!raw || typeof raw !== 'object') return null;
const m = raw as Partial<GithubPullRequest>;
- if (typeof m.number !== 'number' || !m.owner || !m.repo || !m.url) {
+ if (
+ typeof m.number !== 'number' ||
+ typeof m.owner !== 'string' ||
+ typeof m.repo !== 'string' ||
+ typeof m.url !== 'string'
+ ) {
return null;
}
@@
return {
number: m.number,
name: m.name ?? displayName,
owner: m.owner,
repo: m.repo,
url: m.url,
status,
- additions: m.additions ?? 0,
- deletions: m.deletions ?? 0,
- comments: m.comments ?? [],
- checks: (m.checks ?? []).filter(Boolean),
+ additions: typeof m.additions === 'number' ? m.additions : 0,
+ deletions: typeof m.deletions === 'number' ? m.deletions : 0,
+ comments: Array.isArray(m.comments) ? m.comments : [],
+ checks: Array.isArray(m.checks) ? m.checks.filter(Boolean) : [],
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const m = raw as Partial<GithubPullRequest>; | |
| if (typeof m.number !== 'number' || !m.owner || !m.repo || !m.url) { | |
| return null; | |
| } | |
| let status: PullRequestStatus = 'open'; | |
| if (m.status === 'merged') { | |
| status = 'merged'; | |
| } else if (m.status === 'closed') { | |
| status = 'closed'; | |
| } | |
| const displayName = m.displayName ?? `${m.owner}/${m.repo}#${m.number}`; | |
| return { | |
| number: m.number, | |
| name: m.name ?? displayName, | |
| owner: m.owner, | |
| repo: m.repo, | |
| url: m.url, | |
| status, | |
| additions: m.additions ?? 0, | |
| deletions: m.deletions ?? 0, | |
| comments: m.comments ?? [], | |
| checks: (m.checks ?? []).filter(Boolean), | |
| }; | |
| const m = raw as Partial<GithubPullRequest>; | |
| if ( | |
| typeof m.number !== 'number' || | |
| typeof m.owner !== 'string' || | |
| typeof m.repo !== 'string' || | |
| typeof m.url !== 'string' | |
| ) { | |
| return null; | |
| } | |
| let status: PullRequestStatus = 'open'; | |
| if (m.status === 'merged') { | |
| status = 'merged'; | |
| } else if (m.status === 'closed') { | |
| status = 'closed'; | |
| } | |
| const displayName = m.displayName ?? `${m.owner}/${m.repo}#${m.number}`; | |
| return { | |
| number: m.number, | |
| name: m.name ?? displayName, | |
| owner: m.owner, | |
| repo: m.repo, | |
| url: m.url, | |
| status, | |
| additions: typeof m.additions === 'number' ? m.additions : 0, | |
| deletions: typeof m.deletions === 'number' ? m.deletions : 0, | |
| comments: Array.isArray(m.comments) ? m.comments : [], | |
| checks: Array.isArray(m.checks) ? m.checks.filter(Boolean) : [], | |
| }; |
🤖 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 `@js/app/packages/block-pr/utils.ts` around lines 21 - 46, The code assumes
m.checks is an array and calls .filter which can throw if the payload shape
drifts; update the normalization to guard against non-arrays by replacing
checks: (m.checks ?? []).filter(Boolean) with a safe check such as checks:
Array.isArray(m.checks) ? m.checks.filter(Boolean) : [] (or null if you prefer)
so malformed m.checks won't crash the function; locate the return object in
utils.ts and change the checks normalization accordingly.
| return useQuery(() => ({ | ||
| queryKey: pullRequestKeys.entity(id()).queryKey, | ||
| queryFn: async () => | ||
| await throwOnErr(() => | ||
| storageServiceClient.getForeignEntity({ id: id() }) | ||
| ), | ||
| enabled: !!id(), |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In @tanstack/solid-query v5, can a queryFn that closes over a reactive accessor (like id()) return data for a newer value than the queryKey it was created with? What pattern do docs recommend to keep queryKey and queryFn parameters consistent?
💡 Result:
In @tanstack/solid-query v5, a queryFn that closes over a reactive accessor (like id) can indeed return data for a newer value than the queryKey it was originally created with, if you don’t keep the dependency variables aligned between queryKey and queryFn. The docs state that (1) Solid Query tracks reactive options when you pass an accessor/function to useQuery/createQuery, and re-runs it when dependencies change [1][2], and (2) query keys “should include any variables you use in your query function that change” so the cache/refetch behavior stays consistent [3]. If your queryFn uses external/closed-over variables but those variables are not included in the queryKey, you get the classic “stale closure”/mismatch risk; this is explicitly discussed for TanStack Query in general as an expected issue when dependencies are omitted from the queryKey [4]. Recommended pattern to keep queryKey and queryFn consistent 1) Put the variable(s) your queryFn uses into the queryKey. Example pattern: use queryKey: ['todos', todoId] and queryFn: => fetchTodoById(todoId) [3]. 2) Prefer extracting parameters from the queryKey inside queryFn via QueryFunctionContext. The docs for query-functions show using queryFn’s context { queryKey } so the function derives its parameters from the key itself, keeping them aligned [5]. 3) In Solid, keep the whole options object reactive (query options accessor) so Solid can re-run when signals change, but still ensure queryKey/queryFn are wired to the same values. The Solid docs describe that useQuery accepts a function returning options for “reactive options”, and Solid Query “re-run[s] it whenever the dependencies change” [1]. 4) Use the co-located helper pattern: queryOptions(id) that returns both queryKey and queryFn using the same id value. The Solid “query-options” guide recommends using queryOptions to share queryKey and queryFn together “yet keep them co-located to one another” [6]. Concretely, the pattern is: - queryKey includes id (or whatever reactive variable queryFn depends on) - queryFn reads id from either the same closure value or, more robustly, from context.queryKey Example (robust version using queryKey as the source of truth): useQuery( => ({ queryKey: ['todo', id], queryFn: ({ queryKey }) => { const [, todoId] = queryKey return fetchTodoById(todoId) }, })) This keeps the key and the fetch parameters consistent even as reactive state changes [5][3].
Citations:
- 1: https://tanstack.com/query/v5/docs/framework/solid/reference/useQuery.md
- 2: https://mintlify.com/TanStack/query/api/solid/create-query
- 3: https://tanstack.com/query/latest/docs/framework/react/guides/query-keys
- 4: refetch vs invalidateQuery: inconsistent behavior of queryFn TanStack/query#5894
- 5: https://tanstack.com/query/v5/docs/framework/solid/guides/query-functions
- 6: https://tanstack.com/query/v5/docs/framework/solid/guides/query-options
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="js/app/packages/queries/pull-request/pull-request.ts"
if [ ! -f "$FILE" ]; then
echo "Missing $FILE" >&2
exit 1
fi
nl -ba "$FILE" | sed -n '1,120p'Repository: macro-inc/macro
Length of output: 102
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="js/app/packages/queries/pull-request/pull-request.ts"
if [ ! -f "$FILE" ]; then
echo "Missing $FILE" >&2
exit 1
fi
# Print the relevant range around the earlier line numbers
# (adjusts automatically if file is shorter)
LINE_START=1
LINE_END=120
cat -n "$FILE" | sed -n "${LINE_START},${LINE_END}p"Repository: macro-inc/macro
Length of output: 980
Fix potential queryKey/queryFn mismatch in usePullRequestEntityQuery
queryKey is built from id() but queryFn calls id() again at execution time; if id changes between options creation and queryFn execution, fetched data can be cached under the wrong key (lines 13-19). Capture id() once and reuse it for both.
Suggested fix
export function usePullRequestEntityQuery(id: Accessor<string>) {
- return useQuery(() => ({
- queryKey: pullRequestKeys.entity(id()).queryKey,
- queryFn: async () =>
- await throwOnErr(() =>
- storageServiceClient.getForeignEntity({ id: id() })
- ),
- enabled: !!id(),
- }));
+ return useQuery(() => {
+ const currentId = id();
+ return {
+ queryKey: pullRequestKeys.entity(currentId).queryKey,
+ queryFn: async () =>
+ await throwOnErr(() =>
+ storageServiceClient.getForeignEntity({ id: currentId })
+ ),
+ enabled: !!currentId,
+ };
+ });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return useQuery(() => ({ | |
| queryKey: pullRequestKeys.entity(id()).queryKey, | |
| queryFn: async () => | |
| await throwOnErr(() => | |
| storageServiceClient.getForeignEntity({ id: id() }) | |
| ), | |
| enabled: !!id(), | |
| export function usePullRequestEntityQuery(id: Accessor<string>) { | |
| return useQuery(() => { | |
| const currentId = id(); | |
| return { | |
| queryKey: pullRequestKeys.entity(currentId).queryKey, | |
| queryFn: async () => | |
| await throwOnErr(() => | |
| storageServiceClient.getForeignEntity({ id: currentId }) | |
| ), | |
| enabled: !!currentId, | |
| }; | |
| }); | |
| } |
🤖 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 `@js/app/packages/queries/pull-request/pull-request.ts` around lines 13 - 19,
The hook builds query options with a dynamic id() call twice which can cause a
queryKey/queryFn mismatch; modify usePullRequestEntityQuery so it captures the
id once into a local const (e.g., const capturedId = id()) and then use
capturedId for pullRequestKeys.entity(capturedId).queryKey, for the
storageServiceClient.getForeignEntity({ id: capturedId }) call inside the
queryFn (and for enabled), ensuring useQuery receives the same stable id for key
and fetch.
3 participants
Summary
Introduces a new
prblock that allows users to view GitHub pull requests directly within the application, replacing the previous behavior of opening PRs in an external browser.Key Changes
New PR Block Package (
block-pr): Created a complete block implementation with:Block.tsx: Main component that loads and displays PR metadataPullRequestBody.tsx: Main content area showing PR title, details, and commentsPullRequestSidePanelSections.tsx: Side panel displaying PR status, repository info, and check runsPrComment.tsx: Individual comment rendering with author, timestamp, and source typePrChecks.tsx: GitHub check run status display with icons and linksPrStatusBadge.tsx: Visual badge for PR status (open/merged/closed)PullRequestSplitHeader.tsx: Header components for split layout viewutils.ts: Metadata normalization and status constantsQuery Infrastructure: Added
usePullRequestEntityQueryhook to fetch foreign entity data from storage serviceStorage Service Client: Extended
storageServiceClientwithgetForeignEntity()method to retrieve PR metadata by IDBlock Registry: Registered
pras a new block type in the core block systemEntity Handling: Updated entity building and routing logic to:
prblock instead of opening externallyType System: Added
prto block name registry, method registry, and file type colorsImplementation Details
PullRequestMetadatashapehttps://claude.ai/code/session_013CcbMaiGmVBDRDv5qWigQo