feat(ui): subscription set up for real-time logs #973
Conversation
WalkthroughThis pull request standardizes naming conventions by renaming properties (e.g., changing "timeStamp" to "timestamp") and updating LogLevel enumeration styles to camel case. It introduces a new dependency, Changes
Sequence Diagram(s)sequenceDiagram
participant JD as JobDetails Component
participant HL as useLogs Hook
participant WS as WebSocket Client
participant GS as GraphQL Server
JD->>HL: Invoke useLogs(jobId)
HL->>WS: Establish WebSocket connection (via graphql-ws)
WS->>GS: Subscribe using LOG_SUBSCRIPTION query
GS-->>WS: Send real-time log updates
WS-->>HL: Deliver log data
HL->>JD: Update live logs cache
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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
Documentation and Community
|
✅ Deploy Preview for reearth-flow ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![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
🧹 Nitpick comments (17)
ui/src/lib/i18n/locales/zh.json (1)
49-49: Translation Missing: Provide Chinese translation for "Node Id".
The new key"Node Id"has been added with an empty value. Consider providing an appropriate translation (e.g.,"节点ID") to ensure consistency and usability in the Chinese locale.ui/src/lib/i18n/locales/es.json (1)
49-49: Translation Suggestion: Localize "Node Id" in Spanish.
The key"Node Id"appears with an empty string. It is recommended to update it with a meaningful translation such as"ID del nodo", which will improve the localization quality.ui/src/lib/i18n/locales/fr.json (1)
49-49: Translation Suggestion: Provide French translation for "Node Id".
Currently, the"Node Id"key is assigned an empty string. Consider updating it to a meaningful translation (e.g.,"ID du nœud") to enhance the French localization.ui/src/lib/gql/convert.ts (1)
66-72: Property name inconsistency in toLog conversion function.The function renames
logLevelfrom the GraphQL fragment tostatusin the application's Log type. This naming inconsistency could cause confusion for developers working with logs across different parts of the application.Consider either:
- Using consistent naming throughout the application (either
logLevelorstatus)- Adding a comment explaining the reason for the name change
export const toLog = (log: LogFragment): Log => ({ nodeId: log.nodeId, jobId: log.jobId, timestamp: log.timestamp, - status: log.logLevel, + logLevel: log.logLevel, // Use consistent property naming message: log.message, });Or if
statusis the correct name in the Log type:export const toLog = (log: LogFragment): Log => ({ nodeId: log.nodeId, jobId: log.jobId, timestamp: log.timestamp, status: log.logLevel, // Converting from GraphQL's logLevel to application's status field message: log.message, });ui/src/lib/gql/subscriptions/index.ts (1)
19-19: Use standard header name for authorizationThe header uses British spelling "authorisation" instead of the standard "Authorization".
- authorisation: `Bearer ${token}`, + Authorization: `Bearer ${token}`,ui/src/components/LogsTable/index.tsx (1)
166-166: Use direct enum comparison for better type safety.Instead of comparing
getIsVisible()to produce a"default"or"outline"style, consider referencing a typed constant or using an inline conditional with the enum.ui/src/features/WorkspaceJobs/components/JobDetails.tsx (7)
31-33: Ensure consistent naming between “liveLogs” and “urlLogs”.The naming pattern is descriptive, but double-check that future maintainers will easily differentiate the usage contexts (live logs vs. logs from URL).
82-88: Refactor to guard repeated calls to getLogsFromUrl.Currently, if
selectedJob.logsURLchanges repeatedly (e.g., toggles between valid URLs), the callback could get triggered multiple times.
123-123: Consider a fallback UX when logs are unavailable.Setting
urlLogsmight fail or return an empty array. Provide some user-friendly feedback, if feasible, beyond just an empty component.
127-127: Automatically retry on transient network failures.Consider re-attempting the fetch if a network failure occurs, or let the user manually retry logs retrieval to improve resilience.
131-134: Avoid repeated calls to getLogsFromUrl if the job is no longer completed.When a user quickly changes jobs or the status changes from completed to something else, this effect might still trigger. A status check in the effect can prevent unnecessary fetches.
161-167: Remove unnecessary fragments.The static analysis hint indicates this fragment can be removed because it only contains one child.
- <> - {isFetchingLiveLogs ? ( - <LoadingSkeleton /> - ) : liveLogs && liveLogs.length > 0 ? ( - <LogsConsole data={liveLogs} /> - ) : null} - </> + {isFetchingLiveLogs ? ( + <LoadingSkeleton /> + ) : liveLogs && liveLogs.length > 0 ? ( + <LogsConsole data={liveLogs} /> + ) : null}🧰 Tools
🪛 Biome (1.9.4)
[error] 161-167: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
170-176: Remove unnecessary fragments.Similar to the above lines, the fragment only wraps a single conditional expression.
- <> - {isFetchingLogsUrl ? ( - <LoadingSkeleton /> - ) : urlLogs && urlLogs.length > 0 ? ( - <LogsConsole data={urlLogs} /> - ) : null} - </> + {isFetchingLogsUrl ? ( + <LoadingSkeleton /> + ) : urlLogs && urlLogs.length > 0 ? ( + <LogsConsole data={urlLogs} /> + ) : null}🧰 Tools
🪛 Biome (1.9.4)
[error] 170-176: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
ui/src/lib/gql/realTimeLogs/useSubscriptions.ts (4)
23-25: Validate enum usage for clarity.While an internal enum can be helpful, ensure that
LogSubscriptionKeysdoes not risk collisions with other keys in your React Query usage.
27-48: Refine initial query to handle fallback states.Currently, the
queryFnreturns an empty array or cached data. If logs are not present or if there's an error fetching them, consider returning an error state or providing a loading fallback.
98-156: Prevent duplicate logs efficiently.Using
processedLogIdsis a good approach. You could potentially store a more reliable unique ID (e.g., from the server side) instead of combiningmessageandstatusto minimize collisions.
158-167: Expose more log management functionality.The
clearLogsfunction is helpful. If there's a need to partially clear logs or set a max buffer size, you can expand this approach accordingly.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
ui/src/lib/gql/__gen__/gql.tsis excluded by!**/__gen__/**ui/src/lib/gql/__gen__/graphql.tsis excluded by!**/__gen__/**ui/src/lib/gql/__gen__/plugins/graphql-request.tsis excluded by!**/__gen__/**ui/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.lock
📒 Files selected for processing (21)
ui/package.json(1 hunks)ui/src/components/LogsTable/index.tsx(2 hunks)ui/src/features/Editor/components/BottomPanel/components/LogsConsole/index.tsx(2 hunks)ui/src/features/WorkspaceJobs/components/JobDetails.tsx(7 hunks)ui/src/hooks/useProjectExport.ts(1 hunks)ui/src/lib/gql/convert.ts(2 hunks)ui/src/lib/gql/fragment.graphql(1 hunks)ui/src/lib/gql/project/queries.graphql(1 hunks)ui/src/lib/gql/project/useApi.ts(1 hunks)ui/src/lib/gql/project/useQueries.ts(1 hunks)ui/src/lib/gql/realTimeLogs/useSubscriptions.ts(1 hunks)ui/src/lib/gql/subscriptions/index.ts(1 hunks)ui/src/lib/gql/subscriptions/queries.graphql(1 hunks)ui/src/lib/i18n/locales/en.json(1 hunks)ui/src/lib/i18n/locales/es.json(1 hunks)ui/src/lib/i18n/locales/fr.json(1 hunks)ui/src/lib/i18n/locales/ja.json(1 hunks)ui/src/lib/i18n/locales/zh.json(1 hunks)ui/src/mock_data/logsData.ts(1 hunks)ui/src/mock_data/logsv2Data.ts(1 hunks)ui/src/types/log.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
ui/src/features/WorkspaceJobs/components/JobDetails.tsx
[error] 161-167: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
[error] 170-176: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
🔇 Additional comments (36)
ui/package.json (1)
73-73: Dependency Addition: Validate "graphql-ws" integration.
The new dependency"graphql-ws": "6.0.4"has been added to support real-time log subscriptions. Please ensure this version is fully compatible with your GraphQL setup and that thorough testing has been performed for its integration.ui/src/lib/i18n/locales/ja.json (1)
49-49: Proper Translation: Japanese "Node Id" entry is correctly set.
The key"Node Id"is appropriately translated as"ノードID", ensuring consistency with the localization effort.ui/src/lib/i18n/locales/en.json (1)
49-49: New localization entry added for Node Id.This addition is consistent with the existing structure and provides localization support for a new field in the logging interface.
ui/src/lib/gql/fragment.graphql (1)
61-67: LGTM! New Log fragment added for logging data.The new fragment properly captures all the necessary fields for log entries (jobId, nodeId, timestamp, logLevel, message) and follows the same structure as other fragments in the file.
ui/src/lib/gql/subscriptions/queries.graphql (1)
1-9: LGTM! Well-structured GraphQL subscription for real-time logs.The subscription is correctly implemented to fetch logs for a specific job ID with all the necessary fields. This enables real-time log updates in the UI.
ui/src/lib/gql/convert.ts (2)
7-7: LGTM! Proper import of the new LogFragment type.The import is correctly added to support the new toLog conversion function.
10-10: LGTM! Import of Log type for conversion function.The Log type is properly imported to support the return type of the new toLog function.
ui/src/mock_data/logsv2Data.ts (1)
5-214: Consistent property naming convention implementedThe renaming of
timeStamptotimestampimproves code consistency across all log entries in the mock data. This change aligns with JavaScript/TypeScript naming conventions and ensures consistency with other parts of the codebase that use this property.ui/src/mock_data/logsData.ts (1)
6-74: Improved property naming and enum accessTwo positive changes have been made throughout this file:
- Renamed
timeStamptotimestampfor consistent naming conventions- Changed from bracket notation
LogLevel["INFO"]to dot notationLogLevel.Infofor better readabilityBoth changes enhance code consistency and readability.
ui/src/lib/gql/project/queries.graphql (1)
45-49:✅ Verification successful
Enhanced RunProject mutation response structure
The restructuring of the
RunProjectmutation response to include a nestedjobobject withid,status, andstartedAtproperties is a good improvement. This provides more comprehensive information about the job and better aligns with object-oriented design principles.Make sure that all consumers of this mutation have been updated to handle the new structure.
🏁 Script executed:
#!/bin/bash # Search for places where runProject mutation results are accessed rg -A 2 -B 2 "runProject\(" --glob "*.ts*"Length of output: 9125
Enhanced RunProject Mutation Response Verified
The updated mutation now correctly returns a nested
jobobject withid,status, andstartedAt. Our search confirmed that all GraphQL definitions and generated files (e.g., inui/src/lib/gql/__gen__/gql.tsandui/src/lib/gql/__gen__/plugins/graphql-request.ts) have been updated accordingly. Please double-check that any additional consumers of this mutation are adjusted to handle the new response structure.ui/src/hooks/useProjectExport.ts (1)
67-75: Consistent timestamp naming convention appliedThe variable rename from
timeStamptotimestampimproves naming consistency throughout the codebase. This change aligns with the property naming in the log data structures and other components.ui/src/lib/gql/project/useApi.ts (1)
126-126: Updated return structure to use the new job objectThe change correctly adapts to the modified GraphQL mutation response structure, using optional chaining to safely access the job ID and start time from the nested job object.
ui/src/lib/gql/project/useQueries.ts (1)
139-139: Updated return object to include the job objectThis change aligns with the modified GraphQL mutation response structure, properly returning the job object instead of individual fields.
ui/src/features/Editor/components/BottomPanel/components/LogsConsole/index.tsx (3)
18-18: Fixed timestamp property name for consistencyChanged from "timeStamp" to "timestamp" to ensure consistent naming across the codebase.
22-25: Added Node ID column to logs displayThe new column will provide better context by showing which node generated each log entry.
37-37: Updated valid logs check with the new property nameCorrectly updated the condition to use the renamed timestamp property.
ui/src/lib/gql/subscriptions/index.ts (1)
7-25: Well-implemented WebSocket client for GraphQL subscriptionsThe hook properly sets up a WebSocket client for GraphQL subscriptions with appropriate retry logic and authentication.
ui/src/components/LogsTable/index.tsx (7)
92-92: Ensure consistent usage of the renamed "timestamp" column.The column was previously referred to as "timeStamp" in other files. Verify that all references consistently use "timestamp" to avoid runtime errors.
100-100: Confirm that forcing the timestamp column to always be visible aligns with UX requirements.Toggling the timestamp column to
trueon reset might override a user's preference if they had previously hidden it.
132-132: Check enum usage against filter comparison.Confirm that
LogLevel.Errormaps to the string"ERROR". Otherwise,getStatusValue === "ERROR"may not trigger the intended filter.
139-139: Check enum usage against filter comparison.Confirm that
LogLevel.Warnmaps to the string"WARN". Otherwise,getStatusValue === "WARN"may not trigger the intended filter.
146-146: Check enum usage against filter comparison.Confirm that
LogLevel.Debugmaps to the string"DEBUG". Otherwise,getStatusValue === "DEBUG"may not align with the filter logic.
153-153: Check enum usage against filter comparison.Confirm that
LogLevel.Tracemaps to the string"TRACE". Otherwise, the UI may not reflect the correct filtering.
160-160: Check enum usage against filter comparison.Confirm that
LogLevel.Inforesolves to"INFO". Otherwise,getStatusValue === "INFO"might never match.ui/src/features/WorkspaceJobs/components/JobDetails.tsx (5)
8-8: Check for potential race conditions with subscription initialization.Using a custom hook in conjunction with existing state logic is generally fine, but ensure subsequent lines do not inadvertently re-subscribe or override data.
59-65: Validate the completed status check before fetching logs.The status condition checks if
selectedJob.status !== "completed"for live logs and=== "completed"for URL logs. Confirm that edge cases like canceled or failed statuses align with your desired log acquisition flow.
78-80: Verify correct GraphQL subscription usage.Make sure that
selectedJob?.id ?? ""is a valid fallback, especially if an empty string is an unintended job ID.
108-109: Confirm the log transformation keys are consistent with the finalLogtype.All fields (e.g.,
nodeId,message,timestamp,status) should match your domain model to avoid confusion or runtime errors.
136-137: Optimize data synchronization with new subscription data.If there is a race between the URL logs fetch and live logs subscription, ensure the final data merges seamlessly.
ui/src/lib/gql/realTimeLogs/useSubscriptions.ts (3)
1-10: Confirm that React Query and WebSocket usage are compatible.The import pattern looks fine. If you plan to add SSR or more advanced caching, confirm there are no conflicts with the WebSocket lifecycle.
11-21: Keep the GraphQL subscription query in sync with your schema.Changes in the schema might break real-time logs unless updated here.
50-96: Guard against memory leaks when unsubscribing.Unsubscribing in the cleanup is correct. Double-check if additional teardown is needed for the heartbeat interval or any lingering references in the event of a reload or if the jobId changes rapidly.
ui/src/types/log.ts (4)
4-8: Good standardization of LogLevel enum values.The LogLevel enum values are now uppercase strings (like "DEBUG"), which is a common convention for enum literals in logging systems. This change improves consistency with standard logging practices.
12-12: Nice type enhancement for nodeId.Making nodeId optional with multiple nullable types (string | null | undefined) provides better flexibility when handling logs with varying node information.
14-14: Good naming convention standardization.Changing from
timeStamptotimestampfollows better camelCase naming conventions used throughout the codebase. This improves consistency.
20-20: Consistent property naming.Changing from
Logstologsmaintains consistent property naming conventions across the codebase. Properties starting with lowercase is the standard JavaScript/TypeScript practice.
| import { config } from "@flow/config"; | ||
| import { useAuth } from "@flow/lib/auth"; | ||
|
|
||
| export const useWsClient = () => { |
There was a problem hiding this comment.
I think this should be setup as a provider like the other gql providers and wrap the app. If not, when we have multiple subscriptions on a single page we will be setting up multiple clients...
| const heartbeatInterval = setInterval(() => { | ||
| console.log( | ||
| "WebSocket connection check:", | ||
| connectionActive ? "active" : "inactive", | ||
| ); | ||
| }, 10000); |
There was a problem hiding this comment.
Before merging, should remove any code like this that was just for testing
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ui/src/lib/gql/provider/GraphQLSubscriptionProvider.tsx (2)
52-54: Consider type-safe approach for client disposalThe current implementation uses a type cast
(client as any)to access thedisposemethod. Consider checking the graphql-ws documentation to see if there's a more type-safe way to handle client disposal, or add a proper type definition.
33-56: Consider adding error handling for connection failuresWhile you've implemented retry logic, there's no visible error handling or user feedback for persistent WebSocket connection failures. Consider adding a state to track connection failures and provide appropriate UI feedback.
const [wsClient, setWsClient] = useState<Client | undefined>(); + const [connectionError, setConnectionError] = useState<Error | null>(null); const api = config().api; useEffect(() => { + setConnectionError(null); const client = createClient({ url: `${api}/api/graphql`, retryAttempts: 5, shouldRetry: () => true, + onNonLazyError: (error) => { + console.error('WebSocket connection error:', error); + setConnectionError(error); + }, connectionParams: () => { // ... }, });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ui/src/lib/gql/provider/GraphQLSubscriptionProvider.tsx(1 hunks)ui/src/lib/gql/provider/index.tsx(2 hunks)ui/src/lib/gql/realTimeLogs/useSubscriptions.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ui/src/lib/gql/realTimeLogs/useSubscriptions.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Collect information
🔇 Additional comments (5)
ui/src/lib/gql/provider/index.tsx (1)
2-2: Implementation of GraphQL subscriptions looks good!The integration of the GraphQLSubscriptionProvider within the existing provider structure follows best practices. The provider is properly nested, and the access token is correctly passed down from the parent component.
Also applies to: 17-19
ui/src/lib/gql/provider/GraphQLSubscriptionProvider.tsx (4)
13-21: Well-structured context and hook implementation.The context creation and hook implementation follow React best practices. The error handling in the hook ensures that it's only used within the appropriate provider context.
1-64: Overall architecture looks goodThe overall implementation of the GraphQL subscription provider using graphql-ws is well-structured. The component properly manages the WebSocket client lifecycle, and the context pattern allows for clean access to the client throughout the application.
41-41:❓ Verification inconclusive
Check header name for authorization
The header name is spelled as "authorisation" (British spelling) instead of the more commonly used "authorization" (American spelling). Verify that your backend expects this exact spelling, as this could cause authentication issues if there's a mismatch.
🏁 Script executed:
#!/bin/bash # Check if the backend code uses "authorisation" or "authorization" rg -i "authori[sz]ation" --type ts --type js --type go --type rsLength of output: 93
Verify the Authorization Header Spelling
The automated search did not return useful output because including the "rs" file type caused an error. Please re-run the search using only supported file types (TS, JS, Go) to confirm whether your backend expects the British spelling (
authorisation) or the American spelling (authorization). For example, you could run:#!/bin/bash # Check if the backend code uses "authorisation" or "authorization" in TypeScript, JavaScript, or Go files rg -i "authori[sz]ation" --type ts --type js --type goOnce you verify the exact spelling required by your backend, update the header accordingly to avoid authentication issues.
35-35:✅ Verification successful
Verify WebSocket endpoint URL
The WebSocket URL is constructed as
${api}/api/graphql, which assumes the WebSocket endpoint is at the same path as the regular GraphQL endpoint. Confirm that this is the correct endpoint for WebSocket connections, as they sometimes use different paths (like '/graphql/ws' or 'ws://domain/graphql').
🏁 Script executed:
#!/bin/bash # Look for WebSocket configuration in backend code rg -i "websocket|ws://" --type go --type js --type tsLength of output: 7444
WebSocket Endpoint URL Verified
Based on our review of the backend code (e.g., in
server/api/internal/app/graphql.go), GraphQL subscriptions are configured with a WebSocket transport usinggorilla/websocketon the same endpoint as the HTTP GraphQL endpoint. This indicates that constructing the URL as${api}/api/graphqlis likely correct for GraphQL subscriptions.
- The backend code adds the WebSocket transport on the standard GraphQL endpoint.
- No alternative WebSocket path (such as
/graphql/ws) is evident in the codebase.Please confirm that this endpoint aligns with your intended use and that separate endpoints (like those used for other services such as Yjs or Thrift) are not expected in this context.
Overview
What I've done
What I haven't done
How I tested
Screenshot
Which point I want you to review particularly
Memo
Summary by CodeRabbit
New Features
Refactor