Build the position card and the request list - #115
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
🟡 Changes recommended
There are React rendering/accessibility issues in the new components (non-unique list keys and heading level semantics) that should be corrected before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Builds out the investor-facing “position + requests” surface in the vault preview UI by introducing reusable, prop-driven components for positions, request cards, request grouping, and a now variable-length lifecycle panel (per #111, with the lifecycle panel adaptation noted as a deviation from #100).
Changes:
- Added
PositionCard,RequestCard, andRequestListcomponents (with styling) to render holdings and grouped request entries purely from props. - Updated
LifecyclePanelto accept any number of steps (and adjusted layout to stack via container queries). - Extended the preview page to showcase the new components with representative data and states.
File summaries
| File | Description |
|---|---|
| app/src/pages/VaultPreview.tsx | Updates the preview page to render the new position/request surfaces and the updated lifecycle panel. |
| app/src/components/vault/RequestList.tsx | Introduces grouped request list rendering with optional banner and empty-state handling. |
| app/src/components/vault/RequestList.test.tsx | Adds tests for banner conditional rendering and skipping empty groups. |
| app/src/components/vault/RequestList.module.css | Styles the request list container, banner, and group layout. |
| app/src/components/vault/RequestCard.tsx | Adds a request card component supporting rows, optional coverage block, notes, footers, and actions. |
| app/src/components/vault/RequestCard.test.tsx | Tests unavailable actions and nested coverage block rendering. |
| app/src/components/vault/RequestCard.module.css | Styles request cards, state chips, rows, coverage inset, and action buttons. |
| app/src/components/vault/PositionCard.tsx | Adds a position/holding card with “unread” (em dash) rendering and optional note. |
| app/src/components/vault/PositionCard.test.tsx | Tests the em dash behavior when the value is unread (null). |
| app/src/components/vault/PositionCard.module.css | Styles the position card layout, figure, and muted text. |
| app/src/components/vault/LifecyclePanel.tsx | Generalizes lifecycle steps to variable length and adds a “complete” state option. |
| app/src/components/vault/LifecyclePanel.test.tsx | Extends tests for out-of-range currentStep behavior. |
| app/src/components/vault/LifecyclePanel.module.css | Updates layout to variable columns and adds container-query-based stacking rules. |
Review details
Suppressed comments (3)
app/src/components/vault/RequestCard.tsx:114
RequestCardusesaction.labelas a React key, but labels are not guaranteed unique (e.g., multiple actions with the same copy). Duplicate keys can break reconciliation; include the index (or add a dedicated id) to guarantee uniqueness.
{entry.actions.map((action) => (
<button
key={action.label}
app/src/components/vault/RequestCard.tsx:97
RequestCardcoverage rows also userow.labelas a React key, which can collide if two coverage rows share a label. Use an index (or a row id) to ensure keys are unique.
{entry.coverage.rows.map((row) => (
<div className={styles.row} key={row.label}>
app/src/components/vault/RequestList.tsx:57
- React list keys should be unique and stable; using
group.headingas the key can collide if two groups share the same heading, leading to incorrect reconciliation. Include the index (or add a dedicated id field) to guarantee uniqueness.
groups.map((group) =>
group.entries.length > 0 ? (
<div className={styles.group} key={group.heading}>
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| {entry.rows.map((row) => ( | ||
| <div className={styles.row} key={row.label}> |
There was a problem hiding this comment.
Fixed in all three places. RequestRow.label is caller-supplied and nothing in the type stops two rows sharing one, so the key is now the index paired with the label.
No fixture produced a collision today, so this was latent rather than live — but the rows come from outside the component and the type offers no guarantee.
4b4ca68 to
b908fec
Compare
6f3aa81 to
e54e403
Compare
e54e403 to
bb18747
Compare
bb18747 to
09c7da1
Compare
e54e403 to
2e9f6cd
Compare
The threshold came from the prototype, where the equivalent rules are viewport media queries. This one is a container query against the panel's own content box, which is about 700px in the main column, so the stacked layout applied at every desktop width and the steps never ran across.
edacfe2 to
aff24ec
Compare
Summary
Closes #111
Every request card shows the same four things in the same places: in, out, state, action. The list is scanned down a column.
Changes
Acceptance criteria
Test plan
npm run test,lint,format:check,build --workspaces --if-present: pass.npm run dev, open/. Read the list down the state column.Breaking changes
None.
Checklist