From a3024d855d135d0ea88f2243a929a581024b4656 Mon Sep 17 00:00:00 2001 From: davotoula Date: Thu, 7 May 2026 15:14:30 +0200 Subject: [PATCH] =?UTF-8?q?Code=20review:=20-=20Switch=20list-screen=20for?= =?UTF-8?q?matters=20from=20java.text.DateFormat=20to=20=20=20java.time.fo?= =?UTF-8?q?rmat.DateTimeFormatter.=20DateFormat=20/=20SimpleDateFormat=20?= =?UTF-8?q?=20=20are=20documented=20not-thread-safe;=20the=20file-scope=20?= =?UTF-8?q?singletons=20are=20at=20=20=20risk=20under=20any=20future=20mul?= =?UTF-8?q?ti-threaded=20recomposition=20path.=20=20=20DateTimeFormatter?= =?UTF-8?q?=20is=20immutable=20and=20thread-safe.=20-=20LogoutButton:=20wh?= =?UTF-8?q?en=20decodePublicKeyAsHexOrNull=20fails,=20bail=20before=20=20?= =?UTF-8?q?=20the=20Toast=20fires=20so=20we=20don't=20claim=20"Logged=20ou?= =?UTF-8?q?t"=20while=20logOff's=20=20=20coroutine=20has=20aborted=20its?= =?UTF-8?q?=20cleanup.=20Theoretical=20case=20(npub=20from=20=20=20prefs?= =?UTF-8?q?=20is=20well-formed=20in=20practice)=20but=20the=20previous=20f?= =?UTF-8?q?low=20was=20=20=20misleading=20on=20the=20failure=20path.=20-?= =?UTF-8?q?=20ScheduledPostStore.purgeStale:=20document=20the=20=20=20term?= =?UTF-8?q?inatedAtSec=20=3F:=20lastAttemptAtSec=20=3F:=20createdAtSec=20f?= =?UTF-8?q?allback=20so=20=20=20the=20legacy-row=20migration=20semantics?= =?UTF-8?q?=20are=20clear=20from=20the=20source.=20-=20DrawerContent:=20dr?= =?UTF-8?q?op=20fully-qualified=20inline=20references,=20add=20proper=20?= =?UTF-8?q?=20=20imports=20for=20Amethyst,=20ScheduledPostStatus,=20and=20?= =?UTF-8?q?Material3=20Badge.=20-=20ScheduledPostsScreen:=20drop=20the=20r?= =?UTF-8?q?edundant=20`posts`=20collection=20=E2=80=94=20derive=20=20=20th?= =?UTF-8?q?e=20empty-state=20branch=20from=20`groups`=20directly.=20Use=20?= =?UTF-8?q?`group.day`=20as=20the=20=20=20sticky-header=20key=20instead=20?= =?UTF-8?q?of=20an=20interpolated=20string.=20Cache=20the=20SHORT=20=20=20?= =?UTF-8?q?time=20and=20FULL=20date=20`DateFormat`=20instances=20at=20file?= =?UTF-8?q?=20scope=20(matches=20the=20=20=20pattern=20in=20TimeAgoFormatt?= =?UTF-8?q?er).=20Pass=20`today`=20into=20`DayHeader`=20so=20we=20=20=20do?= =?UTF-8?q?n't=20call=20`LocalDate.now()`=20per=20recomposition.=20-=20Sch?= =?UTF-8?q?eduledPostsViewModel:=20drop=20the=20intermediate=20public=20`p?= =?UTF-8?q?osts`=20=20=20StateFlow=20now=20that=20no=20consumer=20needs=20?= =?UTF-8?q?it;=20fold=20the=20filter+sort=20into=20=20=20the=20`groupedPos?= =?UTF-8?q?ts`=20chain.=20-=20ScheduledPostStoreTest:=20collapse=20the=20t?= =?UTF-8?q?wo=20`newStore()`=20overloads=20into=20=20=20one=20with=20a=20d?= =?UTF-8?q?efaulted=20`now:=20()=20->=20Long`=20parameter.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../scheduledposts/ScheduledPostStore.kt | 4 ++ .../drawer/AccountSwitchBottomSheet.kt | 3 ++ .../ui/navigation/drawer/DrawerContent.kt | 16 +++--- .../scheduledposts/ScheduledPostsScreen.kt | 53 +++++++++---------- .../scheduledposts/ScheduledPostsViewModel.kt | 19 ++----- .../scheduledposts/ScheduledPostStoreTest.kt | 4 +- 6 files changed, 43 insertions(+), 56 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/scheduledposts/ScheduledPostStore.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/scheduledposts/ScheduledPostStore.kt index 285286826..325ad10ab 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/scheduledposts/ScheduledPostStore.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/scheduledposts/ScheduledPostStore.kt @@ -220,6 +220,10 @@ class ScheduledPostStore( private fun purgeStale(now: Long): Boolean { val before = posts.size posts.removeAll { post -> + // terminatedAtSec is the post-PR field. Legacy rows lack it; fall back to + // lastAttemptAtSec (set at SENT) and finally createdAtSec. The fallback + // can purge old CANCELLED rows up to 30d earlier than intended on first + // run after upgrade — self-healing once new rows are written. val age = now - (post.terminatedAtSec ?: post.lastAttemptAtSec ?: post.createdAtSec) when (post.status) { ScheduledPostStatus.SENT -> age > SENT_RETENTION_SEC diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/drawer/AccountSwitchBottomSheet.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/drawer/AccountSwitchBottomSheet.kt index d0b92f582..cd818fcd8 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/drawer/AccountSwitchBottomSheet.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/drawer/AccountSwitchBottomSheet.kt @@ -303,6 +303,9 @@ private fun LogoutButton( // tap and the cleanup completing. val confirmedCount = unpublishedCount logoutDialog = false + // Guard against a malformed npub: skip the Toast so we don't + // claim "Logged out" when logOff's coroutine bails early. + if (accountHex == null) return@TextButton accountSessionManager.logOff(acc) val toastMessage = if (confirmedCount > 0) { diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/drawer/DrawerContent.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/drawer/DrawerContent.kt index 049b2ef6e..a4ba39705 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/drawer/DrawerContent.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/drawer/DrawerContent.kt @@ -49,6 +49,7 @@ import androidx.compose.foundation.shape.RoundedCornerShape import androidx.compose.foundation.text.KeyboardActions import androidx.compose.foundation.text.KeyboardOptions import androidx.compose.foundation.verticalScroll +import androidx.compose.material3.Badge import androidx.compose.material3.HorizontalDivider import androidx.compose.material3.Icon import androidx.compose.material3.IconButton @@ -84,6 +85,7 @@ import androidx.compose.ui.unit.dp import androidx.compose.ui.unit.sp import androidx.lifecycle.compose.collectAsStateWithLifecycle import coil3.compose.AsyncImage +import com.vitorpamplona.amethyst.Amethyst import com.vitorpamplona.amethyst.BuildConfig import com.vitorpamplona.amethyst.R import com.vitorpamplona.amethyst.commons.icons.symbols.Icon @@ -97,6 +99,7 @@ import com.vitorpamplona.amethyst.service.relayClient.reqCommand.event.observeNo import com.vitorpamplona.amethyst.service.relayClient.reqCommand.user.observeUserContactCardsFollowerCount import com.vitorpamplona.amethyst.service.relayClient.reqCommand.user.observeUserInfo import com.vitorpamplona.amethyst.service.relayClient.reqCommand.user.observeUserStatuses +import com.vitorpamplona.amethyst.service.scheduledposts.ScheduledPostStatus import com.vitorpamplona.amethyst.ui.components.CreateTextWithEmoji import com.vitorpamplona.amethyst.ui.components.RobohashFallbackAsyncImage import com.vitorpamplona.amethyst.ui.navigation.bottombars.DrawerFeedsItems @@ -622,17 +625,16 @@ private fun ScheduledPostsNavigationRow( nav: INav, ) { val accountHex = accountViewModel.account.signer.pubKey - val allPosts by com.vitorpamplona.amethyst.Amethyst - .instance.scheduledPostStore.flow + val allPosts by Amethyst.instance.scheduledPostStore.flow .collectAsStateWithLifecycle() val pendingCount by remember(accountHex) { derivedStateOf { allPosts.count { it.accountPubkey == accountHex && ( - it.status == com.vitorpamplona.amethyst.service.scheduledposts.ScheduledPostStatus.PENDING || - it.status == com.vitorpamplona.amethyst.service.scheduledposts.ScheduledPostStatus.PUBLISHING || - it.status == com.vitorpamplona.amethyst.service.scheduledposts.ScheduledPostStatus.FAILED + it.status == ScheduledPostStatus.PENDING || + it.status == ScheduledPostStatus.PUBLISHING || + it.status == ScheduledPostStatus.FAILED ) } } @@ -680,9 +682,7 @@ private fun IconRowWithBadge( fontSize = Font18SP, ) if (badgeCount > 0) { - androidx.compose.material3.Badge { - Text(badgeCount.toString()) - } + Badge { Text(badgeCount.toString()) } } } } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/scheduledposts/ScheduledPostsScreen.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/scheduledposts/ScheduledPostsScreen.kt index fdfc50fb1..6022cc3d3 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/scheduledposts/ScheduledPostsScreen.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/scheduledposts/ScheduledPostsScreen.kt @@ -75,10 +75,11 @@ import com.vitorpamplona.amethyst.ui.screen.loggedIn.AccountViewModel import com.vitorpamplona.amethyst.ui.stringRes import com.vitorpamplona.quartz.nip01Core.core.Event import kotlinx.coroutines.delay -import java.text.DateFormat +import java.time.Instant import java.time.LocalDate import java.time.ZoneId -import java.util.Date +import java.time.format.DateTimeFormatter +import java.time.format.FormatStyle @OptIn(ExperimentalMaterial3Api::class, ExperimentalFoundationApi::class) @Composable @@ -91,7 +92,6 @@ fun ScheduledPostsScreen( viewModel(key = "scheduled-posts-$accountPubkey") { ScheduledPostsViewModel.create(accountPubkey) } - val posts by viewModel.posts.collectAsStateWithLifecycle() val groups by viewModel.groupedPosts.collectAsStateWithLifecycle() val context = LocalContext.current @@ -116,17 +116,18 @@ fun ScheduledPostsScreen( ) }, ) { padding -> - if (posts.isEmpty()) { + if (groups.isEmpty()) { EmptyState(modifier = Modifier.padding(padding)) } else { + val today = remember(nowSec) { LocalDate.now(ZoneId.systemDefault()) } LazyColumn( modifier = Modifier.fillMaxSize().padding(padding), contentPadding = PaddingValues(vertical = 8.dp), verticalArrangement = Arrangement.spacedBy(8.dp), ) { groups.forEach { group -> - stickyHeader(key = "header-${group.day}") { - DayHeader(group.day, context) + stickyHeader(key = group.day) { + DayHeader(group.day, today, context) } items(group.posts, key = { it.id }) { post -> SwipeToDeleteWithConfirmation( @@ -227,6 +228,7 @@ private fun ScheduledPostRow( @Composable private fun DayHeader( day: LocalDate, + today: LocalDate, context: android.content.Context, ) { Surface( @@ -234,7 +236,7 @@ private fun DayHeader( modifier = Modifier.fillMaxWidth(), ) { Text( - text = formatDayHeader(day, context), + text = formatDayHeader(day, today, context), style = MaterialTheme.typography.titleSmall, fontWeight = FontWeight.SemiBold, color = MaterialTheme.colorScheme.onSurfaceVariant, @@ -328,13 +330,20 @@ private fun extractPreview(post: ScheduledPost): String = .trim() }.getOrDefault("") +private val shortTimeFormatter: DateTimeFormatter = DateTimeFormatter.ofLocalizedTime(FormatStyle.SHORT) +private val fullDateFormatter: DateTimeFormatter = DateTimeFormatter.ofLocalizedDate(FormatStyle.FULL) + private fun formatAtTime( publishAtSec: Long, nowSec: Long, context: android.content.Context, ): String { - val timeFormat = DateFormat.getTimeInstance(DateFormat.SHORT) - val absolute = timeFormat.format(Date(publishAtSec * 1000)) + val absolute = + Instant + .ofEpochSecond(publishAtSec) + .atZone(ZoneId.systemDefault()) + .toLocalTime() + .format(shortTimeFormatter) return if (publishAtSec > nowSec) { stringRes(context, R.string.scheduled_posts_at_time, absolute, timeAheadNoDot(publishAtSec, context)) } else { @@ -344,25 +353,11 @@ private fun formatAtTime( private fun formatDayHeader( day: LocalDate, + today: LocalDate, context: android.content.Context, -): String { - val today = LocalDate.now(ZoneId.systemDefault()) - return when (day) { - today -> { - stringRes(context, R.string.scheduled_posts_day_today) - } - - today.plusDays(1) -> { - stringRes(context, R.string.scheduled_posts_day_tomorrow) - } - - else -> { - val fullFormat = DateFormat.getDateInstance(DateFormat.FULL) - fullFormat.format( - Date.from( - day.atStartOfDay(ZoneId.systemDefault()).toInstant(), - ), - ) - } +): String = + when (day) { + today -> stringRes(context, R.string.scheduled_posts_day_today) + today.plusDays(1) -> stringRes(context, R.string.scheduled_posts_day_tomorrow) + else -> day.format(fullDateFormatter) } -} diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/scheduledposts/ScheduledPostsViewModel.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/scheduledposts/ScheduledPostsViewModel.kt index e0aa58998..8855a5ca1 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/scheduledposts/ScheduledPostsViewModel.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/scheduledposts/ScheduledPostsViewModel.kt @@ -59,27 +59,14 @@ class ScheduledPostsViewModel( ScheduledPostStatus.FAILED, ) - val posts: StateFlow> = + /** Posts for [accountPubkey] in active statuses, grouped by local-day, sorted ascending. */ + val groupedPosts: StateFlow> = store.flow .map { all -> + val zone = ZoneId.systemDefault() all .filter { it.accountPubkey == accountPubkey && it.status in activeStatuses } .sortedBy { it.publishAtSec } - }.stateIn( - scope = viewModelScope, - started = SharingStarted.WhileSubscribed(5_000), - initialValue = emptyList(), - ) - - /** - * Posts grouped by local-day, sorted ascending. The UI uses this as the - * source for sticky-header sections. - */ - val groupedPosts: StateFlow> = - posts - .map { sorted -> - val zone = ZoneId.systemDefault() - sorted .groupBy { Instant.ofEpochSecond(it.publishAtSec).atZone(zone).toLocalDate() } .map { (day, list) -> ScheduledPostDayGroup(day, list) } .sortedBy { it.day } diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/scheduledposts/ScheduledPostStoreTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/scheduledposts/ScheduledPostStoreTest.kt index b0947747b..ea6e0a09a 100644 --- a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/scheduledposts/ScheduledPostStoreTest.kt +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/scheduledposts/ScheduledPostStoreTest.kt @@ -44,9 +44,7 @@ class ScheduledPostStoreTest { file = File(temp.root, "scheduled_posts.json") } - private fun newStore() = ScheduledPostStore(file) - - private fun newStore(now: () -> Long) = ScheduledPostStore(file, now) + private fun newStore(now: () -> Long = { System.currentTimeMillis() / 1000 }) = ScheduledPostStore(file, now) private fun samplePost( id: String = "id-1",