Removed autosaving of tags from gh-post-settings-menu #22112
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
ghost/admin/app/components/gh-post-settings-menu.jsOops! Something went wrong! :( ESLint: 8.44.0 Error: Failed to load parser '@babel/eslint-parser' declared in 'ghost/admin/.eslintrc.js': Cannot find module '@babel/eslint-parser'
ghost/admin/app/controllers/lexical-editor.jsOops! Something went wrong! :( ESLint: 8.44.0 Error: Failed to load parser '@babel/eslint-parser' declared in 'ghost/admin/.eslintrc.js': Cannot find module '@babel/eslint-parser'
WalkthroughThis pull request revises the tag saving functionality in the post settings menu. The tags input action in the component template is updated from calling Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant P as GhPostSettingsMenu
participant L as LexicalEditorController
participant A as _autosaveTask
U->>P: Modify tags input
P->>P: Trigger saveTags action
P->>L: Call saveTagsTask.perform()
L->>A: Yield to _autosaveTask for saving tags
A-->>L: Return save result
alt Error occurs
L->>P: Invoke showError() and rollback changes
end
Possibly related PRs
Suggested labels
Tip 🌐 Web search-backed reviews and chat
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
🔇 Additional comments (1)
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
🧹 Nitpick comments (2)
ghost/admin/app/controllers/lexical-editor.js (2)
464-468: LGTM! Consider adding validation and error handling.The implementation follows the established pattern for model updates and correctly triggers autosave. However, consider adding validation and error handling for robustness.
Apply this diff to add validation and error handling:
@action setTags(tags) { + if (!Array.isArray(tags)) { + this.notifications.showAlert('Invalid tags format', {type: 'error'}); + return; + } + + try { this.post.set('tags', tags); this._autosaveTask.perform(); + } catch (error) { + this.notifications.showAlert('Failed to update tags', {type: 'error'}); + throw error; + } }
1298-1311: Integration with autosave system is well-implemented.The
setTagsmethod correctly utilizes the existing autosave mechanism, which provides:
- 3-second debounce for optimal performance
- Special handling for new posts
- Draft-only autosave protection
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ghost/admin/app/components/gh-post-settings-menu.hbs(1 hunks)ghost/admin/app/components/gh-post-settings-menu.js(1 hunks)ghost/admin/app/components/gh-psm-tags-input.js(4 hunks)ghost/admin/app/controllers/lexical-editor.js(1 hunks)ghost/admin/app/templates/lexical-editor.hbs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Lint
- GitHub Check: Admin tests - Chrome
🔇 Additional comments (7)
ghost/admin/app/components/gh-psm-tags-input.js (4)
16-16: LGTM: New state management for tags.The addition of
currentTagsprovides proper internal state management for the component.
39-39: LGTM: Proper initialization of internal state.The
currentTagsis correctly initialized from the post's tags in theinitmethod.
53-66: LGTM: Clean tag update handling.The
updateTagsmethod properly:
- Cleans up unsaved tags that are no longer selected
- Updates internal state
- Delegates to parent through
handleTagsUpdate
71-71: LGTM: Consistent state management in tag creation.The
createTagmethod correctly usescurrentTagsfor managing tag state.Also applies to: 96-96
ghost/admin/app/templates/lexical-editor.hbs (1)
140-140: LGTM: Proper wiring of tag updates.The
@setTagsproperty correctly wires the tag update handler from the editor to the post settings menu.ghost/admin/app/components/gh-post-settings-menu.js (1)
624-627: LGTM: Clean tag update delegation.The
updateTagsaction properly delegates tag updates to the parent'ssetTagsmethod.ghost/admin/app/components/gh-post-settings-menu.hbs (1)
73-73: LGTM: Improved tag update handling.Replacing
@savePostOnChangewith@handleTagsUpdateproperly implements the new tag update flow, removing automatic post saving on tag changes.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
ghost/admin/.lint-todo (1)
1-206:🧩 Analysis chain
Standardize event handling patterns.
The addition of
no-actionandno-passed-in-event-handlersrules across multiple components suggests a move towards more consistent event handling patterns. Consider addressing these linting issues to improve code maintainability.Run the following script to identify components still using deprecated event handling patterns:
🏁 Script executed:
#!/bin/bash # Description: Find components using deprecated event handling patterns. # Search for components using the action modifier rg -A 2 'action=' app/components/Length of output: 89
It looks like the initial script’s target directory (“app/components/”) doesn’t exist in the repository, so we didn’t get the expected output. To get a clearer picture of any deprecated event handling patterns (e.g. use of Ember’s action helper) across the project, let’s expand our search to all Handlebars files. We’ll search for the
{{actionhelper usage as a proxy for deprecated event handling patterns.Please run the following script:
If this script produces output, we can review the matches to determine if further refactoring is needed. If no output is found, it may suggest that there are no lingering deprecated patterns using the
{{actionhelper in Handlebars templates.Please share the result when available.
🏁 Script executed:
#!/bin/bash # Description: Search for Ember action helper usage in all Handlebars (.hbs) files. # This should help us verify whether any deprecated event handling patterns are present. rg -n '{{action' --glob '*.hbs'Length of output: 239
Let's try re-running the search command using a fixed-string search to avoid regex interpretation issues. Please run the following script:
This should properly search for any usage of the
{{actionhelper. Please share the output once available.
🏁 Script executed:
#!/bin/bash # Description: Search for Ember's action helper usage in all Handlebars (.hbs) files using a fixed-string match. rg -F -n '{{action' --glob '*.hbs'Length of output: 19622
Standardize event handling patterns in Ember templates
The search confirms that many Handlebars files still use the legacy
{{action …}}helper. Notable instances include:
- Setup Template: Multiple deprecated action usages in
ghost/admin/app/templates/setup.hbs- Post Settings: Several older action patterns in
ghost/admin/app/components/gh-post-settings-menu.hbs- Tags and Modal Components: Similar legacy patterns are also present in files like
ghost/admin/app/templates/tags.hbsand various modal componentsThese findings align with the lint-todo entries related to
no-actionandno-passed-in-event-handlers. Refactoring these patterns (for example, by adopting closure actions or a more standardized event handling approach) would improve consistency and code maintainability.
🧹 Nitpick comments (3)
ghost/admin/tests/integration/components/gh-psm-tags-input-test.js (2)
138-138: Remove debug console.log statement.Debug logging should not be committed to the codebase.
- console.log(calledWith);
126-138: Enhance test coverage for tag updates.While the test verifies that the handler is called, it should also:
- Verify the actual tags passed to the handler
- Test the case when all tags are removed
Consider adding these assertions:
expect(called).to.equal(true); - console.log(calledWith); + expect(calledWith.map(tag => tag.name)).to.deep.equal(['#Tag 2', 'Tag 3', 'Tag 1']); + + // Test removing all tags + let removeBtns = findAll('.ember-power-select-multiple-remove-btn'); + for (let btn of removeBtns) { + await click(btn); + } + expect(calledWith).to.deep.equal([]);ghost/admin/.lint-todo (1)
1-206: Consider addressing accessibility improvements.The linting configuration introduces several accessibility-focused rules (
require-input-label,require-valid-alt-text,require-iframe-title). While these are currently marked as TODO, implementing these improvements would enhance the application's accessibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ghost/admin/.lint-todo(1 hunks)ghost/admin/tests/integration/components/gh-psm-tags-input-test.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Unit tests (Node 18.12.1)
- GitHub Check: Lint
- GitHub Check: Admin tests - Chrome
🔇 Additional comments (1)
ghost/admin/.lint-todo (1)
207-209: Changes align with PR objectives.The linting rule changes for
gh-post-settings-menu.hbsreflect the modifications to tag management, ensuring proper event handling patterns are followed.
allouis
force-pushed
the
onc-756-reduce-tags-api-calls
branch
from
64f5b5f to
827342e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ghost/admin/tests/integration/components/gh-psm-tags-input-test.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Admin tests - Chrome
🔇 Additional comments (1)
ghost/admin/tests/integration/components/gh-psm-tags-input-test.js (1)
126-126: LGTM! Clear and descriptive test title.The updated test title accurately reflects the component's responsibility and aligns with the new tag update handling approach.
ghost/admin/tests/integration/components/gh-psm-tags-input-test.js
Outdated
Show resolved
Hide resolved
allouis
force-pushed
the
onc-756-reduce-tags-api-calls
branch
from
827342e to
f043bc8
Compare
|
I'm interested to know how to test this better, I went to add tests for the post-settings-menu component, but they are unit tests and completely commented out, which a note to migrate them to integration tests - shall I add an integration test for this? I'm pretty confident I've use the right one, but I'm a little bit curious about the difference between the |
Background saves are "quiet", in that they won't trigger post-saved popup notifications or error alerts, they also don't allow the post status to be changed as background saves should never publish a post. I expect it would be good to rename |
ref https://linear.app/ghost/issue/ONC-756 ref d91869e Rather than forcing a save on every tag change, we want to defer to the autosave functionality which will debounce saves, reducing the number of API calls when modifying tags on a post. This does mean that changing tags for existing posts will require an explicit save, as the autosave functionality does not run for published posts.
allouis
force-pushed
the
onc-756-reduce-tags-api-calls
branch
from
f043bc8 to
be2ffe2
Compare
ref https://linear.app/ghost/issue/ONC-756
ref d91869e
Rather than forcing a save on every tag change, we want to defer to the autosave functionality which will debounce saves, reducing the number of API calls when modifying tags on a post. This does mean that changing tags for existing posts will require an explicit save, as the autosave functionality does not run for published posts.