Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new PrCard UI in the catalog realm using mock PR state/review/CI data, along with a GitHub-styled Theme card and a demo card instance to showcase the design before real data integration (blocked by #4074).
Changes:
- Added
PrCardcard definition with isolated/embedded/fitted templates and styling. - Added a GitHub PR-themed Brand Guide (Theme card) to supply design tokens/palette.
- Added a sample
PrCardinstance JSON wired to a demo listing + theme.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| packages/catalog-realm/pr-card/pr-card.gts | New PrCard card definition + isolated/fitted templates with mock CI/review state and extensive scoped CSS. |
| packages/catalog-realm/Theme/github-pr-brand-guide.json | New GitHub PR brand guide Theme card (palette/typography/variables) for consistent styling. |
| packages/catalog-realm/PrCard/b306784c-a34b-40fb-90c8-61bba0d1e9c7.json | Demo PrCard instance referencing the PrCard module, a listing, and the new theme. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| class FittedTemplate extends Component<typeof PrCard> { | ||
| get mockCi() { | ||
| return MOCK_CI; | ||
| } | ||
| get mockReviews() { | ||
| return MOCK_REVIEWS; | ||
| } | ||
| get stateLabel() { | ||
| return prStateLabel(MOCK_STATE); | ||
| } | ||
| get pillColor() { | ||
| return stateColor(this.stateLabel); | ||
| } | ||
|
|
||
| get prTitle() { | ||
| return this.args.model.prTitle ?? 'Pull Request'; | ||
| } | ||
| get prUrl() { | ||
| return this.args.model.prUrl ?? '#'; | ||
| } |
There was a problem hiding this comment.
IsolatedTemplate and FittedTemplate duplicate the same mock-data getters and derived computations (mockCi, mockReviews, stateLabel, pillColor, prTitle, prUrl, review aggregations). This makes future changes error-prone (easy to update one template and forget the other). Consider extracting shared logic into a helper module or a base component/mixin used by both templates.
| /* Horizontal divider between CI and reviews */ | ||
| .status-divider { | ||
| height: 1px; | ||
| background: var(--border, #d0d7de); | ||
| flex-shrink: 0; | ||
| margin: 0 var(--boxel-sp-lg); | ||
| } | ||
|
|
There was a problem hiding this comment.
FittedTemplate’s styles define .status-divider (and container-query tweaks for it), but the fitted template markup doesn’t render an element with class='status-divider'. This is dead CSS (or a missing divider in the layout). Either add the divider element where intended, or remove the unused styles and container-query adjustments.
| /* Horizontal divider between CI and reviews */ | |
| .status-divider { | |
| height: 1px; | |
| background: var(--border, #d0d7de); | |
| flex-shrink: 0; | |
| margin: 0 var(--boxel-sp-lg); | |
| } |
| // === Computed === | ||
| @field cardTitle = contains(StringField, { | ||
| computeVia(this: PrCard) { | ||
| return this.prTitle ?? `PR #${this.prNumber}`; |
There was a problem hiding this comment.
cardTitle can become PR #undefined when both prTitle and prNumber are unset. This produces a confusing title and may leak "undefined" into UI/search. Consider falling back to a generic title when prNumber is null/undefined (or require prNumber).
| return this.prTitle ?? `PR #${this.prNumber}`; | |
| if (this.prTitle) { | |
| return this.prTitle; | |
| } | |
| if (this.prNumber !== null && this.prNumber !== undefined) { | |
| return `PR #${this.prNumber}`; | |
| } | |
| return 'Pull request'; |
| } | ||
|
|
||
| get prUrl() { | ||
| return this.args.model.prUrl ?? '#'; |
There was a problem hiding this comment.
Using '#' as a fallback for prUrl means the external-link anchors will still render and open a meaningless URL in a new tab when the field is missing. Prefer returning null/undefined and conditionally rendering the link only when a real URL is present (same applies to the fitted template’s prUrl getter).
| return this.args.model.prUrl ?? '#'; | |
| return this.args.model.prUrl ?? null; |
| target='_blank' | ||
| rel='noopener noreferrer' | ||
| class='pr-external-link' | ||
| title='Open PR on GitHub' |
There was a problem hiding this comment.
This external-link anchor is icon-only. Add an accessible name (e.g., aria-label or visually-hidden text) so screen readers can announce the purpose. The same pattern appears on the review link(s) and in the fitted template.
| title='Open PR on GitHub' | |
| title='Open PR on GitHub' | |
| aria-label='Open PR on GitHub' |
| target='_blank' | ||
| rel='noopener noreferrer' | ||
| class='review-external-link' | ||
| title='View review on GitHub' |
There was a problem hiding this comment.
The link title says "View review on GitHub" but the href points to the PR URL (this.prUrl), not a review-specific URL. This is misleading; either adjust the copy to match what the link does, or (once available) link directly to the review/comment permalink.
| title='View review on GitHub' | |
| title='View pull request on GitHub' |
lucaslyl
changed the title
|
@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: 62dc32494c
ℹ️ 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".
| const isCheckRunEvent = this.payload?.check_run !== undefined; | ||
| return isCheckRunEvent | ||
| ? this.payload?.check_run?.pull_requests?.[0].number | ||
| : (this.payload?.pull_request?.number ?? null); |
There was a problem hiding this comment.
GithubEventCard.prNumber only branches on check_run and otherwise falls back to payload.pull_request.number, so check_suite events are stored with prNumber = null even when they contain PR references in payload.check_suite.pull_requests. Because PrCard issues searchEventQuery(..., <prNumber>, 'check_suite'), those suite events will never be returned for the target PR, causing CI status rendering to miss suite-level data.
Useful? React with 👍 / 👎.
| case 'closed': | ||
| return 'Closed'; |
There was a problem hiding this comment.
renderPrActionLabel unconditionally maps action === 'closed' to 'Closed', so merged PR events (which come through the closed action with merged metadata) are displayed as closed rather than merged. This misreports state in both templates and makes the 'Merged' icon/color branches effectively unreachable for real merged PRs.
Useful? React with 👍 / 👎.
linear: https://linear.app/cardstack/issue/CS-10224/pr-card
Summary
What's included:
Demo:
Isolated:
Fitted:
