chore: address PR #3262 review feedback

[joeyorlando]

Summary

Addresses review feedback from PR #3262:

• localStorage error handling: Wrap all localStorage.getItem/setItem/removeItem calls in try-catch to handle QuotaExceededError and private browsing restrictions
• message-thread perf: Precompute lastAssistantMessageIndex with useMemo instead of creating a reversed array copy per message per render
• KB tool description length: Truncate knowledge base names list at 500 chars to prevent unbounded tool description length
• Confluence connector failures: Add missing failures: this.flushFailures() to batch yields (matching GitHub/GitLab pattern)
• Jira connector failures: Same fix — refactor buildBatch to accept and include failures
• Jira error extraction depth limit: Add max depth of 5 to extractJiraErrorDetails recursion to prevent stack overflow from circular error references
• Citation error logging: Log console.warn on citation extraction failure instead of silently swallowing
• E2E test guard: Add descriptive assertion message for kbTool null check
• Placeholder consistency: Match edit connector dialog placeholder to create dialog

[claude]

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

---

PR #3266 Review

• Read repository CLAUDE.md and guidelines
• Review backend changes (connectors, mcp-gateway utils)
• Review frontend changes (chat, model-selector, message-thread, citations, preferences)
• Review test changes (e2e tests, unit tests)
• Investigate failing E2E tests
• Post comprehensive review

---

Overall this is a solid batch of fixes that addresses each review item cleanly. The backend changes are straightforward and correct. The frontend model-initialization rework is more complex but the approach is sound — extracting resolveAutoSelectedModel as a pure testable function and using the modelSelectorWasOpenRef / modelInitializedRef pair to distinguish user-initiated vs. auto-triggered model changes is a good pattern. A few items worth discussing:

---

Backend

Confluence connector (confluence-connector.ts:201) — Adding failures: this.flushFailures() to the batch yield aligns with the GitHub/GitLab pattern. Clean fix.

Jira connector (jira-connector.ts) — Refactoring buildBatch to use an object parameter is a good improvement and aligns with the repo's coding conventions (>2 params → object). The depth-limited recursion in extractJiraErrorDetails (max depth 5) is appropriate for preventing stack overflow on circular error references.

KB tool description truncation (mcp-gateway.utils.ts:1028-1033) — The 500-char truncation works. One minor note: the truncated path loses the trailing period (. Available knowledge bases: ${kbList.slice(0, 500)}...) while the non-truncated path has one (. Available knowledge bases: ${kbList}.). This is fine — the ... is a clear enough delimiter — but worth noting for consistency.

Frontend

message-thread perf (message-thread.tsx:70-75) — Good optimization. The useMemo with a backward linear scan replaces creating a reversed array copy per message per render. The dependency on [messages] is correct.

Citation error logging (knowledge-graph-citations.tsx:85-86) — console.warn for citation extraction failures is appropriate for debugging. Note: the repo's CLAUDE.md recommends using the logger instance for backend logging, but for frontend console.warn is standard practice.

Placeholder consistency (edit-connector-dialog.tsx) — Matches the create dialog. Trivial but correct.

localStorage error handling (use-chat-preferences.ts) — All get/set/remove calls wrapped in try-catch. The removeItem calls also get try-catch, which is thorough (though removeItem is unlikely to throw QuotaExceededError, private browsing restrictions could still apply). Well done.

Issues / Questions

1. resolveInitialModel is dead code — resolveInitialModel in use-chat-preferences.ts:176 is exported and has tests (use-chat-preferences.test.ts:79-181) but is not used anywhere in the application. The page.tsx useEffect (lines 287-370) implements its own inline version of the same logic with the additional org-default step. This means:

• The function and its tests add maintenance burden without value
• The priority chain documented in resolveInitialModel (localStorage > agent > fallback) diverges from page.tsx (localStorage > agent > org default > fallback)

Consider either: (a) deleting resolveInitialModel and its tests as dead code, or (b) refactoring page.tsx to use it (adding org-default support).

Fix this →

2. providerToLogoProvider export — The const was changed from non-exported to export (model-selector.tsx:101). This is correct since prompt-input.tsx imports it, but it's unrelated to the review feedback items. Worth noting it was likely already exported in a prior commit (from PR #3262).

3. E2E test failures from earlier CI run — The 3 failures reported in the earlier run (831c499) were:

• chat-localstorage.spec.ts — Two tests for model/agent persistence. These likely failed because 831c499 didn't yet have the modelSelectorWasOpenRef mechanism — the model persistence path changed such that saveModel is only called when the dialog is explicitly opened. The E2E test opens the dialog and picks a model, so it should work with the latest fix. The subsequent commit (9f217a9) addresses this.
• chat.spec.ts › Chat-UI-openai — This test also opens the model selector dialog and selects a model before sending a message. Same root cause — the auto-select logic was interfering with explicit selection.

The latest push should fix these. CI is still running for the latest commit.

4. Missing autoSelectKeyForProvider in effect deps — In page.tsx, the autoSelectKeyForProvider function is defined inside the useEffect callback, so it's not a closure dependency issue. However, this function reads initialApiKeyId and chatApiKeys from the closure. chatApiKeys is listed as a dependency, but initialApiKeyId is too — both are in the dependency array. This is correct.

5. useEffect dependency array dropped initialModel — The model-initialization useEffect (line 287) removed initialModel from its dependency array and uses modelInitializedRef instead. This is intentional to prevent the race condition where ModelSelector's auto-select sets initialModel, causing the effect to re-run and short-circuit via the old if (initialModel) return check. The ref-based approach is sound.

Tests

Unit tests (use-chat-preferences.test.ts) — The resolveAutoSelectedModel test suite is thorough. The regression test (line 252) specifically documents the race condition being fixed. Well-written tests.

E2E test guard (mcp-gateway.spec.ts:1875) — Adding a descriptive assertion message and ! non-null assertions with biome-ignore comments is a reasonable approach. The toBeDefined() assertion guards the subsequent uses.

Summary

The PR cleanly addresses all 9 review feedback items. The main suggestion is to clean up the dead resolveInitialModel function. Everything else looks good.

[London-Cat]

📊 Reputation Summary

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

---

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

[github-actions]

Playwright test results

 3 failed
 594 passed
 6 flaky
 22 skipped

Details

 625 tests across 62 suites
 3 minutes, 32 seconds
 9f217a9

Failed tests

chromium › ui/chat-auth-required.spec.ts › Chat - Auth Required Tool › renders AuthRequiredTool when tool call fails due to missing credentials
chromium › ui/chat-localstorage.spec.ts › Chat localStorage persistence › persists selected model 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/chat-settings.spec.ts › LLM Provider API Keys Access Control › member should be able to read LLM provider API keys
api › api/llm-settings.spec.ts › LLM Settings API › should read back LLM settings after update
api › api/mcp-gateway.spec.ts › MCP Gateway - External MCP Server Tests › should list internal-dev-test-server tool
firefox › ui/auth-redirect.spec.ts › Authentication redirect flows › redirectTo parameter preserves original URL after sign-in
chromium › ui/chat-settings.spec.ts › Provider Settings - API Keys › Admin can create, update, and delete an API key

Skipped tests

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