[BUG] Note review doesn't open next note automatically after a review

[rayanh01]

Describe the bug I the settings I activated "Open next note automatically after a review", but when I review a note, I just see the sr-date, sr-interval, and sr-ease change but the next note doesn't open automatically. I always have to click on the review button at the bottom of the window and choose which tag to review.

To Reproduce

1. Open a note for review.
2. Review it.
3. Notice that the next note doesn't automatically open.

Expected behavior The next note should automatically open.

Screenshots Not applicable.

Versions (please complete the following information):

• OS: MacOS 12.3
• Obsidian version: v0.13.33
• Plugin version: 1.7.2
• If on desktop, Installer version: v0.12.19

Additional context Not applicable.

[axb21]

I have a very similar problem. I noticed it awhile ago but didn't think to check here till just now. Here is my OS/version info:

• OS: Kubuntu 21.04
• Obsidian version: v0.14.7
• Plugin version: 1.7.2
• If on desktop, Installer version: v0.13.30

I've noticed that if I review the same note twice in a row, I am sent to the next note. In other words, if I select "Review: Hard", say, for a note, the various sr-* rows in the front matter are updated, but the note does not change. If I then select "Review: ANYTHING" a second time, the sr-* values change again and I am sent to the next note to review.

EDIT: reviewing twice is not a good workaround because it mucks up the review date of the note as well as the pace of repetitions. The various sr-* values are changed two times, and change depending on whether you select Easy, Good, or Hard.

[CamronWalker]

I am also experiencing this issue. Occasionally it works for the first review item on the list but then stops working for the second again.

[FranSanchezOria]

Same problem here, was this ever fixed?

In my case, after reviewing the note it says response received but it doesn't open the next note. If I review the note again, it does open the next note (but it adds more days to the review). It happens with every note, it needs to send the response twice to work.

[axb21]

I am still having the same problem

• OS: Kubuntu 22.04
• Obsidian version: v0.15.9
• Plugin version: 1.8.0
• If on desktop, Installer version: v0.15.8

In other words since I first posted I've upgraded my OS, Obsidian including the launcher, and the plugin, yet the problem persits for me.

Something I've noticed in the meantime is that the issue affects both vaults I regularly use. I hadn't verified that before.

[karimodm]

I also still have the same problem, what's the issue in fixing it?

[ronzulu]

Hi all, I've tracked down the problem, and hopefully be able to fix in the next couple of days.

In the meantime, there is a workaround.

The first note being reviewed must be done from the review panel (and not from the context menu for the file, etc). I.e. must be done from:

Subsequent notes can be reviewed in any manner of your choosing.

Please let me know if the workaround works, otherwise there might be an additional issue.

Cheers Ronny

[karimodm]

Hey @ronzulu, first of all: thanks for providing feedback on the issue =D.

I was taking the matter into my own hands and started debugging the issue: the problem seems to be that a race-condition existing bewtween the await this.app.vault.modify(note, fileText) (which persists the new frontmatter with the recalculated sr-due based on the review score) and the await this.sync() that follows it.

sync() uses the the Obsidian's cache to obtain the frontmatter of the decks and sort them by their due-date.

           const fileCachedData = this.app.metadataCache.getFileCache(noteFile) || {};

           const frontmatter: FrontMatterCache | Record<string, unknown> =
                fileCachedData.frontmatter || {};

The race condition lies on the fact that such metadataCache might not be updated yet for the modification we just made! We are therefore consuming the frontmatter from before the modification, thus, this note will still result at the top of the deck's stack: we are actually opening the next note in the deck, the problem is that it is just the same one!

To verify that this is the case you can sync and open the next review note only after the cache for the modified file is correctly updated:

        await this.app.vault.modify(note, fileText);

        this.app.metadataCache.on('changed', async (file: TFile, data: string, cache: CachedMetadata) => { 
            if (file.basename == note.basename) { 
                await this.sync();
                if (this.data.settings.autoNextNote) {
                    this.reviewNextNote(this.lastSelectedReviewDeck);
                }
            }
        })

This works, but it is a terrible hack, as we are registering handlers indefinitely! As I've just start looking at the Obsidian API 2 hours ago for the first time, I would need some guidance to design the most appropriate solution!

Is what you also observed? How could it be fixed elegantly?

Cheers!!

[ronzulu]

Hey @karimodm, great detective work, you got deeper into this than I did!

It seems that there are two separate problems, and I found the reproducible simpler one.

lastSelectedReviewDeck not initialised

I noticed that lastSelectedReviewDeck was only being initialised from the sidebar code. So if a person after opening a vault opened a note (not through the sidebar) and reviewed it, lastSelectedReviewDeck would be null.

I've made a fix for this bug, and included lot of console output such as:

race condition

Wow, I haven't heard that phrase for a while!

Definitely seems counter intuitive for await this.app.vault.modify(note, fileText) to return prior to updating its cache (although strangers things have happened in software). Did you find this in the documentation or empirically?

As I haven't personally experienced intermittent part of this bug, are you able to run my version and post the console log here? https://github.com/ronzulu/obsidian-spaced-repetition/tree/bug-424-auto-review-next-note And here is the pre built main.js main_424A.zip

Regarding your initial solution, would you need to setup the metadataCache.on event handler prior to calling await this.app.vault.modify(note, fileText) rather than after? If (the first time around) await this.app.vault.modify happens to return after updating the cache, then the event won't fire.

Curious what happens if after reviewing a note (and thereby setting up the event handler) and the next note is automatically shown. Then instead of reviewing that note, the user updates the frontmatter? Does the event handler still fire and as file.basename == note.basename would be true, the following note would be shown even though the user didn't review it?

potential solution

The current code updates it's internal reviewDecks, dueDatesNotes etc by processing the entire vault.

A cleaner way would be to just update the internal variables for the single note that was just reviewed.

What do you think?

Cheers Ronny

[karimodm]

Hi again @ronzulu

I've just opened a PR that seems to fix the race condition: indeed the best solution seems to be to directly update the due information within the scheduledNotes in the reviewDecks. By doing this I've also removed the need to perform a full sync() on each review :+1:

Definitely seems counter intuitive for await this.app.vault.modify(note, fileText) to return prior to updating its cache (although strangers things have happened in software). Did you find this in the documentation or empirically?

I've verified this empirically: the loaded metadataCache within the sync still contains outdated information. The asynchronous modify does not seem to trigger a synchronous cache rebuild.

Regarding your initial solution, would you need to setup the metadataCache.on event handler prior to calling await this.app.vault.modify(note, fileText) rather than after? If (the first time around) await this.app.vault.modify happens to return after updating the cache, then the event won't fire.

You are indeed right on this point, my bad! The main problem behind this approach still is that we need to deregister the handler within the handling function itself; after digging a bit around the API specification I found this to work:

        const ref = this.app.metadataCache.on('changed', async (file: TFile, data: string, cache: CachedMetadata) => {
            if (file.basename == note.basename) {
                this.app.metadataCache.offref(ref);
                await this.sync();
                if (this.data.settings.autoNextNote) {
                    this.reviewNextNote(this.lastSelectedReviewDeck);
                }
            }
        })

        await this.app.vault.modify(note, fileText);

It essentially deregisters itself after matching the appropriate note's cache update. I still find this solution suboptimal: registering a one-shot handler is quite ugly imo, and I prefer the other approach mentioned.

Please take a look at my PR (https://github.com/st3v3nmw/obsidian-spaced-repetition/pull/821), and let me know what you think!

If you still need the console output from your version I can certainly run it on my end :wink:

[ronzulu]

Hey @karimodm, looking good

Regarding:

I've just opened a PR that seems to fix the race condition: indeed the best solution seems to be to directly update the due information within the scheduledNotes in the reviewDecks. By doing this I've also removed the need to perform a full sync() on each review 👍

Great.

I'm wondering if other variables apart from this.reviewDecks[X].scheduledNotes also needs to be updated. Looking at the code for sync(), it determines the following:

Item	Affected by review
this.reviewDecks[X].scheduledNotes	Updated by your PR :-)
this.reviewDecks[X].dueNotesCount	I think this would always be affected by the review
this.incomingLinks = {}	Not affected by review
graph.reset()	Incoming/outgoing links shouldn't be affected by review
this.pageranks = {}	Generated from graph, so shouldn't be affected
this.easeByPath = new NoteEaseList(this.data.settings)	Ease can change
this.dueNotesCount = 0	I think this would always be affected by the review
this.dueDatesNotes = {}	I think this would always be affected by the review

I wonder if we could refactor sync() into two parts, the second part deriving various variables for all notes. For example I would think that this.reviewDecks[X].dueNotesCount should be derivable from this.reviewDecks[X].scheduledNotes. Similarly for this.dueNotesCount and this.dueDatesNotes.

this.easeByPath should be easy (sorry for the pun) to update directly.

It essentially deregisters itself after matching the appropriate note's cache update. I still find this solution suboptimal: registering a one-shot handler is quite ugly imo, and I prefer the other approach mentioned.

Agreed

Cheers Ronny

[ronzulu]

About

If you still need the console output from your version I can certainly run it on my end 😉

Thanks, but with everything you've progressed, it isn't needed anymore.

Wouldn't mind if you had a chance to cast your eye over the fix I made at: bug-424-auto-review-next-note

In main.ts:

I'm happy for you to include that fix in your branch, as it would simplify admin for @st3v3nmw.

Cheers Ronny

[karimodm]

Hey @ronzulu: I've refactored the sync so that a new method updates and sort the due notes using the pre-populated review decks. This method is the one called by the review after updateding all the necessary properties in place.

There is quite some added complexity there due to the new notes: if you are reviewing one of those, you have to make sure it is removed from the newNotes of the deck and correctly placed in the scheduledNotes.

Given the added complexity I am starting to seriously reconsider the one-shot handler approach: it is way simpler code-wise and it doesn't fragment the business logic, although computationally more intensive, as it always guarantees that all the information the plugin needs is current and correct. Wdyt?

Regarding your change:

if (reviewDeckKeys) does not seem correct to me: Object.keys never returns undefined, you should check if its length is > 0 instead.

[ronzulu]

Hi @karimodm looks good, agree regarding pros/cons. I've run your latest code, as well as visually looking at the changes, and all seems good.

The efficiency benefit might be noticeable to someone with a very large vault, especially if running on an older smartphone. I won't be able to confirm.

Thanks for finding the incorrect condition in my code. I'm not a native TypeScript person, and it looks like when I came across the notion of "truthy" I mistakenly thought it would check that an array had at least 1 element. And thanks for the fix:

if (reviewDeckKeys.length > 0) this.lastSelectedReviewDeck = reviewDeckKeys[0];

BTW I think you also fixed #437 :-)

Cheers Ronny

[karimodm]

Your change looks good now :wink:

Regarding the race fix: should we opt for the one-off handler or the PR as-is? As you have way more experience with the project and API I think it should be your call! Also: what's the process here to get the PR merged? You should approve and then @st3v3nmw merges?

[axb21]

Subsequent notes can be reviewed in any manner of your choosing.

Just a bit of feedback that this workaround does not work for me. Reviewing notes from the review panel works as expected: the metadata is updated and the note switches to the next in queue. However, no matter how many times I review a note from the review panel, if I review the next note using the note's context menu, I observe the same unexpected behavior: the sr-* metadata is updated, but the note does not switch to the next one in the queue.

[ronzulu]

Thanks @axb21, there are two causes of this problem.

I fixed the simpler of the two, and that was included in main_424A.zip a few days ago.

@karimodm fixed the more complicated one at https://github.com/st3v3nmw/obsidian-spaced-repetition/pull/821

I've taken the liberty of combining these two fixes into the following. It would be great if you can test this out and let us know if there is any further issue. main_424B.zip

Cheers Ronny

[ronzulu]

Your change looks good now 😉

Regarding the race fix: should we opt for the one-off handler or the PR as-is? As you have way more experience with the project and API I think it should be your call! Also: what's the process here to get the PR merged? You should approve and then @st3v3nmw merges?

Would be good if we can wait for @axb21 to try out, and then finalise the PR. Your code looks good, so I'll be happy to approve it then.

Minor point, if you can update the #### [Unreleased] in the changelog.md file.

(Note that I created a new issue and PR just for my fix, thought its probably cleaner that way)

Cheers Ronny

[karimodm]

Added to the changelog that #821 fixes #424 and #582 . Let's wait for @axb21 or proceed anyway in a week's time?

[axb21]

@ronzulu @karimodm I'm happy to give this a try. Give me a day or two please.

[axb21]

@ronzulu @karimodm Oops, I overestimated how long it'd take me to try this.

Looks good. I copied my vault and installed the new main.js in the spaced repetition plugin subdirectory. Reviewing a note behaved exactly as I would expect: metadata is updated, the note is switched to the next one, and when the last note up for review is reviewed the appropriate message is displayed. This works when reviewing a note from the context menu of the open note, which is what was not working as expected previously.

So, looks great to me! Thanks for this!

[karimodm]

Awesome news! Let's get the two PRs merged!

[karimodm]

@ronzulu can you please approve the run of the workflow on https://github.com/st3v3nmw/obsidian-spaced-repetition/pull/821 and approve the PR? It would be nice to have our fixes both finally merged and close this long-standing issue!

Cheers! :smile:

[ronzulu]

@karimodm sorry, lost track of this one. I don't think I can approve, so I've requested Stephen to approve/merge on your PR thread. Thanks for the reminder!