Select correct book properly after remote rename (BL-16327)#7915
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR aims to preserve the selected Team Collection book across a reload when the book has been renamed remotely, by resolving the selected book’s ID against the repo before reopening the project.
Changes:
- Adds a TeamCollection helper to resolve a book ID to the local path implied by current repo state.
- Updates workspace reload handling to save that resolved path before reopening.
- Adds a regression test for resolving a remotely renamed repo book.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/BloomExe/TeamCollection/TeamCollection.cs |
Adds GetLikelyLocalPathForBookId() using repo metadata. |
src/BloomExe/Workspace/WorkspaceView.cs |
Updates saved current book path before Team Collection reload. |
src/BloomTests/TeamCollection/TeamCollectionTests.cs |
Adds coverage for resolving a remotely renamed book path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (selectedBook == null || teamCollection == null) | ||
| return; | ||
|
|
||
| var resolvedPath = teamCollection.GetLikelyLocalPathForBookId(selectedBook.ID); |
There was a problem hiding this comment.
Toss up whether it's better to fail fast here. Since we're just trying to keep a book selected, I went ahead and made it safer.
| if (string.IsNullOrEmpty(resolvedPath)) | ||
| return; | ||
|
|
||
| Settings.Default.CurrentBookPath = resolvedPath; |
There was a problem hiding this comment.
Good point. Done.
andrew-polk
left a comment
andrew-polk
left a comment
There was a problem hiding this comment.
@andrew-polk reviewed 3 files and all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on JohnThomson).
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
JohnThomson
left a comment
JohnThomson
left a comment
There was a problem hiding this comment.
@JohnThomson made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on JohnThomson).
| if (selectedBook == null || teamCollection == null) | ||
| return; | ||
|
|
||
| var resolvedPath = teamCollection.GetLikelyLocalPathForBookId(selectedBook.ID); |
There was a problem hiding this comment.
Toss up whether it's better to fail fast here. Since we're just trying to keep a book selected, I went ahead and made it safer.
| if (string.IsNullOrEmpty(resolvedPath)) | ||
| return; | ||
|
|
||
| Settings.Default.CurrentBookPath = resolvedPath; |
There was a problem hiding this comment.
Good point. Done.
| // same book stays selected. Minimally, it makes sure that we are not left with | ||
| // a selected book record pointing at something that doesn't exist, which can cause | ||
| // problems (e.g. BL-16327). | ||
| // It's a bit questionable that this method makes use of the current bookselection, |
| bookId = BookMetaData.FromFolder(folderPath)?.Id; | ||
| return !string.IsNullOrEmpty(bookId); | ||
| } | ||
| catch (Exception e) when (e is IOException || e is UnauthorizedAccessException) |
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
JohnThomson
force-pushed
the
selectAfterRename
branch
from
ebf4dd2 to
cf30d19
Compare
JohnThomson
left a comment
JohnThomson
left a comment
There was a problem hiding this comment.
@JohnThomson made 2 comments and resolved 1 discussion.
Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions (waiting on andrew-polk).
| bookId = BookMetaData.FromFolder(folderPath)?.Id; | ||
| return !string.IsNullOrEmpty(bookId); | ||
| } | ||
| catch (Exception e) when (e is IOException || e is UnauthorizedAccessException) |
| // same book stays selected. Minimally, it makes sure that we are not left with | ||
| // a selected book record pointing at something that doesn't exist, which can cause | ||
| // problems (e.g. BL-16327). | ||
| // It's a bit questionable that this method makes use of the current bookselection, |
andrew-polk
left a comment
andrew-polk
left a comment
There was a problem hiding this comment.
@andrew-polk reviewed 1 file and all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on JohnThomson).
This change is