Skip to content

Conversation

@kadisonm
Copy link

To track changes to the sync history feature #43. This is in progress still.

Apologies for the duplicate pull request, I accidentally messed the other one up. (I'm still fairly new to Github)

@kadisonm kadisonm marked this pull request as draft December 30, 2023 15:27
return;
}

// If the metadata is synced first then we can't state if a sync was successful or not. Unless it is uploaded again after?
Copy link
Owner

Choose a reason for hiding this comment

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

We probably need to upload metadata after a sync, I think this makes sense to me.

Some failed syncs may result in metadata not being uploaded, which is acceptable (not all failures can be sent remotely, like if remote isn't responding or we don't have internet connection).

If a sync fails and we can't send that to the remote can we still show it as a "failed sync" in the local sync history?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for reviewing. I think so as well.

I think having a history array and fetching metadata on plugin load could work to store local sync failures. But any failed syncs added locally wouldn't show after plugin reload. Regardless, I do need some way to access the history in historyView.ts so storing a public array in main.ts might help.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah can store this in the plugin settings for now which is persisted. I think this is worth doing - there is also a local DB which I haven't explored much, could be a better place for persistence.

undefined,
undefined,
[],
false,
Copy link
Owner

Choose a reason for hiding this comment

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

Wonder if this should be true; having first sync be failed doesn't make sense to me, might add confusion after people reset metadata. Definitely a nitpick though and we can see the experience later & decide

Copy link
Author

Choose a reason for hiding this comment

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

As in making the metadata deletion add to the sync history?

Copy link
Owner

Choose a reason for hiding this comment

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

Ah sorry, I see now the parameter is the deletion history - does false make sense as a value here vs undefined? definitely a nitpick, I was thinking it would log the sync as failed but I didn't finish tracing the code.

Copy link
Author

@kadisonm kadisonm Dec 31, 2023

Choose a reason for hiding this comment

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

All good, I'll make it undefined as I just rushed it a bit to see if I could get it working.

I'll also need to add something for the fails.

:)

@sboesen
Copy link
Owner

sboesen commented Dec 30, 2023

Thanks for the PR and no problem! Added a couple comments, will keep an eye out for updates :)

@kadisonm
Copy link
Author

Thanks by the way!

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.

2 participants