Juan.naranjo/flutter embedded view schema#416
Conversation
![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: 45122096ea
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| "description": "Schema of a Wireframe type.", | ||
| "oneOf": [ | ||
| { | ||
| "$ref": "embedded-content-wireframe-schema.json" |
There was a problem hiding this comment.
After this schema ordering change, the checked-in generated declarations still reflect the old order (lib/{cjs,esm}/generated/{mobileSessionReplay,sessionReplay}.d.ts still place EmbeddedContentWireframe and EmbeddedContentWireframeUpdate last). The repository README requires regenerating JS/TS definitions after schema edits, and the check-is-built workflow runs yarn build followed by a clean git status, so this commit will produce uncommitted generated diffs and fail CI until the generated outputs are committed.
Useful? React with 👍 / 👎.
4512209 to
1d860fe
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d860fe4bf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| "description": "Schema of a Wireframe type.", | ||
| "oneOf": [ | ||
| { | ||
| "$ref": "embedded-content-wireframe-schema.json" |
There was a problem hiding this comment.
Given this change relies on mobile SDK decoders picking the first oneOf case that can decode, putting embedded-content-wireframe-schema.json first creates the same shadowing problem for existing webview payloads: the embedded-content and webview wireframe schemas have the same common shape fields and both require type plus slotId, so a type: "webview" object can be accepted as embedded content before the later webview case if the generated decoder is not enforcing the const discriminator. The update-mutation schema is reordered the same way, so webview updates are affected too; this needs an actual type/discriminator check rather than ordering one slot-backed variant ahead of the other.
Useful? React with 👍 / 👎.
@JuanNaranjoDD This seems like a bug in the decoders. This is a discriminated union; the |
sethfowler-datadog
left a comment
sethfowler-datadog
left a comment
There was a problem hiding this comment.
See above; let's investigate the decoders before making this change.
2 participants
What and why?
Reorders the oneOf entries for embedded-content-wireframe(-update)-schema.json in wireframe-schema.json and wireframe-update-mutation-schema.json, moving them to the front of the list instead of the end.
Generated Swift/Kotlin decoders try each oneOf case in declaration order and keep the first one that successfully decodes. SRShapeWireframe's required fields are a strict subset of SREmbeddedContentWireframe's, so with embedded-content listed last, an embedded-content payload was being matched and decoded as a generic shape wireframe first — silently dropping its required slotId and corrupting the type field. This broke any round-trip decode of an embedded-content wireframe (e.g. Flutter add-to-app placeholders), even though encoding was always correct.
How?
Moved the embedded-content-wireframe-schema.json ref to the first position in wireframe-schema.json's oneOf array, and embedded-content-wireframe-update-schema.json to the first position in wireframe-update-mutation-schema.json's oneOf array, so they're tried before the generic shape/text/image/placeholder/webview cases that would otherwise shadow them. No schema field changes — ordering only.