Fix/25020 tag disappears dropdown#25262
Conversation
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on November 3. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
WalkthroughChanges to the tag input component modify how tags are managed and deduplicated in the dropdown. The constructor no longer pre-adds selected tags to initial tags. Available tags are now filtered using ID-based comparison instead of object reference matching. The Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ghost/admin/app/components/gh-tags-token-input.js (2)
48-51: Consider ID-based deduplication for consistency.While object reference equality likely works with Ember Data's identity map, using ID-based deduplication would be more consistent with the
availableTagsfix and more robust against edge cases.Apply this diff for ID-based deduplication:
- // Store all tags, but avoid duplicates - const existingTags = new Set(this._initialTags); - const newTags = tags.filter(tag => !existingTags.has(tag)); - this._initialTags.push(...newTags); + // Store all tags, but avoid duplicates + const existingTagIds = new Set(this._initialTags.map(tag => tag.id)); + const newTags = tags.filter(tag => !existingTagIds.has(tag.id)); + this._initialTags.push(...newTags);
56-59: Consider ID-based deduplication for consistency.Same recommendation as
addInitialTags- ID-based deduplication would be more consistent with theavailableTagsfix.Apply this diff for ID-based deduplication:
- // Store all search results, but avoid duplicates - const existingTags = new Set(this._searchedTags); - const newTags = tags.filter(tag => !existingTags.has(tag)); - this._searchedTags.push(...newTags); + // Store all search results, but avoid duplicates + const existingTagIds = new Set(this._searchedTags.map(tag => tag.id)); + const newTags = tags.filter(tag => !existingTagIds.has(tag.id)); + this._searchedTags.push(...newTags);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ghost/admin/app/components/gh-tags-token-input.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ghost/admin/app/components/gh-tags-token-input.js (1)
ghost/admin/tests/integration/components/gh-psm-tags-input-test.js (3)
tags(17-17)tags(223-223)tags(246-246)
🔇 Additional comments (2)
ghost/admin/app/components/gh-tags-token-input.js (2)
27-28: Good clarification of the new tag initialization behavior.The comment clearly explains that selected tags are no longer pre-added to
_initialTags, allowing the ID-based filtering inavailableTagsto work correctly when tags are removed and should reappear in the dropdown.
33-34: Verify selectedTags management after tag save—ID-based filtering may fail for newly created tags.The fix correctly resolves the reference equality bug by using primitive IDs. However, Ember Data replaces temporary IDs with server-provided IDs when saving newly created records. If newly created tags are added to
selectedTagswith temporary IDs and then saved, theselectedTagIdsSet will contain outdated temporary IDs while the tag objects themselves receive server IDs. This causes the filter to incorrectly include recently-saved tags inavailableTags.Action required:
- Verify how the parent component manages
selectedTagsafter saving the post/page- Confirm whether
selectedTagsis refreshed with server-loaded tags (correct) or maintains references to pre-save objects (buggy)- If the latter, consider using tag names or persisting client-generated IDs instead of relying on Ember Data's auto-assigned IDs
fixes TryGhost#25020 Tag selector was excluding deleted tags permanently by checking against selectedTags Set without filtering out removed tags first. This prevented users from re-selecting tags they had previously removed from a post.
Optimized deduplication logic to use Set.has() instead of Array.includes() across all methods for consistent O(1) performance. Fixed root cause where constructor was incorrectly filtering selected tags from initial tag pool, preventing re-selection after removal. Now stores all available tags and filters only in availableTags getter as intended.
fixes TryGhost#25020 When editing a post, removing a tag while the dropdown was open would cause the tag to disappear from available options. This happened because the Set was comparing object references instead of tag IDs. Tags loaded at different times create different object instances even for the same logical tag, causing Set.has() to fail. Fixed by comparing tag IDs which remain constant regardless of when/how the tag object was created.
ref TryGhost#25020 - restored filtering of already-selected tags from server-side search results, now comparing by tag id instead of object reference - restored seeding of the initial tags list with the selected tags so a tag from beyond the first loaded page reappears in the dropdown after removal; seeding now uses the TrackedArray constructor to avoid a render-time mutation deprecation - removed code comments referencing the issue - added integration tests covering tag re-appearance after removal and selected-tag exclusion from server-side search results
2690e99 to
d9d75b9
Compare
There was a problem hiding this comment.
Pull request overview
Fixes a post-editor tag dropdown bug where removing a selected tag while the dropdown is open could cause the tag to disappear from the available options until the editor is reopened. The fix updates tag filtering/deduplication logic to compare by tag IDs instead of object reference equality.
Changes:
- Initialize
_initialTagswith the currently selected tags and filter available options bytag.idrather than object identity. - Deduplicate loaded/queried tags using tag IDs for both initial-load and server-side search results.
- Add integration coverage for (a) removed tags reappearing in the dropdown and (b) excluding selected tags from server-side search results.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
ghost/admin/app/components/gh-tags-token-input.js |
Switches selection filtering/deduplication from reference equality to ID equality to keep removed tags available immediately. |
ghost/admin/tests/integration/components/gh-psm-tags-input-test.js |
Adds integration tests for the regression scenario and for server-side search excluding selected tags. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
9larsons
left a comment
There was a problem hiding this comment.
Nice find and change! LGTM.
Description
Brief summary of changes:
Fixed tag disappearing from the dropdown after removal in the post editor. When a tag was removed while the tags dropdown was open, it would disappear from the available options until the editor was closed and reopened. This is now fixed by comparing tag IDs instead of object references.
Related issue(s):
Fixes #25020
Type of change:
Testing
How has this been tested?
Test environment:
Manual Testing Steps:
Screenshots (if applicable)
Before:
Tag would disappear from the
Issue_After_Fix_25020.mp4
dropdown after removal and not reappear until editor was closed/reopened.
After:
Tag immediately reappears in dropdown after removal, maintaining consistency with expected behavior.
Checklist
Code Quality:
Testing:
Documentation:
Ghost-Specific:
Additional Notes
Root Cause Analysis:
The
availableTagsgetter ingh-tags-token-input.jswas using a JavaScriptSetto filter out selected tags by storing tag objects directly. However, JavaScript Sets use reference equality for objects, not value equality. When a tag is loaded from the initial tags list versus when it's selected/removed from a post, these are different object instances even though they represent the same logical tag. This causedSet.has(tag)to returnfalsefor removed tags, incorrectly excluding them from the available options.Solution:
Changed the Set to store tag IDs (strings/numbers) instead of tag objects. IDs are primitive values that use value equality, ensuring consistent filtering regardless of when or how the tag object was created.
Code change: