Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] StoryList is shared among clients as of 5.1.22 #4868

Closed
hoelzro opened this issue Sep 29, 2020 · 32 comments
Closed

[BUG] StoryList is shared among clients as of 5.1.22 #4868

hoelzro opened this issue Sep 29, 2020 · 32 comments

Comments

@hoelzro
Copy link
Contributor

hoelzro commented Sep 29, 2020

Describe the bug
If you load a TiddlyWiki served via Node, the story list is shared among all clients.

To Reproduce
Steps to reproduce the behavior:

  1. Set up a TiddlyWiki served via Node (5.1.22)
  2. Load the wiki in two browser windows (let's call them A and B)
  3. Open a tiddler or two in A
  4. Observe how the tiddlers are then opened in B after a sync interval
  5. Close a tiddler in B
  6. Observe how that tiddler is then closed in A after a sync interval

Expected behavior
Each client should have its own story list.

Desktop (please complete the following information):

  • OS: Desktop Linux (Arch)
  • Browser: Firefox 81
  • Version [e.g. 22]

Additional context

I think that I've seen other system tiddlers, such as the sidebar tab state, synced as well - that's weird to me since $:/state/ tiddlers should be excluded via $:/config/SyncFilter, so I'll need to try to reproduce this.

Other reports

@pmario
Copy link
Member

pmario commented Sep 29, 2020

There is a setting in the ControlPanel for $:/DefaultTiddlers which allows to store / reopen the story: [list[$:/StoryList]]. For me this is one of the most important settings. So it should not go away.

If it has to go away, we need a replacement, that also works in browser "private" mode!

@pmario
Copy link
Member

pmario commented Sep 29, 2020

I think, this wouldn't be a problem, if the TW server would be a TiddlyWeb server. TiddlyWeb knows about "recipes" and "bags".

IMO the problems which come up now with the whole multiuser-crux have all been thought through already. ...We have just forgotten them.

More info see: http://tiddlyweb.com/ ... I'm not saying, that it should be a python implementation. I'm saying we should have a closer look about the "battle tested" workflow and configurations.

@pmario
Copy link
Member

pmario commented Sep 29, 2020

@hoelzro
Copy link
Contributor Author

hoelzro commented Sep 30, 2020

@pmario I'm not familiar with TiddlyWeb - is it an implementation of the API that the tiddlyweb sync adaptor uses, similar to the one the NodeJS server provides? If so, does TiddlyWeb exclude system tiddlers in the results for the /recipes/default/tiddlers.json route?

@pmario
Copy link
Member

pmario commented Sep 30, 2020

If so, does TiddlyWeb exclude system tiddlers in the results for the /recipes/default/tiddlers.json route?

At the moment we only have recipes/default/ that would need to be changed. So a multi user setting has several bags, where users have write access. The $:/StoryList tiddler would need to be written to the bag, that belongs to the user.

@pmario
Copy link
Member

pmario commented Sep 30, 2020

If so, does TiddlyWeb exclude system tiddlers in the results for the /recipes/default/tiddlers.json route?

At the moment we only have recipes/default/ that would need to be changed. So a multi user setting has several bags, where users have write access. The $:/StoryList tiddler would need to be written to the bag, that belongs to the user.

A tiddlyweb recipe usually has several lines of "bags" with filters. The bags are read from top to bottom. ... If tiddler names are duplicated, the "bottom one wins"

Writing tiddlers uses the first bag, where the user has write access. .. IMO this could be different. It should start from the bottom. .. Because that's usually the bag the user has write access.

filters can look like this.... But it could also be implemented as filter settings as done now.

@pmario
Copy link
Member

pmario commented Sep 30, 2020

For me the existing server is still a development server. For real usage it would need to be implemented more like TiddlyWeb, with improvements learned.

@hoelzro
Copy link
Contributor Author

hoelzro commented Sep 30, 2020

@pmario Ok, thanks for explaining!

For me the existing server is still a development server

By "existing server", you mean the server as implemented in NodeJS, right?

I should clarify that I'm not exclusively talking about a multi-user situation - I'm the only user of my wiki, but this is still an issue for me if I have multiple tabs open to that wiki.

@saqimtiaz
Copy link
Member

I should clarify that I'm not exclusively talking about a multi-user situation - I'm the only user of my wiki, but this is still an issue for me if I have multiple tabs open to that wiki.

I have run into the same issue even when just testing in different browsers.

It is worth remembering that is due to a change in 5.1.22 by which system tiddlers are synchronized, and this is a change that unintentionally breaks backwards compatibility. Therefore I think we need to firstly prioritize addressing this bug by restoring the previous behaviour from 5.1.21, and separately work out a longer term and better solution.

There have been two more reports of this issue on Discord in the last few weeks. What is worth noting is that using the syncFilter to prevent the StoryList and HistoryList from being synced are not sufficient to address the issues. Reverting to TW 5.1.21 was the only thing that resolved the problems.

@hoelzro
Copy link
Contributor Author

hoelzro commented Sep 30, 2020

Yeah, it's worth mentioning that it's only present in 5.1.22 - my fix for the time being has been the following patch on top of two shadow tiddlers:

diff --git a/tiddlers/_system/core/modules/syncer.js.js b/tiddlers/_system/core/modules/syncer.js.js
index 903bb0a4..0f7ac524 100644
--- a/tiddlers/_system/core/modules/syncer.js.js
+++ b/tiddlers/_system/core/modules/syncer.js.js
@@ -310,6 +310,7 @@ Syncer.prototype.syncFromServer = function() {
                                        self.titlesToBeLoaded[title] = true;
                                });
                                $tw.utils.each(updates.deletions,function(title) {
+                                        if(self.wiki.isSystemTiddler(title)) return;
                                        delete self.tiddlerInfo[title];
                                        self.logger.log("Deleting tiddler missing from server:",title);
                                        self.wiki.deleteTiddler(title);
@@ -355,6 +356,7 @@ Syncer.prototype.syncFromServer = function() {
                        }
                        // Delete any tiddlers that were previously reported but missing this time
                        $tw.utils.each(previousTitles,function(title) {
+                                if(self.wiki.isSystemTiddler(title)) return;
                                delete self.tiddlerInfo[title];
                                self.logger.log("Deleting tiddler missing from server:",title);
                                self.wiki.deleteTiddler(title);
diff --git a/tiddlers/_system/plugins/tiddlywiki/tiddlyweb/tiddlywebadaptor.js.js b/tiddlers/_system/plugins/tiddlywiki/tiddlyweb/tiddlywebadaptor.js.js
index 5bc4e502..9bdcb6c5 100644
--- a/tiddlers/_system/plugins/tiddlywiki/tiddlyweb/tiddlywebadaptor.js.js
+++ b/tiddlers/_system/plugins/tiddlywiki/tiddlyweb/tiddlywebadaptor.js.js
@@ -159,7 +159,7 @@ TiddlyWebAdaptor.prototype.getSkinnyTiddlers = function(callback) {
        $tw.utils.httpRequest({
                url: this.host + "recipes/" + this.recipe + "/tiddlers.json",
                data: {
-                       filter: "[all[tiddlers]] -[[$:/isEncrypted]] -[prefix[$:/temp/]] -[prefix[$:/status/]]"
+                       filter: "[all[tiddlers]] -[[$:/isEncrypted]] -[prefix[$:/temp/]] -[prefix[$:/status/]] -[is[system]]"
                },
                callback: function(err,data) {
                        // Check for errors

The tiddlywebadaptor patch is to make sure changes to system tiddlers like $:/StoryList aren't picked up from the server, and the syncer patch makes sure that system tiddlers aren't deleted as a result of not being in that server sync.

@hoelzro
Copy link
Contributor Author

hoelzro commented Oct 5, 2020

The crux of the issue seems to be that the story list (among other system tiddlers, as @saqimtiaz pointed out) is synced from the server to the client, which is what my patch above addresses. I think this raises a few questions:

  • Which system tiddlers do make sense to sync from the server to the client? I'm guessing things like $:/config/ would qualify here.
  • Which system tiddlers don't? Obviously $:/StoryList, but I'm thinking $:/state/ might also not make sense.
  • Does it ever make sense to sync a system tiddler from the client to the server, but not the server to the client? On the pro side, this is what the old syncer code did; on the con side, you will get the changed system tiddlers when/if you refresh your wiki, and it feels a little off to me that a wiki's tiddler store can diverge from what it would be if one refreshed the wiki.
  • What would be the best way to indicate whether or not a system tiddler should be synced? $:/config/SyncFilter and the tiddlywebadaptor plugin's filter form a sort of blocklist, but I'm wondering if there should be a core field that says "don't sync me".

@pmario
Copy link
Member

pmario commented Oct 5, 2020

  • Which system tiddlers do make sense to sync from the server to the client? I'm guessing things like $:/config/ would qualify here.

All of them. If they are stored, it needs to be possible to load them. Otherwise you can disable storing them. .. IMO it's a per user setting.

  • Which system tiddlers don't? Obviously $:/StoryList, but I'm thinking $:/state/ might also not make sense.

That's not true. From my point of view, I want to have full control of the wiki state. ... The current import mechanism is kind of broken for me, since some tiddlers are physically removed from the import list. Even enabling them again doesn't import it.

The same is true for me in a server based setting. ... The server needs to have an idea of different users or bags or recipes. .. That will solve the problem!

  • Does it ever make sense to sync a system tiddler from the client to the server

Yes. The $:/DefaultTiddlers have a setting like [list[$:/StoryList]] which is probably the most important setting for me. I do want to see the tiddlers from the last session if I open a wiki.

The same is true for state tiddlers. I don't want to select a tab in the ControPanel over and over again. I want the state to be stored and retrieved.

@hoelzro
Copy link
Contributor Author

hoelzro commented Oct 6, 2020

I kind of understand what you're saying - but I'm not super familiar with bags and recipes, so I don't know how they'd work for my workflow. For example, I often have several different tabs/windows (which I'll just call "clients" from now on) open to my wiki:

  • One on my desktop where I'm working on notes for blog post
  • Another on my desktop where I'm looking at notes for a coding project
  • One on my personal laptop where I'm going over journal entries that I need to flesh out.
  • One on my work laptop where I'm consulting notes on what to talk to a colleague about next time we're both online

As things currently stand, since system tiddlers are used so much to drive the UI within TiddlyWiki, if I'm syncing system tiddlers among these, clicking on a sidebar panel tab on one will change the others, opening a tiddler on one will change the story list on the others, etc.

How would my workflow be supported using recipes/bags? Would I need to create a separate recipe per client as I've described them above?

@pmario
Copy link
Member

pmario commented Oct 6, 2020

As things currently stand, since system tiddlers are used so much to drive the UI within TiddlyWiki, if I'm syncing system tiddlers among these, clicking on a sidebar panel tab on one will change the others, opening a tiddler on one will change the story list on the others, etc.

What you describe as a problem. ... Is a problem .. for you. ...

I would love to be able to set up a server with a global address. Call someone on skype and tell them, they should open the address. Whatever I click on my wiki will be synced to the server and synced to the other persons browser.

They can add some comments, that may be "live synced" back to my computer, if they have write access to a bag, that I can see.

@pmario
Copy link
Member

pmario commented Oct 6, 2020

I think the problem here is 2 fold.

  1. As Saq wrote. We need to fix the problem that was introduced with V5.1.22, because it is breaking the experience.
  2. We need to solve the problem with syncing system tiddlers.
  1. can be done fast
  2. will need more time.

@pmario
Copy link
Member

pmario commented Oct 6, 2020

I'll try to create an overview of the TWeb structure.

recipes ... are configurations that describe, who the server should build the content of a wiki.
recipes ... contain a configuration that looks similar to:

    /bags/system/tiddlers ... can contain "system" plugins ... This should always be there, but may be empty.
    /bags/{{ user }}/tiddlers ... contains the {{ user }}'s content tiddlers.

A recipe as a json configuration, that could be part of tiddlywiki.info or some other server related configuration. eg:

rob.recipe.json

{
    "recipe": [
        ["bag-mario", "[!is[system]]"],
        ["bag-rob", ""]
    ],
    "policy": {"read": [], "write": [], "create": [], "delete": [], "manage": [], "owner": ''},
    "desc": "optional description of the bag (may be an empty string or absent)",
}

mario.recipe.json

{
    "recipe": [
        ["bag-mario", ""],
        ["bag-rob", "[!is[system]]"]
    ],
    "policy": {"read": [], "write": [], "create": [], "delete": [], "manage": [], "owner": ''},
    "desc": "optional description of the bag (may be an empty string or absent)",
}

So if the server has the info of the user name, it will show us 2 different wikis. The first one will be shown to user:rob and the bag-mario will not contain any of the system tiddlers in mario's bag.

The second view is will be shown to user:mario without the system tiddlers from "bag-rob". ... AND if tiddlers with the same name exist in both bags rob's tiddlers will win.

To change this user mario will need to be able to change the recipe to:

mario.recipe.json

{
    "recipe": [
        ["bag-rob", "[!is[system]]"],
        ["bag-mario", ""]
    ],
<snip>
}

So now tiddlers show to mario in mario's bag will win.

That's only part of the story, but it should make it clearer how it can work.

@saqimtiaz
Copy link
Member

saqimtiaz commented Oct 6, 2020

I think the problem here is 2 fold.

  1. As Saq wrote. We need to fix the problem that was introduced with V5.1.22, because it is breaking the experience.
  2. We need to solve the problem with syncing system tiddlers.
  1. can be done fast
  2. will need more time.

I suggest we use this issue to discuss a solution for 1), i.e. the bug introduced in 5.1.22 and start another issue to discuss a better long term approach for syncing system tiddlers.

The immediate priority should be to restore the user experience from 5.1.21

@pmario
Copy link
Member

pmario commented Oct 6, 2020

So recipes contain the info for the server how to "build" a wiki

bags contain the real info.

@hoelzro
Copy link
Contributor Author

hoelzro commented Oct 7, 2020

Makes sense to me - I've filed a new issue to focus on the system tiddlers syncing (#4882)

@scarabdesign
Copy link

IMHO, these settings should all be optional. I can appreciate how someone would want the StoryList and state tiddlers synced, especially when using it purely as a personal note taking tool. However, in my case, this is the biggest disruption. I'm setting the DefaultTiddlers with a filter that should make my newest published tiddler the startup tiddler for all other users. It should work, except for the StoryList is clobbering the filter as soon as it loads. I see the correct tiddler appear, then, since my StoryList is empty, it closes everything.
I tried to deal with this by disabling the synching of the StoryList ( [$:/config/SyncFilter] >> -[[$:/StoryList]] ), but how do I keep if from syncing back to every other client on refresh? Any suggestions for a workaround would be great.

@hoelzro
Copy link
Contributor Author

hoelzro commented Oct 13, 2020

@scarabdesign Do you still have $:/StoryList in your tiddlers directory? If so, even though your sync filter should prevent changes to it from syncing back to the server, I believe the server will still include it in subsequent page loads.

@scarabdesign
Copy link

@scarabdesign Do you still have $:/StoryList in your tiddlers directory? If so, even though your sync filter should prevent changes to it from syncing back to the server, I believe the server will still include it in subsequent page loads.

Yes, that's the reason for the problem I sited. Is there a way to prevent it? I've tried deleting the tiddler, but it doesn't get deleted, unless I'm doing it wrong.

@hoelzro
Copy link
Contributor Author

hoelzro commented Oct 14, 2020

@scarabdesign I imagine that if you delete the tiddler from within TiddlyWiki itself, your sync filter will prevent that deletion from going to the server - I think you'd need to shut down the server, delete the tiddler on disk, and then restart the server.

@Jermolene
Copy link
Member

Thanks @hoelzro @saqimtiaz @scarabdesign @pmario. I think it's reasonable to aim for a narrow fix for the moment – to continue to sync system tiddlers, but to explicitly exclude $:/StoryList and any others that might crop up. In my testing, adding $:/StoryList to the filter in $:/config/SyncFilter appears to address the problem. Is it sufficient or am I missing something?

@hoelzro
Copy link
Contributor Author

hoelzro commented Oct 19, 2020

@Jermolene It seems to suffice for the most part - I did a little bit of tinkering and noticed that $:/StoryList (as well as $:/HistoryList) are still entering the syncer's tiddlerInfo store, because tiddlywebadaptor's filter is still picking them up. I wonder if this is the source of @scarabdesign's issue.

@scarabdesign
Copy link

scarabdesign commented Oct 19, 2020

@hoelzro
I finally got a chance to try what you suggested and it seems to have resolved the issue for me. I was weary at first because the term "StoreList" is peppered throughout the codebase, but I killed the server, moved core/ui/Filters/StoryList.tid (and ./editions/es-ES/tiddlers/$__StoryList.tid for good measure) into a backup directory in my home directory, restarted the server and it seems to be behaving. Thanks for the input!

@Jermolene
Copy link
Member

Thanks @hoelzro @scarabdesign I'm going to go ahead and commit the change to the SyncFilter

@saqimtiaz
Copy link
Member

@Jermolene I am not convinced we have the right fix here. We have tried to resolve one bug by breaking another feature, namely the ability to set the story list as the default tiddlers.

I think what we need is to go back to how things were in 5.1.21 until we have a better solution. That is, the story list should be synced from the client to the server, but not from the server to the client.

@scarabdesign
Copy link

@saqimtiaz

That is, the story list should be synced from the client to the server, but not from the server to the client.

Ok, so it would be saved to the server but not synced out to the client during updates. What would that mean for new visitors to the site on the first visit? Would they get a new empty StoryList or would they get the StoryList that was saved to the server by the last user? If they get the saved one, they will see a bunch of tids they never requested and it may be quite confusing. However, if new users do not get the saved copy of the StoryList but an empty one, won't it overwrite the StoryList of the last user? If that's the case, is there a point to saving the StoryList at all?

Spit balling here, but it seems more and more that each user needs to have a unique StoryList. Maybe authenticated users can save their lists to the server, if not, an option to save in local storage (if that's enabled).

@saqimtiaz
Copy link
Member

saqimtiaz commented Nov 4, 2020

@scarabdesign $:/DefaultTiddlers takes precedence over the saved $:/StoryList when the wiki starts in determining what tiddlers to show, and the $:/StoryList is updated accordingly.

So in a multi-user situation if you set your DefaultTiddlers to a specific set of tiddlers, that is what a new user will see when they load the wiki. The problem right now is that after this happens, $:/StoryList is synced from the server to the client and the tiddlers displayed change to match.

Saving the $:/StoryList is often an important part of the workflow for single user use cases, which is the vast majority of TW users. You can set your default tiddlers to [list[$:/StoryList]] to load the tiddlers from the story list, thereby resuming your wiki session where you left off.

Try running 5.1.21 to see the previous behaviour from before this bug with crept in due to the syncing of system tiddlers from the server to the client.

As discussed earlier in this thread, the goal at this point should be to resolve this bug and return to previous stable behaviour, preferably without breaking other functionality, and then figure out a better solution for the multi-user use case separately.

@saqimtiaz
Copy link
Member

@Jermolene another issue that seems related to the excluding of $:/StoryList from the syncFilter is that permalink handling isn't working properly.

When loading a permalink like:
http://127.0.0.1:3333/#:%24%3A%2Fconfig%2FSyncFilter

The $:/config/SyncFilter tiddler appears for a few seconds before being replaced by the DefaultTiddlers.

Removing -[prefix[$:/StoryList]] from the syncFilter resolves the problem.

@saqimtiaz
Copy link
Member

So I've been looking into the sync mechanism and I will try to summarize my understanding of the problem:

  • we have the $:/config/syncFilter which is used by the syncer to determine which tiddlers to sync
  • the tiddlyweb plugin has its own filter which it uses to determine what tiddlers to send in response to get a getSkinnyTiddlers call.
  • Tiddlers that are not on the client but on the server, are fetched by the client.
  • Tiddlers that are on the client but not on the sever, are deleted by the client.
  • What seems missing here is a mechanism for uni-directional sync. For instance a way to sync tiddlers from the client to the server but not from the server to the client. So one could sync changes to the StoryList to the server but not have them propagated to other clients. Such an approach would resolve this bug and still allow using StoryList in DefaultTiddlers.
  • I wonder if the syncer needs its own filter to exclude tiddlers from deletion from the server.

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

No branches or pull requests

5 participants