scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-09-14 23:22:51 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 4

refactor: simplify steering review findings

Browse source 
- steer.rs: drop duplicate parse+archive checks in steer_run; control_run
  is the single gate
- use-run-toasts: bound dedupe Set to 256 entries (was unbounded for the
  lifetime of a run)
- steer-composer: read onClose via ref so the keydown effect doesn't
  re-attach on every parent re-render
- discard_session: drop unused parent_session_id parameter (was None at
  all 14 call sites)
- strip narrative/WHAT comments and restore trailing newlines

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-05-05 14:44:45 -04:00
parent d94ed82223
commit 4c7115d2ca
No known key found for this signature in database
6 changed files with 35 additions and 51 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

21
apps/fabro-web/app/components/steer-composer.tsx
View file

@ -10,24 +10,14 @@ interface SteerComposerProps {
onClose: () => void;
}
/**
* Modal composer for sending a mid-run steering message to a running run.
*
* Renders a textarea + two action buttons:
* - Send: appends to the steering queue (default).
* - Interrupt: cancels the in-flight LLM stream / tool calls and delivers
* the message as the next user turn.
*
* Surfaces 409 errors inline so users see the rejection reason (e.g., the
* `cli_agent_not_steerable` code).
*/
export function SteerComposer({ runId, open, onClose }: SteerComposerProps) {
const [text, setText] = useState("");
const [errorMessage, setErrorMessage] = useState<string | null>(null);
const textareaRef = useRef<HTMLTextAreaElement | null>(null);
const onCloseRef = useRef(onClose);
onCloseRef.current = onClose;
const { trigger, isMutating } = useSteerRun(runId);
// Autofocus when opening; reset state when closing.
useEffect(() => {
if (open) {
requestAnimationFrame(() => textareaRef.current?.focus());
@ -37,18 +27,17 @@ export function SteerComposer({ runId, open, onClose }: SteerComposerProps) {
}
}, [open]);
// Close on Escape.
useEffect(() => {
if (!open) return;
const onKey = (e: KeyboardEvent) => {
if (e.key === "Escape") {
e.preventDefault();
onClose();
onCloseRef.current();
}
};
window.addEventListener("keydown", onKey);
return () => window.removeEventListener("keydown", onKey);
}, [open, onClose]);
}, [open]);
if (!open) return null;
@ -151,4 +140,4 @@ export function SteerComposer({ runId, open, onClose }: SteerComposerProps) {
</div>
</div>
);
}
}

13
apps/fabro-web/app/hooks/use-run-toasts.ts
View file

@ -5,6 +5,7 @@ import { subscribeToRunEvents, type RunEventPayload } from "../lib/run-events";
import type { MutateFn } from "../lib/sse";
const NOOP_MUTATE = (() => undefined) as MutateFn;
const DEDUPE_WINDOW = 256;
export function useRunToasts(runId: string | undefined) {
const { push } = useToast();
@ -13,13 +14,19 @@ export function useRunToasts(runId: string | undefined) {
useEffect(() => {
if (!runId) return;
seenEventIdsRef.current.clear();
const seen = new Set<string>();
seenEventIdsRef.current = seen;
return subscribeToRunEvents(runId, NOOP_MUTATE, undefined, {
onEvent: (payload) => {
const dedupeId = eventDedupeId(payload);
if (dedupeId) {
if (seenEventIdsRef.current.has(dedupeId)) return;
seenEventIdsRef.current.add(dedupeId);
if (seen.has(dedupeId)) return;
seen.add(dedupeId);
if (seen.size > DEDUPE_WINDOW) {
// Set iteration order is insertion order; drop the oldest.
const oldest = seen.values().next().value;
if (oldest !== undefined) seen.delete(oldest);
}
}
const message = steeringToastMessage(payload);

2
apps/fabro-web/app/lib/mutations.ts
View file

@ -168,4 +168,4 @@ export function useLoginDevToken() {
return response.json() as Promise<{ ok: boolean }>;
},
);
}
}

4
apps/fabro-web/app/routes/runs.tsx
View file

@ -463,8 +463,6 @@ function SortablePrCard({
function BoardColumn({ column }: { column: Column }) {
const Icon = iconMap[column.iconType];
// Steer button is shown in all modes (no longer demo-gated). The modal/
// composer logic lives below in `PrCard`; clicking the button opens it.
const actions = column.actions;
return (
<div className="flex min-w-0 flex-col">
@ -904,4 +902,4 @@ export default function Runs() {
</div>
</DndContext>
);
}
}

15
lib/crates/fabro-server/src/server/handler/steer.rs
View file

@ -37,17 +37,8 @@ async fn steer_run(
Path(id): Path<String>,
Json(req): Json<SteerRunRequest>,
) -> Response {
let id = match parse_run_id_path(&id) {
Ok(id) => id,
Err(response) => return response,
};
if let Some(response) = reject_if_archived(state.as_ref(), &id).await {
return response;
}
// Body validation. OpenAPI enforces minLength=1/maxLength=8192 at the
// type boundary already, so the only thing left to guard against is a
// payload that's whitespace-only.
// OpenAPI enforces minLength=1/maxLength=8192 already; only whitespace-only
// payloads can slip through.
let SteerRunRequest { text, interrupt } = req;
let text: String = text.into();
if text.trim().is_empty() {
@ -59,7 +50,7 @@ async fn steer_run(
RunControlRequest::Steer { text }
};
control_run(auth, state, id.to_string(), control).await
control_run(auth, state, id, control).await
}
async fn interrupt_run(

31
lib/crates/fabro-workflow/src/handler/llm/api.rs
View file

@ -136,7 +136,6 @@ fn discard_session(
session: &mut Session,
lease: &mut Option<Arc<ActivationLease>>,
emitter: &Arc<Emitter>,
parent_session_id: Option<String>,
) {
if let Some(lease) = lease.take() {
lease.release();
@ -145,7 +144,7 @@ fn discard_session(
if session.close() {
emitter.emit(&Event::AgentSessionEnded {
session_id,
parent_session_id,
parent_session_id: None,
});
}
}
@ -710,12 +709,12 @@ impl CodergenBackend for AgentApiBackend {
Err(err) => match classify_agent_error(err, allow_failover_primary) {
AgentApiErrorDisposition::Cancelled => {
bridge.abort();
discard_session(&mut session, &mut lease, emitter, None);
discard_session(&mut session, &mut lease, emitter);
return Err(Error::Cancelled);
}
AgentApiErrorDisposition::Terminal(err) => {
bridge.abort();
discard_session(&mut session, &mut lease, emitter, None);
discard_session(&mut session, &mut lease, emitter);
return Err(err);
}
AgentApiErrorDisposition::FailoverEligible(sdk_err) => {
@ -733,7 +732,7 @@ impl CodergenBackend for AgentApiBackend {
Ok(active_lease) => lease = Some(active_lease),
Err(err) => {
bridge.abort();
discard_session(&mut session, &mut lease, emitter, None);
discard_session(&mut session, &mut lease, emitter);
return Err(err);
}
}
@ -748,12 +747,12 @@ impl CodergenBackend for AgentApiBackend {
Err(err) => match classify_agent_error(err, allow_failover_primary) {
AgentApiErrorDisposition::Cancelled => {
bridge.abort();
discard_session(&mut session, &mut lease, emitter, None);
discard_session(&mut session, &mut lease, emitter);
return Err(Error::Cancelled);
}
AgentApiErrorDisposition::Terminal(err) => {
bridge.abort();
discard_session(&mut session, &mut lease, emitter, None);
discard_session(&mut session, &mut lease, emitter);
return Err(err);
}
AgentApiErrorDisposition::FailoverEligible(sdk_err) => {
@ -765,7 +764,7 @@ impl CodergenBackend for AgentApiBackend {
let mut succeeded = false;
bridge.abort();
discard_session(&mut session, &mut lease, emitter, None);
discard_session(&mut session, &mut lease, emitter);
for (index, target) in self.fallback_chain.iter().enumerate() {
emitter.emit_scoped(
@ -828,18 +827,18 @@ impl CodergenBackend for AgentApiBackend {
match classify_agent_error(err, allow_failover_next) {
AgentApiErrorDisposition::Cancelled => {
bridge.abort();
discard_session(&mut session, &mut lease, emitter, None);
discard_session(&mut session, &mut lease, emitter);
return Err(Error::Cancelled);
}
AgentApiErrorDisposition::Terminal(err) => {
bridge.abort();
discard_session(&mut session, &mut lease, emitter, None);
discard_session(&mut session, &mut lease, emitter);
return Err(err);
}
AgentApiErrorDisposition::FailoverEligible(sdk_err) => {
last_err = Error::Llm(sdk_err);
bridge.abort();
discard_session(&mut session, &mut lease, emitter, None);
discard_session(&mut session, &mut lease, emitter);
continue;
}
}
@ -853,7 +852,7 @@ impl CodergenBackend for AgentApiBackend {
Ok(active_lease) => lease = Some(active_lease),
Err(err) => {
bridge.abort();
discard_session(&mut session, &mut lease, emitter, None);
discard_session(&mut session, &mut lease, emitter);
return Err(err);
}
}
@ -865,18 +864,18 @@ impl CodergenBackend for AgentApiBackend {
Err(err) => match classify_agent_error(err, allow_failover_next) {
AgentApiErrorDisposition::Cancelled => {
bridge.abort();
discard_session(&mut session, &mut lease, emitter, None);
discard_session(&mut session, &mut lease, emitter);
return Err(Error::Cancelled);
}
AgentApiErrorDisposition::Terminal(err) => {
bridge.abort();
discard_session(&mut session, &mut lease, emitter, None);
discard_session(&mut session, &mut lease, emitter);
return Err(err);
}
AgentApiErrorDisposition::FailoverEligible(sdk_err) => {
last_err = Error::Llm(sdk_err);
bridge.abort();
discard_session(&mut session, &mut lease, emitter, None);
discard_session(&mut session, &mut lease, emitter);
}
},
}
@ -891,7 +890,7 @@ impl CodergenBackend for AgentApiBackend {
// bridge's `Drop` will abort the spawned task on early return.
if let Err(err) = result {
bridge.abort();
discard_session(&mut session, &mut lease, emitter, None);
discard_session(&mut session, &mut lease, emitter);
return Err(err);
}