Skip to content

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.

Sign up for GitHub

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

Open
keshxvdayal wants to merge 5 commits into
base: main
Choose a base branch
from
Open

feat: resolving issue #8 #9 and #10 #18

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1d22447
feat: resolving issue #8 #9 and #10
keshxvdayal Jan 25, 2026
993e9b1
feat: suggested changes
keshxvdayal Jan 25, 2026
a789ebd
feat: removing hard coded tes instance status
keshxvdayal Jan 25, 2026
cfccbae
feat: simplified status badges and removed the hardcoded dependencies
keshxvdayal Jan 27, 2026
a63c052
feat: suggested changes by copilot are implemented
keshxvdayal Jan 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion backend/.tes_instances
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ TESK/Kubernetes @ ELIXIR-CZ (NA),https://tesk-na.cloud.e-infra.cz/
TESK/Kubernetes @ ELIXIR-DE,https://tesk.elixir-cloud.bi.denbi.de/
TESK/Kubernetes @ ELIXIR-GR,https://tesk-eu.hypatia-comp.athenarc.gr/
TESK/OpenShift @ ELIXIR-FI,https://csc-tesk-noauth.rahtiapp.fi/
TESK North America,https://tesk-na.cloud.e-infra.cz/

# Local Development
Local TES,http://localhost:8080
21 changes: 21 additions & 0 deletions backend/routes/instances.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,27 @@ def get_healthy_instances_route():
print(f"❌ Error in get_healthy_instances: {str(e)}")
return jsonify({'error': str(e)}), 500

@instances_bp.route('/api/instances-with-status', methods=['GET'])
def get_instances_with_status():
"""Get all TES instances with their current health status"""
try:
from utils.tes_utils import load_tes_location_data
instances = load_tes_location_data()

# Fetch status for all instances in parallel
with ThreadPoolExecutor(max_workers=8) as pool:
results = list(pool.map(fetch_tes_status, instances))

return jsonify({
'instances': results,
'count': len(results),
'last_updated': datetime.now(timezone.utc).isoformat()
})

except Exception as e:
print(f"Error in get_instances_with_status: {str(e)}")
return jsonify({'error': str(e)}), 500
Comment on lines +31 to +50
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new '/api/instances-with-status' endpoint is not included in the cache patterns in middleware_config.py. This endpoint performs real-time health checks on all TES instances which can be expensive (lines 38-40 show parallel HTTP requests to multiple endpoints). The endpoint should be added to the cache_patterns list to avoid repeated expensive operations, similar to '/api/instances' and '/api/tes_locations' which are already cached.

Copilot uses AI. Check for mistakes.

@instances_bp.route('/api/tes_locations', methods=['GET'])
def tes_locations():
from utils.tes_utils import load_tes_location_data
Expand Down
8 changes: 7 additions & 1 deletion frontend/src/hooks/useInstances.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import instanceService from '../services/instanceService';
const useInstances = () => {
const [state, setState] = useState({
instances: [],
allInstances: [],
loading: true,
error: null,
lastUpdate: null
Expand All @@ -15,7 +16,11 @@ const useInstances = () => {
};
instanceService.addListener(handleUpdate);
const initialState = instanceService.getHealthyInstances();
setState(initialState);
const allInstancesState = instanceService.getAllInstancesWithStatus();
setState({
...initialState,
allInstances: allInstancesState.instances
});
Comment on lines 17 to +23
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a potential state synchronization issue in the useEffect hook. The code gets initialState from getHealthyInstances() and allInstancesState from getAllInstancesWithStatus() separately, then merges them manually. However, the listener added at line 17 receives updates that already include both 'instances' and 'allInstances' (as seen in instanceService.notifyListeners() which passes both fields). This manual merging could be out of sync with the listener updates.

A cleaner approach would be to initialize the state directly from the service's cached data without the manual merge, since the notifyListeners already provides the complete state structure with both fields.

Copilot uses AI. Check for mistakes.
return () => {
instanceService.removeListener(handleUpdate);
};
Expand All @@ -26,6 +31,7 @@ const useInstances = () => {

return {
instances: state.instances,
allInstances: state.allInstances,
loading: state.loading,
error: state.error,
lastUpdate: state.lastUpdate,
Expand Down
85 changes: 61 additions & 24 deletions frontend/src/pages/SubmitTask.js
View file
Open in desktop
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';
Expand All @@ -8,6 +8,8 @@ import ErrorMessage from '../components/common/ErrorMessage';
import useInstances from '../hooks/useInstances';
import { ArrowLeft, Play, Zap, RefreshCw } from 'lucide-react';

const ELIXIR_FI_INSTANCE_SUBSTRING = 'csc-tesk-noauth.rahtiapp.fi';
Copy link
Copy Markdown
Member

@uniqueg uniqueg Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't hardcore this. Just set it to any healthy instance. That way, we don't need to change anything if the instance goes down.

If there isn't a single healthy instance, don't select a default instance. Users should then check the drop down list themselves and realize that there is currently no healthy instance, prompting them to manage the instances and configure a healthy one.


const PageContainer = styled.div`
padding: 20px;
background-color: #f8f9fa;
Expand Down Expand Up @@ -196,11 +198,40 @@ const SubmitTask = () => {

const {
instances,
allInstances,
loading: instancesLoading,
error: instancesError,
refresh: refreshInstances
} = useInstances();

// Helper function to get status badge
const getStatusBadge = (status) => {
switch(status) {
case 'healthy':
return '✓';
case 'unhealthy':
return '✗';
case 'unreachable':
return '⚠';
default:
return '?';
}
};

useEffect(() => {
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]);

Comment on lines +212 to +225
Copy link

Copilot AI Jan 26, 2026

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.

Suggested change
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]);

Copilot uses AI. Check for mistakes.
const handleTestConnection = async () => {
try {
setTestingConnection(true);
Expand Down Expand Up @@ -238,9 +269,14 @@ const SubmitTask = () => {
};

const getDemoTaskData = (demoType = 'basic') => {
const defaultTesInstance = instances.length > 0
? instances[0].url
: 'https://csc-tesk-noauth.rahtiapp.fi/v1/tasks';
let defaultTesInstance = 'https://csc-tesk-noauth.rahtiapp.fi/v1/tasks';

if (instances.length > 0) {
const elixirFiInstance = instances.find(
instance => instance.url && instance.url.includes(ELIXIR_FI_INSTANCE_SUBSTRING)
);
Comment on lines +263 to +269
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue #8 requested that the Finland instance (https://csc-tesk-noauth.rahtiapp.fi/) should be prioritized as the default for demo tasks. However, the current implementation only finds the first healthy instance from the list without specifically prioritizing Finland. Since the instances come from the backend in the order defined in .tes_instances, and Finland is listed 7th (after CZ, CZ, CZ, DE, GR instances), a different instance will be selected if it's healthy first. Consider either: 1) Moving the Finland instance to the top of .tes_instances, or 2) Adding logic to specifically prefer instances with 'csc-tesk-noauth.rahtiapp.fi' in the URL when selecting the default.

Suggested change
// Use first healthy instance as default for demos
let defaultTesInstance = '';
if (instances.length > 0) {
const healthyInstance = (allInstances.length > 0 ? allInstances : instances).find(
instance => instance.status === 'healthy'
);
// Prefer Finland instance for demos, fallback to first healthy instance
let defaultTesInstance = '';
if (instances.length > 0) {
const candidateInstances = (allInstances.length > 0 ? allInstances : instances);
const preferredHost = 'csc-tesk-noauth.rahtiapp.fi';
// First, try to find a healthy Finland instance
const preferredHealthyInstance = candidateInstances.find(
instance =>
instance.status === 'healthy' &&
typeof instance.url === 'string' &&
instance.url.includes(preferredHost)
);
// If none found, fall back to the first healthy instance
const healthyInstance = preferredHealthyInstance || candidateInstances.find(
instance => instance.status === 'healthy'
);

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown

@ninsch3000 ninsch3000 Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see my comment above about not prioritizing Finland per se.

defaultTesInstance = elixirFiInstance ? elixirFiInstance.url : instances[0].url;
}

const demoTasks = {
basic: {
Expand Down Expand Up @@ -287,15 +323,7 @@ const SubmitTask = () => {
const handleRunDemo = (demoType = 'basic') => {
const demoData = getDemoTaskData(demoType);
setFormData(demoData);
setError(null);

const taskNames = {
basic: 'Basic Hello World',
python: 'Python Script',
fileops: 'File Operations'
};

alert(`${taskNames[demoType] || 'Demo'} task data loaded! Review the form and click "Submit Task" when ready.`);
setError(null);
};

const handleSubmit = async (e) => {
Expand Down Expand Up @@ -329,12 +357,7 @@ const SubmitTask = () => {
const result = await taskService.submitTask(submitData);

console.log('Task submission result:', result);

if (result && result.message) {
alert(`Success: ${result.message}`);
} else {
alert('Task submitted successfully!');
}

navigate('/tasks');
} catch (err) {
console.error('Task submission error:', err);
Expand Down Expand Up @@ -440,11 +463,25 @@ const SubmitTask = () => {
required
>
<option value="">Select TES Instance</option>
{instances.map((instance, index) => (
<option key={index} value={instance.url}>
{instance.name}
</option>
))}
{(allInstances.length > 0 ? allInstances : instances)
.slice()
.sort((a, b) => {
const aIsElixirFi = a.url && a.url.includes(ELIXIR_FI_INSTANCE_SUBSTRING);
const bIsElixirFi = b.url && b.url.includes(ELIXIR_FI_INSTANCE_SUBSTRING);
if (aIsElixirFi && !bIsElixirFi) return -1;
if (!aIsElixirFi && bIsElixirFi) return 1;
return 0;
})
Comment on lines +461 to +467
Copy link

Copilot AI Jan 26, 2026

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.

Copilot uses AI. Check for mistakes.
.map((instance, index) => (
<option
key={index}
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using array index as the key prop is an anti-pattern in React when the list can be reordered (which happens here due to sorting). This can cause React to incorrectly reuse elements and lead to bugs with component state. Since instances have unique URLs, consider using instance.url as the key instead of index.

Suggested change
key={index}
key={instance.url}

Copilot uses AI. Check for mistakes.
value={instance.url}
>
{getStatusBadge(instance.status)} {instance.name}
{instance.status === 'unhealthy' ? ' (Unhealthy)' : ''}
{instance.status === 'unreachable' ? ' (Unreachable)' : ''}
</option>
))}
Comment on lines +459 to +475
Copy link

Copilot AI Jan 26, 2026

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:

  1. Disabling (making unselectable) unhealthy/unreachable instances in the dropdown
  2. Adding a warning when a user selects an unhealthy instance
  3. Only showing unhealthy instances as informational but preventing their selection for task submission

Copilot uses AI. Check for mistakes.
</Select>
</FormGroup>

Expand Down
4 changes: 2 additions & 2 deletions frontend/src/pages/Utilities.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,7 +406,7 @@ const Utilities = () => {
));

const serviceInfo = response.data;
let successMessage = `✅ Connection Test Successful!\n\nInstance: ${instanceName}\nResponse Time: ${responseTime}ms`;
let successMessage = `Connection Test Successful!\n\nInstance: ${instanceName}\nResponse Time: ${responseTime}ms`;

if (serviceInfo && serviceInfo.name) {
successMessage += `\nService Name: ${serviceInfo.name}`;
Expand All @@ -417,7 +417,7 @@ const Utilities = () => {

alert(successMessage);
} catch (error) {
let errorMessage = `❌ Connection Test Failed\n\nInstance: ${instanceName}\nURL: ${url}\n\n`;
let errorMessage = `Connection Test Failed\n\nInstance: ${instanceName}\nURL: ${url}\n\n`;
let errorReason = '';
let errorCode = '';
let errorType = '';
Expand Down
38 changes: 26 additions & 12 deletions frontend/src/services/instanceService.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import api from './api';
class InstanceService {
constructor() {
this.healthyInstances = [];
this.allInstancesWithStatus = [];
this.loading = false;
this.error = null;
this.lastUpdate = null;
Expand All @@ -24,6 +25,7 @@ class InstanceService {
try {
callback({
instances: this.healthyInstances,
allInstances: this.allInstancesWithStatus,
loading: this.loading,
error: this.error,
lastUpdate: this.lastUpdate
Expand All @@ -43,31 +45,47 @@ class InstanceService {
};
}

getAllInstancesWithStatus() {
return {
instances: this.allInstancesWithStatus,
loading: this.loading,
error: this.error,
lastUpdate: this.lastUpdate
};
}

async fetchHealthyInstances() {
try {
const isInitialLoad = this.healthyInstances.length === 0;

if (isInitialLoad) {
this.loading = true;
this.notifyListeners();
console.log('🔄 Loading initial healthy TES instances...');
console.log('🔄 Loading TES instances with status...');
} else {
console.log('🔄 Refreshing cached healthy TES instances...');
console.log('🔄 Refreshing TES instances with status...');
}

const response = await api.get('/api/healthy-instances', {
timeout: 5000
const response = await api.get('/api/instances-with-status', {
timeout: 10000
});

const data = response.data;
this.healthyInstances = data.instances || [];
const allInstances = data.instances || [];

// Filter to only healthy instances, but keep status info
this.healthyInstances = allInstances.filter(inst => inst.status === 'healthy');

// Store all instances (including unhealthy) for dropdown display
this.allInstancesWithStatus = allInstances;

this.lastUpdate = data.last_updated ? new Date(data.last_updated) : new Date();
this.error = null;

console.log(`✅ Got ${this.healthyInstances.length} cached healthy TES instances (updated: ${this.lastUpdate.toLocaleTimeString()})`);
console.log(`✅ Got ${this.healthyInstances.length} healthy instances out of ${allInstances.length} total (updated: ${this.lastUpdate.toLocaleTimeString()})`);

} catch (err) {
console.error('Error fetching healthy instances:', err);
console.error('Error fetching instances with status:', err);

if (err.code === 'ECONNABORTED') {
this.error = 'Connection timeout - using cached data';
Expand All @@ -84,11 +102,7 @@ class InstanceService {
console.log(`Using ${this.healthyInstances.length} cached healthy instances during error`);
}
} finally {
if (this.healthyInstances.length === 0) {
this.loading = false;
} else {
this.loading = false;
}
this.loading = false;
this.notifyListeners();
}
}
Expand Down
Loading