Persist tabs. #4862
Persist tabs. #4862
Conversation
|
Fixes #3373 |
Codecov Report
@@ Coverage Diff @@
## main #4862 +/- ##
==========================================
- Coverage 39.11% 38.09% -1.02%
==========================================
Files 159 167 +8
Lines 12054 12376 +322
==========================================
Hits 4715 4715
- Misses 7339 7661 +322
Continue to review full report at Codecov.
|
| // Note that this means a database is only actually needed after Sync fetches remote tabs, | ||
| // and because sync users are in the minority, the use of a database here is purely | ||
| // optional and created on demand. The implication here is that asking for the "remote tabs" | ||
| // when no database exists is considered a normal situation and just implies no remote tabs exist. |
There was a problem hiding this comment.
So get_remote_tabs will retrieve the remote tabs as of the last sync (if any exist) and the consumer should call sync if they want to get the absolute latest remote tabs, right?
There was a problem hiding this comment.
Yep - which is very similar to how things work now, except that before the first sync we previously returned no tabs whereas now we return what was previously persisted.
There was a problem hiding this comment.
Looks good! Reviewing this PR (particularly with the comments you added) made it easier for me to build a conceptual model of how this sync engine works, specifically how it differs from the other components. I actually like the storage schema as it holds only what's needed, would be easy to extend if the task continuity work calls for it, and adheres to architectural patterns we've already established.
mhammond
force-pushed
the
tabs-persist
branch
from
bc6ea7f to
07894b2
Compare
components/tabs/src/storage.rs
Outdated
| |row| -> Result<_> { Ok(serde_json::from_str(&row.get::<_, String>(0)?)?) }, | ||
| ) { | ||
| Ok(crts) => { | ||
| // only return clients that actually have tabs. |
There was a problem hiding this comment.
Best I can tell, the old code doesn't filter out clients with no open tabs - presumably it relies on the consumer doing this, and there might even be a use-case for knowing the clients exist even if they currently have no tabs? So I'm removing this filtering.
mhammond
force-pushed
the
tabs-persist
branch
from
07894b2 to
14370bb
Compare
|
👍 |
I think this is about ready, but I need to do another self-review - but putting it up for feedback, particularly from @skhamis and @lougeniaC64. Also includes #4859, but I'll rebase once this lands.