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
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