feat(app): memory citations UI, respond queue, silent ingest refresh#800
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 58 minutes and 18 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- ChatService + ChatRuntimeProvider + threadSlice: citation metadata on events - Conversations + CitationChips: render memory sources under assistant bubbles - Provider surfaces: API client, Redux slice, RespondQueuePanel, Accounts wiring - webviewAccountService: silent fetchRespondQueue after ingest - config: CONSUMER_FIRST_SESSION flag (opt-in) Merge after core PR: openhuman.provider_surfaces_list_queue + citation payloads. Made-with: Cursor
jwalin-shah
force-pushed
the
pr/app-retrieval-queue-v1
branch
from
0e92105 to
db8c095
Compare
senamakel
left a comment
senamakel
left a comment
There was a problem hiding this comment.
Walkthrough
This PR adds three related UI capabilities to the Accounts page and conversation view: (1) a respond queue panel that polls every 10 seconds and shows pending provider events in a sidebar next to the agent chat pane; (2) memory citation chips that surface retrieval citations attached to assistant messages via a new field propagated from / socket events; and (3) a silent background refresh of the respond queue triggered immediately after each ingest event in . Supporting scaffolding includes a new Redux slice (), a typed API wrapper (), a shared type file (), and migration of two accesses to constants (, ).
The Rust back-end for both features exists (provider_surfaces domain + citations on chat events), so this is a pure front-end wiring PR. The overall approach is sound, well-structured, and follows existing patterns. A few correctness gaps and one structural concern need attention before merge.
Changes
| File | Summary |
|---|---|
app/src/components/accounts/RespondQueuePanel.tsx |
New sidebar component rendering the respond queue with time-relative labels and deep-link buttons |
app/src/pages/Accounts.tsx |
Adds Redux selectors for providerSurfaces state, a 10 s polling interval, and wires RespondQueuePanel into the agent pane layout |
app/src/pages/Conversations.tsx |
Renders CitationChips below agent bubbles when extraMetadata.citations is present |
app/src/pages/conversations/components/CitationChips.tsx |
New component: collapsible <details> chips for each memory citation |
app/src/providers/ChatRuntimeProvider.tsx |
Threads event.citations through addInferenceResponse for both segment and done events; switches import.meta.env.PROD to IS_PROD |
app/src/services/api/providerSurfacesApi.ts |
New API wrapper calling openhuman.provider_surfaces_list_queue with dual-envelope parsing |
app/src/services/chatService.ts |
Adds ChatCitation type and citations field to ChatDoneEvent and ChatSegmentEvent |
app/src/services/webviewAccountService.ts |
Dispatches a silent queue refresh after each ingest event |
app/src/store/__tests__/providerSurfaceSlice.test.ts |
Vitest unit tests for the new slice (success, failure, silent-pending paths) |
app/src/store/index.ts |
Adds providerSurfaces reducer to the store |
app/src/store/providerSurfaceSlice.ts |
New Redux slice with fetchRespondQueue thunk and silent-refresh guard |
app/src/store/threadSlice.ts |
Adds extraMetadata param to addInferenceResponse; switches import.meta.env.DEV to IS_DEV |
app/src/types/providerSurfaces.ts |
Shared TS types mirroring Rust RespondQueueItem / RespondQueueListResponse |
app/src/utils/config.ts |
Exports IS_PROD and CONSUMER_FIRST_SESSION_ENABLED feature flag |
Actionable comments (4)
Major
1. app/src/store/providerSurfaceSlice.ts:54-57 — rejected status is never cleared on next silent success
When a silent refresh is in flight, the pending reducer leaves state.status untouched (correctly). But when that silent request succeeds, fulfilled sets state.status = 'succeeded' without caring whether the previous state was 'failed'. That path is fine. The problem is the mirror case: if the first (non-silent) fetch fails, the UI shows the error panel. The next silent poll arrives 10 s later. Its pending action is suppressed (good), but if it then fails again, rejected overwrites status to 'failed' — including for silent calls — causing a visible error flash on every background retry. Conversely, a successful silent recovery after failure correctly clears the error because fulfilled always sets status = 'succeeded'. The asymmetry is that silent failures still mark the slice as 'failed', which triggers the error state in the panel even though the user never initiated that request.
// before
.addCase(fetchRespondQueue.rejected, (state, action) => {
state.status = 'failed';
state.error = (action.payload as string) ?? 'Failed to load provider respond queue';
});
// after — silent failures don't thrash the visible error state
.addCase(fetchRespondQueue.rejected, (state, action) => {
if (!action.meta.arg?.silent) {
state.status = 'failed';
state.error = (action.payload as string) ?? 'Failed to load provider respond queue';
}
// silent failures: leave status/error as-is; next successful poll will clear them
});2. app/src/services/api/providerSurfacesApi.ts:11-22 — envelope parser silently swallows RPC errors as empty queue
parseQueueEnvelope returns EMPTY_QUEUE for any shape mismatch — including when the Rust core returns an ApiEnvelope with error set and no data. A core error (e.g. mutex poisoned, serialization failure) is therefore indistinguishable from an empty queue, so the queue resets to 0 items and the error state in the Redux slice is never reached. The caller in the thunk catches thrown errors, so the fix is to throw rather than silently fall back.
// after
function parseQueueEnvelope(raw: unknown): RespondQueueList {
if (!raw || typeof raw !== 'object') {
throw new Error('provider_surfaces_list_queue: unexpected empty response');
}
const envelope = raw as ProviderSurfacesQueueEnvelope;
// Surface any server-side error reported in the envelope
const topLevel = envelope as { error?: { message?: string } };
if (topLevel.error) {
throw new Error(topLevel.error.message ?? 'Core RPC error');
}
const candidate = envelope.result?.data ?? envelope.data;
if (!candidate || !Array.isArray(candidate.items) || typeof candidate.count !== 'number') {
// Legitimate empty-result: data absent but no error — treat as empty queue
return EMPTY_QUEUE;
}
return candidate;
}Minor / refactor
3. app/src/components/accounts/RespondQueuePanel.tsx:50 — magic pixel offset in calc() is fragile
The scrollable body uses h-[calc(100%-57px)]. The 57 px value is the measured height of the header (py-3 top + py-3 bottom + border + text lines), but it isn't derived from anything — it will silently break if the header text size, padding, or font changes. The standard Tailwind fix is a flex column with the header as flex-none and the list as flex-1 overflow-y-auto:
// before
<aside className="w-80 flex-none border-l border-stone-200 bg-white">
<div className="flex items-center justify-between border-b border-stone-100 px-4 py-3">
{/* header */}
</div>
<div className="h-[calc(100%-57px)] overflow-y-auto px-3 py-3">
// after
<aside className="flex w-80 flex-none flex-col border-l border-stone-200 bg-white">
<div className="flex flex-none items-center justify-between border-b border-stone-100 px-4 py-3">
{/* header */}
</div>
<div className="flex-1 overflow-y-auto px-3 py-3">4. app/src/pages/conversations/components/CitationChips.tsx:23-34 — <details>/<summary> inside a message list has no aria-label and the summary text may be cryptic
The citation.key is an opaque memory key (e.g. mem:linkedin:acct-1:thread-1). Screen readers will announce the raw key plus the score percentage. Adding aria-label on the <summary> and a visible namespace badge would improve usability with minimal effort:
// before
<summary
className="list-none cursor-pointer rounded-full border border-stone-300 bg-stone-100 px-2 py-0.5 text-[10px] text-stone-600 hover:bg-stone-200"
title={title}>
{citation.key}
{scoreLabel}
</summary>
// after
<summary
className="list-none cursor-pointer rounded-full border border-stone-300 bg-stone-100 px-2 py-0.5 text-[10px] text-stone-600 hover:bg-stone-200"
aria-label={title}
title={title}>
{citation.namespace ?? citation.key}
{scoreLabel}
</summary>Nitpicks (3)
app/src/store/__tests__/providerSurfaceSlice.test.ts:28-29— test for 'stores queue payload on success' dispatchesfetchRespondQueue.pending('', undefined)which hits the non-silent branch and sets status to'loading'. This is the correct assertion, but once item 1 above is fixed (silent failures), you should add a 4th test: silent rejection does not change status from its current value.app/src/components/accounts/RespondQueuePanel.tsx:68—items.slice(0, 30)is a client-side cap but the store can hold up to 500 items (RustMAX_QUEUE_ITEMS). If the panel cap is intentional, document it with a comment. If not, consider removing the slice and relying on virtual scrolling or server-side pagination.app/src/utils/config.ts:42-43—CONSUMER_FIRST_SESSION_ENABLEDis added but never referenced anywhere in this PR's diff. If this flag belongs to a different feature branch, moving it there would keepconfig.tsclean. If it belongs here (for the Accounts layout change gating), wire it up or document why it is currently unused.
Verified / looks good
- The dual-envelope parser in
providerSurfacesApi.tscorrectly handles both the no-logs (bareApiEnvelope) and with-logs ({ result: ApiEnvelope, logs }) shapes thatRpcOutcome::into_cli_compatible_jsoncan produce. fetchRespondQueue's silent-pending guard correctly suppresses the loading flicker for background polls.- Citations are validated at the call site in
Conversations.tsxbefore being passed toCitationChips, avoiding runtime crashes from malformed socket payloads. - The 10 s polling interval in
Accounts.tsxis cleaned up via theuseEffectreturn value (no leak on unmount). - The
ChatCitationtype inchatService.tsstructurally matchesMessageCitationinCitationChips.tsx— no hidden field mismatch. IS_PROD/IS_DEVmigration fromimport.meta.envtoconfig.tsis consistent with the CLAUDE.md convention.RespondQueueItemTypeScript type structurally mirrors the RustRespondQueueItemstruct (all fields, optional/required alignment, camelCase on TS side which serde maps correctly from snake_case).
| .addCase(fetchRespondQueue.rejected, (state, action) => { | ||
| state.status = 'failed'; | ||
| state.error = (action.payload as string) ?? 'Failed to load provider respond queue'; | ||
| }); |
There was a problem hiding this comment.
Major: silent failures still thrash the error state
When a background (silent) poll fails, this branch sets state.status = 'failed' unconditionally, which causes the error panel to flash for the user even though they never triggered this request. The pending case already suppresses the loading flicker for silent calls; the rejected case should mirror that:
.addCase(fetchRespondQueue.rejected, (state, action) => {
if (!action.meta.arg?.silent) {
state.status = 'failed';
state.error = (action.payload as string) ?? 'Failed to load provider respond queue';
}
// silent failures: leave status/error as-is; a subsequent successful poll will clear
});Also add a Vitest case: 'silent rejection does not change status'.
| .addCase(fetchRespondQueue.rejected, (state, action) => { | ||
| state.status = 'failed'; | ||
| state.error = (action.payload as string) ?? 'Failed to load provider respond queue'; | ||
| }); |
There was a problem hiding this comment.
Major: silent rejections still thrash the error state
The pending case correctly suppresses the loading flicker for silent refreshes, but the rejected case always writes status = 'failed' regardless of whether the call was silent. This means a background 10-second poll failure will flash the error panel over the user's current view even though they never triggered a reload.
Suggested fix — mirror the silent guard from the pending case:
.addCase(fetchRespondQueue.rejected, (state, action) => {
if (!action.meta.arg?.silent) {
state.status = 'failed';
state.error = (action.payload as string) ?? 'Failed to load provider respond queue';
}
// silent failures leave status/error untouched; the next successful poll will resolve them
});Also worth adding a test case: 'silent rejection does not change status'.
| return EMPTY_QUEUE; | ||
| } | ||
| return candidate; | ||
| } |
There was a problem hiding this comment.
Major: envelope parser silently swallows core-side RPC errors
When the Rust core returns an ApiEnvelope with an error field (e.g. mutex poisoned, deserialization failure) and data absent, parseQueueEnvelope returns EMPTY_QUEUE — the queue silently resets to 0 items and the Redux 'failed' state is never reached. Network-unreachable and HTTP-error cases are already surfaced (they throw before reaching this function), but this leaves Rust-reported logical errors invisible.
Suggested fix — check for envelope.error before the candidate lookup:
function parseQueueEnvelope(raw: unknown): RespondQueueList {
if (!raw || typeof raw !== 'object') {
throw new Error('provider_surfaces_list_queue: unexpected empty response');
}
const envelope = raw as ProviderSurfacesQueueEnvelope & { error?: { message?: string } };
if (envelope.error) {
throw new Error(envelope.error.message ?? 'Core RPC returned an error');
}
const candidate = envelope.result?.data ?? envelope.data;
if (!candidate || !Array.isArray(candidate.items) || typeof candidate.count !== 'number') {
return EMPTY_QUEUE; // no error + no data = legitimately empty
}
return candidate;
}
| Refresh | ||
| </button> | ||
| </div> | ||
| <div className="h-[calc(100%-57px)] overflow-y-auto px-3 py-3"> |
There was a problem hiding this comment.
Minor: magic pixel offset in calc() is fragile
The 57px literal is the current measured height of the header div (two py-3 paddings + border + two lines of text), but it isn't derived from anything structural. If the font size, padding, or header content changes, the scrollable area will clip or show a gap with no compiler error.
Preferred fix — flex column layout that fills the remaining height automatically:
// before
<aside className="w-80 flex-none border-l border-stone-200 bg-white">
<div className="flex items-center justify-between border-b border-stone-100 px-4 py-3">
...
<div className="h-[calc(100%-57px)] overflow-y-auto px-3 py-3">
// after
<aside className="flex w-80 flex-none flex-col border-l border-stone-200 bg-white">
<div className="flex flex-none items-center justify-between border-b border-stone-100 px-4 py-3">
...
<div className="flex-1 overflow-y-auto px-3 py-3">
| <details key={citation.id} className="group"> | ||
| <summary | ||
| className="list-none cursor-pointer rounded-full border border-stone-300 bg-stone-100 px-2 py-0.5 text-[10px] text-stone-600 hover:bg-stone-200" | ||
| title={title}> |
There was a problem hiding this comment.
Minor / accessibility: summary text is an opaque memory key; no aria-label
citation.key is an internal memory address (e.g. mem:linkedin:acct-1:thread-1). Screen readers will announce the raw key plus a percentage, which is not useful. citation.namespace is more human-readable and is optional — falling back to key is fine.
Also: <summary> has a title attribute but no aria-label. The title attribute is tooltip-only and not reliably announced by screen readers; aria-label is needed for accessible description.
// after
<summary
className="list-none cursor-pointer rounded-full border border-stone-300 bg-stone-100 px-2 py-0.5 text-[10px] text-stone-600 hover:bg-stone-200"
aria-label={title}
title={title}>
{citation.namespace ?? citation.key}
{scoreLabel}
</summary>
senamakel
left a comment
senamakel
left a comment
There was a problem hiding this comment.
Thanks for this — structure looks solid and follows project conventions. Two blocking issues from the inline review that need fixing before merge:
-
Silent poll failures flash the error panel (
app/src/store/providerSurfaceSlice.ts:54-57). Thependingcase correctly suppresses the loading flicker whenaction.meta.arg?.silent, but therejectedcase unconditionally setsstatus = 'failed'. Every 10s background poll failure will flicker the error state at the user. Mirror the pending-case guard in rejected. -
Core RPC errors are silently swallowed (
app/src/services/api/providerSurfacesApi.ts:11-22).parseQueueEnvelopefalls back toEMPTY_QUEUEon any shape mismatch, which includesApiEnvelope { error: ..., data: None }coming back from the Rust core. The user sees the queue silently reset to 0 items and Redux never learns about the failure. Checkenvelope.errorand throw before the fallback.
Also a few non-blocking items inline (magic-pixel height, a11y on CitationChips, a stray CONSUMER_FIRST_SESSION_ENABLED that looks like it belongs on another branch). Happy to approve after the two majors are addressed.
3 participants
Depends on: #803
...