Skip to content

Commit 4ad316f

Browse files
committed
fix(share): address remaining copilot review findings
1 parent e2f99d9 commit 4ad316f

13 files changed

Lines changed: 150 additions & 25 deletions

File tree

src-server/src/collab.rs

Lines changed: 28 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ use claudette::room::{ParticipantInfo, Vote};
1111
use serde_json::json;
1212

1313
use crate::handler::ConnectionCtx;
14-
use crate::ws::{ServerState, Writer, send_message};
14+
use crate::ws::{ServerState, Writer, try_send_message};
1515

1616
/// Register a participant against a room, spawn their per-connection event
1717
/// forwarder, and return a snapshot of the room's current state so the
@@ -76,14 +76,16 @@ pub async fn handle_join_session(
7676
let writer = Arc::clone(writer);
7777
let mut rx = room.subscribe();
7878
let chat_session_id_for_forwarder = chat_session_id.to_string();
79-
tokio::spawn(async move {
79+
let forwarder = tokio::spawn(async move {
8080
loop {
8181
match rx.recv().await {
8282
Ok(evt) => {
83-
send_message(&writer, &evt.0).await;
83+
if try_send_message(&writer, &evt.0).await.is_err() {
84+
break;
85+
}
8486
}
8587
Err(tokio::sync::broadcast::error::RecvError::Lagged(_)) => {
86-
let _ = send_message(
88+
if try_send_message(
8789
&writer,
8890
&json!({
8991
"event": "resync-required",
@@ -92,12 +94,20 @@ pub async fn handle_join_session(
9294
},
9395
}),
9496
)
95-
.await;
97+
.await
98+
.is_err()
99+
{
100+
break;
101+
}
96102
}
97103
Err(tokio::sync::broadcast::error::RecvError::Closed) => break,
98104
}
99105
}
100106
});
107+
ctx.room_forwarders
108+
.lock()
109+
.await
110+
.insert(chat_session_id.to_string(), forwarder);
101111
}
102112

103113
// Snapshot for late joiners: full chat history + current participants +
@@ -129,6 +139,9 @@ pub async fn handle_leave_session(
129139
};
130140
let removed = ctx.joined_sessions.lock().await.remove(chat_session_id);
131141
if removed {
142+
if let Some(handle) = ctx.room_forwarders.lock().await.remove(chat_session_id) {
143+
handle.abort();
144+
}
132145
room.remove_participant(&ctx.participant_id).await;
133146
room.publish(json!({
134147
"event": "participants-changed",
@@ -192,6 +205,16 @@ pub async fn handle_vote_plan_approval(
192205
/// resulting roster updates. Called from the WS connection-close path.
193206
pub async fn drop_all_joined_sessions(state: &Arc<ServerState>, ctx: &ConnectionCtx) {
194207
let session_ids: Vec<String> = ctx.joined_sessions.lock().await.drain().collect();
208+
let forwarders: Vec<tokio::task::JoinHandle<()>> = ctx
209+
.room_forwarders
210+
.lock()
211+
.await
212+
.drain()
213+
.map(|(_, h)| h)
214+
.collect();
215+
for handle in forwarders {
216+
handle.abort();
217+
}
195218
for session_id in session_ids {
196219
let Some(room) = state.rooms.get(&session_id).await else {
197220
continue;

src-server/src/handler.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use std::collections::HashSet;
1+
use std::collections::{HashMap, HashSet};
22
use std::sync::Arc;
33

44
use claudette::agent::{self, AgentEvent, AgentSettings, InnerStreamEvent, StreamEvent};
@@ -44,6 +44,7 @@ pub struct ConnectionCtx {
4444
pub display_name: String,
4545
pub is_host: bool,
4646
pub joined_sessions: Arc<Mutex<HashSet<String>>>,
47+
pub room_forwarders: Arc<Mutex<HashMap<String, tokio::task::JoinHandle<()>>>>,
4748
/// The id of the share this connection authenticated against. Each RPC
4849
/// re-checks the share still exists in the live config; a missing
4950
/// share means the host revoked it and every subsequent request fails.
@@ -76,6 +77,7 @@ impl ConnectionCtx {
7677
display_name,
7778
is_host: false,
7879
joined_sessions: Arc::new(Mutex::new(HashSet::new())),
80+
room_forwarders: Arc::new(Mutex::new(HashMap::new())),
7981
share_id,
8082
allowed_workspace_ids,
8183
collaborative,

src-server/src/ws.rs

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -190,10 +190,17 @@ pub type Writer = tokio::sync::Mutex<
190190
futures_util::stream::SplitSink<WebSocketStream<TlsStream<TcpStream>>, Message>,
191191
>;
192192

193-
pub async fn send_message(writer: &Writer, value: &serde_json::Value) {
193+
pub async fn try_send_message(
194+
writer: &Writer,
195+
value: &serde_json::Value,
196+
) -> Result<(), tokio_tungstenite::tungstenite::Error> {
194197
let text = serde_json::to_string(value).unwrap_or_default();
195198
let mut w = writer.lock().await;
196-
let _ = w.send(Message::Text(text.into())).await;
199+
w.send(Message::Text(text.into())).await
200+
}
201+
202+
pub async fn send_message(writer: &Writer, value: &serde_json::Value) {
203+
let _ = try_send_message(writer, value).await;
197204
}
198205

199206
/// Accept a TLS connection and upgrade it to WebSocket.

src-server/tests/workspace_archive_propagation.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ fn make_workspace(id: &str, repo_id: &str, status: WorkspaceStatus) -> Workspace
5050
status,
5151
agent_status: AgentStatus::Idle,
5252
status_line: String::new(),
53+
sort_order: 0,
5354
created_at: "2026-01-01 00:00:00".into(),
5455
}
5556
}

src-tauri/src/commands/share.rs

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@ use tauri::{AppHandle, State};
1616

1717
#[cfg(feature = "server")]
1818
use std::sync::Arc;
19+
#[cfg(feature = "server")]
20+
use tauri::Manager;
1921

2022
use crate::state::AppState;
2123

@@ -208,6 +210,7 @@ async fn ensure_share_server(app: &AppHandle, state: &AppState) -> Result<bool,
208210
let rooms = std::sync::Arc::clone(&state.rooms);
209211
let workspace_events = std::sync::Arc::clone(&state.workspace_events);
210212
let cfg_for_server = Arc::clone(&cfg_arc);
213+
let app_for_server = app.clone();
211214
let opts = claudette_server::ServerOptions {
212215
existing_config: Some(cfg_for_server),
213216
..Default::default()
@@ -218,15 +221,15 @@ async fn ensure_share_server(app: &AppHandle, state: &AppState) -> Result<bool,
218221
{
219222
eprintln!("[share] in-process server exited: {e}");
220223
}
224+
let state = app_for_server.state::<AppState>();
225+
*state.collab_server_running.write().await = false;
221226
});
222227

223228
// The host event subscriber attaches via `RoomRegistry::set_on_create`,
224229
// installed once at app startup in `main.rs::setup`. Each new room
225230
// gets a host-side mirror task synchronously at creation time, before
226231
// any handler can publish into it (see the on_create hook docstring
227232
// in `src/room.rs` for why this ordering is load-bearing).
228-
let _ = app;
229-
230233
*running = true;
231234
Ok(true)
232235
}

src-tauri/src/state.rs

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -414,12 +414,13 @@ pub struct AppState {
414414
/// constructed unconditionally in `AppState::new` so publishers don't
415415
/// need to handle the absent case.
416416
pub workspace_events: Arc<WorkspaceEventBus>,
417-
/// `true` once the in-process collaborative server has been started for
417+
/// `true` while the in-process collaborative server is running for
418418
/// this app instance. Distinct from `local_server` (subprocess) — the
419419
/// in-process server shares `rooms` with `AppState` and is used only for
420-
/// collab share. Remains `true` for the rest of the app lifetime; we
421-
/// don't currently support tearing it down. Used to refuse to start the
422-
/// subprocess server when an in-process server already owns the port.
420+
/// collab share. Reset when the server task exits so a failed bind or
421+
/// early shutdown can be retried without restarting the app. Used to
422+
/// refuse to start the subprocess server when an in-process server
423+
/// already owns the port.
423424
pub collab_server_running: tokio::sync::RwLock<bool>,
424425
/// Shared `ServerConfig` for the in-process share server. Lazily
425426
/// constructed on first `start_share` call and kept as a clone of the

src/ui/src/components/chat/ParticipantsRoster.tsx

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { useState } from "react";
2+
import { useTranslation } from "react-i18next";
23
import { Crown, MicOff, MoreHorizontal, UserMinus, Users } from "lucide-react";
34
import { useAppStore } from "../../stores/useAppStore";
45
import { kickParticipant, muteParticipant } from "../../services/tauri";
@@ -26,6 +27,7 @@ export function ParticipantsRoster({
2627
sessionId: string;
2728
selfParticipantId: string | null;
2829
}) {
30+
const { t } = useTranslation("chat");
2931
const participants = useAppStore((s) => s.participants[sessionId]);
3032
const [openMenuFor, setOpenMenuFor] = useState<string | null>(null);
3133

@@ -48,7 +50,7 @@ export function ParticipantsRoster({
4850
gap: 4,
4951
padding: "0 6px",
5052
}}
51-
title={`${participants.length} connected`}
53+
title={t("participants_connected", { count: participants.length })}
5254
>
5355
<Users size={12} style={{ opacity: 0.6 }} />
5456
{participants.map((p) => {
@@ -83,8 +85,10 @@ export function ParticipantsRoster({
8385
onClick={() =>
8486
setOpenMenuFor(openMenuFor === p.id ? null : p.id)
8587
}
86-
title="Moderate"
87-
aria-label={`Moderate ${p.display_name}`}
88+
title={t("participant_moderate_title")}
89+
aria-label={t("participant_moderate_aria", {
90+
name: p.display_name,
91+
})}
8892
style={{
8993
background: "transparent",
9094
border: "none",
@@ -126,7 +130,7 @@ export function ParticipantsRoster({
126130
}}
127131
>
128132
<MicOff size={12} />
129-
{p.muted ? "Unmute" : "Mute"}
133+
{p.muted ? t("participant_unmute") : t("participant_mute")}
130134
</button>
131135
<button
132136
style={moderationItemStyle}
@@ -140,7 +144,7 @@ export function ParticipantsRoster({
140144
}}
141145
>
142146
<UserMinus size={12} />
143-
Kick
147+
{t("participant_kick")}
144148
</button>
145149
</div>
146150
)}

src/ui/src/components/chat/PlanApprovalCard.tsx

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ export function PlanApprovalCard({
4848
let content: string;
4949
if (remoteConnectionId) {
5050
content = (await sendRemoteCommand(remoteConnectionId, "read_plan_file", {
51+
chat_session_id: approval.sessionId,
5152
path: approval.planFilePath,
5253
})) as string;
5354
} else {
@@ -217,6 +218,7 @@ export function PlanApprovalCard({
217218
* `"host"`, matching what the Rust resolver records.
218219
*/
219220
function ConsensusProgress({ approval }: { approval: PlanApproval }) {
221+
const { t } = useTranslation("chat");
220222
const vote = useAppStore((s) => s.consensusVotes[approval.sessionId]);
221223
// Compare voter ids against the local participant's id (the workspace's
222224
// self-pid), NOT the literal `"host"` — on a remote client the local
@@ -232,21 +234,26 @@ function ConsensusProgress({ approval }: { approval: PlanApproval }) {
232234
return (
233235
<div style={{ marginTop: 8, display: "flex", flexDirection: "column", gap: 4, fontSize: 12 }}>
234236
<div>
235-
<strong>Consensus required:</strong> {totalVoted}/{totalRequired} voted
237+
<strong>
238+
{t("plan_approval_consensus_required", {
239+
voted: totalVoted,
240+
required: totalRequired,
241+
})}
242+
</strong>
236243
</div>
237244
{vote.requiredVoters.map((voter) => {
238245
const cast = vote.votes[voter.id];
239246
const status = cast
240247
? cast.kind === "approve"
241-
? "approved"
242-
: `denied: ${cast.reason}`
243-
: "waiting";
248+
? t("plan_approval_vote_approved")
249+
: t("plan_approval_vote_denied", { reason: cast.reason })
250+
: t("plan_approval_vote_waiting");
244251
const isSelf = voter.id === selfParticipantId;
245252
return (
246253
<div key={voter.id}>
247254
<span>{voter.display_name}</span>
248-
{isSelf ? " (you)" : ""}
249-
{voter.is_host ? " · host" : ""}
255+
{isSelf ? ` ${t("plan_approval_you_marker")}` : ""}
256+
{voter.is_host ? ` · ${t("plan_approval_host_marker")}` : ""}
250257
{": "}
251258
<em>{status}</em>
252259
</div>

src/ui/src/locales/en/chat.json

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,19 @@
8585
"plan_approval_or_feedback": "Or provide feedback below",
8686
"plan_approval_feedback_placeholder": "Request changes or ask for a revision…",
8787
"plan_approval_send": "Send",
88+
"plan_approval_consensus_required": "Consensus required: {{voted}}/{{required}} voted",
89+
"plan_approval_vote_approved": "approved",
90+
"plan_approval_vote_denied": "denied: {{reason}}",
91+
"plan_approval_vote_waiting": "waiting",
92+
"plan_approval_you_marker": "(you)",
93+
"plan_approval_host_marker": "host",
94+
"participants_connected_one": "{{count}} connected",
95+
"participants_connected_other": "{{count}} connected",
96+
"participant_moderate_title": "Moderate",
97+
"participant_moderate_aria": "Moderate {{name}}",
98+
"participant_mute": "Mute",
99+
"participant_unmute": "Unmute",
100+
"participant_kick": "Kick",
88101

89102
"session_new": "New session",
90103
"session_close": "Close session",

src/ui/src/locales/es/chat.json

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,9 @@
2222
"composer_placeholder_queued": "Escribe para encolar un mensaje...",
2323
"composer_placeholder_shell": "Ejecuta un comando shell...",
2424
"shell_mode_label": "Shell",
25+
"composer_locked_placeholder": "{{name}} está preguntando…",
26+
"composer_locked_title": "Bloqueado: {{name}} está controlando el agente",
27+
"user_label": "Usuario",
2528
"voice_input": "Entrada de voz",
2629
"voice_stop": "Detener entrada de voz",
2730
"voice_discard": "Descartar transcripción",
@@ -82,6 +85,19 @@
8285
"plan_approval_or_feedback": "O proporciona comentarios abajo",
8386
"plan_approval_feedback_placeholder": "Solicita cambios o pide una revisión…",
8487
"plan_approval_send": "Enviar",
88+
"plan_approval_consensus_required": "Consenso requerido: {{voted}}/{{required}} votaron",
89+
"plan_approval_vote_approved": "aprobó",
90+
"plan_approval_vote_denied": "denegó: {{reason}}",
91+
"plan_approval_vote_waiting": "esperando",
92+
"plan_approval_you_marker": "(tú)",
93+
"plan_approval_host_marker": "anfitrión",
94+
"participants_connected_one": "{{count}} conectado",
95+
"participants_connected_other": "{{count}} conectados",
96+
"participant_moderate_title": "Moderar",
97+
"participant_moderate_aria": "Moderar a {{name}}",
98+
"participant_mute": "Silenciar",
99+
"participant_unmute": "Reactivar sonido",
100+
"participant_kick": "Expulsar",
85101

86102
"session_new": "Nueva sesión",
87103
"session_close": "Cerrar sesión",

0 commit comments

Comments
 (0)