Various UI improvements #176
Conversation
WalkthroughThe changes expand favicon and metadata support in the HTML head, add a new web app manifest file with icon and display settings, and enhance EventSource log streaming in the API context with automatic retry logic and exponential backoff on connection errors. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant APIProvider
Client->>APIProvider: enableProxyLogs/onMessage
APIProvider->>APIProvider: connect() (attempt 1)
APIProvider->>APIProvider: open EventSource
Note right of APIProvider: If error occurs
APIProvider->>APIProvider: close EventSource
APIProvider->>APIProvider: wait (exponential backoff)
APIProvider->>APIProvider: reconnect (up to 3 attempts)
APIProvider-->>Client: onmessage handler called on new message
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
ui/src/contexts/APIProvider.tsx (2)
122-141: Consider adding retry logic to enableModelUpdates for consistency.The
enableModelUpdatesfunction lacks the same retry logic implemented for log streaming, which could lead to inconsistent behavior when the model status SSE connection fails.Consider applying similar retry logic to this function for consistent error handling across all EventSource connections.
143-149: Add cleanup for retry timeouts in useEffect.The cleanup effect should also clear any pending retry timeouts to prevent memory leaks.
useEffect(() => { return () => { proxyEventSource.current?.close(); upstreamEventSource.current?.close(); modelStatusEventSource.current?.close(); + if (proxyRetryTimeoutRef.current) { + clearTimeout(proxyRetryTimeoutRef.current); + } + if (upstreamRetryTimeoutRef.current) { + clearTimeout(upstreamRetryTimeoutRef.current); + } }; }, []);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
ui/public/apple-touch-icon.pngis excluded by!**/*.pngui/public/favicon-96x96.pngis excluded by!**/*.pngui/public/favicon.icois excluded by!**/*.icoui/public/favicon.svgis excluded by!**/*.svgui/public/web-app-manifest-192x192.pngis excluded by!**/*.pngui/public/web-app-manifest-512x512.pngis excluded by!**/*.png
📒 Files selected for processing (3)
ui/index.html(1 hunks)ui/public/site.webmanifest(1 hunks)ui/src/contexts/APIProvider.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests
🔇 Additional comments (3)
ui/public/site.webmanifest (1)
6-6: Verify the existence of referenced icon files.The manifest references specific icon files that may not exist in the repository. Ensure these files are present in the public directory.
#!/bin/bash # Description: Check if the referenced icon files exist in the public directory # Expected: Files should exist for the manifest to work properly fd -t f "web-app-manifest-192x192.png" ui/public/ fd -t f "web-app-manifest-512x512.png" ui/public/Also applies to: 12-12
ui/index.html (1)
6-10: Excellent comprehensive icon support for cross-platform compatibility.The addition of multiple icon formats (PNG, SVG, ICO, Apple touch icon) and the web manifest reference provides excellent support across different devices and platforms.
#!/bin/bash # Description: Verify all referenced icon files exist # Expected: All icon files should be present for proper display echo "Checking for icon files..." fd -t f "favicon-96x96.png" ui/public/ fd -t f "favicon.svg" ui/public/ fd -t f "favicon.ico" ui/public/ fd -t f "apple-touch-icon.png" ui/public/ fd -t f "site.webmanifest" ui/public/ui/src/contexts/APIProvider.tsx (1)
119-119: Dependency array inconsistency resolved.Good improvement removing the unnecessary
upstreamEventSourcedependency from the array, leaving onlyhandleUpstreamMessage.
| let retryCount = 0; | ||
| const maxRetries = 3; | ||
| const initialDelay = 1000; // 1 second | ||
|
|
||
| const connect = () => { | ||
| const eventSource = new EventSource("/logs/streamSSE/upstream"); | ||
|
|
||
| eventSource.onmessage = handleUpstreamMessage; | ||
| eventSource.onerror = () => { | ||
| eventSource.close(); | ||
| if (retryCount < maxRetries) { | ||
| retryCount++; | ||
| const delay = initialDelay * Math.pow(2, retryCount - 1); | ||
| setTimeout(connect, delay); | ||
| } | ||
| }; | ||
|
|
||
| upstreamEventSource.current = eventSource; | ||
| }; | ||
|
|
||
| connect(); |
There was a problem hiding this comment.
Apply the same retry logic fixes to enableUpstreamLogs.
The upstream logs function has the same scope and cleanup issues as the proxy logs function.
Apply similar fixes to enableUpstreamLogs as suggested for enableProxyLogs above, using upstreamRetryCountRef and upstreamRetryTimeoutRef.
🤖 Prompt for AI Agents
In ui/src/contexts/APIProvider.tsx around lines 93 to 113, the retry logic in
the connect function for upstream logs uses local variables for retry count and
timeout, causing scope and cleanup issues. Refactor this by replacing the local
retryCount with a ref variable upstreamRetryCountRef to persist retry attempts
across calls, and store the timeout ID in upstreamRetryTimeoutRef to allow
proper cleanup. Update the error handler to increment
upstreamRetryCountRef.current, calculate the delay, and assign the timeout ID to
upstreamRetryTimeoutRef.current, ensuring retries are managed consistently and
can be cleared when needed.
| let retryCount = 0; | ||
| const maxRetries = 3; | ||
| const initialDelay = 1000; // 1 second | ||
|
|
||
| const connect = () => { | ||
| const eventSource = new EventSource("/logs/streamSSE/proxy"); | ||
|
|
||
| eventSource.onmessage = handleProxyMessage; | ||
| eventSource.onerror = () => { | ||
| eventSource.close(); | ||
| if (retryCount < maxRetries) { | ||
| retryCount++; | ||
| const delay = initialDelay * Math.pow(2, retryCount - 1); | ||
| setTimeout(connect, delay); | ||
| } | ||
| }; | ||
|
|
||
| proxyEventSource.current = eventSource; | ||
| }; | ||
|
|
||
| connect(); |
There was a problem hiding this comment.
Fix retry logic scope and cleanup issues.
The retry implementation has several issues:
retryCountis scoped within the callback and resets each time the function is called- No cleanup of
setTimeoutwhen the component unmounts or whenenabled=false retryCountis not reset when connection succeeds
const enableProxyLogs = useCallback(
(enabled: boolean) => {
+ // Clear any pending retry timeouts
+ if (proxyRetryTimeoutRef.current) {
+ clearTimeout(proxyRetryTimeoutRef.current);
+ proxyRetryTimeoutRef.current = null;
+ }
+
if (enabled) {
- let retryCount = 0;
const maxRetries = 3;
const initialDelay = 1000; // 1 second
const connect = () => {
const eventSource = new EventSource("/logs/streamSSE/proxy");
eventSource.onmessage = handleProxyMessage;
+ eventSource.onopen = () => {
+ proxyRetryCountRef.current = 0; // Reset on successful connection
+ };
eventSource.onerror = () => {
eventSource.close();
- if (retryCount < maxRetries) {
- retryCount++;
- const delay = initialDelay * Math.pow(2, retryCount - 1);
- setTimeout(connect, delay);
+ if (proxyRetryCountRef.current < maxRetries) {
+ proxyRetryCountRef.current++;
+ const delay = initialDelay * Math.pow(2, proxyRetryCountRef.current - 1);
+ proxyRetryTimeoutRef.current = setTimeout(connect, delay);
}
};
proxyEventSource.current = eventSource;
};
connect();
} else {
proxyEventSource.current?.close();
proxyEventSource.current = null;
+ proxyRetryCountRef.current = 0;
}
},
[handleProxyMessage]
);You'll also need to add the refs at the component level:
const proxyEventSource = useRef<EventSource | null>(null);
const upstreamEventSource = useRef<EventSource | null>(null);
+const proxyRetryCountRef = useRef(0);
+const proxyRetryTimeoutRef = useRef<NodeJS.Timeout | null>(null);
+const upstreamRetryCountRef = useRef(0);
+const upstreamRetryTimeoutRef = useRef<NodeJS.Timeout | null>(null);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let retryCount = 0; | |
| const maxRetries = 3; | |
| const initialDelay = 1000; // 1 second | |
| const connect = () => { | |
| const eventSource = new EventSource("/logs/streamSSE/proxy"); | |
| eventSource.onmessage = handleProxyMessage; | |
| eventSource.onerror = () => { | |
| eventSource.close(); | |
| if (retryCount < maxRetries) { | |
| retryCount++; | |
| const delay = initialDelay * Math.pow(2, retryCount - 1); | |
| setTimeout(connect, delay); | |
| } | |
| }; | |
| proxyEventSource.current = eventSource; | |
| }; | |
| connect(); | |
| // At the top of your component (alongside other refs) | |
| const proxyEventSource = useRef<EventSource | null>(null); | |
| const upstreamEventSource = useRef<EventSource | null>(null); | |
| const proxyRetryCountRef = useRef(0); | |
| const proxyRetryTimeoutRef = useRef<NodeJS.Timeout | null>(null); | |
| const upstreamRetryCountRef = useRef(0); | |
| const upstreamRetryTimeoutRef = useRef<NodeJS.Timeout | null>(null); | |
| const enableProxyLogs = useCallback( | |
| (enabled: boolean) => { | |
| // Clear any pending retry timeouts | |
| if (proxyRetryTimeoutRef.current) { | |
| clearTimeout(proxyRetryTimeoutRef.current); | |
| proxyRetryTimeoutRef.current = null; | |
| } | |
| if (enabled) { | |
| const maxRetries = 3; | |
| const initialDelay = 1000; // 1 second | |
| const connect = () => { | |
| const eventSource = new EventSource("/logs/streamSSE/proxy"); | |
| eventSource.onmessage = handleProxyMessage; | |
| eventSource.onopen = () => { | |
| proxyRetryCountRef.current = 0; // Reset on successful connection | |
| }; | |
| eventSource.onerror = () => { | |
| eventSource.close(); | |
| if (proxyRetryCountRef.current < maxRetries) { | |
| proxyRetryCountRef.current++; | |
| const delay = initialDelay * Math.pow(2, proxyRetryCountRef.current - 1); | |
| proxyRetryTimeoutRef.current = setTimeout(connect, delay); | |
| } | |
| }; | |
| proxyEventSource.current = eventSource; | |
| }; | |
| connect(); | |
| } else { | |
| proxyEventSource.current?.close(); | |
| proxyEventSource.current = null; | |
| proxyRetryCountRef.current = 0; | |
| } | |
| }, | |
| [handleProxyMessage] | |
| ); |
🤖 Prompt for AI Agents
In ui/src/contexts/APIProvider.tsx around lines 61 to 81, the retryCount
variable is currently scoped inside the connect function causing it to reset on
each retry, and there is no cleanup for the setTimeout timers on component
unmount or when enabled becomes false. To fix this, move retryCount and the
timeout ID to refs at the component level to preserve their values across
renders. Reset retryCount to zero when the connection succeeds. Also, store the
timeout ID from setTimeout in a ref and clear it in a cleanup function to
prevent memory leaks when the component unmounts or retries are disabled.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ui/public/site.webmanifest (1)
2-3: App names have been correctly updated.The placeholder names have been properly updated to "llama-swap" as previously suggested, ensuring consistency with the application branding.
🧹 Nitpick comments (2)
ui/public/site.webmanifest (2)
18-21: PWA configuration looks good.The theme color, background color, and display mode configuration follows PWA best practices. The "standalone" display mode will provide a native app-like experience.
Consider whether white colors align with your application's branding - you may want to use brand-specific colors for better visual consistency.
4-17: Enhance icon configuration for better PWA compatibility.Consider the following improvements:
- Add "any" purpose alongside "maskable" for broader device compatibility:
{ "src": "/web-app-manifest-192x192.png", "sizes": "192x192", "type": "image/png", - "purpose": "maskable" + "purpose": "maskable any" }, { "src": "/web-app-manifest-512x512.png", "sizes": "512x512", "type": "image/png", - "purpose": "maskable" + "purpose": "maskable any" }
- Verify that the referenced icon files exist at the specified paths.
#!/bin/bash # Verify that the referenced icon files exist echo "Checking for icon files..." ls -la ui/public/web-app-manifest-192x192.png ui/public/web-app-manifest-512x512.png 2>/dev/null || echo "Icon files not found"
Summary by CodeRabbit
New Features
Bug Fixes