refactor(reads): address all review findings — security, performance, simplicity

P1 Critical: - Add 300ms debounce on editor preview to prevent AST re-parse per keystroke - Hoist Regex constants in ReadingTimeCalculator and TableOfContents - Add URI scheme allowlist to editor onLinkClick (was missing vs reader) - Add MAX_CONTENT_BYTES (100KB) validation before publish P2 Important: - Delete MarkdownSpikeScreen (278 LOC spike artifact) - Delete HighlightCreator (95 LOC, zero callers — YAGNI) - Delete DraftLongTextNoteEvent (162 LOC, no consumers — YAGNI) - Replace ArticleMediaRenderer interface with onLinkClick lambda - Extract inline subscription to FilterBuilders.longFormByAddress() - Replace SimpleDateFormat with thread-safe DateTimeFormatter - Remove dead placeholders (bookmark button, reactions row, unused param) - Cache draft index in memory to avoid redundant disk reads - Add TODO for publish-before-relay-ack issue P3 Nice-to-have: - Remove timestamp from subscription subId for stability - Remove redundant slug ".." removal (regex handles it) - Add metadata field length limits (title 256, summary 1024) - Fix identical isMacOS branches in editor - Validate image URLs (http/https only) in ArticleHeader Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-03-24 07:18:42 +02:00
parent 4fcbffdb3b
commit 328fcf86d7
20 changed files with 368 additions and 419 deletions
@@ -147,6 +147,12 @@ sealed class DesktopScreen {
        val addressTag: String,
    ) : DesktopScreen()

+    data class Editor(
+        val draftSlug: String? = null,
+    ) : DesktopScreen()
+
+    data object Drafts : DesktopScreen()
+
    data object Settings : DesktopScreen()
 }

@@ -61,6 +61,7 @@ class DesktopDraftStore(
 ) {
    private val mapper = jacksonObjectMapper()
    private val mutex = Mutex()
+    private var cachedIndex: MutableMap<String, DraftMetadata>? = null

    private val _drafts = MutableStateFlow<List<DraftEntry>>(emptyList())
    val drafts: StateFlow<List<DraftEntry>> = _drafts.asStateFlow()
@@ -77,7 +78,10 @@ class DesktopDraftStore(
    private val indexFile: File get() = File(draftsDir, "index.json")

    init {
-        scope.launch(Dispatchers.IO) { loadIndex() }
+        scope.launch(Dispatchers.IO) {
+            cachedIndex = null
+            loadIndex()
+        }
    }

    /**
@@ -86,7 +90,6 @@ class DesktopDraftStore(
    private fun sanitizeSlug(slug: String): String {
        val sanitized =
            slug
-                .replace("..", "")
                .replace("/", "")
                .replace("\\", "")
                .replace("\u0000", "")
@@ -133,9 +136,10 @@ class DesktopDraftStore(
            atomicWrite(contentFile, content)

            // Update index
-            val index = loadIndexMap().toMutableMap()
+            val index = loadIndexMap()
            index[safeSlug] = metadata.copy(updatedAt = Instant.now().toString())
            atomicWriteIndex(index)
+            cachedIndex = index

            // Refresh state
            _drafts.value =
@@ -172,9 +176,10 @@ class DesktopDraftStore(
        mutex.withLock {
            File(draftsDir, "$safeSlug.md").delete()

-            val index = loadIndexMap().toMutableMap()
+            val index = loadIndexMap()
            index.remove(safeSlug)
            atomicWriteIndex(index)
+            cachedIndex = index

            _drafts.value =
                index.entries
@@ -189,10 +194,11 @@ class DesktopDraftStore(
    suspend fun markPublished(slug: String) {
        val safeSlug = sanitizeSlug(slug)
        mutex.withLock {
-            val index = loadIndexMap().toMutableMap()
+            val index = loadIndexMap()
            val existing = index[safeSlug] ?: return
            index[safeSlug] = existing.copy(published = true, updatedAt = Instant.now().toString())
            atomicWriteIndex(index)
+            cachedIndex = index

            _drafts.value =
                index.entries
@@ -201,14 +207,21 @@ class DesktopDraftStore(
        }
    }

-    private fun loadIndexMap(): Map<String, DraftMetadata> {
-        if (!indexFile.exists()) return emptyMap()
-        return try {
-            mapper.readValue(indexFile)
-        } catch (e: Exception) {
-            System.err.println("Failed to read drafts index: ${e.message}")
-            emptyMap()
-        }
+    private fun loadIndexMap(): MutableMap<String, DraftMetadata> {
+        cachedIndex?.let { return it }
+        val loaded: MutableMap<String, DraftMetadata> =
+            if (!indexFile.exists()) {
+                mutableMapOf()
+            } else {
+                try {
+                    mapper.readValue<MutableMap<String, DraftMetadata>>(indexFile)
+                } catch (e: Exception) {
+                    System.err.println("Failed to read drafts index: ${e.message}")
+                    mutableMapOf()
+                }
+            }
+        cachedIndex = loaded
+        return loaded
    }

    private suspend fun loadIndex() {
@@ -576,6 +576,24 @@ object FilterBuilders {
            until = until,
        )

+    /**
+     * Creates a filter for a specific long-form article by author pubkey and d-tag.
+     *
+     * @param pubkey Author public key (hex-encoded, 64 chars)
+     * @param dTag The d-tag (slug) identifier for the addressable event
+     * @return Filter for a specific long-form article
+     */
+    fun longFormByAddress(
+        pubkey: String,
+        dTag: String,
+    ): Filter =
+        Filter(
+            kinds = listOf(30023), // LongTextNoteEvent.KIND
+            authors = listOf(pubkey),
+            tags = mapOf("d" to listOf(dTag)),
+            limit = 1,
+        )
+
    /**
     * Creates a filter for long-form content (kind 30023) from specific authors.
     *
@@ -62,18 +62,18 @@ import androidx.compose.ui.unit.dp
 import androidx.compose.ui.unit.sp
 import com.vitorpamplona.amethyst.commons.compose.editor.MarkdownToolbar
 import com.vitorpamplona.amethyst.commons.compose.editor.MetadataPanel
-import com.vitorpamplona.amethyst.commons.compose.markdown.ArticleMediaRenderer
 import com.vitorpamplona.amethyst.commons.compose.markdown.RenderMarkdown
 import com.vitorpamplona.amethyst.commons.model.nip23LongContent.LongFormPublishAction
 import com.vitorpamplona.amethyst.desktop.account.AccountState
 import com.vitorpamplona.amethyst.desktop.network.DesktopRelayConnectionManager
 import com.vitorpamplona.amethyst.desktop.service.drafts.DesktopDraftStore
 import com.vitorpamplona.amethyst.desktop.service.drafts.DraftMetadata
+import kotlinx.coroutines.delay
 import kotlinx.coroutines.launch
 import java.awt.Desktop
 import java.net.URI

-private val isMacOS = System.getProperty("os.name").lowercase().contains("mac")
+private val ALLOWED_SCHEMES = setOf("https", "http", "nostr", "lightning")

@Composable
 fun ArticleEditorScreen(
@@ -94,6 +94,12 @@ fun ArticleEditorScreen(
    var contentField by remember { mutableStateOf(TextFieldValue("")) }
    var publishing by remember { mutableStateOf(false) }
    var saveMessage by remember { mutableStateOf<String?>(null) }
+    var debouncedContent by remember { mutableStateOf("") }
+
+    LaunchedEffect(contentField.text) {
+        delay(300)
+        debouncedContent = contentField.text
+    }

    // Load existing draft
    LaunchedEffect(draftSlug) {
@@ -120,23 +126,11 @@ fun ArticleEditorScreen(
        }
    }

-    val mediaRenderer =
+    val onLinkClick: (String) -> Unit =
        remember {
-            object : ArticleMediaRenderer {
-                @Composable
-                override fun renderImage(
-                    url: String,
-                    alt: String?,
-                ) {
-                    // Simple text placeholder for editor preview
-                    Text(
-                        "[Image: ${alt ?: url}]",
-                        style = MaterialTheme.typography.bodySmall,
-                        color = MaterialTheme.colorScheme.onSurfaceVariant,
-                    )
-                }
-
-                override fun onLinkClick(url: String) {
+            { url: String ->
+                val scheme = url.substringBefore(":").lowercase()
+                if (scheme in ALLOWED_SCHEMES) {
                    try {
                        Desktop.getDesktop().browse(URI(url))
                    } catch (_: Exception) {
@@ -179,6 +173,8 @@ fun ArticleEditorScreen(
                        dTag = slug.ifBlank { draftStore.slugFromTitle(title) },
                        signer = account.signer,
                    )
+                // TODO: send() is fire-and-forget; markPublished runs before relay ack.
+                //  Consider waiting for relay OK response before marking as published.
                relayManager.send(event)
                draftStore.markPublished(slug)
                onPublished()
@@ -198,7 +194,7 @@ fun ArticleEditorScreen(
                    // Ctrl/Cmd+S to save
                    if (event.type == KeyEventType.KeyDown &&
                        event.key == Key.S &&
-                        (if (isMacOS) event.isMetaPressed else event.isMetaPressed)
+                        event.isMetaPressed
                    ) {
                        saveDraft()
                        true
@@ -316,10 +312,10 @@ fun ArticleEditorScreen(
                    horizontalAlignment = Alignment.CenterHorizontally,
                ) {
                    Column(modifier = Modifier.widthIn(max = 680.dp)) {
-                        if (contentField.text.isNotBlank()) {
+                        if (debouncedContent.isNotBlank()) {
                            RenderMarkdown(
-                                content = contentField.text,
-                                mediaRenderer = mediaRenderer,
+                                content = debouncedContent,
+                                onLinkClick = onLinkClick,
                            )
                        } else {
                            Text(
@@ -36,7 +36,6 @@ import androidx.compose.foundation.rememberScrollState
 import androidx.compose.foundation.verticalScroll
 import androidx.compose.material.icons.Icons
 import androidx.compose.material.icons.automirrored.filled.ArrowBack
-import androidx.compose.material.icons.filled.BookmarkBorder
 import androidx.compose.material3.HorizontalDivider
 import androidx.compose.material3.Icon
 import androidx.compose.material3.IconButton
@@ -56,7 +55,6 @@ import androidx.compose.ui.unit.dp
 import com.vitorpamplona.amethyst.commons.compose.article.ArticleHeader
 import com.vitorpamplona.amethyst.commons.compose.article.TableOfContents
 import com.vitorpamplona.amethyst.commons.compose.article.extractTableOfContents
-import com.vitorpamplona.amethyst.commons.compose.markdown.ArticleMediaRenderer
 import com.vitorpamplona.amethyst.commons.compose.markdown.RenderMarkdown
 import com.vitorpamplona.amethyst.commons.model.nip23LongContent.ReadingTimeCalculator
 import com.vitorpamplona.amethyst.commons.ui.components.EmptyState
@@ -65,15 +63,16 @@ import com.vitorpamplona.amethyst.desktop.account.AccountState
 import com.vitorpamplona.amethyst.desktop.cache.DesktopLocalCache
 import com.vitorpamplona.amethyst.desktop.network.DesktopRelayConnectionManager
 import com.vitorpamplona.amethyst.desktop.subscriptions.DesktopRelaySubscriptionsCoordinator
+import com.vitorpamplona.amethyst.desktop.subscriptions.FilterBuilders
 import com.vitorpamplona.amethyst.desktop.subscriptions.SubscriptionConfig
 import com.vitorpamplona.amethyst.desktop.subscriptions.rememberSubscription
-import com.vitorpamplona.quartz.nip01Core.relay.filters.Filter
 import com.vitorpamplona.quartz.nip23LongContent.LongTextNoteEvent
-import java.text.SimpleDateFormat
-import java.util.Date
+import java.time.Instant
+import java.time.ZoneId
+import java.time.format.DateTimeFormatter
 import java.util.Locale

-private val articleDateFormat = SimpleDateFormat("MMM d, yyyy", Locale.getDefault())
+private val articleDateFormat = DateTimeFormatter.ofPattern("MMM d, yyyy", Locale.getDefault())

 /**
 * Parses a NIP-23 address tag in the format "30023:pubkey:d-tag".
@@ -100,7 +99,6 @@ fun ArticleReaderScreen(
    subscriptionsCoordinator: DesktopRelaySubscriptionsCoordinator? = null,
    onBack: () -> Unit,
    onNavigateToProfile: (String) -> Unit = {},
-    onNavigateToThread: (String) -> Unit = {},
 ) {
    val connectedRelays by relayManager.connectedRelays.collectAsState()
    val relayStatuses by relayManager.relayStatuses.collectAsState()
@@ -118,32 +116,18 @@ fun ArticleReaderScreen(
    // Active ToC entry tracking (placeholder — no scroll-position-based tracking yet)
    var activeTocIndex by remember { mutableStateOf<Int?>(null) }

-    // Media renderer for markdown
-    val mediaRenderer =
+    // Link click handler for markdown
+    val onLinkClick: (String) -> Unit =
        remember {
-            object : ArticleMediaRenderer {
-                @Composable
-                override fun renderImage(
-                    url: String,
-                    alt: String?,
-                ) {
-                    // TODO: Replace with image loading library (coil3 not available on desktop)
-                    Text(
-                        text = "[Image: ${alt ?: url}]",
-                        modifier = Modifier.fillMaxWidth().padding(vertical = 8.dp),
-                    )
-                }
-
-                override fun onLinkClick(url: String) {
-                    if (url.startsWith("nostr:")) {
-                        // TODO: Parse nostr: URI and navigate
-                    } else {
-                        try {
-                            java.awt.Desktop
-                                .getDesktop()
-                                .browse(java.net.URI(url))
-                        } catch (_: Exception) {
-                        }
+            { url: String ->
+                if (url.startsWith("nostr:")) {
+                    // TODO: Parse nostr: URI and navigate
+                } else {
+                    try {
+                        java.awt.Desktop
+                            .getDesktop()
+                            .browse(java.net.URI(url))
+                    } catch (_: Exception) {
                    }
                }
            }
@@ -163,16 +147,8 @@ fun ArticleReaderScreen(
        }

        SubscriptionConfig(
-            subId = "article-${addressTag.hashCode()}-${System.currentTimeMillis()}",
-            filters =
-                listOf(
-                    Filter(
-                        kinds = listOf(LongTextNoteEvent.KIND),
-                        authors = listOf(pubkey),
-                        tags = mapOf("d" to listOf(dTag)),
-                        limit = 1,
-                    ),
-                ),
+            subId = "article-${addressTag.hashCode()}",
+            filters = listOf(FilterBuilders.longFormByAddress(pubkey, dTag)),
            relays = configuredRelays,
            onEvent = { event, _, _, _ ->
                if (event is LongTextNoteEvent) {
@@ -201,7 +177,11 @@ fun ArticleReaderScreen(
    val publishedAt =
        article?.let { art ->
            val ts = art.publishedAt() ?: art.createdAt
-            articleDateFormat.format(Date(ts * 1000))
+            Instant
+                .ofEpochSecond(ts)
+                .atZone(ZoneId.systemDefault())
+                .toLocalDate()
+                .format(articleDateFormat)
        }

    // Author info from local cache
@@ -231,15 +211,6 @@ fun ArticleReaderScreen(
                    color = MaterialTheme.colorScheme.onBackground,
                )
            }
-
-            // Bookmark placeholder
-            IconButton(onClick = { /* TODO: bookmark */ }) {
-                Icon(
-                    Icons.Default.BookmarkBorder,
-                    contentDescription = "Bookmark",
-                    modifier = Modifier.size(24.dp),
-                )
-            }
        }

        // Loading / error / content states
@@ -326,7 +297,7 @@ fun ArticleReaderScreen(
                                // Markdown body
                                RenderMarkdown(
                                    content = content,
-                                    mediaRenderer = mediaRenderer,
+                                    onLinkClick = onLinkClick,
                                )

                                Spacer(Modifier.height(32.dp))
@@ -350,21 +321,6 @@ fun ArticleReaderScreen(

                                HorizontalDivider(thickness = 1.dp)

-                                // Reactions placeholder row
-                                Row(
-                                    modifier =
-                                        Modifier
-                                            .fillMaxWidth()
-                                            .padding(vertical = 16.dp),
-                                    horizontalArrangement = Arrangement.spacedBy(24.dp),
-                                ) {
-                                    Text(
-                                        "Reactions coming soon",
-                                        style = MaterialTheme.typography.bodySmall,
-                                        color = MaterialTheme.colorScheme.onSurfaceVariant,
-                                    )
-                                }
-
                                Spacer(Modifier.height(48.dp))
                            }
                        }
@@ -68,13 +68,19 @@ import com.vitorpamplona.amethyst.desktop.subscriptions.createLongFormFeedSubscr
 import com.vitorpamplona.amethyst.desktop.subscriptions.rememberSubscription
 import com.vitorpamplona.quartz.nip02FollowList.ContactListEvent
 import com.vitorpamplona.quartz.nip23LongContent.LongTextNoteEvent
-import java.text.SimpleDateFormat
-import java.util.Date
+import java.time.Instant
+import java.time.ZoneId
+import java.time.format.DateTimeFormatter
 import java.util.Locale

-private val dateFormat = SimpleDateFormat("MMM d, yyyy", Locale.getDefault())
+private val dateFormat = DateTimeFormatter.ofPattern("MMM d, yyyy", Locale.getDefault())

-private fun formatDate(timestamp: Long): String = dateFormat.format(Date(timestamp * 1000))
+private fun formatDate(timestamp: Long): String =
+    Instant
+        .ofEpochSecond(timestamp)
+        .atZone(ZoneId.systemDefault())
+        .toLocalDate()
+        .format(dateFormat)

 /**
 * Card displaying long-form content (NIP-23) with title, summary, and image.
@@ -132,6 +132,9 @@ fun DeckColumnType.icon(): ImageVector =
        DeckColumnType.MyProfile -> Icons.Default.Person
        DeckColumnType.Chess -> Icons.Default.Extension
        DeckColumnType.Settings -> Icons.Default.Settings
+        is DeckColumnType.Article -> Icons.AutoMirrored.Filled.Article
+        is DeckColumnType.Editor -> Icons.AutoMirrored.Filled.Article
+        DeckColumnType.Drafts -> Icons.AutoMirrored.Filled.Article
        is DeckColumnType.Profile -> Icons.Default.Person
        is DeckColumnType.Thread -> Icons.AutoMirrored.Filled.Article
        is DeckColumnType.Hashtag -> Icons.Default.Tag
@@ -46,7 +46,11 @@ import com.vitorpamplona.amethyst.desktop.model.DesktopIAccount
 import com.vitorpamplona.amethyst.desktop.network.DesktopRelayConnectionManager
 import com.vitorpamplona.amethyst.desktop.subscriptions.DesktopRelaySubscriptionsCoordinator
 import com.vitorpamplona.amethyst.desktop.subscriptions.FeedMode
+import com.vitorpamplona.amethyst.desktop.service.drafts.DesktopDraftStore
+import com.vitorpamplona.amethyst.desktop.ui.ArticleEditorScreen
+import com.vitorpamplona.amethyst.desktop.ui.ArticleReaderScreen
 import com.vitorpamplona.amethyst.desktop.ui.BookmarksScreen
+import com.vitorpamplona.amethyst.desktop.ui.DraftsScreen
 import com.vitorpamplona.amethyst.desktop.ui.FeedScreen
 import com.vitorpamplona.amethyst.desktop.ui.NotificationsScreen
 import com.vitorpamplona.amethyst.desktop.ui.ReadsScreen
@@ -139,6 +143,7 @@ fun DeckColumnContainer(
                onZapFeedback = onZapFeedback,
                onNavigateToProfile = { navState.push(DesktopScreen.UserProfile(it)) },
                onNavigateToThread = { navState.push(DesktopScreen.Thread(it)) },
+                onNavigateToArticle = { navState.push(DesktopScreen.Article(it)) },
            )
            if (currentOverlay != null) {
                Surface(
@@ -182,6 +187,7 @@ internal fun RootContent(
    onZapFeedback: (ZapFeedback) -> Unit,
    onNavigateToProfile: (String) -> Unit,
    onNavigateToThread: (String) -> Unit,
+    onNavigateToArticle: (String) -> Unit = {},
 ) {
    val scope = rememberCoroutineScope()

@@ -232,7 +238,7 @@ internal fun RootContent(
                localCache = localCache,
                account = account,
                onNavigateToProfile = onNavigateToProfile,
-                onNavigateToArticle = onNavigateToThread,
+                onNavigateToArticle = onNavigateToArticle,
            )
        }

@@ -323,6 +329,38 @@ internal fun RootContent(
            )
        }

+        is DeckColumnType.Article -> {
+            ArticleReaderScreen(
+                addressTag = columnType.addressTag,
+                relayManager = relayManager,
+                localCache = localCache,
+                account = account,
+                subscriptionsCoordinator = subscriptionsCoordinator,
+                onBack = {},
+                onNavigateToProfile = onNavigateToProfile,
+            )
+        }
+
+        is DeckColumnType.Editor -> {
+            val draftStore = remember { DesktopDraftStore(scope) }
+            ArticleEditorScreen(
+                draftSlug = columnType.draftSlug,
+                draftStore = draftStore,
+                account = account,
+                relayManager = relayManager,
+                onBack = {},
+                onPublished = {},
+            )
+        }
+
+        DeckColumnType.Drafts -> {
+            val draftStore = remember { DesktopDraftStore(scope) }
+            DraftsScreen(
+                draftStore = draftStore,
+                onOpenEditor = {},
+            )
+        }
+
        is DeckColumnType.Hashtag -> {
            SearchScreen(
                localCache = localCache,
@@ -383,6 +421,18 @@ internal fun OverlayContent(
            )
        }

+        is DesktopScreen.Article -> {
+            ArticleReaderScreen(
+                addressTag = screen.addressTag,
+                relayManager = relayManager,
+                localCache = localCache,
+                account = account,
+                subscriptionsCoordinator = subscriptionsCoordinator,
+                onBack = onBack,
+                onNavigateToProfile = onNavigateToProfile,
+            )
+        }
+
        else -> {
            androidx.compose.material3.Text(
                "Unsupported screen type",
@@ -51,6 +51,16 @@ sealed class DeckColumnType {
        val noteId: String,
    ) : DeckColumnType()

+    data class Article(
+        val addressTag: String,
+    ) : DeckColumnType()
+
+    data class Editor(
+        val draftSlug: String? = null,
+    ) : DeckColumnType()
+
+    object Drafts : DeckColumnType()
+
    data class Hashtag(
        val tag: String,
    ) : DeckColumnType()
@@ -67,6 +77,9 @@ sealed class DeckColumnType {
            MyProfile -> "Profile"
            Chess -> "Chess"
            Settings -> "Settings"
+            is Article -> "Article"
+            is Editor -> "New Article"
+            Drafts -> "Drafts"
            is Profile -> "Profile"
            is Thread -> "Thread"
            is Hashtag -> "#$tag"
@@ -84,6 +97,9 @@ sealed class DeckColumnType {
            MyProfile -> "my_profile"
            Chess -> "chess"
            Settings -> "settings"
+            is Article -> "article"
+            is Editor -> "editor"
+            Drafts -> "drafts"
            is Profile -> "profile"
            is Thread -> "thread"
            is Hashtag -> "hashtag"