Add job result download option to WebUI #4702
Conversation
WalkthroughThe changes in this pull request introduce enhancements to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant JobTabs
participant JobExecutions
participant DownloadButton
User->>JobTabs: View Job Executions
JobTabs->>JobExecutions: Check job.Tasks and job.State
alt If tasks and state exist
JobTabs->>JobExecutions: Render JobExecutions
JobExecutions->>DownloadButton: Render DownloadButton
DownloadButton->>User: User clicks to download
DownloadButton->>JobExecutions: Handle download logic
else
JobTabs->>User: Display no job data
end
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: 1
🧹 Outside diff range and nitpick comments (3)
webui/components/jobs/details/JobInformation.tsx (1)
47-51: Consider handling multiple tasks with different publishers.The current implementation passes all tasks to
JobTaskPublisherDisplaybut only checks the first task's publisher type in the condition. Consider:
- Handling cases where tasks might have different publisher types
- Showing multiple publisher displays if needed
Example approach:
-{job.ID && job.Tasks?.[0]?.Publisher?.Type && ( - <InfoItem label="Publisher"> - <JobTaskPublisherDisplay - jobId={job.ID} - tasks={job.Tasks} - state={job.State} - /> - </InfoItem> -)} +{job.ID && job.Tasks?.some(task => task.Publisher?.Type) && ( + <InfoItem label="Publishers"> + {job.Tasks.map((task, index) => + task.Publisher?.Type && ( + <JobTaskPublisherDisplay + key={`${job.ID}-${index}`} + jobId={job.ID} + tasks={[task]} + state={job.State} + /> + ) + )} + </InfoItem> +)}webui/components/jobs/JobTaskPublisher.tsx (2)
22-62: Consider extracting button styles for better maintainability.The button styling is duplicated across different states. Consider extracting common styles into a shared class or using a base button component.
Example refactor:
+ const baseButtonClass = "p-2"; + const enabledClass = "text-black transition-colors duration-150 hover:text-blue-500 active:text-blue-700"; + const disabledClass = "text-gray-300"; + const loadingClass = "text-black"; return ( <button - className="p-2 text-black" + className={`${baseButtonClass} ${loadingClass}`} disabled> <Loader size={18} strokeWidth={2} className="animate-spin" /> </button> ); // ... similar changes for other button states
109-161: Consider these improvements for better maintainability.
- Extract magic strings and arrays into named constants
- Add error state handling in UI
- Consider memoizing more values to prevent unnecessary re-renders
Here's a suggested implementation:
+ const SUPPORTED_PUBLISHERS = ['local', 's3'] as const; + const COMPLETED_JOB_STATES = [ + models_JobStateType.JobStateTypeCompleted, + models_JobStateType.JobStateTypeFailed, + models_JobStateType.JobStateTypeStopped, + ] as const; const JobTaskPublisherDisplay: React.FC<JobTaskPublisherDisplayProps> = ({ jobId, tasks, state }) => { const { isLoading, error, execute, } = useApiOperation<apimodels_ListJobResultsResponse>() + const publisherType = useMemo(() => + tasks[0]?.Publisher?.Type ?? "UNKNOWN", + [tasks] + ); // ... rest of the code ... const isTaskCompleted = jobState ? [ - models_JobStateType.JobStateTypeCompleted, - models_JobStateType.JobStateTypeFailed, - models_JobStateType.JobStateTypeStopped, - ].includes(jobState) + COMPLETED_JOB_STATES.includes(jobState) : false; - const isDownloadable = isSupportedPublisher && isTaskCompleted; + const isDownloadable = SUPPORTED_PUBLISHERS.includes(publisherType) && isTaskCompleted; return ( <div className="flex items-center space-x-2"> {publisherType} + {error && ( + <div className="text-red-500 text-sm"> + Failed to download results. Please try again. + </div> + )} <DownloadButton isDownloadable={isDownloadable} isLoading={isLoading} disabledReason={disabledReason} onDownload={onDownload} /> </div> ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
webui/components/jobs/JobTaskPublisher.tsx(1 hunks)webui/components/jobs/details/JobInformation.tsx(2 hunks)
🧰 Additional context used
🪛 Biome
webui/components/jobs/JobTaskPublisher.tsx
[error] 103-103: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
🔇 Additional comments (4)
webui/components/jobs/details/JobInformation.tsx (2)
7-7: LGTM! Import statement follows conventions.
The import statement is well-structured and follows React/TypeScript naming conventions.
45-45: Verify the assumption about first task's publisher.
The condition job.Tasks?.[0]?.Publisher?.Type only checks the first task's publisher. Please verify if this is the intended behavior or if we need to consider publishers from all tasks.
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Using first task's publisher is the intended behavior
The codebase consistently uses the first task's publisher throughout the implementation:
JobTaskPublisher.tsxexplicitly usestasks[0]?.Publisher?.Type- The job submission analytics in
pkg/analytics/job_submit.gotracks a single publisher type per job - The executor in
pkg/compute/executor.gouses a single publisher for the entire job - The job ranking features in
pkg/orchestrator/selection/ranking/features.gouse a single publisher type per job
This indicates that the current implementation is designed to work with a single publisher per job, and using job.Tasks?.[0]?.Publisher?.Type is the correct approach.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are jobs with multiple tasks having different publisher types
# Search for job task definitions to understand the task structure
rg -A 10 "type\s+(\w+)?Task\s+struct"
# Search for publisher-related code to understand how multiple publishers are handled
rg -A 5 "Publisher.*Type"
Length of output: 32069
webui/components/jobs/JobTaskPublisher.tsx (2)
1-20: LGTM! Well-structured interfaces and imports.
The interfaces are well-defined with appropriate types, and all necessary dependencies are properly imported.
1-161: Overall implementation looks good and meets PR objectives.
The component successfully implements the job result download functionality for both local and S3 publishers, with appropriate state handling and user feedback. The suggestions above will help improve maintainability and security, but the core functionality is solid.
🧰 Tools
🪛 Biome
[error] 103-103: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
webui/components/jobs/JobTaskPublisher.tsx (1)
29-73: Consider using a more maintainable CSS organization.While the component is well-implemented, consider using Tailwind's
clsxorcnutility to better organize the conditional classes:+ const buttonStyles = { + base: "p-2", + enabled: "text-black transition-colors duration-150 hover:text-blue-500 active:text-blue-700", + disabled: "text-gray-300", + loading: "text-black" + }; - const enabledClass = "p-2 text-black transition-colors duration-150 hover:text-blue-500 active:text-blue-700"; - const disabledClass = "p-2 text-gray-300"; - const loadingClass = "p-2 text-black"; + const getButtonClass = (variant: 'enabled' | 'disabled' | 'loading') => + clsx(buttonStyles.base, buttonStyles[variant]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
webui/components/jobs/JobTaskPublisher.tsx(1 hunks)
🔇 Additional comments (2)
webui/components/jobs/JobTaskPublisher.tsx (2)
1-28: LGTM! Well-structured type definitions and constants.
The interfaces and constants are well-defined with proper typing, making the code type-safe and maintainable.
1-197: Overall implementation successfully meets PR objectives.
The component effectively implements the job result download functionality with:
- Support for local and S3 publisher types
- Proper button state management based on job status
- Good error handling and user feedback
- Accessibility features
While there are some suggested improvements, the core functionality is solid and meets the requirements.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
webui/components/jobs/JobTaskPublisher.tsx (1)
30-32: Consider using a CSS module or Tailwind classes object for better organization.The button state classes could be better organized to improve maintainability.
Consider refactoring to:
const buttonStyles = { enabled: "p-2 text-black transition-colors duration-150 hover:text-blue-500 active:text-blue-700", disabled: "p-2 text-gray-300", loading: "p-2 text-black" } as const;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
webui/components/jobs/JobTaskPublisher.tsx(1 hunks)
🔇 Additional comments (2)
webui/components/jobs/JobTaskPublisher.tsx (2)
1-27: LGTM! Well-structured type definitions and constants.
The imports, constants, and interface definitions are clear and appropriate for the component's functionality.
1-197: Overall implementation looks good with room for improvements.
The component successfully implements the job result download functionality with appropriate security measures and user feedback. Consider implementing the suggested improvements for better reliability and maintainability.
There was a problem hiding this comment.
Outstanding work. Thanks for landing this in a short time!
I've tested the changes and the only issue I found is when a job have multiple results, such as the case when running multiple executions. We always end up downloading the result of the first execution. I don't suggest we aggregate anything in this case, but just display all the results to the user and let them decide what to download.
Ways to re-produce this:
# run devstack with three nodes and webui enabled
export PREDICTABLE_API_PORT=1;
bacalhau devstack
# in an another terminal, submit a job with count=3, which will ask for 3 executions
bacalhau docker run --publisher local --count 3 hello-world
# downloading from the webui will only download the first result
Here is the API output for reference. Don't worry about pagination for now as it doesn't actually work for this API and we just return all results, I think!
→ curl 127.0.0.1:1234/api/v1/orchestrator/jobs/j-05a1e060-8a4c-41b8-a23e-b0b210cfdf8d/results | jq
{
"NextToken": "",
"Items": [
{
"Type": "urlDownload",
"Params": {
"URL": "http://127.0.0.1:51837/e-29f22fb1-bf5e-4f36-9391-19715b9d0ab1.tar.gz"
}
},
{
"Type": "urlDownload",
"Params": {
"URL": "http://127.0.0.1:51841/e-b85ff14d-ec5c-4954-9f22-112e32648a67.tar.gz"
}
},
{
"Type": "urlDownload",
"Params": {
"URL": "http://127.0.0.1:51844/e-eef8a55b-b8a8-4558-971d-b7d7f45792c0.tar.gz"
}
}
]
}
Suggestions:
- Current the current download button, and show a popup with list of results to download iff there are multiple results
- Have a separate tab for results and show list of available results
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
webui/components/jobs/details/JobTabs.tsx (2)
44-50: LGTM! Consider enhancing type safety.The conditional rendering is a good practice to prevent runtime errors. However, we could make the type checking more explicit.
Consider this improvement:
- {job.Tasks && job.State && ( + {Array.isArray(job.Tasks) && job.State?.status && ( <JobExecutions executions={executions} tasks={job.Tasks} state={job.State} /> )}
44-50: Consider adding a placeholder for better UX.When the required data is not available, users might benefit from seeing a message explaining why the executions tab is empty.
Consider adding an else clause:
{job.Tasks && job.State && ( <JobExecutions executions={executions} tasks={job.Tasks} state={job.State} /> - )} + )} + {(!job.Tasks || !job.State) && ( + <div className="text-center py-4 text-gray-500"> + Execution details are not available for this job + </div> + )}webui/components/jobs/details/JobExecutions.tsx (1)
193-201: OptimizeonDownloadfunction withuseCallbackWrapping
onDownloadwithuseCallbackcan prevent unnecessary re-creations of the function on each render, improving performance.Apply this change:
- const onDownload = async (publishedResult: models_SpecConfig | undefined) => { + const onDownload = useCallback(async (publishedResult: models_SpecConfig | undefined) => { // function body - }; + }, [publisherType]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
webui/components/jobs/details/JobExecutions.tsx(2 hunks)webui/components/jobs/details/JobInformation.tsx(1 hunks)webui/components/jobs/details/JobTabs.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- webui/components/jobs/details/JobInformation.tsx
Changes
This change allows WebUI users to download job results for local and s3 publisher types.
Testing
Pushed long running jobs to EC2 fleet and verified that download button is only enabled if publisher is supported and job is not running. S3 and local jobs download successfully.
Summary by CodeRabbit
New Features
Bug Fixes