Code review:

- Switch list-screen formatters from java.text.DateFormat to
  java.time.format.DateTimeFormatter. DateFormat / SimpleDateFormat
  are documented not-thread-safe; the file-scope singletons are at
  risk under any future multi-threaded recomposition path.
  DateTimeFormatter is immutable and thread-safe.
- LogoutButton: when decodePublicKeyAsHexOrNull fails, bail before
  the Toast fires so we don't claim "Logged out" while logOff's
  coroutine has aborted its cleanup. Theoretical case (npub from
  prefs is well-formed in practice) but the previous flow was
  misleading on the failure path.
- ScheduledPostStore.purgeStale: document the
  terminatedAtSec ?: lastAttemptAtSec ?: createdAtSec fallback so
  the legacy-row migration semantics are clear from the source.
- DrawerContent: drop fully-qualified inline references, add proper
  imports for Amethyst, ScheduledPostStatus, and Material3 Badge.
- ScheduledPostsScreen: drop the redundant `posts` collection — derive
  the empty-state branch from `groups` directly. Use `group.day` as the
  sticky-header key instead of an interpolated string. Cache the SHORT
  time and FULL date `DateFormat` instances at file scope (matches the
  pattern in TimeAgoFormatter). Pass `today` into `DayHeader` so we
  don't call `LocalDate.now()` per recomposition.
- ScheduledPostsViewModel: drop the intermediate public `posts`
  StateFlow now that no consumer needs it; fold the filter+sort into
  the `groupedPosts` chain.
- ScheduledPostStoreTest: collapse the two `newStore()` overloads into
  one with a defaulted `now: () -> Long` parameter.
This commit is contained in:
davotoula
2026-05-07 15:14:30 +02:00
parent 0bbbdc2f65
commit a3024d855d
6 changed files with 43 additions and 56 deletions
@@ -220,6 +220,10 @@ class ScheduledPostStore(
private fun purgeStale(now: Long): Boolean { private fun purgeStale(now: Long): Boolean {
val before = posts.size val before = posts.size
posts.removeAll { post -> 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) val age = now - (post.terminatedAtSec ?: post.lastAttemptAtSec ?: post.createdAtSec)
when (post.status) { when (post.status) {
ScheduledPostStatus.SENT -> age > SENT_RETENTION_SEC ScheduledPostStatus.SENT -> age > SENT_RETENTION_SEC
@@ -303,6 +303,9 @@ private fun LogoutButton(
// tap and the cleanup completing. // tap and the cleanup completing.
val confirmedCount = unpublishedCount val confirmedCount = unpublishedCount
logoutDialog = false 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) accountSessionManager.logOff(acc)
val toastMessage = val toastMessage =
if (confirmedCount > 0) { if (confirmedCount > 0) {
@@ -49,6 +49,7 @@ import androidx.compose.foundation.shape.RoundedCornerShape
import androidx.compose.foundation.text.KeyboardActions import androidx.compose.foundation.text.KeyboardActions
import androidx.compose.foundation.text.KeyboardOptions import androidx.compose.foundation.text.KeyboardOptions
import androidx.compose.foundation.verticalScroll import androidx.compose.foundation.verticalScroll
import androidx.compose.material3.Badge
import androidx.compose.material3.HorizontalDivider import androidx.compose.material3.HorizontalDivider
import androidx.compose.material3.Icon import androidx.compose.material3.Icon
import androidx.compose.material3.IconButton import androidx.compose.material3.IconButton
@@ -84,6 +85,7 @@ import androidx.compose.ui.unit.dp
import androidx.compose.ui.unit.sp import androidx.compose.ui.unit.sp
import androidx.lifecycle.compose.collectAsStateWithLifecycle import androidx.lifecycle.compose.collectAsStateWithLifecycle
import coil3.compose.AsyncImage import coil3.compose.AsyncImage
import com.vitorpamplona.amethyst.Amethyst
import com.vitorpamplona.amethyst.BuildConfig import com.vitorpamplona.amethyst.BuildConfig
import com.vitorpamplona.amethyst.R import com.vitorpamplona.amethyst.R
import com.vitorpamplona.amethyst.commons.icons.symbols.Icon 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.observeUserContactCardsFollowerCount
import com.vitorpamplona.amethyst.service.relayClient.reqCommand.user.observeUserInfo import com.vitorpamplona.amethyst.service.relayClient.reqCommand.user.observeUserInfo
import com.vitorpamplona.amethyst.service.relayClient.reqCommand.user.observeUserStatuses 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.CreateTextWithEmoji
import com.vitorpamplona.amethyst.ui.components.RobohashFallbackAsyncImage import com.vitorpamplona.amethyst.ui.components.RobohashFallbackAsyncImage
import com.vitorpamplona.amethyst.ui.navigation.bottombars.DrawerFeedsItems import com.vitorpamplona.amethyst.ui.navigation.bottombars.DrawerFeedsItems
@@ -622,17 +625,16 @@ private fun ScheduledPostsNavigationRow(
nav: INav, nav: INav,
) { ) {
val accountHex = accountViewModel.account.signer.pubKey val accountHex = accountViewModel.account.signer.pubKey
val allPosts by com.vitorpamplona.amethyst.Amethyst val allPosts by Amethyst.instance.scheduledPostStore.flow
.instance.scheduledPostStore.flow
.collectAsStateWithLifecycle() .collectAsStateWithLifecycle()
val pendingCount by remember(accountHex) { val pendingCount by remember(accountHex) {
derivedStateOf { derivedStateOf {
allPosts.count { allPosts.count {
it.accountPubkey == accountHex && it.accountPubkey == accountHex &&
( (
it.status == com.vitorpamplona.amethyst.service.scheduledposts.ScheduledPostStatus.PENDING || it.status == ScheduledPostStatus.PENDING ||
it.status == com.vitorpamplona.amethyst.service.scheduledposts.ScheduledPostStatus.PUBLISHING || it.status == ScheduledPostStatus.PUBLISHING ||
it.status == com.vitorpamplona.amethyst.service.scheduledposts.ScheduledPostStatus.FAILED it.status == ScheduledPostStatus.FAILED
) )
} }
} }
@@ -680,9 +682,7 @@ private fun IconRowWithBadge(
fontSize = Font18SP, fontSize = Font18SP,
) )
if (badgeCount > 0) { if (badgeCount > 0) {
androidx.compose.material3.Badge { Badge { Text(badgeCount.toString()) }
Text(badgeCount.toString())
}
} }
} }
} }
@@ -75,10 +75,11 @@ import com.vitorpamplona.amethyst.ui.screen.loggedIn.AccountViewModel
import com.vitorpamplona.amethyst.ui.stringRes import com.vitorpamplona.amethyst.ui.stringRes
import com.vitorpamplona.quartz.nip01Core.core.Event import com.vitorpamplona.quartz.nip01Core.core.Event
import kotlinx.coroutines.delay import kotlinx.coroutines.delay
import java.text.DateFormat import java.time.Instant
import java.time.LocalDate import java.time.LocalDate
import java.time.ZoneId import java.time.ZoneId
import java.util.Date import java.time.format.DateTimeFormatter
import java.time.format.FormatStyle
@OptIn(ExperimentalMaterial3Api::class, ExperimentalFoundationApi::class) @OptIn(ExperimentalMaterial3Api::class, ExperimentalFoundationApi::class)
@Composable @Composable
@@ -91,7 +92,6 @@ fun ScheduledPostsScreen(
viewModel(key = "scheduled-posts-$accountPubkey") { viewModel(key = "scheduled-posts-$accountPubkey") {
ScheduledPostsViewModel.create(accountPubkey) ScheduledPostsViewModel.create(accountPubkey)
} }
val posts by viewModel.posts.collectAsStateWithLifecycle()
val groups by viewModel.groupedPosts.collectAsStateWithLifecycle() val groups by viewModel.groupedPosts.collectAsStateWithLifecycle()
val context = LocalContext.current val context = LocalContext.current
@@ -116,17 +116,18 @@ fun ScheduledPostsScreen(
) )
}, },
) { padding -> ) { padding ->
if (posts.isEmpty()) { if (groups.isEmpty()) {
EmptyState(modifier = Modifier.padding(padding)) EmptyState(modifier = Modifier.padding(padding))
} else { } else {
val today = remember(nowSec) { LocalDate.now(ZoneId.systemDefault()) }
LazyColumn( LazyColumn(
modifier = Modifier.fillMaxSize().padding(padding), modifier = Modifier.fillMaxSize().padding(padding),
contentPadding = PaddingValues(vertical = 8.dp), contentPadding = PaddingValues(vertical = 8.dp),
verticalArrangement = Arrangement.spacedBy(8.dp), verticalArrangement = Arrangement.spacedBy(8.dp),
) { ) {
groups.forEach { group -> groups.forEach { group ->
stickyHeader(key = "header-${group.day}") { stickyHeader(key = group.day) {
DayHeader(group.day, context) DayHeader(group.day, today, context)
} }
items(group.posts, key = { it.id }) { post -> items(group.posts, key = { it.id }) { post ->
SwipeToDeleteWithConfirmation( SwipeToDeleteWithConfirmation(
@@ -227,6 +228,7 @@ private fun ScheduledPostRow(
@Composable @Composable
private fun DayHeader( private fun DayHeader(
day: LocalDate, day: LocalDate,
today: LocalDate,
context: android.content.Context, context: android.content.Context,
) { ) {
Surface( Surface(
@@ -234,7 +236,7 @@ private fun DayHeader(
modifier = Modifier.fillMaxWidth(), modifier = Modifier.fillMaxWidth(),
) { ) {
Text( Text(
text = formatDayHeader(day, context), text = formatDayHeader(day, today, context),
style = MaterialTheme.typography.titleSmall, style = MaterialTheme.typography.titleSmall,
fontWeight = FontWeight.SemiBold, fontWeight = FontWeight.SemiBold,
color = MaterialTheme.colorScheme.onSurfaceVariant, color = MaterialTheme.colorScheme.onSurfaceVariant,
@@ -328,13 +330,20 @@ private fun extractPreview(post: ScheduledPost): String =
.trim() .trim()
}.getOrDefault("") }.getOrDefault("")
private val shortTimeFormatter: DateTimeFormatter = DateTimeFormatter.ofLocalizedTime(FormatStyle.SHORT)
private val fullDateFormatter: DateTimeFormatter = DateTimeFormatter.ofLocalizedDate(FormatStyle.FULL)
private fun formatAtTime( private fun formatAtTime(
publishAtSec: Long, publishAtSec: Long,
nowSec: Long, nowSec: Long,
context: android.content.Context, context: android.content.Context,
): String { ): String {
val timeFormat = DateFormat.getTimeInstance(DateFormat.SHORT) val absolute =
val absolute = timeFormat.format(Date(publishAtSec * 1000)) Instant
.ofEpochSecond(publishAtSec)
.atZone(ZoneId.systemDefault())
.toLocalTime()
.format(shortTimeFormatter)
return if (publishAtSec > nowSec) { return if (publishAtSec > nowSec) {
stringRes(context, R.string.scheduled_posts_at_time, absolute, timeAheadNoDot(publishAtSec, context)) stringRes(context, R.string.scheduled_posts_at_time, absolute, timeAheadNoDot(publishAtSec, context))
} else { } else {
@@ -344,25 +353,11 @@ private fun formatAtTime(
private fun formatDayHeader( private fun formatDayHeader(
day: LocalDate, day: LocalDate,
today: LocalDate,
context: android.content.Context, context: android.content.Context,
): String { ): String =
val today = LocalDate.now(ZoneId.systemDefault()) when (day) {
return when (day) { today -> stringRes(context, R.string.scheduled_posts_day_today)
today -> { today.plusDays(1) -> stringRes(context, R.string.scheduled_posts_day_tomorrow)
stringRes(context, R.string.scheduled_posts_day_today) else -> day.format(fullDateFormatter)
}
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(),
),
)
}
} }
}
@@ -59,27 +59,14 @@ class ScheduledPostsViewModel(
ScheduledPostStatus.FAILED, ScheduledPostStatus.FAILED,
) )
val posts: StateFlow<List<ScheduledPost>> = /** Posts for [accountPubkey] in active statuses, grouped by local-day, sorted ascending. */
val groupedPosts: StateFlow<List<ScheduledPostDayGroup>> =
store.flow store.flow
.map { all -> .map { all ->
val zone = ZoneId.systemDefault()
all all
.filter { it.accountPubkey == accountPubkey && it.status in activeStatuses } .filter { it.accountPubkey == accountPubkey && it.status in activeStatuses }
.sortedBy { it.publishAtSec } .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<List<ScheduledPostDayGroup>> =
posts
.map { sorted ->
val zone = ZoneId.systemDefault()
sorted
.groupBy { Instant.ofEpochSecond(it.publishAtSec).atZone(zone).toLocalDate() } .groupBy { Instant.ofEpochSecond(it.publishAtSec).atZone(zone).toLocalDate() }
.map { (day, list) -> ScheduledPostDayGroup(day, list) } .map { (day, list) -> ScheduledPostDayGroup(day, list) }
.sortedBy { it.day } .sortedBy { it.day }
@@ -44,9 +44,7 @@ class ScheduledPostStoreTest {
file = File(temp.root, "scheduled_posts.json") file = File(temp.root, "scheduled_posts.json")
} }
private fun newStore() = ScheduledPostStore(file) private fun newStore(now: () -> Long = { System.currentTimeMillis() / 1000 }) = ScheduledPostStore(file, now)
private fun newStore(now: () -> Long) = ScheduledPostStore(file, now)
private fun samplePost( private fun samplePost(
id: String = "id-1", id: String = "id-1",