Code review

- catch CancellationException explicitly
- document the deliberate tradeoff of holding the data mutex across the disk write in persist()
- migrate hardcoded strings to R.string.*
- Store.mutate: use value equality (==) instead of reference (===)
- Store.persist: drop the redundant parent.exists() check
- ViewModel: drop the Context parameter from publishNow(id)
- Screen: memoize extractPreview via remember(post.id) so JSON scanning doesn't run on every recomposition of a row.
- Screen: replace hardcoded Color(0xFF...) literals
- Screen.formatPublishMoment: delegate the past-tense branch to the existing timeAgoNoDot() helper instead of hand-rolled TimeUnit math.
This commit is contained in:
davotoula
2026-05-07 12:40:33 +02:00
parent 8e5e3c634a
commit 214a35d620
8 changed files with 128 additions and 110 deletions
@@ -168,7 +168,7 @@ class ScheduledPostStore(
if (idx < 0) return false
val before = posts[idx]
val after = transform(before)
if (after === before) return false
if (after == before) return false
posts[idx] = after
return true
}
@@ -190,11 +190,21 @@ class ScheduledPostStore(
_flow.value = posts.toList()
}
/**
* Writes the snapshot to disk *while holding the data mutex*. This is a
* deliberate tradeoff: moving the write outside the lock would require a
* separate write-mutex (or a sequence number) to preserve write ordering
* across concurrent mutations — otherwise an older snapshot can clobber a
* newer one if the OS schedules the second write to finish first. For a
* file that's a few KB and a single-process owner with infrequent writes,
* holding the mutex across the rename is the simpler and correct choice.
* Revisit if the store ever grows past a hundred rows or starts seeing
* concurrent multi-writer pressure.
*/
private fun persist() {
val snapshot = posts.toList()
_flow.value = snapshot
val parent = storageFile.parentFile
if (parent != null && !parent.exists()) parent.mkdirs()
storageFile.parentFile?.mkdirs()
val tmp = File(storageFile.parentFile, storageFile.name + ".tmp")
try {
mapper.writeValue(tmp, ScheduledPostFile(version = 1, posts = snapshot))
@@ -34,6 +34,7 @@ import com.vitorpamplona.amethyst.Amethyst
import com.vitorpamplona.quartz.nip01Core.core.Event
import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl
import com.vitorpamplona.quartz.utils.Log
import kotlinx.coroutines.CancellationException
import java.util.concurrent.TimeUnit
/**
@@ -152,6 +153,8 @@ class ScheduledPostWorker(
store.markSent(post.id)
Log.d(TAG) { "client.publish(${post.id}) done; marked SENT" }
} catch (e: CancellationException) {
throw e
} catch (e: Exception) {
Log.e(TAG, "Failed to publish scheduled post ${post.id}", e)
store.markFailed(post.id, e.message)
@@ -160,6 +163,8 @@ class ScheduledPostWorker(
Log.d(TAG) { "doWork() EXIT success" }
Result.success()
} catch (e: CancellationException) {
throw e
} catch (e: Exception) {
Log.e(TAG, "doWork() unexpected failure", e)
Result.retry()
@@ -25,22 +25,26 @@ import androidx.compose.material3.IconButton
import androidx.compose.material3.MaterialTheme
import androidx.compose.runtime.Composable
import androidx.compose.ui.Modifier
import androidx.compose.ui.graphics.Color
import androidx.compose.ui.unit.dp
import com.vitorpamplona.amethyst.R
import com.vitorpamplona.amethyst.commons.icons.symbols.Icon
import com.vitorpamplona.amethyst.commons.icons.symbols.MaterialSymbols
import com.vitorpamplona.amethyst.ui.stringRes
@Composable
fun ScheduleAtButton(
isActive: Boolean,
onClick: () -> Unit,
) {
IconButton(onClick = { onClick() }) {
IconButton(onClick = onClick) {
Icon(
symbol = MaterialSymbols.Schedule,
contentDescription = if (isActive) "Cancel scheduling" else "Schedule post",
contentDescription =
stringRes(
if (isActive) R.string.schedule_post_button_remove else R.string.schedule_post_button_add,
),
modifier = Modifier.size(20.dp),
tint = if (isActive) Color(0xFF1E88E5) else MaterialTheme.colorScheme.onBackground,
tint = if (isActive) MaterialTheme.colorScheme.primary else MaterialTheme.colorScheme.onBackground,
)
}
}
@@ -20,6 +20,7 @@
*/
package com.vitorpamplona.amethyst.ui.note.creators.scheduling
import android.text.format.DateFormat
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.Row
import androidx.compose.foundation.layout.Spacer
@@ -49,14 +50,15 @@ import androidx.compose.runtime.remember
import androidx.compose.runtime.setValue
import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier
import androidx.compose.ui.graphics.Color
import androidx.compose.ui.platform.LocalContext
import androidx.compose.ui.text.font.FontWeight
import androidx.compose.ui.unit.dp
import androidx.compose.ui.unit.sp
import com.vitorpamplona.amethyst.R
import com.vitorpamplona.amethyst.commons.icons.symbols.Icon
import com.vitorpamplona.amethyst.commons.icons.symbols.MaterialSymbols
import com.vitorpamplona.amethyst.ui.note.timeAheadNoDot
import com.vitorpamplona.amethyst.ui.stringRes
import com.vitorpamplona.amethyst.ui.theme.DividerThickness
import com.vitorpamplona.amethyst.ui.theme.placeholderText
import com.vitorpamplona.quartz.utils.TimeUtils
@@ -98,15 +100,15 @@ fun ScheduleAtPicker(
},
)
val context = LocalContext.current
val timePickerState =
rememberTimePickerState(
initialHour = currentTime.hour,
initialMinute = currentTime.minute,
is24Hour = false,
is24Hour = DateFormat.is24HourFormat(context),
)
val context = LocalContext.current
Column(Modifier.fillMaxWidth()) {
Row(
verticalAlignment = Alignment.CenterVertically,
@@ -117,13 +119,13 @@ fun ScheduleAtPicker(
) {
Icon(
symbol = MaterialSymbols.Timer,
contentDescription = "Scheduled time",
contentDescription = stringRes(R.string.schedule_post_time_label),
modifier = Modifier.size(20.dp),
tint = Color(0xFF1E88E5),
tint = MaterialTheme.colorScheme.primary,
)
Text(
text = "Schedule",
text = stringRes(R.string.schedule_post),
fontSize = 20.sp,
fontWeight = FontWeight.W500,
modifier = Modifier.padding(start = 10.dp),
@@ -133,7 +135,7 @@ fun ScheduleAtPicker(
HorizontalDivider(thickness = DividerThickness)
Text(
text = "Posts publish within ~15 minutes of the scheduled time.",
text = stringRes(R.string.schedule_post_helper),
color = MaterialTheme.colorScheme.placeholderText,
modifier = Modifier.padding(vertical = 10.dp),
)
@@ -150,14 +152,14 @@ fun ScheduleAtPicker(
modifier = Modifier.padding(16.dp),
verticalAlignment = Alignment.CenterVertically,
) {
Icon(MaterialSymbols.Timer, contentDescription = "Pick scheduled time")
Icon(MaterialSymbols.Timer, contentDescription = stringRes(R.string.schedule_post_pick_time))
Spacer(Modifier.width(12.dp))
if (scheduledForSec < TimeUtils.oneMinuteFromNow()) {
Text("Schedule for…", style = MaterialTheme.typography.bodyLarge)
Text(stringRes(R.string.schedule_post_pick_label), style = MaterialTheme.typography.bodyLarge)
} else {
Text(
text = "Publishes in ${timeAheadNoDot(scheduledForSec, context)}",
text = stringRes(R.string.schedule_post_publishes_in, timeAheadNoDot(scheduledForSec, context)),
style = MaterialTheme.typography.bodyLarge,
)
}
@@ -172,7 +174,7 @@ fun ScheduleAtPicker(
TextButton(onClick = {
showDatePicker = false
showTimePicker = true
}) { Text("Next") }
}) { Text(stringRes(R.string.next)) }
},
) {
DatePicker(state = datePickerState)
@@ -181,7 +183,7 @@ fun ScheduleAtPicker(
if (showTimePicker) {
TimePickerDialog(
title = { Text("Time") },
title = { Text(stringRes(R.string.schedule_post_picker_time_title)) },
onDismissRequest = { showTimePicker = false },
confirmButton = {
TextButton(
@@ -199,7 +201,7 @@ fun ScheduleAtPicker(
onChanged(roundUpToNextQuarterHour(rawSec))
showTimePicker = false
},
) { Text("Confirm") }
) { Text(stringRes(R.string.confirm)) }
},
) {
TimePicker(state = timePickerState)
@@ -225,7 +227,7 @@ private fun ReliabilityWarning(hasMultipleAccounts: Boolean) {
tint = MaterialTheme.colorScheme.onErrorContainer,
)
Text(
text = "Always-on notifications disabled",
text = stringRes(R.string.schedule_post_warning_title),
fontWeight = FontWeight.Bold,
color = MaterialTheme.colorScheme.onErrorContainer,
modifier = Modifier.padding(start = 8.dp),
@@ -233,11 +235,13 @@ private fun ReliabilityWarning(hasMultipleAccounts: Boolean) {
}
Text(
text =
if (hasMultipleAccounts) {
"Scheduled posts may not publish until you reopen the app. Other accounts' scheduled posts won't fire while this account is active. Enable always-on in Settings → UI Preferences for reliable background scheduling."
} else {
"Scheduled posts may not publish until you next reopen the app. Enable always-on in Settings → UI Preferences for reliable background scheduling."
},
stringRes(
if (hasMultipleAccounts) {
R.string.schedule_post_warning_multi
} else {
R.string.schedule_post_warning_single
},
),
color = MaterialTheme.colorScheme.onErrorContainer,
modifier = Modifier.padding(top = 6.dp),
)
@@ -409,13 +409,13 @@ private fun NewPostScreenBody(
}
}
val alwaysOnEnabled by accountViewModel.account.settings.alwaysOnNotificationService
.collectAsStateWithLifecycle()
val savedAccounts by com.vitorpamplona.amethyst.LocalPreferences
.accountsFlow()
.collectAsStateWithLifecycle()
val hasMultipleAccounts = (savedAccounts?.size ?: 0) > 1
postViewModel.scheduledForSec?.let { current ->
val alwaysOnEnabled by accountViewModel.account.settings.alwaysOnNotificationService
.collectAsStateWithLifecycle()
val savedAccounts by com.vitorpamplona.amethyst.LocalPreferences
.accountsFlow()
.collectAsStateWithLifecycle()
val hasMultipleAccounts = (savedAccounts?.size ?: 0) > 1
Row(
verticalAlignment = CenterVertically,
modifier = Modifier.padding(vertical = Size10dp, horizontal = Size10dp),
@@ -49,23 +49,26 @@ import androidx.compose.runtime.remember
import androidx.compose.runtime.setValue
import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier
import androidx.compose.ui.graphics.Color
import androidx.compose.ui.platform.LocalContext
import androidx.compose.ui.text.font.FontWeight
import androidx.compose.ui.text.style.TextOverflow
import androidx.compose.ui.unit.dp
import androidx.lifecycle.compose.collectAsStateWithLifecycle
import androidx.lifecycle.viewmodel.compose.viewModel
import com.vitorpamplona.amethyst.R
import com.vitorpamplona.amethyst.commons.icons.symbols.Icon
import com.vitorpamplona.amethyst.commons.icons.symbols.MaterialSymbols
import com.vitorpamplona.amethyst.service.scheduledposts.ScheduledPost
import com.vitorpamplona.amethyst.service.scheduledposts.ScheduledPostStatus
import com.vitorpamplona.amethyst.service.scheduledposts.ScheduledPostWorker
import com.vitorpamplona.amethyst.ui.navigation.navs.INav
import com.vitorpamplona.amethyst.ui.navigation.topbars.ShorterTopAppBar
import com.vitorpamplona.amethyst.ui.note.ArrowBackIcon
import com.vitorpamplona.amethyst.ui.note.timeAgoNoDot
import com.vitorpamplona.amethyst.ui.note.timeAheadNoDot
import com.vitorpamplona.amethyst.ui.screen.loggedIn.AccountViewModel
import java.util.concurrent.TimeUnit
import com.vitorpamplona.amethyst.ui.stringRes
import com.vitorpamplona.quartz.nip01Core.core.Event
@OptIn(ExperimentalMaterial3Api::class)
@Composable
@@ -87,7 +90,7 @@ fun ScheduledPostsScreen(
Scaffold(
topBar = {
ShorterTopAppBar(
title = { Text("Scheduled posts") },
title = { Text(stringRes(R.string.scheduled_posts)) },
navigationIcon = {
IconButton(onClick = { nav.popBack() }) { ArrowBackIcon() }
},
@@ -115,11 +118,12 @@ fun ScheduledPostsScreen(
pendingPublishId?.let { id ->
ConfirmDialog(
title = "Send now?",
message = "This post will publish to relays immediately. The original schedule will be discarded.",
confirmLabel = "Send",
title = stringRes(R.string.scheduled_posts_send_now_title),
message = stringRes(R.string.scheduled_posts_send_now_message),
confirmLabel = stringRes(R.string.scheduled_posts_send_now_confirm),
onConfirm = {
viewModel.publishNow(id, context)
viewModel.publishNow(id)
ScheduledPostWorker.scheduleCatchUp(context)
pendingPublishId = null
},
onDismiss = { pendingPublishId = null },
@@ -128,9 +132,9 @@ fun ScheduledPostsScreen(
pendingCancelId?.let { id ->
ConfirmDialog(
title = "Delete scheduled post?",
message = "The post will not be published. This cannot be undone.",
confirmLabel = "Delete",
title = stringRes(R.string.scheduled_posts_delete_title),
message = stringRes(R.string.scheduled_posts_delete_message),
confirmLabel = stringRes(R.string.scheduled_posts_delete_confirm),
destructive = true,
onConfirm = {
viewModel.cancel(id)
@@ -148,6 +152,7 @@ private fun ScheduledPostRow(
onCancel: () -> Unit,
) {
val context = LocalContext.current
val preview = remember(post) { extractPreview(post) }
Card(
modifier = Modifier.fillMaxWidth(),
colors = CardDefaults.outlinedCardColors(),
@@ -171,7 +176,7 @@ private fun ScheduledPostRow(
}
Text(
text = extractPreview(post),
text = preview,
style = MaterialTheme.typography.bodyMedium,
maxLines = 3,
overflow = TextOverflow.Ellipsis,
@@ -179,7 +184,7 @@ private fun ScheduledPostRow(
if (post.status == ScheduledPostStatus.FAILED && post.lastError != null) {
Text(
text = "Error: ${post.lastError}",
text = stringRes(R.string.scheduled_posts_error_prefix, post.lastError),
style = MaterialTheme.typography.bodySmall,
color = MaterialTheme.colorScheme.error,
maxLines = 2,
@@ -194,7 +199,7 @@ private fun ScheduledPostRow(
IconButton(onClick = onCancel) {
Icon(
symbol = MaterialSymbols.Delete,
contentDescription = "Delete",
contentDescription = stringRes(R.string.scheduled_posts_action_delete),
modifier = Modifier.size(22.dp),
tint = MaterialTheme.colorScheme.error,
)
@@ -202,7 +207,7 @@ private fun ScheduledPostRow(
IconButton(onClick = onPublishNow) {
Icon(
symbol = MaterialSymbols.AutoMirrored.Send,
contentDescription = "Send now",
contentDescription = stringRes(R.string.scheduled_posts_action_send_now),
modifier = Modifier.size(22.dp),
tint = MaterialTheme.colorScheme.primary,
)
@@ -214,17 +219,17 @@ private fun ScheduledPostRow(
@Composable
private fun StatusChip(status: ScheduledPostStatus) {
val (label, tint) =
val (labelRes, tint) =
when (status) {
ScheduledPostStatus.PENDING -> "Scheduled" to Color(0xFF1E88E5)
ScheduledPostStatus.PUBLISHING -> "Sending…" to Color(0xFFFFA000)
ScheduledPostStatus.FAILED -> "Failed" to MaterialTheme.colorScheme.error
ScheduledPostStatus.SENT -> "Sent" to Color(0xFF43A047)
ScheduledPostStatus.CANCELLED -> "Cancelled" to MaterialTheme.colorScheme.onSurfaceVariant
ScheduledPostStatus.PENDING -> R.string.scheduled_posts_status_pending to MaterialTheme.colorScheme.primary
ScheduledPostStatus.PUBLISHING -> R.string.scheduled_posts_status_publishing to MaterialTheme.colorScheme.tertiary
ScheduledPostStatus.FAILED -> R.string.scheduled_posts_status_failed to MaterialTheme.colorScheme.error
ScheduledPostStatus.SENT -> R.string.scheduled_posts_status_sent to MaterialTheme.colorScheme.tertiary
ScheduledPostStatus.CANCELLED -> R.string.scheduled_posts_status_cancelled to MaterialTheme.colorScheme.onSurfaceVariant
}
AssistChip(
onClick = {},
label = { Text(label, fontWeight = FontWeight.Medium) },
label = { Text(stringRes(labelRes), fontWeight = FontWeight.Medium) },
colors =
AssistChipDefaults.assistChipColors(
labelColor = tint,
@@ -249,11 +254,11 @@ private fun EmptyState(modifier: Modifier = Modifier) {
tint = MaterialTheme.colorScheme.onSurfaceVariant,
)
Text(
text = "No scheduled posts",
text = stringRes(R.string.scheduled_posts_empty_title),
style = MaterialTheme.typography.titleMedium,
)
Text(
text = "Compose a note and tap the clock icon to schedule it for later.",
text = stringRes(R.string.scheduled_posts_empty_hint),
style = MaterialTheme.typography.bodyMedium,
color = MaterialTheme.colorScheme.onSurfaceVariant,
)
@@ -283,60 +288,28 @@ private fun ConfirmDialog(
}
},
dismissButton = {
TextButton(onClick = onDismiss) { Text("Cancel") }
TextButton(onClick = onDismiss) { Text(stringRes(R.string.cancel)) }
},
)
}
private fun extractPreview(post: ScheduledPost): String {
val json = post.signedEventJson
val needle = "\"content\":\""
val start = json.indexOf(needle)
if (start < 0) return ""
val from = start + needle.length
val sb = StringBuilder()
var i = from
while (i < json.length) {
val c = json[i]
if (c == '\\' && i + 1 < json.length) {
when (json[i + 1]) {
'n' -> sb.append('\n')
't' -> sb.append('\t')
'\\' -> sb.append('\\')
'"' -> sb.append('"')
else -> sb.append(json[i + 1])
}
i += 2
} else if (c == '"') {
break
} else {
sb.append(c)
i++
}
if (sb.length > 200) break
}
return sb.toString().trim()
}
private fun extractPreview(post: ScheduledPost): String =
runCatching {
Event
.fromJson(post.signedEventJson)
.content
.take(200)
.trim()
}.getOrDefault("")
private fun formatPublishMoment(
publishAtSec: Long,
context: android.content.Context,
): String {
val nowSec = System.currentTimeMillis() / 1000
val deltaSec = publishAtSec - nowSec
return when {
deltaSec > 0 -> {
"Publishes in ${timeAheadNoDot(publishAtSec, context)}"
}
else -> {
val ago = -deltaSec
val mins = TimeUnit.SECONDS.toMinutes(ago)
when {
mins < 1 -> "Due now"
mins < 60 -> "Was due ${mins}m ago"
else -> "Was due ${TimeUnit.SECONDS.toHours(ago)}h ago"
}
}
return if (publishAtSec > nowSec) {
stringRes(context, R.string.schedule_post_publishes_in, timeAheadNoDot(publishAtSec, context))
} else {
stringRes(context, R.string.schedule_post_was_due, timeAgoNoDot(publishAtSec, context).trim())
}
}
@@ -20,14 +20,12 @@
*/
package com.vitorpamplona.amethyst.ui.screen.loggedIn.scheduledposts
import android.content.Context
import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewModelScope
import com.vitorpamplona.amethyst.Amethyst
import com.vitorpamplona.amethyst.service.scheduledposts.ScheduledPost
import com.vitorpamplona.amethyst.service.scheduledposts.ScheduledPostStatus
import com.vitorpamplona.amethyst.service.scheduledposts.ScheduledPostStore
import com.vitorpamplona.amethyst.service.scheduledposts.ScheduledPostWorker
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.flow.SharingStarted
import kotlinx.coroutines.flow.StateFlow
@@ -70,14 +68,9 @@ class ScheduledPostsViewModel(
}
}
fun publishNow(
id: String,
context: Context,
) {
fun publishNow(id: String) {
viewModelScope.launch(Dispatchers.IO) {
if (store.publishNow(id)) {
ScheduledPostWorker.scheduleCatchUp(context)
}
store.publishNow(id)
}
}
+29
View File
@@ -429,6 +429,35 @@
<string name="migrate_bookmarks_success">Bookmarks migrated successfully</string>
<string name="drafts">Drafts</string>
<string name="scheduled_posts">Scheduled posts</string>
<string name="schedule_post">Schedule</string>
<string name="schedule_post_time_label">Scheduled time</string>
<string name="schedule_post_helper">Posts publish within ~15 minutes of the scheduled time.</string>
<string name="schedule_post_pick_time">Pick scheduled time</string>
<string name="schedule_post_pick_label">Schedule for…</string>
<string name="schedule_post_publishes_in">Publishes in %1$s</string>
<string name="schedule_post_was_due">Was due %1$s ago</string>
<string name="schedule_post_picker_time_title">Time</string>
<string name="schedule_post_button_add">Schedule post</string>
<string name="schedule_post_button_remove">Cancel scheduling</string>
<string name="schedule_post_warning_title">Always-on notifications disabled</string>
<string name="schedule_post_warning_single">Scheduled posts may not publish until you next reopen the app. Enable always-on in Settings → UI Preferences for reliable background scheduling.</string>
<string name="schedule_post_warning_multi">Scheduled posts may not publish until you reopen the app. Other accounts\' scheduled posts won\'t fire while this account is active. Enable always-on in Settings → UI Preferences for reliable background scheduling.</string>
<string name="scheduled_posts_send_now_title">Send now?</string>
<string name="scheduled_posts_send_now_message">This post will publish to relays immediately. The original schedule will be discarded.</string>
<string name="scheduled_posts_send_now_confirm">Send</string>
<string name="scheduled_posts_delete_title">Delete scheduled post?</string>
<string name="scheduled_posts_delete_message">The post will not be published. This cannot be undone.</string>
<string name="scheduled_posts_delete_confirm">Delete</string>
<string name="scheduled_posts_action_delete">Delete</string>
<string name="scheduled_posts_action_send_now">Send now</string>
<string name="scheduled_posts_empty_title">No scheduled posts</string>
<string name="scheduled_posts_empty_hint">Compose a note and tap the clock icon to schedule it for later.</string>
<string name="scheduled_posts_error_prefix">Error: %1$s</string>
<string name="scheduled_posts_status_pending">Scheduled</string>
<string name="scheduled_posts_status_publishing">Sending…</string>
<string name="scheduled_posts_status_failed">Failed</string>
<string name="scheduled_posts_status_sent">Sent</string>
<string name="scheduled_posts_status_cancelled">Cancelled</string>
<string name="polls">Polls</string>
<string name="open_polls">Open</string>
<string name="closed_polls">Closed</string>