fix: preserve ERC-8021 service attribution#200
Conversation
📝 WalkthroughWalkthrough
ChangesBuilder code service code preservation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/builder-code.ts (1)
30-33: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDiscards entire array if a single entry is invalid.
If
info.scontains one non-string entry alongside otherwise-valid codes,.every(...)fails and the whole array falls back to[], dropping all previously valid attribution codes. The issue asks to "keep any existing valid service codes already present" — filtering out only the invalid entries would better preserve attribution.♻️ Proposed fix to filter instead of discard
- const existingServiceCodes = - Array.isArray(existing.info?.s) && existing.info.s.every((code) => typeof code === "string") - ? existing.info.s - : []; + const existingServiceCodes = Array.isArray(existing.info?.s) + ? existing.info.s.filter((code): code is string => typeof code === "string") + : [];🤖 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/builder-code.ts` around lines 30 - 33, The existing service-code extraction in the builder logic currently drops the entire list when any entry in existing.info.s is invalid. Update the existingServiceCodes handling to preserve valid strings by filtering existing.info.s for string entries instead of requiring all entries to pass via every(...), so the builder code keeps any already-valid attribution codes while ignoring only bad values. Use the existingServiceCodes assignment in src/builder-code.ts as the target for this change.
🤖 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.
Nitpick comments:
In `@src/builder-code.ts`:
- Around line 30-33: The existing service-code extraction in the builder logic
currently drops the entire list when any entry in existing.info.s is invalid.
Update the existingServiceCodes handling to preserve valid strings by filtering
existing.info.s for string entries instead of requiring all entries to pass via
every(...), so the builder code keeps any already-valid attribution codes while
ignoring only bad values. Use the existingServiceCodes assignment in
src/builder-code.ts as the target for this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9f81ebde-bebc-44db-8829-7ae85c6058c6
📒 Files selected for processing (2)
src/builder-code.test.tssrc/builder-code.ts
|
Addressed the malformed-array review note in |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/builder-code.test.ts (1)
35-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asserting non-mutation for the malformed-array path too.
The first test verifies the input array reference/content is untouched (Line 31-32); this test doesn't, leaving that guarantee unverified when the array contains non-string entries.
♻️ Suggested addition
it("preserves valid service codes from a partially malformed array", () => { + const serviceCodes = ["bc_existing", 42, null]; const ext = withBuilderCodeServiceCode({ - "builder-code": { info: { s: ["bc_existing", 42, null] } }, + "builder-code": { info: { s: serviceCodes } }, }); expect((ext["builder-code"] as BuilderCodeExtension).info.s).toEqual([ "bc_existing", BLOCKRUN_SERVICE_CODE, ]); + expect(serviceCodes).toEqual(["bc_existing", 42, null]); });🤖 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/builder-code.test.ts` around lines 35 - 44, Add a non-mutation assertion for the malformed-array case in the withBuilderCodeServiceCode test. Keep the same input object/array reference before calling the helper, then verify that the original builder-code.info.s array still contains the mixed values after the call while the returned extension has the filtered/normalized codes. This should mirror the existing immutability check from the valid-array test and cover the path with non-string entries.
🤖 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.
Nitpick comments:
In `@src/builder-code.test.ts`:
- Around line 35-44: Add a non-mutation assertion for the malformed-array case
in the withBuilderCodeServiceCode test. Keep the same input object/array
reference before calling the helper, then verify that the original
builder-code.info.s array still contains the mixed values after the call while
the returned extension has the filtered/normalized codes. This should mirror the
existing immutability check from the valid-array test and cover the path with
non-string entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b5465b7d-a46e-4102-8d55-d07cce0d08b0
📒 Files selected for processing (2)
src/builder-code.test.tssrc/builder-code.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/builder-code.ts
1 participant
Summary
builder-code.info.sservice codesFixes #199.
Proof
No changelog edit: contributor PR; maintainer release notes own the entry.
Summary by CodeRabbit
Bug Fixes
builder-code.info.sservice code entries when updating builder code.Tests