Make sync result message clearer and add FW loading state #2027
Conversation
github-actions
bot
added
the
💻 FW Lite
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds a loading-text utility class to CSS, introduces a loading prop to the relative date component, updates sync function signatures to accept commit counts, propagates loading states and class bindings across sync UI, and revises SyncDialog messages to include detailed commit/change counts. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
frontend/viewer/src/app.postcss (1)
30-32: Improve skeleton rendering on inline elements and consider a11yrounded-md on inline text often doesn’t display as expected; add inline-block so the background radius is visible. Also, since text is transparent, screen readers may still announce it—consider marking placeholders aria-hidden or the container aria-busy at usage sites.
Apply:
.loading-text { - @apply bg-foreground/15 animate-pulse rounded-md text-transparent; + @apply bg-foreground/15 animate-pulse rounded-md text-transparent inline-block; }frontend/viewer/src/lib/components/ui/format/format-relative-date.svelte (1)
65-66: Apply skeleton only to the time element, not the wrapperBinding loading-text on the span hides the info icon and popover trigger. Apply it to instead to limit the effect to the text.
-{#if showActualDate && actualFormattedDate} - <span class:loading-text={loading} class="inline-flex items-center gap-1"> - <time {...restProps}>{formattedRelativeDate}</time> +{#if showActualDate && actualFormattedDate} + <span class="inline-flex items-center gap-1"> + <time class:loading-text={loading} {...restProps}>{formattedRelativeDate}</time> <Popover.Root>frontend/viewer/src/project/SyncDialog.svelte (1)
67-74: Avoid concatenating translated strings; use one message with pluralsConcatenating $t segments complicates localization (word order, spacing, grammar). Prefer a single translation with plural macros.
- success: (result) => { - const fwdataCommitsText = $plural(lexboxToFlexCount ?? 0, {one: '# commit', other: '# commits'}); - const crdtCommitsText = $plural(flexToLexboxCount ?? 0, {one: '# commit', other: '# commits'}); - const fwdataChangesText = $plural(result.syncResult?.fwdataChanges ?? 0, {one: '# change', other: '# changes'}); - const crdtChangesText = $plural(result.syncResult?.crdtChanges ?? 0, {one: '# change', other: '# changes'}); - return $t`Synced ${fwdataCommitsText} to FieldWorks, which resulted in ${fwdataChangesText} ` - + $t`and ${crdtCommitsText} to FieldWorks Lite, which resulted in ${crdtChangesText}. ` - + $t`(Changes to FieldWorks Lite can be caused by new releases that include more fields/data from FieldWorks).`; - }, + success: (result) => + $t`Synced ${$plural(lexboxToFlexCount ?? 0, { one: '# commit', other: '# commits' })} to FieldWorks, which resulted in ${$plural(result.syncResult?.fwdataChanges ?? 0, { one: '# change', other: '# changes' })} and ${$plural(flexToLexboxCount ?? 0, { one: '# commit', other: '# commits' })} to FieldWorks Lite, which resulted in ${$plural(result.syncResult?.crdtChanges ?? 0, { one: '# change', other: '# changes' })}. (Changes to FieldWorks Lite can be caused by new releases that include more fields/data from FieldWorks).`,frontend/viewer/src/project/sync/SyncStatusPrimitive.svelte (1)
25-26: Align default handler arity with prop typeMinor: match the declared (a, b) => Promise signature to avoid potential strictFunctionTypes issues and clarify intent.
- syncLexboxToFlex?: (flexToLexboxCount: number, lexboxToFlexCount: number) => Promise<void>; + syncLexboxToFlex?: (flexToLexboxCount: number, lexboxToFlexCount: number) => Promise<void>; @@ - syncLexboxToFlex = async () => { + syncLexboxToFlex = async (_flexToLexboxCount: number, _lexboxToFlexCount: number) => { },Also applies to: 39-41
frontend/viewer/src/project/sync/FwLiteToFwMergeDetails.svelte (2)
40-41: Nice loading coordination; add simple a11y affordanceloading derived from remoteStatus is reasonable; forwarding it to FormatRelativeDate is good. For accessibility, consider:
- Marking the parent container aria-busy={loading}.
- Marking the skeleton spans aria-hidden={loading} so screen readers don’t announce placeholder “? Commits”.
Example:
-<FormatRelativeDate +<FormatRelativeDate date={lastFwLiteSyncDate} showActualDate {loading} defaultValue={remoteStatus?.status === ProjectSyncStatusEnum.NeverSynced ? $t`Never` : $t`Unknown`}/> @@ -<span class:loading-text={loading}>{$t`${flexToLexboxCount} Commits`}</span> +<span class:loading-text={loading} aria-hidden={loading}>{$t`${flexToLexboxCount} Commits`}</span> @@ -<span class:loading-text={loading}>{$t`${lexboxToFlexCount} Commits`}</span> +<span class:loading-text={loading} aria-hidden={loading}>{$t`${lexboxToFlexCount} Commits`}</span>Also applies to: 56-61, 96-98
77-80: Avoid casting; prefer typed countsInstead of using '?' sentinel with casts, type counts as number | undefined and guard, which removes the need for as number and reduces risk.
- let lexboxToFlexCount = $derived(remoteStatus?.pendingCrdtChanges ?? '?'); - let flexToLexboxCount = $derived(remoteStatus?.pendingMercurialChanges ?? '?'); + let lexboxToFlexCount = $derived<number | undefined>(remoteStatus?.pendingCrdtChanges); + let flexToLexboxCount = $derived<number | undefined>(remoteStatus?.pendingMercurialChanges); @@ - disabled={loadingSyncLexboxToLocal || !canSyncLexboxToFlex || !remoteStatus - || (typeof flexToLexboxCount !== 'number' || typeof lexboxToFlexCount !== 'number')} - onclick={() => onSyncLexboxToFlex(flexToLexboxCount as number, lexboxToFlexCount as number)} + disabled={loadingSyncLexboxToLocal || !canSyncLexboxToFlex || !remoteStatus + || flexToLexboxCount == null || lexboxToFlexCount == null} + onclick={() => onSyncLexboxToFlex(flexToLexboxCount!, lexboxToFlexCount!)} @@ - <span class:loading-text={loading}>{$t`${flexToLexboxCount} Commits`}</span> + <span class:loading-text={loading}>{$t`${flexToLexboxCount ?? '?'} Commits`}</span> @@ - <span class:loading-text={loading}>{$t`${lexboxToFlexCount} Commits`}</span> + <span class:loading-text={loading}>{$t`${lexboxToFlexCount ?? '?'} Commits`}</span>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/viewer/src/app.postcss(1 hunks)frontend/viewer/src/lib/components/ui/format/format-relative-date.svelte(4 hunks)frontend/viewer/src/project/SyncDialog.svelte(1 hunks)frontend/viewer/src/project/sync/FwLiteToFwMergeDetails.svelte(4 hunks)frontend/viewer/src/project/sync/SyncStatusPrimitive.svelte(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build UI / publish-ui
- GitHub Check: Build API / publish-api
- GitHub Check: check-and-lint
- GitHub Check: Build FW Lite and run tests
- GitHub Check: frontend-component-unit-tests
🔇 Additional comments (5)
frontend/viewer/src/lib/components/ui/format/format-relative-date.svelte (2)
17-18: Prop addition looks goodOptional loading prop and destructuring are fine.
Also applies to: 28-29
78-79: LGTM on else pathApplying loading-text directly to here is appropriate.
frontend/viewer/src/project/SyncDialog.svelte (2)
60-60: Signature change is consistent with callersTwo-count signature matches upstream components.
Please confirm all call sites were updated (SyncStatusPrimitive and FwLiteToFwMergeDetails look aligned).
65-65: Clearer loading copy: LGTMReadable, concise loading message.
frontend/viewer/src/project/sync/FwLiteToFwMergeDetails.svelte (1)
33-35: Wiring and handler: LGTMProps signature and onSync handler passing the two counts look correct; loading flag management is clear.
Also applies to: 42-47
myieye
force-pushed
the
adapt-sync-messages
branch
from
ff76174 to
3dfb8dd
Compare
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
|
Talking to Sara, in order to handle the discrepancy between commit and change counts, it sounds like a decent idea to hide commit counts behind a tooltip that also explains why the commit count likely does not match the change count. |
myieye
force-pushed
the
adapt-sync-messages
branch
from
519ec2c to
91028c3
Compare
myieye
force-pushed
the
adapt-sync-messages
branch
from
1998b49 to
1788113
Compare
| {$t`This will synchronize the FieldWorks Lite and FieldWorks Classic copies of your project in Lexbox.`} | ||
| </p> | ||
| <p> | ||
| {$t`After the sync, changes made in FW Classic will appear in FW Lite and changes made in FW Lite will appear in FW Classic after the next Send/Receive.`} |
There was a problem hiding this comment.
💭 This sentence is a little hard to parse, because of the initial "after" clause that applies to both directions and the final "after" clause that only applies to the latter direction.
Perhaps two sentences? Something like:
After the sync, changes made in FieldWorks Classic will appear in FieldWorks Lite. You will also need to perform Send/Receive for the changes made in FieldWorks Lite to appear in FieldWorks Classic.
I don't really love this suggestion much better, but it should demonstrate a rewording direction.
There was a problem hiding this comment.
🔧 Also, FW -> FieldWorks for public messages.
There was a problem hiding this comment.
What do you think of that?
This is the reason I initially shortened FieldWorks to FW 😆
There was a problem hiding this comment.
💭 I quite like your rewording, though one thing gives me pause (which is also an issue in your original wording):
... will automatically receive changes made in FieldWorks Classic.
refers to changes made before the sync, but could be misread as
... will henceforth start to automatically receive changes as they are made in FieldWorks Classic.
I hesitate to give wordier suggestion, but "changes that were made" may be better than "changes made" to avoid any mis-implication of the sync being the activation of an ongoing auto-sync.
Or another approach:
The sync makes prior changes in FieldWorks Classic available to all FieldWorks Lite users. FieldWorks Classic users will not see synced changes from FieldWorks Lite until they also do Send/Receive.
There was a problem hiding this comment.
💭 With your original pre-"FW" wording, a slightly wider popup window could give a nicer looking read.
| {$t`One FieldWorks Classic commit may consist of changes to multiple entries or fields. On the other hand, a commit may only affect data that is not synced to FieldWorks Lite.`} | ||
| </span> | ||
| <span> | ||
| {$t`Changes can also be the result of additional fields being added to new versions of FieldWorks Lite.`} |
There was a problem hiding this comment.
⛏️ "additional fields being added" is a bit redundant
There was a problem hiding this comment.
I'm not sure I follow. It's trying to describe the data model growing i.e. FieldWorks Lite suddenly supporting an additional field that it didn't previously.
I put it in there, because when we add a new field to the model, even if there are 0 incoming commits from FieldWorks, a sync still might generate thousands of changes. And this line explains why that's the case.
There was a problem hiding this comment.
💭 The description is very clear. It may be that something like
... the result of fields being added to new versions of FieldWorks Lite.
or
... the result of new versions of FieldWorks Lite having additional fields.
would suffice.
There was a problem hiding this comment.
Oh, I see. Yes, you're probably right.
| <Icon src={flexLogo} class="size-10 mb-1" alt={$t`FieldWorks logo`}/> | ||
| <p>FieldWorks Classic</p> | ||
| <Icon src={flexLogo} class="size-10 mb-1" alt={$t`FieldWorks logo`} /> | ||
| <p><span class="max-xs:hidden">FieldWorks </span> Classic</p> |
There was a problem hiding this comment.
❓ With this change, it'd be nice to have a associated xs-width window screenshot.
There was a problem hiding this comment.
Which seems like a minor improvement to:
There was a problem hiding this comment.
Thanks! It is an improvement.
🌱 Dropping the hours (if days >= 1), may also help.
| <Tabs.List class="w-full md:mb-2 max-md:mt-4 max-md:sticky bottom-0 z-[1]"> | ||
| <Tabs.Trigger class="flex-1" value="lite">{$t`FieldWorks Lite`}</Tabs.Trigger> | ||
| <Tabs.Trigger class="flex-1" value="classic">{$t`FieldWorks Classic`}</Tabs.Trigger> | ||
| <Tabs.Trigger class="flex-1 gap-2" value="classic"><Icon icon={cloudIcon} class="-my-1" /> {$t`Lexbox`}</Tabs.Trigger> |
There was a problem hiding this comment.
❓ I find the change of the second tab's name from "FieldWorks Classic" to " ☁️ Lexbox" a bit confusing. Don't both tabs involve syncing with/via Lexbox?
There was a problem hiding this comment.
Yeah...I don't know what the perfect name is.
I like ☁️ Lexbox, because it (accurately) reflects the top part of the FieldWorks Lite tab.
I.e. your changes get synced up to Lexbox and then you can "go to Lexbox" and sync them there:
There have been indicators that "FieldWorks Classic" is confusing, because the user could actually have a FieldWorks Classic copy of the project on their harddrive and it would be plausible to think that they could sync the FieldWorks Lite project with that.
Of all the ideas I currently have, I like this one most.
And Sara (our liaison) is happy with it.
There was a problem hiding this comment.
❓ I'm confused why your annotated screenshot is associating the "Lexbox" content of the active left tab with the "Lexbox" title of the inactive right tab.
There was a problem hiding this comment.
💭 Oh, sorry, I now see that with the arrow, it does illustrate what you said:
get synced up to Lexbox and then you can "go to Lexbox" and sync
I was mentally categorizing the two tabs as two alternative, independent syncing options. If the right tab is " ☁️ Lexbox", would it make sense for the left tab to be " 💻 Local"? (Probably not, if it's only dealing with local FW Lite projects.)
There was a problem hiding this comment.
Yeah, "local" doesn't feel satisfactory to me, because it's a sync with the cloud. So, only half of it is local 😕.
Originally, Kevin had "Local" and "Remote". Then I suggested changing it to "FieldWorks Lite" and "FieldWorks Classic". That proved to not be intuitive and I primarily got the feedback that the "FieldWorks Classic"/remote/Lexbox side should be more obviously something server-side.
There was a problem hiding this comment.
What about wordless " 🖥️
2 participants
Example of new sync result message:
No more commit counts displayed automatically:
FW -> FWL Tooltip:
Expanded:
FWL -> FW Tooltip:
Expanded:
Updated titles and tab names:
Lexbox sync tooltip:
Discarded attempt at a very explicit and verbose message:
Loading indicators:
loading-flex-sync-dialog.mp4