-
Notifications
You must be signed in to change notification settings - Fork 0
feat: resolving issue #8 #9 and #10 #18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
1d22447
993e9b1
a789ebd
cfccbae
a63c052
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,4 +1,4 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import React, { useState } from 'react'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import React, { useState, useEffect } from 'react'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { useNavigate } from 'react-router-dom'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import styled from 'styled-components'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { testConnection } from '../services/api'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -201,6 +201,22 @@ const SubmitTask = () => { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| refresh: refreshInstances | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } = useInstances(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Issue #8: bug: set TES Instance for Demo task to healthy instance | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| useEffect(() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (instances.length > 0 && !formData.tes_instance) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const elixirFiInstance = instances.find( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| instance => instance.url && instance.url.includes('csc-tesk-noauth.rahtiapp.fi') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
sourcery-ai[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (elixirFiInstance) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setFormData(prev => ({ ...prev, tes_instance: elixirFiInstance.url })); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setFormData(prev => ({ ...prev, tes_instance: instances[0].url })); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
sourcery-ai[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, [instances]); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, [instances]); | |
| }, [instances, formData.tes_instance]); |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The useEffect hook has a problematic dependency array that includes 'formData.tes_instance'. This can cause unnecessary re-renders or potential infinite loops because the effect modifies the same state it depends on. When the effect sets formData.tes_instance, it triggers the effect again (though the condition '!formData.tes_instance' prevents the actual loop).
A better approach is to only depend on 'instances' and check the condition more carefully, or to remove formData.tes_instance from the dependency array since the effect only runs when it's empty anyway.
| if (instances.length > 0 && !formData.tes_instance) { | |
| const elixirFiInstance = instances.find( | |
| instance => instance.url && instance.url.includes(ELIXIR_FI_INSTANCE_SUBSTRING) | |
| ); | |
| if (elixirFiInstance) { | |
| setFormData(prev => ({ ...prev, tes_instance: elixirFiInstance.url })); | |
| } else { | |
| setFormData(prev => ({ ...prev, tes_instance: instances[0].url })); | |
| } | |
| } | |
| }, [instances, formData.tes_instance]); | |
| if (instances.length === 0) { | |
| return; | |
| } | |
| setFormData(prev => { | |
| // If tes_instance is already set, do not override it | |
| if (prev.tes_instance) { | |
| return prev; | |
| } | |
| const elixirFiInstance = instances.find( | |
| instance => instance.url && instance.url.includes(ELIXIR_FI_INSTANCE_SUBSTRING) | |
| ); | |
| if (elixirFiInstance) { | |
| return { ...prev, tes_instance: elixirFiInstance.url }; | |
| } | |
| return { ...prev, tes_instance: instances[0].url }; | |
| }); | |
| }, [instances]); |
Outdated
Copilot
AI
Jan 25, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The inline sort operation only affects the rendering of the dropdown options and does not mutate the original instances array. This means that when demo tasks are loaded via handleRunDemo (lines 303-316), the getDemoTaskData function will use instances[0].url which refers to the first instance in the unsorted array, not necessarily the Finland instance. This defeats the purpose of Issue #8, which requires demo tasks to use the Finland instance. Consider storing a reference to the Finland instance in a separate variable or refactoring getDemoTaskData to use the same instance-finding logic.
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sorting function checks if 'a.url' and 'b.url' exist before calling '.includes()', which prevents errors. However, if an instance has no URL, it will be treated as non-ELIXIR-FI. Consider whether instances without URLs should be sorted differently (e.g., to the end of the list) or filtered out entirely, as they likely cannot be used for task submission anyway.
sourcery-ai[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dropdown now displays unhealthy and unreachable instances alongside healthy ones. While users can see the status badges and labels, they can still select these non-functional instances. This could lead to task submission failures. Consider either:
- Disabling (making unselectable) unhealthy/unreachable instances in the dropdown
- Adding a warning when a user selects an unhealthy instance
- Only showing unhealthy instances as informational but preventing their selection for task submission
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding status annotations like "(Not Working)", "(DNS resolution Failure)", and "(Auth Failure)" directly to instance names in the configuration file is problematic. These status annotations become part of the instance name that's displayed throughout the application, which is misleading because instance health status is dynamic and should be determined at runtime, not hardcoded in configuration. Instance health is already checked dynamically by the backend health check system. Remove these status annotations and let the system determine instance health programmatically.