Sync dialog touch ups#2001
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 📝 WalkthroughWalkthroughIntroduces sync-status-driven UI in FwLiteToFwMergeDetails.svelte with new props (syncStatus, server, onLoginStatusChange) and conditional rendering for offline/login states. SyncStatusPrimitive.svelte updates layout, moves “Last sync” display, adjusts remote gating, and passes new props downstream. Locale files replace “Last change: #” with “Last sync: #” and update message locations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks (3 passed)✅ 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 |
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
frontend/viewer/src/locales/es.po (1)
601-604: Provide Spanish translation for new key.Suggested translation to avoid English fallback.
#: src/project/sync/SyncStatusPrimitive.svelte msgid "Last sync: #" -msgstr "" +msgstr "Última sincronización: #"frontend/viewer/src/locales/ms.po (1)
601-604: Add Malay translation for “Last sync: #”.#: src/project/sync/SyncStatusPrimitive.svelte msgid "Last sync: #" -msgstr "" +msgstr "Penyegerakan terakhir: #"frontend/viewer/src/locales/id.po (1)
601-604: Add Indonesian translation for “Last sync: #”.#: src/project/sync/SyncStatusPrimitive.svelte msgid "Last sync: #" -msgstr "" +msgstr "Sinkronisasi terakhir: #"frontend/viewer/src/locales/ko.po (1)
601-604: Add Korean translation for “Last sync: #”.#: src/project/sync/SyncStatusPrimitive.svelte msgid "Last sync: #" -msgstr "" +msgstr "마지막 동기화: #"frontend/viewer/src/locales/sw.po (1)
601-604: Add Swahili translation for “Last sync: #”.#: src/project/sync/SyncStatusPrimitive.svelte msgid "Last sync: #" -msgstr "" +msgstr "Usawazishaji wa mwisho: #"frontend/viewer/src/locales/fr.po (1)
601-604: Add French translation for “Last sync: #”.#: src/project/sync/SyncStatusPrimitive.svelte msgid "Last sync: #" -msgstr "" +msgstr "Dernière synchronisation : #"frontend/viewer/src/project/sync/FwLiteToFwMergeDetails.svelte (3)
34-35: Avoid string-literal drift for auth status.If
LoginButtonexports a status type, prefer reusing it here (e.g.,type AuthStatus = ...) to keep both sides in sync.
62-83: Handle NotLoggedIn without server, add a11y, and simplify handler.
- Gate on
NotLoggedInregardless ofserver, and show a fallback message ifserveris missing.- Add
aria-labelto the offline icon.- Pass the handler by reference.
Apply:
- {#if syncStatus === SyncStatus.Offline} - <Icon icon="i-mdi-cloud-off-outline" class="size-10 m-4" /> - {:else if syncStatus === SyncStatus.NotLoggedIn && server} - <LoginButton - text={$t`Login`} - status={{loggedIn: false, server: server}} - statusChange={s => onLoginStatusChange(s)}/> + {#if syncStatus === SyncStatus.Offline} + <Icon icon="i-mdi-cloud-off-outline" class="size-10 m-4" aria-label={$t`Offline`} /> + {:else if syncStatus === SyncStatus.NotLoggedIn} + {#if server} + <LoginButton + text={$t`Login`} + status={{ loggedIn: false, server }} + statusChange={onLoginStatusChange}/> + {:else} + <span class="text-sm text-foreground/80">{$t`Not logged in`}</span> + {/if}
54-60: PR objective mismatch: “Last change” vs “Last sync”.This component still renders “Last change: #”, but the PR aims to use “Last sync: #”. If these dates are intended to represent last sync, update the label (and ensure the dates reflect sync events, not commit times).
- <T msg="Last change: #"> + <T msg="Last sync: #">Repeat for both occurrences. Also confirm locale entries exist for the new message.
Also applies to: 89-95
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
frontend/viewer/src/locales/en.po(3 hunks)frontend/viewer/src/locales/es.po(3 hunks)frontend/viewer/src/locales/fr.po(3 hunks)frontend/viewer/src/locales/id.po(3 hunks)frontend/viewer/src/locales/ko.po(3 hunks)frontend/viewer/src/locales/ms.po(3 hunks)frontend/viewer/src/locales/sw.po(3 hunks)frontend/viewer/src/project/sync/FwLiteToFwMergeDetails.svelte(2 hunks)frontend/viewer/src/project/sync/SyncStatusPrimitive.svelte(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-02T14:27:02.745Z
Learnt from: myieye
PR: sillsdev/languageforge-lexbox#1720
File: frontend/viewer/src/locales/es.json:1786-1790
Timestamp: 2025-06-02T14:27:02.745Z
Learning: Spanish locale file (frontend/viewer/src/locales/es.json) contains generated code that may have null line numbers in origin entries due to limitations in the code generation process.
Applied to files:
frontend/viewer/src/locales/es.po
⏰ 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). (6)
- GitHub Check: Build UI / publish-ui
- GitHub Check: Build API / publish-api
- GitHub Check: check-and-lint
- GitHub Check: frontend
- GitHub Check: frontend-component-unit-tests
- GitHub Check: Build FW Lite and run tests
🔇 Additional comments (20)
frontend/viewer/src/locales/es.po (2)
641-645: Location mapping for “Local” looks right.No change needed.
650-654: “Login” reference addition is fine.Matches usage in both components.
frontend/viewer/src/locales/ms.po (2)
641-645: Dual context for “Local” is correct.
650-654: “Login” mapping moved — OK.frontend/viewer/src/locales/id.po (2)
641-645: Added “Local” context — OK.
650-654: “Login” context addition — OK.frontend/viewer/src/locales/ko.po (2)
641-645: “Local” extra location is fine.
650-654: “Login” mapping move — OK.frontend/viewer/src/locales/sw.po (2)
641-645: “Local” extra location — OK.
650-654: “Login” mapping — OK.frontend/viewer/src/locales/en.po (3)
596-599: New msgid “Last sync: #” added — OK.
636-640: “Local” now referenced here too — OK.
645-649: “Login” also referenced from FwLiteToFwMergeDetails — OK.frontend/viewer/src/locales/fr.po (2)
641-645: “Local/Locale” mapping looks correct.
650-654: “Login/Connexion” mapping addition — OK.frontend/viewer/src/project/sync/SyncStatusPrimitive.svelte (3)
53-54: Confirm intent: remote tab now displays even when offline.Previously gated by server && !Offline. Ensure this UX change is deliberate.
138-139: Good: “Local” now localized.
145-148: Prop forwarding to details component — OK.Matches the new public API on FwLiteToFwMergeDetails.
frontend/viewer/src/project/sync/FwLiteToFwMergeDetails.svelte (2)
11-13: Confirm SyncStatus is a runtime enum.You compare against
SyncStatus.Offline/NotLoggedInin the template. Please verifySyncStatusis emitted at runtime (not just a type or erasedconst enum), otherwise these checks will fail.
15-36: LGTM on new props and defaults.The additions (
syncStatus,server,onLoginStatusChangewith no-op default) look correct and safe.
1 participant
Made a few changes, because I was looking at it while writing to a user.
The date is a bit tricky.
It actually means: this is the last time a change went across the wire.
So, I think "Last sync" is a fair bit more accurate than "Last change".