Conversation
ba6f6e7 to
627e97c
Compare
JohnThomson
left a comment
There was a problem hiding this comment.
This is hard to review. A lot of code moved and it's not clear whether it changed significantly or just got refactored. Also, a lot of the code feels 'magical', since I know little about service workers. Probably a lot of it was CoPilot. I've gone on the principle that since it works and seems vaguely reasonable, it's probably OK. It's all only going to affect books with this new feature, and we can quickly deploy a fix to Blorg if we need to. I picked up a couple of minor things. If there's anything you think I should particularly pay attention to, please point it out.
@JohnThomson reviewed 3 files and all commit messages, and made 3 comments.
Reviewable status: 3 of 11 files reviewed, 2 unresolved discussions (waiting on nabalone).
src/book-navigation-interceptor-sw.js line 127 at r1 (raw file):
const data = await response.json(); return data.results?.[0];
Between this and the limit: "1" above, it seems you decided to ignore the possibility that there is more than one matching book. I think that's OK...this is a big step forward even without such error handling, and hopefully people who upload linked collections of books know enough to avoid such mistakes, and even if the mistake is made the book it takes you to has some hope of being relevant. Might be worth noting, though, in a comment.
src/model/BookUrlUtils.ts line 46 at r1 (raw file):
filePath: string ): string | undefined { // TODO update comment
Looks like this still needs attention
JohnThomson
left a comment
There was a problem hiding this comment.
@JohnThomson reviewed 8 files.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on nabalone).
This PR addresses the issue BL-15694 by adding support for navigation links on blorg.
This change is