Conversation
Reviewer's GuideSets the ELIXIR-FI TES instance as the default and prioritizes it in the dropdown, removes demo and submission alert pop-ups in favor of direct navigation/inline behavior, and normalizes connection test alert messages while updating TES instance configuration. Sequence diagram for default TES instance selection in SubmitTasksequenceDiagram
actor User
participant SubmitTaskComponent
participant useInstancesHook
User->>SubmitTaskComponent: Open SubmitTask page
SubmitTaskComponent->>useInstancesHook: useInstances()
useInstancesHook-->>SubmitTaskComponent: instances list
rect rgb(230,230,250)
SubmitTaskComponent->>SubmitTaskComponent: useEffect on instances change
SubmitTaskComponent->>SubmitTaskComponent: Check instances.length > 0 and no formData.tes_instance
alt ELIXIR_FI instance exists
SubmitTaskComponent->>SubmitTaskComponent: Find instance with url containing csc-tesk-noauth.rahtiapp.fi
SubmitTaskComponent->>SubmitTaskComponent: setFormData tes_instance to ELIXIR_FI url
else No ELIXIR_FI instance
SubmitTaskComponent->>SubmitTaskComponent: setFormData tes_instance to instances[0].url
end
end
User->>SubmitTaskComponent: Open TES Instance dropdown
SubmitTaskComponent->>SubmitTaskComponent: Sort instances to prioritize ELIXIR_FI at top
SubmitTaskComponent-->>User: Display dropdown with ELIXIR_FI preselected and first in list
Sequence diagram for updated demo run and task submission without alertssequenceDiagram
actor User
participant SubmitTaskComponent
participant BackendTESService
participant Router
User->>SubmitTaskComponent: Click Run Demo
SubmitTaskComponent->>SubmitTaskComponent: getDemoTaskData demoType
SubmitTaskComponent->>SubmitTaskComponent: setFormData with demoData
SubmitTaskComponent->>SubmitTaskComponent: setError null
SubmitTaskComponent-->>User: Demo form fields populated (no alert)
User->>SubmitTaskComponent: Click Submit Task
SubmitTaskComponent->>BackendTESService: POST submit task with formData
BackendTESService-->>SubmitTaskComponent: result with message
SubmitTaskComponent->>Router: navigate to tasks list
Router-->>User: Tasks page displayed (no success alert popup)
File-Level Changes
Possibly linked issues
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 3 issues, and left some high level feedback:
- In the TES instance , calling .sort directly on instances mutates the original array from state/hooks; create a shallow copy first (e.g. [...instances].sort(...)) to avoid side effects. The ELIXIR-Fi instance URL string is hard-coded in multiple places; consider extracting it into a shared constant to avoid duplication and potential mismatches. Instead of leaving the old alert and task name logic commented out in handleRunDemo and handleSubmit, remove the dead code to keep the component easier to read and maintain. Prompt for AI Agents Please address the comments from this code review: ## Overall Comments - In the TES instance <Select>, calling `.sort` directly on `instances` mutates the original array from state/hooks; create a shallow copy first (e.g. `[...instances].sort(...)`) to avoid side effects. - The ELIXIR-Fi instance URL string is hard-coded in multiple places; consider extracting it into a shared constant to avoid duplication and potential mismatches. - Instead of leaving the old alert and task name logic commented out in `handleRunDemo` and `handleSubmit`, remove the dead code to keep the component easier to read and maintain. ## Individual Comments ### Comment 1 <location> `frontend/src/pages/SubmitTask.js:206-215` </location> <code_context> + // 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') + ); + + if (elixirFiInstance) { + setFormData(prev => ({ ...prev, tes_instance: elixirFiInstance.url })); + } else { + setFormData(prev => ({ ...prev, tes_instance: instances[0].url })); + } + } + }, [instances]); + const handleTestConnection = async () => { </code_context> <issue_to_address> **issue (bug_risk):** Effect uses `formData.tes_instance` but only depends on `instances`, which can lead to stale reads and hook lint warnings. Since the effect reads `formData.tes_instance` but only depends on `instances`, it may both trigger `react-hooks/exhaustive-deps` warnings and miss updates if `tes_instance` changes independently of `instances`. Either include the relevant `formData` field (or a derived `hasTesInstance` flag) in the dependency array, or restructure so the choice of `tes_instance` is made where `formData` is initialized / passed in, avoiding reading it directly inside this effect. </issue_to_address> ### Comment 2 <location> `frontend/src/pages/SubmitTask.js:460-469` </location> <code_context> - {instance.name} - </option> - ))} + {instances + .sort((a, b) => { + // Prioritize ELIXIR-Fi instance at the top + const aIsElixirFi = a.url && a.url.includes('csc-tesk-noauth.rahtiapp.fi'); + const bIsElixirFi = b.url && b.url.includes('csc-tesk-noauth.rahtiapp.fi'); + if (aIsElixirFi && !bIsElixirFi) return -1; + if (!aIsElixirFi && bIsElixirFi) return 1; + return 0; + }) + .map((instance, index) => ( + <option key={index} value={instance.url}> + {instance.name} </code_context> <issue_to_address> **issue (bug_risk):** Sorting `instances` directly mutates the array, which can lead to subtle bugs when `instances` comes from state or context. Since `.sort()` mutates the original array, this can violate immutability expectations if `instances` comes from React state or context, leading to hard-to-track bugs. Create a copy first, e.g. `instances.slice().sort(...)` or `[...instances].sort(...)`, before mapping to options. </issue_to_address> ### Comment 3 <location> `frontend/src/pages/SubmitTask.js:208-209` </location> <code_context> + + useEffect(() => { + if (instances.length > 0 && !formData.tes_instance) { + const elixirFiInstance = instances.find( + instance => instance.url && instance.url.includes('csc-tesk-noauth.rahtiapp.fi') + ); + </code_context> <issue_to_address> **suggestion:** The ELIXIR-Fi instance URL substring is hardcoded in multiple places and could be centralized. This substring is also used in the select sorting logic; to avoid divergence and clarify intent, extract it into a shared constant (e.g. `ELIXIR_FI_INSTANCE_SUBSTRING`) and reuse it in both the `find` and sort comparator. Suggested implementation: ```javascript import { useNavigate } from 'react-router-dom'; import styled from 'styled-components'; import { testConnection } from '../services/api'; const ELIXIR_FI_INSTANCE_SUBSTRING = 'csc-tesk-noauth.rahtiapp.fi'; refresh: refreshInstances } = useInstances(); ``` ```javascript const elixirFiInstance = instances.find( instance => instance.url && instance.url.includes(ELIXIR_FI_INSTANCE_SUBSTRING) ); ``` Also update the select sorting logic in this file where the same substring `'csc-tesk-noauth.rahtiapp.fi'` is used. Replace that hardcoded string with the shared `ELIXIR_FI_INSTANCE_SUBSTRING` constant so that both the `find` and the comparator rely on the same centralized value. </issue_to_address> Sourcery is free for open source - if you like our reviews please consider sharing them ✨ X Mastodon LinkedIn Facebook 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 addresses three UX issues related to the task submission form: setting a default healthy TES instance for demo tasks (Issue #8), removing redundant pop-ups when loading demo task data (Issue #9), and removing redundant pop-ups on task submission (Issue #10).
Changes:
- Added default TES instance selection that prioritizes the Finland (ELIXIR-FI) instance on component mount
- Removed alert pop-ups when demo task buttons are clicked
- Removed success alert pop-ups after task submission
- Sorted TES instance dropdown to show Finland instance first
- Added status annotations to instance names in the .tes_instances configuration file
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| frontend/src/pages/SubmitTask.js | Added useEffect hook to set default instance, removed demo and submission alerts, sorted instance dropdown |
| backend/.tes_instances | Added status annotations (Working/Not Working/etc.) to instance names |
| frontend/src/pages/Utilities.js | Removed emoji characters from connection test messages |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
frontend/src/pages/SubmitTask.js
Outdated
| {instances | ||
| .sort((a, b) => { | ||
| // Prioritize ELIXIR-Fi instance at the top | ||
| const aIsElixirFi = a.url && a.url.includes('csc-tesk-noauth.rahtiapp.fi'); | ||
| const bIsElixirFi = b.url && b.url.includes('csc-tesk-noauth.rahtiapp.fi'); | ||
| if (aIsElixirFi && !bIsElixirFi) return -1; | ||
| if (!aIsElixirFi && bIsElixirFi) return 1; | ||
| return 0; | ||
| }) |
There was a problem hiding this comment.
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.
backend/.tes_instances
Outdated
| Funnel/OpenPBS @ ELIXIR-CZ (Not Working),https://funnel.cloud.e-infra.cz/ | ||
| Funnel/Slurm @ ELIXIR-FI (DNS resolution Failure),https://vm4816.kaj.pouta.csc.fi/ | ||
| TESK/Kubernetes @ ELIXIR-CZ (Prod) (Auth Failure),https://tesk-prod.cloud.e-infra.cz/ | ||
| TESK/Kubernetes @ ELIXIR-CZ (NA) (403 Forbidden),https://tesk-na.cloud.e-infra.cz/ | ||
| TESK/Kubernetes @ ELIXIR-DE (DNS resolution Failure),https://tesk.elixir-cloud.bi.denbi.de/ | ||
| TESK/Kubernetes @ ELIXIR-GR (Working),https://tesk-eu.hypatia-comp.athenarc.gr/ | ||
| TESK/OpenShift @ ELIXIR-FI (Working),https://csc-tesk-noauth.rahtiapp.fi/ |
There was a problem hiding this comment.
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.
| Funnel/OpenPBS @ ELIXIR-CZ (Not Working),https://funnel.cloud.e-infra.cz/ | |
| Funnel/Slurm @ ELIXIR-FI (DNS resolution Failure),https://vm4816.kaj.pouta.csc.fi/ | |
| TESK/Kubernetes @ ELIXIR-CZ (Prod) (Auth Failure),https://tesk-prod.cloud.e-infra.cz/ | |
| TESK/Kubernetes @ ELIXIR-CZ (NA) (403 Forbidden),https://tesk-na.cloud.e-infra.cz/ | |
| TESK/Kubernetes @ ELIXIR-DE (DNS resolution Failure),https://tesk.elixir-cloud.bi.denbi.de/ | |
| TESK/Kubernetes @ ELIXIR-GR (Working),https://tesk-eu.hypatia-comp.athenarc.gr/ | |
| TESK/OpenShift @ ELIXIR-FI (Working),https://csc-tesk-noauth.rahtiapp.fi/ | |
| Funnel/OpenPBS @ ELIXIR-CZ,https://funnel.cloud.e-infra.cz/ | |
| Funnel/Slurm @ ELIXIR-FI,https://vm4816.kaj.pouta.csc.fi/ | |
| TESK/Kubernetes @ ELIXIR-CZ (Prod),https://tesk-prod.cloud.e-infra.cz/ | |
| 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/ |
frontend/src/pages/SubmitTask.js
Outdated
| setFormData(prev => ({ ...prev, tes_instance: instances[0].url })); | ||
| } | ||
| } | ||
| }, [instances]); |
There was a problem hiding this comment.
The useEffect hook is missing formData.tes_instance in its dependency array. React's exhaustive-deps rule requires all values used inside the effect that come from the component scope to be listed in the dependency array. Without this, the linter will warn about a missing dependency. Add formData.tes_instance to the dependency array.
| }, [instances]); | |
| }, [instances, formData.tes_instance]); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .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; | ||
| }) |
There was a problem hiding this comment.
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.
| 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]); | ||
|
|
There was a problem hiding this comment.
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]); |
| @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 |
There was a problem hiding this comment.
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.
| {(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; | ||
| }) | ||
| .map((instance, index) => ( | ||
| <option | ||
| key={index} | ||
| value={instance.url} | ||
| > | ||
| {getStatusBadge(instance.status)} {instance.name} | ||
| {instance.status === 'unhealthy' ? ' (Unhealthy)' : ''} | ||
| {instance.status === 'unreachable' ? ' (Unreachable)' : ''} | ||
| </option> | ||
| ))} |
There was a problem hiding this comment.
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
| const initialState = instanceService.getHealthyInstances(); | ||
| setState(initialState); | ||
| const allInstancesState = instanceService.getAllInstancesWithStatus(); | ||
| setState({ | ||
| ...initialState, | ||
| allInstances: allInstancesState.instances | ||
| }); |
There was a problem hiding this comment.
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.
uniqueg
left a comment
uniqueg
left a comment
There was a problem hiding this comment.
Please address the one issue I raised, as well as anything that Sourcery and GitHub Copilot complain about.
Also, from your screenshot, I think it would be better to just use emojis in the drop-down to indicate health: ✅ for healthy, ❌ for broken. No details. You can indicate details (403, 500 etc.) in the instance management / health overview, but in the instance selection in the drop-down, I think it's best to keep it simple.
Rest looks fine (as far as I can tell).
frontend/src/pages/SubmitTask.js
Outdated
| import useInstances from '../hooks/useInstances'; | ||
| import { ArrowLeft, Play, Zap, RefreshCw } from 'lucide-react'; | ||
|
|
||
| const ELIXIR_FI_INSTANCE_SUBSTRING = 'csc-tesk-noauth.rahtiapp.fi'; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
frontend/src/pages/SubmitTask.js
Outdated
| // Sort by status: healthy first, then others | ||
| if (a.status === 'healthy' && b.status !== 'healthy') return -1; | ||
| if (a.status !== 'healthy' && b.status === 'healthy') return 1; |
There was a problem hiding this comment.
The dropdown sorting only separates healthy instances from unhealthy ones but doesn't prioritize the Finland instance as requested in issue #8. According to the issue, the Finland instance (TESK/OpenShift @ ELIXIR-FI) should be the preferred default. Consider adding an additional sort criterion to place the Finland instance first among healthy instances. For example, you could check if the instance URL contains 'csc-tesk-noauth.rahtiapp.fi' and sort it to the top.
| // Sort by status: healthy first, then others | |
| if (a.status === 'healthy' && b.status !== 'healthy') return -1; | |
| if (a.status !== 'healthy' && b.status === 'healthy') return 1; | |
| const finlandHost = 'csc-tesk-noauth.rahtiapp.fi'; | |
| const aHealthy = a.status === 'healthy'; | |
| const bHealthy = b.status === 'healthy'; | |
| const aFinlandHealthy = aHealthy && (a.url || '').includes(finlandHost); | |
| const bFinlandHealthy = bHealthy && (b.url || '').includes(finlandHost); | |
| // Prefer the Finland instance among healthy ones | |
| if (aFinlandHealthy && !bFinlandHealthy) return -1; | |
| if (!aFinlandHealthy && bFinlandHealthy) return 1; | |
| // Then sort by status: healthy first, then others | |
| if (aHealthy && !bHealthy) return -1; | |
| if (!aHealthy && bHealthy) return 1; |
There was a problem hiding this comment.
We don't need to prioritize the "Finland instance", just any healthy, reachable instance without authorization. If those three criteria are met the instance is suitable for the examples to run.
(Are these criteria met? If I recall correctly, there is a healthy instance we don't have access rights to, so submitting there will fail. Is this taken into consideration? @uniqueg is authorization status something we would like the TES dashboard to check in advance or is that too much overhead?)
frontend/src/pages/SubmitTask.js
Outdated
| }) | ||
| .map((instance, index) => ( | ||
| <option | ||
| key={index} |
There was a problem hiding this comment.
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.
| key={index} | |
| key={instance.url} |
frontend/src/pages/SubmitTask.js
Outdated
| setFormData(prev => ({ ...prev, tes_instance: healthyInstance.url })); | ||
| } | ||
| } | ||
| }, [instances, allInstances, formData.tes_instance]); |
There was a problem hiding this comment.
The useEffect has a dependency on formData.tes_instance which causes it to re-run every time the instance is selected. This creates a problematic behavior: if a user manually selects a different instance, the effect will not reset it back because the condition !formData.tes_instance will be false. However, this dependency is unnecessary and could be removed since we only want to auto-select on initial load when no instance is selected. Consider removing formData.tes_instance from the dependency array.
| }, [instances, allInstances, formData.tes_instance]); | |
| }, [instances, allInstances]); |
| // 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' | ||
| ); |
There was a problem hiding this comment.
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.
| // 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' | |
| ); |
There was a problem hiding this comment.
see my comment above about not prioritizing Finland per se.
|
Some copilot raised issues to address still. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # If tasks endpoint returns 401/403, mark as unhealthy (auth required but not configured) | ||
| if tasks_response.status_code in [401, 403]: | ||
| print(f"⚠️ {instance.get('name')} tasks endpoint requires authentication (status {tasks_response.status_code})") | ||
| status = "unhealthy" | ||
| else: | ||
| # Tasks endpoint is accessible (200) or returns other non-auth error | ||
| status = "healthy" | ||
| except Exception as tasks_error: |
There was a problem hiding this comment.
In fetch_tes_status, the instance is marked healthy for any tasks endpoint response that isn’t 401/403. This can incorrectly classify instances as healthy even when /tasks returns 404/500/405, which would still break demo submissions. Consider only marking healthy for 2xx (ideally 200) responses, and treating other status codes as unhealthy (while still keeping the special-case for 401/403).
| status = "unhealthy" | ||
| else: | ||
| # Service-info is accessible, but we need to check if tasks endpoint is usable | ||
| # Try a HEAD/OPTIONS request to tasks endpoint to see if it requires auth |
There was a problem hiding this comment.
The comment says "Try a HEAD/OPTIONS request" to the tasks endpoint, but the code uses requests.get(...). Either update the comment to match the implementation or switch to HEAD/OPTIONS if that’s the intended (lower-impact) check.
| # Try a HEAD/OPTIONS request to tasks endpoint to see if it requires auth | |
| # Try a request to the tasks endpoint to see if it requires auth |
|
|
||
| tes_endpoint = working_endpoint | ||
| print(f"🚀 Submitting task to {tes_endpoint}") | ||
| print(f"📦 Task payload: {json.dumps(tes_task, indent=2)}") |
There was a problem hiding this comment.
print(f"📦 Task payload: ...") logs the full TES task payload. Since this includes user-provided fields (command, URLs, descriptions), it can leak data and become very noisy in production. Consider using logger.debug (or guarding behind a debug flag) and/or redacting fields before logging.
| print(f"📦 Task payload: {json.dumps(tes_task, indent=2)}") | |
| logger.debug("📦 Task payload keys: %s", list(tes_task.keys())) |
| print(f"❌ TES returned status {response.status_code}") | ||
| print(f"Response headers: {dict(response.headers)}") | ||
| print(f"Response body: {response.text[:500]}") | ||
|
|
||
| try: | ||
| error_data = response.json() | ||
| print(f"Error data JSON: {error_data}") |
There was a problem hiding this comment.
On submission errors, the code prints full TES response headers and body (Response headers, Response body). Headers/bodies can include sensitive information and can be large; this is better handled via logger.debug with redaction/truncation (and ideally only when debugging).
| print(f"❌ TES returned status {response.status_code}") | |
| print(f"Response headers: {dict(response.headers)}") | |
| print(f"Response body: {response.text[:500]}") | |
| try: | |
| error_data = response.json() | |
| print(f"Error data JSON: {error_data}") | |
| logger.debug("TES submission failed with status %s", response.status_code) | |
| logger.debug( | |
| "TES response header names: %s", | |
| list(response.headers.keys()) | |
| ) | |
| logger.debug( | |
| "TES response body (truncated): %s", | |
| (response.text[:200] + "..." if response.text and len(response.text) > 200 else response.text) | |
| ) | |
| try: | |
| error_data = response.json() | |
| logger.debug( | |
| "TES error data JSON (truncated): %s", | |
| str(error_data)[:500] + ("..." if len(str(error_data)) > 500 else "") | |
| ) |
Issue bug: set TES Instance for Demo task to healthy instance #8 (SOLVED)
Solution - now the Finland instance is set as a default and i have created get_instances_with_status() function which helps in providing all TES instances with their current health status which is to be shown in the dropdown
issue ue: remove demo task pop-ups upon form population #9 (SOLVED)
Solution - I have removed the Alert now the details will directly display in the form without any alert functionality
issue ue: remove demo task pop-ups upon submission #10 (SOLVED)
Solution - same as for issue ue: remove demo task pop-ups upon form population #9, i have removed alert.
Implementation Screenshot:
Summary by Sourcery
Set a healthy TES instance as the default for demo task submissions and streamline the demo and submission UX by removing alert pop-ups and clarifying connection test messaging.
New Features:
Bug Fixes:
Enhancements: