Updated time verified not working while using Genreic Version Storage/JSONBIN

[kibbon]

Describe the bug When using this plug-in for multi-person collaboration, I noticed that the plug-in does not remind us that there are conflicts in our changes. Even if the local data is older than the remote data, it can always be submitted and overwritten. In the end, we tested and determined that the logic of comparing local and remote update timestamps in the plugin source code did not work.

To Reproduce Steps to reproduce the behavior:

1. Use JSONBin or Genreic Version Storage as sync method, People A & People B use same key.
2. People A update his design token (Update on changes clicked)
3. People B update his design token without syncing
4. People A sync.

Expected behavior People B should see warning like this:

Screenshots or Screencasts In fact, B updated successfully like this: and A found that his update overwritten.

Figma file (optional) Any Figma

JSON (optional) Any JSON

Point After checking source code we find:

rootState.uiState.lastUpdatedAt (tokenState.updateDocument) 
-> lastUpdatedAt (updateTokensOnSources) 
-> lastUpdatedAt  (updateRemoteTokens) 
-> oldUpdatedAt (updateGenericVersionedTokens)
-> compareUpdatedAt

It seems that rootUI.state.lastUpdatedAt never update after initialized as null, and rootState.uiState.lastUpdatedAt ?? new Date().toISOString() always go to fallback logic.

Other bugs

1. Auto update never triggered since REMOVING/RENAMING tokens.
2. Close the plugin then reopen it, sync connection losed, need to be reactived by update settings.
3. PUT Request failed (404/500/400) without any notification while using Genreic Version Storage.

Suggestions

1. Use data version instead of data updatedAt may be better, and 'version/updatedAt' need to be shown at plugin panel.
2. notifyToUI should be triggered when any request failed, and its content should contain response.message.
3. GET request before PUT during retrieval need to be diffrenet from normal GET request (by send local updatedAt timestamp for server checking or other resolutions), because the first one may block the subsequent process, while second one always do sync logic successfully.

[six7]

Mistakenly marked this as To be released as I thought that would've solved the issue, but it didn't. Moved it back to Backlog.

[kibbon]

Mistakenly marked this as To be released as I thought that would've solved the issue, but it didn't. Moved it back to Backlog.

How long will it take for this problem to be resolved? Teamwork is important for promoting this plugin in my design team. I want to start using it as soon as possible. Thanks for any support！🙏

[six7]

cc'ing @SorsOps here who worked on the generic version storage who can shed more light on this

[kibbon]

Issue confirmed? @SorsOps @six7 cc

[SorsOps]

@kibbon Apologies I was elsewhere doing things .

You're right in that the logic for the retrieval and writing for both Jsonbin, and generic storage which was based off it was incorrect.

The culprit was fetch in the following lines

    if (response.ok) {
      return response.json();
    }

    return false;

It looks like the expectation was that fetch would throw an error if there was a failure but it didn't which caused silent failures

Additionally there were try catch handlers inside the jsonBin useStorage hooks that would detect an error and the just send to sentry or dump to console without any display to the user.

In terms of the date parsing. I believe that could be due to https://github.com/tokens-studio/figma-plugin/pull/1736 where generic storage had a bug fixed where it became more permissive of parsing different incoming date values

Regarding the other parts, I think the generic storage needs to have a better lifecycle as well. but it was initially designed to be the most basic implementation based on a simple flow of just pushing and pulling with minimal additional overhead so that others could hook into it as they want.

To make a V2 we probably want to support additional features like delta calculations, though we're currently skipping that by putting all that responsibility on the server

These other bugs

Auto update never triggered since REMOVING/RENAMING tokens. Close the plugin then reopen it, sync connection losed, need to be reactived by update settings.

I've also seen that importing a preset also doesn't trigger it updates, but neither the JSONbin or generic storage handle trigger logic as they're just providers so likely this shouldn't work for other providers?

I've made a branch to address the time issues and notification here https://github.com/tokens-studio/figma-plugin/pull/1823

[kibbon]

Thanks for your reply! I totally agree that 'putting all that responsibility on the server', so that the behavior of generic stroage could be more preticable. For 'removeing/renaming tokens never trigger update', the 'update' means 'sync to remote', for example, I delete token A, this change would not sync, then I update value of token B, it sync correctly. So if I want to delete token and update to remote, I need do other things.

[SorsOps]

@kibbon I've updated my MR, is should now send an update to the server for deletion, renaming and duplication of token values, hopefully this will help as part of the way to resolving how to synchronize the states

[kibbon]

@SorsOps I have test current token studio plugin code on the next branch, thanks again for resolving the "network error notify" and "update on delete/rename" bugs, it works now. However, the "compareUpdatedAt always passed" bug still exist, it seems that lastUpdate state always be current while sending PUT request. Is it possible to carry storage server callback data in client request? For example, server response with '{ callbackData: { tokenVersion: '1.0'} }', client request with it, and server check query tokenVersion with DB tokenVersion, and decide to pass or refuse. Instead of local timestamp validation, it could be much more controllable for storage server developers. @six7 cc

[kibbon]

@six7 @SorsOps remind~

[kibbon]

@SorsOps "compareUpdatedAt always passed" issue still exist while using, hope for any resolution 🙏🏻

[six7]

that PR is going to be part of the next major release 👍

[kibbon]

that PR is going to be part of the next major release 👍

Thank you for replying! My question is that I have test the code in this PR and local update time always be NOW (from request)，and the bad case: 'A upload -> B upload -> A upload without syncing' still exist.

My Check Code: src/app/store/providers/generic/versionStorage.ts -> updateGenericVersionTokens L40-L50

const remoteTokens = await storage.retrieve();
      if (remoteTokens?.status === 'failure') {
        notifyToUI('Error updating Generic Storage, check console (F12) ', { error: true });
        return {
          status: 'failure',
          errorMessage: remoteTokens?.errorMessage,
        };
      }

      console.log('updatings', oldUpdatedAt, remoteTokens?.metadata?.updatedAt); // NOTICE HERE, oldUpdatedAt seems always be Date.now()
      const comparison = await compareUpdatedAt(oldUpdatedAt, remoteTokens?.metadata?.updatedAt ?? '');

I guess: After getting remote token, we update remoteToken.metadata.updatedAt to lastSyncedState (@store/models/tokenState) While checking token updateTime, we use local oldUpdatedAt from lastUpdatedAt (@store/models/uiState) So I wish this issue could be check again. Many thanks for your help!

@six7 @SorsOps cc， I have tried 1.35.5, still failed

[kibbon]

anothoer two issues @src/app/store/providers/versionStorages.ts

1. notifyToUI('Error updating Generic Storage, check console (F12) ', { error: true }); -> except notify response.errorMessage
2. save request ('PUT' method) will not trigger error (404/500/400 -> response.ok) @six7 @SorsOps CC

[kibbon]

@six7 @SorsOps Please see above

[kibbon]

that PR is going to be part of the next major release 👍

Thank you for replying! My question is that I have test the code in this PR and local update time always be NOW (from request)，and the bad case: 'A upload -> B upload -> A upload without syncing' still exist.

My Check Code: src/app/store/providers/generic/versionStorage.ts -> updateGenericVersionTokens L40-L50

const remoteTokens = await storage.retrieve();
      if (remoteTokens?.status === 'failure') {
        notifyToUI('Error updating Generic Storage, check console (F12) ', { error: true });
        return {
          status: 'failure',
          errorMessage: remoteTokens?.errorMessage,
        };
      }

      console.log('updatings', oldUpdatedAt, remoteTokens?.metadata?.updatedAt); // NOTICE HERE, oldUpdatedAt seems always be Date.now()
      const comparison = await compareUpdatedAt(oldUpdatedAt, remoteTokens?.metadata?.updatedAt ?? '');

I guess: After getting remote token, we update remoteToken.metadata.updatedAt to lastSyncedState (@store/models/tokenState) While checking token updateTime, we use local oldUpdatedAt from lastUpdatedAt (@store/models/uiState) So I wish this issue could be check again. Many thanks for your help!

@six7 @SorsOps cc， I have tried 1.35.5, still failed

@six7 @SorsOps cc

[SamIam4Hyma]

@SorsOps can this issue be closed as its quite stale?

[SorsOps]

I'm sorry , I'll take a look at this, I lost this in all my messages

[SamIam4Hyma]

@SorsOps no worries! I added the label to add it to the FR backlog and assigned you to it so you can take a peek when you get a chance. I suspect you can likely just close it.