Skip to content

Desktop: Fix reorder not beeing persisted#4002

Open
timon-schelling wants to merge 1 commit intomasterfrom
desktop-fix-reorder-persistence
Open

Desktop: Fix reorder not beeing persisted#4002
timon-schelling wants to merge 1 commit intomasterfrom
desktop-fix-reorder-persistence

Conversation

@timon-schelling
Copy link
Copy Markdown
Member

No description provided.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request renames set_document_order to force_document_order and updates its implementation to synchronize the document store's order. Additionally, the force_order method was refactored to accept a slice instead of a vector reference and no longer truncates the document list. A review comment suggests reordering operations in force_document_order to avoid an unnecessary unwrap() call by passing the reference before moving the value.

Comment on lines 71 to +72
self.document_order = Some(order);
self.documents.force_order(self.document_order.as_ref().unwrap());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The unwrap() call can be avoided by reordering the operations. Passing a reference to order before moving it into self.document_order is more idiomatic and avoids an unnecessary unwrap() on a value that was just assigned.

Suggested change
self.document_order = Some(order);
self.documents.force_order(self.document_order.as_ref().unwrap());
self.documents.force_order(&order);
self.document_order = Some(order);

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 2 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant