-
Notifications
You must be signed in to change notification settings - Fork 84
Add log viewer overlay #1016
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
Add log viewer overlay #1016
Conversation
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.
Pull request overview
This PR adds a log viewer overlay feature that allows users to view jmwalletd logs from the application's footer. The implementation follows the established orderbook overlay pattern for consistency.
Changes:
- Added a "Logs" button to the footer that opens a full-screen overlay
- Created a new LogsOverlay component that displays logs in a dialog
- Created a LogsContent component that handles fetching, displaying, and managing log file content
- Added translation key for the new logs button
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/i18n/locales/en/translation.json | Added "logs" translation key to footer section |
| src/components/layout/Layout.tsx | Added state management for logs overlay and integrated LogsOverlay component |
| src/components/layout/AppFooter.tsx | Added logs button with ScrollTextIcon and onClick handler |
| src/components/LogsOverlay.tsx | New component implementing full-screen dialog for log viewing |
| src/components/LogsContent.tsx | New component implementing log fetching, display, refresh, and download functionality |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (enabled && !isInitialized) { | ||
| const abortCtrl = new AbortController() | ||
| refresh(abortCtrl.signal).finally(() => { | ||
| if (abortCtrl.signal.aborted) return | ||
| setIsInitialized(true) | ||
| }) | ||
| } | ||
|
|
Copilot
AI
Jan 22, 2026
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.
This code is executing side effects (calling refresh and setting state) during the render phase. This violates React's rules and can cause issues such as multiple fetch calls, memory leaks if the component unmounts before the promise resolves, and inconsistent behavior. This should be moved to a useEffect hook. For reference, see the OrderbookContent component which uses React Query's enabled prop to handle conditional fetching properly.
| if (enabled && !isInitialized) { | |
| const abortCtrl = new AbortController() | |
| refresh(abortCtrl.signal).finally(() => { | |
| if (abortCtrl.signal.aborted) return | |
| setIsInitialized(true) | |
| }) | |
| } | |
| useEffect(() => { | |
| if (!enabled || isInitialized) { | |
| return | |
| } | |
| const abortCtrl = new AbortController() | |
| refresh(abortCtrl.signal).finally(() => { | |
| if (!abortCtrl.signal.aborted) { | |
| setIsInitialized(true) | |
| } | |
| }) | |
| return () => { | |
| abortCtrl.abort() | |
| } | |
| }, [enabled, isInitialized, refresh]) |
| disabled={isScrolledToLogBottom} | ||
| size="icon" | ||
| onClick={scrollToLogBottom} | ||
| title={/* TODO: i18n */ 'Scroll to bottom'} |
Copilot
AI
Jan 22, 2026
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.
This hardcoded string should be internationalized. Add a translation key such as 'logs.scroll_to_bottom' to the translation file and use t('logs.scroll_to_bottom') instead of the hardcoded English text.
| title={/* TODO: i18n */ 'Scroll to bottom'} | |
| title={t('logs.scroll_to_bottom')} |
| const handleRefresh = useCallback(async () => { | ||
| if (isLoadingRefresh) return | ||
|
|
||
| setIsLoadingRefresh(true) | ||
| const abortCtrl = new AbortController() | ||
|
|
||
| try { | ||
| await refresh(abortCtrl.signal).then(() => delayedPromise(210)) | ||
| } finally { | ||
| setIsLoadingRefresh(false) | ||
| } | ||
| }, [isLoadingRefresh, refresh]) |
Copilot
AI
Jan 22, 2026
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.
The AbortController created in handleRefresh is never aborted if the component unmounts while a refresh is in progress. This could lead to state updates on an unmounted component. Consider adding cleanup logic to abort the request when the component unmounts, or move this logic into a useEffect with proper cleanup.
|
Nice. I'll move logs related components to their own dir, remove duplicate code and add a storybook story for the logs content. Thanks @kunal-595 🙏 |
Adds a logs button in the footer that opens a full-screen overlay to view jmwalletd logs. Follows the same pattern as the orderbook overlay