85a7f9f · opencode-android

git.iko.soy/opencode-android

on master help sign in

Fix four UI/streaming/SSE bugs found by a codebase bug hunt

85a7f9fiko3 months ago

app/src/main/java/soy/iko/opencode/data/network/EventStreamClient.kt+38−5

compare with master

unifiedsplit

@@ -21,9 +21,9 @@ import kotlinx.coroutines.flow.SharingStarted

21import kotlinx.coroutines.flow.asStateFlow

22import kotlinx.coroutines.flow.buffer

23import kotlinx.coroutines.flow.channelFlow

24−import kotlinx.coroutines.flow.produceIn

25import kotlinx.coroutines.flow.shareIn

24import kotlinx.coroutines.flow.shareIn

26import kotlinx.coroutines.isActive

25import kotlinx.coroutines.isActive

26+import kotlinx.coroutines.launch

27import kotlinx.coroutines.selects.onTimeout

28import kotlinx.coroutines.selects.select

29import java.io.IOException

@@ -108,7 +108,15 @@ open class EventStreamClient(

108 onTimeout(idleTimeoutMs) { null }

109 }

110 if (result == null) throw IOException("SSE idle timeout, reconnecting")

111− val sse = result.getOrNull() ?: break

111+ val sse = result.getOrNull()

112+ if (sse == null) {

113+ // The producer channel is closed. A non-null cause means `incoming`

114+ // failed (e.g. a socket reset bridged from the producer) — rethrow it so

115+ // the outer loop logs and retries with backoff. A clean close (null

116+ // cause) just ends this attempt and lets the loop reconnect.

117+ result.exceptionOrNull()?.let { throw it }

118+ break

119+ }

112 val data = sse.data

120 val data = sse.data

113 if (data == null) {

121 if (data == null) {

114 // A comment or keep-alive event (no data field) resets the idle watchdog

122 // A comment or keep-alive event (no data field) resets the idle watchdog

@@ -169,9 +177,33 @@ open class EventStreamClient(

169 // made while the previous stream was healthy, so it doesn't suppress

177 // made while the previous stream was healthy, so it doesn't suppress

170 // the next legitimate backoff.

178 // the next legitimate backoff.

171 while (reconnectSignal.tryReceive().isSuccess) { /* drain */ }

179 while (reconnectSignal.tryReceive().isSuccess) { /* drain */ }

172− // `incoming` is a cold Flow; bridge it to a ReceiveChannel so each

180+ // Bridge ktor's `incoming` flow to a channel so each element can be

173− // element can be raced against an idle timeout via select.

181+ // raced against the idle-timeout watchdog via select. We collect it

174− val events = incoming.produceIn(scope)

182+ // ourselves instead of `incoming.produceIn(scope)`: produceIn launches

183+ // the collector as a child of this channelFlow, so when `incoming`

184+ // throws on an abnormal socket close — a TCP reset, i.e. the common

185+ // mobile case: Wi-Fi⇄cellular handoff, server or proxy RST — the child's

186+ // failure cancels the whole channelFlow and bypasses the retry `catch`

187+ // below. That killed reconnect outright and surfaced the IOException as

188+ // an uncaught crash through the (handler-less) shareIn scope. Collecting

189+ // into our own channel and closing it with the cause keeps the failure

190+ // local: readSseEvents sees the closed channel, rethrows the cause, and

191+ // the normal retry/backoff path handles it. (A clean EOF closes the

192+ // channel with no cause → readSseEvents breaks and reconnects as before.)

193+ val events = Channel<ServerSentEvent>(Channel.BUFFERED)

194+ val producer = scope.launch {

195+ try {

196+ incoming.collect { events.send(it) }

197+ events.close()

198+ } catch (c: CancellationException) {

199+ events.close(c)

200+ throw c

201+ } catch (t: Throwable) {

202+ // Deliver the failure via the channel rather than letting it

203+ // propagate up and cancel the parent channelFlow.

204+ events.close(t)

205+ }

206+ }

175 // Reset the backoff only once real data actually arrives, not on

207 // Reset the backoff only once real data actually arrives, not on

176 // block entry: a server that accepts the connection then immediately

208 // block entry: a server that accepts the connection then immediately

177 // drops it would otherwise loop forever at the initial backoff and

209 // drops it would otherwise loop forever at the initial backoff and

@@ -186,6 +218,7 @@ open class EventStreamClient(

186 send(it)

218 send(it)

187 }

219 }

188 } finally {

220 } finally {

221+ producer.cancel()

189 events.cancel()

222 events.cancel()

190 }

223 }

191 }

224 }

app/src/main/java/soy/iko/opencode/ui/chat/ChatScreen.kt+25−8

@@ -61,6 +61,7 @@ import androidx.compose.runtime.getValue

61import androidx.compose.runtime.mutableStateOf

62import androidx.compose.runtime.remember

63import androidx.compose.runtime.rememberCoroutineScope

64+import androidx.compose.runtime.rememberUpdatedState

64import androidx.compose.runtime.saveable.rememberSaveable

65import androidx.compose.runtime.saveable.rememberSaveable

65import androidx.compose.runtime.setValue

66import androidx.compose.runtime.setValue

66import androidx.compose.runtime.snapshotFlow

67import androidx.compose.runtime.snapshotFlow

@@ -440,6 +441,15 @@ fun ChatScreen(

440 val listItems = remember(messages, todayLabel, yesterdayLabel) {

441 val listItems = remember(messages, todayLabel, yesterdayLabel) {

441 buildMessageListItems(messages, todayLabel, yesterdayLabel)

442 buildMessageListItems(messages, todayLabel, yesterdayLabel)

442 }

443 }

444+ // listItems is a plain (non-snapshot) local, rebuilt each recomposition. The

445+ // derivedStateOf and snapshotFlow lambdas below are created once (keyless

446+ // remember / LaunchedEffect(Unit)); reading `listItems` directly inside them

447+ // would capture the first composition's list — which is empty, since `messages`

448+ // starts empty — freezing it forever. That pinned isPinnedToBottom at true and

449+ // AutoScrollSignal.size at 0, breaking streaming auto-scroll and the

450+ // jump-to-latest FAB. rememberUpdatedState hands those lambdas a stable State

451+ // whose value tracks the latest list.

452+ val currentListItems by rememberUpdatedState(listItems)

443

453

444 val isPinnedToBottom by remember {

454 val isPinnedToBottom by remember {

445 derivedStateOf {

455 derivedStateOf {

@@ -449,8 +459,9 @@ fun ChatScreen(

449 // includes the working indicator — otherwise the pin check targets

459 // includes the working indicator — otherwise the pin check targets

450 // listItems.lastIndex, the typing row sits just below the viewport,

460 // listItems.lastIndex, the typing row sits just below the viewport,

451 // and the user never sees the progress indicator even when pinned.

461 // and the user never sees the progress indicator even when pinned.

462+ val items = currentListItems

452− val effectiveLast = if (running) listItems.size else listItems.lastIndex

463+ val effectiveLast = if (running) items.size else items.lastIndex

453− lastVisible >= effectiveLast || listItems.isEmpty()

464+ lastVisible >= effectiveLast || items.isEmpty()

454 }

465 }

455 }

466 }

456

467

@@ -495,13 +506,14 @@ fun ChatScreen(

495 // and tool output — not just TextPart — keeps a pinned view following a long

506 // and tool output — not just TextPart — keeps a pinned view following a long

496 // reasoning/tool block while it streams (previously it stalled at 0).

507 // reasoning/tool block while it streams (previously it stalled at 0).

497 val lastLen = streamingContentLength(messages.lastOrNull()?.parts?.lastOrNull())

508 val lastLen = streamingContentLength(messages.lastOrNull()?.parts?.lastOrNull())

498− AutoScrollSignal(listItems.size, lastLen, isPinnedToBottom)

509+ AutoScrollSignal(currentListItems.size, lastLen, isPinnedToBottom)

499 }.collect { signal ->

510 }.collect { signal ->

500 if (signal.size > 0 && signal.pinned) {

511 if (signal.size > 0 && signal.pinned) {

501 // Scroll to the effective last index, including the trailing

512 // Scroll to the effective last index, including the trailing

502 // "__typing" row when a run is active so the working indicator

513 // "__typing" row when a run is active so the working indicator

503 // is brought into view (not just the last message).

514 // is brought into view (not just the last message).

515+ val items = currentListItems

504− val target = if (running) listItems.size else listItems.lastIndex

516+ val target = if (running) items.size else items.lastIndex

505 listState.scrollToItem(target)

517 listState.scrollToItem(target)

506 }

518 }

507 }

519 }

@@ -1056,9 +1068,13 @@ private sealed interface MessageListItem {

1056 override val contentType: Any get() = message.info::class

1068 override val contentType: Any get() = message.info::class

1057 }

1069 }

1058

1070

1059− data class Separator(val label: String) : MessageListItem {

1071+ data class Separator(val label: String, val ordinal: Int) : MessageListItem {

1060− // Stable key per label so a recomposition doesn't re-create the item.

1072+ // Key on the day-occurrence ordinal, not the label: two non-contiguous groups

1073+ // can share a label (e.g. an untimestamped message — empty label — between two

1074+ // same-day messages, or two untimestamped groups), and a label-only key would

1075+ // then collide, which makes LazyColumn throw on duplicate keys. The ordinal is

1076+ // stable for a given message ordering so slots aren't needlessly recreated.

1061− override val key: Any get() = "sep_$label"

1077+ override val key: Any get() = "sep_${ordinal}_$label"

1062 // Separators share a contentType so the LazyColumn can recycle their slots.

1078 // Separators share a contentType so the LazyColumn can recycle their slots.

1063 override val contentType: Any get() = "separator"

1079 override val contentType: Any get() = "separator"

1064 }

1080 }

@@ -1085,11 +1101,12 @@ private fun buildMessageListItems(

1085 val dateFmt = java.text.DateFormat.getDateInstance(java.text.DateFormat.MEDIUM)

1101 val dateFmt = java.text.DateFormat.getDateInstance(java.text.DateFormat.MEDIUM)

1086 val result = ArrayList<MessageListItem>(messages.size + 4)

1102 val result = ArrayList<MessageListItem>(messages.size + 4)

1087 var lastDayKey: String? = null

1103 var lastDayKey: String? = null

1104+ var sepOrdinal = 0

1088 for (message in messages) {

1105 for (message in messages) {

1089 val ts = message.info.time?.created ?: message.info.time?.updated ?: message.info.time?.completed

1106 val ts = message.info.time?.created ?: message.info.time?.updated ?: message.info.time?.completed

1090 val dayKey = ts?.let { dayKey(it) } ?: ""

1107 val dayKey = ts?.let { dayKey(it) } ?: ""

1091 if (dayKey != lastDayKey) {

1108 if (dayKey != lastDayKey) {

1092− result.add(MessageListItem.Separator(dayLabel(dayKey, ts ?: 0L, today, todayLabel, yesterdayLabel, dateFmt)))

1109+ result.add(MessageListItem.Separator(dayLabel(dayKey, ts ?: 0L, today, todayLabel, yesterdayLabel, dateFmt), sepOrdinal++))

1093 lastDayKey = dayKey

1110 lastDayKey = dayKey

1094 }

1111 }

1095 result.add(MessageListItem.Message(message))

1112 result.add(MessageListItem.Message(message))

app/src/main/java/soy/iko/opencode/ui/session/TwoPaneSessionChat.kt+16−6

@@ -16,6 +16,7 @@ import androidx.compose.material3.Text

16import androidx.compose.runtime.Composable

17import androidx.compose.runtime.LaunchedEffect

18import androidx.compose.runtime.getValue

19+import androidx.compose.runtime.key

19import androidx.compose.runtime.mutableStateOf

20import androidx.compose.runtime.mutableStateOf

20import androidx.compose.runtime.remember

21import androidx.compose.runtime.remember

21import androidx.compose.runtime.saveable.rememberSaveable

22import androidx.compose.runtime.saveable.rememberSaveable

@@ -114,12 +115,21 @@ fun TwoPaneSessionChat(

114 EmptyDetail()

115 EmptyDetail()

115 } else {

116 } else {

116 BackHandler { selected = null }

117 BackHandler { selected = null }

117− ChatScreen(

118+ // key() on the session id so switching conversations in two-pane mode

119+ // gives ChatScreen a fresh composition. Without it, position-anchored

120+ // state (rememberLazyListState, didInitialScroll, and the LaunchedEffect(Unit)

121+ // that wires error/retry snackbars) persists across the switch: the new

122+ // session inherits the old scroll offset, skips its initial scroll-to-bottom,

123+ // and its error events go unhandled. viewModel(key=sessionId) already swaps

124+ // the VM; this aligns the composition state with it.

125+ key(sessionId) {

126+ ChatScreen(

118− container = container,

127+ container = container,

119− sessionId = sessionId,

128+ sessionId = sessionId,

120− onBack = { selected = null },

129+ onBack = { selected = null },

121− onOpenFile = onOpenFile,

130+ onOpenFile = onOpenFile,

122− )

131+ )

132+ }

123 }

133 }

124 }

134 }

125 }

135 }