Added e2e tests for transistor integration/rendering#26806
Merged
Conversation
ref https://linear.app/ghost/issue/NY-1152/ - these tests cover the rendering cases - tests do not handle the insertion of the card, as that's handled in component tests
Contributor
WalkthroughAdds a Lexical data factory module that builds editor JSON with optional card nodes and exports 🚥 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)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
e2e/helpers/pages/public/post-page.ts (1)
9-11: Make the asserted locators explicitlypublic readonly.These fields are used directly in assertions from
e2e/tests/public/transistor.test.ts, so the access contract should be explicit on the page object.Proposed fix
- readonly transistorCard: Locator; - readonly transistorIframe: Locator; - readonly transistorPlaceholder: Locator; + public readonly transistorCard: Locator; + public readonly transistorIframe: Locator; + public readonly transistorPlaceholder: Locator;As per coding guidelines, "Page Objects should expose locators as
public readonlywhen used with assertions".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/pages/public/post-page.ts` around lines 9 - 11, The three locator fields transistorCard, transistorIframe, and transistorPlaceholder are declared as readonly but used directly in external tests, so update their declarations to be explicitly public readonly (e.g., change "readonly transistorCard: Locator;" to "public readonly transistorCard: Locator;") so the page object's contract matches usage; apply the same explicit public readonly visibility for each of transistorCard, transistorIframe, and transistorPlaceholder in the class where they are declared.e2e/tests/public/transistor.test.ts (1)
52-54: Prefer locator assertions overgetAttribute()snapshots here.
getAttribute()gives you a one-timestring | nulland bypasses Playwright's retry loop.toHaveAttribute()will keep auto-waiting for the finaldata-srcvalue and produce a clearer failure if the attribute is late or missing.As per coding guidelines, "Use Playwright's auto-waiting capabilities in E2E tests".
Also applies to: 75-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/public/transistor.test.ts` around lines 52 - 54, Replace the one-time snapshot assertions that call postPage.transistorIframe.getAttribute(...) with Playwright locator assertions that auto-wait: use expect(postPage.transistorIframe).toHaveAttribute('data-src', valueMatcher) instead of capturing a string and testing it; assert the attribute does not contain the encoded placeholder and that it contains member.uuid (e.g. use negative/positive matchers or a regex) and apply the same change for the second occurrence referenced around lines 75-77; keep references to postPage.transistorIframe, getAttribute, toHaveAttribute, and member.uuid to locate and update the two assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/data-factory/factories/lexical.ts`:
- Around line 27-33: The Transistor fixture in CARD_DEFAULTS is missing the
visibility payload, so update the CARD_DEFAULTS constant (the 'transistor' entry
of type CardNode) to include the same node.visibility shape your production
renderer expects (the member-gating fields used by createWithCards('transistor')
/ the renderer), e.g. add a visibility property with the proper keys and
defaults used in production so test serialization follows the real Transistor
card shape and exercises the gating path rather than falling back to undefined
behavior.
- Around line 74-78: buildLexical currently generates its own lorem paragraph
when no cards are passed, causing inconsistent bodies vs the post factory that
builds html/plaintext from a separate content string; remove the internal
faker.lorem.paragraphs call and instead add a parameter (e.g., bodyText?:
string) to buildLexical so callers supply the paragraph text, use that text to
create the buildParagraphNode when cards.length === 0, and update the post
factory caller to pass its content through to buildLexical; ensure buildLexical
and buildParagraphNode usage and tests are updated accordingly.
In `@e2e/helpers/services/settings/settings-service.ts`:
- Around line 76-79: The helper setTransistorEnabled currently writes a
top-level settings key 'transistor' which is ignored; change it to update the
labs JSON instead by calling the existing updateLabsSettings function with
{transistor: enabled} so the labs payload is merged/preserved; locate
setTransistorEnabled in settings-service.ts and replace the direct PUT (key:
'transistor') call with a call to updateLabsSettings({transistor: enabled}) to
ensure the real feature flag is toggled.
---
Nitpick comments:
In `@e2e/helpers/pages/public/post-page.ts`:
- Around line 9-11: The three locator fields transistorCard, transistorIframe,
and transistorPlaceholder are declared as readonly but used directly in external
tests, so update their declarations to be explicitly public readonly (e.g.,
change "readonly transistorCard: Locator;" to "public readonly transistorCard:
Locator;") so the page object's contract matches usage; apply the same explicit
public readonly visibility for each of transistorCard, transistorIframe, and
transistorPlaceholder in the class where they are declared.
In `@e2e/tests/public/transistor.test.ts`:
- Around line 52-54: Replace the one-time snapshot assertions that call
postPage.transistorIframe.getAttribute(...) with Playwright locator assertions
that auto-wait: use
expect(postPage.transistorIframe).toHaveAttribute('data-src', valueMatcher)
instead of capturing a string and testing it; assert the attribute does not
contain the encoded placeholder and that it contains member.uuid (e.g. use
negative/positive matchers or a regex) and apply the same change for the second
occurrence referenced around lines 75-77; keep references to
postPage.transistorIframe, getAttribute, toHaveAttribute, and member.uuid to
locate and update the two assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a4beae3f-d949-4202-b6a4-2428651f6d9f
📒 Files selected for processing (6)
e2e/data-factory/factories/lexical.tse2e/data-factory/factories/post-factory.tse2e/helpers/pages/public/post-page.tse2e/helpers/services/settings/settings-service.tse2e/tests/public/transistor.test.tse2e/tsconfig.json
Comment on lines
+74
to
+78
| export function buildLexical(...cards: CardSpec[]): string { | ||
| let children: (LexicalParagraphNode | CardNode)[]; | ||
|
|
||
| if (cards.length === 0) { | ||
| children = [buildParagraphNode(faker.lorem.paragraphs(3))]; |
Contributor
There was a problem hiding this comment.
buildLexical() now creates a second, unrelated body for default posts.
When no cards are passed, this helper generates its own lorem text. e2e/data-factory/factories/post-factory.ts still builds html and plaintext from a different content string, so default post fixtures now have inconsistent bodies across fields. Please let callers provide the paragraph text instead of generating another one here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/data-factory/factories/lexical.ts` around lines 74 - 78, buildLexical
currently generates its own lorem paragraph when no cards are passed, causing
inconsistent bodies vs the post factory that builds html/plaintext from a
separate content string; remove the internal faker.lorem.paragraphs call and
instead add a parameter (e.g., bodyText?: string) to buildLexical so callers
supply the paragraph text, use that text to create the buildParagraphNode when
cards.length === 0, and update the post factory caller to pass its content
through to buildLexical; ensure buildLexical and buildParagraphNode usage and
tests are updated accordingly.
Comment on lines
+76
to
+79
| async setTransistorEnabled(enabled: boolean) { | ||
| const data = {settings: [{key: 'transistor', value: enabled}]}; | ||
| const response = await this.request.put(`${this.adminEndpoint}/settings`, {data}); | ||
| return await response.json() as SettingsResponse; |
Contributor
There was a problem hiding this comment.
Update the labs setting, not a standalone transistor key.
Ghost stores transistor inside the labs JSON blob, so writing {key: 'transistor', value: enabled} here will not toggle the real feature flag. That means this helper can return success while leaving Transistor disabled. Reuse updateLabsSettings({transistor: enabled}) so the existing labs payload is preserved.
Proposed fix
async setTransistorEnabled(enabled: boolean) {
- const data = {settings: [{key: 'transistor', value: enabled}]};
- const response = await this.request.put(`${this.adminEndpoint}/settings`, {data});
- return await response.json() as SettingsResponse;
+ return await this.updateLabsSettings({transistor: enabled});
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/helpers/services/settings/settings-service.ts` around lines 76 - 79, The
helper setTransistorEnabled currently writes a top-level settings key
'transistor' which is ignored; change it to update the labs JSON instead by
calling the existing updateLabsSettings function with {transistor: enabled} so
the labs payload is merged/preserved; locate setTransistorEnabled in
settings-service.ts and replace the direct PUT (key: 'transistor') call with a
call to updateLabsSettings({transistor: enabled}) to ensure the real feature
flag is toggled.
Contributor
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 23014610010 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
Member
🤖 Velo CI Failure AnalysisClassification: 🟠 SOFT FAIL
|
Contributor
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 23017031081 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
Member
🤖 Velo CI Failure AnalysisClassification: 🟠 SOFT FAIL
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
e2e/tests/public/transistor.test.ts (1)
78-90: Consider using page object method for preview navigation.Line 82 uses
postPage.goto()with a direct URL path. This works but couples the test to the URL structure. If agotoPreview(uuid, memberStatus)method exists or could be added toPostPage, it would improve maintainability.Otherwise, the test correctly validates the preview mode placeholder behavior per the Ghost core
_buildTransistorPlaceholder()function.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/public/transistor.test.ts` around lines 78 - 90, Test uses PostPage.goto(...) with a raw URL which couples the test to routing; add or use a PostPage method like gotoPreview(uuid, memberStatus) and update this test to call postPage.gotoPreview(post.uuid, 'free') instead. Locate the PostPage class (methods: goto, waitForPostToLoad, postContent, transistorPlaceholder, transistorIframe) and implement gotoPreview(uuid, memberStatus) to build the preview path and navigate, then replace the direct call in transistor.test.ts to use gotoPreview for clearer intent and reduced URL coupling.e2e/data-factory/factories/lexical.ts (1)
56-61: Shallow merge may lose nested visibility properties.When merging overrides, this uses spread which performs a shallow copy. If a caller passes
{transistor: {visibility: {web: {nonMember: true}}}}, the entirevisibilityobject gets replaced, losing theConsider a deep merge for nested objects like
visibility:♻️ Suggested approach
function resolveCard(spec: CardSpec): CardNode { if (typeof spec === 'string') { const defaults = CARD_DEFAULTS[spec]; if (!defaults) { throw new Error(`Unknown card type: "${spec}". Register it in CARD_DEFAULTS in lexical.ts.`); } return {...defaults}; } const [cardType, overrides] = Object.entries(spec)[0]; const defaults = CARD_DEFAULTS[cardType]; if (!defaults) { throw new Error(`Unknown card type: "${cardType}". Register it in CARD_DEFAULTS in lexical.ts.`); } - return {...defaults, ...overrides}; + // Deep merge for nested objects like visibility + const result = {...defaults}; + for (const [key, value] of Object.entries(overrides)) { + if (typeof value === 'object' && value !== null && !Array.isArray(value) && typeof result[key] === 'object') { + result[key] = {...(result[key] as Record<string, unknown>), ...value}; + } else { + result[key] = value; + } + } + return result; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/data-factory/factories/lexical.ts` around lines 56 - 61, The merge in the factory uses a shallow spread when returning {...defaults, ...overrides} which will overwrite nested objects (e.g., visibility) and lose sub-properties; update the factory to perform a deep merge of defaults and overrides (at least for nested objects like visibility/transistor) before returning so nested keys are preserved—use a utility or recursive merge inside the lexical factory function that merges CARD_DEFAULTS[cardType] with overrides (merging nested plain objects rather than replacing them) and ensure CARD_DEFAULTS and the returned object keep unchanged nested defaults not present in overrides.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/data-factory/factories/lexical.ts`:
- Around line 56-61: The merge in the factory uses a shallow spread when
returning {...defaults, ...overrides} which will overwrite nested objects (e.g.,
visibility) and lose sub-properties; update the factory to perform a deep merge
of defaults and overrides (at least for nested objects like
visibility/transistor) before returning so nested keys are preserved—use a
utility or recursive merge inside the lexical factory function that merges
CARD_DEFAULTS[cardType] with overrides (merging nested plain objects rather than
replacing them) and ensure CARD_DEFAULTS and the returned object keep unchanged
nested defaults not present in overrides.
In `@e2e/tests/public/transistor.test.ts`:
- Around line 78-90: Test uses PostPage.goto(...) with a raw URL which couples
the test to routing; add or use a PostPage method like gotoPreview(uuid,
memberStatus) and update this test to call postPage.gotoPreview(post.uuid,
'free') instead. Locate the PostPage class (methods: goto, waitForPostToLoad,
postContent, transistorPlaceholder, transistorIframe) and implement
gotoPreview(uuid, memberStatus) to build the preview path and navigate, then
replace the direct call in transistor.test.ts to use gotoPreview for clearer
intent and reduced URL coupling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2ba1f93b-bbac-41a5-b3e6-51aa9d7f7b18
📒 Files selected for processing (2)
e2e/data-factory/factories/lexical.tse2e/tests/public/transistor.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref https://linear.app/ghost/issue/NY-1152/