fix: fix selecting services on Deployment Logs tab#2181
fix: fix selecting services on Deployment Logs tab#2181jzsfkzm wants to merge 1 commit intoakash-network:mainfrom
Conversation
WalkthroughThe DeploymentLogs component's websocket connection and loading state management has been refactored to prevent runtime exceptions and stuck loading indicators when services are deselected. Changes include introducing an Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Component as DeploymentLogs
participant WebSocket as WebSocket Connection
User->>Component: Select services
Component->>Component: Check guards (services selected, not connecting)
Component->>Component: Set isConnecting = true
Component->>WebSocket: Initiate connection
WebSocket-->>Component: Log message received
Component->>Component: Set isConnecting = false
Component->>Component: Append logs
User->>Component: Deselect all services
Component->>Component: Set isLoadingLogs = false
Component->>Component: Reset isConnectionEstablished = false
Component->>Component: Reset isConnecting = false
Component->>WebSocket: Close connection
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
jzsfkzm
force-pushed
the
bugfixes/2105-multiple-services-logs
branch
from
0729304 to
7d9ded9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/deploy-web/src/components/deployments/DeploymentLogs.tsx (1)
41-41: Define proper TypeScript types instead ofany.The use of
anytype forlogs.current(line 41) andparsedLog(line 184) violates the coding guideline that states: "Never use type any or cast to type any. Always define the proper TypeScript types."As per coding guidelines.
Define proper interfaces for the log structure:
interface LogEntry { service: string; message: string; name?: string; type?: string; reason?: string; object?: { name?: string; kind?: string; }; note?: string; }Then update the declarations:
- // TODO Type - const logs = useRef<any[]>([]); + const logs = useRef<LogEntry[]>([]);- // TODO Type - let parsedLog: any = null; + let parsedLog: LogEntry | null = null;Also applies to: 184-184
🧹 Nitpick comments (1)
apps/deploy-web/src/components/deployments/DeploymentLogs.tsx (1)
80-85: Avoid casting toanyfor Monaco editor events.The type assertion to
any(line 82) violates the coding guideline. Consider using a type guard or properly typing the Monaco editor event structure.As per coding guidelines.
Replace the type assertion with a type guard:
editor.onDidScrollChange(event => { - // TODO Verify - if (event.scrollTop < (event as any)._oldScrollTop) { + const eventWithOldScroll = event as { scrollTop: number; _oldScrollTop?: number }; + if (eventWithOldScroll._oldScrollTop !== undefined && event.scrollTop < eventWithOldScroll._oldScrollTop) { setStickToBottom(false); } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/deploy-web/src/components/deployments/DeploymentLogs.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/deploy-web/src/components/deployments/DeploymentLogs.tsx
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/deploy-web/src/components/deployments/DeploymentLogs.tsx
🧠 Learnings (1)
📓 Common learnings
Learnt from: jzsfkzm
Repo: akash-network/console PR: 1994
File: apps/deploy-web/src/hooks/useListSelection/useListSelection.spec.ts:58-64
Timestamp: 2025-10-17T10:04:00.940Z
Learning: In the Akash Console deployment list multi-select feature (PR #1994, apps/deploy-web/src/hooks/useListSelection), the requirement is for additive Shift+click behavior: subsequent Shift+clicks should expand/add to the existing selection rather than replacing the previous Shift range. This differs from canonical Gmail/Finder-style replacement behavior.
🧬 Code graph analysis (1)
apps/deploy-web/src/components/deployments/DeploymentLogs.tsx (1)
apps/deploy-web/src/hooks/useProviderWebsocket.ts (2)
useProviderWebsocket(18-67)sendJsonMessage(47-65)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test-build
🔇 Additional comments (6)
apps/deploy-web/src/components/deployments/DeploymentLogs.tsx (6)
36-37: LGTM! State initialization supports the fix.The initial
falsevalues for bothisLoadingLogsandisConnectingproperly reflect the initial state before any websocket connection is established, preventing the stuck loading indicator issue.
63-63: LGTM! Proper websocket cleanup capability.Extracting
getWebSocketenables manual connection closure when services change, which is essential for the fix.
124-136: LGTM! Guards prevent the runtime exception.The guard at line 130 (
selectedServices.length === 0) directly fixes issue #2105 by preventing websocket connection attempts when no services are selected. TheisConnectingcheck at line 131 also prevents concurrent connection attempts.
157-168: Verify dependency completeness.The effect uses
providerInfo(line 126) but doesn't include it in the dependency array. WhileproviderInfois derived fromselectedLease(which is included), it also depends onprovidersfrom a query hook. This could cause stale closures if provider data changes without the lease changing.Consider adding
providerInfoto the dependency array:}, [ providerCredentials, selectedLogsMode, selectedLease, selectedServices, services.length, updateLogText, canSetConnection, isConnectionEstablished, isConnecting, - sendJsonMessage + sendJsonMessage, + providerInfo ]);
170-175: LGTM! Proper connection state and empty message handling.Setting
isConnectingto false when the first message arrives correctly transitions the connection state. The empty message guard prevents unnecessary processing and state updates.
230-239: LGTM! Proper state and resource cleanup.The handler correctly stops the loading indicator (line 234), resets connection states (lines 235-236), and closes the websocket (line 238). This ensures the loading bar doesn't get stuck when services are changed or unselected.
Note: There's a potential race if
onLogReceivedfires during cleanup and setsisConnectionEstablished(true)at line 203 after the reset. This is minor since the connection is being closed, but consider whetheronLogReceivedshould check if the connection is still active.
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2181 +/- ##
==========================================
- Coverage 46.75% 46.39% -0.37%
==========================================
Files 1021 1011 -10
Lines 28999 28665 -334
Branches 7521 7477 -44
==========================================
- Hits 13559 13299 -260
+ Misses 15069 14998 -71
+ Partials 371 368 -3
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
@stalniy I just checked it and now it works as expected:
There's however one JS error in the logs when you open the select due to a prop deprecated in MUI5, I just created a PR for that: |
2 participants
closes #2105
Summary by CodeRabbit