chore: sticky footer dialogs, knowledge sources tool improvements, chat fixes

[joeyorlando]

Summary

• DialogStickyFooter: Fix sticky footer positioning in identity provider, connector, manage-users, and agent dialogs so footer buttons stay visible when content scrolls
• Knowledge sources tool: Rename query_knowledge_base → query_knowledge_sources, build dynamic tool description with KB names and connector types, auto-sync connectors on creation, add "Sync Now" UX, hide from tool assignment dialog
• Chat improvements: Fix citation placement to show on the last assistant message after streaming (search backwards for KB tool calls), fix orphaned numbered list markers where LLM outputs numbers on their own line
• Connector checkpoint reset: Support resetting connector sync state
• Chat localStorage: Refactor chat model/agent persistence helpers
• Structured logging: Improvements to backend logging

[claude]

Claude finished @joeyorlando's task in 4m 24s —— View job

---

PR Review: chore: sticky footer dialogs, knowledge sources tool improvements, chat fixes

Thorough review of all 61 changed files across backend, frontend, tests, and migrations. This is a large PR touching many concerns — findings organized by severity.

---

Failing E2E Tests (3 failures)

1. chat-localstorage.spec.ts — "persists selected agent to localStorage and restores on revisit"
The test expects the agent to be auto-saved to localStorage on initial page load, but looking at page.tsx, saveAgent() is only called inside user-triggered handlers (handleInitialAgentChange). The initial agent resolution effect sets initialAgentId but never calls saveAgent(). The test's storedAgent will be falsy.

2. chat-localstorage.spec.ts — "persists selected model to localStorage and restores on revisit"
Similar issue — the test may also have a timing problem where effects haven't completed before the assertion runs after navigation. Additionally, line 64 asserts the trigger button contains the raw model ID from localStorage, but the button likely displays the model name.

3. chat.spec.ts — "can send a message and receive a response from OpenAI"
The model option filter (filter({ hasText: \(${config.modelId})` })`) is brittle — it assumes the model ID appears in parentheses in the option text. If the UI format changed in this PR, this won't match. There's also no wait for the model to be persisted to localStorage before sending the message.

---

Critical / High Severity

1. use-chat-preferences.ts — No try-catch around localStorage calls (use-chat-preferences.ts:30-72)
All localStorage.setItem/getItem calls can throw QuotaExceededError or fail in private browsing. The functions silently fail but could crash the component tree.

// Current
export function saveModel(modelId: string): void {
  if (typeof window === "undefined") return;
  localStorage.setItem(CHAT_STORAGE_KEYS.selectedModel, modelId); // Can throw!
}

Fix this →

2. connector-sync.ts — Missing checkpoint reset on sync failure (connector-sync.ts:289-310)
When a sync fails mid-batch, the connector status is set to "failed" but the checkpoint is NOT reset. On the next retry, the sync resumes from the stale checkpoint, potentially missing documents between the old checkpoint and the failure point.

3. message-thread.tsx:162-169 — Inefficient last-assistant-message computation per render

const isLastAssistantMessage =
  message.role === "assistant" &&
  idx === messages.length - 1 -
    [...messages].reverse().findIndex((m) => m.role === "assistant");

This creates a new reversed array and runs findIndex for every message in the list. For a conversation with 100 messages, this runs 100 times per render, each copying and reversing the full array. Should be precomputed once via useMemo.
Fix this →

4. mcp-gateway.utils.ts:1020-1037 — Unbounded dynamic tool description length
buildKnowledgeSourcesDescription concatenates all KB names and connector types into the tool description with no length limit. With many knowledge bases, this could exceed MCP spec limits for tool descriptions.
Fix this →

5. Confluence connector missing failures field in batch yields (confluence-connector.ts:199-211)
GitHub, GitLab, and Jira connectors all yield failures: this.flushFailures() in their batch output, but the Confluence connector does not. This means Confluence item-level failures won't be tracked in sync runs, and itemFailures will accumulate in memory without being flushed.
Fix this →

---

Medium Severity

6. knowledge-graph-citations.tsx:85-86 — Silent error swallowing in citation extraction
The extractCitations function catches ALL errors with catch { continue } — no logging whatsoever. This makes debugging KB citation issues very difficult.

7. chat-api-key-selector.tsx:155-235 — Race condition in auto-selection
The hasAutoSelectedRef reset pattern creates a window where two mutations can fire: if conversationId changes → ref resets to false → main effect runs again before the first mutation completes.

8. dialog.tsx:142 — Negative bottom offset bottom-[-24px]
The DialogStickyFooter uses bottom-[-24px] to counteract DialogContent's p-6 padding. While the approach works, this is an unusual pattern — bottom-0 with adjusted margin would be more conventional and less likely to cause misalignment on edge cases.

9. connector-sync.ts:190-204 — Time budget check stops at 90%, might overshoot
The sync stops processing batches when elapsed time exceeds maxDurationMs * 0.9, but the current batch's processing and subsequent DB writes could push actual runtime to 105-110% of the limit.

10. Connector checkpoint type casting without validation (all connectors)
All connectors cast checkpoints with as ConnectorCheckpoint | null without runtime validation. If a checkpoint was corrupted or saved by a different connector type, the assertion silently succeeds but fields will be undefined at runtime.

11. jira-connector.ts:409-461 — Recursive error detail extraction with no depth limit
extractJiraErrorDetails recurses through err.cause without a depth limit. Circular error references could cause a stack overflow.

12. agent-tools-editor.tsx — query_knowledge_sources hidden from tool assignment
The filtering logic at agent-tools-editor.tsx hides the KB query tool from the assignment UI. Good UX decision, but the filter condition should have a comment explaining why it's hidden (auto-injected by the system).

---

Low Severity / Nits

13. use-chat-preferences.ts — Clean architecture. The pure-function extraction pattern with clear priority chain (localStorage > agent > fallback) is well-designed and thoroughly tested. Nice refactor.

14. DialogForm and DialogStickyFooter are well-documented with JSDoc examples (dialog.tsx:150-169). The padding counteraction comment on line 141 is helpful.

15. The KB description cache TTL of 30 seconds (mcp-gateway.utils.ts:989) is reasonable for dynamic descriptions but could be bumped to 2-5 minutes to reduce DB queries under load.

16. The migration (0175_rename-query-knowledge-base-to-query-knowledge-sources.sql) is a simple UPDATE ... SET without validation that the source row exists or a rollback path. Acceptable for a rename, but worth noting.

17. E2E test mcp-gateway.spec.ts:1878 — no null guard before accessing kbTool.description. If kbTool is undefined, the test fails with a confusing "Cannot read properties of undefined" error instead of a clear assertion message.

18. Inconsistent placeholder text between create-connector-dialog ("Engineering Jira Connector") and edit-connector-dialog ("Connector name").

19. provider-settings-api-keys.tsx:169-171 — Empty catch {} blocks with only comments. Could swallow unexpected errors.

---

Positive Highlights

• use-chat-preferences.ts — Excellent extraction of localStorage logic into pure, testable functions with comprehensive test coverage (181 lines of tests). The priority resolution chain is clean and well-documented.
• Dynamic KB tool descriptions — The buildKnowledgeSourcesDescription approach with caching is a good design for contextual tool descriptions.
• Connector checkpoint-based incremental sync — The async generator pattern for batched document processing is well-architected.
• DialogStickyFooter consistency — Applied uniformly across ~10 dialogs with good documentation.
• Citation backward search — The logic to search backwards for KB tool calls while stopping at user messages to avoid stale citations is a thoughtful approach to a tricky UX problem.
• Auto-sync on connector creation — Good UX improvement to trigger initial sync immediately.

---

Summary

Severity	Count	Key Areas
E2E Failures	3	chat-localstorage (2), chat OpenAI (1) — likely caused by missing saveAgent() call on initial load and brittle selectors
Critical/High	5	localStorage error handling, checkpoint reset on failure, perf issue in message-thread, unbounded description, Confluence missing failures
Medium	7	Silent citation errors, race conditions, time budget overshoot, type safety
Low/Nit	7	Cache TTL, migration validation, placeholder consistency

The PR's core features (sticky footer, KB tool rename, dynamic descriptions, citation placement) are well-implemented. The main concerns are the failing e2e tests (likely fixable with a saveAgent() call in the initial resolution effect) and a few robustness issues in error handling and sync state management.

[London-Cat]

📊 Reputation Summary

User	Rep	Pull Requests	Activity	Assigned	Core Reactions
joeyorlando	⚡ 2351	92✅ 7🔄 1❌	100 issues, 50 comments	6	—

---

How is the score calculated? Read about it in the Reputation Bot repository 🤖

[github-actions]

Playwright test results

 4 failed
 594 passed
 4 flaky
 23 skipped

Details

 625 tests across 62 suites
 4 minutes, 29 seconds
 a6e002b

Failed tests

api › api/chat-settings.spec.ts › LLM Provider API Keys Access Control › member should be able to read LLM provider API keys
chromium › ui/chat-localstorage.spec.ts › Chat localStorage persistence › persists selected model to localStorage and restores on revisit
chromium › ui/chat-localstorage.spec.ts › Chat localStorage persistence › persists selected agent to localStorage and restores on revisit
chromium › ui/chat.spec.ts › Chat-UI-openai › can send a message and receive a response from OpenAI

Flaky tests

api › api/built-in-agents.spec.ts › Built-In Agents API › auto-configure creates policies for tool via route
api › api/built-in-agents.spec.ts › Built-In Agents API › auto-configure triggers on individual tool assignment
webkit › ui/auth-redirect.spec.ts › Authentication redirect flows › redirectTo parameter preserves OAuth consent URL with protocol in query params
chromium › ui/chat-settings.spec.ts › Provider Settings - Virtual API Keys › Can create a virtual key from the Virtual API Keys tab

Skipped tests

api › api/chat-settings.spec.ts › LLM Provider API Keys Access Control › member should not be able to create LLM provider API keys
api › api/knowledge-bases.spec.ts › Knowledge Bases API › Connector CRUD › connectors are cascade-deleted when knowledge base is deleted
credentials-with-vault › ui/credentials-with-vault.ee.spec.ts › Test self-hosted MCP server with Readonly Vault › Test self-hosted MCP server with Vault - with prompt on installation
credentials-with-vault › ui/credentials-with-vault.ee.spec.ts › Test self-hosted MCP server with Readonly Vault › Test self-hosted MCP server with Vault - without prompt on installation
chromium › ui/chat.spec.ts › Chat-UI-gemini › can send a message and receive a response from Google
chromium › ui/chat.spec.ts › Chat-UI-cerebras › can send a message and receive a response from Cerebras
chromium › ui/chat.spec.ts › Chat-UI-cohere › can send a message and receive a response from Cohere
chromium › ui/chat.spec.ts › Chat-UI-mistral › can send a message and receive a response from Mistral
chromium › ui/chat.spec.ts › Chat-UI-perplexity › can send a message and receive a response from Perplexity
chromium › ui/chat.spec.ts › Chat-UI-groq › can send a message and receive a response from Groq
chromium › ui/chat.spec.ts › Chat-UI-xai › can send a message and receive a response from xAI
chromium › ui/chat.spec.ts › Chat-UI-openrouter › can send a message and receive a response from OpenRouter
chromium › ui/chat.spec.ts › Chat-UI-ollama › can send a message and receive a response from Ollama
chromium › ui/chat.spec.ts › Chat-UI-vllm › can send a message and receive a response from vLLM
chromium › ui/chat.spec.ts › Chat-UI-zhipuai › can send a message and receive a response from ZhipuAI
chromium › ui/chat.spec.ts › Chat-UI-deepseek › can send a message and receive a response from DeepSeek
chromium › ui/chat.spec.ts › Chat-UI-minimax › can send a message and receive a response from MiniMax
chromium › ui/dynamic-credentials.spec.ts › Verify tool calling using dynamic credentials
chromium › ui/static-credentials-management.spec.ts › Custom Self-hosted MCP Server - installation and static credentials management (vault disabled, prompt-on-installation disabled) › Admin
chromium › ui/static-credentials-management.spec.ts › Custom Self-hosted MCP Server - installation and static credentials management (vault disabled, prompt-on-installation disabled) › Editor
chromium › ui/static-credentials-management.spec.ts › Custom Self-hosted MCP Server - installation and static credentials management (vault disabled, prompt-on-installation disabled) › Member
chromium › ui/static-credentials-management.spec.ts › Verify Manage Credentials dialog shows correct other users credentials
chromium › ui/static-credentials-management.spec.ts › Verify tool calling using different static credentials