Open
Conversation
Reviewer's GuideImplements sortable columns for the Tasks table with memoized, non-mutating sorting integrated with existing search, and updates Logs to consume the new dashboard task shape and provide richer, more resilient task log metadata and fallbacks. Sequence diagram for the updated task table sorting and searchsequenceDiagram
actor User
participant TasksPage
User->>TasksPage: Type in searchTerm
activate TasksPage
TasksPage->>TasksPage: setSearchTerm(newTerm)
deactivate TasksPage
loop Polling interval
TasksPage->>TasksPage: usePolling(taskService.listTasks)
TasksPage-->>TasksPage: tasksData updated
end
Note over TasksPage: useEffect runs when tasksData, searchTerm, sortColumn, sortDirection change
TasksPage->>TasksPage: filter healthyTasks
alt searchTerm present
TasksPage->>TasksPage: tasksToDisplay = healthyTasks.filter(matches searchTerm)
else no searchTerm
TasksPage->>TasksPage: tasksToDisplay = healthyTasks
end
TasksPage->>TasksPage: sortTasks(tasksToDisplay)
TasksPage-->>TasksPage: sortedTasks
TasksPage->>TasksPage: setFilteredTasks(sortedTasks)
User->>TasksPage: Click column header
activate TasksPage
TasksPage->>TasksPage: handleSort(column)
alt column equals sortColumn
TasksPage->>TasksPage: toggle sortDirection
else new column
TasksPage->>TasksPage: setSortColumn(column)
TasksPage->>TasksPage: setSortDirection(asc)
end
deactivate TasksPage
Note over TasksPage: State change triggers useEffect to reapply search and memoized sortTasks
Sequence diagram for loading logs from updated dashboard taskssequenceDiagram
actor User
participant LogsPage
participant TaskService
participant LogService
User->>LogsPage: Open logs page
activate LogsPage
LogsPage->>TaskService: getDashboardData()
TaskService-->>LogsPage: dashboardData (with tasks[])
LogsPage->>LogsPage: console.log(Tasks found)
alt dashboardData.tasks is array
LogsPage->>LogsPage: for task in tasks.slice(-20)
loop For each task
LogsPage->>LogsPage: taskId = task.task_id or task.id
LogsPage->>LogService: getTaskLogs(taskId, task.tes_url)
LogService-->>LogsPage: logResponse
alt logResponse.success is true
LogsPage->>LogsPage: content = logResponse.log or success default
else logResponse.log exists
LogsPage->>LogsPage: content = logResponse.log
else
LogsPage->>LogsPage: content = fallback with task name, status or state, tes_name, submitted_at or creation_time
end
LogsPage->>LogsPage: build TaskLogEntry with id, title, content, timestamp, metadata
end
else tasks not array
LogsPage->>LogsPage: no task logs loaded
end
LogsPage-->>User: Render logs list with enriched metadata
deactivate LogsPage
Updated class diagram for dashboard tasks and task logsclassDiagram
class TaskService {
+getDashboardData()
+listTasks()
}
class LogService {
+getTaskLogs(taskId, tesUrl)
}
class DashboardData {
+tasks: DashboardTask[]
}
class DashboardTask {
+id: string
+task_id: string
+name: string
+task_name: string
+status: string
+state: string
+tes_name: string
+tes_url: string
+submitted_at: string
+creation_time: string
+type: string
}
class TaskLogEntry {
+id: string
+type: string
+title: string
+content: string
+timestamp: string
+metadata_status: string
+metadata_tesInstance: string
}
TaskService --> DashboardData
DashboardData --> DashboardTask
LogsPage ..> TaskService
LogsPage ..> LogService
LogsPage ..> TaskLogEntry
class LogsPage {
+loadDashboardTasks()
+buildTaskLogEntry(task, logResponse)
}
class TasksPage {
+searchTerm: string
+filteredTasks: DashboardTask[]
+sortColumn: string
+sortDirection: string
+handleSort(column)
+getSortIndicator(column)
+sortTasks(tasks)
}
TasksPage ..> TaskService
TasksPage ..> DashboardTask
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
handleSort, consider using functional state updates (e.g.,setSortDirection(prev => prev === 'asc' ? 'desc' : 'asc')) to avoid potential issues with stalesortDirectionwhen multiple state updates are batched. - The
useEffectthat updatesfilteredTasksdepends on bothsortTasksandsortColumn/sortDirection, even thoughsortTasksalready depends on those; you can simplify the dependency array (or inline the sort logic) to avoid redundant dependencies. - In the logs error-handling branch, the pushed log object contains
tesInstanceboth insidemetadataand as a top-level field; if the extra top-leveltesInstanceisn't needed, removing it will keep the log shape consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `handleSort`, consider using functional state updates (e.g., `setSortDirection(prev => prev === 'asc' ? 'desc' : 'asc')`) to avoid potential issues with stale `sortDirection` when multiple state updates are batched.
- The `useEffect` that updates `filteredTasks` depends on both `sortTasks` and `sortColumn`/`sortDirection`, even though `sortTasks` already depends on those; you can simplify the dependency array (or inline the sort logic) to avoid redundant dependencies.
- In the logs error-handling branch, the pushed log object contains `tesInstance` both inside `metadata` and as a top-level field; if the extra top-level `tesInstance` isn't needed, removing it will keep the log shape consistent.
## Individual Comments
### Comment 1
<location> `frontend/src/pages/Logs.js:313` </location>
<code_context>
if (logResponse && logResponse.success) {
content = logResponse.log || 'Log endpoint returned success but no log content';
} else if (logResponse && logResponse.log) {
- content = logResponse.log;
} else {
</code_context>
<issue_to_address>
**issue (bug_risk):** The `else if (logResponse && logResponse.log)` branch is now a no-op, so valid log content is ignored.
Previously this branch assigned `content = logResponse.log;`. Now it does nothing, so when `logResponse.success` is false but `logResponse.log` exists, `content` incorrectly remains the fallback string. Consider restoring the assignment:
```js
} else if (logResponse && logResponse.log) {
content = logResponse.log;
} else {
content = `Task Log for ${taskId}...`;
}
```
</issue_to_address>
### Comment 2
<location> `frontend/src/pages/Logs.js:326` </location>
<code_context>
metadata: {
- status: task.status,
+ status: task.status || task.state,
tesInstance: task.tes_name || 'Unknown'
}
});
</code_context>
<issue_to_address>
**suggestion:** Duplicated `tesInstance` field inside and outside `metadata` may cause confusion or inconsistent access patterns.
In the error case you’re now adding both `metadata.tesInstance` and a top-level `tesInstance`, while the success path only uses `metadata.tesInstance`. This inconsistency can confuse consumers of `allLogs`. Consider keeping `tesInstance` only in `metadata` and removing the top-level property here.
Suggested implementation:
```javascript
const taskId = task.task_id || task.id;
allLogs.push({
id: taskId,
type: 'task',
```
```javascript
id: taskId,
type: 'task',
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR enhances the task management overview with sortable columns and updates the logs view so that newly run demo tasks (from the main task list/dashboard) are correctly surfaced in the Logs page with richer metadata.
Changes:
- Adds client-side sorting for Tasks by ID, Name, Status, TES Instance, and Created columns (defaulting to newest-first by creation time) while integrating with the existing search filter.
- Updates the Logs page to pull recent tasks from
dashboardData.tasksinstead ofsubmitted_tasks, handling bothtask_idandidshapes and enriching fallback log content with more task details. - Adjusts log construction and console diagnostics to better reflect the new data sources and task representations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
frontend/src/pages/Tasks.js |
Introduces sortable table headers and a memoized sortTasks function to provide ascending/descending sorting across all task columns while preserving search and health filtering. |
frontend/src/pages/Logs.js |
Switches the Logs view to use dashboardData.tasks, supports both task_id and id, fetches more recent tasks, and enriches log entries and fallbacks for both successful and failed log fetches. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if (logResponse && logResponse.success) { | ||
| content = logResponse.log || 'Log endpoint returned success but no log content'; | ||
| } else if (logResponse && logResponse.log) { |
There was a problem hiding this comment.
The else if (logResponse && logResponse.log) branch is now empty, so when getTaskLogs returns a response where success is false but log is present, the log content is silently discarded and the fallback summary is used instead. This regresses the previous behavior (where content was set from logResponse.log in this case) and can hide useful log details; consider assigning content from logResponse.log in this branch.
Suggested change
| } else if (logResponse && logResponse.log) { | |
| } else if (logResponse && logResponse.log) { | |
| content = logResponse.log; |
frontend/src/pages/Logs.js
Outdated
Comment on lines
+341
to
+342
| }, | ||
| tesInstance: task.tes_name || 'Unknown' |
There was a problem hiding this comment.
In the error path for loading task logs, the pushed log object includes a top-level tesInstance field in addition to metadata.tesInstance, whereas the rest of the code (and filterLogs) only uses metadata.tesInstance. This redundant field makes the log object shape inconsistent across code paths and can cause confusion for future maintainers; consider removing the extra top-level tesInstance or standardizing on a single location.
Suggested change
| }, | |
| tesInstance: task.tes_name || 'Unknown' | |
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Only reviewed descriptions and UI screenshots: looks good to me. If all Copilot suggestions have been addressed this should be good to merge. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue #13 (SOLVED)
Used useCallback to memoize sorting function
Proper React hooks dependencies
Non-mutating sort (creates new array)
Handles edge cases (null values, missing fields)
Works seamlessly with existing search functionality
issue #14 (SOLVED)
Implementation Screenshot:
Summary by Sourcery
Add sortable task table columns and improve task log loading for the dashboard.
New Features:
Enhancements: