1106 content planner create spark limit modal notification#23173
Conversation
|
A merge conflict has been detected for the proposed code changes in this PR. Please resolve the conflict by either rebasing the PR or merging in changes from the base branch. |
…te-spark-limit-modal-notification
|
A merge conflict has been detected for the proposed code changes in this PR. Please resolve the conflict by either rebasing the PR or merging in changes from the base branch. |
…06-content-planner-create-spark-limit-modal-notification
There was a problem hiding this comment.
Pull request overview
Adds/adjusts the “sparks limit” notification behavior and integrates it into the AI Content Planner’s content suggestions modal, alongside a refactor that splits the modal UI into smaller components.
Changes:
- Integrates
SparksLimitNotificationinto the content planner’s content suggestions modal and updates related tests. - Updates
SparksLimitNotificationtitle/visibility logic for free vs. unlimited sparks. - Refactors the content suggestions modal by extracting modal content, suggestion list, and suggestion button into separate components.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/js/tests/ai-content-planner/components/feature-modal.test.js | Mocks SparksLimitNotification to keep planner tests isolated. |
| packages/js/tests/ai-content-planner/components/content-suggestions-modal.test.js | Mocks SparksLimitNotification for the modal unit tests. |
| packages/js/src/ai-generator/components/sparks-limit-notification.js | Changes notification display threshold + introduces dynamic “sparks left” title logic. |
| packages/js/src/ai-content-planner/containers/content-suggestions-modal.js | Renames mapped prop to usageCountStatus. |
| packages/js/src/ai-content-planner/components/suggestion-list.js | New extracted suggestions list UI. |
| packages/js/src/ai-content-planner/components/suggestion-button.js | New extracted suggestion button + skeleton loader. |
| packages/js/src/ai-content-planner/components/content-suggestions-modal.js | Refactors modal to use extracted content component and adds notifications container + sparks toast. |
| packages/js/src/ai-content-planner/components/content-suggestions-modal-content.js | New extracted loading/success modal content with (optional) transitions. |
Comments suppressed due to low confidence (1)
packages/js/src/ai-generator/components/sparks-limit-notification.js:176
SparksLimitUpsellContentexpects anisWooProductEntityprop, but the call site passesisWooUpsell. As a result the component will always use the defaultfalseand display the non-Woo copy even for Woo upsells. Align the prop name (or update the component signature) so the correct upsell label is shown.
{ hasUnlimitedSparks
? <SparksLimitContent onClose={ hideNotification } />
: <SparksLimitUpsellContent onClose={ hideNotification } upsellLink={ upsellLink } isWooUpsell={ isWooUpsell } />
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
A merge conflict has been detected for the proposed code changes in this PR. Please resolve the conflict by either rebasing the PR or merging in changes from the base branch. |
…06-content-planner-create-spark-limit-modal-notification
…06-content-planner-create-spark-limit-modal-notification
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
packages/js/src/ai-content-planner/store/modal.js:21
openModalwas removed from the modal slice actions, but there are still callers expecting it on the content planner store (e.g.packages/js/src/ai-content-planner/blocks/banner-block.jscallsconst { openModal } = useDispatch( CONTENT_PLANNER_STORE ); openModal();). That will makeopenModalundefined and throw at runtime when the inline banner is clicked. Either reintroduce anopenModalaction (and keepisOpenmeaningful) or update remaining consumers to rely only onfeatureModalStatusto control visibility.
initialState: {
isOpen: false,
featureModalStatus: null,
},
reducers: {
closeModal: () => slice.getInitialState(),
setFeatureModalStatus: ( state, { payload } ) => {
state.featureModalStatus = payload;
},
},
packages/js/src/ai-content-planner/components/outline-modal-content.js:280
- In the outline error state, the "Try again" button will effectively be a no-op because
onRetryis expected as a prop but isn't provided by the connected container (containers/outline-modal-content.jsonly injectsonBackToSuggestions). As a result,ContentPlannerErrorfalls back to its defaultnoopretry handler. Consider wiring retry up here (e.g. calluseFetchContentOutline()with the currentsuggestion) or have the container supply anonRetrycallback.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…during in-flight requests
FeatureModal and App were both instantiating useApplyOutline with the same editedOutlineRef, resulting in two hook instances for the same action. Move the single instance to App (which already owns the ref) and pass handleApplyOutline down as a prop to FeatureModal. Cleanup of tests
|
Looser CR + acceptance test than usual, as discussed in the standup, to unblock the numerous moving parts. Merging for now, I'll share a comment with some acceptance-related notes after that. |
| useEffect( () => { | ||
| const showNotificationPremium = hasUnlimitedSparks && usageCount === usageCountLimit; | ||
| const showNotificationFree = ! hasUnlimitedSparks && isUsageCountLimitReached; | ||
| const showNotificationFree = ! hasUnlimitedSparks && remainingSparks <= 5; |
There was a problem hiding this comment.
Shouldn't we add remainingSparks to the dependency list?
|
|
||
| return { | ||
| onClose: closeModal, | ||
| setStatus: setFeatureModalStatus, |
There was a problem hiding this comment.
This is not used in the approve modal component, so maybe it's safe to remove?
There was a problem hiding this comment.
to be more precise: the setStatus seems that it's not being used by the respective component
| @@ -120,9 +120,11 @@ export const SparksLimitNotification = ( { className = "" } ) => { | |||
|
|
|||
| const [ showNotification, , setShowNotification, , hideNotification ] = useToggleState( usageCount === usageCountLimit ); | |||
There was a problem hiding this comment.
The usageCount === usageCountLimit check is now obsolete in non Premium setups, so we have a mismatch here with the useEffect below.
Maybe consider making it into
const [ showNotification, , setShowNotification, , hideNotification ] = useToggleState( false );
and let the effect own visibility?
|
As per the above, I noticed a couple of visual discrepancies @vraja-pro , let's discuss which ones are expected, which ones can be fixed later and which ones are blockers:
|




Context
Summary
This PR can be summarized in the following changelog entry:
Relevant technical choices:
Test instructions
Test instructions for the acceptance test before the PR gets merged
This PR can be acceptance tested by following these steps:
4 free sparks left!.You're out of free sparks!.Test AI generator:
Reset sparks with new site.
Without focus keyphrase try to use the AI generator
Check focus is switched to the focus keyphrase field.
Use AI generator untill you get to 5 sparks
Check you see the same notifications
5 free sparks left!.Keep generating and check the number changes in the notification.
When you reach the 10th spark check you get
You're out of free sparks!notification.With premium, change the usage count via the console to reach 100.
Relevant test scenarios
Test instructions for QA when the code is in the RC
QA can test this PR by following these steps:
Impact check
This PR affects the following parts of the plugin, which may require extra testing:
Other environments
[shopify-seo], added test instructions for Shopify and attached theShopifylabel to this PR.[yoast-doc-extension], added test instructions for Yoast SEO for Google Docs and attached theGoogle Docs Add-onlabel to this PR.Documentation
Quality assurance
grunt build:imagesand commited the results, if my PR introduces new images or SVGs.Innovation
innovationlabel.Fixes #https://github.com/Yoast/reserved-tasks/issues/1106