Repository navigation
Conversation
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 5467f171dd
ℹ️ 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".
| }) | ||
| } | ||
| references.add(widgetId) | ||
| const widget = getFormWidgetById(model, widgetId) |
There was a problem hiding this comment.
When a consumer restores a large valid model on the main thread, each getFormWidgetById() call rescans the widget array, while the preceding widget loop similarly rescans fields with getFormFieldById(). This makes default-strict parseFormModel() quadratic and synchronous: a direct Node 24 check took approximately 0.5 seconds for 5,000 one-widget fields and 2.2 seconds for 10,000, blocking interaction during restoration. Build field/widget lookup maps once and reuse them, with per-field reference sets for reverse-membership checks.
Useful? React with 👍 / 👎.
|
Thanks for opening your first pull request in this repository! ✌️ |
vitormattos
left a comment
vitormattos
left a comment
There was a problem hiding this comment.
Requesting changes before merge to address two data-integrity issues documented in the inline comments: lossless model serialization and protection of widget-owned overlay identity and geometry. Each finding includes focused regression cases and positive controls.
Please also address the existing unresolved relationship-validation performance discussion without duplicating that thread, and add the missing DCO Signed-off-by trailer to the commit.
| * Suitable for persistence and transport. | ||
| */ | ||
| export function serializeFormModel(model: PDFFormModel): PDFFormModel { | ||
| return JSON.parse(JSON.stringify(model)) as PDFFormModel |
There was a problem hiding this comment.
serializeFormModel() can silently corrupt data before persistence because it calls JSON.stringify() without validating the input model.
For example, Infinity and NaN become null, while function-valued metadata is silently removed. The resulting snapshot may subsequently pass parseFormModel() even though information has already been lost.
Please validate the model's shape and JSON constraints before serialization, and add focused regression tests in tests/utils/formModel.spec.ts.
Required test cases:
- Non-finite field values: With an otherwise valid model, set
field.valuetoInfinityandNaNin named parameterized cases.serializeFormModel()must throw instead of returningnull. - Invalid metadata: Test
metadata: { fn: () => 42 }andmetadata: { date: new Date() }. Serialization must reject these inputs rather than dropping or converting their contents. - Invalid widget geometry: Set
widget.x = Infinityon an otherwise valid model. Serialization must reject it. - Positive control: Serialize a model containing
value: 42andmetadata: { source: 'import' }. Verify that the snapshot preserves both values and that a subsequentparseFormModel()round-trip produces an equivalent model. Reuse the existing valid round-trip test where possible.
The negative tests must call serializeFormModel() directly. Existing tests that only reject these inputs through parseFormModel() do not exercise the affected path.
The expected behavior is lossless serialization of valid models and explicit rejection of invalid data.
| } | ||
| if (options.resizable !== undefined) element.resizable = options.resizable | ||
| if (options.extra) { | ||
| Object.assign(element, options.extra) |
There was a problem hiding this comment.
formWidgetToElementObject() merges options.extra after assigning widget-owned properties. As a result, extra properties can overwrite the widget's overlay identity and geometry.
For example, extra: { id: 'fake', x: 999 } replaces values derived from widget.elementId and widget.x. This can break the documented widget-to-overlay relationship and cause later operations to target an incorrect overlay.
Please protect the widget-owned id, x, y, width, and height properties while continuing to support additional visual properties.
Required regression tests in tests/utils/formModel.spec.ts:
- Conflicting properties: Create a widget with
elementId: 'overlay-1',x: 12,y: 34,width: 160, andheight: 48. CallformWidgetToElementObject()withextra: { id: 'fake', x: 999, y: 999, width: 1, height: 1, icon: 'pen' }. The result must retainid: 'overlay-1'and all four original geometry values, while preservingicon: 'pen'. - Positive control: Call the same helper with
extra: { icon: 'pen' }. Verify that the additional property is retained and the widget-owned properties remain unchanged. - Existing behavior: Preserve the current conversion test without conflicting extra properties to ensure that the normal field/widget bridge does not regress.
The conflicting-input test should fail against the current implementation and pass after the fix.
Resolves: #84
📝 Summary
A single overlay rectangle cannot represent radio groups or the same form field appearing on several pages. Introduces a stable public model separating logical fields from visual widgets, providing the foundation for generic form-field editing in #85.
PDFFormFieldPDFFormWidgetPDFFormModelOne field can reference multiple widgets across pages. Unknown field types retain their identity. The existing
PDFElementObjectAPI remains compatible; form-aware consumers can adopt the model alongside their overlay store.Also adds:
docs/FORM_MODEL.mdAcroForm parsing/writing, PDF mutation, signing and host workflow concepts remain outside scope.
🧪 How to test
npm run test -- tests/utils/formModel.spec.tsnpm run test npm run build npm run validate:distValidation evidence
⚙️ Public API changes
🚧 Tasks
✅ Checklist
🤖 AI (if applicable)