Generalized visibility property logic and added to CallToActionNode #1431
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
packages/koenig-lexical/src/utils/visibility.jsOops! Something went wrong! :( ESLint: 8.57.0 ESLint couldn't find the config "react-app" to extend from. Please check that the name of the config is correct. The config "react-app" was referenced from the config file in "/packages/koenig-lexical/.eslintrc.cjs". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. packages/kg-default-nodes/lib/generate-decorator-node.jsOops! Something went wrong! :( ESLint: 8.57.0 Error: Failed to load parser '@babel/eslint-parser' declared in 'packages/kg-default-nodes/.eslintrc.js': Cannot find module '@babel/eslint-parser'
packages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.jsOops! Something went wrong! :( ESLint: 8.57.0 Error: Failed to load parser '@babel/eslint-parser' declared in 'packages/kg-default-nodes/.eslintrc.js': Cannot find module '@babel/eslint-parser'
WalkthroughThis pull request introduces a new property, Changes
Sequence Diagram(s)sequenceDiagram
participant Node as CallToAction/Html Node
participant VisibilityUtil as buildDefaultVisibility()
participant Default as DEFAULT_VISIBILITY
Node ->> VisibilityUtil: Request default visibility
VisibilityUtil ->> Default: Create deep copy (using JSON methods)
Default -->> VisibilityUtil: Return copy
VisibilityUtil -->> Node: Return default visibility
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.js (1)
22-47: Update constructor to handle visibility parameter.The constructor should be updated to handle the new
visibilityproperty consistently with other properties.constructor({ layout, textValue, showButton, buttonText, buttonUrl, buttonColor, buttonTextColor, hasSponsorLabel, backgroundColor, hasImage, - imageUrl + imageUrl, + visibility } = {}, key) { super(key); this.__layout = layout ?? 'minimal'; this.__textValue = textValue ?? ''; this.__showButton = showButton ?? false; this.__buttonText = buttonText ?? ''; this.__buttonUrl = buttonUrl ?? ''; this.__buttonColor = buttonColor ?? 'none'; this.__buttonTextColor = buttonTextColor ?? 'none'; this.__hasSponsorLabel = hasSponsorLabel ?? true; this.__backgroundColor = backgroundColor ?? 'grey'; this.__hasImage = hasImage ?? false; this.__imageUrl = imageUrl ?? ''; + this.__visibility = visibility ?? buildDefaultVisibility(); }
🧹 Nitpick comments (2)
packages/kg-default-nodes/lib/utils/visibility.js (1)
14-17: Consider using structuredClone for better performance.While
JSON.parse(JSON.stringify())works for this case,structuredClone()is a more efficient built-in method for deep cloning objects.- return JSON.parse(JSON.stringify(DEFAULT_VISIBILITY)); + return structuredClone(DEFAULT_VISIBILITY);packages/kg-default-nodes/test/nodes/call-to-action.test.js (1)
69-69: Consider adding edge case tests for visibility.The visibility getter and setter tests look good and properly verify the basic functionality. Consider adding tests for:
- Invalid visibility object structures
- Partial visibility updates
- Null/undefined values
Example test cases:
it('handles invalid visibility objects', editorTest(function () { const callToActionNode = new CallToActionNode(); // Should retain default when setting invalid structure callToActionNode.visibility = {invalid: true}; callToActionNode.visibility.should.deepEqual(utils.visibility.buildDefaultVisibility()); // Should handle null/undefined callToActionNode.visibility = null; callToActionNode.visibility.should.deepEqual(utils.visibility.buildDefaultVisibility()); })); it('handles partial visibility updates', editorTest(function () { const callToActionNode = new CallToActionNode(); const defaultVisibility = utils.visibility.buildDefaultVisibility(); // Should merge with existing visibility callToActionNode.visibility = { web: {nonMembers: false} }; callToActionNode.visibility.should.deepEqual({ ...defaultVisibility, web: { ...defaultVisibility.web, nonMembers: false } }); }));Also applies to: 119-137
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.js(2 hunks)packages/kg-default-nodes/lib/nodes/html/HtmlNode.js(1 hunks)packages/kg-default-nodes/lib/utils/visibility.js(2 hunks)packages/kg-default-nodes/test/nodes/call-to-action.test.js(4 hunks)packages/koenig-lexical/src/utils/visibility.js(1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/kg-default-nodes/lib/utils/visibility.js (1)
Learnt from: kevinansfield
PR: TryGhost/Koenig#1424
File: packages/kg-default-nodes/lib/utils/visibility.js:14-18
Timestamp: 2025-01-28T18:55:16.157Z
Learning: The `usesOldVisibilityFormat` function in `visibility.js` already checks for the existence of the 'web' property using `Object.prototype.hasOwnProperty.call(visibility, 'web')` before accessing `visibility.web.nonMember`.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node 20.11.1
🔇 Additional comments (6)
packages/kg-default-nodes/lib/utils/visibility.js (1)
4-12: LGTM! Good practice to prevent external modifications.Making
DEFAULT_VISIBILITYa local constant prevents accidental modifications from external code.packages/kg-default-nodes/lib/nodes/html/HtmlNode.js (1)
5-5: LGTM! Consistent usage of buildDefaultVisibility.The changes correctly implement the new visibility initialization pattern in both the property definition and constructor.
Also applies to: 10-10, 15-15
packages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.js (1)
3-3: LGTM! Visibility property added correctly.The import and property definition are properly implemented.
Also applies to: 17-18
packages/koenig-lexical/src/utils/visibility.js (1)
3-3: LGTM! Using buildDefaultVisibility correctly.The change correctly uses the new factory function to initialize the default visibility.
packages/kg-default-nodes/test/nodes/call-to-action.test.js (2)
5-5: LGTM!The import of
utilsis correctly structured and aligns with the PR's objective to support visibility functionality.
41-42: LGTM!The dataset initialization correctly uses the new
buildDefaultVisibility()factory function for the visibility property, which aligns with the PR's goal of preventing issues with nested object references.
kevinansfield
force-pushed
the
cta-card-visibility-property
branch
from
10cc158 to
7177aaa
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/kg-default-nodes/test/nodes/call-to-action.test.js (1)
69-69: Consider adding more test cases for visibility property.The current tests verify basic getter/setter functionality well. However, consider adding these test cases for more comprehensive coverage:
- Partial updates to visibility structure
- Invalid visibility structure handling
- Edge cases (null, undefined values)
Example test cases to add:
it('handles partial visibility updates', editorTest(function () { const callToActionNode = new CallToActionNode(); // Test partial web update callToActionNode.visibility = { web: { nonMembers: false } }; callToActionNode.visibility.web.nonMembers.should.equal(false); callToActionNode.visibility.web.memberSegment.should.equal(''); // Test partial email update callToActionNode.visibility = { email: { memberSegment: 'paid' } }; callToActionNode.visibility.email.memberSegment.should.equal('paid'); })); it('validates visibility structure', editorTest(function () { const callToActionNode = new CallToActionNode(); // Test invalid structure (() => { callToActionNode.visibility = { invalid: true }; }).should.throw(); // Test null/undefined (() => { callToActionNode.visibility = null; }).should.throw(); }));Also applies to: 119-137
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.js(2 hunks)packages/kg-default-nodes/lib/nodes/html/HtmlNode.js(1 hunks)packages/kg-default-nodes/lib/utils/visibility.js(2 hunks)packages/kg-default-nodes/test/nodes/call-to-action.test.js(4 hunks)packages/koenig-lexical/src/utils/visibility.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.js
- packages/kg-default-nodes/lib/utils/visibility.js
- packages/kg-default-nodes/lib/nodes/html/HtmlNode.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node 20.11.1
🔇 Additional comments (3)
packages/koenig-lexical/src/utils/visibility.js (1)
46-47: LGTM! Good improvement to prevent shared state mutation.The change from using a default parameter to explicitly creating default visibility objects prevents issues with modifying nested objects by reference. This ensures each function call gets a fresh copy of the default visibility settings.
packages/kg-default-nodes/test/nodes/call-to-action.test.js (2)
5-5: LGTM: Import changes are correct.The addition of
utilsto the destructured import is appropriate for accessing the visibility utilities.
41-42: LGTM: Dataset initialization properly includes visibility.The dataset correctly initializes the visibility property using
buildDefaultVisibility(), aligning with the PR's objective to prevent issues with modifying nested objects by reference.
kevinansfield
force-pushed
the
cta-card-visibility-property
branch
from
7177aaa to
549c94c
Compare
kevinansfield
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/kg-default-nodes/lib/generate-decorator-node.js (1)
150-155: Add more explicit handling for unexpected old visibility formats.
The migration logic is good, but consider logging or fallback behavior if the old schema doesn't match expectations. This could improve debugging and reliability.packages/kg-default-nodes/lib/kg-default-nodes.js (1)
66-72: LGTM! Consider adding JSDoc comments.The new
utilsexport object provides a clean interface for accessing utility functions. The structure looks good and maintains backward compatibility.Consider adding JSDoc comments to document the purpose and usage of the exported utilities:
+/** + * Utility functions for node generation and visibility management + * @type {Object} + * @property {Function} generateDecoratorNode - Generates a decorator node with optional visibility + * @property {Object} visibility - Utilities for managing node visibility + */ export const utils = { generateDecoratorNode, visibility: visibilityUtils };packages/kg-default-nodes/test/generate-decorator-node.test.js (1)
79-103: Consider adding more test cases for migration.The migration test is good, but consider adding tests for:
- Invalid old format data
- Empty or null segment values
- Multiple segments
Example test case:
it('handles invalid old visibility format gracefully', editorTest(function () { const node = NodeWithVisibility.importJSON({ visibility: { showOnWeb: 'invalid', showOnEmail: null, segment: undefined } }); // should fall back to defaults for invalid values node.visibility.should.deepEqual({ showOnWeb: false, showOnEmail: false, segment: '', web: { nonMember: true, memberSegment: '' }, email: { memberSegment: '' } }); }));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/kg-default-nodes/lib/generate-decorator-node.js(4 hunks)packages/kg-default-nodes/lib/kg-default-nodes.js(1 hunks)packages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.js(2 hunks)packages/kg-default-nodes/lib/nodes/html/HtmlNode.js(1 hunks)packages/kg-default-nodes/lib/utils/visibility.js(2 hunks)packages/kg-default-nodes/test/generate-decorator-node.test.js(1 hunks)packages/kg-default-nodes/test/nodes/call-to-action.test.js(3 hunks)packages/koenig-lexical/src/utils/visibility.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/koenig-lexical/src/utils/visibility.js
- packages/kg-default-nodes/lib/utils/visibility.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node 20.11.1
🔇 Additional comments (9)
packages/kg-default-nodes/lib/generate-decorator-node.js (3)
3-3: Import statement changes look consistent with usage.
All imported utilities appear to be properly referenced throughout the file.
39-39: Doc and function signature alignment looks good.
The inclusion of thehasVisibilityparameter is clearly documented and matches the function definition.Also applies to: 42-42
51-62: Conditional property addition for ‘visibility’ is well-structured.
Using a getter to create a fresh visibility object viabuildDefaultVisibility()avoids reference-sharing issues. Great choice.packages/kg-default-nodes/lib/nodes/html/HtmlNode.js (2)
7-7:hasVisibilityimplementation is consistent.
Ensuring this node supports visibility complements the updates ingenerateDecoratorNode.
9-9: Property definition appears well-chosen.
DeclaringhtmlaswordCount: truehelps accurately include HTML text in word counts.packages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.js (2)
4-6: Visibility support for CallToActionNode is properly enabled.
This lines up with the newhasVisibilitypattern ingenerateDecoratorNode.
19-20: Properties array finalization is valid.
No issues with the structure; it's consistent withgenerateDecoratorNodeusage.packages/kg-default-nodes/test/generate-decorator-node.test.js (1)
41-47: LGTM! Tests cover the core functionality.The test verifies that the default visibility is correctly initialized and accessible through different methods.
packages/kg-default-nodes/test/nodes/call-to-action.test.js (1)
141-144: LGTM! Dataset includes visibility.The dataset correctly includes the visibility property with default values.
ref https://linear.app/ghost/issue/PLG-332 - switched from exporting `DEFAULT_VISIBILITY` as a constant to a `buildDefaultVisibility()` factory function to avoid potential modify-by-reference of the nested objects in an exported constant - added `hasVisibility` option to `generateDecoratorNode()` - keeps all visibility related logic in one place - adds `visibility` property to the node - ensures default value is correct and can't accidentally be modified if nested objects in `node.visibility` are modified - adds automatic migration of `visibility` from old beta format to new when importing older serialized documents - added associated test file so behaviour can be tested outside of card-specific tets - updated `HtmlNode` to use new `hasVisibility` option - added `hasVisibility` option to new `CallToActionNode`
kevinansfield
force-pushed
the
cta-card-visibility-property
branch
from
549c94c to
ac621a8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/kg-default-nodes/test/generate-decorator-node.test.js (1)
49-103: Consider adding edge cases to strengthen the test suite.While the current tests are good, consider adding these scenarios:
- Invalid visibility format handling
- Partial visibility updates
- Edge cases for segment values
+ it('handles invalid visibility format gracefully', editorTest(function () { + const node = NodeWithVisibility.importJSON({ + visibility: { + invalid: 'format' + } + }); + node.visibility.should.deepEqual(defaultVisibility); + })); + it('handles partial visibility updates', editorTest(function () { + const node = $createNodeWithVisibility(); + node.visibility.web.nonMember = false; + node.visibility.web.should.deepEqual({ + nonMember: false, + memberSegment: defaultVisibility.web.memberSegment + }); + }));packages/kg-default-nodes/lib/generate-decorator-node.js (2)
147-161: Add error handling for invalid visibility format.While the migration logic is good, consider adding error handling for invalid visibility formats to prevent runtime errors.
static importJSON(serializedNode) { const data = {}; // migrate older nodes that were saved with an earlier version of the visibility format const visibility = serializedNode.visibility; - if (visibility && usesOldVisibilityFormat(visibility)) { + if (visibility) { + try { + if (usesOldVisibilityFormat(visibility)) { + migrateOldVisibilityFormat(visibility); + } + } catch (error) { + console.warn(`Invalid visibility format: ${error.message}`); + data.visibility = buildDefaultVisibility(); + } + } properties.forEach((prop) => { data[prop.name] = serializedNode[prop.name]; }); return new this(data); }🧰 Tools
🪛 Biome (1.9.4)
[error] 160-160: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
244-261: Consider caching the visibility format check.The implementation is correct but could be optimized by caching the format check result.
getIsVisibilityActive() { if (!properties.some(prop => prop.name === 'visibility')) { return false; } const self = this.getLatest(); const visibility = self.__visibility; + + // Cache the format check result + if (!this.__visibilityFormat) { + this.__visibilityFormat = usesOldVisibilityFormat(visibility) ? 'old' : 'new'; + } - if (usesOldVisibilityFormat(visibility)) { + if (this.__visibilityFormat === 'old') { return visibility.showOnEmail === false || visibility.showOnWeb === false || visibility.segment !== ''; } else { return visibility.web.nonMember === false || visibility.web.memberSegment !== ALL_MEMBERS_SEGMENT || visibility.email.memberSegment !== ALL_MEMBERS_SEGMENT; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/kg-default-nodes/lib/generate-decorator-node.js(4 hunks)packages/kg-default-nodes/lib/kg-default-nodes.js(1 hunks)packages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.js(2 hunks)packages/kg-default-nodes/lib/nodes/html/HtmlNode.js(1 hunks)packages/kg-default-nodes/lib/utils/visibility.js(2 hunks)packages/kg-default-nodes/test/generate-decorator-node.test.js(1 hunks)packages/kg-default-nodes/test/nodes/call-to-action.test.js(3 hunks)packages/koenig-lexical/src/utils/visibility.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.js
- packages/kg-default-nodes/test/nodes/call-to-action.test.js
- packages/kg-default-nodes/lib/utils/visibility.js
- packages/kg-default-nodes/lib/kg-default-nodes.js
- packages/koenig-lexical/src/utils/visibility.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node 20.11.1
🔇 Additional comments (7)
packages/kg-default-nodes/lib/nodes/html/HtmlNode.js (1)
6-10: LGTM! Verify visibility behavior in integration tests.The changes correctly implement the new visibility handling by:
- Adding
hasVisibility: trueto utilize the centralized visibility logic- Removing the
visibilityproperty to prevent duplicate definitionRun the following script to verify that the visibility behavior is tested:
✅ Verification successful
Visibility integration tests confirmed
The integration tests in
packages/kg-default-nodes/test/nodes/html.test.jsexercise theHtmlNodebehavior with a visibility parameter. The tests verify the node's export behavior—ensuring that types such as"value"and"inner"are returned as expected—which confirms that:
- The addition of
hasVisibility: truecorrectly delegates visibility handling to the centralized logic.- The removal of the
visibilityproperty avoids duplicate definitions and aligns with the new implementation.No further issues were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for visibility-related tests for HtmlNode # Search for visibility-related tests in test files rg -A 5 'HtmlNode.*visibility' --type jsLength of output: 4218
packages/kg-default-nodes/test/generate-decorator-node.test.js (4)
1-21: Well-structured test setup with proper error handling!The test setup is clean and follows best practices:
- Proper error handling in the
editorTestutility ensures test failures are correctly reported- Clear comments explaining the importance of using the utility function
23-39: Clean test node setup with proper encapsulation!The test node setup follows good practices:
- Node configuration is isolated in the
beforehook- Node creation is encapsulated in a utility function for reuse
41-47: Comprehensive visibility verification!The test thoroughly verifies the default visibility:
- Checks visibility through direct property access
- Verifies dataset access
- Validates JSON export
105-127: Good coverage of constructor initialization!The test properly verifies that visibility can be set through the constructor with complete visibility structure.
packages/kg-default-nodes/lib/generate-decorator-node.js (2)
1-61: Excellent implementation of immutable default visibility!The implementation follows best practices:
- Uses getter for default visibility to prevent mutation
- Clear JSDoc documentation of the new parameter
- Proper validation of arguments
282-293: Clean implementation of property accessors!The implementation follows best practices:
- Uses Object.defineProperty for clean getter/setter definition
- Properly handles Lexical's immutability with getLatest/getWritable
ref https://linear.app/ghost/issue/PLG-332
DEFAULT_VISIBILITYas a constant to abuildDefaultVisibility()factory function to avoid potential modify-by-reference of the nested objects in an exported constanthasVisibilityoption togenerateDecoratorNode()visibilityproperty to the nodenode.visibilityare modifiedvisibilityfrom old beta format to new when importing older serialized documentsHtmlNodeto use newhasVisibilityoptionhasVisibilityoption to newCallToActionNode