Fix object form rendering to avoid field duplication#11
Conversation
Lines Of Code
|
Go API Changes# summary Inferred base version: v0.2.7 Suggested version: v0.2.8 |
Unit Test Coveragetotal: (statements) 45.9% Coverage of changed lines
Coverage diff with base branchNo changes in coverage. |
There was a problem hiding this comment.
AI Code Review by LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Request Changes
This PR fixes a bug causing duplicate form entries for nested object properties by skipping object-typed properties in the form item generation. However, test coverage for the new behavior is incomplete.
📄 Documentation Diagram
This diagram documents the refactored object form rendering logic that avoids field duplication.
sequenceDiagram
participant R as Repository
participant RF as Reflector
participant IC as InterceptProp
participant FM as FormItem Map
R->>RF: Reflect(value)
RF->>IC: InterceptProp for each property
alt !params.Processed OR params.PropertySchema.HasType(Object)
IC-->>RF: return nil (skip)
else
IC->>FM: Add FormItem for property
end
note over IC: PR #35;11: skip object-type properties to avoid duplication
RF-->>R: Return FormSchema
R->>FM: Assemble final form
🌟 Strengths
- Core fix is correct and passes all CI checks.
| Priority | File | Category | Impact Summary (≤12 words) | Anchors |
|---|---|---|---|---|
| P1 | schema.go | Bug | Prevents field duplication for nested objects | |
| P2 | schema_test.go | Testing | Removes expected duplicates from test assertion | |
| P2 | .github/.../golangci-lint.yml | Maintainability | Updates CI and removes dead config |
🔍 Notable Themes
- Missing test coverage for nested objects: Despite the fix, the test suite only leverages the existing
User.countersfield, not the newly addedImagetype. Adding a unit test that verifies the form output forImage(nestedSettings) would prevent future regressions.
📈 Risk Diagram
This diagram illustrates the risk of breaking existing forms that depend on whole-object form items.
sequenceDiagram
participant Code as PR #35;11 Code
participant Form as Form Renderer
participant User as Downstream Consumer
Code->>Code: Check property type
alt Property is Object
Code->>Code: Skip adding FormItem (whole object)
note over Code: R1(P1): Object property skipped; existing forms depending on whole object key may break
Code->>Form: Only sub-fields rendered
else
Code->>Form: Add FormItem
end
Form->>User: Render form (missing expected object-level field)
💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.
| {"key":"user.code","type":"ace","aceMode":"ace/mode/sql"}, | ||
| {"key":"user.counters"},{"key":"user"}, | ||
| { | ||
| "type":"fieldhtml", |
There was a problem hiding this comment.
P2 | Confidence: High
The test expectations are updated to remove the duplicate items that were previously generated for object-typed properties. This correctly aligns the test with the fixed behaviour. However, the test coverage for nested objects is limited to the User struct's counters field and the Neighbors array items. The PR adds an Image struct with a nested Settings object as an example, but no dedicated unit test validates this scenario. Adding a test case that explicitly verifies the form output for a type like Image would provide stronger regression protection.
Code Suggestion:
Consider adding a test case using the `Image` type (similar to how the existing example harness works) to assert that no duplicate items appear and that all sub‑fields are correctly listed.| - name: golangci-lint | ||
| uses: golangci/golangci-lint-action@v8.0.0 | ||
| uses: golangci/golangci-lint-action@v9.2.0 | ||
| with: | ||
| # Required: the version of golangci-lint is required and must be specified without patch version: we always use the latest patch version. | ||
| version: v2.11.3 |
There was a problem hiding this comment.
P2 | Confidence: High
The PR removes long-commented optional configuration lines from the CI workflow, updates the golangci/golangci-lint-action from v8.0.0 to v9.2.0, and updates several dependency versions in go.mod and example/go.mod. The lint configuration .golangci.yml also disables two linters (embeddedstructfieldcheck, nilnil) and adds a complexity limit. These changes reduce technical debt but are not directly related to the bug fix. The dependency bumps may introduce slight behavioural changes in downstream libraries; however, all CI checks pass, so they are safe. No functional impact is expected.
| if !params.Processed || params.PropertySchema.HasType(jsonschema.Object) { | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
[Contextual Comment]
This comment refers to code near real line 127. Anchored to nearest_changed(125) line 125.
P1 | Confidence: High
The core logic change prevents object-typed properties from being added as a whole FormItem. Previously, the condition !params.Processed added a FormItem for every property, including those with type object. Because object properties are also recursively expanded into their sub-fields, this resulted in duplicate form entries (e.g., both {"key":"user.counters"} and the expanded sub-fields of counters appeared). The fix adds an explicit check for params.PropertySchema.HasType(jsonschema.Object), causing object-typed properties to be skipped. The test file schema_test.go confirms this by removing the expected duplicates {"key":"user.counters"}, {"key":"user"}, and {"key":"neighbors[].counters"} from the golden assertion. The example code introduces an Image type with nested ImageSettings to manually verify correct rendering with nested objects. The change is correct and passes all CI checks.
No description provided.