[MBL-2883] RichText parsing and conversion to Swift-friendly normalized types#2810
[MBL-2883] RichText parsing and conversion to Swift-friendly normalized types#2810stevestreza-ksr wants to merge 3 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a normalized RichTextElement model and a set of GraphAPI-to-model conversion helpers, with tests, to make Apollo-generated RichText output easier to consume for SwiftUI rendering.
Changes:
- Introduces
RichTextElement(normalized block-level RichText representation). - Adds conversion extensions on
RichTextComponentFragment/nested item types to produceRichTextElementvalues. - Adds conversion-focused tests using Swift Testing.
Reviewed changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| ServerDrivenUI/Sources/ServerDrivenUI/RichTextElement.swift | Defines the normalized RichText element enum + associated value types (text/media/oembed). |
| ServerDrivenUI/Sources/ServerDrivenUI/RichTextGQLConversions.swift | Implements GraphQL fragment/type conversion into RichTextElement. |
| ServerDrivenUI/Tests/ServerDrivenUITests/RichTextGQLConversionTests.swift | Adds unit tests validating conversions across the supported GraphQL RichText variants. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public enum HeaderLevel: String { | ||
| case one = "HEADING_1" | ||
| case two = "HEADING_2" | ||
| case three = "HEADING_3" | ||
| case four = "HEADING_4" | ||
|
|
||
| init?(_ rawValues: [String]?) { | ||
| for rawValue in rawValues ?? [] { | ||
| if let level = HeaderLevel(rawValue: rawValue) { | ||
| self = level | ||
| return | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
HeaderLevel.init(_:) introduces behavior (finding the first heading style in the styles array) but the new test suite doesn’t cover any inputs containing HEADING_1…HEADING_4. Add a test that ensures the expected header level is produced when heading styles are present (and, if relevant, which one wins when multiple heading styles appear).
| extension RichTextComponentFragment { | ||
| public func asRichTextElements() -> [RichTextElement] { | ||
| self.items.map { $0.asRichTextElement() } | ||
| } | ||
| } | ||
|
|
||
| extension RichTextComponentFragment.Item { | ||
| func asRichTextElement() -> RichTextElement { | ||
| if let x = asRichText { return x.asRichTextElement } |
There was a problem hiding this comment.
asRichTextElement is a method on RichTextComponentFragment.Item/Child but a computed property on the concrete GraphQL subtype structs. This inconsistency makes call sites harder to follow and encourages mixed usage. Consider standardizing on either a method or a property across all these conversions (and aligning asRichTextElements() accordingly).
| if let x = asRichTextPhoto { return x.asRichTextElement } | ||
| if let x = asRichTextAudio { return x.asRichTextElement } | ||
| if let x = asRichTextVideo { return x.asRichTextElement } | ||
| if let x = asRichTextOembed { return x.asRichTextElement } | ||
| return .text(makeText(text: nil, link: nil, styles: nil, children: []), nil) | ||
| } |
There was a problem hiding this comment.
When none of the asRichText* cases match, this falls back to an empty .text element. That will silently drop/obscure unexpected GraphQL union cases (e.g., when the schema adds a new RichText item) and can make rendering bugs hard to diagnose. Consider surfacing this explicitly (assert/log in debug, or add an .unknown case / return optional).
There was a problem hiding this comment.
I agree with Copilot here - I like the idea of an .unknown case or adding an assert.
amy-at-kickstarter
left a comment
amy-at-kickstarter
left a comment
There was a problem hiding this comment.
Some non-blocking nits and questions.
| if let x = asRichTextPhoto { return x.asRichTextElement } | ||
| if let x = asRichTextAudio { return x.asRichTextElement } | ||
| if let x = asRichTextVideo { return x.asRichTextElement } | ||
| if let x = asRichTextOembed { return x.asRichTextElement } | ||
| return .text(makeText(text: nil, link: nil, styles: nil, children: []), nil) | ||
| } |
There was a problem hiding this comment.
I agree with Copilot here - I like the idea of an .unknown case or adding an assert.
| if let x = asRichTextAudio { return x.asRichTextElement } | ||
| if let x = asRichTextVideo { return x.asRichTextElement } | ||
| if let x = asRichTextOembed { return x.asRichTextElement } | ||
| return .text(makeText(text: nil, link: nil, styles: nil, children: []), nil) |
There was a problem hiding this comment.
Nit: given that .text is such a generic name, I would find these easier to skim if you qualified them like
| return .text(makeText(text: nil, link: nil, styles: nil, children: []), nil) | |
| return RichTextElement.text(makeText(text: nil, link: nil, styles: nil, children: []), nil) |
| import Foundation | ||
| import GraphAPI | ||
| @testable import ServerDrivenUI | ||
| import Testing |
There was a problem hiding this comment.
It looks like SDUI is the first place we're using Swift testing instead of XCTest. Seems like a good thing, but two requests:
- Can you double-check that it's playing nicely with CircleCI?
- Can you give a quick demo/rundown for how we're going to use it in a future iOS sync?
Generated by 🚫 Danger |
📲 What
Adds parsing, conversion, and types to turn GraphQL output into much easier-to-use block-level elements for RichText. These will be used for rendering.
🤔 Why
Due to quirks in how GraphQL types are laid out, there are 40 different Swift types that get generated by Apollo. We need something a little less crazy to use in the client.
🛠 How
A
RichTextElementenum type was added which represents the different types that can be in GraphQL RichText in a normalized form that can be easily handed to SwiftUI for rendering.Then, a whoooooole bunch of converter functions were added as extensions to those 40 types to turn the GraphQL specific types into their
RichTextElementtypes.And some tests.