Fixes #12507: Allow renaming a tag to the title of an orphaned tag#14728
Fixes #12507: Allow renaming a tag to the title of an orphaned tag#14728windsnow1025 wants to merge 1 commit intolaurent22:devfrom
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
📝 WalkthroughWalkthroughThe changes modify the Tag model to permit renaming a tag to match an orphaned tag's name by first deleting the unused tag. A test case is added to validate this renaming scenario without throwing an error. Changes
Suggested labels
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 Tip You can get early access to new features in CodeRabbit.Enable the |
|
I have read the CLA Document and I hereby sign the CLA |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/lib/models/Tag.test.ts (1)
226-232: Strengthen this regression test to assert de-duplication explicitly.This currently proves “rename succeeds”, but it can still pass if two tags named
'un'remain. Please also assert that only one'un'tag exists and that it istagDeux.id(withnote2still associated).Suggested assertion extension
const renamedTag = await Tag.load(tagDeux.id); expect(renamedTag.title).toBe('un'); + const tagsNamedUn = await Tag.modelSelectAll('SELECT id FROM tags WHERE title = ?', ['un']); + expect(tagsNamedUn).toHaveLength(1); + expect(tagsNamedUn[0].id).toBe(tagDeux.id); + expect(await Tag.noteIds(tagDeux.id)).toEqual([note2.id]); });As per coding guidelines, "Focus on testing essential behaviour and edge cases — avoid adding tests for every minor detail".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lib/models/Tag.test.ts` around lines 226 - 232, Add explicit de-duplication assertions after the rename: load the tag by title (const unTag = await Tag.loadByTitle('un')) and assert it exists and its id equals tagDeux.id; assert there is only one tag named 'un' (e.g. await Tag.findAll() then filter by t.title==='un' and expect length === 1); and assert note2 still refers to that tag (e.g. const reloadedNote2 = await Note.load(note2.id) and expect(reloadedNote2.tagIds/includes(tagDeux.id))).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/lib/models/Tag.ts`:
- Around line 237-240: The orphan-resolution logic around
Tag.noteIds(existingTag.id) and Tag.untagAll(existingTag.id) must be made atomic
to avoid races: wrap the conflict check, potential untag/delete and the
subsequent rename into a single DB transaction (or obtain a row-level lock for
the existingTag) so you re-check the current note count under that same
transaction/lock before calling Tag.untagAll and performing the rename; if your
ORM supports transactions use Tag.transaction(...) (or the equivalent) and
re-run Tag.noteIds(existingTag.id) or a SELECT ... FOR UPDATE inside it, then
proceed with Tag.untagAll(existingTag.id) and rename only when the re-checked
count is zero.
---
Nitpick comments:
In `@packages/lib/models/Tag.test.ts`:
- Around line 226-232: Add explicit de-duplication assertions after the rename:
load the tag by title (const unTag = await Tag.loadByTitle('un')) and assert it
exists and its id equals tagDeux.id; assert there is only one tag named 'un'
(e.g. await Tag.findAll() then filter by t.title==='un' and expect length ===
1); and assert note2 still refers to that tag (e.g. const reloadedNote2 = await
Note.load(note2.id) and expect(reloadedNote2.tagIds/includes(tagDeux.id))).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 455ab493-4bc3-4910-b92c-70c9ff980cae
📒 Files selected for processing (2)
packages/lib/models/Tag.test.tspackages/lib/models/Tag.ts
| const noteIds = await Tag.noteIds(existingTag.id); | ||
| if (noteIds.length === 0) { | ||
| await Tag.untagAll(existingTag.id); | ||
| } else { |
There was a problem hiding this comment.
Make orphan resolution atomic to avoid data loss.
The orphan check and deletion are split across separate async steps. A concurrent note-tag association in between can delete a tag that has become in-use. Please execute conflict check + orphan deletion + rename in a single transaction (or re-check under the same lock) before deleting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/lib/models/Tag.ts` around lines 237 - 240, The orphan-resolution
logic around Tag.noteIds(existingTag.id) and Tag.untagAll(existingTag.id) must
be made atomic to avoid races: wrap the conflict check, potential untag/delete
and the subsequent rename into a single DB transaction (or obtain a row-level
lock for the existingTag) so you re-check the current note count under that same
transaction/lock before calling Tag.untagAll and performing the rename; if your
ORM supports transactions use Tag.transaction(...) (or the equivalent) and
re-run Tag.noteIds(existingTag.id) or a SELECT ... FOR UPDATE inside it, then
proceed with Tag.untagAll(existingTag.id) and rename only when the re-checked
count is zero.
There's an important point which was made on one of the comments on my closed PR: #13628 (comment) It's no good checking if a tag has been orphaned locally, because you can't guarantee that it's not still in use by another client which is not in sync. So your approach still has the same fundamental problem that the original change on my PR did |
|
Please confirm you'd like to complete this PR |
I think I don't have a clean solution for the sync problem at this point. |
3 participants
When a note with an exclusive tag is permanently deleted, the tag record remains in the database but becomes invisible in the UI (no active notes reference it). Attempting to rename another tag to this orphaned tag's name fails with "The tag already exists", even though the user cannot see or interact with the orphaned tag.
This fix checks whether the conflicting tag is an orphan (has no associated active notes) during rename validation. If so, it removes the orphaned tag and its stale note_tags records, allowing the rename to proceed. If the conflicting tag is still in use, the existing error is thrown as before.
Previous PRs (#13628, #14616) were closed due to concerns about sync complexity. #13628 cleaned up note_tags during permanent deletion; #14616 both cleaned up orphaned tags during deletion and handled rename conflicts. This PR takes a narrower approach — it only handles the rename conflict case without modifying the deletion flow, minimizing the impact on sync behavior.
A test case has been added to verify that renaming to an orphaned tag name succeeds.