-
Notifications
You must be signed in to change notification settings - Fork 4
feat: new LogViewer design #315
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a connection-status callback to the LogViewer, reworks its loading/error overlays and UI, and refactors services page layout into a sidebar + main area with parent-managed connectionStatus state wired to the LogViewer. Changes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@dashboard/app/`(private)/devices/[serial]/services/LogViewer.tsx:
- Around line 102-112: The useEffect reporting connection status can get stuck
on "connecting" if the WebSocket closes before onopen; update the WebSocket
close handler so it clears the connecting flag (e.g., call
setIsConnecting(false)) inside ws.onclose (and optionally ws.onerror) in
LogViewer.tsx so isConnecting is reset and onStatusChange will emit
"disconnected" instead of staying "connecting".
In `@dashboard/app/`(private)/devices/[serial]/services/page.tsx:
- Around line 21-29: When a new service is selected the UI can show stale
connectionStatus; update handleSelectService to also reset connectionStatus by
calling setConnectionStatus("connecting") (in addition to
setSelectedService(serviceName)) so the header shows the fresh state while
LogViewer updates; locate the connectionStatus state and the handleSelectService
function and add the setConnectionStatus call there.
🧹 Nitpick comments (2)
dashboard/app/(private)/devices/[serial]/services/LogViewer.tsx (1)
115-155: Make the copy button visible on keyboard focus.It’s currently hover-only; keyboard users can land on an invisible control. Consider revealing it on focus and adding a focus-visible style.
♿ Suggested tweak
- className="absolute top-2 right-2 p-1.5 bg-gray-800 hover:bg-gray-700 text-gray-400 hover:text-white rounded transition-colors opacity-0 group-hover:opacity-100 cursor-pointer" + className="absolute top-2 right-2 p-1.5 bg-gray-800 hover:bg-gray-700 text-gray-400 hover:text-white rounded transition-colors opacity-0 group-hover:opacity-100 group-focus-within:opacity-100 cursor-pointer focus-visible:outline focus-visible:outline-2 focus-visible:outline-offset-2 focus-visible:outline-gray-400"dashboard/app/(private)/devices/[serial]/services/page.tsx (1)
136-155: Add accessible status/close labeling.The status dot is color-only, and the close button is icon-only. Consider adding an aria-label (or sr-only text) so screen readers can identify them.
♿ Suggested tweak
<div className={`w-2 h-2 rounded-full ${ connectionStatus === "connecting" ? "bg-yellow-400 animate-pulse" : connectionStatus === "connected" ? "bg-green-500" : "bg-gray-400" }`} + role="status" + aria-label={`Connection status: ${connectionStatus}`} /> ... <button onClick={handleCloseLogs} className="p-1 text-gray-400 hover:text-gray-600 hover:bg-gray-200 rounded transition-colors cursor-pointer" + aria-label="Close log viewer" >
No description provided.