♻️ Refactor | flatten brick props — data fields spread directly onto component - #1
♻️ Refactor | flatten brick props — data fields spread directly onto component#1davidgaspardev wants to merge 3 commits into
Conversation
…onent
BrickComponent now receives { id, children, ...data } instead of { brick }.
BrickComponentProps is replaced by BrickProps<D>. All built-in web adapters
and the README examples updated accordingly.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Removed mobile platform checklist, Flutter widget tests, and complex branching rules. Replaced with a minimal template focused on public API impact, typecheck, and README coverage. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR replaces the ChangesFlat BrickProps contract
PR Workflow Tooling
Sequence Diagram(s)sequenceDiagram
participant Renderer as BrickNode (renderer.tsx)
participant Props as Flat BrickProps
participant Adapter as BrickComponent (e.g. Text/Image/Container)
Renderer->>Props: build { id, ...brick.data, children }
Renderer->>Adapter: <Component {...props} />
Adapter->>Adapter: destructure { id, value, style, ... } from props
Adapter-->>Renderer: rendered JSX
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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: 2
🧹 Nitpick comments (1)
src/core/types.ts (1)
54-59: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider documenting or guarding against reserved prop names.
The intersection
D & { id: string; children?: ReactNode }will cause unexpected type behavior if a user's data shapeDdefinesidorchildrenwith incompatible types (e.g.,id: numberwould intersect tonever). TypeScript catches this at compile time, but a clearer API might use a conditional type or document the constraint explicitly.Optional safeguard approach:
type ReservedKeys = 'id' | 'children'; export type BrickProps<D extends object = object> = (keyof D & ReservedKeys) extends never ? D & { id: string; children?: ReactNode } : never; // Forces compile error with clear intentAlternatively, add a JSDoc note that
Dmust not containidorchildrenproperties.🤖 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 `@src/core/types.ts` around lines 54 - 59, The BrickProps type definition has no guard against users passing a data shape D that contains id or children properties with incompatible types, which silently resolves to never. Add a conditional type check to the BrickProps generic that validates D does not contain the reserved keys id or children. If D extends a type with these reserved keys, the type should resolve to never, forcing a compile-time error with clear intent. Alternatively, if you prefer a documentation-first approach, add explicit JSDoc comments to BrickProps clarifying that the generic type parameter D must not define id or children properties.
🤖 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 @.claude/commands/open-pr.md:
- Around line 19-26: Step 5 in open-pr.md references PR template sections that
do not exist in the actual .github/PULL_REQUEST_TEMPLATE.md file. Update Step 5
to remove instructions for non-existent sections (Screenshots, Testing,
Reference Links/Dependencies) and keep only the sections that are actually
present in the template: Why?, Type of Change, What Changed?, and Checklist.
Ensure the instructions accurately reflect what users will encounter when they
create the PR so there is no contradiction with the earlier instruction on line
14 to read the template structure.
In `@src/core/renderer.tsx`:
- Around line 34-35: The props object on line 34 is spreading brick.data after
setting id, which allows brick.data.id to overwrite the canonical brick.id
value. This breaks the flat-props contract. Reorder the spread operation in the
props object so that the id assignment comes after spreading brick.data,
ensuring the renderer-controlled brick.id field takes precedence and cannot be
overwritten by any id property in brick.data.
---
Nitpick comments:
In `@src/core/types.ts`:
- Around line 54-59: The BrickProps type definition has no guard against users
passing a data shape D that contains id or children properties with incompatible
types, which silently resolves to never. Add a conditional type check to the
BrickProps generic that validates D does not contain the reserved keys id or
children. If D extends a type with these reserved keys, the type should resolve
to never, forcing a compile-time error with clear intent. Alternatively, if you
prefer a documentation-first approach, add explicit JSDoc comments to BrickProps
clarifying that the generic type parameter D must not define id or children
properties.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: aabef493-a59d-4e0f-98f9-2eb6f12d43a3
📒 Files selected for processing (11)
.claude/commands/open-pr.md.github/PULL_REQUEST_TEMPLATE.mdREADME.mdsrc/core/catalog.tssrc/core/index.tssrc/core/renderer.tsxsrc/core/types.tssrc/web/adapters/Container.tsxsrc/web/adapters/Image.tsxsrc/web/adapters/Text.tsxtest/renderer.test.tsx
| 5. Run `gh pr create --base <base>` filling each section: | ||
| - **Why?** — the problem this solves | ||
| - **Type of Change** — tick the correct checkbox | ||
| - **What Changed?** — layers, classes, entities affected | ||
| - **Screenshots** — remove the table if there are no UI changes | ||
| - **Testing** — tick platforms and verification items based on the diff; describe test scenarios | ||
| - **Reference Links / Dependencies** — include `$ARGUMENTS` if provided; omit if empty | ||
| - **Checklist** — tick Self-review and Tests/Docs items based on the diff |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the command file to see Step 5 exactly
cat -n .claude/commands/open-pr.md | head -30Repository: squillteam/bricks-web-client
Length of output: 1855
🏁 Script executed:
# Then check the PR template file
cat -n .github/PULL_REQUEST_TEMPLATE.mdRepository: squillteam/bricks-web-client
Length of output: 803
Mismatch between command instructions and actual PR template structure.
Step 5 instructs users to fill sections—Screenshots, Testing, Reference Links / Dependencies, and Merge Rules—that do not exist in .github/PULL_REQUEST_TEMPLATE.md. The template contains only: Why?, Type of change, What changed?, and Checklist.
Additionally, line 14 instructs users to read the template to understand its structure, but Step 5 then contradicts this by listing non-existent fields. This creates confusion when users follow the command and discover the template lacks these sections.
Either expand the template to include the missing sections or update Step 5 to match the actual template structure.
🤖 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 @.claude/commands/open-pr.md around lines 19 - 26, Step 5 in open-pr.md
references PR template sections that do not exist in the actual
.github/PULL_REQUEST_TEMPLATE.md file. Update Step 5 to remove instructions for
non-existent sections (Screenshots, Testing, Reference Links/Dependencies) and
keep only the sections that are actually present in the template: Why?, Type of
Change, What Changed?, and Checklist. Ensure the instructions accurately reflect
what users will encounter when they create the PR so there is no contradiction
with the earlier instruction on line 14 to read the template structure.
| const props = { id: brick.id, ...(brick.data as object | undefined), children }; | ||
| return <Component {...(props as any)} />; |
There was a problem hiding this comment.
Preserve canonical brick identity when flattening props.
Line 34 currently lets brick.data.id overwrite brick.id because of spread order. That breaks the flat-props contract and can propagate incorrect IDs into adapters (data-brick-id, analytics hooks, test selectors). Ensure renderer-controlled fields win.
🔧 Proposed fix
- const props = { id: brick.id, ...(brick.data as object | undefined), children };
+ const props = { ...(brick.data as object | undefined), id: brick.id, children };
return <Component {...(props as any)} />;📝 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 props = { id: brick.id, ...(brick.data as object | undefined), children }; | |
| return <Component {...(props as any)} />; | |
| const props = { ...(brick.data as object | undefined), id: brick.id, children }; | |
| return <Component {...(props as any)} />; |
🤖 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 `@src/core/renderer.tsx` around lines 34 - 35, The props object on line 34 is
spreading brick.data after setting id, which allows brick.data.id to overwrite
the canonical brick.id value. This breaks the flat-props contract. Reorder the
spread operation in the props object so that the id assignment comes after
spreading brick.data, ensuring the renderer-controlled brick.id field takes
precedence and cannot be overwritten by any id property in brick.data.
1 participant
Why?
The previous API required every brick component to destructure
brick.data?.field, which was verbose, not type-safe at the component signature level, and inconsistent with how typical React components work. Spreading data fields directly as props gives components a plain, idiomatic React interface.Type of change
What changed?
BrickProps<D>replacesBrickComponentProps<D>— now a type aliasD & { id: string; children?: ReactNode }instead of{ brick: Brick<..., D>; children? }. Consumers must update imports.BrickComponent<D>now constrainsD extends object(was unconstrained).BrickNoderenderer spreads{ id: brick.id, ...brick.data, children }onto the component instead of passing the fullbrickprop.Container,Image,Text) updated to destructure flat props.idavailability..github/PULL_REQUEST_TEMPLATE.mdsimplified for a web lib project..claude/commands/open-pr.mdadded for project automation.Consumers need to update: Any
BrickComponentthat readsbrick.data?.fieldmust switch to destructuringfielddirectly. Any import ofBrickComponentPropsmust be updated toBrickProps.Checklist
bun testpassesbun run typecheckpassesSummary by CodeRabbit
New Features
Documentation
Refactor
Tests