Conversation
no ref
- added dynamic replacement of {uuid} within the {{content}} block
- updated renderer
- added tests
WalkthroughThe changes implement a deferred UUID substitution mechanism for embedded content. The transistor-renderer is modified to use a fixed {uuid} placeholder in URLs instead of requiring memberUuid at render time. Separately, post-gating.js adds logic to replace URL-encoded {uuid} placeholders (%7Buuid%7D) with actual member.uuid values during post-processing. This shifts UUID injection from render time to a later processing stage. Test coverage is added for both the post-gating substitution logic and transistor-renderer behavior across web and email rendering paths. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
| @@ -0,0 +1,124 @@ | |||
| const assert = require('assert/strict'); | |||
There was a problem hiding this comment.
I should've added these tests with the original renderer commit.
| const iframe = document.createElement('iframe'); | ||
| iframe.setAttribute('width', '100%'); | ||
| iframe.setAttribute('height', '400'); | ||
| iframe.setAttribute('height', '325'); |
There was a problem hiding this comment.
This matches the content height based on the embed. You can see this here with https://partner.transistor.fm/ghost/embed/7d75f320-d0ed-4e6a-9ef5-840163033dbd for the moment.
There was a problem hiding this comment.
A little concerned that we have a fixed height and scrolling=no below. But that's probably an issue for another ticket.
|
|
||
| const embedUrl = new URL(`https://partner.transistor.fm/ghost/embed/${memberUuid}`); | ||
| // Use {uuid} placeholder - content.js will substitute with member UUID at request time | ||
| const embedUrl = new URL(`https://partner.transistor.fm/ghost/embed/{uuid}`); |
There was a problem hiding this comment.
We no longer get to null check here because it must get substituted at the API level.
| const member = frame.original.context.member; | ||
| if (member && member.uuid && attrs.html) { | ||
| attrs.html = attrs.html.replace(/%7Buuid%7D/gi, member.uuid); | ||
| } |
There was a problem hiding this comment.
It seemed best to handle this as encoded, though I'm ambivalent here.
|
|
||
| gating.forPost(attrs, frame); | ||
|
|
||
| // Non-encoded {uuid} should NOT be replaced |
There was a problem hiding this comment.
nit: I think this comment is unnecessary, based on the title of the test.
| // Non-encoded {uuid} should NOT be replaced |
| const result = renderForWeb(getTestData()); | ||
|
|
||
| assert.ok(result.html); | ||
| // URL constructor encodes {uuid} to %7Buuid%7D |
There was a problem hiding this comment.
nit: I think this comment is unnecessary.
| // URL constructor encodes {uuid} to %7Buuid%7D |
| it('renders iframe with URL-encoded %7Buuid%7D placeholder', function () { | ||
| const result = renderForWeb(getTestData()); | ||
|
|
||
| assert.ok(result.html); |
There was a problem hiding this comment.
nit: I think all of these assert.ok(result.html)s are unnecessary because you immediately check something about them later. I'd probably remove unless you're getting type errors.
| const result = renderForEmail(getTestData({accentColor: null})); | ||
|
|
||
| assert.ok(result.html); | ||
| // Default is #15171A |
There was a problem hiding this comment.
nit: I think it's clear without this comment.
| // Default is #15171A |
| const iframe = document.createElement('iframe'); | ||
| iframe.setAttribute('width', '100%'); | ||
| iframe.setAttribute('height', '400'); | ||
| iframe.setAttribute('height', '325'); |
There was a problem hiding this comment.
A little concerned that we have a fixed height and scrolling=no below. But that's probably an issue for another ticket.
ref https://linear.app/ghost/issue/NY-1005/
This PR fully wires up the substitution of
{uuid}in the outputted content (API response). The token is set within the rendered html content so that we don't need to re-render the content at request time for each member. This instead allows us to do it at the API, just like we do with content visibility, which is also leveraged by this functionality.This handles the actual display on the frontend. This is difficult to test because you need the integration set up with an available domain - you can see the build on site 352618 on staging.
This does not change the email rendering pipeline.
The Editor > Post Preview > Web view is still broken - we have to follow up with changes to inject a valid uuid in order to be able to preview it there. That is tracked separately.