fix: server web logging running server #3408
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
apps/server-web/src/main/helpers/services/libs/server-task.tsOops! Something went wrong! :( ESLint: 8.46.0 ESLint couldn't find the config "erb" to extend from. Please check that the name of the config is correct. The config "erb" was referenced from the config file in "/apps/server-web/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThe changes in this pull request introduce a new constant Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
CodeRabbit Configuration File (
|
![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 and nitpick comments (5)
apps/server-web/src/renderer/pages/Server.tsx (2)
10-15: Consider using a more efficient log state structureThe current implementation using array spread operator (
...prev) insetLogsmight cause performance issues with large log histories. Consider implementing a circular buffer or limiting the number of stored logs.- const [logs, setLogs] = useState< - { - message: string; - type: 'error-log' | 'log'; - }[] - >([]); + const [logs, setLogs] = useState<{ + message: string; + type: 'error-log' | 'log'; + }[]>(() => []); + const MAX_LOGS = 1000; // Adjust based on requirementsThen update the log addition logic to maintain the limit:
setLogs((prev) => [ - ...prev, + ...prev.slice(-MAX_LOGS + 1), { message: arg.msg, type: 'log', }, ]);
69-73: Add user control for auto-scrolling behaviorThe current auto-scroll implementation might be disruptive if users are reading older logs. Consider adding a toggle for auto-scroll functionality.
+ const [autoScroll, setAutoScroll] = useState(true); const scrollToLast = () => { + if (!autoScroll) return; logRef.current?.scrollIntoView({ behavior: 'smooth', }); };Add a toggle in the UI:
<summary className="flex justify-between items-center font-medium cursor-pointer list-none"> <span className="p-2"> Server Logs</span> + <label className="mr-4"> + <input + type="checkbox" + checked={autoScroll} + onChange={(e) => setAutoScroll(e.target.checked)} + /> + Auto-scroll + </label> <span className="transition group-open:rotate-180">apps/server-web/src/main/helpers/services/libs/server-task.ts (1)
85-90: Consolidate server status loggingThe server status logging is spread across multiple notify calls. Consider consolidating them for better readability and maintenance.
- console.log(this.args) - this.loggerObserver.notify( - `☣︎ ${name.toUpperCase()} running on http://${this.args.DESKTOP_WEB_SERVER_HOSTNAME}:${this.args.PORT}` - ); - this.loggerObserver.notify( - `☣︎ ${name.toUpperCase()} connected to api ${this.args.GAUZY_API_SERVER_URL}` - ); + const serverInfo = { + name: name.toUpperCase(), + url: `http://${this.args.DESKTOP_WEB_SERVER_HOSTNAME}:${this.args.PORT}`, + api: this.args.GAUZY_API_SERVER_URL + }; + this.loggerObserver.notify( + `☣︎ ${serverInfo.name} running on ${serverInfo.url}\n` + + `☣︎ ${serverInfo.name} connected to api ${serverInfo.api}` + );apps/server-web/src/main/main.ts (2)
71-79: LGTM! Error logging implementation looks good.The implementation correctly handles server error logs by:
- Checking for log window existence
- Properly formatting the message
- Using the correct IPC channel and message type
Consider consolidating duplicate logging logic.
The error logging block is nearly identical to the normal logging block above it. Consider refactoring to reduce duplication.
- if (message.data[0] === LOG_TYPES.SERVER_LOG) { - if (logWindow) { - const msg = message.data.join(' '); - logWindow.webContents.send(IPC_TYPES.SERVER_PAGE, { - type: LOG_TYPES.SERVER_LOG, - msg - }); - } - } - - if (message.data[0] === LOG_TYPES.SERVER_LOG_ERROR) { - if (logWindow) { - const msg = message.data.join(' '); - logWindow.webContents.send(IPC_TYPES.SERVER_PAGE, { - type: LOG_TYPES.SERVER_LOG_ERROR, - msg - }); - } - } + const logTypes = [LOG_TYPES.SERVER_LOG, LOG_TYPES.SERVER_LOG_ERROR]; + if (logTypes.includes(message.data[0])) { + if (logWindow) { + const msg = message.data.join(' '); + logWindow.webContents.send(IPC_TYPES.SERVER_PAGE, { + type: message.data[0], + msg + }); + } + }
71-79: Consider enhancing the logging architecture.The current implementation could benefit from:
- A dedicated logging service to handle different log types
- Structured logging format for better parsing
- Log buffering when the log window is not available
Example structured logging format:
interface LogMessage { timestamp: number; type: LOG_TYPES; level: 'info' | 'error' | 'warn'; message: string; metadata?: Record<string, unknown>; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
apps/server-web/src/main/helpers/constant.ts(1 hunks)apps/server-web/src/main/helpers/services/libs/server-task.ts(3 hunks)apps/server-web/src/main/main.ts(1 hunks)apps/server-web/src/renderer/pages/Server.tsx(4 hunks)
🔇 Additional comments (1)
apps/server-web/src/main/helpers/constant.ts (1)
53-54: LGTM! Clean addition of error log type constant
The new SERVER_LOG_ERROR constant follows the existing naming pattern and integrates well with the logging system.
| {log.type === 'error-log' && ( | ||
| <span className="text-red-600">{log.message}</span> | ||
| )} | ||
| {log.type === 'log' && ( | ||
| <span className="text-white">{log.message}</span> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error boundary for log rendering
The log rendering logic should be wrapped in an error boundary to handle potential rendering failures gracefully.
Create an error boundary component:
class LogErrorBoundary extends React.Component {
state = { hasError: false };
static getDerivedStateFromError(error) {
return { hasError: true };
}
render() {
if (this.state.hasError) {
return <div className="text-red-500">Error rendering logs</div>;
}
return this.props.children;
}
}Then wrap the log rendering:
- {log.type === 'error-log' && (
- <span className="text-red-600">{log.message}</span>
- )}
- {log.type === 'log' && (
- <span className="text-white">{log.message}</span>
- )}
+ <LogErrorBoundary>
+ {log.type === 'error-log' ? (
+ <span className="text-red-600">{log.message}</span>
+ ) : (
+ <span className="text-white">{log.message}</span>
+ )}
+ </LogErrorBoundary>| if (msg.includes('stderr:')) { | ||
| console.log(LOG_TYPES.SERVER_LOG_ERROR, msg); | ||
| } else { | ||
| console.log(LOG_TYPES.SERVER_LOG, msg); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error logging with structured error information
The current error detection relies on string includes, which is fragile. Consider using a more structured approach for error detection and logging.
- if (msg.includes('stderr:')) {
- console.log(LOG_TYPES.SERVER_LOG_ERROR, msg);
- } else {
- console.log(LOG_TYPES.SERVER_LOG, msg);
- }
+ const logType = isErrorMessage(msg) ? LOG_TYPES.SERVER_LOG_ERROR : LOG_TYPES.SERVER_LOG;
+ const logData = {
+ type: logType,
+ timestamp: new Date().toISOString(),
+ message: msg,
+ pid: this.pid
+ };
+ console.log(logType, logData);Add helper function:
private isErrorMessage(msg: string): boolean {
return msg.includes('stderr:') ||
this.criticalMessageError.some(error => msg.includes(error));
}| @@ -93,12 +99,11 @@ | |||
| }); | |||
|
|
|||
| service.stderr?.on('data', (data: any) => { | |||
| console.log(LOG_TYPES.SERVER_LOG, 'stderr:', data.toString()); | |||
| this.loggerObserver.notify(data.toString()); | |||
| this.loggerObserver.notify(`stderr: ${data.toString()}`); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement consistent error handling pattern
The error handling is inconsistent between different event handlers. Standardize the approach across all handlers.
- service.stderr?.on('data', (data: any) => {
- this.loggerObserver.notify(`stderr: ${data.toString()}`);
- });
+ service.stderr?.on('data', this.handleStdErr.bind(this));
- service.on('disconnect', () => {
- this.loggerObserver.notify('Webserver disconnected')
+ service.on('disconnect', this.handleDisconnect.bind(this));
+ private handleStdErr(data: any): void {
+ const errorMessage = data.toString();
+ this.loggerObserver.notify(`stderr: ${errorMessage}`);
+ this.handleError(new Error(errorMessage), false);
+ }
+
+ private handleDisconnect(): void {
+ this.loggerObserver.notify('Webserver disconnected');
+ this.stateObserver.notify(false);
+ if (this.eventEmitter) {
+ this.eventEmitter.emit(EventLists.webServerStopped);
+ }
+ }Also applies to: 106-106
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
apps/server-web/src/renderer/pages/Server.tsx (4)
7-9: Consider enhancing the LogView component.The component could benefit from the following improvements:
- Define a proper interface for props
- Memoize the component to prevent unnecessary re-renders
+interface LogViewProps { + children: ReactNode; +} -const LogView = ({ children }: { children: ReactNode }) => { +const LogView = memo(({ children }: LogViewProps) => { return <div className="py-1">{children}</div>; -}; +}); + +LogView.displayName = 'LogView';
Line range hint
24-60: Add cleanup for event listener.The useEffect should return a cleanup function to remove the event listener.
useEffect(() => { window.electron.ipcRenderer.removeEventListener(IPC_TYPES.SERVER_PAGE); window.electron.ipcRenderer.on(IPC_TYPES.SERVER_PAGE, (arg: any) => { // ... existing code ... }); + return () => { + window.electron.ipcRenderer.removeEventListener(IPC_TYPES.SERVER_PAGE); + }; }, []);
73-77: Consider debouncing the scroll function.With rapid log updates, frequent scrolling could impact performance. Consider debouncing the scroll function.
+import { debounce } from 'lodash'; -const scrollToLast = () => { +const scrollToLast = debounce(() => { logRef.current?.scrollIntoView({ behavior: 'smooth', }); -}; +}, 100);
144-144: Improve scroll anchor implementation.The empty div for scrolling could be replaced with a more semantic approach.
-<div className="py-1" ref={logRef}></div> +<div ref={logRef} aria-hidden="true" className="h-px" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/server-web/src/main/helpers/services/libs/server-task.ts(5 hunks)apps/server-web/src/renderer/pages/Server.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/server-web/src/main/helpers/services/libs/server-task.ts
🔇 Additional comments (2)
apps/server-web/src/renderer/pages/Server.tsx (2)
12-22: LGTM: Well-structured state management.
The state management is well-organized with proper TypeScript types and clear purpose for each state variable.
134-142: Add error boundary for log rendering.
The previous review comment about adding an error boundary is still valid. This will help handle potential rendering failures gracefully.
| <LogView key={i}> | ||
| {log.type === 'error-log' ? ( | ||
| <span className="text-red-600">{log.message}</span> | ||
| ) : ( | ||
| <span className="text-white">{log.message}</span> | ||
| )} | ||
| </LogView> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using array index as key.
Using array index as key prop can lead to rendering issues when logs are added or removed.
Consider adding a timestamp or unique ID to each log entry:
-logs.map((log, i) => (
- <LogView key={i}>
+logs.map((log) => (
+ <LogView key={`${log.type}-${Date.now()}`}>Committable suggestion skipped: line range outside the PR's diff.
| setLogs((prev) => [ | ||
| ...prev, | ||
| { | ||
| message: arg.msg, | ||
| type: 'log', | ||
| }, | ||
| ]); | ||
| scrollToLast(); | ||
| break; | ||
| case LOG_TYPES.SERVER_LOG_ERROR: | ||
| setLogs((prev) => [ | ||
| ...prev, | ||
| { | ||
| message: arg.msg, | ||
| type: 'error-log', | ||
| }, | ||
| ]); | ||
| scrollToLast(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor log handling to reduce duplication.
The log handling logic for both normal and error logs is duplicated. Consider extracting it into a helper function.
+const addLog = (prev: typeof logs, message: string, type: 'log' | 'error-log') => [
+ ...prev,
+ { message, type }
+];
-setLogs((prev) => [
- ...prev,
- {
- message: arg.msg,
- type: 'log',
- },
-]);
+setLogs((prev) => addLog(prev, arg.msg, 'log'));Committable suggestion skipped: line range outside the PR's diff.
Description
Please include a summary of the changes and the related issue.
Type of Change
Checklist
Previous screenshots
Please add here videos or images of previous status
Current screenshots
Please add here videos or images of previous status
Summary by CodeRabbit
New Features
ServerPagecomponent to support structured log entries, improving the display of error and standard logs.LogViewcomponent for consistent log entry styling.Bug Fixes
Documentation